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

[Biju Das <biju.das.jz@xxxxxxxxxxxxxx>]

Hi Krzysztof Kozlowski,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Sent: 17 December 2024 07:51
> Subject: Re: [PATCH v4 1/7] dt-bindings: pinctrl: renesas: Add alpha-numerical port support for RZ/V2H
> 
> 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@xxxxxx
> > esas.com/
> 
> 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.

The header files are part of DT bindings. So if it is wrong, why the 
Commit "997daa8de64ccbb" "dt-bindings: clock: add ExynosAuto v920 SoC CMU bindings"
is part of bindings?



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

Bu the definitions are part of bindings just like Commit "997daa8de64ccbb".

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

I agree. Macro converted to a number which binding care of.

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

On RZ/G2L all ports are in numbers not an issue. But on RZ/V2H an RZ/G3E
hardware manual just talks about Port {0..8} {A..H}{J..M}{S}. Hardware constant 28 is just derived one.

A device user just refer, hardware manual and pinctrl list and put the definitions on binding.
He does not need to undergo mapping for alpha numeric to hardware index conversion.


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

OK, It is just definitions, so you mean it has to merge with driver + dts patch. so it won't
create any confusion and we can ignore check patch warning, "binding patch should be
Separate patch"

What about then merging this patch with [2] and [3] similar to [4], 

[2] https://lore.kernel.org/all/20241216195325.164212-4-biju.das.jz@xxxxxxxxxxxxxx/
[3] https://lore.kernel.org/all/20241216195325.164212-6-biju.das.jz@xxxxxxxxxxxxxx/

[4] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/include/dt-bindings?h=next-20241217&id=ecc79ab919ec54c658fb14f955c76872119829b8

Cheers,
Biju