[v5,2/3] dt-bindings: media: imx274: Add optional input clock and supplies

This patch adds IMX274 optional external clock input and voltage
supplies to device tree bindings.

Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 .../devicetree/bindings/media/i2c/sony,imx274.yaml  | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Hi again...

On 02/09/20 08:46, Luca Ceresoli wrote:
> Hi Sowjanya,
> 
> On 02/09/20 04:04, Sowjanya Komatineni wrote:
>> This patch adds IMX274 optional external clock input and voltage
>> supplies to device tree bindings.
>>
>> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>  .../devicetree/bindings/media/i2c/sony,imx274.yaml  | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
>> index 7ae47a6..57e7176 100644
>> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
>> @@ -25,6 +25,27 @@ properties:
>>    reset-gpios:
>>      maxItems: 1
>>  
>> +  clocks:
>> +    maxItems: 1
>> +    description: Reference to the sensor input clock
>> +
>> +  clock-names:
>> +    maxItems: 1
>> +    items:
>> +      - const: inck
> 
> I think this can be simpler:
> 
>   clock-names:
>     maxItems: 1
>     items:
>       - const: inck

Which is equal to the original... copy-paste-and-forgot.

Here's the simplified form, for real:

  clocks:
    maxItems: 1
  clock-names:
    const: inck

As in:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/idt,versaclock5.yaml?h=v5.9-rc3#n90

Hello Sowjanya,

On Tue, Sep 01, 2020 at 07:04:37PM -0700, Sowjanya Komatineni wrote:
> This patch adds IMX274 optional external clock input and voltage
> supplies to device tree bindings.
>
> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  .../devicetree/bindings/media/i2c/sony,imx274.yaml  | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
> index 7ae47a6..57e7176 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
> @@ -25,6 +25,27 @@ properties:
>    reset-gpios:
>      maxItems: 1
>

I just sent an update to my json-schema conversion of this bindings
document (not yet on patchwork, sorry) and Sakari pointed me to the
fact in between my v2 and my v4 this patch from you went in:
4ea3273d24b ("dt-bindings: media: imx274: Add optional input clock and supplies")

I should probably now update my bindings conversion patch, basically
taking in what you've done here, but I would have one question.

> +  clocks:
> +    maxItems: 1
> +    description: Reference to the sensor input clock
> +
> +  clock-names:
> +    maxItems: 1
> +    items:
> +      - const: inck
> +
> +  vana-supply:
> +    description:
> +      Analog voltage supply, 2.8 volts
> +
> +  vdig-supply:
> +    description:
> +      Digital IO voltage supply, 1.8 volts
> +
> +  vddl-supply:
> +    description:
> +      Digital core voltage supply, 1.2 volts

4ea3273d24b introduced these regulators as VANA-supply, VDIG-supply
and VDDL-supply (please note the upper-case names). This version uses
lower-case ones instead. Is this intentional ? The driver currently
does not parse any of these if I'm not mistaken, but as the bindings
in textual form defines an ABI which should be preserved during the
conversion to json-schema, should these be kept in upper-case ?

Thanks
   j

> +
>    port:
>      type: object
>      description: |
> --
> 2.7.4
>

Hi Sowjanya,

