[v2,1/8] media: dt-bindings: i2c: Document ov5670

  Provide the bindings documentation for Omnivision OV5670 image sensor.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

---
v1->v2 (comments from Krzysztof)

- Rename to include manufacturer name
- Add entry to MAINTAINERS
- Add maxItems: to -gpios properties
- Use common clock properties
- Use enum: [1, 2] for data lanes
- Fix whitespace issue in example
---

 .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 100 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml

--
2.35.1
  

On 14/03/2022 17:27, Jacopo Mondi wrote:
> Provide the bindings documentation for Omnivision OV5670 image sensor.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> ---
> v1->v2 (comments from Krzysztof)
> 
> - Rename to include manufacturer name
> - Add entry to MAINTAINERS
> - Add maxItems: to -gpios properties
> - Use common clock properties
> - Use enum: [1, 2] for data lanes
> - Fix whitespace issue in example
> ---
> 
>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 100 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> new file mode 100644
> index 000000000000..73cf72203f17
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> @@ -0,0 +1,99 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Omnivision OV5670 5 Megapixels raw image sensor
> +
> +maintainers:
> +  - Jacopo Mondi <jacopo@jmondi.org>
> +
> +description: |-
> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> +  controlled through an I2C compatible control bus.
> +
> +properties:
> +  compatible:
> +    const: ovti,ov5670
> +
> +  reg:
> +    maxItems: 1
> +
> +  assigned-clocks: true
> +  assigned-clock-parents: true
> +  assigned-clock-rates: true

You should not need these. These are coming with schema. You can add
these to example schema below and double-check.

> +
> +  clocks:
> +    description: System clock. From 6 to 27 MHz.
> +    maxItems: 1
> +
> +  pwdn-gpios:
> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.

This does not look like a standard property, so you need a vendor prefix.

> +    maxItems: 1
> +

Best regards,
Krzysztof
  

Hi Krzysztof, Jacopo,

On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
> On 14/03/2022 17:27, Jacopo Mondi wrote:
> > Provide the bindings documentation for Omnivision OV5670 image sensor.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > 
> > ---
> > v1->v2 (comments from Krzysztof)
> > 
> > - Rename to include manufacturer name
> > - Add entry to MAINTAINERS
> > - Add maxItems: to -gpios properties
> > - Use common clock properties
> > - Use enum: [1, 2] for data lanes
> > - Fix whitespace issue in example
> > ---
> > 
> >  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 100 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > new file mode 100644
> > index 000000000000..73cf72203f17
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > @@ -0,0 +1,99 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision OV5670 5 Megapixels raw image sensor
> > +
> > +maintainers:
> > +  - Jacopo Mondi <jacopo@jmondi.org>
> > +
> > +description: |-
> > +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> > +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> > +  controlled through an I2C compatible control bus.
> > +
> > +properties:
> > +  compatible:
> > +    const: ovti,ov5670
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  assigned-clocks: true
> > +  assigned-clock-parents: true
> > +  assigned-clock-rates: true
> 
> You should not need these. These are coming with schema. You can add
> these to example schema below and double-check.

They should probably be required actually.

> 
> > +
> > +  clocks:
> > +    description: System clock. From 6 to 27 MHz.
> > +    maxItems: 1
> > +
> > +  pwdn-gpios:
> > +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
> 
> This does not look like a standard property, so you need a vendor prefix.

The similarly named property exists elsewhere. I wouldn't use a vendor
prefix, also for the reason that the functionality is quite common. I guess
alternative name would be possible, too --- "shutdown" seems to be more
common.
  

On 15/03/2022 08:59, Sakari Ailus wrote:
> Hi Krzysztof, Jacopo,
> 
> On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
>> On 14/03/2022 17:27, Jacopo Mondi wrote:
>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>
>>> ---
>>> v1->v2 (comments from Krzysztof)
>>>
>>> - Rename to include manufacturer name
>>> - Add entry to MAINTAINERS
>>> - Add maxItems: to -gpios properties
>>> - Use common clock properties
>>> - Use enum: [1, 2] for data lanes
>>> - Fix whitespace issue in example
>>> ---
>>>
>>>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
>>>  MAINTAINERS                                   |  1 +
>>>  2 files changed, 100 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>> new file mode 100644
>>> index 000000000000..73cf72203f17
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>> @@ -0,0 +1,99 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
>>> +
>>> +maintainers:
>>> +  - Jacopo Mondi <jacopo@jmondi.org>
>>> +
>>> +description: |-
>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
>>> +  controlled through an I2C compatible control bus.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: ovti,ov5670
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  assigned-clocks: true
>>> +  assigned-clock-parents: true
>>> +  assigned-clock-rates: true
>>
>> You should not need these. These are coming with schema. You can add
>> these to example schema below and double-check.
> 
> They should probably be required actually.

