Re: [PATCH v5 1/6] dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface bindings

On Mon, Jan 30, 2023 at 09:06:25AM +0000, yuji2.ishikawa@toshiba.co.jp wrote:

On Monday, January 23, 2023 4:26 AM, Laurent Pinchart wrote:

quoted

On Tue, Jan 17, 2023 at 06:01:27PM +0100, Krzysztof Kozlowski wrote:

quoted

On 17/01/2023 16:58, Laurent Pinchart wrote:

quoted

On Tue, Jan 17, 2023 at 04:42:51PM +0100, Krzysztof Kozlowski wrote:

quoted

On 17/01/2023 16:26, Laurent Pinchart wrote:

quoted

quoted

+
+          clock-lanes:
+            description: VIIF supports 1 clock line

s/line/lane/

Sorry for a late reply.
I'll fix the description.

quoted

quoted

quoted

quoted

quoted

quoted

+            const: 0

I would also add

          clock-noncontinuous: true
          link-frequencies: true

to indicate that the above two properties are used by this device.

No, these are coming from other schema and there is never need to
mention some property to indicate it is more used than other case.
None of the bindings are created such way, so this should not be exception.

There are some bindings that do so, but that may not be a good
enough reason, as there's a chance I wrote those myself :-)

I would have sworn that at some point in the past the schema
wouldn't have validated the example with this omitted. I'm not sure
if something changed or if I got this wrong.

You probably think about case when using additionalProperties:false,
where one has to explicitly list all valid properties. But not for
unevaluatedProperties:false.

Possibly, yes.

quoted

quoted

video-interfaces.yaml defines lots of properties applicable to
endpoints. For a given device, those properties should be required

required:
 - foo

quoted

(easy, that's defined in the bindings), optional,

by default (with unevaluatedProperties:false) or explicitly mention
"foo: true (with additionalProperties:false)

quoted

 or forbidden. How do

foo: false (with unevaluatedProperties:false) or by default (with
additionalProperties:false)

I think we should default to the latter. video-interfaces.yaml contains lots of
properties endpoint properties, most bindings will use less than half of them, so
having to explicitly list all the ones that are not used with "foo: false" would be
quite inconvenient. Furthermore, I expect more properties to be added to
video-interfaces.yaml over time, and those shouldn't be accepted by default in
existing bindings.

I caught up with this discussion after some exercise on JSON schema validator.
I'll remove "unevaluatedProperties: false" at the "endpoint" and add "aditionalProperties: false" instead.
Furthermore, I'll explicitly declare required properties (required: ["foo"]) and optional properties (properties: {foo: true}) for Visconti.
Is this correct understanding?

Looks very good to me !

Are these changes also applied to "port", which is the parent node of
the "endpoint" ?

That shouldn't be needed, as the "port" node should only have "endpoint"
children and no other properties (except for reg, and possibly
#address-cells and #size-cells of course).

quoted

quoted

quoted

we differentiate between the latter two cases ?

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel