Re: [PATCH v3 2/7] dt-bindings: pinctrl: renesas: Document RZ/G3E SoC

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Biju,

On Fri, Dec 13, 2024 at 6:39 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> Add documentation for the pin controller found on the Renesas RZ/G3E
> (R9A09G047) SoC. The RZ/G3E PFC is similar to the RZ/V2H SoC but has more
> pins(P00-PS3).
>
> Acked-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> ---
> v2->v3:
>  * Updated the macros with hardware indices in the hardware manual.
>  * The changes are trivial, so retained ack tag from Conor.

Thanks for the update!

> --- a/include/dt-bindings/pinctrl/rzg2l-pinctrl.h
> +++ b/include/dt-bindings/pinctrl/rzg2l-pinctrl.h
> @@ -24,16 +24,35 @@
>  #define PORT_P9                9
>  #define PORT_PA                10
>  #define PORT_PB                11
> +#define PORT_PC                12
> +#define PORT_PD                13
> +#define PORT_PE                14
> +#define PORT_PF                15
> +#define PORT_PG                16
> +#define PORT_PH                17
> +#define PORT_PI                18

Port PI does not exist on any supported SoC.
Hence please drop it, so DTS writers cannot use it by accident.

> +#define PORT_PJ                19
> +#define PORT_PK                20
> +#define PORT_PL                21
> +#define PORT_PM                22
> +#define PORT_PN                23
> +#define PORT_PO                24
> +#define PORT_PP                25
> +#define PORT_PQ                26
> +#define PORT_PR                27

Same for ports PN-PR.

I understand you need to keep the definition for PORT_P9, as it is
shared with RZ/V2H.

However, that could be fixed by having separate RZV2H_P* and RZG3E_P*
port definitions, like you had for RZ/G3E in v2. You already have
SoC-specific *_PORT_PINMUX() and *_GPIO() macros below.
A disadvantage is that it may grow this file when new SoCs are added.
But that can be mitigated by splitting it in multiple files:

--- include/dt-bindings/pinctrl/renesas,r9a09g047-pinctrl.h ---

    #include <dt-bindings/pinctrl/rzg2l-pinctrl.h>

    #define RZG3E_...
    ...

--- include/dt-bindings/pinctrl/renesas,r9a09g057-pinctrl.h ---

    #include <dt-bindings/pinctrl/rzg2l-pinctrl.h>

    #define RZV2H_...
    ...

What do you think?

> +#define PORT_PS                28
>
>  /*
>   * Create the pin index from its bank and position numbers and store in
>   * the upper 16 bits the alternate function identifier
>   */
>  #define RZG2L_PORT_PINMUX(b, p, f)     ((b) * RZG2L_PINS_PER_PORT + (p) | ((f) << 16))
> +#define RZG3E_PORT_PINMUX(b, p, f)     RZG2L_PORT_PINMUX(PORT_P##b, p, f)
>  #define RZV2H_PORT_PINMUX(b, p, f)     RZG2L_PORT_PINMUX(PORT_P##b, p, f)
>
>  /* Convert a port and pin label to its global pin index */
>  #define RZG2L_GPIO(port, pin)  ((port) * RZG2L_PINS_PER_PORT + (pin))
> +#define RZG3E_GPIO(port, pin)  RZG2L_GPIO(PORT_P##port, pin)
>  #define RZV2H_GPIO(port, pin)  RZG2L_GPIO(PORT_P##port, pin)
>
>  #endif /* __DT_BINDINGS_RZG2L_PINCTRL_H */

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds