Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2021-09-08 13:00, Jacopo Mondi wrote:
> Hi Peter,
>    thanks for your detailed answer
> 
> On Mon, Sep 06, 2021 at 01:03:52AM +0200, Peter Rosin wrote:
>> The way I read this is that interactions with other I2C devices that squeeze
>> in are not a fundamental problem. Not unless there are so many of these 3rd
>> party xfers that the device times out again. If my assessment is correct,
>> then I would suggest handling this in the driver by somehow making sure that
>> it doesn't clobber its own pairs of wakeup+work interactions. But see below.
> 
> So, I tested by sending a double wakeup signal, to verify if the chip
> goes back to sleep after -any- kind of transaction after the first
> wakeup. It seems it does from what I see.
> 
> I also tested by inserting a spurious i2c transaction to a
> non-existing address between the wakeup and the actual transaction,
> but I cannot say if that proves anything as I'm not sure if messages
> directed to non-registered addresses are actually put on the bus.
> 
> Anyway, quoting the chip manual:
> 
> ------------------------------------------------------------------------------
> Senseair Sunrise spend most of its time in deep sleep mode to minimize
> power consumption, this have the effect that it is necessary to wake
> up the sensor before it is possible to communicate with it.  Sensor
> will wake up on a falling edge on SDA, it is recommended to send
> sensors address to wake it up. When sensors address is used to wake up
> the sensor the sensor will not acknowledge this byte.
> 
> Communication sequence:
> 1) Wake up sensor by sending sensor address (START, sensor address, STOP).
>    Sensor will not ACK this byte.
> 
> 2) Normal I2C read/write operations. I2C communication must be started
>    within 15ms after the wake-up byte, each byte sent to or from the
>    sensor sets the timeout to 15 ms. After a complete read or write
>    sequence sensor will enter sleep mode immediately.
> ------------------------------------------------------------------------------
> 
> I think you're correct assuming the only way 3rd parties could
> interfere with the wakeup session is by issuing so many transactions
> that the bus is not available to the device for such a long time
> (15msec) that the wakeup session expires.
> 
> Other messages, not directed to the chip, doesn't seem to interfere.
> 
> So locking in the driver should be good enough I think.

I think so too. Even if 15ms is kind of short... I mean, locking the
I2C segment can certainly exclude (some) 3rd party xfers on the bus and
that may help, but there is so much else that could potentially cause
a 15ms stall, especially on smallish single-cpu devices.

>>
>> Because there really is no way to protect against those extra I2C accesses.
>> With a parent-locked mux you can (ignoring arbitrators, where another
>> system may possibly take over the bus if too much time is spent between
>> two xfers that were supposed to be adjacent). But if there's a mux-locked
>> mux in the path it's simply not possible to lock out all other xfers on
>> the root adapter.
>>
>> With a parent-locked I2C tree, "locking the segment" is equivalent to
>> locking everything all the way to the root adapter. But the whole point
>> of mux-locked muxes is that they can't operate if you do that. Mux-locked
>> muxes are allowed to depend on other (ignorant) drivers using other parts
>> of the I2C tree while the segment is locked. If you lock the root adapter
>> when there is a mux-locked mux involved, you simply kill that property
>> and sign up for a deadlock. Which also means that you cannot prevent 3rd
>> party xfers to other parts of the I2C tree.
>>
>> However, if you worry about 3rd party xfers causing the wakeup to timeout
>> again and that only handling it in the driver as suggested above is
>> insufficient, then it's an option to lock the segment. For parent-locked I2C
>> trees, this should prevent (most) 3rd party actions and minimize the delay.
>> In the odd case that there are mux-locked muxes involved, there will simply
>> be a higher risk of failure, but there is little to do about that.
> 
> It doesn't feel to me this is required, but I let Jonathan and Andy
> which have discussed this with me in the past express an opinion as
> well.
> 
> In case I need to go for this solution am I correct assuming I should
> lock the bus for the whole wakeup-work session and override the
> regmap_bus operations to perform unlocked access to the i2c bus?
> 
> In my mind this could be realized as
> 
> int wakeup()
> {
> 
>         /* Unlocked wakup ping */
>         __i2c_smbus_xfer()
> }
> 
> int regmap_bus_read()
> {
>         i2c_lock_bus();
> 
>         wakeup();
>         /* Unlocked i2c read transaction */
>         __i2c_transfer();
> 
>         i2c_unlock_bus();
> }
> 
> 
> struct regmap_bus regmap_bus = {
>         .read = regmap_bus_read,
>         ...
> }
> 
> int probe()
> {
> 
>         regmap_init(..., regmap_bus, ..).
> }

Yep, that looks like the right direction from here as well, should you take
that path.

> But I somehow feel like I could have locking and wakeup handled by a mux's
> select/deselect and have a simpler read function. However even if I
> think I could have the driver register a mux even if there's actually
> no muxed bus behind the chip, I'm missing how that would work if not
> by exposing this in the DT bindings or by registering an
> i2c_board_info, but this feels already too complicated to be worth it,
> right ?

This basically is exactly what is otherwise called an I2C gate. You have
to do <something> to get I2C xfers safely past the gate, in this case
wake the device up. So, that model isn't wrong.

However, since the device wakes up on *any* action on SDA, would it not
also work to make special I2C xfers, with a restart instead of a full
stop-start after the "wakeup call". That is, if you can assume that the
I2C adapter is flexible enough...

I.e. something like this:

static int
sunrise_foo(struct sunrise_context *ctx)
{
	unsigned char reg = 0x17;
	unsigned char buf[17];
	struct i2c_msg msg[3] = {
		{	/* wakeup */
			.addr = 0x68,
			.flags = I2C_M_RD | I2C_M_IGNORE_NAK,
			.len = 0,
		}, {	/* write register number */
			.addr = 0x68,
			.flags = 0,
			.len = 1,
			.buf = &reg,
		}, {	/* read register contents */
			.addr = 0x68,
			.flags = I2C_M_RD,
			.len = 17,
			.buf = buf,
		},
	};
	int ret;

	ret = i2c_transfer(ctx->adapter, msg, 3);

	...

	return ret;
}

Cheers,
Peter

> Thanks
>    j
> 
>>
>> See Documentation/i2c/i2c-topology.rst for some discussion on the details
>> of mux-locked and parent-locked muxes.
>>
>> I hope I make at least some sense...
>>
>> Cheers,
>> Peter



[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux