-----Original Message-----
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Sent: Wednesday, February 1, 2023 6:46 PM
To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
[off-list ref]
Cc: krzysztof.kozlowski@linaro.org; hverkuil@xs4all.nl; mchehab@kernel.org;
iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
[off-list ref]; robh+dt@kernel.org;
krzysztof.kozlowski+dt@linaro.org; rafael.j.wysocki@intel.com;
broonie@kernel.org; linux-media@vger.kernel.org;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
devicetree@vger.kernel.org
Subject: 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:

quoted

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.

quoted

quoted

quoted

quoted

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.

quoted

quoted

I caught up with this discussion after some exercise on JSON schema

validator.

quoted

I'll remove "unevaluatedProperties: false" at the "endpoint" and add

"aditionalProperties: false" instead.

quoted

Furthermore, I'll explicitly declare required properties (required: ["foo"]) and

optional properties (properties: {foo: true}) for Visconti.

quoted

Is this correct understanding?

Looks very good to me !

quoted

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).

All right. I'll apply the change to "endpoint".

quoted

quoted

quoted

quoted

we differentiate between the latter two cases ?

--
Regards,

Laurent Pinchart

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