2025. 08. 13. 15:10 keltezéssel, Andy Shevchenko írta:
On Wed, Aug 13, 2025 at 12:36:45PM +0200, Gabor Juhos wrote:
- 22:26 keltezéssel, Andy Shevchenko írta:
On Mon, Aug 11, 2025 at 09:49:56PM +0200, Gabor Juhos wrote:
...
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.
It is not an error in the pinctrl code. I have checked and even fixed a few bugs in that.
And I didn't get how recovery code affects the initialisation (enumeration).
Without the fix, it is not possible to initiate a transaction on the bus, which in turn prevents enumeration.
But why? As you said below the first pin control state is changed during the probe, which is fine, and the culprit one happens on the recovery.
Erm, no. Both happens during probe, before the I2C core tries to enumerate the devices on the bus.
Why is recovery involved in probe? This is quite confusing...
Let me try to explain it differently. Here is the simplified call chain:
i2c_pxa_probe() ... i2c_pxa_init_recovery() pinctrl_select_state() <- selects GPIO state pinctrl_select_state() <- selects default (I2C) state ... i2c_add_numbered_adapter() i2c_register_adapter() ... i2c_init_recovery() i2c_gpio_init_recovery() i2c_gpio_init_generic_recovery() pinctrl_select_state() <- selects GPIO state*** ... pinctrl_select_state() <- selects default (I2C) state ... bus_for_each_drv() __process_new_adapter() i2c_do_add_adapter() i2c_detect() <- enumerates the devices
The culprit is the first pinctrl_select_state() call in i2c_gpio_init_generic_recovery() marked with '***'.
That call causes the controller to go stuck, which makes it impossible to transfer anything on the bus.
Do we set pin control state back and forth during probe? May be this is a root cause?
Yes, basically. The state gets changed back and forth twice. Once in driver probe before the controller gets initialized, then once again in i2c_gpio_init_generic_recovery(). The problem is caused by the second state change as it runs after the controller gets enabled which confuses the hardware.
...
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.
I agree, but since the change is targeted also to stable kernels, I wanted to keep the change as minimal as possible.
Renaming is not a big deal AFAICS, but leaving this _cb will be confusing in a long term. I prefer name to be changed.
Ok, will change the name.
Regards, Gabor