Re: [PATCH v11 34/37] 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] Missing open-drain configuration array causes runtime `-EINVAL` probe failures

Pre-existing issues:
- [High] Hardcoded SCU compatible string causes probe failure on EN7523
--

--- Patch [34]: [PATCH v11 34/37] pinctrl: airoha: add support of en7523 SoC ---
commit 923709aee684a9571b7d6f8e6ecdbaff7482d40d
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_match_data pinctrl_match_data = {
> +	.pinctrl_name = KBUILD_MODNAME,
> +	.pinctrl_owner = THIS_MODULE,
> +	.pins = pinctrl_pins,
> +	.num_pins = ARRAY_SIZE(pinctrl_pins),
> +	.grps = pinctrl_groups,
> +	.num_grps = ARRAY_SIZE(pinctrl_groups),
> +	.funcs = pinctrl_funcs,
> +	.num_funcs = ARRAY_SIZE(pinctrl_funcs),
> +	.confs_info = {
> +		[AIROHA_PINCTRL_CONFS_PULLUP] = {
> +			.confs = pinctrl_pullup_conf,
> +			.num_confs = ARRAY_SIZE(pinctrl_pullup_conf),
> +		},
> +		[AIROHA_PINCTRL_CONFS_PULLDOWN] = {
> +			.confs = pinctrl_pulldown_conf,
> +			.num_confs = ARRAY_SIZE(pinctrl_pulldown_conf),
> +		},
> +		[AIROHA_PINCTRL_CONFS_DRIVE_E2] = {
> +			.confs = pinctrl_drive_e2_conf,
> +			.num_confs = ARRAY_SIZE(pinctrl_drive_e2_conf),
> +		},
> +		[AIROHA_PINCTRL_CONFS_DRIVE_E4] = {
> +			.confs = pinctrl_drive_e4_conf,
> +			.num_confs = ARRAY_SIZE(pinctrl_drive_e4_conf),
> +		},
> +	},
> +};

[Severity: High]
Does this miss the open-drain configuration array (AIROHA_PINCTRL_CONFS_PCIE_RST_OD)?

The EN7523 device tree binding explicitly defines open-drain configurations
for pcie_reset pins. If a consumer device requests open-drain configuration,
the pinctrl core will try to apply it.

Because this confs_info array omits AIROHA_PINCTRL_CONFS_PCIE_RST_OD,
airoha_pinctrl_get_conf_reg() will return NULL, causing airoha_pinctrl_set_conf()
to return -EINVAL:

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_pinctrl_set_conf() {
    ...
    reg = airoha_pinctrl_get_conf_reg(confs_info->confs,
                                      confs_info->num_confs,
                                      pin);
    if (!reg)
        return -EINVAL;
    ...
}

Since the pinctrl core treats -EINVAL as a fatal error (unlike -ENOTSUPP),
will this permanently fail the pin configuration and abort the consumer
device's probe?

[ ... ]

> +static struct platform_driver airoha_pinctrl_driver = {
> +	.probe = airoha_pinctrl_probe,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This isn't a bug introduced by this patch, but will this probe succeed on
EN7523?

Looking at airoha_pinctrl_probe() in pinctrl-airoha.c, the SCU compatible
string is currently hardcoded:

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_pinctrl_probe() {
    ...
    map = syscon_regmap_lookup_by_compatible("airoha,en7581-chip-scu");
    ...
}

If the EN7523 device tree correctly uses its own SCU compatible (e.g.,
"airoha,en7523-chip-scu") instead of the EN7581 string, will the lookup fail
and abort the probe for the entire SoC?

> +	.driver = {
> +		.name = "pinctrl-airoha-en7523",
> +		.of_match_table = airoha_pinctrl_of_match,
> +	},
> +};

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