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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 30/10/2024 23:13, Vladimir Zapolskiy wrote:
> On 10/30/24 23:06, Rob Herring wrote:
>> On Wed, Oct 30, 2024 at 9:20 AM Vladimir Zapolskiy
>> <vladimir.zapolskiy@linaro.org> wrote:
>>>
>>> Hi Rob.
>>>
>>> On 10/11/24 17:41, Rob Herring wrote:
>>>> On Fri, Oct 11, 2024 at 09:31:06AM +0100, Bryan O'Donoghue wrote:
>>>>> On 11/10/2024 08:14, Vladimir Zapolskiy wrote:
>>>>>>
>>>>>> 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.
>>>>>
>>>>> My preference is sort by address not sort by name => we sort the device
>>>>> nodes themselves by address so it seems more consistent to sort by address
>>>>> inside of the devices too.
>>>>
>>>> Strictly speaking, the values of addresses are unknown to the binding,
>>>> so you can't sort by address. However, if something is truly a single
>>>> block, then the offsets are probably fixed in order by offset makes
>>>> sense. But when a block is changed, any rule on sorting may go out
>>>> the window since we add new regions on the end.
>>>
>>> Exactly, and this is an argument why the sorting is a subject to a device
>>> driver policy, kind of any sorting order is equally bad. Sorting 'reg'
>>> values by addresses helps to avoid a notorious problem with unit addresses.
>>
>> What notorious problem?
>>
> 
> Here the problem I reference to is the problem of an incorrespondence between
> device tree node unit address and the address of the first value of 'reg'
> values.

Sorting by unit address does not solve it. Sorting by anything else does
not cause it. You repeat this statement multiple times already. Sorry,
it's not true. You can sort by address and still put wrong unit address.

Tools address it already, not sorting.

> 
> Having a sorting by addresses allows to grasp IO ranges easily, and setting
> device tree node unit addresses to some almost arbitrary chosen value from
> the middle of IP's IO range is suspicious and confusing in my opinion.

It's not arbitrary. It's the main, control address space often. At least
it should be. Your sorting rule enforces putting irrelevant address as
unit address, imagine:

isp@1000 {
reg = <0x1000 0x4>, <0x2000, 0x1000>;
reg-names = "reset-control-block-one-unimportant-register",
            "main-control-block-with-99%-of-registers";

Sorry, this is just wrong coding style.

Best regards,
Krzysztof