Re: [PATCH v8 34/35] pinctrl: airoha: add support of an7563 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing pinmux functions to restore dedicated GPIO pads to GPIO mode and standalone pads to their native functions, and the MDIO function fails to clear GPIO override bits.
--

--- Patch [34]: [PATCH v8 34/35] pinctrl: airoha: add support of an7563 SoC ---
commit 17737ef79ed00a92161a7a544b4f3bec1f3d0e5c
Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
pinctrl: airoha: add support of an7563 SoC

This commit introduces support for the Airoha AN7563 SoC pin controller. It
provides pin definitions, group mappings, and pinmux functionalities for various
interfaces like PON, MDIO, I2C, and PWM.

> diff --git a/drivers/pinctrl/airoha/pinctrl-an7563.c b/drivers/pinctrl/airoha/pinctrl-an7563.c
> new file mode 100644
> index 0000000000000..40cbbe90cc464
> --- /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: High]
Does this code leave the I2C_SCL_GPIO_MODE_MASK and I2C_SDA_GPIO_MODE_MASK
bits set when configuring the mdio function?

If these pads were previously overridden to GPIO mode (for instance, by the
bootloader), setting the SGMII MDIO mode bit here might not be sufficient to
restore MDIO functionality if the GPIO override bits aren't also cleared.

[ ... ]

> +static const struct airoha_pinctrl_func_group gpio_func_group[] = {
> +	AIROHA_PINCTRL_GPIO_EXT("gpio28", GPIO28_FLASH_MODE_CFG,
> +				GPIO_PCIE_RESET0_MASK),
> +	AIROHA_PINCTRL_GPIO_EXT("gpio29", GPIO29_FLASH_MODE_CFG,
> +				GPIO_PCIE_RESET1_MASK),
> +	AIROHA_PINCTRL_GPIO_EXT("gpio30", GPIO30_FLASH_MODE_CFG,
> +				I2C_SCL_GPIO_MODE_MASK),
> +	AIROHA_PINCTRL_GPIO_EXT("gpio31", GPIO31_FLASH_MODE_CFG,
> +				I2C_SDA_GPIO_MODE_MASK),
> +	AIROHA_PINCTRL_GPIO("gpio32", SPI_CLK_GPIO_MODE_MASK),
> +	AIROHA_PINCTRL_GPIO("gpio33", SPI_CS_GPIO_MODE_MASK),
> +	AIROHA_PINCTRL_GPIO("gpio34", SPI_MOSI_GPIO_MODE_MASK),
> +	AIROHA_PINCTRL_GPIO("gpio35", SPI_MISO_GPIO_MODE_MASK),
> +	AIROHA_PINCTRL_GPIO_EXT("gpio36", GPIO36_FLASH_MODE_CFG,
> +				UART_TXD_GPIO_MODE_MASK),
> +	AIROHA_PINCTRL_GPIO_EXT("gpio37", GPIO37_FLASH_MODE_CFG,
> +				UART_RXD_GPIO_MODE_MASK),
> +};

[Severity: High]
Is this missing definitions for gpio0 through gpio27?

If these dedicated GPIO pads are ever configured for an alternate function
like PWM, it appears they cannot be switched back to GPIO mode via the pinctrl
subsystem because they are omitted from this function group.

[ ... ]

> +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),
> +	PINCTRL_FUNC_DESC("pcie_reset", pcie_reset),
> +	PINCTRL_FUNC_DESC("pwm", pwm),
> +	PINCTRL_FUNC_DESC("phy1_led0", phy1_led0),
> +	PINCTRL_FUNC_DESC("phy2_led0", phy2_led0),
> +	PINCTRL_FUNC_DESC("phy3_led0", phy3_led0),
> +	PINCTRL_FUNC_DESC("phy4_led0", phy4_led0),
> +	PINCTRL_FUNC_DESC("phy1_led1", phy1_led1),
> +	PINCTRL_FUNC_DESC("phy2_led1", phy2_led1),
> +	PINCTRL_FUNC_DESC("phy3_led1", phy3_led1),
> +	PINCTRL_FUNC_DESC("phy4_led1", phy4_led1),
> +};

[Severity: High]
Should this list include native pad functions for I2C, SPI, and UART1?

If the bootloader leaves these standalone pads in GPIO mode, there currently
appears to be no pinmux function available here to clear the GPIO override
bits and restore their native interfaces.

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