Re: [PATCH v2 61/66] dt-bindings: media: Add Allwinner A31 ISP bindings documentation

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 4730 bytes --]

Hi,

On Fri 11 Feb 22, 22:52, Laurent Pinchart wrote:
> Hi Rob,
> 
> On Fri, Feb 11, 2022 at 09:13:30AM -0600, Rob Herring wrote:
> > On Mon, Feb 07, 2022 at 05:51:21PM +0200, Laurent Pinchart wrote:
> > > On Sat, Feb 05, 2022 at 07:54:24PM +0100, Paul Kocialkowski wrote:
> > > > This introduces YAML bindings documentation for the Allwinner A31 Image
> > > > Signal Processor (ISP).
> > > > 
> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > ---
> > > >  .../media/allwinner,sun6i-a31-isp.yaml        | 117 ++++++++++++++++++
> > > >  1 file changed, 117 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
> > > > new file mode 100644
> > > > index 000000000000..2d87022c43ce
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun6i-a31-isp.yaml
> > > > @@ -0,0 +1,117 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/media/allwinner,sun6i-a31-isp.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Allwinner A31 Image Signal Processor Driver (ISP) Device Tree Bindings
> > > > +
> > > > +maintainers:
> > > > +  - Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - allwinner,sun6i-a31-isp
> > > > +      - allwinner,sun8i-v3s-isp
> > > > +
> > > > +  reg:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  clocks:
> > > > +    items:
> > > > +      - description: Bus Clock
> > > > +      - description: Module Clock
> > > > +      - description: DRAM Clock
> > > 
> > > That's interesting, does the ISP have a dedicated DRAM ?
> > > 
> > > > +
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: bus
> > > > +      - const: mod
> > > > +      - const: ram
> > > > +
> > > > +  resets:
> > > > +    maxItems: 1
> > > > +
> > > > +  ports:
> > > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > > +
> > > > +    properties:
> > > > +      port@0:
> > > > +        $ref: /schemas/graph.yaml#/$defs/port-base
> > > > +        description: CSI0 input port
> > > > +
> > > > +        properties:
> > > > +          reg:
> > > > +            const: 0
> > > > +
> > > > +          endpoint:
> > > > +            $ref: video-interfaces.yaml#
> > > > +            unevaluatedProperties: false
> > > 
> > > If no other property than remote-endpoint are allowed, I'd write
> > > 
> > >           endpoint:
> > >             $ref: video-interfaces.yaml#
> > > 	    remote-endpoint: true
> > 
> > You just mixed a node and a property...
> 
> Yes, I meant
> 
>            endpoint:
>              $ref: video-interfaces.yaml#
>              properties:
>                remote-endpoint: true
> 
> and actually add
> 
>              additionalProperties: false
> 
> > 'remote-endpoint' is always allowed, so need to put it here and every 
> > other user. So 'unevaluatedProperties' is correct. But it would be good 
> > to define what properties from video-interfaces.yaml are used here.
> 
> I've been looking at this recently. The usual pattern is to write
> 
>     endpoint:
>       $ref: video-interfaces.yaml#
>       unevaluatedProperties: false
>       properties:
>         hsync-polarity: true
>         vsync-polarity: true
> 
> to express that the hsync-polarity and vsync-polarity properties are
> used. However, this will still validate fine if, for instance,
> data-lanes was specified in the device tree. Shouldn't we use
> additionalProperties instead of unevaluatedProperties here ? If so,
> specifying remote-endpoint: true seems needed.

My understanding is that unevaluatedProperties well allow all properties
defined in the included video-interfaces.yaml ref but reject others
while additionalProperties will reject any unspecified local property,
even if it is declared in the ref.

In any case with the ISP maybe we don't even want to take the ref from
video-interfaces.yaml since we are dealing with an internal fifo between
two devices. Maybe it would be more appropriate to ref
/schemas/graph.yaml#/$defs/endpoint-base, which already defines
remote-endpoint too.

What do you think?

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]