| Message ID | 20241012-b4-linux-next-202041004-i2c-media-yaml-fixes-v1-0-a2bb12a1796d@linaro.org |
|---|---|
| Headers | show |
| Series | media: i2c: Cleanup assigned-clocks and endpoint: properties: unevaluatedProperties: false | expand |
On Sat, Oct 12, 2024 at 04:02:51PM +0100, Bryan O'Donoghue wrote: > Some of our sensor schemas use unevaluatedProperities: false for endpoint: > properties: while other schemas use additionalProperties: false. > > The effect of using unevaluatedProperities: false in this instance is that > any property in media/video-interfaces.yaml can be considered in a dts for > an endpoint. ... which is what we want and what is expected. You change the code from expected to less expected variant, so please clearly document why. > > Converting to additionalProperties: false and running DT checkers show that > such a liberal policy is unnecessary. > > We should have a consistent way of defining these properties if for no > other reason than aid other developers in the preferred way of writing > these schemas for media/i2c in the future. There is consistent way - common schema defines them. I do not understand the reasoning behind this change at all. I don't think DT maintainers ever suggested it (in fact, rather opposite: suggested using unevaluatedProps) and I think is not a consensus of any talks. Best regards, Krzysztof
On 14/10/2024 08:45, Krzysztof Kozlowski wrote: > I do not understand the reasoning behind this change at all. I don't > think DT maintainers ever suggested it (in fact, rather opposite: > suggested using unevaluatedProps) and I think is not a consensus of any > talks. No there is not but then, how do you give consistent feedback except proposing something to be a baseline. On the one hand you have upstream additionalProperties: false and unevaluatedProperites: false - it'd be better to have a consistent message on which is to be used. --- bod
On 14/10/2024 11:03, Bryan O'Donoghue wrote: > On 14/10/2024 09:47, Krzysztof Kozlowski wrote: >> If a common binding for a group of devices encourages you to list its >> subset, then it is not that common. >> >> Solution is to fix that, e.g. split it per classes of devices. > > It might be possible to have > > $ref: /schemas/media/video-interfaces-endpoint-defaults.yaml# > > which declares the typical list -> > > $ref: /schemas/media/video-interfaces.yaml# > additonalProperties:false I meant something else - define common schema matching this class of devices. Not a schema for some defaults. This is supposed to reflect how we look at hardware, not some library of schemas or library of functions. > > properties: > data-lanes: true > link-frequencies: true > remote-endpoints: true and I still don't like all these. I rather expect people to list the hardware constraints or just drop it. I mentioned it some time ago in the first patch you sent, which I think started this entire discussion. > > required: > data-lanes > link-frequencies > remote-endpoints > > and then if you need say clock-noncontinuous you'd just include > > $ref: /schemas/media/video-interfaces.yaml# > unevaluatedProperties: false > > and then list whatever you need > >> Or don't care and use unevaluatedProps because it makes people's life >> easier and is still correct. If it is not correct, then this should be >> used as an argument. > > I'll wait to see what people think before progressing this patch further. Best regards, Krzysztof
Hi Krzysztof, On Tue, Oct 15, 2024 at 08:11:18AM +0200, Krzysztof Kozlowski wrote: > On 14/10/2024 22:29, Laurent Pinchart wrote: > > On Mon, Oct 14, 2024 at 10:47:31AM +0200, Krzysztof Kozlowski wrote: > >> On 14/10/2024 10:31, Bryan O'Donoghue wrote: > >>> On 14/10/2024 08:45, Krzysztof Kozlowski wrote: > >>>> I do not understand the reasoning behind this change at all. I don't > >>>> think DT maintainers ever suggested it (in fact, rather opposite: > >>>> suggested using unevaluatedProps) and I think is not a consensus of any > >>>> talks. > >>> > >>> No there is not but then, how do you give consistent feedback except > >>> proposing something to be a baseline. > >>> > >>> On the one hand you have upstream additionalProperties: false and > >>> unevaluatedProperites: false - it'd be better to have a consistent > >>> message on which is to be used. > >> > >> Well, I am afraid that push towards additionalProps will lead to grow > >> common schema (video-interface-devices or video-interfaces) into huge > >> one-fit-all binding. And that's not good. > >> > >> If a common binding for a group of devices encourages you to list its > >> subset, then it is not that common. > >> > >> Solution is to fix that, e.g. split it per classes of devices. > > > > I think splitting large schemas per class is a good idea, but the > > problem will still exist. For instance, if we were to move the > > CSI-2-specific properties to a separate schema, that schema would define > > clock-lanes, data-lanes and clock-noncontinuous. The clock-lanes and > > clock-noncontinuous properties do not apply to every device, how would > > we then handle that ? I see three options: > > Why is this a problem? Why is this a problem here, but not in other > subsystems having exactly the same case? I won't talk for other subsystems, but I can say I see value in explicitly expressing what properties are valid for a device in DT bindings both to inform DT authors and to perform validation on DT sources. That's the whole point of YAML schemas, and I can't see a good reason not to use the tooling we have developed when it has an easy way to do the job.
On 15/10/2024 13:28, Laurent Pinchart wrote: > Hi Krzysztof, > > On Tue, Oct 15, 2024 at 08:11:18AM +0200, Krzysztof Kozlowski wrote: >> On 14/10/2024 22:29, Laurent Pinchart wrote: >>> On Mon, Oct 14, 2024 at 10:47:31AM +0200, Krzysztof Kozlowski wrote: >>>> On 14/10/2024 10:31, Bryan O'Donoghue wrote: >>>>> On 14/10/2024 08:45, Krzysztof Kozlowski wrote: >>>>>> I do not understand the reasoning behind this change at all. I don't >>>>>> think DT maintainers ever suggested it (in fact, rather opposite: >>>>>> suggested using unevaluatedProps) and I think is not a consensus of any >>>>>> talks. >>>>> >>>>> No there is not but then, how do you give consistent feedback except >>>>> proposing something to be a baseline. >>>>> >>>>> On the one hand you have upstream additionalProperties: false and >>>>> unevaluatedProperites: false - it'd be better to have a consistent >>>>> message on which is to be used. >>>> >>>> Well, I am afraid that push towards additionalProps will lead to grow >>>> common schema (video-interface-devices or video-interfaces) into huge >>>> one-fit-all binding. And that's not good. >>>> >>>> If a common binding for a group of devices encourages you to list its >>>> subset, then it is not that common. >>>> >>>> Solution is to fix that, e.g. split it per classes of devices. >>> >>> I think splitting large schemas per class is a good idea, but the >>> problem will still exist. For instance, if we were to move the >>> CSI-2-specific properties to a separate schema, that schema would define >>> clock-lanes, data-lanes and clock-noncontinuous. The clock-lanes and >>> clock-noncontinuous properties do not apply to every device, how would >>> we then handle that ? I see three options: >> >> Why is this a problem? Why is this a problem here, but not in other >> subsystems having exactly the same case? > > I won't talk for other subsystems, but I can say I see value in > explicitly expressing what properties are valid for a device in DT > bindings both to inform DT authors and to perform validation on DT > sources. That's the whole point of YAML schemas, and I can't see a good > reason not to use the tooling we have developed when it has an easy way > to do the job. I understand. The benefit, which you see, comes with complexity of the binding and need of listing properties. We do not enforce such rules (narrowing common schema in very strict way) in other subsystems, maybe with exception of input and touchscreen devices, but there common schema is quite big. And DT maintainers suggested to drop such code even for these, BTW. Best regards, Krzysztof
On Tue, Oct 15, 2024 at 02:28:06PM +0300, Laurent Pinchart wrote: > Hi Krzysztof, > > On Tue, Oct 15, 2024 at 08:11:18AM +0200, Krzysztof Kozlowski wrote: > > On 14/10/2024 22:29, Laurent Pinchart wrote: > > > On Mon, Oct 14, 2024 at 10:47:31AM +0200, Krzysztof Kozlowski wrote: > > >> On 14/10/2024 10:31, Bryan O'Donoghue wrote: > > >>> On 14/10/2024 08:45, Krzysztof Kozlowski wrote: > > >>>> I do not understand the reasoning behind this change at all. I don't > > >>>> think DT maintainers ever suggested it (in fact, rather opposite: > > >>>> suggested using unevaluatedProps) and I think is not a consensus of any > > >>>> talks. > > >>> > > >>> No there is not but then, how do you give consistent feedback except > > >>> proposing something to be a baseline. > > >>> > > >>> On the one hand you have upstream additionalProperties: false and > > >>> unevaluatedProperites: false - it'd be better to have a consistent > > >>> message on which is to be used. There are 3 options: - no $ref => additionalProperties - has a $ref: - additionalProperties and list ref-ed properties - unevaluatedProperties and don't list ref-ed properties I do debate (with myself) that that is too complicated as many don't understand the difference. We could go back to always using additionalProperties which is what we had before unevaluatedProperties was added. The other option is always use unevaluatedProperties. 2 things have stopped me from going that route. I don't care to fix 'additionalProperties' treewide which would be necessary to implement a meta-schema or check that unevaluatedProperties is used. It's not something I want to manually check in reviews. The other reason is just to not change what the rules are again. > > >> > > >> Well, I am afraid that push towards additionalProps will lead to grow > > >> common schema (video-interface-devices or video-interfaces) into huge > > >> one-fit-all binding. And that's not good. > > >> > > >> If a common binding for a group of devices encourages you to list its > > >> subset, then it is not that common. > > >> > > >> Solution is to fix that, e.g. split it per classes of devices. > > > > > > I think splitting large schemas per class is a good idea, but the > > > problem will still exist. For instance, if we were to move the > > > CSI-2-specific properties to a separate schema, that schema would define > > > clock-lanes, data-lanes and clock-noncontinuous. The clock-lanes and > > > clock-noncontinuous properties do not apply to every device, how would > > > we then handle that ? I see three options: > > > > Why is this a problem? Why is this a problem here, but not in other > > subsystems having exactly the same case? > > I won't talk for other subsystems, but I can say I see value in > explicitly expressing what properties are valid for a device in DT > bindings both to inform DT authors and to perform validation on DT > sources. That's the whole point of YAML schemas, and I can't see a good > reason not to use the tooling we have developed when it has an easy way > to do the job. This topic is just one piece of validation. A property being used that's defined, but meaningless for a device is low on the list of what I care about validating. I can't see how it would cause an actual problem? A driver is going to read the property and do what with it? Could it be an ABI issue ever? I can't see how other than a driver failing for some reason if it finds the property, but that seems a bit far fetched. Rob
On 15/10/2024 20:44, Rob Herring wrote: > On Tue, Oct 15, 2024 at 02:28:06PM +0300, Laurent Pinchart wrote: >> Hi Krzysztof, >> >> On Tue, Oct 15, 2024 at 08:11:18AM +0200, Krzysztof Kozlowski wrote: >>> On 14/10/2024 22:29, Laurent Pinchart wrote: >>>> On Mon, Oct 14, 2024 at 10:47:31AM +0200, Krzysztof Kozlowski wrote: >>>>> On 14/10/2024 10:31, Bryan O'Donoghue wrote: >>>>>> On 14/10/2024 08:45, Krzysztof Kozlowski wrote: >>>>>>> I do not understand the reasoning behind this change at all. I don't >>>>>>> think DT maintainers ever suggested it (in fact, rather opposite: >>>>>>> suggested using unevaluatedProps) and I think is not a consensus of any >>>>>>> talks. >>>>>> >>>>>> No there is not but then, how do you give consistent feedback except >>>>>> proposing something to be a baseline. >>>>>> >>>>>> On the one hand you have upstream additionalProperties: false and >>>>>> unevaluatedProperites: false - it'd be better to have a consistent >>>>>> message on which is to be used. > > There are 3 options: > > - no $ref => additionalProperties I interpret this to mean that omitting additionalProperties/unevaluatedProperties is logically the same as having additionalProperties as you will then need to list the properties explicitly. > - has a $ref: > - additionalProperties and list ref-ed properties > - unevaluatedProperties and don't list ref-ed properties > > I do debate (with myself) that that is too complicated as many don't > understand the difference. We could go back to always using > additionalProperties which is what we had before unevaluatedProperties > was added. The other option is always use unevaluatedProperties. 2 > things have stopped me from going that route. I don't care to fix > 'additionalProperties' treewide which would be necessary to implement a > meta-schema or check that unevaluatedProperties is used. It's not > something I want to manually check in reviews. The other reason is just > to not change what the rules are again. Right so I received feedback to change link-frequencies if I recall. I thought I had been very-clever (tm) by copying an upstream source and when I received feedback to change assumed the upstream source I had copied had bit-rot w/r/t the current preferred way. Some additional discussion shows there really isn't a preferred way at present. Is there a place to meaningfully document that conclusion instead both for reviewers and implementers ? Should I already know the answer to that question ? --- bod
Hi Krzysztof, On Tue, Oct 15, 2024 at 01:51:43PM +0200, Krzysztof Kozlowski wrote: > On 15/10/2024 13:28, Laurent Pinchart wrote: > > On Tue, Oct 15, 2024 at 08:11:18AM +0200, Krzysztof Kozlowski wrote: > >> On 14/10/2024 22:29, Laurent Pinchart wrote: > >>> On Mon, Oct 14, 2024 at 10:47:31AM +0200, Krzysztof Kozlowski wrote: > >>>> On 14/10/2024 10:31, Bryan O'Donoghue wrote: > >>>>> On 14/10/2024 08:45, Krzysztof Kozlowski wrote: > >>>>>> I do not understand the reasoning behind this change at all. I don't > >>>>>> think DT maintainers ever suggested it (in fact, rather opposite: > >>>>>> suggested using unevaluatedProps) and I think is not a consensus of any > >>>>>> talks. > >>>>> > >>>>> No there is not but then, how do you give consistent feedback except > >>>>> proposing something to be a baseline. > >>>>> > >>>>> On the one hand you have upstream additionalProperties: false and > >>>>> unevaluatedProperites: false - it'd be better to have a consistent > >>>>> message on which is to be used. > >>>> > >>>> Well, I am afraid that push towards additionalProps will lead to grow > >>>> common schema (video-interface-devices or video-interfaces) into huge > >>>> one-fit-all binding. And that's not good. > >>>> > >>>> If a common binding for a group of devices encourages you to list its > >>>> subset, then it is not that common. > >>>> > >>>> Solution is to fix that, e.g. split it per classes of devices. > >>> > >>> I think splitting large schemas per class is a good idea, but the > >>> problem will still exist. For instance, if we were to move the > >>> CSI-2-specific properties to a separate schema, that schema would define > >>> clock-lanes, data-lanes and clock-noncontinuous. The clock-lanes and > >>> clock-noncontinuous properties do not apply to every device, how would > >>> we then handle that ? I see three options: > >> > >> Why is this a problem? Why is this a problem here, but not in other > >> subsystems having exactly the same case? > > > > I won't talk for other subsystems, but I can say I see value in > > explicitly expressing what properties are valid for a device in DT > > bindings both to inform DT authors and to perform validation on DT > > sources. That's the whole point of YAML schemas, and I can't see a good > > reason not to use the tooling we have developed when it has an easy way > > to do the job. > > I understand. The benefit, which you see, comes with complexity of the > binding and need of listing properties. I agree, the benefit comes at a cost of additional complexity in the bindings. For me the benefit outweights the cost here as I find the cost to be relatively small, but I understand that this is a personal opinion. > We do not enforce such rules (narrowing common schema in very strict > way) in other subsystems, maybe with exception of input and touchscreen > devices, but there common schema is quite big. And DT maintainers > suggested to drop such code even for these, BTW.
Hi Rob, On Tue, Oct 15, 2024 at 02:44:18PM -0500, Rob Herring wrote: > On Tue, Oct 15, 2024 at 02:28:06PM +0300, Laurent Pinchart wrote: > > On Tue, Oct 15, 2024 at 08:11:18AM +0200, Krzysztof Kozlowski wrote: > > > On 14/10/2024 22:29, Laurent Pinchart wrote: > > > > On Mon, Oct 14, 2024 at 10:47:31AM +0200, Krzysztof Kozlowski wrote: > > > >> On 14/10/2024 10:31, Bryan O'Donoghue wrote: > > > >>> On 14/10/2024 08:45, Krzysztof Kozlowski wrote: > > > >>>> I do not understand the reasoning behind this change at all. I don't > > > >>>> think DT maintainers ever suggested it (in fact, rather opposite: > > > >>>> suggested using unevaluatedProps) and I think is not a consensus of any > > > >>>> talks. > > > >>> > > > >>> No there is not but then, how do you give consistent feedback except > > > >>> proposing something to be a baseline. > > > >>> > > > >>> On the one hand you have upstream additionalProperties: false and > > > >>> unevaluatedProperites: false - it'd be better to have a consistent > > > >>> message on which is to be used. > > There are 3 options: > > - no $ref => additionalProperties > - has a $ref: > - additionalProperties and list ref-ed properties > - unevaluatedProperties and don't list ref-ed properties > > I do debate (with myself) Those are the best and worst debates at the same time :-) > that that is too complicated as many don't > understand the difference. We could go back to always using > additionalProperties which is what we had before unevaluatedProperties > was added. The other option is always use unevaluatedProperties. 2 > things have stopped me from going that route. I don't care to fix > 'additionalProperties' treewide which would be necessary to implement a > meta-schema or check that unevaluatedProperties is used. It's not > something I want to manually check in reviews. The other reason is just > to not change what the rules are again. > > > > >> > > > >> Well, I am afraid that push towards additionalProps will lead to grow > > > >> common schema (video-interface-devices or video-interfaces) into huge > > > >> one-fit-all binding. And that's not good. > > > >> > > > >> If a common binding for a group of devices encourages you to list its > > > >> subset, then it is not that common. > > > >> > > > >> Solution is to fix that, e.g. split it per classes of devices. > > > > > > > > I think splitting large schemas per class is a good idea, but the > > > > problem will still exist. For instance, if we were to move the > > > > CSI-2-specific properties to a separate schema, that schema would define > > > > clock-lanes, data-lanes and clock-noncontinuous. The clock-lanes and > > > > clock-noncontinuous properties do not apply to every device, how would > > > > we then handle that ? I see three options: > > > > > > Why is this a problem? Why is this a problem here, but not in other > > > subsystems having exactly the same case? > > > > I won't talk for other subsystems, but I can say I see value in > > explicitly expressing what properties are valid for a device in DT > > bindings both to inform DT authors and to perform validation on DT > > sources. That's the whole point of YAML schemas, and I can't see a good > > reason not to use the tooling we have developed when it has an easy way > > to do the job. > > This topic is just one piece of validation. A property being used that's > defined, but meaningless for a device is low on the list of what I care > about validating. I can't see how it would cause an actual problem? A > driver is going to read the property and do what with it? Could it be an > ABI issue ever? I can't see how other than a driver failing for some > reason if it finds the property, but that seems a bit far fetched. I agree the risk of issues at runtime is quite low. My personal take on this is that the additional complexity of specifying "$prop: true" in the bindings is low (for me at least), and the increased correctness in DT sources to avoid confusion for DT readers is worth it. I also like how more explicit bindings cleary show in a single place what properties are expected, making it easier for DT authors. That's a personal opinion though.
On a recent schema submission I did what most well adjusted schema writers do and tried to find a base file to work from to copy/paste and forget. Confusingly/predictably I received feedback to remove or alter several of the properties included in my devious copy/paste plan. The first bit of feedback was that assigned-clock-* was to be dropped. Removing assigned-clock-* as assigned-clock-* is a core property which doesn't need to be listed in a schema. The second bit of feedback landed on use of additionalProperties:false along with enumeration of all required endpoint properties instead of an implied list of valid properties from unevaluatedProperties:false. Link: https://lore.kernel.org/linux-media/20241010-b4-master-24-11-25-ov08x40-v6-0-cf966e34e685@linaro.org This series removes the assigned-clock-* from upstream sensor property schemas and applies additionalProperities:false to properties: endpoint:. A few missing properties: or required: are added to the schemas based on output of DT checkers. The one new DT complaint I didn't fix with the move to additionalProperties: false is: /home/deckard/Development/qualcomm/qlt-kernel-tools/qlt-kernel/build/x1e80100-crd_qlt_integration/arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dtb: imx219@10: port:endpoint: 'remote-endpoint' is a required property Since this appears to be some sort of temporary/commented thing upstream which I don't know the provenance of. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- Bryan O'Donoghue (2): media: dt-bindings: Remove assigned-clock-* from various schema media: dt-bindings: Use additionalProperties: false for endpoint: properties: .../bindings/media/i2c/alliedvision,alvium-csi2.yaml | 5 ++++- .../devicetree/bindings/media/i2c/galaxycore,gc05a2.yaml | 4 +++- .../devicetree/bindings/media/i2c/galaxycore,gc08a3.yaml | 4 +++- .../devicetree/bindings/media/i2c/galaxycore,gc2145.yaml | 6 +++++- .../devicetree/bindings/media/i2c/hynix,hi846.yaml | 12 +++--------- Documentation/devicetree/bindings/media/i2c/imx219.yaml | 6 +++++- Documentation/devicetree/bindings/media/i2c/mipi-ccs.yaml | 4 +++- .../devicetree/bindings/media/i2c/ovti,og01a1b.yaml | 4 +++- .../devicetree/bindings/media/i2c/ovti,ov02a10.yaml | 4 +++- .../devicetree/bindings/media/i2c/ovti,ov4689.yaml | 4 +++- .../devicetree/bindings/media/i2c/ovti,ov5648.yaml | 13 ++++--------- .../devicetree/bindings/media/i2c/ovti,ov5675.yaml | 3 ++- .../devicetree/bindings/media/i2c/ovti,ov7251.yaml | 4 +++- .../devicetree/bindings/media/i2c/ovti,ov8865.yaml | 13 ++++--------- .../devicetree/bindings/media/i2c/ovti,ov9282.yaml | 8 +++----- .../devicetree/bindings/media/i2c/sony,imx214.yaml | 4 +++- .../devicetree/bindings/media/i2c/sony,imx258.yaml | 8 +++----- .../devicetree/bindings/media/i2c/sony,imx283.yaml | 4 +++- .../devicetree/bindings/media/i2c/sony,imx290.yaml | 4 +++- .../devicetree/bindings/media/i2c/sony,imx334.yaml | 8 +++----- .../devicetree/bindings/media/i2c/sony,imx335.yaml | 8 +++----- .../devicetree/bindings/media/i2c/sony,imx412.yaml | 8 +++----- .../devicetree/bindings/media/i2c/toshiba,tc358746.yaml | 4 +++- 23 files changed, 75 insertions(+), 67 deletions(-) --- base-commit: 58ca61c1a866bfdaa5e19fb19a2416764f847d75 change-id: 20241005-b4-linux-next-202041004-i2c-media-yaml-fixes-fcf5c0c1e08d Best regards,