Why required? The hardware can work with different clocks, get their
rate and configure internal PLLs/clocks to new value. Having it required
might have sense for current implementation of driver but this is
independent of bindings. Bindings do not describe driver, but hardware.

>>
>>> +
>>> +  clocks:
>>> +    description: System clock. From 6 to 27 MHz.
>>> +    maxItems: 1
>>> +
>>> +  pwdn-gpios:
>>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>>
>> This does not look like a standard property, so you need a vendor prefix.
> 
> The similarly named property exists elsewhere. I wouldn't use a vendor
> prefix, also for the reason that the functionality is quite common. I guess
> alternative name would be possible, too --- "shutdown" seems to be more
> common.

It exists in three bindings, so it is not a standard property...
although something closer to standard is "powerdown-gpios" so maybe just
use that one?

Best regards,
Krzysztof
  

Hi Sakari,

On Tue, Mar 15, 2022 at 09:59:17AM +0200, Sakari Ailus wrote:
> On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
> > On 14/03/2022 17:27, Jacopo Mondi wrote:
> > > Provide the bindings documentation for Omnivision OV5670 image sensor.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > 
> > > ---
> > > v1->v2 (comments from Krzysztof)
> > > 
> > > - Rename to include manufacturer name
> > > - Add entry to MAINTAINERS
> > > - Add maxItems: to -gpios properties
> > > - Use common clock properties
> > > - Use enum: [1, 2] for data lanes
> > > - Fix whitespace issue in example
> > > ---
> > > 
> > >  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
> > >  MAINTAINERS                                   |  1 +
> > >  2 files changed, 100 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > > new file mode 100644
> > > index 000000000000..73cf72203f17
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > > @@ -0,0 +1,99 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Omnivision OV5670 5 Megapixels raw image sensor
> > > +
> > > +maintainers:
> > > +  - Jacopo Mondi <jacopo@jmondi.org>
> > > +
> > > +description: |-
> > > +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> > > +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> > > +  controlled through an I2C compatible control bus.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: ovti,ov5670
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  assigned-clocks: true
> > > +  assigned-clock-parents: true
> > > +  assigned-clock-rates: true
> > 
> > You should not need these. These are coming with schema. You can add
> > these to example schema below and double-check.
> 
> They should probably be required actually.

Why so ?

> > > +
> > > +  clocks:
> > > +    description: System clock. From 6 to 27 MHz.
> > > +    maxItems: 1
> > > +
> > > +  pwdn-gpios:
> > > +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
> > 
> > This does not look like a standard property, so you need a vendor prefix.
> 
> The similarly named property exists elsewhere. I wouldn't use a vendor
> prefix, also for the reason that the functionality is quite common. I guess
> alternative name would be possible, too --- "shutdown" seems to be more
> common.

There's a desire to standardize GPIO names ("reset" and "enable" being
the two most common candidates), but I'm not aware of an official list
of standard names. Have I missed it ?

In this case, I'd use powerdown-gpios, as it's more common than
pwdn-gpios (used in 21 bindings, compared to 2 for pwdn-gpios).
  

Hi Krzysztof,

On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
> On 14/03/2022 17:27, Jacopo Mondi wrote:
> > Provide the bindings documentation for Omnivision OV5670 image sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >
> > ---
> > v1->v2 (comments from Krzysztof)
> >
> > - Rename to include manufacturer name
> > - Add entry to MAINTAINERS
> > - Add maxItems: to -gpios properties
> > - Use common clock properties
> > - Use enum: [1, 2] for data lanes
> > - Fix whitespace issue in example
> > ---
> >
> >  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 100 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > new file mode 100644
> > index 000000000000..73cf72203f17
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > @@ -0,0 +1,99 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Omnivision OV5670 5 Megapixels raw image sensor
> > +
> > +maintainers:
> > +  - Jacopo Mondi <jacopo@jmondi.org>
> > +
> > +description: |-
> > +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> > +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> > +  controlled through an I2C compatible control bus.
> > +
> > +properties:
> > +  compatible:
> > +    const: ovti,ov5670
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  assigned-clocks: true
> > +  assigned-clock-parents: true
> > +  assigned-clock-rates: true
>
> You should not need these. These are coming with schema. You can add
> these to example schema below and double-check.
>

