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

[Gabor Juhos <j4g8y7@gmail.com>]

2025. 08. 13. 15:10 keltezéssel, Andy Shevchenko írta:
> On Wed, Aug 13, 2025 at 12:36:45PM +0200, Gabor Juhos wrote:
>> 2025. 08. 11. 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