Re: [PATCH] media: i2c: adv7343: fix the DT binding properties

On 09/19/2013 06:06 PM, Prabhakar Lad wrote:

On Mon, Sep 16, 2013 at 9:54 PM, Stephen Warren[off-list ref]  wrote:

quoted

On 09/13/2013 11:23 PM, Prabhakar Lad wrote:

quoted

On Sat, Sep 14, 2013 at 4:16 AM, Stephen Warren[off-list ref]  wrote:

quoted

On 09/13/2013 05:57 AM, Prabhakar Lad wrote:

quoted

From: "Lad, Prabhakar"<redacted>

This patch fixes the DT binding properties of adv7343 decoder.
The pdata which was being read from the DT property, is removed
as this can done internally in the driver using cable detection
register.

This patch also removes the pdata of ADV7343 which was passed from
DA850 machine.

quoted

diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt b/Documentation/devicetree/bindings/media/i2c/adv7343.txt

quoted

  Required Properties :
  - compatible: Must be "adi,adv7343"
+- reg: I2C device address.
+- vddio-supply: I/O voltage supply.
+- vddcore-supply: core voltage supply.
+- vaa-supply: Analog power supply.
+- pvdd-supply: PLL power supply.

Old DTs won't contain those properties. This breaks the DT ABI if those
properties are required. Is that acceptable?

As of now adv7343 via DT binding is not enabled in any platforms
so this wont break any DT ABI.

Well, if the binding has already been written, it technically already is
an ABI. Perhaps the binding can be fixed if it isn't in use yet, but
this is definitely not the correct approach to DT.

The binding got merged for 3.12-rc1 and the intention of this patch was
to correct that binding. There we some issues like mismatch between names
of properties documented and used in the driver.

After Mark's suggestion Prabhakar removed some properties and the platform
data usage altogether. IMHO there should be only minimal changes in that
"fixup" patch, i.e. no platform data usage should be removed. Perhaps it
is fine since that's just code removal. I guess it is better to do this
sort of cleanup for the next kernel release.

Also I believe the argument of backward compatibility shouldn't really be
considered here. The $subject patch is supposed to correct the binding
before it becomes and ABI.

quoted

quoted

quoted

If it is, I think we should document that older versions of the binding
didn't require those properties, so they may in fact be missing.

I note that this patch doesn't actually update the driver to
regulator_get() anything. Shouldn't it?

As of now the driver isn’t enabling/accepting the regulators,
so should I add those in DT properties or not ?

The binding should describe the HW, not what the driver does/doesn't yet
do. I wrote the above because it looked like the driver was broken, not
to encourage you to remove properties from the binding.

OK

quoted

How does the
driver work if it doesn't enable the required regulators though, I
wonder? I suppose the boards this driver has been tested on all must
used fixed (non-SW-controlled) regulators.

on all the boards on which this decoder is connected the power to it
is provided by static circuit and not by regulators, So for this how would
you suggest to add the DT nodes for regulators ?

I believe the regulator DT properties should be made optional. Since some
(actually all upstream) boards don't bother with software controlled
regulators. We might have specified them and have defined relevant fixed
regulator(s) in DT. But I doubt it is sensible, given that it may never
happen in practice the regulators are required to be controlled by software
through the regulators API. Such devices can often be put in a low power
mode by a write to one of the registers, where their supply current is at
uA level. Looking at the datasheet ADV7343 has SLEEP_MODE in which its
typical current consumption is 5 uA.

That said the chip could be supplied from shared voltage regulators and
the driver would then have to properly request and enable the regulators.

Anyway I'm inclined to make the regulator properties optional.

--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html