Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe

From: Eddie James <eajames@linux.ibm.com>
To: Peter Rosin <peda@axentia.se>,
	Jonathan Cameron <jic23@kernel.org>,
	devicetree@vger.kernel.org
Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
	krzysztof.kozlowski+dt@linaro.org, robh+dt@kernel.org,
	lars@metafoo.de, miltonm@us.ibm.com, linux-i2c@vger.kernel.org
Subject: Re: [PATCH v2 0/2] iio: humidity: si7020: Check device property for skipping reset in probe
Date: Wed, 18 May 2022 10:51:26 -0500	[thread overview]
Message-ID: <6d517473-9ac4-8a58-64c5-1c27ecd6f95f@linux.ibm.com> (raw)
In-Reply-To: <bbeaa4b6-1412-dfac-a6ef-dbcd9f1e3f5c@linux.ibm.com>

On 5/18/22 10:28, Eddie James wrote:
>
> On 5/14/22 10:02, Peter Rosin wrote:
>> 2022-05-14 at 15:43, Jonathan Cameron wrote:
>>> On Sat, 14 May 2022 00:48:51 +0200
>>> Peter Rosin <peda@axentia.se> wrote:
>>>
>>>> Hi!
>>>>
>>>> 2022-05-13 at 18:45, Jonathan Cameron wrote:
>>>>> On Thu, 12 May 2022 12:08:07 -0500
>>>>> Eddie James <eajames@linux.ibm.com> wrote:
>>>>>> On 5/12/22 11:48, Jonathan Cameron wrote:
>>>>>>> On Thu, 12 May 2022 11:20:18 -0500
>>>>>>> Eddie James <eajames@linux.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.
>
>
> Hm, interesting. Sounds a bit complicated, though very flexible. For a 
> less flexible, but less complex, approch, we could add a i2c_msg flag 
> that says to do a delay in the core? And then si7020 could just submit 
> a couple of raw messages rather than smbus... What do you think?

Um, nevermind... that would require changes in all the bus drivers. I'll 
look into implementing the hook/callback.

Thanks,

Eddie

>
>
> Thanks,
>
> Eddie
>
>
>
>>
>> Me waves hand a bit...
>>
>> Cheers,
>> Peter
>
>