Re: [PATCH v2 2/3] pinctrl: starfive: jh7110: add support for PAD_INTERNAL_* for GPI

[E Shattow <e@freeshell.de>]

On 4/23/25 23:20, Icenowy Zheng wrote:
> The JH7110 SoC's both pin controller support routing GPI signals to
> internal fixed low/high level.
> 
> As we allocated two special "pin" numbers for these situations
> (PAD_INTERNAL_{LOW,HIGH}), add special handling code for these "pins".
> The DOEn/DOUT/FUNCTION fields are ignored and the internal input signal
> specified by the DIN field is routed to fixed low/high level.
> 
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> ---
>  .../starfive/pinctrl-starfive-jh7110.c        | 41 +++++++++++++++----
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c b/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
> index 1d0d6c224c104..fb18c7974ec86 100644
> --- a/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
> +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jh7110.c
> @@ -291,6 +291,24 @@ void jh7110_set_gpiomux(struct jh7110_pinctrl *sfp, unsigned int pin,
>  }
>  EXPORT_SYMBOL_GPL(jh7110_set_gpiomux);
>  

> +static void jh7110_set_gpi(struct jh7110_pinctrl *sfp, u32 gpi, u32 val)
> +{
> +	u32 offset, shift;
> +	u32 reg_val;
> +	const struct jh7110_pinctrl_soc_info *info = sfp->info;
> +
> +	offset = 4 * (gpi / 4);
> +	shift  = 8 * (gpi % 4);
> +
> +	reg_val = readl_relaxed(sfp->base +
> +				info->gpi_reg_base + offset);
> +	reg_val &= info->gpi_mask << shift;
> +	reg_val |= (val & info->gpi_mask) << shift;
> +
> +	writel_relaxed(reg_val, sfp->base +
> +				info->gpi_reg_base + offset);
> +}
> +

Are there some bit ops masking macros common to Linux that should be
used here?

>  static int jh7110_set_mux(struct pinctrl_dev *pctldev,
>  			  unsigned int fsel, unsigned int gsel)
>  {
> @@ -307,14 +325,23 @@ static int jh7110_set_mux(struct pinctrl_dev *pctldev,
>  	pinmux = group->data;
>  	for (i = 0; i < group->grp.npins; i++) {
>  		u32 v = pinmux[i];
> +		u32 pin = jh7110_pinmux_pin(v);
>  
> -		if (info->jh7110_set_one_pin_mux)
> -			info->jh7110_set_one_pin_mux(sfp,
> -					jh7110_pinmux_pin(v),
> -					jh7110_pinmux_din(v),
> -					jh7110_pinmux_dout(v),
> -					jh7110_pinmux_doen(v),
> -					jh7110_pinmux_function(v));

I cannot think of any reason why you would need to do it this way:

> +		switch (pin) {
> +		case PAD_INTERNAL_LOW:
> +		case PAD_INTERNAL_HIGH:
> +			jh7110_set_gpi(sfp, jh7110_pinmux_din(v),
> +				       pin == PAD_INTERNAL_HIGH);
> +			break;

Please, just more readable and let compiler do its job to optimize:

		switch (pin) {
		case PAD_INTERNAL_LOW:
			jh7110_set_gpi(sfp, jh7110_pinmux_din(v), 0);
			break;

		case PAD_INTERNAL_HIGH:
			jh7110_set_gpi(sfp, jh7110_pinmux_din(v), 1);
			break;

> +		default:
> +			if (info->jh7110_set_one_pin_mux)
> +				info->jh7110_set_one_pin_mux(sfp,
> +						jh7110_pinmux_pin(v),
> +						jh7110_pinmux_din(v),
> +						jh7110_pinmux_dout(v),
> +						jh7110_pinmux_doen(v),
> +						jh7110_pinmux_function(v));
> +		}
>  	}
>  
>  	return 0;

Thank you for your work on this series!  -E