Re: [PATCH v6 4/5] gpio: aaeon: Add GPIO driver for SRG-IMX8P MCU

[Thomas Perrot <thomas.perrot@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 4551 bytes --]

Hello Sashiko,

On Tue, 2026-06-30 at 13:10 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider:
> - [High] Unconditional resetting of all hardware pins in the `probe`
> function breaks bootloader hand-off and destroys firmware
> configuration.
> - [Medium] Ignoring critical hardware errors during probe
> initialization leads to inconsistent software state.
> --
> 
> commit 73ee6d66b506331ac77e36c415e83febae316cf0
> Author: Thomas Perrot (Schneider Electric)
> <thomas.perrot@bootlin.com>
> 
> gpio: aaeon: Add GPIO driver for SRG-IMX8P MCU
> 
> This commit adds a GPIO driver for the Aaeon SRG-IMX8P embedded
> controller.
> It supports 7 GPO pins and 12 GPIO pins, providing access to pins
> handled
> by the microcontroller. During probe, all pins are reset to a known
> state.
> 
> > diff --git a/drivers/gpio/gpio-aaeon-mcu.c b/drivers/gpio/gpio-
> > aaeon-mcu.c
> > new file mode 100644
> > index 0000000000000..a9e048c865f5c
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-aaeon-mcu.c
> 
> [ ... ]
> 
> > +static void aaeon_mcu_gpio_reset(struct aaeon_mcu_gpio *data,
> > struct device *dev)
> > +{
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	/* Reset all GPOs */
> > +	for (i = 0; i < MAX_GPOS; i++) {
> > +		ret = aaeon_mcu_gpo_set_cmd(data, i, 0);
> > +		if (ret < 0)
> > +			dev_warn(dev, "Failed to reset GPO %u
> > state: %d\n", i, ret);
> > +		clear_bit(i, data->dir_in);
> 
> [Severity: Medium]
> Is it safe to update the internal software state tracking when the
> hardware
> command fails?

aaeon_mcu_gpio_reset() is a best-effort initialization: the MCU does
not reset GPIO state on soft reboot, so the driver attempts to bring
both hardware and software into a known state at probe time.

If an I2C error occurs, the true hardware state is unknown and
unrecoverable without a power cycle. Keeping the software bitmaps at
the desired post-reset state (GPOs as outputs, GPIOs as inputs) is
safer than preserving stale values from the previous boot, which could
cause get_direction() to return wrong results for subsequent callers. 
Any ongoing I2C fault will surface as errors in the regular GPIO ops.

> 
> If aaeon_mcu_gpo_set_cmd() returns an error because the underlying
> bus is
> temporarily inaccessible, clear_bit() is still executed. This could
> result
> in the GPIO controller registering with a mismatched view of the
> hardware.
> 
> > +	}
> > +
> > +	/* Reset all GPIOs */
> > +	for (i = MAX_GPOS; i < MAX_GPOS + MAX_GPIOS; i++) {
> > +		ret = aaeon_mcu_gpio_config_input_cmd(data, i);
> > +		if (ret < 0)
> > +			dev_warn(dev, "Failed to reset GPIO %u
> > state: %d\n", i, ret);
> > +		set_bit(i, data->dir_in);
> 
> [Severity: Medium]
> Similarly, does this code lead to an inconsistent state by blindly
> updating
> data->dir_in even if aaeon_mcu_gpio_config_input_cmd() fails?
> 

Please refer to my other answers.

> > +	}
> > +}
> > +
> > +static int aaeon_mcu_gpio_probe(struct platform_device *pdev)
> > +{
> 
> [ ... ]
> 
> > +	/*
> > +	 * Reset all GPIO states to a known configuration. The MCU
> > does not
> > +	 * reset GPIO state on soft reboot, only on power cycle
> > (hard reboot).
> > +	 * Without this reset, GPIOs would retain their previous
> > state across
> > +	 * reboots, which could lead to unexpected behavior.
> > +	 */
> > +	aaeon_mcu_gpio_reset(data, &pdev->dev);
> 
> [Severity: High]
> Could forcing all output pins to low and resetting GPIO
> configurations to
> inputs unconditionally during probe disrupt the state established by
> the
> bootloader?
> 

There is currently no bootloader support for the SRG-IMX8P MCU, so no
bootloader-established GPIO state exists to preserve. The reset at
probe time ensures a known quiescent state on every boot, which is
necessary because the MCU retains GPIO state across soft reboots
(kexec, reboot) but not across power cycles. Without it, GPIO state
would be non-deterministic depending on the boot path.

Kind regards,
Thomas

> Overriding the hardware state on every boot could inadvertently cut
> power to
> critical system components like regulators, eMMC, or displays that
> were left
> on by the bootloader or previous kernel execution.
> 
> > +
> > +	return devm_gpiochip_add_data(&pdev->dev, &data->gc,
> > data);
> > +}

-- 
Thomas Perrot, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]