Re: [PATCH v11 04/37] pinctrl: airoha: an7581: fix pinconf of i2c_scl/i2c_sda pins

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

Pre-existing issues:
- [High] The macro definitions for AN7583_I2C1_SCL_PD_MASK and AN7583_I2C1_SDA_PD_MASK are swapped compared to their PU, E2, and E4 counterparts, resulting in a cross-configuration of pulldown settings for AN7583 I2C1 pins.
--

--- Patch [4]: [PATCH v11 04/37] pinctrl: airoha: an7581: fix pinconf of i2c_scl/i2c_sda pins ---
commit fc9a8553da742d5b51b742208157bbf029a89fdf
Author: Mikhail Kshevetskiy <[email protected]>

pinctrl: airoha: an7581: fix pinconf of i2c_scl/i2c_sda pins

Pinconfs of i2c_sda/i2c_scl pins are swapped, this needs to be fixed.

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index 4d0b76f270300..817d51141fed1 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c

[ ... ]

> @@ -2078,8 +2078,8 @@ static const struct airoha_pinctrl_conf an7583_pinctrl_pulldown_conf[] = {

[Severity: High]
This is a pre-existing issue, but since you are fixing the I2C pin
configurations here, could there be a similar mismatch in the AN7583 I2C1
pulldown definitions?

Looking at drivers/pinctrl/airoha/pinctrl-airoha.c, the macro definitions
for the AN7583 I2C1 pulldown masks are swapped compared to their pullup, E2,
and E4 counterparts:

#define AN7583_I2C1_SDA_PD_MASK			BIT(16)
#define AN7583_I2C1_SCL_PD_MASK			BIT(15)

These swapped masks are then used in the an7583_pinctrl_pulldown_conf
array:

static const struct airoha_pinctrl_conf an7583_pinctrl_pulldown_conf[] = {
    ...
	PINCTRL_CONF_DESC(43, REG_I2C_SDA_PD, AN7583_I2C1_SCL_PD_MASK),
	PINCTRL_CONF_DESC(44, REG_I2C_SDA_PD, AN7583_I2C1_SDA_PD_MASK),
    ...
}

Does this cause a hardware misconfiguration where setting a pulldown on the
I2C1 SCL pin erroneously configures the pulldown on the SDA pin, and vice 
versa, potentially causing bus lockups?

>  static const struct airoha_pinctrl_conf en7581_pinctrl_drive_e2_conf[] = {
>  	PINCTRL_CONF_DESC(0, REG_I2C_SDA_E2, UART1_TXD_E2_MASK),
>  	PINCTRL_CONF_DESC(1, REG_I2C_SDA_E2, UART1_RXD_E2_MASK),
> -	PINCTRL_CONF_DESC(2, REG_I2C_SDA_E2, I2C_SDA_E2_MASK),
> -	PINCTRL_CONF_DESC(3, REG_I2C_SDA_E2, I2C_SCL_E2_MASK),
> +	PINCTRL_CONF_DESC(2, REG_I2C_SDA_E2, I2C_SCL_E2_MASK),
> +	PINCTRL_CONF_DESC(3, REG_I2C_SDA_E2, I2C_SDA_E2_MASK),

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