On Thu, Sep 03, 2020 at 09:05:27AM -0700, Sowjanya Komatineni wrote:
>
> On 9/3/20 5:55 AM, Jacopo Mondi wrote:
> > Hello Sowjanya,
> >
> > On Tue, Sep 01, 2020 at 07:04:37PM -0700, Sowjanya Komatineni wrote:
> > > This patch adds IMX274 optional external clock input and voltage
> > > supplies to device tree bindings.
> > >
> > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
> > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> > > ---
> > >   .../devicetree/bindings/media/i2c/sony,imx274.yaml  | 21 +++++++++++++++++++++
> > >   1 file changed, 21 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
> > > index 7ae47a6..57e7176 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
> > > @@ -25,6 +25,27 @@ properties:
> > >     reset-gpios:
> > >       maxItems: 1
> > >
> > I just sent an update to my json-schema conversion of this bindings
> > document (not yet on patchwork, sorry) and Sakari pointed me to the
> > fact in between my v2 and my v4 this patch from you went in:
> > 4ea3273d24b ("dt-bindings: media: imx274: Add optional input clock and supplies")
> >
> > I should probably now update my bindings conversion patch, basically
> > taking in what you've done here, but I would have one question.
> >
> > > +  clocks:
> > > +    maxItems: 1
> > > +    description: Reference to the sensor input clock
> > > +
> > > +  clock-names:
> > > +    maxItems: 1
> > > +    items:
> > > +      - const: inck
> > > +
> > > +  vana-supply:
> > > +    description:
> > > +      Analog voltage supply, 2.8 volts
> > > +
> > > +  vdig-supply:
> > > +    description:
> > > +      Digital IO voltage supply, 1.8 volts
> > > +
> > > +  vddl-supply:
> > > +    description:
> > > +      Digital core voltage supply, 1.2 volts
> > 4ea3273d24b introduced these regulators as VANA-supply, VDIG-supply
> > and VDDL-supply (please note the upper-case names). This version uses
> > lower-case ones instead. Is this intentional ? The driver currently
> > does not parse any of these if I'm not mistaken, but as the bindings
> > in textual form defines an ABI which should be preserved during the
> > conversion to json-schema, should these be kept in upper-case ?
> >
> > Thanks
> >     j
>
> Yes, based on feedback lower case was recommended. So, changed to use
> lower-case names.
>
> These properties were not used by driver currently and from my prior series
> only dt-binding got merged as  no feedback was received on it for all prior
> versions.
>
> So, should be ok to change to lower-case as there properties are introduced
> now and driver update using these properties is under review
>

Well, I see that patch went in v5.9-rc1, so it will be part of v5.9.

If the bindings update goes in in v5.10 (or whatever comes after v5.9)
then we have a problem, as the DTB created for v5.9 won't work anymore
on any later version, and that should not happen. Alternatively, a fix
for the next -rc release could be fast-tracked, but you would
need to synchronize with the dt maintainers for that and make a patch
for the existing .txt bindings file.

If the name change happens in the yaml file and one release is made
with the old names, then we're stuck with those forever and ever, if I
got the situation right.

Please check with the dt and media maintainers, or they can comment
here if they glance through these lines.

Thanks
  j

> > > +
> > >     port:
> > >       type: object
> > >       description: |
> > > --
> > > 2.7.4
> > >

On 9/3/20 9:35 AM, Jacopo Mondi wrote:
> Hi Sowjanya,
>
> On Thu, Sep 03, 2020 at 09:05:27AM -0700, Sowjanya Komatineni wrote:
>> On 9/3/20 5:55 AM, Jacopo Mondi wrote:
>>> Hello Sowjanya,
>>>
>>> On Tue, Sep 01, 2020 at 07:04:37PM -0700, Sowjanya Komatineni wrote:
>>>> This patch adds IMX274 optional external clock input and voltage
>>>> supplies to device tree bindings.
>>>>
>>>> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>> ---
>>>>    .../devicetree/bindings/media/i2c/sony,imx274.yaml  | 21 +++++++++++++++++++++
>>>>    1 file changed, 21 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
>>>> index 7ae47a6..57e7176 100644
>>>> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
>>>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
>>>> @@ -25,6 +25,27 @@ properties:
>>>>      reset-gpios:
>>>>        maxItems: 1
>>>>
>>> I just sent an update to my json-schema conversion of this bindings
>>> document (not yet on patchwork, sorry) and Sakari pointed me to the
>>> fact in between my v2 and my v4 this patch from you went in:
>>> 4ea3273d24b ("dt-bindings: media: imx274: Add optional input clock and supplies")
>>>
>>> I should probably now update my bindings conversion patch, basically
>>> taking in what you've done here, but I would have one question.
>>>
>>>> +  clocks:
>>>> +    maxItems: 1
>>>> +    description: Reference to the sensor input clock
>>>> +
>>>> +  clock-names:
>>>> +    maxItems: 1
>>>> +    items:
>>>> +      - const: inck
>>>> +
>>>> +  vana-supply:
>>>> +    description:
>>>> +      Analog voltage supply, 2.8 volts
>>>> +
>>>> +  vdig-supply:
>>>> +    description:
>>>> +      Digital IO voltage supply, 1.8 volts
>>>> +
>>>> +  vddl-supply:
>>>> +    description:
>>>> +      Digital core voltage supply, 1.2 volts
>>> 4ea3273d24b introduced these regulators as VANA-supply, VDIG-supply
>>> and VDDL-supply (please note the upper-case names). This version uses
>>> lower-case ones instead. Is this intentional ? The driver currently
>>> does not parse any of these if I'm not mistaken, but as the bindings
>>> in textual form defines an ABI which should be preserved during the
>>> conversion to json-schema, should these be kept in upper-case ?
>>>
>>> Thanks
>>>      j
>> Yes, based on feedback lower case was recommended. So, changed to use
>> lower-case names.
>>
>> These properties were not used by driver currently and from my prior series
>> only dt-binding got merged as  no feedback was received on it for all prior
>> versions.
>>
>> So, should be ok to change to lower-case as there properties are introduced
>> now and driver update using these properties is under review
>>
> Well, I see that patch went in v5.9-rc1, so it will be part of v5.9.
>
> If the bindings update goes in in v5.10 (or whatever comes after v5.9)
> then we have a problem, as the DTB created for v5.9 won't work anymore
> on any later version, and that should not happen. Alternatively, a fix
> for the next -rc release could be fast-tracked, but you would
> need to synchronize with the dt maintainers for that and make a patch
> for the existing .txt bindings file.
>
> If the name change happens in the yaml file and one release is made
> with the old names, then we're stuck with those forever and ever, if I
> got the situation right.
>
> Please check with the dt and media maintainers, or they can comment
> here if they glance through these lines.
>
> Thanks
>    j

