Re: [PATCH v2 1/4] media: dt-bindings: adi,adv7180: add VPP and CSI register maps

[Michael Tretter <m.tretter@pengutronix.de>]

On Thu, 20 Nov 2025 10:05:43 +0100, Krzysztof Kozlowski wrote:
> On 20/11/2025 09:53, Michael Tretter wrote:
> > On Thu, 20 Nov 2025 09:04:48 +0100, Krzysztof Kozlowski wrote:
> >> On Wed, Nov 19, 2025 at 05:25:51PM +0100, Michael Tretter wrote:
> >>> diff --git a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> >>> index dee8ce7cb7ba..dbbbe76291bc 100644
> >>> --- a/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> >>> +++ b/Documentation/devicetree/bindings/media/i2c/adi,adv7180.yaml
> >>> @@ -30,7 +30,27 @@ properties:
> >>>            - adi,adv7282-m
> >>>  
> >>>    reg:
> >>> -    maxItems: 1
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - description: main register map
> >>> +      - description: CSI register map
> >>> +      - description: VPP register map
> >>> +    description:
> >>> +      The ADV7180 family may have up to three register maps. All chips have
> >>> +      the main register map. The availability of the CSI and VPP register maps
> >>> +      depends on the chip variant.
> >>> +
> >>> +      The addresses of the CSI and VPP register maps are programmable by
> >>> +      software. They depend on the board layout and other devices on the I2C
> >>> +      bus and are determined by the hardware designer to avoid address
> >>> +      conflicts on the I2C bus.
> >>> +
> >>> +  reg-names:
> >>> +    minItems: 1
> >>> +    items:
> >>> +      - const: main
> >>> +      - enum: [ csi, vpp ]
> >>> +      - enum: [ csi, vpp ]
> >>
> >> Last entry must be:
> >>
> >> const: vpp
> >>
> >> We do not allow flexible order... but the problem is that your if:then:
> >> does not match above at all. You do not have three items anywhere.
> > 
> > I'm not entirely sure, if I correctly understand that comment.
> > 
> > The adi,adv7280-m and adi,adv7282-m have all three items and don't need
> > an if:then:. Do I have explicitly define the binding with three items,
> > too?
> 
> Which comment? That third item cannot be csi? What is odd here?
> 
> 
> > 
> > The chip has the following variants:
> > 
> > adi,adv7180:     main
> > adi,adv7180cp:   main
> > adi,adv7180st:   main
> > adi,adv7182:     main
> > adi,adv7280:     main, vpp
> > adi,adv7280-m:   main, csi, vpp
> > adi,adv7281:     main, csi
> > adi,adv7281-m:   main, csi
> > adi,adv7281-ma:  main, csi
> > adi,adv7282:     main, vpp
> > adi,adv7282-m:   main, csi, vpp
> 
> So where is csi as third item?
> 
> Anyway, you also miss minItems in your if:then: cases.

The minItems in the if:then: cases would be the same as in the top level
definition. Do I have to override the minItems if it's the same?

> 
> > 
> > If I make the last entry (vpp) const, I allow exactly these variants.
> > 
> > For the adi,adv7280-m compatible, the following combinations would be
> > valid or invalid:
> > 
> > adi,adv7280-m:   main
> > 
> > is valid, because only main is mandatory. For csi and vpp, the default
> > addresses are used.
> > 
> > adi,adv7280-m:   main, vpp
> > 
> > is valid, because the second entry may be vpp. For csi, the default
> > address is used.
> > 
> > adi,adv7280-m:   main, vpp, csi
> > 
> > is invalid, because the entries must be in the defined order, and
> > flexible order is not possible.
> > 
> > Is this correct and matches the binding definition?
> 
> It does not match your code.

If I change the last entry to

	- const: csi

to disallow vpp as a third item (which is compliant to the example), the
binding is fine. Correct?

Michael