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

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

[Stephen Warren]

On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
> From: "Lad, Prabhakar" <prabhakar.csengg@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?

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?

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

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

[Prabhakar Lad]

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@wwwdotorg.org> wrote:
> On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
>> From: "Lad, Prabhakar" <prabhakar.csengg@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.

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

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

Regards,
--Prabhakar

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

[Stephen Warren]

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@wwwdotorg.org> wrote:
>> On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
>>> From: "Lad, Prabhakar" <prabhakar.csengg@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.

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

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

[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@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@wwwdotorg.org> wrote:
>>> On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
>>>> From: "Lad, Prabhakar" <prabhakar.csengg@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

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

[Sylwester Nawrocki]

On 09/19/2013 06:06 PM, Prabhakar Lad wrote:
> On Mon, Sep 16, 2013 at 9:54 PM, Stephen Warren<swarren@wwwdotorg.org>  wrote:
>> On 09/13/2013 11:23 PM, Prabhakar Lad wrote:
>>> On Sat, Sep 14, 2013 at 4:16 AM, Stephen Warren<swarren@wwwdotorg.org>  wrote:
>>>> On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
>>>>> From: "Lad, Prabhakar"<prabhakar.csengg@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.

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.

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

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

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

[Prabhakar Lad]

Hi Sylwester,

On Fri, Sep 20, 2013 at 1:19 AM, Sylwester Nawrocki
<sylvester.nawrocki@gmail.com> wrote:
> On 09/19/2013 06:06 PM, Prabhakar Lad wrote:
>>
>> On Mon, Sep 16, 2013 at 9:54 PM, Stephen Warren<swarren@wwwdotorg.org>
>> wrote:
>>>
>>> On 09/13/2013 11:23 PM, Prabhakar Lad wrote:
>>>>
>>>> On Sat, Sep 14, 2013 at 4:16 AM, Stephen Warren<swarren@wwwdotorg.org>
>>>> wrote:
>>>>>
>>>>> On 09/13/2013 05:57 AM, Prabhakar Lad wrote:
>>>>>>
>>>>>> From: "Lad, Prabhakar"<prabhakar.csengg@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.
>
>
> 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.
>
OK I will, just send out a fix up patch which fixes the mismatch between
names for the rc-cycle, and later send out a patch which removes the
platform data usage for next release with proper DT bindings.

> 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.
>
>
>>>>> 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 ?
>
>
> 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.
>
I'm OK with making regulator properties as optional, But still it would
change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
pins are required properties (but still making them as optional) :-(

I think there might several devices where this situation may arise so
just thinking of a alternative solution.

say we have property 'software-regulator' which takes true/false(0/1)
If set to true we make the regulators as required property or else we
assume it is handled and ignore it ?


Thanks,
--Prabhakar Lad

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

[Sylwester Nawrocki]

Hi Prabhakar,

On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
> OK I will, just send out a fix up patch which fixes the mismatch between
> names for the rc-cycle, and later send out a patch which removes the
> platform data usage for next release with proper DT bindings.

I think the binding need to be fully corrected now, I just meant to not
touch the board file, i.e. leave non-dt support unchanged.

> I'm OK with making regulator properties as optional, But still it would
> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
> pins are required properties (but still making them as optional) :-(
> 
> I think there might several devices where this situation may arise so
> just thinking of a alternative solution.
> 
> say we have property 'software-regulator' which takes true/false(0/1)
> If set to true we make the regulators as required property or else we
> assume it is handled and ignore it ?

I don't think this is a good idea. You would have to add a similar platform 
data flag for non-dt, it doesn't sound right. I can see two options here: 

1. Make the regulator properties mandatory and, e.g. define a fixed 
   voltage GPIO regulator in DT with an empty 'gpio' property. Then
   pass a phandle to that regulator in the adv7343 *-supply properties.
   For non-dt similarly a fixed voltage regulator(s) and voltage 
   supplies  would need to be defined in the board files.

2. Make the properties optional and use (devm_)regulator_get_optional()
   calls in the driver (a recently added function). I must admit I don't 
   fully understand description of this function, it currently looks 
   pretty much same as (devm_)regulator_get(). Thus the driver would 
   need to be handling regulator supplies only when non ERR_PTR() is 
   returned from regulator_get_optional() and otherwise assume a non 
   critical error. There is already quite a few example occurrences of 
   regulator_get_optional() usage.

--
Regards,
Sylwester

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

[Prabhakar Lad]

Hi Sylwester,

On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> Hi Prabhakar,
>
> On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
>> OK I will, just send out a fix up patch which fixes the mismatch between
>> names for the rc-cycle, and later send out a patch which removes the
>> platform data usage for next release with proper DT bindings.
>
> I think the binding need to be fully corrected now, I just meant to not
> touch the board file, i.e. leave non-dt support unchanged.
>
Ok

>> I'm OK with making regulator properties as optional, But still it would
>> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
>> pins are required properties (but still making them as optional) :-(
>>
>> I think there might several devices where this situation may arise so
>> just thinking of a alternative solution.
>>
>> say we have property 'software-regulator' which takes true/false(0/1)
>> If set to true we make the regulators as required property or else we
>> assume it is handled and ignore it ?
>
> I don't think this is a good idea. You would have to add a similar platform
> data flag for non-dt, it doesn't sound right. I can see two options here:
>
> 1. Make the regulator properties mandatory and, e.g. define a fixed
>    voltage GPIO regulator in DT with an empty 'gpio' property. Then
>    pass a phandle to that regulator in the adv7343 *-supply properties.
>    For non-dt similarly a fixed voltage regulator(s) and voltage
>    supplies  would need to be defined in the board files.
>
> 2. Make the properties optional and use (devm_)regulator_get_optional()
>    calls in the driver (a recently added function). I must admit I don't
>    fully understand description of this function, it currently looks
>    pretty much same as (devm_)regulator_get(). Thus the driver would
>    need to be handling regulator supplies only when non ERR_PTR() is
>    returned from regulator_get_optional() and otherwise assume a non
>    critical error. There is already quite a few example occurrences of
>    regulator_get_optional() usage.
>
Thanks for pointing it I'll choose option 2 and post the patch.

Regards,
--Prabhakar Lad

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

[Laurent Pinchart]

On Monday 23 September 2013 08:18:52 Prabhakar Lad wrote:
> On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki wrote:
> > On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
> >> OK I will, just send out a fix up patch which fixes the mismatch between
> >> names for the rc-cycle, and later send out a patch which removes the
> >> platform data usage for next release with proper DT bindings.
> > 
> > I think the binding need to be fully corrected now, I just meant to not
> > touch the board file, i.e. leave non-dt support unchanged.
> 
> Ok
> 
> >> I'm OK with making regulator properties as optional, But still it would
> >> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
> >> pins are required properties (but still making them as optional) :-(
> >> 
> >> I think there might several devices where this situation may arise so
> >> just thinking of a alternative solution.
> >> 
> >> say we have property 'software-regulator' which takes true/false(0/1)
> >> If set to true we make the regulators as required property or else we
> >> assume it is handled and ignore it ?
> > 
> > I don't think this is a good idea. You would have to add a similar
> > platform data flag for non-dt, it doesn't sound right. I can see two
> > options here:
> > 
> > 1. Make the regulator properties mandatory and, e.g. define a fixed
> >    voltage GPIO regulator in DT with an empty 'gpio' property. Then
> >    pass a phandle to that regulator in the adv7343 *-supply properties.
> >    For non-dt similarly a fixed voltage regulator(s) and voltage
> >    supplies  would need to be defined in the board files.
> > 
> > 2. Make the properties optional and use (devm_)regulator_get_optional()
> >    calls in the driver (a recently added function). I must admit I don't
> >    fully understand description of this function, it currently looks
> >    pretty much same as (devm_)regulator_get(). Thus the driver would
> >    need to be handling regulator supplies only when non ERR_PTR() is
> >    returned from regulator_get_optional() and otherwise assume a non
> >    critical error. There is already quite a few example occurrences of
> >    regulator_get_optional() usage.
> 
> Thanks for pointing it I'll choose option 2 and post the patch.

Isn't regulator_get_optional() intended for devices that can have supplies 
unconnected in normal use ? The ADV7343 supplies are mandatory from a hardware 
point of view, so I think we should use regulator_get(). Otherwise the driver 
won't be able to tell the difference between a regulator that isn't present 
yet (for instance because the regulator device/driver hasn't been probed yet), 
which should result in deferred probing, and an always-on regulator that has 
been left out.

-- 
Regards,

Laurent Pinchart

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

[Stephen Warren]

On 09/23/2013 05:50 AM, Laurent Pinchart wrote:
> On Monday 23 September 2013 08:18:52 Prabhakar Lad wrote:
>> On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki wrote:
>>> On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
>>>> OK I will, just send out a fix up patch which fixes the mismatch between
>>>> names for the rc-cycle, and later send out a patch which removes the
>>>> platform data usage for next release with proper DT bindings.
>>>
>>> I think the binding need to be fully corrected now, I just meant to not
>>> touch the board file, i.e. leave non-dt support unchanged.
>>
>> Ok
>>
>>>> I'm OK with making regulator properties as optional, But still it would
>>>> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
>>>> pins are required properties (but still making them as optional) :-(
>>>>
>>>> I think there might several devices where this situation may arise so
>>>> just thinking of a alternative solution.
>>>>
>>>> say we have property 'software-regulator' which takes true/false(0/1)
>>>> If set to true we make the regulators as required property or else we
>>>> assume it is handled and ignore it ?
>>>
>>> I don't think this is a good idea. You would have to add a similar
>>> platform data flag for non-dt, it doesn't sound right. I can see two
>>> options here:
>>>
>>> 1. Make the regulator properties mandatory and, e.g. define a fixed
>>>    voltage GPIO regulator in DT with an empty 'gpio' property. Then
>>>    pass a phandle to that regulator in the adv7343 *-supply properties.
>>>    For non-dt similarly a fixed voltage regulator(s) and voltage
>>>    supplies  would need to be defined in the board files.
>>>
>>> 2. Make the properties optional and use (devm_)regulator_get_optional()
>>>    calls in the driver (a recently added function). I must admit I don't
>>>    fully understand description of this function, it currently looks
>>>    pretty much same as (devm_)regulator_get(). Thus the driver would
>>>    need to be handling regulator supplies only when non ERR_PTR() is
>>>    returned from regulator_get_optional() and otherwise assume a non
>>>    critical error. There is already quite a few example occurrences of
>>>    regulator_get_optional() usage.
>>
>> Thanks for pointing it I'll choose option 2 and post the patch.
> 
> Isn't regulator_get_optional() intended for devices that can have supplies 
> unconnected in normal use ?

I believe so, yes.

> The ADV7343 supplies are mandatory from a hardware 
> point of view, so I think we should use regulator_get(). Otherwise the driver 
> won't be able to tell the difference between a regulator that isn't present 
> yet (for instance because the regulator device/driver hasn't been probed yet), 
> which should result in deferred probing, and an always-on regulator that has 
> been left out.

So I think you want to make the supply properties mandatory in DT (since
some form of supply is mandatory in HW), yet make the driver support
broken DTs which don't have those properties, by error-checking the
return value from regulator_get(). You might want to put a note into DT
saying that a previous version of the binding didn't require those
supply properties, so they may be missing from older DTs.

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

[Prabhakar Lad]

On Tue, Sep 24, 2013 at 3:03 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 09/23/2013 05:50 AM, Laurent Pinchart wrote:
>> On Monday 23 September 2013 08:18:52 Prabhakar Lad wrote:
>>> On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki wrote:
>>>> On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
>>>>> OK I will, just send out a fix up patch which fixes the mismatch between
>>>>> names for the rc-cycle, and later send out a patch which removes the
>>>>> platform data usage for next release with proper DT bindings.
>>>>
>>>> I think the binding need to be fully corrected now, I just meant to not
>>>> touch the board file, i.e. leave non-dt support unchanged.
>>>
>>> Ok
>>>
>>>>> I'm OK with making regulator properties as optional, But still it would
>>>>> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
>>>>> pins are required properties (but still making them as optional) :-(
>>>>>
>>>>> I think there might several devices where this situation may arise so
>>>>> just thinking of a alternative solution.
>>>>>
>>>>> say we have property 'software-regulator' which takes true/false(0/1)
>>>>> If set to true we make the regulators as required property or else we
>>>>> assume it is handled and ignore it ?
>>>>
>>>> I don't think this is a good idea. You would have to add a similar
>>>> platform data flag for non-dt, it doesn't sound right. I can see two
>>>> options here:
>>>>
>>>> 1. Make the regulator properties mandatory and, e.g. define a fixed
>>>>    voltage GPIO regulator in DT with an empty 'gpio' property. Then
>>>>    pass a phandle to that regulator in the adv7343 *-supply properties.
>>>>    For non-dt similarly a fixed voltage regulator(s) and voltage
>>>>    supplies  would need to be defined in the board files.
>>>>
>>>> 2. Make the properties optional and use (devm_)regulator_get_optional()
>>>>    calls in the driver (a recently added function). I must admit I don't
>>>>    fully understand description of this function, it currently looks
>>>>    pretty much same as (devm_)regulator_get(). Thus the driver would
>>>>    need to be handling regulator supplies only when non ERR_PTR() is
>>>>    returned from regulator_get_optional() and otherwise assume a non
>>>>    critical error. There is already quite a few example occurrences of
>>>>    regulator_get_optional() usage.
>>>
>>> Thanks for pointing it I'll choose option 2 and post the patch.
>>
>> Isn't regulator_get_optional() intended for devices that can have supplies
>> unconnected in normal use ?
>
> I believe so, yes.
>
Agreed

>> The ADV7343 supplies are mandatory from a hardware
>> point of view, so I think we should use regulator_get(). Otherwise the driver
>> won't be able to tell the difference between a regulator that isn't present
>> yet (for instance because the regulator device/driver hasn't been probed yet),
>> which should result in deferred probing, and an always-on regulator that has
>> been left out.
>
> So I think you want to make the supply properties mandatory in DT (since
> some form of supply is mandatory in HW), yet make the driver support
> broken DTs which don't have those properties, by error-checking the
> return value from regulator_get(). You might want to put a note into DT
> saying that a previous version of the binding didn't require those
> supply properties, so they may be missing from older DTs.

OK, I'll fix it and repost the patch.

Regards,
--Prabhakar

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

[Laurent Pinchart]

On Monday 23 September 2013 15:33:10 Stephen Warren wrote:
> On 09/23/2013 05:50 AM, Laurent Pinchart wrote:
> > On Monday 23 September 2013 08:18:52 Prabhakar Lad wrote:
> >> On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki wrote:
> >>> On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
> >>>> OK I will, just send out a fix up patch which fixes the mismatch
> >>>> between
> >>>> names for the rc-cycle, and later send out a patch which removes the
> >>>> platform data usage for next release with proper DT bindings.
> >>> 
> >>> I think the binding need to be fully corrected now, I just meant to not
> >>> touch the board file, i.e. leave non-dt support unchanged.
> >> 
> >> Ok
> >> 
> >>>> I'm OK with making regulator properties as optional, But still it would
> >>>> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
> >>>> pins are required properties (but still making them as optional) :-(
> >>>> 
> >>>> I think there might several devices where this situation may arise so
> >>>> just thinking of a alternative solution.
> >>>> 
> >>>> say we have property 'software-regulator' which takes true/false(0/1)
> >>>> If set to true we make the regulators as required property or else we
> >>>> assume it is handled and ignore it ?
> >>> 
> >>> I don't think this is a good idea. You would have to add a similar
> >>> platform data flag for non-dt, it doesn't sound right. I can see two
> >>> options here:
> >>> 
> >>> 1. Make the regulator properties mandatory and, e.g. define a fixed
> >>> 
> >>>    voltage GPIO regulator in DT with an empty 'gpio' property. Then
> >>>    pass a phandle to that regulator in the adv7343 *-supply properties.
> >>>    For non-dt similarly a fixed voltage regulator(s) and voltage
> >>>    supplies  would need to be defined in the board files.
> >>> 
> >>> 2. Make the properties optional and use (devm_)regulator_get_optional()
> >>> 
> >>>    calls in the driver (a recently added function). I must admit I don't
> >>>    fully understand description of this function, it currently looks
> >>>    pretty much same as (devm_)regulator_get(). Thus the driver would
> >>>    need to be handling regulator supplies only when non ERR_PTR() is
> >>>    returned from regulator_get_optional() and otherwise assume a non
> >>>    critical error. There is already quite a few example occurrences of
> >>>    regulator_get_optional() usage.
> >> 
> >> Thanks for pointing it I'll choose option 2 and post the patch.
> > 
> > Isn't regulator_get_optional() intended for devices that can have supplies
> > unconnected in normal use ?
> 
> I believe so, yes.
> 
> > The ADV7343 supplies are mandatory from a hardware
> > point of view, so I think we should use regulator_get(). Otherwise the
> > driver won't be able to tell the difference between a regulator that
> > isn't present yet (for instance because the regulator device/driver
> > hasn't been probed yet), which should result in deferred probing, and an
> > always-on regulator that has been left out.
> 
> So I think you want to make the supply properties mandatory in DT (since
> some form of supply is mandatory in HW), yet make the driver support
> broken DTs which don't have those properties, by error-checking the
> return value from regulator_get(). You might want to put a note into DT
> saying that a previous version of the binding didn't require those
> supply properties, so they may be missing from older DTs.

Are there such devices in the wild ?

-- 
Regards,

Laurent Pinchart

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

[Mark Brown]

On Tue, Sep 24, 2013 at 11:44:57AM +0200, Laurent Pinchart wrote:
> On Monday 23 September 2013 15:33:10 Stephen Warren wrote:

> > So I think you want to make the supply properties mandatory in DT (since
> > some form of supply is mandatory in HW), yet make the driver support
> > broken DTs which don't have those properties, by error-checking the
> > return value from regulator_get(). You might want to put a note into DT
> > saying that a previous version of the binding didn't require those
> > supply properties, so they may be missing from older DTs.

> Are there such devices in the wild ?

>From v3.12 on the kernel will stub in a dummy supply if one isn't found
when using DT but I wouldn't mention that in the bindings since it's
very much fixing up broken DTs rather than a good idea.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130924/c9956124/attachment.sig>

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

[Mark Brown]

On Mon, Sep 23, 2013 at 01:50:51PM +0200, Laurent Pinchart wrote:

> Isn't regulator_get_optional() intended for devices that can have supplies 
> unconnected in normal use ? The ADV7343 supplies are mandatory from a hardware 
> point of view, so I think we should use regulator_get(). Otherwise the driver 
> won't be able to tell the difference between a regulator that isn't present 
> yet (for instance because the regulator device/driver hasn't been probed yet), 
> which should result in deferred probing, and an always-on regulator that has 
> been left out.

Yes, everything you say here is correct.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130924/fc1dba98/attachment-0001.sig>

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

[Prabhakar Lad]

Hi All,

On Mon, Sep 23, 2013 at 8:18 AM, Prabhakar Lad
<prabhakar.csengg@gmail.com> wrote:
> Hi Sylwester,
>
> On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> Hi Prabhakar,
>>
>> On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
>>> OK I will, just send out a fix up patch which fixes the mismatch between
>>> names for the rc-cycle, and later send out a patch which removes the
>>> platform data usage for next release with proper DT bindings.
>>
>> I think the binding need to be fully corrected now, I just meant to not
>> touch the board file, i.e. leave non-dt support unchanged.
>>
> Ok
>
>>> I'm OK with making regulator properties as optional, But still it would
>>> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
>>> pins are required properties (but still making them as optional) :-(
>>>
>>> I think there might several devices where this situation may arise so
>>> just thinking of a alternative solution.
>>>
>>> say we have property 'software-regulator' which takes true/false(0/1)
>>> If set to true we make the regulators as required property or else we
>>> assume it is handled and ignore it ?
>>
>> I don't think this is a good idea. You would have to add a similar platform
>> data flag for non-dt, it doesn't sound right. I can see two options here:
>>
>> 1. Make the regulator properties mandatory and, e.g. define a fixed
>>    voltage GPIO regulator in DT with an empty 'gpio' property. Then
>>    pass a phandle to that regulator in the adv7343 *-supply properties.
>>    For non-dt similarly a fixed voltage regulator(s) and voltage
>>    supplies  would need to be defined in the board files.
>>
>> 2. Make the properties optional and use (devm_)regulator_get_optional()
>>    calls in the driver (a recently added function). I must admit I don't
>>    fully understand description of this function, it currently looks
>>    pretty much same as (devm_)regulator_get(). Thus the driver would
>>    need to be handling regulator supplies only when non ERR_PTR() is
>>    returned from regulator_get_optional() and otherwise assume a non
>>    critical error. There is already quite a few example occurrences of
>>    regulator_get_optional() usage.
>>
The same question arises in case of the clock, The adv7343 encoder has
two input clocks CLKIN_A and CLKIN_B. I case of da850 EVM the
clock source to adv7343 encoder is fixed source which is enabled
by default so none of the bridge nor the adv7343 driver cares of the
clock to enable/disable.
So in this case should I be registering  (v4l2_clk_register() /
v4l2_clk_unregister())
a dummy clock in the bridge driver or in the board file ?

Regards,
--Prabhakar Lad

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

[Laurent Pinchart]

Hi Prabhakar,

On Monday 30 September 2013 18:57:01 Prabhakar Lad wrote:
> On Mon, Sep 23, 2013 at 8:18 AM, Prabhakar Lad wrote:
> > On Fri, Sep 20, 2013 at 3:22 PM, Sylwester Nawrocki wrote:
> >> On 09/20/2013 10:11 AM, Prabhakar Lad wrote:
> >>> OK I will, just send out a fix up patch which fixes the mismatch between
> >>> names for the rc-cycle, and later send out a patch which removes the
> >>> platform data usage for next release with proper DT bindings.
> >> 
> >> I think the binding need to be fully corrected now, I just meant to not
> >> touch the board file, i.e. leave non-dt support unchanged.
> > 
> > Ok
> > 
> >>> I'm OK with making regulator properties as optional, But still it would
> >>> change the meaning of what DT is, we know that the VDD/VDD_IO .. etc
> >>> pins are required properties (but still making them as optional) :-(
> >>> 
> >>> I think there might several devices where this situation may arise so
> >>> just thinking of a alternative solution.
> >>> 
> >>> say we have property 'software-regulator' which takes true/false(0/1)
> >>> If set to true we make the regulators as required property or else we
> >>> assume it is handled and ignore it ?
> >> 
> >> I don't think this is a good idea. You would have to add a similar
> >> platform data flag for non-dt, it doesn't sound right. I can see two
> >> options here:
> >> 
> >> 1. Make the regulator properties mandatory and, e.g. define a fixed
> >>    voltage GPIO regulator in DT with an empty 'gpio' property. Then
> >>    pass a phandle to that regulator in the adv7343 *-supply properties.
> >>    For non-dt similarly a fixed voltage regulator(s) and voltage
> >>    supplies  would need to be defined in the board files.
> >> 
> >> 2. Make the properties optional and use (devm_)regulator_get_optional()
> >>    calls in the driver (a recently added function). I must admit I don't
> >>    fully understand description of this function, it currently looks
> >>    pretty much same as (devm_)regulator_get(). Thus the driver would
> >>    need to be handling regulator supplies only when non ERR_PTR() is
> >>    returned from regulator_get_optional() and otherwise assume a non
> >>    critical error. There is already quite a few example occurrences of
> >>    regulator_get_optional() usage.
> 
> The same question arises in case of the clock, The adv7343 encoder has two
> input clocks CLKIN_A and CLKIN_B. I case of da850 EVM the clock source to
> adv7343 encoder is fixed source which is enabled by default so none of the
> bridge nor the adv7343 driver cares of the clock to enable/disable. So in
> this case should I be registering  (v4l2_clk_register() /
> v4l2_clk_unregister()) a dummy clock in the bridge driver or in the board
> file ?

The fixed clock (which is a real clock, not a dummy clock) should be 
registered in board file, preferably using the common clock framework if 
that's available on your platform.

-- 
Regards,

Laurent Pinchart