Adds the Device Tree binding documentation that allows to describe
the Video Input Interface found in Toshiba Visconti SoCs.
Signed-off-by: Yuji Ishikawa <[email protected]>
Reviewed-by: Nobuhiro Iwamatsu <[email protected]>
---
Changelog v2:
- no change
Changelog v3:
- no change
Changelog v4:
- fix style problems at the v3 patch
- remove "index" member
- update example
Changelog v5:
- no change
---
.../bindings/media/toshiba,visconti-viif.yaml | 98 +++++++++++++++++++
1 file changed, 98 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
diff --git a/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml b/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
new file mode 100644
index 00000000000..71442724d1a
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
@@ -0,0 +1,98 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/media/toshiba,visconti-viif.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Toshiba Visconti5 SoC Video Input Interface Device Tree Bindings
+
+maintainers:
+ - Nobuhiro Iwamatsu <[email protected]>
+
+description:
+ Toshiba Visconti5 SoC Video Input Interface (VIIF)
+ receives MIPI CSI2 video stream,
+ processes the stream with embedded image signal processor (L1ISP, L2ISP),
+ then stores pictures to main memory.
+
+properties:
+ compatible:
+ const: toshiba,visconti-viif
+
+ reg:
+ items:
+ - description: registers for capture control
+ - description: registers for CSI2 receiver control
+
+ interrupts:
+ items:
+ - description: Sync Interrupt
+ - description: Status (Error) Interrupt
+ - description: CSI2 Receiver Interrupt
+ - description: L1ISP Interrupt
+
+ port:
+ $ref: /schemas/graph.yaml#/$defs/port-base
+ unevaluatedProperties: false
+ description: Input port, single endpoint describing the CSI-2 transmitter.
+
+ properties:
+ endpoint:
+ $ref: video-interfaces.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ data-lanes:
+ description: VIIF supports 2 or 4 data lines
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ minItems: 1
+ maxItems: 4
+ items:
+ minimum: 1
+ maximum: 4
+
+ clock-lanes:
+ description: VIIF supports 1 clock line
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ viif@1c000000 {
+ compatible = "toshiba,visconti-viif";
+ reg = <0 0x1c000000 0 0x6000>,
+ <0 0x1c008000 0 0x400>;
+ interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
+
+ port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ csi_in0: endpoint {
+ remote-endpoint = <&imx219_out0>;
+ bus-type = <4>;
+ data-lanes = <1 2>;
+ clock-lanes = <0>;
+ clock-noncontinuous;
+ link-frequencies = /bits/ 64 <456000000>;
+ };
+ };
+ };
+ };
--
2.25.1
On 11/01/2023 03:24, Yuji Ishikawa wrote:
> Adds the Device Tree binding documentation that allows to describe
> the Video Input Interface found in Toshiba Visconti SoCs.
>
> Signed-off-by: Yuji Ishikawa <[email protected]>
> Reviewed-by: Nobuhiro Iwamatsu <[email protected]>
> ---
> Changelog v2:
> - no change
>
> Changelog v3:
> - no change
>
> Changelog v4:
> - fix style problems at the v3 patch
> - remove "index" member
> - update example
>
> Changelog v5:
> - no change
No change? so all comments got ignored?
This is a friendly reminder during the review process.
It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.
Thank you.
Best regards,
Krzysztof
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Wednesday, January 11, 2023 6:20 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <[email protected]>; Hans Verkuil <[email protected]>; Laurent
> Pinchart <[email protected]>; Mauro Carvalho Chehab
> <[email protected]>; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> <[email protected]>; Rob Herring <[email protected]>;
> Krzysztof Kozlowski <[email protected]>; Rafael J . Wysocki
> <[email protected]>; Mark Brown <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v5 1/6] dt-bindings: media: platform: visconti: Add Toshiba
> Visconti Video Input Interface bindings
>
> On 11/01/2023 03:24, Yuji Ishikawa wrote:
> > Adds the Device Tree binding documentation that allows to describe the
> > Video Input Interface found in Toshiba Visconti SoCs.
> >
> > Signed-off-by: Yuji Ishikawa <[email protected]>
> > Reviewed-by: Nobuhiro Iwamatsu <[email protected]>
> > ---
> > Changelog v2:
> > - no change
> >
> > Changelog v3:
> > - no change
> >
> > Changelog v4:
> > - fix style problems at the v3 patch
> > - remove "index" member
> > - update example
> >
> > Changelog v5:
> > - no change
>
> No change? so all comments got ignored?
>
> This is a friendly reminder during the review process.
>
> It seems my previous comments were not fully addressed. Maybe my feedback
> got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all requested
> changes or keep discussing them.
I'm very sorry. I was upset about the recipient list and totally missed your comment.
I'll make a reply to v4 thread.
>
> Thank you.
>
> Best regards,
> Krzysztof
Regards,
Yuji
Hi Yuji,
Thank you for the patch.
On Wed, Jan 11, 2023 at 11:24:28AM +0900, Yuji Ishikawa wrote:
> Adds the Device Tree binding documentation that allows to describe
> the Video Input Interface found in Toshiba Visconti SoCs.
>
> Signed-off-by: Yuji Ishikawa <[email protected]>
> Reviewed-by: Nobuhiro Iwamatsu <[email protected]>
> ---
> Changelog v2:
> - no change
>
> Changelog v3:
> - no change
>
> Changelog v4:
> - fix style problems at the v3 patch
> - remove "index" member
> - update example
>
> Changelog v5:
> - no change
> ---
> .../bindings/media/toshiba,visconti-viif.yaml | 98 +++++++++++++++++++
> 1 file changed, 98 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
>
> diff --git a/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml b/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
> new file mode 100644
> index 00000000000..71442724d1a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/toshiba,visconti-viif.yaml
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/toshiba,visconti-viif.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Toshiba Visconti5 SoC Video Input Interface Device Tree Bindings
> +
> +maintainers:
> + - Nobuhiro Iwamatsu <[email protected]>
> +
> +description:
> + Toshiba Visconti5 SoC Video Input Interface (VIIF)
> + receives MIPI CSI2 video stream,
> + processes the stream with embedded image signal processor (L1ISP, L2ISP),
> + then stores pictures to main memory.
> +
> +properties:
> + compatible:
> + const: toshiba,visconti-viif
> +
> + reg:
> + items:
> + - description: registers for capture control
> + - description: registers for CSI2 receiver control
Nitpicking, s/registers/Registers/ in the two lines above as you
capitalize the descriptions below.
> +
> + interrupts:
> + items:
> + - description: Sync Interrupt
> + - description: Status (Error) Interrupt
> + - description: CSI2 Receiver Interrupt
> + - description: L1ISP Interrupt
> +
> + port:
> + $ref: /schemas/graph.yaml#/$defs/port-base
> + unevaluatedProperties: false
> + description: Input port, single endpoint describing the CSI-2 transmitter.
I would write
description:
CSI-2 input port, with a single endpoint connected to the CSI-2
transmitter.
> +
> + properties:
> + endpoint:
> + $ref: video-interfaces.yaml#
> + unevaluatedProperties: false
> +
> + properties:
> + data-lanes:
> + description: VIIF supports 2 or 4 data lines
s/lines/lanes/
> + $ref: /schemas/types.yaml#/definitions/uint32-array
You can drop this line, it's already handled by video-interfaces.yaml.
> + minItems: 1
> + maxItems: 4
If only 2 or 4 data lanes are supported, shouldn't minItems be 2 ?
> + items:
> + minimum: 1
> + maximum: 4
Can the CSI-2 receiver reorder the data lanes ? If not, I think you can
write
items:
- const: 1
- const: 2
- const: 3
- const: 4
> +
> + clock-lanes:
> + description: VIIF supports 1 clock line
s/line/lane/
> + const: 0
I would also add
clock-noncontinuous: true
link-frequencies: true
to indicate that the above two properties are used by this device.
Also, mark the properties that are required:
required:
- data-lanes
- clock-lanes
I'm wondering, though, if clock-lanes shouldn't be simply omitted. If
the hardware doesn't support any other option than using lane 0 for the
clock lane (as in, no lane remapping), then you can drop the clock-lanes
property completely.
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - port
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/interrupt-controller/irq.h>
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + viif@1c000000 {
> + compatible = "toshiba,visconti-viif";
> + reg = <0 0x1c000000 0 0x6000>,
> + <0 0x1c008000 0 0x400>;
> + interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
> +
> + port {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + csi_in0: endpoint {
> + remote-endpoint = <&imx219_out0>;
> + bus-type = <4>;
Does the hardware support any other bus type ? If not, you can drop the
bus-type. If it does, bus-type should be added to the binding, with the
value set to "const: 4".
> + data-lanes = <1 2>;
> + clock-lanes = <0>;
> + clock-noncontinuous;
> + link-frequencies = /bits/ 64 <456000000>;
> + };
> + };
> + };
> + };
--
Regards,
Laurent Pinchart
On 17/01/2023 16:26, Laurent Pinchart wrote:
>
>> +
>> + clock-lanes:
>> + description: VIIF supports 1 clock line
>
> s/line/lane/
>
>> + 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.
Best regards,
Krzysztof
Hi Krzysztof,
On Tue, Jan 17, 2023 at 04:42:51PM +0100, Krzysztof Kozlowski wrote:
> On 17/01/2023 16:26, Laurent Pinchart wrote:
> >
> >> +
> >> + clock-lanes:
> >> + description: VIIF supports 1 clock line
> >
> > s/line/lane/
> >
> >> + 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.
video-interfaces.yaml defines lots of properties applicable to
endpoints. For a given device, those properties should be required
(easy, that's defined in the bindings), optional, or forbidden. How do
we differentiate between the latter two cases ?
--
Regards,
Laurent Pinchart
On 17/01/2023 16:58, Laurent Pinchart wrote:
> Hi Krzysztof,
>
> On Tue, Jan 17, 2023 at 04:42:51PM +0100, Krzysztof Kozlowski wrote:
>> On 17/01/2023 16:26, Laurent Pinchart wrote:
>>>
>>>> +
>>>> + clock-lanes:
>>>> + description: VIIF supports 1 clock line
>>>
>>> s/line/lane/
>>>
>>>> + 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.
>
> video-interfaces.yaml defines lots of properties applicable to
> endpoints. For a given device, those properties should be required
required:
- foo
> (easy, that's defined in the bindings), optional,
by default (with unevaluatedProperties:false)
or explicitly mention "foo: true (with additionalProperties:false)
> or forbidden. How do
foo: false (with unevaluatedProperties:false)
or by default (with additionalProperties:false)
> we differentiate between the latter two cases ?
Best regards,
Krzysztof
Hi Krzysztof,
On Tue, Jan 17, 2023 at 06:01:27PM +0100, Krzysztof Kozlowski wrote:
> On 17/01/2023 16:58, Laurent Pinchart wrote:
> > On Tue, Jan 17, 2023 at 04:42:51PM +0100, Krzysztof Kozlowski wrote:
> >> On 17/01/2023 16:26, Laurent Pinchart wrote:
> >>>
> >>>> +
> >>>> + clock-lanes:
> >>>> + description: VIIF supports 1 clock line
> >>>
> >>> s/line/lane/
> >>>
> >>>> + 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.
> > video-interfaces.yaml defines lots of properties applicable to
> > endpoints. For a given device, those properties should be required
>
> required:
> - foo
>
> > (easy, that's defined in the bindings), optional,
>
> by default (with unevaluatedProperties:false)
> or explicitly mention "foo: true (with additionalProperties:false)
>
> > 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.
> > we differentiate between the latter two cases ?
--
Regards,
Laurent Pinchart
> -----Original Message-----
> From: Laurent Pinchart <[email protected]>
> Sent: Monday, January 23, 2023 4:26 AM
> To: Krzysztof Kozlowski <[email protected]>
> Cc: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <[email protected]>; Hans Verkuil <[email protected]>; Mauro
> Carvalho Chehab <[email protected]>; iwamatsu nobuhiro(岩松 信洋 □Ｓ
> ＷＣ◯ＡＣＴ) <[email protected]>; Rob Herring
> <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Rafael J . Wysocki
> <[email protected]>; Mark Brown <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH v5 1/6] dt-bindings: media: platform: visconti: Add Toshiba
> Visconti Video Input Interface bindings
>
> Hi Krzysztof,
>
> On Tue, Jan 17, 2023 at 06:01:27PM +0100, Krzysztof Kozlowski wrote:
> > On 17/01/2023 16:58, Laurent Pinchart wrote:
> > > On Tue, Jan 17, 2023 at 04:42:51PM +0100, Krzysztof Kozlowski wrote:
> > >> On 17/01/2023 16:26, Laurent Pinchart wrote:
> > >>>
> > >>>> +
> > >>>> + clock-lanes:
> > >>>> + description: VIIF supports 1 clock line
> > >>>
> > >>> s/line/lane/
Sorry for a late reply.
I'll fix the description.
> > >>>
> > >>>> + 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.
>
> > > video-interfaces.yaml defines lots of properties applicable to
> > > endpoints. For a given device, those properties should be required
> >
> > required:
> > - foo
> >
> > > (easy, that's defined in the bindings), optional,
> >
> > by default (with unevaluatedProperties:false) or explicitly mention
> > "foo: true (with additionalProperties:false)
> >
> > > 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?
Are these changes also applied to "port", which is the parent node of the "endpoint" ?
> > > we differentiate between the latter two cases ?
>
> --
> Regards,
>
> Laurent Pinchart
Regards,
Yuji Ishikawa
On Mon, Jan 30, 2023 at 09:06:25AM +0000, [email protected] wrote:
> On Monday, January 23, 2023 4:26 AM, Laurent Pinchart wrote:
> > On Tue, Jan 17, 2023 at 06:01:27PM +0100, Krzysztof Kozlowski wrote:
> > > On 17/01/2023 16:58, Laurent Pinchart wrote:
> > > > On Tue, Jan 17, 2023 at 04:42:51PM +0100, Krzysztof Kozlowski wrote:
> > > >> On 17/01/2023 16:26, Laurent Pinchart wrote:
> > > >>>
> > > >>>> +
> > > >>>> + clock-lanes:
> > > >>>> + description: VIIF supports 1 clock line
> > > >>>
> > > >>> s/line/lane/
>
> Sorry for a late reply.
> I'll fix the description.
>
> > > >>>
> > > >>>> + 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.
> >
> > > > video-interfaces.yaml defines lots of properties applicable to
> > > > endpoints. For a given device, those properties should be required
> > >
> > > required:
> > > - foo
> > >
> > > > (easy, that's defined in the bindings), optional,
> > >
> > > by default (with unevaluatedProperties:false) or explicitly mention
> > > "foo: true (with additionalProperties:false)
> > >
> > > > 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).
> > > > we differentiate between the latter two cases ?
--
Regards,
Laurent Pinchart
> -----Original Message-----
> From: Laurent Pinchart <[email protected]>
> Sent: Wednesday, February 1, 2023 6:46 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> 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, [email protected] wrote:
> > On Monday, January 23, 2023 4:26 AM, Laurent Pinchart wrote:
> > > On Tue, Jan 17, 2023 at 06:01:27PM +0100, Krzysztof Kozlowski wrote:
> > > > On 17/01/2023 16:58, Laurent Pinchart wrote:
> > > > > On Tue, Jan 17, 2023 at 04:42:51PM +0100, Krzysztof Kozlowski wrote:
> > > > >> On 17/01/2023 16:26, Laurent Pinchart wrote:
> > > > >>>
> > > > >>>> +
> > > > >>>> + clock-lanes:
> > > > >>>> + description: VIIF supports 1 clock line
> > > > >>>
> > > > >>> s/line/lane/
> >
> > Sorry for a late reply.
> > I'll fix the description.
> >
> > > > >>>
> > > > >>>> + 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.
> > >
> > > > > video-interfaces.yaml defines lots of properties applicable to
> > > > > endpoints. For a given device, those properties should be
> > > > > required
> > > >
> > > > required:
> > > > - foo
> > > >
> > > > > (easy, that's defined in the bindings), optional,
> > > >
> > > > by default (with unevaluatedProperties:false) or explicitly
> > > > mention
> > > > "foo: true (with additionalProperties:false)
> > > >
> > > > > 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).
All right. I'll apply the change to "endpoint".
> > > > > we differentiate between the latter two cases ?
>
> --
> Regards,
>
> Laurent Pinchart
Regards,
Yuji Ishikawa