Re: [PATCH v4 1/7] dt-bindings: pinctrl: renesas: Add alpha-numerical port support for RZ/V2H

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On 17/12/2024 08:29, Biju Das wrote:
> Hi Krzysztof Kozlowski,
> 
> Thanks for the feedback.
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
>> Sent: 17 December 2024 06:32
>> Subject: Re: [PATCH v4 1/7] dt-bindings: pinctrl: renesas: Add alpha-numerical port support for RZ/V2H
>>
>> On Mon, Dec 16, 2024 at 07:53:11PM +0000, Biju Das wrote:
>>> RZ/V2H has ports P0-P9 and PA-PB. Add support for defining
>>> alpha-numerical ports in DT using RZV2H_* macros.
>>
>> So this is only for DT? Not really a binding. Binding binds driver implementation with DTS and you do
>> not have here driver.
> 
> Please see patch [1], see how this definition binds driver implementation with DTS
> 
> [1] https://lore.kernel.org/all/20241216195325.164212-4-biju.das.jz@xxxxxxxxxxxxxx/

I don't know what is this patch, it is not part of these series
addressed to me and commit msg says "in DT". If you want to receive
meaningful review, make it easier for reviewers.


> 
>>
>> Calling it a binding makes it immutable and gives us, DT maintainers, more work, so really no benefits
>> at all.
> 
>>
>> I guess other DT maintainers will ack it, I prefer to reduce number of headers.
> 
> DT describes hardware. The port names are alpha numeric on hardware manual.

We talk about binding, not DT.

> 
> For example, consider the case of  hardware pin PS1 mentioned in hardware manual.
> 
> With current changes,
> pinmux = <RZG3E_PORT_PINMUX(S, 1, 0)>;
> 
> With existing code
> pinmux = <RZG3E_PORT_PINMUX(28, 1, 0)>;

Based on this pure code: still not a binding.

> 
> What do you prefer here? 28 is just a number derived from hardware indices

Let me ask rhetorical question: if 28 hardware constant is suitable for
binding, then why are you not defining GPIO numbers, IRQ numbers and
MMIO addresses as bindings as well?

> Or actual port name PS1 as mentioned in hardware manual?

Well, I don't know. Commit says DTS, no driver patches here in my inbox,
so what do I know?

Best regards,
Krzysztof