Thanks, I'll try removing and see how it works

> > +
> > +  clocks:
> > +    description: System clock. From 6 to 27 MHz.
> > +    maxItems: 1
> > +
> > +  pwdn-gpios:
> > +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>
> This does not look like a standard property, so you need a vendor prefix.
>

$ git grep powerdown-gpio arch/arm*/boot/dts | wc -l
48

$ git grep pwdn-gpio arch/arm*/boot/dts | wc -l
5

I'll go with the majority.

I wonder however if 'standard' proeprties are documented somewhere.

Thanks
  j


> > +    maxItems: 1
> > +
>
> Best regards,
> Krzysztof
  

On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote:
> On 15/03/2022 08:59, Sakari Ailus wrote:
> > Hi Krzysztof, Jacopo,
> > 
> > On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
> >> On 14/03/2022 17:27, Jacopo Mondi wrote:
> >>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>
> >>> ---
> >>> v1->v2 (comments from Krzysztof)
> >>>
> >>> - Rename to include manufacturer name
> >>> - Add entry to MAINTAINERS
> >>> - Add maxItems: to -gpios properties
> >>> - Use common clock properties
> >>> - Use enum: [1, 2] for data lanes
> >>> - Fix whitespace issue in example
> >>> ---
> >>>
> >>>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
> >>>  MAINTAINERS                                   |  1 +
> >>>  2 files changed, 100 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> >>> new file mode 100644
> >>> index 000000000000..73cf72203f17
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> >>> @@ -0,0 +1,99 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Omnivision OV5670 5 Megapixels raw image sensor
> >>> +
> >>> +maintainers:
> >>> +  - Jacopo Mondi <jacopo@jmondi.org>
> >>> +
> >>> +description: |-
> >>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> >>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> >>> +  controlled through an I2C compatible control bus.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: ovti,ov5670
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  assigned-clocks: true
> >>> +  assigned-clock-parents: true
> >>> +  assigned-clock-rates: true
> >>
> >> You should not need these. These are coming with schema. You can add
> >> these to example schema below and double-check.
> > 
> > They should probably be required actually.
> 
> Why required? The hardware can work with different clocks, get their
> rate and configure internal PLLs/clocks to new value. Having it required
> might have sense for current implementation of driver but this is
> independent of bindings. Bindings do not describe driver, but hardware.

We've had this discussion before and the result of that was this (see
"Handling clocks"):

Documentation/driver-api/media/camera-sensor.rst

> 
> >>
> >>> +
> >>> +  clocks:
> >>> +    description: System clock. From 6 to 27 MHz.
> >>> +    maxItems: 1
> >>> +
> >>> +  pwdn-gpios:
> >>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
> >>
> >> This does not look like a standard property, so you need a vendor prefix.
> > 
> > The similarly named property exists elsewhere. I wouldn't use a vendor
> > prefix, also for the reason that the functionality is quite common. I guess
> > alternative name would be possible, too --- "shutdown" seems to be more
> > common.
> 
> It exists in three bindings, so it is not a standard property...
> although something closer to standard is "powerdown-gpios" so maybe just
> use that one?

Seems like a better choice.
  

