Re: [PATCH v9 35/36] pinctrl: airoha: add support of an7563 SoC

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing i2c, spi, and uart configurations to clear GPIO mode bits on standalone pads.
- [Medium] Missing gpio function support for pins 0-27 prevents restoring them to GPIO mode.
--

--- Patch [35]: [PATCH v9 35/36] pinctrl: airoha: add support of an7563 SoC ---
commit 6207c71cb41f704bc923f2a3370df4d4674e76e8
Author: Mikhail Kshevetskiy <[email protected]>

    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
> index 0000000000000..40cbbe90cc464
> --- /dev/null
> +++ b/drivers/pinctrl/airoha/pinctrl-an7563.c

[ ... ]

> +static const char *const gpio_groups[] = {
> +	"gpio28", "gpio29", "gpio30", "gpio31", "gpio32",
> +	"gpio33", "gpio34", "gpio35", "gpio36", "gpio37"
> +};

[Severity: Medium]
Will this prevent restoring pins 0-27 back to GPIO mode if they were configured
for alternate functions (like PWM or PON) by the bootloader?

Since pins 0-27 are omitted from this group and gpio_func_group[], users won't
be able to explicitly request them as "gpio".

[ ... ]

> +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 leave pins 30 and 31 stuck in GPIO mode if the bootloader previously
set I2C_SCL_GPIO_MODE_MASK or I2C_SDA_GPIO_MODE_MASK?

It looks like this function doesn't clear those GPIO mode bits when enabling
MDIO.

[ ... ]

> +static const struct airoha_pinctrl_func_group spi_func_group[] = {
> +	{
> +		.name = "spi_quad",
> +		.regmap[0] = {
> +			AIROHA_FUNC_MUX,
> +			REG_GPIO_SPI_CS1_MODE,
> +			GPIO_SPI_QUAD_MODE_MASK,
> +			GPIO_SPI_QUAD_MODE_MASK
> +		},
> +		.regmap_size = 1,
> +	}, {
> +		.name = "spi_cs1",
> +		.regmap[0] = {
> +			AIROHA_FUNC_MUX,
> +			REG_GPIO_SPI_CS1_MODE,
> +			GPIO_SPI_CS1_MODE_MASK,
> +			GPIO_SPI_CS1_MODE_MASK
> +		},
> +		.regmap_size = 1,
> +	},
> +};

[Severity: High]
Is the "spi" group (pins 32-35) intentionally omitted here?

Without it, there appears to be no way to clear the SPI_*_GPIO_MODE_MASK bits
to restore the base SPI functionality if the bootloader configured these pins
as GPIOs.

[ ... ]

> +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]
Does this code completely omit the "i2c" function?

It seems missing from this array, meaning I2C cannot be selected via the device
tree to clear the I2C_SCL/SDA_GPIO_MODE_MASK bits on pins 30 and 31.

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