Re: [PATCH 2/2] media: dt-bindings: Use additionalProperties: false for endpoint: properties:

[Rob Herring]

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