Hi Leon Luo,

I used upper case for regulator supply names in all prior 4 versions of 
IMX274 patch series as I see some other media i2c drivers doing it and 
dt-binding patch from v3 got merged in 5.9-rc1 which was using upper-case.

Later received feedback from Sakari requesting to use lower-case names 
so updated to use lower case name now in v5.

Not sure if we have timeline to squeeze in patch to change names to 
lower-case before they get into 5.10.

Can you please comment?

Sakari,

Can you also help understand why can't we keep upper case for regulator 
supplies?

I see some other media i2c drivers using upper case as well.

Thanks

Sowjanya




>>>> +
>>>>      port:
>>>>        type: object
>>>>        description: |
>>>> --
>>>> 2.7.4
>>>>

On Thu, Sep 03, 2020 at 09:40:57AM -0700, Sowjanya Komatineni wrote:
> 
> On 9/3/20 9:35 AM, Jacopo Mondi wrote:
> > Hi Sowjanya,
> > 
> > On Thu, Sep 03, 2020 at 09:05:27AM -0700, Sowjanya Komatineni wrote:
> > > On 9/3/20 5:55 AM, Jacopo Mondi wrote:
> > > > Hello Sowjanya,
> > > > 
> > > > On Tue, Sep 01, 2020 at 07:04:37PM -0700, Sowjanya Komatineni wrote:
> > > > > This patch adds IMX274 optional external clock input and voltage
> > > > > supplies to device tree bindings.
> > > > > 
> > > > > Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
> > > > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> > > > > ---
> > > > >    .../devicetree/bindings/media/i2c/sony,imx274.yaml  | 21 +++++++++++++++++++++
> > > > >    1 file changed, 21 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
> > > > > index 7ae47a6..57e7176 100644
> > > > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
> > > > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
> > > > > @@ -25,6 +25,27 @@ properties:
> > > > >      reset-gpios:
> > > > >        maxItems: 1
> > > > > 
> > > > I just sent an update to my json-schema conversion of this bindings
> > > > document (not yet on patchwork, sorry) and Sakari pointed me to the
> > > > fact in between my v2 and my v4 this patch from you went in:
> > > > 4ea3273d24b ("dt-bindings: media: imx274: Add optional input clock and supplies")
> > > > 
> > > > I should probably now update my bindings conversion patch, basically
> > > > taking in what you've done here, but I would have one question.
> > > > 
> > > > > +  clocks:
> > > > > +    maxItems: 1
> > > > > +    description: Reference to the sensor input clock
> > > > > +
> > > > > +  clock-names:
> > > > > +    maxItems: 1
> > > > > +    items:
> > > > > +      - const: inck
> > > > > +
> > > > > +  vana-supply:
> > > > > +    description:
> > > > > +      Analog voltage supply, 2.8 volts
> > > > > +
> > > > > +  vdig-supply:
> > > > > +    description:
> > > > > +      Digital IO voltage supply, 1.8 volts
> > > > > +
> > > > > +  vddl-supply:
> > > > > +    description:
> > > > > +      Digital core voltage supply, 1.2 volts
> > > > 4ea3273d24b introduced these regulators as VANA-supply, VDIG-supply
> > > > and VDDL-supply (please note the upper-case names). This version uses
> > > > lower-case ones instead. Is this intentional ? The driver currently
> > > > does not parse any of these if I'm not mistaken, but as the bindings
> > > > in textual form defines an ABI which should be preserved during the
> > > > conversion to json-schema, should these be kept in upper-case ?
> > > > 
> > > > Thanks
> > > >      j
> > > Yes, based on feedback lower case was recommended. So, changed to use
> > > lower-case names.
> > > 
> > > These properties were not used by driver currently and from my prior series
> > > only dt-binding got merged as  no feedback was received on it for all prior
> > > versions.
> > > 
> > > So, should be ok to change to lower-case as there properties are introduced
> > > now and driver update using these properties is under review
> > > 
> > Well, I see that patch went in v5.9-rc1, so it will be part of v5.9.
> > 
> > If the bindings update goes in in v5.10 (or whatever comes after v5.9)
> > then we have a problem, as the DTB created for v5.9 won't work anymore
> > on any later version, and that should not happen. Alternatively, a fix
> > for the next -rc release could be fast-tracked, but you would
> > need to synchronize with the dt maintainers for that and make a patch
> > for the existing .txt bindings file.
> > 
> > If the name change happens in the yaml file and one release is made
> > with the old names, then we're stuck with those forever and ever, if I
> > got the situation right.
> > 
> > Please check with the dt and media maintainers, or they can comment
> > here if they glance through these lines.
> > 
> > Thanks
> >    j
> 
> Hi Leon Luo,
> 
> I used upper case for regulator supply names in all prior 4 versions of
> IMX274 patch series as I see some other media i2c drivers doing it and
> dt-binding patch from v3 got merged in 5.9-rc1 which was using upper-case.
> 
> Later received feedback from Sakari requesting to use lower-case names so
> updated to use lower case name now in v5.
> 
> Not sure if we have timeline to squeeze in patch to change names to
> lower-case before they get into 5.10.
> 
> Can you please comment?

