Re: [PATCH v10 32/36] pinctrl: airoha: add support of en7523 SoC

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

New issues:
- [High] The common Airoha pinctrl probe function hardcodes the SCU compatible string, unconditionally failing on EN7523.

Pre-existing issues:
- [High] Conflicting multiplexer bits are not cleared when switching functions on shared pins, leading to hardware state corruption.
--

--- Patch [32]: [PATCH v10 32/36] pinctrl: airoha: add support of en7523 SoC ---
commit 5c0e4632d44fa5720497cb4a8eadc13fc1e3b799
Author: Mikhail Kshevetskiy <[email protected]>

pinctrl: airoha: add support of en7523 SoC

This patch adds support of Airoha en7523 SoC pin controller.

> diff --git a/drivers/pinctrl/airoha/pinctrl-en7523.c b/drivers/pinctrl/airoha/pinctrl-en7523.c
> new file mode 100644
> index 0000000000000..91490418e9ab1
> --- /dev/null
> +++ b/drivers/pinctrl/airoha/pinctrl-en7523.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]
This is a pre-existing issue, but can this lead to hardware state
corruption if functions sharing the same pins are toggled?

If a driver claims "mdio" (pins 20 and 21), airoha_pinmux_set_mux() will
set GPIO_SGMII_MDIO_MODE_MASK. If that driver is later unbound, the MDIO
bit remains set because airoha_pmxops lacks a .disable callback.

If another driver then claims "uart2" on those same shared pins,
airoha_pinmux_set_mux() will use regmap_update_bits() to set
GPIO_UART2_MODE_MASK:

    regmap_update_bits(pinctrl->chip_scu,
                       group->regmap[j].offset,
                       group->regmap[j].mask,
                       group->regmap[j].val);

Since the group mask only covers the specific function's bit, it won't clear
competing function bits. Could this result in both MDIO and UART2 being
simultaneously enabled on the same pins?

[ ... ]

> +static struct platform_driver airoha_pinctrl_driver = {
> +	.probe = airoha_pinctrl_probe,
> +	.driver = {
> +		.name = "pinctrl-airoha-en7523",
> +		.of_match_table = airoha_pinctrl_of_match,
> +	},
> +};

[Severity: High]
Could this result in a probe regression on the EN7523 SoC?

Looking at airoha_pinctrl_probe() in pinctrl-airoha.c, it appears to
hardcode the SCU compatible string:

    map = syscon_regmap_lookup_by_compatible("airoha,en7581-chip-scu");
    if (IS_ERR(map))
        return PTR_ERR(map);

Since the EN7523 SoC device tree uses a different compatible string,
won't this lookup always fail and return -ENODEV during boot? Does this
need to be parameterized (for example, through match data) to support
the new SoC?

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