Message-ID: <0569bb70-e2dc-de85-268d-30ee7c9491fb@axentia.se>
Date:   Sat, 14 May 2022 17:02:31 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Jonathan Cameron <jic23@...nel.org>, devicetree@...r.kernel.org
Cc:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Eddie James <eajames@...ux.ibm.com>, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, krzysztof.kozlowski+dt@...aro.org,
        robh+dt@...nel.org, lars@...afoo.de, miltonm@...ibm.com,
        linux-i2c@...r.kernel.org
Subject: Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for
 skipping reset in probe

2022-05-14 at 15:43, Jonathan Cameron wrote:
> On Sat, 14 May 2022 00:48:51 +0200
> Peter Rosin <peda@...ntia.se> wrote:
> 
>> Hi!
>>
>> 2022-05-13 at 18:45, Jonathan Cameron wrote:
>>> On Thu, 12 May 2022 12:08:07 -0500
>>> Eddie James <eajames@...ux.ibm.com> wrote:
>>>   
>>>> On 5/12/22 11:48, Jonathan Cameron wrote:  
>>>>> On Thu, 12 May 2022 11:20:18 -0500
>>>>> Eddie James <eajames@...ux.ibm.com> wrote:
>>>>>    
>>>>>> I2C commands issued after the SI7020 is starting up or after reset
>>>>>> can potentially upset the startup sequence. Therefore, the host
>>>>>> needs to wait for the startup sequence to finish before issuing
>>>>>> further i2c commands. This is impractical in cases where the SI7020
>>>>>> is on a shared bus or behind a mux, which may switch channels at
>>>>>> any time (generating I2C traffic). Therefore, check for a device
>>>>>> property that indicates that the driver should skip resetting the
>>>>>> device when probing.    
>>>>> Why not lock the bus?  It's not ideal, but then not resetting and hence
>>>>> potentially ending up in an unknown state isn't great either.    
>>>>
>>>>
>>>> Agreed, but locking the bus doesn't work in the case where the chip is 
>>>> behind a mux. The mux core driver deselects the mux immediately after 
>>>> the transfer to reset the si7020, causing some i2c traffic, breaking the 
>>>> si7020. So it would also be a requirement to configure the mux to idle 
>>>> as-is... That's why I went with the optional skipping of the reset. 
>>>> Maybe I should add the bus lock too?
>>>>  
>>>
>>> +Cc Peter and linux-i2c for advice as we should resolve any potential
>>> issue with the mux side of things rather than hiding it in the driver
>>> (if possible!)  
>>
>> IIUC, the chip in question cannot handle *any* action on the I2C bus
>> for 15ms (or so) after a "soft reset", or something bad<tm> happens
>> (or at least may happen).
>>
>> If that's the case, then providing a means of skipping the reset is
>> insufficient. If you don't lock the bus, you would need to *always*
>> skip the reset, because you don't know for certain if something else
>> does I2C xfers.
>>
>> So, in order to make the soft reset not be totally dangerous even in
>> a normal non-muxed environment, the bus must be locked for the 15ms.
>>
>> However, Eddie is correct in that the I2C mux code may indeed do its
>> muxing xfer right after the soft reset command. There is currently
>> no way to avoid that muxing xfer. However, it should be noted that
>> there are ways to mux an I2C bus without using xfers on the bus
>> itself, so it's not problematic for *all* mux variants.
>>
>> It can be debated if the problem should be worked around with extra
>> dt properties like this, or if a capability should be added to delay
>> a trailing muxing xfer.
>>
>> I bet there are other broken chips that have drivers that do in fact
>> lock the bus to give the chip a break, but then it all stumbles
>> because of the unexpected noise if there's a (wrong kind of) mux in
>> the mix.
> 
> Ok, so for now I think we need the bus lock for the reset + either
> a work around similar to this series, or additions to the i2c mux code
> to stop it doing a muxing xfer if the bus is locked?

I think there might be cases where it might be valid to restore the mux
directly after an xfer even if the mux is externally locked prior to the
muxed xfer. But I'm not sure? In any case, it will be a bit convoluted
for the mux code to remember that it might need to restore the mux
later. And it will get even hairier when multiple levels of muxing is
considered...

Maybe some kind of hook/callback that could be installed temporarily on
the I2C adapter that is called right after the "real" xfer, where the
driver could then make the needed mdelay call?

I.e.
1. lock the bus
2. install this new hook/callback
3. do an unlocked xfer, get notified and call mdelay
5. uninstall the hook/callback
6. unlock the bus

The hook/callback could be uninstalled automatically on unlock, then
you would not need to handle multiple notifications. But then again,
there is probably some existing framework that should be used that
handles all than neatly and efficiently.

Me waves hand a bit...

Cheers,
Peter

Powered by blists - more mailing lists