Re: [PATCH 2/3] arm64: dts: agilex5: define the i2c0 pins for pinctrl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] arm64: dts: agilex5: missing scl-gpios and sda-gpios on i2c0 prevents recovery
--

commit 4fe1ecb21c260e09211a4f5ec837b9b80b38c36d
Author: Dinh Nguyen <dinguyen@kernel.org>

arm64: dts: agilex5: define the i2c0 pins for pinctrl

This commit adds pinctrl definitions for i2c0 default and gpio states on the
Agilex5 SoCDK NAND daughter card to support I2C bus recovery.

> diff --git a/arch/arm64/boot/dts/altera/socfpga_agilex5_socdk_nand.dts b/arch/arm64/boot/dts/altera/socfpga_agilex5_socdk_nand.dts
> index 81443096cf991..346a4ee22f95b 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_agilex5_socdk_nand.dts
> +++ b/arch/arm64/boot/dts/altera/socfpga_agilex5_socdk_nand.dts
> @@ -39,6 +39,22 @@ memory@80000000 {
>  	};
>  };
>  
> +&pinctrl0 {
> +	i2c0_pmx_func: i2c0-pmx-func-pins {
> +		pinctrl-single,pins = <
> +			0x10   0x4   /* I2C0_SDA (IOA4) PIN4SEL) */
> +			0x14   0x4   /* I2C0_SCL (IOA5) PIN5SEL */
> +		>;
> +	};
> +
> +	i2c0_pmx_func_gpio: i2c0-pmx-func-gpio-pins {
> +		pinctrl-single,pins = <
> +			0x10   0x8   /* I2C0_SDA (IOA4) PIN4SEL) */
> +			0x14   0x8   /* I2C0_SCL (IOA5) PIN5SEL */
> +		>;
> +	};
> +};
> +
>  &gmac0 {
>  	status = "okay";
>  	phy-mode = "rgmii"; /* TX/RX clock delays provided by Agilex5 I/O hardware */
> @@ -66,6 +82,10 @@ &gpio1 {
>  
>  &i2c0 {
>  	status = "okay";
> +
> +	pinctrl-names = "default", "gpio";
> +	pinctrl-0 = <&i2c0_pmx_func>;
> +	pinctrl-1 = <&i2c0_pmx_func_gpio>;
>  };

[Severity: Medium]
Does i2c0 also need scl-gpios and sda-gpios properties for I2C recovery to
work?

When probing i2c0, i2c_dw_probe_master() calls i2c_dw_init_recovery_info() in
drivers/i2c/busses/i2c-designware-master.c:

	gpio = devm_gpiod_get_optional(dev->dev, "scl", GPIOD_OUT_HIGH);
	if (IS_ERR_OR_NULL(gpio))
		return PTR_ERR_OR_ZERO(gpio);

	rinfo->scl_gpiod = gpio;

Without scl-gpios in the i2c0 node, devm_gpiod_get_optional() returns NULL and
i2c_dw_init_recovery_info() returns early before calling devm_pinctrl_get() or
populating adap->bus_recovery_info.

Consequently, i2c_init_recovery() in drivers/i2c/i2c-core-base.c sees
adap->bus_recovery_info as NULL and skips i2c_gpio_init_recovery(), leaving
the gpio pinctrl state unused and bus recovery disabled.

In arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts, the pinctrl states
are paired with the GPIO descriptors:

	pinctrl-names = "default", "gpio";
	pinctrl-0 = <&i2c1_pmx_func>;
	pinctrl-1 = <&i2c1_pmx_func_gpio>;

	scl-gpios = <&portb 6 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
	sda-gpios = <&portb 7 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;

Should i2c0 here also specify scl-gpios and sda-gpios (for example on &porta 5
and &porta 4 for IOA5 and IOA4)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029213201.718625-1-dinguyen@kernel.org?part=2