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

Commit Message

Yuji Ishikawa Jan. 11, 2023, 2:24 a.m. UTC

Adds the Device Tree binding documentation that allows to describe
the Video Input Interface found in Toshiba Visconti SoCs.

Signed-off-by: Yuji Ishikawa <[e-mail]>
Reviewed-by: Nobuhiro Iwamatsu <[e-mail]>
---
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

Comments

Krzysztof Kozlowski Jan. 11, 2023, 9:19 a.m. UTC | #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 <[e-mail]>
> Reviewed-by: Nobuhiro Iwamatsu <[e-mail]>
> ---
> 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

Yuji Ishikawa Jan. 11, 2023, 12:48 p.m. UTC | #2

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Wednesday, January 11, 2023 6:20 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <[e-mail]>; Hans Verkuil <[e-mail]>; Laurent
> Pinchart <[e-mail]>; Mauro Carvalho Chehab
> <[e-mail]>; iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> <[e-mail]>; Rob Herring <[e-mail]>;
> Krzysztof Kozlowski <[e-mail]>; Rafael J . Wysocki
> <[e-mail]>; Mark Brown <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> 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 <[e-mail]>
> > Reviewed-by: Nobuhiro Iwamatsu <[e-mail]>
> > ---
> > 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

Laurent Pinchart Jan. 17, 2023, 3:26 p.m. UTC | #3

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 <[e-mail]>
> Reviewed-by: Nobuhiro Iwamatsu <[e-mail]>
> ---
> 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 <[e-mail]>
> +
> +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>;
> +                };
> +            };
> +        };
> +    };

Krzysztof Kozlowski Jan. 17, 2023, 3:42 p.m. UTC | #4

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

Laurent Pinchart Jan. 17, 2023, 3:58 p.m. UTC | #5

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 ?

Krzysztof Kozlowski Jan. 17, 2023, 5:01 p.m. UTC | #6

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

Laurent Pinchart Jan. 22, 2023, 7:25 p.m. UTC | #7

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 ?

Yuji Ishikawa Jan. 30, 2023, 9:06 a.m. UTC | #8

> -----Original Message-----
> From: Laurent Pinchart <[e-mail]>
> Sent: Monday, January 23, 2023 4:26 AM
> To: Krzysztof Kozlowski <[e-mail]>
> Cc: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <[e-mail]>; Hans Verkuil <[e-mail]>; Mauro
> Carvalho Chehab <[e-mail]>; iwamatsu nobuhiro(岩松 信洋 □Ｓ
> ＷＣ◯ＡＣＴ) <[e-mail]>; Rob Herring
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Rafael J . Wysocki
> <[e-mail]>; Mark Brown <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> 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

Laurent Pinchart Feb. 1, 2023, 9:45 a.m. UTC | #9

On Mon, Jan 30, 2023 at 09:06:25AM +0000, [e-mail] 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 ?

Yuji Ishikawa Feb. 1, 2023, 11:24 a.m. UTC | #10

> -----Original Message-----
> From: Laurent Pinchart <[e-mail]>
> Sent: Wednesday, February 1, 2023 6:46 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ)
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> 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, [e-mail] 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

Patch

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 <[e-mail]>
+
+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>;
+                };
+            };
+        };
+    };