Re: [PATCH v7 33/34] pinctrl: airoha: add support of an7563 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing pinmux functions to restore default pad behaviors (I2C, SPI, UART) and failure to clear GPIO mode override bits when selecting alternate functions (MDIO, PNAND).
- [Medium] The `hsuart_cts_rts` pin group alters the mux state of the base HSUART pins without claiming them, causing potential pin conflicts and DT application order races.
--

--- Patch [33]: [PATCH v7 33/34] pinctrl: airoha: add support of an7563 SoC ---
commit fc95ee9a88a14501536fae31cd14f1346c7074c0
Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>

pinctrl: airoha: add support of an7563 SoC

This patch adds support of Airoha an7563 SoC pin controller.

> diff --git a/drivers/pinctrl/airoha/pinctrl-an7563.c b/drivers/pinctrl/airoha/pinctrl-an7563.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pinctrl/airoha/pinctrl-an7563.c

[ ... ]

> +static const struct airoha_pinctrl_func_group mdio_func_group[] = {
> +	{
> +		.name = "mdio",
> +		.regmap[0] = {
> +			AIROHA_FUNC_MUX,
> +			REG_GPIO_PON_MODE,
> +			GPIO_SGMII_MDIO_MODE_MASK,
> +			GPIO_SGMII_MDIO_MODE_MASK
> +		},
> +		.regmap_size = 1,
> +	},
> +};

[Severity: Medium]
Does this configuration properly clear the GPIO mode override bits if the
bootloader left these pads in GPIO mode?

Looking at mdio_func_group, it only sets GPIO_SGMII_MDIO_MODE_MASK.
However, MDIO shares the I2C_SCL and I2C_SDA pads. If the bootloader
previously set I2C_SCL_GPIO_MODE_MASK and I2C_SDA_GPIO_MODE_MASK, they
would not be cleared here, potentially leaving the pads in GPIO mode
instead of routing them to MDIO.

> +static const struct airoha_pinctrl_func_group uart_func_group[] = {
> +	{
> +		.name = "hsuart",
> +		.regmap[0] = {
> +			AIROHA_FUNC_MUX,
> +			REG_GPIO_PON_MODE,
> +			GPIO_HSUART_MODE_MASK | GPIO_HSUART_CTS_RTS_MODE_MASK,
> +			GPIO_HSUART_MODE_MASK
> +		},
> +		.regmap_size = 1,
> +	},
> +	{
> +		.name = "hsuart_cts_rts",
> +		.regmap[0] = {
> +			AIROHA_FUNC_MUX,
> +			REG_GPIO_PON_MODE,
> +			GPIO_HSUART_MODE_MASK | GPIO_HSUART_CTS_RTS_MODE_MASK,
> +			GPIO_HSUART_MODE_MASK | GPIO_HSUART_CTS_RTS_MODE_MASK
> +		},
> +		.regmap_size = 1,
> +	},
> +};

[Severity: Medium]
Will applying the hsuart group silently disable CTS/RTS if it happens to be
evaluated after hsuart_cts_rts? The hsuart function clears the
GPIO_HSUART_CTS_RTS_MODE_MASK bit.

Additionally, hsuart_cts_rts modifies GPIO_HSUART_MODE_MASK (which routes
pins 16 and 17) despite those pins not being claimed by the group in
hsuart_cts_rts_pins. Could this allow the pinctrl core to mistakenly grant
pins 16 and 17 to another driver as GPIOs, creating a silent hardware
conflict?

[ ... ]

> +static const struct airoha_pinctrl_func_group pnand_func_group[] = {
> +	{
> +		.name = "pnand",
> +		.regmap[0] = {
> +			AIROHA_FUNC_MUX,
> +			REG_GPIO_PON_MODE,
> +			GPIO_PARALLEL_NAND_MODE_MASK,
> +			GPIO_PARALLEL_NAND_MODE_MASK
> +		},
> +		.regmap_size = 1,
> +	},
> +};

[Severity: Medium]
Is this missing the clearing of SPI GPIO mode override bits? Similar to the
MDIO function above, if the bootloader leaves the shared SPI pads in GPIO
mode, setting only GPIO_PARALLEL_NAND_MODE_MASK might not be enough to route
them to the PNAND controller.

[ ... ]

> +static const struct airoha_pinctrl_func pinctrl_funcs[] = {
> +	PINCTRL_FUNC_DESC("pon", pon),
> +	PINCTRL_FUNC_DESC("sipo", sipo),
> +	PINCTRL_FUNC_DESC("mdio", mdio),
> +	PINCTRL_FUNC_DESC("uart", uart),
> +	PINCTRL_FUNC_DESC("jtag", jtag),
> +	PINCTRL_FUNC_DESC("pcm", pcm),
> +	PINCTRL_FUNC_DESC("spi", spi),
> +	PINCTRL_FUNC_DESC("pnand", pnand),
> +	PINCTRL_FUNC_DESC("gpio", gpio),

[Severity: Medium]
Are we missing function descriptors to restore the default I2C and base
SPI/UART pad functions? Since there are no i2c, spi (base), or uart
functions registered here, it appears there is no way for Linux to clear the
GPIO override bits if the bootloader initialized them as GPIOs, which could
leave those hardware blocks unusable.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727074234.3761170-1-mikhail.kshevetskiy@iopsys.eu?part=33