Re: [PATCH] pinctrl: renesas: rzg2l: Add GPIO set_config

[Claudiu Beznea]

Hi, Geert,

On 3/24/26 19:28, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Mon, 16 Mar 2026 at 11:19, claudiu beznea <claudiu.beznea@xxxxxxxxx> wrote:
> > On 3/13/26 15:15, Geert Uytterhoeven wrote:
> > > On Wed, 18 Feb 2026 at 16:19, Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
> > > > From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> > > >
> > > > Add GPIO set_config to allow setting GPIO specific functionalities.
> > > >
> > > > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> > >
> > > Thanks for your patch!
> > >
> > > > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > > > @@ -1848,6 +1848,25 @@ static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int offset)
> > > >           rzg2l_gpio_direction_input(chip, offset);
> > > >    }
> > > >
> > > > +static int rzg2l_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
> > > > +                                unsigned long config)
> > > > +{
> > > > +       switch (pinconf_to_config_param(config)) {
> > > > +       case PIN_CONFIG_BIAS_DISABLE:
> > > > +       case PIN_CONFIG_BIAS_PULL_UP:
> > > > +       case PIN_CONFIG_BIAS_PULL_DOWN:
> > > > +       case PIN_CONFIG_DRIVE_OPEN_DRAIN:
> > > > +       case PIN_CONFIG_DRIVE_PUSH_PULL:
> > > > +       case PIN_CONFIG_SLEW_RATE:
> > > > +       case PIN_CONFIG_DRIVE_STRENGTH:
> > > > +       case PIN_CONFIG_DRIVE_STRENGTH_UA:
> > > > +       case PIN_CONFIG_POWER_SOURCE:
> > >
> > > Shouldn't you handle all types that are supported by
> > > rzg2l_pinctrl_pinconf_[gs]et()?
> > >
> > > The following are missing:
> > > PIN_CONFIG_INPUT_ENABLE
> > > PIN_CONFIG_OUTPUT_ENABLE
> > > PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS
> > > PIN_CONFIG_INPUT_SCHMITT_ENABLE
> > > RENESAS_RZV2H_PIN_CONFIG_OUTPUT_IMPEDANCE
> >
> > I'll add these as well.
>
> Apparently you can't just add RENESAS_RZV2H_PIN_CONFIG_OUTPUT_IMPEDANCE
> to the switch statement, as gcc requires all case statements to use values
> that are actually defined in the enum:
>
>      drivers/pinctrl/renesas/pinctrl-rzg2l.c:2072:9: error: case value
> ‘128’ not in enumerated type ‘enum pin_config_param’ [-Werror=switch]
>
> As the documentation states this range is meant for custom
> configurations:
>
>       * @PIN_CONFIG_END: this is the last enumerator for pin configurations, if
>       *      you need to pass in custom configurations to the pin controller, use
>       *      PIN_CONFIG_END+1 as the base offset.
>       * @PIN_CONFIG_MAX: this is the maximum configuration value that can be
>       *      presented using the packed format.
>
> I fixed that by replacing the enum by u8 in the conversion macros:
>
>      --- a/include/linux/pinctrl/pinconf-generic.h
>      +++ b/include/linux/pinctrl/pinconf-generic.h
>      @@ -173,9 +173,9 @@ enum pin_config_param {
>        * upper 24 bits.
>        */
>
>      -static inline enum pin_config_param
> pinconf_to_config_param(unsigned long config)
>      +static inline u8 pinconf_to_config_param(unsigned long config)
>       {
>      -       return (enum pin_config_param) (config & 0xffUL);
>      +       return config & 0xffUL;
>       }
>
>       static inline u32 pinconf_to_config_argument(unsigned long config)
>      @@ -183,8 +183,7 @@ static inline u32
> pinconf_to_config_argument(unsigned long config)
>              return (u32) ((config >> 8) & 0xffffffUL);
>       }
>
>      -static inline unsigned long pinconf_to_config_packed(enum
> pin_config_param param,
>      -                                                    u32 argument)
>      +static inline unsigned long pinconf_to_config_packed(u8 param,
> u32 argument)
>       {
>              return PIN_CONF_PACKED(param, argument);
>       }
>
> Probably a few more should be updated, too?
>
> > > > @@ -2819,6 +2838,7 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
> > > >         chip->direction_output = rzg2l_gpio_direction_output;
> > > >         chip->get = rzg2l_gpio_get;
> > > >         chip->set = rzg2l_gpio_set;
> > > > +       chip->set_config = rzg2l_gpio_set_config;
> > > >         chip->label = name;
> > > >         chip->parent = pctrl->dev;
> > > >         chip->owner = THIS_MODULE;
>
> This change breaks pin control and GPIO on RZ/Five:
>
>      -pinctrl-rzg2l 11030000.pinctrl: pinctrl-rzg2l support registered
>      +gpio gpiochip0: (11030000.pinctrl): setup of own GPIO can0_stb failed
>      +requesting hog GPIO can0_stb (chip 11030000.pinctrl, offset 18) failed, -95
>      +gpiochip_add_data_with_key: GPIOs 512..743 (11030000.pinctrl)
> failed to register, -95
>      +pinctrl-rzg2l 11030000.pinctrl: error -EOPNOTSUPP: failed to add
> GPIO controller
>      +pinctrl-rzg2l 11030000.pinctrl: error -EOPNOTSUPP: failed to add GPIO chip
>      +pinctrl-rzg2l 11030000.pinctrl: probe with driver pinctrl-rzg2l
> failed with error -95

Thank you for looking to this. I haven't experimented this with my RZ/G2L desk 
board but looking on other RZ/G2L board that I have access to (not on my desk) I 
can see similar errors even on RZ/G2L. I'll look into it.

Thank you,
Claudiu

> For the can0_stb hog, rzg2l_gpio_set_config() is called with offset 18 and
> config 0x115 (PIN_CONFIG_PERSIST_STATE = 1).
>
> Just adding PIN_CONFIG_PERSIST_STATE to the switch doesn't help,
> as pinctrl_gpio_set_config() also returns -EOPNOTSUPP.
> Ignoring PIN_CONFIG_PERSIST_STATE helps a bit, but the next call
> uses config 0x8, and pinctrl_gpio_set_config() now returns -EINVAL,
> but the pin controller now gets registered?...
>
> Gr{oetje,eeting}s,
>
>                          Geert