Re: [PATCH v2 2/3] i2c: pxa: prevent calling of the generic recovery init code

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Mon, Aug 11, 2025 at 09:49:56PM +0200, Gabor Juhos wrote:
> The I2C communication is completely broken on the Armada 3700 platform
> since commit 0b01392c18b9 ("i2c: pxa: move to generic GPIO recovery").
> 
> For example, on the Methode uDPU board, probing of the two onboard
> temperature sensors fails ...
> 
>   [    7.271713] i2c i2c-0: using pinctrl states for GPIO recovery
>   [    7.277503] i2c i2c-0:  PXA I2C adapter
>   [    7.282199] i2c i2c-1: using pinctrl states for GPIO recovery
>   [    7.288241] i2c i2c-1:  PXA I2C adapter
>   [    7.292947] sfp sfp-eth1: Host maximum power 3.0W
>   [    7.299614] sfp sfp-eth0: Host maximum power 3.0W
>   [    7.308178] lm75 1-0048: supply vs not found, using dummy regulator
>   [   32.489631] lm75 1-0048: probe with driver lm75 failed with error -121
>   [   32.496833] lm75 1-0049: supply vs not found, using dummy regulator
>   [   82.890614] lm75 1-0049: probe with driver lm75 failed with error -121
> 
> ... and accessing the plugged-in SFP modules also does not work:
> 
>   [  511.298537] sfp sfp-eth1: please wait, module slow to respond
>   [  536.488530] sfp sfp-eth0: please wait, module slow to respond
>   ...
>   [ 1065.688536] sfp sfp-eth1: failed to read EEPROM: -EREMOTEIO
>   [ 1090.888532] sfp sfp-eth0: failed to read EEPROM: -EREMOTEIO
> 
> After a discussion [1], there was an attempt to fix the problem by
> reverting the offending change by commit 7b211c767121 ("Revert "i2c:
> pxa: move to generic GPIO recovery""), but that only helped to fix
> the issue in the 6.1.y stable tree. The reason behind the partial succes
> is that there was another change in commit 20cb3fce4d60 ("i2c: Set i2c
> pinctrl recovery info from it's device pinctrl") in the 6.3-rc1 cycle
> which broke things further.
> 
> The cause of the problem is the same in case of both offending commits
> mentioned above. Namely, the I2C core code changes the pinctrl state to
> GPIO while running the recovery initialization code. Although the PXA
> specific initialization also does this, but the key difference is that
> it happens before the conrtoller is getting enabled in i2c_pxa_reset(),
> whereas in the case of the generic initialization it happens after that.
> 
> To resolve the problem, provide an empty init_recovery() callback
> function thus preventing the I2C core to call the generic recovery
> initialization code.
> 
> As the result this change restores the original behaviour, which in
> turn makes the I2C communication to work again as it can be seen from
> the following log:
> 
>   [    7.305277] i2c i2c-0:  PXA I2C adapter
>   [    7.310198] i2c i2c-1:  PXA I2C adapter
>   [    7.315012] sfp sfp-eth1: Host maximum power 3.0W
>   [    7.324061] lm75 1-0048: supply vs not found, using dummy regulator
>   [    7.331738] sfp sfp-eth0: Host maximum power 3.0W
>   [    7.337000] hwmon hwmon0: temp1_input not attached to any thermal zone
>   [    7.343593] lm75 1-0048: hwmon0: sensor 'tmp75c'
>   [    7.348526] lm75 1-0049: supply vs not found, using dummy regulator
>   [    7.356858] hwmon hwmon1: temp1_input not attached to any thermal zone
>   [    7.363463] lm75 1-0049: hwmon1: sensor 'tmp75c'
>   ...
>   [    7.730315] sfp sfp-eth1: module Mikrotik         S-RJ01           rev 1.0  sn 61B103C55C58     dc 201022
>   [    7.840318] sfp sfp-eth0: module MENTECHOPTO      POS22-LDCC-KR    rev 1.0  sn MNC208U90009     dc 200828
>   [    7.850083] mvneta d0030000.ethernet eth0: unsupported SFP module: no common interface modes
>   [    7.990335] hwmon hwmon2: temp1_input not attached to any thermal zone

TBH this sounds to me like trying to hack the solution and as you pointed out
the problem is in pinctrl state changes. I think it may affect not only I2C case.

And I didn't get how recovery code affects the initialisation (enumeration). Do we
set pin control state back and forth during probe? May be this is a root cause?

...

> [1] https://lore.kernel.org/r/20230926160255.330417-1-robert.marko@sartura.hr
> 

Can you make this a Link tag?
Link: $URL #1

> Cc: stable@vger.kernel.org # 6.3+
> Fixes: 20cb3fce4d60 ("i2c: Set i2c pinctrl recovery info from it's device pinctrl")
> Signed-off-by: Gabor Juhos <j4g8y7@gmail.com>
> Signed-off-by: Imre Kaloz <kaloz@openwrt.org>

...

>  static int i2c_pxa_init_recovery(struct pxa_i2c *i2c)
>  {
>  	struct i2c_bus_recovery_info *bri = &i2c->recovery;

>  		return 0;
>  	}
>  
> +	bri->init_recovery = i2c_pxa_init_recovery_cb;

This is unfortunate. I would keep the naming schema consistent, i.e. rename
existing function and use its original name for the new callback.

>  	bri->prepare_recovery = i2c_pxa_prepare_recovery;
>  	bri->unprepare_recovery = i2c_pxa_unprepare_recovery;
>  	bri->recover_bus = i2c_generic_scl_recovery;

-- 
With Best Regards,
Andy Shevchenko