Re: [PATCH v6 2/5] dt-bindings: media: camss: Add qcom,sdm670-camss

[Krzysztof Kozlowski <krzk@kernel.org>]

On 30/10/2024 15:06, Vladimir Zapolskiy wrote:
> Hi Krzysztof,
> 
> On 10/11/24 17:29, Krzysztof Kozlowski wrote:
>> On Fri, Oct 11, 2024 at 10:14:49AM +0300, Vladimir Zapolskiy wrote:
>>>> +    soc {
>>>> +        #address-cells = <2>;
>>>> +        #size-cells = <2>;
>>>> +
>>>> +        camss@ac65000 {
>>>> +            compatible = "qcom,sdm670-camss";
>>>> +
>>>> +            reg = <0 0x0acb3000 0 0x1000>,
>>>
>>> This is immediately wrong, unit address shall be the same as the address of the
>>> first value of reg property.
>>>
>>> I still object to the sorting order of reg values dictated by reg-names property.
>>>
>>> There are a few recently added CAMSS device tree binding descriptions, where
>>> reg values are sorted by address values without a connection to another property
>>> values, and I believe this is the correct way to go.
>>>
>>> Two most recently added CAMSS IP descriptions (qcom,sm8250-camss.yaml and
>>> qcom,sc8280xp-camss.yaml) do implement sorting by reg values, I believe from now on
>>> it should be assumed that all subsequently added CAMSS IP descriptions to follow
>>> the same established policy.
>>
>> Heh, sc8280xp introduced entirely different sorting also in interrupt-names.
>>
>> Just look at interrupts of sm8250 and sc8280xp. Luckily clocks are still
>> keeping style.
>>
>> Can you start keeping consistency? All bindings from the same family of
>> devices, especially if they share something, should have similar order
>> in lists.
>>
>> How do you imagine writing drivers and request items by order (not by
>> name) if the order is different in each flavor?
> 
> I don't see a problem here, and I don't remember any reports about this
> kind of problem while adding CAMSS support in the driver to new platforms.

And I see problem, would create enormous probe code to handle different
variants for clock[0] and then clock[1], etc.

> 
> While the problem of improper CAMSS unit address appears again and again,
> the focus shall be on removing a chance to make a commin mistake here.

This is not a problem. Tools already point it out. Order of reg-names
also does not affect that, you can put fake unit address regardless of
the order of reg-names items.

> 
> As I've already said above, device tree bindings of CAMSS in two most
> recently added platforms sm8250 and sc8280xp follow the numerical order
> of addresses from reg value. This becomes the policy.
> 
> Sorting lists of interrupts or clocks by numerical values makes no sense,
> thus the argument of *-names sorting becomes valid here. For clarity, reg

There is no such argument, no such coding style.

> property is very special, also a snippet of its value goes as a unit
> address.

And order of items does not matter for above "specialness of reg".

Best regards,
Krzysztof