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
