Re: [PATCH v6 17/17] pinctrl: airoha: add support of an7563 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `hsuart_cts_rts` pin group introduces a pin claiming gap and an order-dependent hardware routing bug that can silently disable UART CTS/RTS.
- [Low] The `i2c` and `spi` pin groups are defined and registered but are completely unreachable, resulting in dead code.
--

commit 80f4c6428d6088ebf03f569dea1ce788b34979a5
Author: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>

pinctrl: airoha: add support of an7563 SoC

This patch adds support of Airoha an7563 SoC pin controller.
Also it adds corresponding device tree binding schema.

> diff --git a/drivers/pinctrl/airoha/pinctrl-an7563.c b/drivers/pinctrl/airoha/pinctrl-an7563.c
> index 0000000000000..40cbbe90cc464 100644
> --- /dev/null
> +++ b/drivers/pinctrl/airoha/pinctrl-an7563.c
> @@ -0,0 +1,1115 @@
[ ... ]
> +static const int hsuart_cts_rts_pins[] = { 14, 15 };
> +static const int i2c_pins[] = { 30, 31 };
> +static const int jtag_udi_pins[] = { 7, 8, 9, 10, 11 };
> +static const int jtag_dfd_pins[] = { 7, 8, 9, 10, 11 };
> +static const int pcm1_pins[] = { 22, 23, 24, 25 };
> +static const int pcm2_pins[] = { 1, 2, 3, 4 };
> +static const int spi_pins[] = { 32, 33, 34, 35 };

[Severity: Medium]
Are pins 16 and 17 missing from hsuart_cts_rts_pins[]?

The hsuart_cts_rts function (defined later in uart_func_group) configures the
hardware routing for all 4 UART pins, but this array only claims pins 14
and 15.

Could this leave pins 16 and 17 vulnerable to being concurrently claimed as
GPIOs while still being forcefully routed to UART in hardware?

[Severity: Low]
Are the i2c_pins and spi_pins groups actually reachable?

These groups are defined in pinctrl_groups but do not appear to be included in
any function's group array (such as i2c_groups or spi_groups). This means they
cannot be selected via a function mux.

Additionally, the device tree schema strictly forbids the use of the 'groups'
property in 'conf' nodes, preventing direct configuration. This seems to leave
them as unused dead code.

[ ... ]
> +static const struct airoha_pinctrl_func_group uart_func_group[] = {
> +	{
> +		.name = "hsuart",
> +		.regmap[0] = {
> +			AIROHA_FUNC_MUX,
> +			REG_GPIO_PON_MODE,
> +			GPIO_HSUART_MODE_MASK | GPIO_HSUART_CTS_RTS_MODE_MASK,
> +			GPIO_HSUART_MODE_MASK
> +		},
> +		.regmap_size = 1,
> +	},
> +	{
> +		.name = "hsuart_cts_rts",
> +		.regmap[0] = {
> +			AIROHA_FUNC_MUX,
> +			REG_GPIO_PON_MODE,
> +			GPIO_HSUART_MODE_MASK | GPIO_HSUART_CTS_RTS_MODE_MASK,
> +			GPIO_HSUART_MODE_MASK | GPIO_HSUART_CTS_RTS_MODE_MASK
> +		},
> +		.regmap_size = 1,
> +	},
> +};

[Severity: Medium]
Could the hsuart function unintentionally disable CTS/RTS hardware routing?

If a device tree requests both "hsuart" and "hsuart_cts_rts" (as shown in the
dt-binding example) but they are applied such that hsuart goes last, it will
actively clear the CTS/RTS bit set by hsuart_cts_rts.

This happens because the hsuart mask explicitly includes
GPIO_HSUART_CTS_RTS_MODE_MASK but the value only sets GPIO_HSUART_MODE_MASK.

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