Re: [PATCH v3 5/8] pinctrl: renesas: rzg2l: Add OEN support for RZ/G3L

[Geert Uytterhoeven]

Hi Biju,

On Tue, 17 Mar 2026 at 11:16, Biju <biju.das.au@xxxxxxxxx> wrote:
> From: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> Add support for configuring the ETH_MODE register on the RZ/G3L SoC to
> enable output-enable control for specific pins. On this SoC, certain
> pins such as P{B,E}1_ISO need to support switching between input and
> output modes depending on the PHY interface mode (e.g., RMII vs RGMII).
> This functionality maps to the 'output-enable' property in the device
> tree and requires explicit control via the ETH_MODE register.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>

Thanks for your patch!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -1198,6 +1198,23 @@ static int rzg2l_write_oen(struct rzg2l_pinctrl *pctrl, unsigned int _pin, u8 oe
>         return 0;
>  }
>
> +static int rzg3l_pin_to_oen_bit(struct rzg2l_pinctrl *pctrl, unsigned int _pin)
> +{
> +       u64 *pin_data = pctrl->desc.pins[_pin].drv_data;
> +       u8 port, pin;
> +
> +       if (*pin_data & RZG2L_SINGLE_PIN)
> +               return -EINVAL;
> +
> +       pin = RZG2L_PIN_ID_TO_PIN(_pin);
> +       if (pin != pctrl->data->hwcfg->oen_max_pin)
> +               return -EINVAL;
> +
> +       port = RZG2L_PIN_ID_TO_PORT(_pin);
> +
> +       return (port == pctrl->data->hwcfg->oen_max_port) ? 1 : 0;

IMHO these oen_max_* comparisons are very obscure. For the casual
reader, it is far from obvious there are only two valid combinations.

Why not use the same logic as on RZ/G2L?

        /*
         * We can determine which Ethernet interface we're dealing with from
         * the caps.
         */
        if (caps & PIN_CFG_IO_VMC_ETH0)
                return 0;
        if (caps & PIN_CFG_IO_VMC_ETH1)
                return 1;

        return -EINVAL;

I think you can even reuse rzg2l_pin_to_oen_bit()?

> +}
> +
>  static int rzg3s_pin_to_oen_bit(struct rzg2l_pinctrl *pctrl, unsigned int _pin)
>  {
>         u64 *pin_data = pctrl->desc.pins[_pin].drv_data;

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