Re: [PATCH v7 30/34] pinctrl: airoha: add support of en7523 SoC

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Missing hardware configuration for dedicated non-GPIO and PCIe reset pins
- [Low] Dead code: Unused macro definitions

Pre-existing issues:
- [High] Driver probe failure due to hardcoded EN7581 syscon dependency
--

--- Patch [30]: [PATCH v7 30/34] pinctrl: airoha: add support of en7523 SoC ---
commit cb202b59244d84caae5e01de5b50036f419bfc6e
Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
Subject: 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
> --- /dev/null
> +++ b/drivers/pinctrl/airoha/pinctrl-en7523.c
> @@ -0,0 +1,1124 @@

[ ... ]

> +#define REG_NPU_UART_EN				0x0220
> +#define JTAG_UDI_EN_MASK			BIT(4)
> +#define JTAG_DFD_EN_MASK			BIT(3)
> +#define NPU_UART_EN_MASK			BIT(2)
> +
> +#define REG_FORCE_GPIO_EN			0x0224
> +#define FORCE_GPIO_EN(n)			BIT(n)
> +
> +/* LED MAP */

[Severity: Low]
Are these register definitions for REG_FORCE_GPIO_EN and FORCE_GPIO_EN
utilized anywhere in this file? They seem to be defined here but never
mapped or used. Is this leftover code from another SoC driver?

[ ... ]

> +#define AIROHA_PINCTRL_GPIO(gpio, mux_val)			\
> +	{							\
> +		.name = (gpio),					\
> +		.regmap[0] = {					\
> +			AIROHA_FUNC_MUX,			\
> +			REG_GPIO_PON_MODE,			\
> +			(mux_val),				\
> +			(mux_val)				\
> +		},						\
> +		.regmap_size = 1,				\
> +	}
> +
> +#define AIROHA_PINCTRL_GPIO_EXT(gpio, mux_val, smux_val)	\

[Severity: Low]
Is the AIROHA_PINCTRL_GPIO macro needed? It appears only
AIROHA_PINCTRL_GPIO_EXT is used below in gpio_func_group.

[ ... ]

> +static const struct airoha_pinctrl_conf pinctrl_pullup_conf[] = {
> +	PINCTRL_CONF_DESC(12, REG_GPIO_L_PU, BIT(0)),
> +	PINCTRL_CONF_DESC(13, REG_GPIO_L_PU, BIT(1)),

[Severity: Medium]
Should pins 2 through 9 be included in this configuration array?

The driver defines registers and bitmasks for configuring pull-up, pull-down,
and drive strength for these dedicated I2C, SPI, and UART pins (like
REG_I2C_SDA_PU), but they appear to be entirely missing from the
pinctrl_pullup_conf, pinctrl_pulldown_conf, pinctrl_drive_e2_conf, and
pinctrl_drive_e4_conf arrays.

Will device tree properties attempting to configure these electrical
characteristics fail?

[ ... ]

> +	PINCTRL_CONF_DESC(38, REG_GPIO_L_PU, BIT(26)),
> +	PINCTRL_CONF_DESC(39, REG_GPIO_L_PU, BIT(27)),
> +	PINCTRL_CONF_DESC(40, REG_GPIO_L_PU, BIT(28)),
> +	PINCTRL_CONF_DESC(41, REG_GPIO_L_PU, BIT(29)),
> +};

[Severity: Medium]
Are pins 40 and 41 correctly mapped here?

They are mapped to REG_GPIO_L_PU bits 28 and 29, but earlier in the file
PCIE0_RESET_PU_MASK and PCIE1_RESET_PU_MASK are defined as bits 8 and 9 of
REG_I2C_SDA_PU.

Will configuring the PCIe resets write to the wrong register?

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but will this driver successfully probe on
EN7523 hardware?

The core function airoha_pinctrl_probe() appears to unconditionally look up
the system controller using syscon_regmap_lookup_by_compatible() with the
hardcoded string "airoha,en7581-chip-scu". Since the EN7523 device tree
likely uses its own compatible string, won't this lookup fail and cause the
probe function to return -ENODEV?

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