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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Pinmux function groups fail to clear conflicting independent mux bits, leading to multiple active functions on the same pad.

Pre-existing issues:
- [High] The pinctrl driver framework lacks `.gpio_request_enable`, preventing dedicated GPIO pads from being returned to GPIO mode at runtime.
--

--- Patch [35]: [PATCH v10 35/36] pinctrl: airoha: add support of an7563 SoC ---
commit 5532fca9c49b720f31c9db638ccbdf1a32ecb7ac
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
> @@ -0,0 +1,1115 @@

[ ... ]

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

[Severity: High]
This is a pre-existing issue in the shared driver core, but does the
omission of dedicated GPIO pads (0-27) here prevent them from being returned
to GPIO mode at runtime?

The shared pinctrl driver framework appears to lack the gpio_request_enable
callback. Because pins 0-27 are explicitly excluded from gpio_groups, they
cannot be muxed to GPIO mode via devicetree either. 

If a pad was previously configured for an alternate function (like PWM) by
the bootloader, standard GPIO requests might succeed in software but the pin
could remain physically stuck in the alternate function.

> +static const char *const pcie_reset_groups[] = {
> +	"pcie_reset0", "pcie_reset1"
> +};

[ ... ]

> +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]
Can this lead to multiple active functions on the same pad?

This group sets the MDIO mux bit (GPIO_SGMII_MDIO_MODE_MASK) but does not
seem to include conflicting I2C (I2C_SCL_GPIO_MODE_MASK) or PWM bits in its
mask to clear them. 

If a pad's state is switched from one function to another, or the bootloader
leaves a pad configured for GPIO, the previous routing bits might remain
active. Could this result in multiple internal SoC peripherals simultaneously
driving the same pad and causing undefined bus behavior?

> +static const struct airoha_pinctrl_func_group uart_func_group[] = {

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