Re: [PATCH v4 2/2] gpio: axiado: add AX3005 SGPIO controller support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] gpio: axiado: SGPIO_POS macro sets identical first and last positions
- [Medium] gpio: axiado: spurious interrupts due to unsynchronized input cache initialization
--

commit 19833b35bd592291102f572f7810cdc59b72089f
Author: Petar Stepanovic <pstepanovic@axiado.com>

gpio: axiado: add AX3005 SGPIO controller support

This commit adds support for the Axiado AX3005 SGPIO controller. It provides
a serialized interface for controlling multiple GPIO signals over limited
physical lines, registering the controller as a standard gpio_chip with
interrupt support.

> diff --git a/drivers/gpio/gpio-axiado-sgpio.c b/drivers/gpio/gpio-axiado-sgpio.c
> new file mode 100644
> index 0000000000000..b57a5074ccef3
> --- /dev/null
> +++ b/drivers/gpio/gpio-axiado-sgpio.c

[ ... ]

> +/* A POS register holds the first and the last position driven by the slice. */
> +#define SGPIO_POS_FIRST			GENMASK(31, 16)
> +#define SGPIO_POS_LAST			GENMASK(15, 0)
> +#define SGPIO_POS(pos)			(FIELD_PREP(SGPIO_POS_FIRST, (pos)) | \
> +					 FIELD_PREP(SGPIO_POS_LAST, (pos)))

[Severity: High]
Does this macro intentionally set both the start and end positions to the
identical value? 

When using SGPIO_POS_FULL_BANK, this populates both the first and last
bitfields with the same position, reducing the active slice range to a single
position rather than the full range from 0 to 31. Will this prevent the
hardware from shifting out the required number of signals per bank and break
the serialized data exchange?

[ ... ]

> +static int axiado_sgpio_probe(struct platform_device *pdev)
> +{
> +	struct gpio_regmap_config config = { };

[ ... ]

> +	/*
> +	 * From here on the hardware may be running, so the shutdown action has
> +	 * to be in place before the first register is programmed. It only ever
> +	 * writes zeroes, which is harmless if initialisation never got that
> +	 * far, and it runs before the APB clock is disabled because that was
> +	 * requested earlier.
> +	 */
> +	rc = devm_add_action_or_reset(&pdev->dev, axiado_sgpio_disable, sgpio);
> +	if (rc)
> +		return rc;
> +
> +	rc = axiado_sgpio_hw_init(sgpio);
> +	if (rc)
> +		return dev_err_probe(&pdev->dev, rc,
> +				     "Failed to initialize hardware\n");
> +
> +	rc = axiado_sgpio_init_input_cache(sgpio);
> +	if (rc)
> +		return dev_err_probe(&pdev->dev, rc,
> +				     "Failed to initialize input cache\n");

[Severity: Medium]
Is there a race condition between initializing the hardware and reading
the input cache here?

axiado_sgpio_hw_init() enables the background shift engine, but
axiado_sgpio_init_input_cache() immediately reads the din_ss registers to
populate din_shadow. If the first serial hardware exchange has not yet
completed, won't din_shadow be initialized with stale or uninitialized zero
values? 

Could this cause consumers to receive a spurious edge interrupt during
initialization if a signal is physically high, as the parent handler will later
observe a transition from the zeroed cache to the true physical level?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-axiado-ax3000-sgpio-controller-v4-0-79c444be7faf@axiado.com?part=2