We can merge patches through the fixes branch if needed. That is not an
issue.

> 
> Sakari,
> 
> Can you also help understand why can't we keep upper case for regulator
> supplies?
> 
> I see some other media i2c drivers using upper case as well.

The vast majority of bindings use lower case, that's it, simply.

On 9/8/20 7:34 AM, Jacopo Mondi wrote:
> Hi Sakari, Sowjanya,
>
> On Tue, Sep 08, 2020 at 12:33:41PM +0300, Sakari Ailus wrote:
>> On Thu, Sep 03, 2020 at 09:40:57AM -0700, Sowjanya Komatineni wrote:
>>> On 9/3/20 9:35 AM, Jacopo Mondi wrote:
>>>> Hi Sowjanya,
>>>>
>>>> On Thu, Sep 03, 2020 at 09:05:27AM -0700, Sowjanya Komatineni wrote:
>>>>> On 9/3/20 5:55 AM, Jacopo Mondi wrote:
>>>>>> Hello Sowjanya,
>>>>>>
>>>>>> On Tue, Sep 01, 2020 at 07:04:37PM -0700, Sowjanya Komatineni wrote:
>>>>>>> This patch adds IMX274 optional external clock input and voltage
>>>>>>> supplies to device tree bindings.
>>>>>>>
>>>>>>> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>> ---
>>>>>>>     .../devicetree/bindings/media/i2c/sony,imx274.yaml  | 21 +++++++++++++++++++++
>>>>>>>     1 file changed, 21 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
>>>>>>> index 7ae47a6..57e7176 100644
>>>>>>> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx274.yaml
>>>>>>> @@ -25,6 +25,27 @@ properties:
>>>>>>>       reset-gpios:
>>>>>>>         maxItems: 1
>>>>>>>
>>>>>> I just sent an update to my json-schema conversion of this bindings
>>>>>> document (not yet on patchwork, sorry) and Sakari pointed me to the
>>>>>> fact in between my v2 and my v4 this patch from you went in:
>>>>>> 4ea3273d24b ("dt-bindings: media: imx274: Add optional input clock and supplies")
>>>>>>
>>>>>> I should probably now update my bindings conversion patch, basically
>>>>>> taking in what you've done here, but I would have one question.
>>>>>>
>>>>>>> +  clocks:
>>>>>>> +    maxItems: 1
>>>>>>> +    description: Reference to the sensor input clock
>>>>>>> +
>>>>>>> +  clock-names:
>>>>>>> +    maxItems: 1
>>>>>>> +    items:
>>>>>>> +      - const: inck
>>>>>>> +
>>>>>>> +  vana-supply:
>>>>>>> +    description:
>>>>>>> +      Analog voltage supply, 2.8 volts
>>>>>>> +
>>>>>>> +  vdig-supply:
>>>>>>> +    description:
>>>>>>> +      Digital IO voltage supply, 1.8 volts
>>>>>>> +
>>>>>>> +  vddl-supply:
>>>>>>> +    description:
>>>>>>> +      Digital core voltage supply, 1.2 volts
>>>>>> 4ea3273d24b introduced these regulators as VANA-supply, VDIG-supply
>>>>>> and VDDL-supply (please note the upper-case names). This version uses
>>>>>> lower-case ones instead. Is this intentional ? The driver currently
>>>>>> does not parse any of these if I'm not mistaken, but as the bindings
>>>>>> in textual form defines an ABI which should be preserved during the
>>>>>> conversion to json-schema, should these be kept in upper-case ?
>>>>>>
>>>>>> Thanks
>>>>>>       j
>>>>> Yes, based on feedback lower case was recommended. So, changed to use
>>>>> lower-case names.
>>>>>
>>>>> These properties were not used by driver currently and from my prior series
>>>>> only dt-binding got merged as  no feedback was received on it for all prior
>>>>> versions.
>>>>>
>>>>> So, should be ok to change to lower-case as there properties are introduced
>>>>> now and driver update using these properties is under review
>>>>>
>>>> Well, I see that patch went in v5.9-rc1, so it will be part of v5.9.
>>>>
>>>> If the bindings update goes in in v5.10 (or whatever comes after v5.9)
>>>> then we have a problem, as the DTB created for v5.9 won't work anymore
>>>> on any later version, and that should not happen. Alternatively, a fix
>>>> for the next -rc release could be fast-tracked, but you would
>>>> need to synchronize with the dt maintainers for that and make a patch
>>>> for the existing .txt bindings file.
>>>>
>>>> If the name change happens in the yaml file and one release is made
>>>> with the old names, then we're stuck with those forever and ever, if I
>>>> got the situation right.
>>>>
>>>> Please check with the dt and media maintainers, or they can comment
>>>> here if they glance through these lines.
>>>>
>>>> Thanks
>>>>     j
>>> Hi Leon Luo,
>>>
>>> I used upper case for regulator supply names in all prior 4 versions of
>>> IMX274 patch series as I see some other media i2c drivers doing it and
>>> dt-binding patch from v3 got merged in 5.9-rc1 which was using upper-case.
>>>
>>> Later received feedback from Sakari requesting to use lower-case names so
>>> updated to use lower case name now in v5.
>>>
>>> Not sure if we have timeline to squeeze in patch to change names to
>>> lower-case before they get into 5.10.
>>>
>>> Can you please comment?
>> We can merge patches through the fixes branch if needed. That is not an
>> issue.
>>
> Good! So I'll make a v5 of the json-schema bindings soon that includes
> the lower-case supplies and clock names and let's merge it as a fix in
> this release cycle.
>
> Sowjanya is this ok with you ?
Yes fine for me.
> Sakari, I'll then trust you to fast-track the patch if no other
> issues!
>
> Thanks
>    j
>
>>> Sakari,
>>>
>>> Can you also help understand why can't we keep upper case for regulator
>>> supplies?
>>>
>>> I see some other media i2c drivers using upper case as well.
>> The vast majority of bindings use lower case, that's it, simply.
>>
>> --
>> Regards,
>>
>> Sakari Ailus