| Message ID | 20220314162714.153970-2-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: ov5670: OF support, runtime_pm, regulators | expand |
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.
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 + + 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. + maxItems: 1 + + reset-gpios: + description: + Reference to the GPIO connected to the XSHUTDOWN pin. Active low. + maxItems: 1 + + avdd-supply: + description: Analog circuit power. Typically 2.8V. + + dvdd-supply: + description: Digital circuit power. Typically 1.2V. + + dovdd-supply: + description: Digital I/O circuit power. Typically 2.8V or 1.8V. + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + additionalProperties: false + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + data-lanes: + description: The sensor supports 1 or 2 data lanes operations. + minItems: 1 + maxItems: 2 + items: + enum: [1, 2] + + clock-noncontinuous: true + +required: + - compatible + - reg + - clocks + - port + +additionalProperties: false + +examples: + - | + i2c0 { + #address-cells = <1>; + #size-cells = <0>; + + ov5670: sensor@36 { + compatible = "ovti,ov5670"; + reg = <0x36>; + clocks = <&sensor_xclk>; + + port { + endpoint { + remote-endpoint = <&csi_ep>; + data-lanes = <1 2>; + clock-noncontinuous; + }; + }; + }; + }; + +... + diff --git a/MAINTAINERS b/MAINTAINERS index 83d27b57016f..6aa32609a3cf 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14259,6 +14259,7 @@ M: Chiranjeevi Rapolu <chiranjeevi.rapolu@intel.com> L: linux-media@vger.kernel.org S: Maintained T: git git://linuxtv.org/media_tree.git +F: Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml F: drivers/media/i2c/ov5670.c OMNIVISION OV5675 SENSOR DRIVER
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