Re: [PATCH v12 4/6] pinctrl: add NXP MC33978/MC34978 pinctrl driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `cached_bias` and `cached_drive` arrays independently cache the same physical hardware state (`MC33978_REG_CONFIG`), leading to conflicting state restoration when changing GPIO direction.
--

commit ae57c2734af81a36d2a256fc43f258b7210de10a
Author: David Jander <david@protonic.nl>

pinctrl: add NXP MC33978/MC34978 pinctrl driver

Add pin control and GPIO driver for the NXP MC33978/MC34978 Multiple Switch
Detection Interface (MSDI) devices. This driver translates physical switch
states to logical GPIO levels, emulates outputs via hardware tri-state,
and implements a hierarchical irq_chip for interrupt routing.

> diff --git a/drivers/pinctrl/pinctrl-mc33978.c b/drivers/pinctrl/pinctrl-mc33978.c
> index 0000000000000..2ee1b1284c87a
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-mc33978.c
[ ... ]
> +struct mc33978_pinctrl {
[ ... ]
> +	enum mc33978_bias_state cached_bias[MC33978_NGPIO];
> +	enum mc33978_drive_mode cached_drive[MC33978_NGPIO];
> +};

Does the separation of cached_bias and cached_drive lead to conflicting
state restoration?

Both arrays appear to independently cache the same physical hardware state
(MC33978_REG_CONFIG) for SP pins:

[ ... ]
> +static int mc33978_set_drive(struct mc33978_pinctrl *mpc, unsigned int pin,
> +			     enum mc33978_drive_mode mode)
> +{
[ ... ]
> +	ret = mc33978_update_bits(mpc, MC33978_REG_CONFIG, mask,
> +				  (mode == MC33978_OPEN_DRAIN) ? mask : 0);
> +	if (!ret)
> +		mpc->cached_drive[pin] = mode;
> +
> +	return ret;
> +}

When mc33978_set_drive() updates MC33978_REG_CONFIG and cached_drive,
cached_bias is left stale.

[ ... ]
> +static int mc33978_set_pull(struct mc33978_pinctrl *mpc, unsigned int pin,
> +			    enum mc33978_bias_state val)
> +{
[ ... ]
> +	/* Configure direction (Exclusively for SP pins) */
> +	if (mc33978_is_sp(pin) && val != MC33978_TRISTATE) {
> +		ret = mc33978_update_bits(mpc, MC33978_REG_CONFIG, mask,
> +					  (val == MC33978_PD) ? mask : 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Enable current source or set to tri-state  */
> +	ret = mc33978_update_bits(mpc, mc33978_spsg(MC33978_REG_TRI_SP, pin),
> +				  mask,
> +				   (val == MC33978_TRISTATE) ? mask : 0);
> +	if (!ret)
> +		mpc->cached_bias[pin] = val;
> +
> +	return ret;
> +}

Similarly, when mc33978_set_pull() updates MC33978_REG_CONFIG and
cached_bias, cached_drive is left stale.

[ ... ]
> +static int mc33978_direction_input(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct mc33978_pinctrl *mpc = gpiochip_get_data(chip);
> +
> +	guard(mutex)(&mpc->lock);
[ ... ]
> +	return mc33978_set_pull(mpc, offset, mpc->cached_bias[offset]);
> +}

If mc33978_direction_input() unconditionally restores cached_bias, could
this write back stale MC33978_REG_CONFIG data, clobbering a recently
configured output drive mode?

[ ... ]
> +static int mc33978_direction_output(struct gpio_chip *chip, unsigned int offset,
> +				    int value)
> +{
> +	struct mc33978_pinctrl *mpc = gpiochip_get_data(chip);
> +	u32 mask = BIT(offset);
> +	u32 bits = value ? mask : 0;
> +	int ret;
> +
> +	guard(mutex)(&mpc->lock);
> +
> +	ret = mc33978_set_drive(mpc, offset, mpc->cached_drive[offset]);
> +	if (ret)
> +		return ret;
> +
> +	return mc33978_update_tri_state(mpc, mask, bits);
> +}

And likewise, could mc33978_direction_output() clobber the pinctrl bias
setting when it restores the potentially stale cached_drive?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515160537.115808-1-o.rempel@pengutronix.de?part=4