Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery

[<Codrin.Ciubotariu@microchip.com>]

On 03.08.2020 17:16, Russell King - ARM Linux admin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Jul 30, 2020 at 09:00:36AM +0000, Codrin.Ciubotariu@microchip.com wrote:
>> On 27.07.2020 13:50, Russell King - ARM Linux admin wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Mon, Jul 27, 2020 at 10:44:57AM +0000, Codrin.Ciubotariu@microchip.com wrote:
>>>> On 24.07.2020 23:52, Russell King - ARM Linux admin wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
>>>>>> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
>>>>>>>
>>>>>>>> +- pinctrl
>>>>>>>> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
>>>>>>>> + recovery, call it "gpio" or "recovery" state
>>>>>>>
>>>>>>> I think we should stick with "gpio" only. That is what at91 and imx have
>>>>>>> in their bindings. pxa uses "recovery" as a pinctrl state name but I
>>>>>>> can't find any further use or documentation of that. PXA is not fully
>>>>>>> converted to the best of my knowledge, so maybe it is no problem for PXA
>>>>>>> to switch to "gpio", too? We should ask Russell King (cced).
>>>>>
>>>>> Fully converted to what?  The generic handling where the i2c core layer
>>>>> handles everything to do with recovery, including the switch between
>>>>> modes?
>>>>>
>>>>> i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
>>>>> GPIO mode, and I don't see a generic driver doing that to avoid causing
>>>>> any additional glitches on the bus.  Given the use case that this recovery
>>>>> is targetted at, avoiding glitches is very important to keep.
>>>>
>>>> Why is it not possbile to handle glitches in a generic way? I guess it
>>>> depends on the pinctl, but we could treat a worst-case scenario to
>>>> assure the switch between states is done properly.
>>>
>>> Please look at how i2c-pxa switches between the two, and decide whether
>>> the generic implementation can do the same.
>>
>> The handling of glitches from initialization looks generic to me. I see
>> that there are specific clear/reset routines that are in the
>> (un)prepare_recovery() callbacks, but these callbacks are not replaced
>> by the generic i2c recovery and will still be used if given by the
>> driver. The only thing the generic recovery does is to switch the pinmux
>> state. We can discuss whether we want to change the pinmux state first
>> or call the (un)preapre_recovery().
> 
> Right, the key point i2c-pxa does is that on prepare:
> - read the current state of the SCL and SDA lines and set the GPIO to
>    reflect those values.
> - then switch the pinmux state.
> 
> That must be preserved, otherwise if SCL is being held low by the I2C
> master, and we switch to GPIO mode, SCL will be released.  So the
> driver needs to be involved before the pinmux state is changed.

I understand, and I admit that I didn't see this case. In my mind, the 
master would always be in (almost) a reset state before calling for SDA 
recovery, so it won't hold any lines.
These steps can't be generic, of course. Also, not all I2C masters have 
a way to show the state of its lines. For these masters, one idea would 
be to reset them before calling i2c_recover_bus()

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
>