linux-kernel - Re: [PATCH v2 2/3] pinctrl: starfive: jh7110: add support for PAD_INTERNAL

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0ce7dff4-a6cf-4e5b-a5b1-3c58926ef9ab@freeshell.de>
Date: Thu, 24 Apr 2025 00:57:26 -0700
From: E Shattow <e@...eshell.de>
To: Icenowy Zheng <uwu@...nowy.me>, Emil Renner Berthing <kernel@...il.dk>,
 Jianlong Huang <jianlong.huang@...rfivetech.com>,
 Hal Feng <hal.feng@...rfivetech.com>,
 Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v2 2/3] pinctrl: starfive: jh7110: add support for
 PAD_INTERNAL_* for GPI

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@...nowy.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