On 15/03/2022 13:47, Sakari Ailus wrote:
> On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote:
>> On 15/03/2022 08:59, Sakari Ailus wrote:
>>> Hi Krzysztof, Jacopo,
>>>
>>> On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
>>>> On 14/03/2022 17:27, Jacopo Mondi wrote:
>>>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>>
>>>>> ---
>>>>> v1->v2 (comments from Krzysztof)
>>>>>
>>>>> - Rename to include manufacturer name
>>>>> - Add entry to MAINTAINERS
>>>>> - Add maxItems: to -gpios properties
>>>>> - Use common clock properties
>>>>> - Use enum: [1, 2] for data lanes
>>>>> - Fix whitespace issue in example
>>>>> ---
>>>>>
>>>>>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
>>>>>  MAINTAINERS                                   |  1 +
>>>>>  2 files changed, 100 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..73cf72203f17
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>>>> @@ -0,0 +1,99 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
>>>>> +
>>>>> +maintainers:
>>>>> +  - Jacopo Mondi <jacopo@jmondi.org>
>>>>> +
>>>>> +description: |-
>>>>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
>>>>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
>>>>> +  controlled through an I2C compatible control bus.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: ovti,ov5670
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  assigned-clocks: true
>>>>> +  assigned-clock-parents: true
>>>>> +  assigned-clock-rates: true
>>>>
>>>> You should not need these. These are coming with schema. You can add
>>>> these to example schema below and double-check.
>>>
>>> They should probably be required actually.
>>
>> Why required? The hardware can work with different clocks, get their
>> rate and configure internal PLLs/clocks to new value. Having it required
>> might have sense for current implementation of driver but this is
>> independent of bindings. Bindings do not describe driver, but hardware.
> 
> We've had this discussion before and the result of that was this (see
> "Handling clocks"):
> 
> Documentation/driver-api/media/camera-sensor.rst

... and the "Devicetree" chapter explains usage of assigned-clock-xxx.
None of these explain why it should be in the bindings. The chapter you
referred explicitly mentions that "clock tree is generally configured by
the driver", so it has nothing to do with the bindings.

Bindings describe hardware, not Linux driver implementation. The
hardware can work with multiple frequencies and can support changing
these frequencies, probably combined with some reset sequence. Therefore
hardware does not require the clock frequency to be always fixed and
defined.

Driver requires assigned-clock-xxx, not bindings. Do not put Linux
implementation specifics into the bindings.

Best regards,
Krzysztof
  

Hi Sakari,

On Tue, Mar 15, 2022 at 02:47:43PM +0200, Sakari Ailus wrote:
> On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote:
> > On 15/03/2022 08:59, Sakari Ailus wrote:
> > > On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
> > >> On 14/03/2022 17:27, Jacopo Mondi wrote:
> > >>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> > >>>
> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>>
> > >>> ---
> > >>> v1->v2 (comments from Krzysztof)
> > >>>
> > >>> - Rename to include manufacturer name
> > >>> - Add entry to MAINTAINERS
> > >>> - Add maxItems: to -gpios properties
> > >>> - Use common clock properties
> > >>> - Use enum: [1, 2] for data lanes
> > >>> - Fix whitespace issue in example
> > >>> ---
> > >>>
> > >>>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
> > >>>  MAINTAINERS                                   |  1 +
> > >>>  2 files changed, 100 insertions(+)
> > >>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > >>>
> > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > >>> new file mode 100644
> > >>> index 000000000000..73cf72203f17
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > >>> @@ -0,0 +1,99 @@
> > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >>> +%YAML 1.2
> > >>> +---
> > >>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml#
> > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>> +
> > >>> +title: Omnivision OV5670 5 Megapixels raw image sensor
> > >>> +
> > >>> +maintainers:
> > >>> +  - Jacopo Mondi <jacopo@jmondi.org>
> > >>> +
> > >>> +description: |-
> > >>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> > >>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> > >>> +  controlled through an I2C compatible control bus.
> > >>> +
> > >>> +properties:
> > >>> +  compatible:
> > >>> +    const: ovti,ov5670
> > >>> +
> > >>> +  reg:
> > >>> +    maxItems: 1
> > >>> +
> > >>> +  assigned-clocks: true
> > >>> +  assigned-clock-parents: true
> > >>> +  assigned-clock-rates: true
> > >>
> > >> You should not need these. These are coming with schema. You can add
> > >> these to example schema below and double-check.
> > > 
> > > They should probably be required actually.
> > 
> > Why required? The hardware can work with different clocks, get their
> > rate and configure internal PLLs/clocks to new value. Having it required
> > might have sense for current implementation of driver but this is
> > independent of bindings. Bindings do not describe driver, but hardware.
> 
> We've had this discussion before and the result of that was this (see
> "Handling clocks"):
> 
> Documentation/driver-api/media/camera-sensor.rst

