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

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

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.

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.

>>
>>> This one in particular I have to wonder why csiphy is not a separate
>>> node.
>>
>> There were dicussions about it in the past, and kind of enforced outcome of
>> the discussions is to keep all CAMSS IP components together under one huge
>> plain device tree node. I personally dislike this approach, but obedience
>> is the way to get things merged.
> 
> Who are you saying would be in the way to get things merged? DT
> maintainers? I feel certain I would have pushed for separate blocks,
> but I'll defer to people that know the h/w. I can't learn the details
> of everyone's h/w. If they get it wrong, it's their problem not mine.

I had this discussion with Qualcomm/CAMSS maintainers long time ago, it
may be restarted, if there is a necessity.

>>>> Which means sorting reg by address and irq too.
>>>
>>> IRQs make little sense to sort IMO.
>>
>> For all non-reg properties with a present *-names property the sorting
>> order should be done by *-names property. Only 'reg' is very special.
> 
> No. If you had 'main' and 'error', I'd put 'main' first. If they are
> somewhat equal (e.g. rx, tx), then sure, sort them however you like
> (assuming no existing binding). The only real rules here are how new
> entries should be added (on the end). Otherwise, there is no policy.
> 

Here in the proposed terms the start of an IO region is 'main', while
some value in the middle of it (the first one in alphabetical sorting)
is too secondary to dictate the device tree node unit address, I believe.

--
Best wishes,
Vladimir