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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 20/11/2025 13:01, Michael Tretter wrote:
> 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?

Yes, you need to make it explicit. All constraints, except min=maxItems,
which you imply here, must be explicit.

> 
>>
>>>
>>> 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?

Probably, lost context, because I asked you to do that but it felt like
you keep discussing. So if you implement reviewer's suggestion, it is
enough to say "ack" or "ok".


Best regards,
Krzysztof