I don't think those properties should be required in the sensor
bindings. There are platforms where the clock provided to the sensor
comes from a fixed-frequency oscillator, assigning a rate or parent
makes no sense for those (assigning a parent would actually be
impossible).

Assigning a parent or rate is fine when applicable, but as it can't be
required, there's also no point in listing the properties here.

> > >>> +
> > >>> +  clocks:
> > >>> +    description: System clock. From 6 to 27 MHz.
> > >>> +    maxItems: 1
> > >>> +
> > >>> +  pwdn-gpios:
> > >>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
> > >>
> > >> This does not look like a standard property, so you need a vendor prefix.
> > > 
> > > The similarly named property exists elsewhere. I wouldn't use a vendor
> > > prefix, also for the reason that the functionality is quite common. I guess
> > > alternative name would be possible, too --- "shutdown" seems to be more
> > > common.
> > 
> > It exists in three bindings, so it is not a standard property...
> > although something closer to standard is "powerdown-gpios" so maybe just
> > use that one?
> 
> Seems like a better choice.
  

Hi Laurent,

On Tue, Mar 15, 2022 at 03:01:35PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tue, Mar 15, 2022 at 02:47:43PM +0200, Sakari Ailus wrote:
> > On Tue, Mar 15, 2022 at 09:03:41AM +0100, Krzysztof Kozlowski wrote:
> > > On 15/03/2022 08:59, Sakari Ailus wrote:
> > > > On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
> > > >> On 14/03/2022 17:27, Jacopo Mondi wrote:
> > > >>> Provide the bindings documentation for Omnivision OV5670 image sensor.
> > > >>>
> > > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >>>
> > > >>> ---
> > > >>> v1->v2 (comments from Krzysztof)
> > > >>>
> > > >>> - Rename to include manufacturer name
> > > >>> - Add entry to MAINTAINERS
> > > >>> - Add maxItems: to -gpios properties
> > > >>> - Use common clock properties
> > > >>> - Use enum: [1, 2] for data lanes
> > > >>> - Fix whitespace issue in example
> > > >>> ---
> > > >>>
> > > >>>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
> > > >>>  MAINTAINERS                                   |  1 +
> > > >>>  2 files changed, 100 insertions(+)
> > > >>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > > >>>
> > > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > > >>> new file mode 100644
> > > >>> index 000000000000..73cf72203f17
> > > >>> --- /dev/null
> > > >>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
> > > >>> @@ -0,0 +1,99 @@
> > > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > >>> +%YAML 1.2
> > > >>> +---
> > > >>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml#
> > > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > >>> +
> > > >>> +title: Omnivision OV5670 5 Megapixels raw image sensor
> > > >>> +
> > > >>> +maintainers:
> > > >>> +  - Jacopo Mondi <jacopo@jmondi.org>
> > > >>> +
> > > >>> +description: |-
> > > >>> +  The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits
> > > >>> +  RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is
> > > >>> +  controlled through an I2C compatible control bus.
> > > >>> +
> > > >>> +properties:
> > > >>> +  compatible:
> > > >>> +    const: ovti,ov5670
> > > >>> +
> > > >>> +  reg:
> > > >>> +    maxItems: 1
> > > >>> +
> > > >>> +  assigned-clocks: true
> > > >>> +  assigned-clock-parents: true
> > > >>> +  assigned-clock-rates: true
> > > >>
> > > >> You should not need these. These are coming with schema. You can add
> > > >> these to example schema below and double-check.
> > > > 
> > > > They should probably be required actually.
> > > 
> > > Why required? The hardware can work with different clocks, get their
> > > rate and configure internal PLLs/clocks to new value. Having it required
> > > might have sense for current implementation of driver but this is
> > > independent of bindings. Bindings do not describe driver, but hardware.
> > 
> > We've had this discussion before and the result of that was this (see
> > "Handling clocks"):
> > 
> > Documentation/driver-api/media/camera-sensor.rst
> 
> I don't think those properties should be required in the sensor
> bindings. There are platforms where the clock provided to the sensor
> comes from a fixed-frequency oscillator, assigning a rate or parent
> makes no sense for those (assigning a parent would actually be
> impossible).
> 
> Assigning a parent or rate is fine when applicable, but as it can't be
> required, there's also no point in listing the properties here.

The cases where the clock is fixed are quite rare but admittedly they
exist.

It's just easy to get this wrong.