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

[Prabhakar Lad]

Hi Stephen,

On Mon, Sep 16, 2013 at 9:54 PM, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 09/13/2013 11:23 PM, Prabhakar Lad wrote:
>> Hi Stephen,
>>
>> This patch should have been marked as RFC.
>>
>> Thanks for the review.
>>
>> On Sat, Sep 14, 2013 at 4:16 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>> On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
>>>> From: "Lad, Prabhakar" <prabhakar.csengg at gmail.com>
>>>>
>>>> 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.
>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/adv7343.txt b/Documentation/devicetree/bindings/media/i2c/adv7343.txt
>>>
>>>>  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.
>
Ok

>>> 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

> 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 ?

>>>>  Optional Properties :
>>>> -- adi,power-mode-sleep-mode: on enable the current consumption is reduced to
>>>> -                           micro ampere level. All DACs and the internal PLL
>>>> -                           circuit are disabled.
>>>> -- adi,power-mode-pll-ctrl: PLL and oversampling control. This control allows
>>>> -                        internal PLL 1 circuit to be powered down and the
>>>> -                        oversampling to be switched off.
>>>> -- ad,adv7343-power-mode-dac: array configuring the power on/off DAC's 1..6,
>>>> -                           0 = OFF and 1 = ON, Default value when this
>>>> -                           property is not specified is <0 0 0 0 0 0>.
>>>> -- ad,adv7343-sd-config-dac-out: array configure SD DAC Output's 1 and 2, 0 = OFF
>>>> -                              and 1 = ON, Default value when this property is
>>>> -                              not specified is <0 0>.
>>>
>>> At a very quick glance, it's not really clear why those properties are
>>> being removed. They seem like HW configuration, so might be fine to put
>>> into DT. What replaces these?
>>
>> Yes these were HW configuration but, its now internally handled in
>> the driver.  The 'ad,adv7343-power-mode-dac' property which enabled the
>> DAC's 1..6 , so now in the driver by default all the DAC's are enabled by
>> default and enable unconnected DAC auto power down. Similarly
>> 'ad,adv7343-sd-config-dac-out' property enabled SD DAC's 1..2 but
>> now is enabled by reading the CABLE DETECT register which tells
>> the status of DAC1/2.
>
> OK, that's probably fine for the two properties you mentioned (you
> didn't describe two of them...). Some more discussion on why SW doesn't
> need these options might be useful in the patch description. Note that
> the discussion should be written for software in general (i.e. any OS's
> driver), and not for Linux's specific driver, since DT is not tied to
> any one OS.
OK will update the description.

Regards,
--Prabhakar Lad