Re: [PATCH v11 5/6] pinctrl: s32cc: implement GPIO functionality

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] GPIO output states (PGPDO) are lost during system suspend and resume, causing hardware glitches or data loss upon resume.
- [Low] Semantic mismatch and confusing naming in `s32_gpio_pad_map_xlate()` where a pin ID is compared against properties documented as GPIO line offsets.
--

commit ff45721a707b92abe04df6541806874a10e772ee
Author: Andrei Stefanescu <andrei.stefanescu@oss.nxp.com>

pinctrl: s32cc: implement GPIO functionality

This commit introduces GPIO functionality to the S32CC pin controller
using the gpio-regmap library. It adds virtual regmaps to translate GPIO
operations to the underlying SIUL2 registers (MSCR, PGPDI, PGPDO) and
supports both legacy and extended DT bindings.

Link: https://lore.kernel.org/linux-gpio/20260120115923.3463866-4-khristineandreea.barbulescu@oss.nxp.com/T/#m543c9edbdde74bdc68b6a2364e8b975356c33043
Link: https://lore.kernel.org/all/20260504131148.3622697-7-khristineandreea.barbulescu@oss.nxp.com/

> diff --git a/drivers/pinctrl/nxp/pinctrl-s32.h b/drivers/pinctrl/nxp/pinctrl-s32.h
> --- a/drivers/pinctrl/nxp/pinctrl-s32.h
> +++ b/drivers/pinctrl/nxp/pinctrl-s32.h

[ ... ]

> +/**
> + * struct s32_gpio_pad_map - mapping between GPIO ranges and PGPD pads
> + * @gpio_start: first GPIO line offset in the range
> + * @gpio_end: last GPIO line offset in the range
> + * @pad: PGPD pad number serving the range
> + */
> +struct s32_gpio_pad_map {
> +	unsigned int gpio_start;
> +	unsigned int gpio_end;
> +	unsigned int pad;
> +};

[ ... ]

> diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
> --- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
> +++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c

[ ... ]

> +static int s32_gpio_pad_map_xlate(struct s32_pinctrl *ipctl,
> +				  unsigned int gpio,
> +				  unsigned int *reg_offset,
> +				  u16 *mask)
> +{

[ ... ]

> +	for (i = 0; i < soc_data->num_gpio_pad_maps; i++) {
> +		map = &soc_data->gpio_pad_maps[i];
> +
> +		if (gpio < map->gpio_start || gpio > map->gpio_end)
> +			continue;

[Severity: Low]
Are gpio_start and gpio_end actually meant to be pin IDs instead of GPIO
line offsets?

The documentation in struct s32_gpio_pad_map says they are GPIO line
offsets, but here they are compared against the incoming gpio parameter
which actually receives a pin ID from s32_gpio_xlate_pgpd().

While this does not break functionality currently because gpio_base equals
pin_base for all sparse ranges, is it worth updating the naming and
documentation to reflect that these variables map to pin IDs?

> +
> +		bit = gpio - map->gpio_start;
> +		*mask = BIT(S32_PGPD_SIZE - 1 - bit);
> +		*reg_offset = S32_PGPD(map->pad);

[ ... ]

> @@ -709,8 +1238,7 @@ int s32_pinctrl_suspend(struct device *dev)
>  	const struct pinctrl_pin_desc *pin;
>  	const struct s32_pinctrl_soc_info *info = ipctl->info;
>  	struct s32_pinctrl_context *saved_context = &ipctl->saved_context;
> -	int i;
> -	int ret;
> +	int i, ret;
>  	unsigned int config;
>  
>  	for (i = 0; i < info->soc_data->npins; i++) {
> @@ -721,7 +1249,7 @@ int s32_pinctrl_suspend(struct device *dev)
>  
>  		ret = s32_regmap_read(ipctl->pctl, pin->number, &config);
>  		if (ret)
> -			return -EINVAL;
> +			return ret;
>  
>  		saved_context->pads[i] = config;
>  	}

[Severity: High]
Should PGPDO output register states be saved and restored during system
suspend and resume?

Currently, s32_pinctrl_suspend() saves the MSCR configurations but omits
saving the newly introduced PGPDO registers.

> @@ -736,7 +1264,7 @@ int s32_pinctrl_resume(struct device *dev)
>  	const struct s32_pinctrl_soc_info *info = ipctl->info;
>  	const struct pinctrl_pin_desc *pin;
>  	struct s32_pinctrl_context *saved_context = &ipctl->saved_context;
> -	int ret, i;
> +	int i, ret;
>  
>  	for (i = 0; i < info->soc_data->npins; i++) {
>  		pin = &info->soc_data->pins[i];
> @@ -745,7 +1273,7 @@ int s32_pinctrl_resume(struct device *dev)
>  			continue;
>  
>  		ret = s32_regmap_write(ipctl->pctl, pin->number,
> -					 saved_context->pads[i]);
> +				 saved_context->pads[i]);

During s32_pinctrl_resume(), when the MSCR registers are restored, the
Output Buffer Enable (OBE) bits are re-enabled.

Because PGPDO registers lose power and reset to 0 during suspend, and
they are not explicitly restored here, could active GPIO outputs immediately
drive their hardware reset values instead of their pre-suspend states
upon resume, potentially causing hardware glitches?

>  		if (ret)
>  			return ret;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610132116.1998140-1-khristineandreea.barbulescu@oss.nxp.com?part=5