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

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) <[email protected]>

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?

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?

> +	}
> +}
> +
> +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?

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);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4