| Message ID | 20230126165909.121302-1-jacopo.mondi@ideasonboard.com |
|---|---|
| Headers | show |
| Series | media: i2c: ov5670: OF support, runtime_pm, regulators | expand |
On 26/01/2023 17:59, Jacopo Mondi wrote: > Add the bindings documentation for Omnivision OV5670 image sensor. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- (...) > + > + 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: > + minItems: 1 > + maxItems: 2 > + items: > + enum: [1, 2] > + > + clock-noncontinuous: true You do not need this. Drop. > + Best regards, Krzysztof
Hi Krzysztof On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote: > On 26/01/2023 17:59, Jacopo Mondi wrote: > > Add the bindings documentation for Omnivision OV5670 image sensor. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > (...) > > > + > > + 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: > > + minItems: 1 > > + maxItems: 2 > > + items: > > + enum: [1, 2] > > + > > + clock-noncontinuous: true > > You do not need this. Drop. > Is this due to "unevaluatedProperties: false" ? I read you recent explanation to a similar question on the Visconti bindings. Let me summarize my understanding: For a given schema a property could be - required required: - foo - optional by default with "unevaluatedProperties: false" "foo: true" with "additionalProperties: false" - forbidden "foo: false" with "unevaluatedProperties: false" by default wiht "additionalProperties: false" clock-noncontinuous is defined in video-interfaces.yaml. as I specify "unevaluatedProperties: false" does it mean all the properties defined in video-interfaces.yaml are optionally accepted ? If that's the case that's not what I want as clock-noncontinuous is -the only- property from that file we want to accept here (and data-lanes ofc). Should I change "unevaluatedProperties: false" to "additionalProperties: false" and keep "clock-noncontinuous: true" ? Thanks j > > + > > Best regards, > Krzysztof >
On 27/01/2023 19:14, Jacopo Mondi wrote: > Hi Krzysztof > > On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote: >> On 26/01/2023 17:59, Jacopo Mondi wrote: >>> Add the bindings documentation for Omnivision OV5670 image sensor. >>> >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>> --- >> >> (...) >> >>> + >>> + 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: >>> + minItems: 1 >>> + maxItems: 2 >>> + items: >>> + enum: [1, 2] >>> + >>> + clock-noncontinuous: true >> >> You do not need this. Drop. >> > > Is this due to "unevaluatedProperties: false" ? > > I read you recent explanation to a similar question on the Visconti > bindings. Let me summarize my understanding: > > For a given schema a property could be > - required > required: > - foo > > - optional > by default with "unevaluatedProperties: false" > "foo: true" with "additionalProperties: false" > > - forbidden > "foo: false" with "unevaluatedProperties: false" > by default wiht "additionalProperties: false" > > clock-noncontinuous is defined in video-interfaces.yaml. as I specify > "unevaluatedProperties: false" does it mean > all the properties defined in video-interfaces.yaml are optionally > accepted ? If that's the case that's not what I want as > clock-noncontinuous is -the only- property from that file we want to > accept here (and data-lanes ofc). > > Should I change "unevaluatedProperties: false" to > "additionalProperties: false" and keep "clock-noncontinuous: true" ? > Why would you disallow other properties? Just because driver does not use them? That's not correct, driver change but bindings should stay the same. Best regards, Krzysztof
Hi Krzysztof, On Fri, Jan 27, 2023 at 08:58:20PM +0100, Krzysztof Kozlowski wrote: > On 27/01/2023 19:14, Jacopo Mondi wrote: > > Hi Krzysztof > > > > On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote: > >> On 26/01/2023 17:59, Jacopo Mondi wrote: > >>> Add the bindings documentation for Omnivision OV5670 image sensor. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > >>> --- > >> > >> (...) > >> > >>> + > >>> + 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: > >>> + minItems: 1 > >>> + maxItems: 2 > >>> + items: > >>> + enum: [1, 2] > >>> + > >>> + clock-noncontinuous: true > >> > >> You do not need this. Drop. > >> > > > > Is this due to "unevaluatedProperties: false" ? > > > > I read you recent explanation to a similar question on the Visconti > > bindings. Let me summarize my understanding: > > > > For a given schema a property could be > > - required > > required: > > - foo > > > > - optional > > by default with "unevaluatedProperties: false" > > "foo: true" with "additionalProperties: false" > > > > - forbidden > > "foo: false" with "unevaluatedProperties: false" > > by default wiht "additionalProperties: false" > > > > clock-noncontinuous is defined in video-interfaces.yaml. as I specify > > "unevaluatedProperties: false" does it mean > > all the properties defined in video-interfaces.yaml are optionally > > accepted ? If that's the case that's not what I want as > > clock-noncontinuous is -the only- property from that file we want to > > accept here (and data-lanes ofc). > > > > Should I change "unevaluatedProperties: false" to > > "additionalProperties: false" and keep "clock-noncontinuous: true" ? > > > > Why would you disallow other properties? Just because driver does not > use them? That's not correct, driver change but bindings should stay the > same. The clock-noncontinuous property is relevant for the hardware. There are some properties not listed here that might be relevant (for all camera sensors) but most properties in video-interfaces.yaml are not applicable to this device. I also think is be useful to say what is relevant in DT bindings, as the other sources of information left are hardware datasheets (if you have access to them) or the driver (which is supposed not to be relevant for the bindings).
On 27/01/2023 21:38, Sakari Ailus wrote: > Hi Krzysztof, > > On Fri, Jan 27, 2023 at 08:58:20PM +0100, Krzysztof Kozlowski wrote: >> On 27/01/2023 19:14, Jacopo Mondi wrote: >>> Hi Krzysztof >>> >>> On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote: >>>> On 26/01/2023 17:59, Jacopo Mondi wrote: >>>>> Add the bindings documentation for Omnivision OV5670 image sensor. >>>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>>> --- >>>> >>>> (...) >>>> >>>>> + >>>>> + 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: >>>>> + minItems: 1 >>>>> + maxItems: 2 >>>>> + items: >>>>> + enum: [1, 2] >>>>> + >>>>> + clock-noncontinuous: true >>>> >>>> You do not need this. Drop. >>>> >>> >>> Is this due to "unevaluatedProperties: false" ? >>> >>> I read you recent explanation to a similar question on the Visconti >>> bindings. Let me summarize my understanding: >>> >>> For a given schema a property could be >>> - required >>> required: >>> - foo >>> >>> - optional >>> by default with "unevaluatedProperties: false" >>> "foo: true" with "additionalProperties: false" >>> >>> - forbidden >>> "foo: false" with "unevaluatedProperties: false" >>> by default wiht "additionalProperties: false" >>> >>> clock-noncontinuous is defined in video-interfaces.yaml. as I specify >>> "unevaluatedProperties: false" does it mean >>> all the properties defined in video-interfaces.yaml are optionally >>> accepted ? If that's the case that's not what I want as >>> clock-noncontinuous is -the only- property from that file we want to >>> accept here (and data-lanes ofc). >>> >>> Should I change "unevaluatedProperties: false" to >>> "additionalProperties: false" and keep "clock-noncontinuous: true" ? >>> >> >> Why would you disallow other properties? Just because driver does not >> use them? That's not correct, driver change but bindings should stay the >> same. > > The clock-noncontinuous property is relevant for the hardware. There are > some properties not listed here that might be relevant (for all camera > sensors) but most properties in video-interfaces.yaml are not applicable to > this device. > > I also think is be useful to say what is relevant in DT bindings, as the > other sources of information left are hardware datasheets (if you have > access to them) or the driver (which is supposed not to be relevant for the > bindings). > Then it might be meaningful to list all allowed properties - even if not currently supported by the driver - and use additionalProperties:false. This has drawback - whenever video-interfaces gets something new, the bindings here (and other such devices) will have to be explicitly enabled. Best regards, Krzysztof
Hi Krzysztof On Fri, Jan 27, 2023 at 09:44:25PM +0100, Krzysztof Kozlowski wrote: > On 27/01/2023 21:38, Sakari Ailus wrote: > > Hi Krzysztof, > > > > On Fri, Jan 27, 2023 at 08:58:20PM +0100, Krzysztof Kozlowski wrote: > >> On 27/01/2023 19:14, Jacopo Mondi wrote: > >>> Hi Krzysztof > >>> > >>> On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote: > >>>> On 26/01/2023 17:59, Jacopo Mondi wrote: > >>>>> Add the bindings documentation for Omnivision OV5670 image sensor. > >>>>> > >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > >>>>> --- > >>>> > >>>> (...) > >>>> > >>>>> + > >>>>> + 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: > >>>>> + minItems: 1 > >>>>> + maxItems: 2 > >>>>> + items: > >>>>> + enum: [1, 2] > >>>>> + > >>>>> + clock-noncontinuous: true > >>>> > >>>> You do not need this. Drop. > >>>> > >>> > >>> Is this due to "unevaluatedProperties: false" ? > >>> > >>> I read you recent explanation to a similar question on the Visconti > >>> bindings. Let me summarize my understanding: > >>> > >>> For a given schema a property could be > >>> - required > >>> required: > >>> - foo > >>> > >>> - optional > >>> by default with "unevaluatedProperties: false" > >>> "foo: true" with "additionalProperties: false" > >>> > >>> - forbidden > >>> "foo: false" with "unevaluatedProperties: false" > >>> by default wiht "additionalProperties: false" > >>> > >>> clock-noncontinuous is defined in video-interfaces.yaml. as I specify > >>> "unevaluatedProperties: false" does it mean > >>> all the properties defined in video-interfaces.yaml are optionally > >>> accepted ? If that's the case that's not what I want as > >>> clock-noncontinuous is -the only- property from that file we want to > >>> accept here (and data-lanes ofc). > >>> > >>> Should I change "unevaluatedProperties: false" to > >>> "additionalProperties: false" and keep "clock-noncontinuous: true" ? > >>> > >> > >> Why would you disallow other properties? Just because driver does not > >> use them? That's not correct, driver change but bindings should stay the > >> same. > > > > The clock-noncontinuous property is relevant for the hardware. There are > > some properties not listed here that might be relevant (for all camera > > sensors) but most properties in video-interfaces.yaml are not applicable to > > this device. > > > > I also think is be useful to say what is relevant in DT bindings, as the > > other sources of information left are hardware datasheets (if you have > > access to them) or the driver (which is supposed not to be relevant for the > > bindings). > > > > Then it might be meaningful to list all allowed properties - even if not > currently supported by the driver - and use additionalProperties:false. Have a look at what properties video-interfaces.yaml lists. Some of them only apply to CSI-2 sensors (data lanes, link-frequencies etc), some of them only to parallel sensors (lines polarities, bus-width etc). I see most of the bindings in media/i2c reporting $ref: /schemas/media/video-interfaces.yaml# unevaluatedProperties: false I think that's actually wrong as there's no way all the properties in video-interfaces.yaml can apply to a single device (with the exception of a few sensors that support both bus types). > This has drawback - whenever video-interfaces gets something new, the > bindings here (and other such devices) will have to be explicitly enabled. video-interfaces is rarely updated, and when it happes it's to add properties required by a newly supported device, so this doesn't concern me much personally. > > Best regards, > Krzysztof >
Hi Jacopo, Krzysztof, On Sat, Jan 28, 2023 at 10:58:31AM +0100, Jacopo Mondi wrote: > Hi Krzysztof > > On Fri, Jan 27, 2023 at 09:44:25PM +0100, Krzysztof Kozlowski wrote: > > On 27/01/2023 21:38, Sakari Ailus wrote: > > > Hi Krzysztof, > > > > > > On Fri, Jan 27, 2023 at 08:58:20PM +0100, Krzysztof Kozlowski wrote: > > >> On 27/01/2023 19:14, Jacopo Mondi wrote: > > >>> Hi Krzysztof > > >>> > > >>> On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote: > > >>>> On 26/01/2023 17:59, Jacopo Mondi wrote: > > >>>>> Add the bindings documentation for Omnivision OV5670 image sensor. > > >>>>> > > >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > >>>>> --- > > >>>> > > >>>> (...) > > >>>> > > >>>>> + > > >>>>> + 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: > > >>>>> + minItems: 1 > > >>>>> + maxItems: 2 > > >>>>> + items: > > >>>>> + enum: [1, 2] > > >>>>> + > > >>>>> + clock-noncontinuous: true > > >>>> > > >>>> You do not need this. Drop. > > >>>> > > >>> > > >>> Is this due to "unevaluatedProperties: false" ? > > >>> > > >>> I read you recent explanation to a similar question on the Visconti > > >>> bindings. Let me summarize my understanding: > > >>> > > >>> For a given schema a property could be > > >>> - required > > >>> required: > > >>> - foo > > >>> > > >>> - optional > > >>> by default with "unevaluatedProperties: false" > > >>> "foo: true" with "additionalProperties: false" > > >>> > > >>> - forbidden > > >>> "foo: false" with "unevaluatedProperties: false" > > >>> by default wiht "additionalProperties: false" > > >>> > > >>> clock-noncontinuous is defined in video-interfaces.yaml. as I specify > > >>> "unevaluatedProperties: false" does it mean > > >>> all the properties defined in video-interfaces.yaml are optionally > > >>> accepted ? If that's the case that's not what I want as > > >>> clock-noncontinuous is -the only- property from that file we want to > > >>> accept here (and data-lanes ofc). > > >>> > > >>> Should I change "unevaluatedProperties: false" to > > >>> "additionalProperties: false" and keep "clock-noncontinuous: true" ? > > >>> > > >> > > >> Why would you disallow other properties? Just because driver does not > > >> use them? That's not correct, driver change but bindings should stay the > > >> same. > > > > > > The clock-noncontinuous property is relevant for the hardware. There are > > > some properties not listed here that might be relevant (for all camera > > > sensors) but most properties in video-interfaces.yaml are not applicable to > > > this device. > > > > > > I also think is be useful to say what is relevant in DT bindings, as the > > > other sources of information left are hardware datasheets (if you have > > > access to them) or the driver (which is supposed not to be relevant for the > > > bindings). > > > > > > > Then it might be meaningful to list all allowed properties - even if not > > currently supported by the driver - and use additionalProperties:false. > > Have a look at what properties video-interfaces.yaml lists. Some of > them only apply to CSI-2 sensors (data lanes, link-frequencies etc), > some of them only to parallel sensors (lines polarities, bus-width > etc). > > I see most of the bindings in media/i2c reporting > > $ref: /schemas/media/video-interfaces.yaml# > unevaluatedProperties: false > > I think that's actually wrong as there's no way all the properties in > video-interfaces.yaml can apply to a single device (with the exception > of a few sensors that support both bus types). It's been in my plan to split this into multiple files so you could refer to fewer than all the properties. I have no schedule for this though. > > > This has drawback - whenever video-interfaces gets something new, the > > bindings here (and other such devices) will have to be explicitly enabled. > > video-interfaces is rarely updated, and when it happes it's to add > properties required by a newly supported device, so this doesn't > concern me much personally. Me neither.
Since I got the attention of both of you, let me point out another
issue I'm facing.
We also have video-interface-devices.yaml which lists properties for
the device node and not for the endpoints.
video-interface-devices lists properties that should be all optionally
accepted, as they can potentially apply to all sensors (things like
rotation, orientation, lens-focus, flash-leds are valid for all
devices)
Being properties for the device node they should be specified in the
schema top-level and I see a few schema that do that by
allOf:
- $ref: /schemas/media/video-interface-devices.yaml#
However top level schemas usually specify
additionalProperties: false
Which means each sensor schema has to list the properties it accepts from
video-interface-devices.yaml. It's easy to verify this just by
adding "orientation" to the example in a schema that refers to
video-interface-devices.yaml and see that the bindings validation
fails (see below)
TL;DR is there a way to tell in a schema with a top-level
"additionalProperties: false" that all properties from a referenced
schema are accepted ?
I'll leave video-interface-devices.yaml this out from this series for
now and only resend this patch with the previous comments on the usage
of unevaluatedProperties fixed.
Thanks
j
-----------------------------------------------------------------------------
--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
@@ -109,6 +109,7 @@ examples:
powerdown-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
rotation = <180>;
+ orientation = <0>;
port {
/* MIPI CSI-2 bus endpoint */
$ make ARCH=arm64 dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
DTEX Documentation/devicetree/bindings/media/i2c/ovti,ov5640.example.dts
DTC_CHK Documentation/devicetree/bindings/media/i2c/ovti,ov5640.example.dtb
/home/jmondi/linux-worktree/mainline/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.example.dtb: camera@3c: 'orientation' does not match any of the regexes: 'pinctrl-[0-9]+'
From schema: /home/jmondi/linux-worktree/mainline/Documentation/devicetree/bindings/media/i2c/ovti,ov5640.yaml
On Sat, Jan 28, 2023 at 12:07:44PM +0200, Sakari Ailus wrote:
> Hi Jacopo, Krzysztof,
>
> On Sat, Jan 28, 2023 at 10:58:31AM +0100, Jacopo Mondi wrote:
> > Hi Krzysztof
> >
> > On Fri, Jan 27, 2023 at 09:44:25PM +0100, Krzysztof Kozlowski wrote:
> > > On 27/01/2023 21:38, Sakari Ailus wrote:
> > > > Hi Krzysztof,
> > > >
> > > > On Fri, Jan 27, 2023 at 08:58:20PM +0100, Krzysztof Kozlowski wrote:
> > > >> On 27/01/2023 19:14, Jacopo Mondi wrote:
> > > >>> Hi Krzysztof
> > > >>>
> > > >>> On Fri, Jan 27, 2023 at 03:19:08PM +0100, Krzysztof Kozlowski wrote:
> > > >>>> On 26/01/2023 17:59, Jacopo Mondi wrote:
> > > >>>>> Add the bindings documentation for Omnivision OV5670 image sensor.
> > > >>>>>
> > > >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > >>>>> ---
> > > >>>>
> > > >>>> (...)
> > > >>>>
> > > >>>>> +
> > > >>>>> + 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:
> > > >>>>> + minItems: 1
> > > >>>>> + maxItems: 2
> > > >>>>> + items:
> > > >>>>> + enum: [1, 2]
> > > >>>>> +
> > > >>>>> + clock-noncontinuous: true
> > > >>>>
> > > >>>> You do not need this. Drop.
> > > >>>>
> > > >>>
> > > >>> Is this due to "unevaluatedProperties: false" ?
> > > >>>
> > > >>> I read you recent explanation to a similar question on the Visconti
> > > >>> bindings. Let me summarize my understanding:
> > > >>>
> > > >>> For a given schema a property could be
> > > >>> - required
> > > >>> required:
> > > >>> - foo
> > > >>>
> > > >>> - optional
> > > >>> by default with "unevaluatedProperties: false"
> > > >>> "foo: true" with "additionalProperties: false"
> > > >>>
> > > >>> - forbidden
> > > >>> "foo: false" with "unevaluatedProperties: false"
> > > >>> by default wiht "additionalProperties: false"
> > > >>>
> > > >>> clock-noncontinuous is defined in video-interfaces.yaml. as I specify
> > > >>> "unevaluatedProperties: false" does it mean
> > > >>> all the properties defined in video-interfaces.yaml are optionally
> > > >>> accepted ? If that's the case that's not what I want as
> > > >>> clock-noncontinuous is -the only- property from that file we want to
> > > >>> accept here (and data-lanes ofc).
> > > >>>
> > > >>> Should I change "unevaluatedProperties: false" to
> > > >>> "additionalProperties: false" and keep "clock-noncontinuous: true" ?
> > > >>>
> > > >>
> > > >> Why would you disallow other properties? Just because driver does not
> > > >> use them? That's not correct, driver change but bindings should stay the
> > > >> same.
> > > >
> > > > The clock-noncontinuous property is relevant for the hardware. There are
> > > > some properties not listed here that might be relevant (for all camera
> > > > sensors) but most properties in video-interfaces.yaml are not applicable to
> > > > this device.
> > > >
> > > > I also think is be useful to say what is relevant in DT bindings, as the
> > > > other sources of information left are hardware datasheets (if you have
> > > > access to them) or the driver (which is supposed not to be relevant for the
> > > > bindings).
> > > >
> > >
> > > Then it might be meaningful to list all allowed properties - even if not
> > > currently supported by the driver - and use additionalProperties:false.
> >
> > Have a look at what properties video-interfaces.yaml lists. Some of
> > them only apply to CSI-2 sensors (data lanes, link-frequencies etc),
> > some of them only to parallel sensors (lines polarities, bus-width
> > etc).
> >
> > I see most of the bindings in media/i2c reporting
> >
> > $ref: /schemas/media/video-interfaces.yaml#
> > unevaluatedProperties: false
> >
> > I think that's actually wrong as there's no way all the properties in
> > video-interfaces.yaml can apply to a single device (with the exception
> > of a few sensors that support both bus types).
>
> It's been in my plan to split this into multiple files so you could refer
> to fewer than all the properties. I have no schedule for this though.
>
> >
> > > This has drawback - whenever video-interfaces gets something new, the
> > > bindings here (and other such devices) will have to be explicitly enabled.
> >
> > video-interfaces is rarely updated, and when it happes it's to add
> > properties required by a newly supported device, so this doesn't
> > concern me much personally.
>
> Me neither.
>
> --
> Kind regards,
>
> Sakari Ailus
Hi Jacopo, On Donnerstag, 26. Jänner 2023 17:59:00 CET Jacopo Mondi wrote: > Hello > this small series introduces OF support for the ov5670 sensor and > adds support for regulators clocks and GPIOs. > > Last year I dropped the ball as I didn't have access to HW anymore. > Luca (in cc) has reported he has a sensor and might give this new version > a spin, thanks! With this series the sensor initializes correctly for me on msm8974pro- fairphone-fp2. I locally have essentially one more change that configures the sensor for single-lane operation (currently dual-lane is hardcoded) and with that I can get image that using libcamera. Tested-by: Luca Weiss <luca@z3ntu.xyz> Thanks for respinning this! Regards Luca > > Cheers > j > > v5->v6: > - Rework clock parsing as suggested by Sakari > - Move runtime_pm enablement after async subdev registration > - Use DIV_ROUND_UP to round clock freq > > v4->v5: > - Enable clock in ov5670_runtime_resume() as suggested by Luca > - Add a patch to handle HBLANK, PIXEL_RATE and LINK_FREQ in .set_ctrl() > to fix a warning again reported by Luca > > v3->v4: > - Rework power enablement in power up sequence to support !CONFIG_OF > - Minor changes as per Sakari's review > > v2->v3: > - bindings: > - Drop assigned-clock properties from schema (moved to example) > - s/pwdn-gpios/powerdown-gpios/ > > - driver > - Use is_of_node() to decide how to parse clocks > - Fix: > drivers/media/i2c/ov5670.c:1787:18: error: initializer element is not a > compile-time constant .analog_crop = ov5670_analog_crop, > ^~~~~~~~~~~~~~~~~~ > > reported by kernel test robot and Nathan Chancellor with > clang15 and gcc < 8 > > v1->v2: > - Address Krzysztof comments on bindings > - 2/8: new patch to use the common clock framework > - Address Lauren's comment on runtime_pm function names > - 7/8: new patch to implement init_cfg as suggested by Laurent > - Rework 8/8 which was incorrect as reported by Laurent > > Thanks > j > > Jacopo Mondi (8): > media: dt-bindings: Add OV5670 > media: i2c: ov5670: Allow probing with OF > media: i2c: ov5670: Use common clock framework > media: i2c: ov5670: Probe regulators > media: i2c: ov5670: Probe GPIOs > media: i2c: ov5670: Add runtime_pm operations > media: i2c: ov5670: Implement init_cfg > media: i2c: ov5670: Handle RO controls in set_ctrl > > Jean-Michel Hautbois (1): > media: i2c: ov5670: Add .get_selection() support > > .../bindings/media/i2c/ovti,ov5670.yaml | 92 ++++++ > MAINTAINERS | 1 + > drivers/media/i2c/ov5670.c | 312 +++++++++++++++--- > 3 files changed, 360 insertions(+), 45 deletions(-) > create mode 100644 > Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml > > -- > 2.39.0
On 28/01/2023 12:03, Jacopo Mondi wrote: > Since I got the attention of both of you, let me point out another > issue I'm facing. > > We also have video-interface-devices.yaml which lists properties for > the device node and not for the endpoints. > > video-interface-devices lists properties that should be all optionally > accepted, as they can potentially apply to all sensors (things like > rotation, orientation, lens-focus, flash-leds are valid for all > devices) > > Being properties for the device node they should be specified in the > schema top-level and I see a few schema that do that by > > allOf: > - $ref: /schemas/media/video-interface-devices.yaml# > > However top level schemas usually specify > > additionalProperties: false > > Which means each sensor schema has to list the properties it accepts from > video-interface-devices.yaml. It's easy to verify this just by > adding "orientation" to the example in a schema that refers to > video-interface-devices.yaml and see that the bindings validation > fails (see below) > > TL;DR is there a way to tell in a schema with a top-level > "additionalProperties: false" that all properties from a referenced > schema are accepted ? No, because this would make it exactly the same as unevaluatedProperties, so we would have two keywords with same meaning. https://lore.kernel.org/all/c2740d66-b51f-efc2-6583-a69bde950c68@linaro.org/ Best regards, Krzysztof
Hello this small series introduces OF support for the ov5670 sensor and adds support for regulators clocks and GPIOs. Last year I dropped the ball as I didn't have access to HW anymore. Luca (in cc) has reported he has a sensor and might give this new version a spin, thanks! Cheers j v5->v6: - Rework clock parsing as suggested by Sakari - Move runtime_pm enablement after async subdev registration - Use DIV_ROUND_UP to round clock freq v4->v5: - Enable clock in ov5670_runtime_resume() as suggested by Luca - Add a patch to handle HBLANK, PIXEL_RATE and LINK_FREQ in .set_ctrl() to fix a warning again reported by Luca v3->v4: - Rework power enablement in power up sequence to support !CONFIG_OF - Minor changes as per Sakari's review v2->v3: - bindings: - Drop assigned-clock properties from schema (moved to example) - s/pwdn-gpios/powerdown-gpios/ - driver - Use is_of_node() to decide how to parse clocks - Fix: drivers/media/i2c/ov5670.c:1787:18: error: initializer element is not a compile-time constant .analog_crop = ov5670_analog_crop, ^~~~~~~~~~~~~~~~~~ reported by kernel test robot and Nathan Chancellor with clang15 and gcc < 8 v1->v2: - Address Krzysztof comments on bindings - 2/8: new patch to use the common clock framework - Address Lauren's comment on runtime_pm function names - 7/8: new patch to implement init_cfg as suggested by Laurent - Rework 8/8 which was incorrect as reported by Laurent Thanks j Jacopo Mondi (8): media: dt-bindings: Add OV5670 media: i2c: ov5670: Allow probing with OF media: i2c: ov5670: Use common clock framework media: i2c: ov5670: Probe regulators media: i2c: ov5670: Probe GPIOs media: i2c: ov5670: Add runtime_pm operations media: i2c: ov5670: Implement init_cfg media: i2c: ov5670: Handle RO controls in set_ctrl Jean-Michel Hautbois (1): media: i2c: ov5670: Add .get_selection() support .../bindings/media/i2c/ovti,ov5670.yaml | 92 ++++++ MAINTAINERS | 1 + drivers/media/i2c/ov5670.c | 312 +++++++++++++++--- 3 files changed, 360 insertions(+), 45 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml -- 2.39.0