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

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

[Jonathan Cameron]

On Tue, 31 Aug 2021 09:40:11 +0200
Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Jonathan,
>    two more clarification requests, sorry to bother :)
No problem.  First one: No idea +CC wolfram and i2c list.
> 
> On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote:
> > On Mon, 30 Aug 2021 18:20:51 +0200  
> > > > > +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> > > > > +{
> > > > > +	__be16 be_data = cpu_to_be16(data);
> > > > > +	int ret;
> > > > > +
> > > > > +	sunrise_wakeup(sunrise);  
> > > >
> > > > Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
> > > > between the wakeup and the following command.  That would make the device going back
> > > > to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
> > > > you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
> > > > gives an example.  
> > >
> > > Right, there might be another call stealing the wakeup session!
> > >
> > > I should lock the underlying i2c bus, probably not root adapter like
> > > mlx90614 does but only the segment.  
> >
> > Ideally only segment as you say as could be some muxes involved.  
> 
> If not that i2c_smbus_xfer() which is called by my wakeup and by the
> regmap_smbus_* calls tries to lock the segment as well, so we deadlock :)
> 
> And actually, locking the underlying i2c segment seems even too
> strict, what we have to guarantee is that no other read/write function
> call from this driver interrupts a [wakeup-trasactions] sequence.
> 
> Wouldn't it be better if I handle that with a dedicated mutex ?

I'm not sure what best route is. +CC Wolfram, Peter and linux-i2c.

Short story here is we have a device which autonomously goes to sleep.
Datasheet suggests waking it up with a failed xfer followed by what we
actually want to do (sufficiently quickly).

Obviously we can't actually guarantee that will ever happen but it's a lot
more likely to succeed if we briefly stop anything else using he i2c bus.

How should we handle this?




> 
> >  
> > >  
> > > >
> > > > Perhaps worth a look is the regmap-sccb implementation which has a dance that looks
> > > > a tiny bit like what you have to do here (be it for a different reason).
> > > > It might be nice to do something similar here and have a custom regmap bus which
> > > > has the necessary wakeups in the relevant places.
> > > >
> > > > Note I haven't thought it through in depth, so it might not work!  
> > >
> > > the dance is similar if not regmap-sccb tranfers a byte instead of
> > > sending only the R/W bit (notice the usage of I2C_SMBUS_QUICK here and
> > > I2C_SMBUS_BYTE in regmap-sccb). Practically speaking it makes no
> > > difference as the sensor nacks the first message, so the underlying
> > > bus implementation bails out, but that's a bit of work-by-accident
> > > thing, isn't it ?
> > >
> > > If fine with you, I would stick to this implementation and hold the
> > > segment locked between the wakup and the actual messages.  
> >
> > That's fine.  I was just thinking you could hid the magic in a custom regmap then
> > the rest of the driver would not have to be aware of it.  Slightly neater than
> > wrapping regmap functions with this extra call in the wrapper.
> >  
> 
> [snip]
> 
> > > > > +		}
> > > > > +
> > > > > +	case IIO_CHAN_INFO_SCALE:
> > > > > +		/* Chip temperature scale = 1/100 */  
> > > >
> > > > IIO temperatures are measured in milli degrees.  1lsb = 1/100*1000 degrees centigrade seems very accurate
> > > > for a device like this!  I'm guessing this should be 10.  
> > >
> > > Ah yes, I thought it had to be given in the chip's native format,
> > > which is 1/100 degree.
> > >
> > > I guess I should then multiply by 10 the temperature raw read and
> > > return IIO_VAL_INT here.  
> >
> > You could do that, but can cause a mess if buffered support comes along later as
> > it is then not a whole number of bits for putting in the buffer.
> >  
> 
> Care to clarify ? What shouldn't I do here ? Multiply by 10 the raw
> value (which I think is wrong as pointed out in a later reply) or
> return IIO_VAL_INT ? Sorry I didn't get the connection with the number
> of bits to put in the buffer :)

So.  If you stick to output of _raw and _scale in the buffer the data
would be whatever you read from the register.  That is typically a whole number of bits.
If you were to multiply by 10 you end up something that may be say 12 or 13 bits depending
on the value.  That's a bit ugly. We can handle it of course, but I'd rather not if it's
as simple as not doing the *10 in kernel for the sysfs path.

So not critical but things end up more elegant / standard if we don't do the *10 and
instead make that a problem for userspace.

Jonathan


> 
> Thanks
>    j

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

[Peter Rosin]

On 2021-09-05 12:04, Jonathan Cameron wrote:
> On Tue, 31 Aug 2021 09:40:11 +0200
> Jacopo Mondi <jacopo@jmondi.org> wrote:
> 
>> Hi Jonathan,
>>    two more clarification requests, sorry to bother :)
> No problem.  First one: No idea +CC wolfram and i2c list.
>>
>> On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote:
>>> On Mon, 30 Aug 2021 18:20:51 +0200  
>>>>>> +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
>>>>>> +{
>>>>>> +	__be16 be_data = cpu_to_be16(data);
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	sunrise_wakeup(sunrise);  
>>>>>
>>>>> Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
>>>>> between the wakeup and the following command.  That would make the device going back
>>>>> to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
>>>>> you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
>>>>> gives an example.  
>>>>
>>>> Right, there might be another call stealing the wakeup session!
>>>>
>>>> I should lock the underlying i2c bus, probably not root adapter like
>>>> mlx90614 does but only the segment.  
>>>
>>> Ideally only segment as you say as could be some muxes involved.  
>>
>> If not that i2c_smbus_xfer() which is called by my wakeup and by the
>> regmap_smbus_* calls tries to lock the segment as well, so we deadlock :)
>>
>> And actually, locking the underlying i2c segment seems even too
>> strict, what we have to guarantee is that no other read/write function
>> call from this driver interrupts a [wakeup-trasactions] sequence.
>>
>> Wouldn't it be better if I handle that with a dedicated mutex ?
> 
> I'm not sure what best route is. +CC Wolfram, Peter and linux-i2c.
> 
> Short story here is we have a device which autonomously goes to sleep.
> Datasheet suggests waking it up with a failed xfer followed by what we
> actually want to do (sufficiently quickly).
> 
> Obviously we can't actually guarantee that will ever happen but it's a lot
> more likely to succeed if we briefly stop anything else using he i2c bus.
> 
> How should we handle this?

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.

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.

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

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

[Peter Rosin]

On 2021-09-06 01:03, Peter Rosin wrote:
> On 2021-09-05 12:04, Jonathan Cameron wrote:
>> On Tue, 31 Aug 2021 09:40:11 +0200
>> Jacopo Mondi <jacopo@jmondi.org> wrote:
>>
>>> Hi Jonathan,
>>>    two more clarification requests, sorry to bother :)
>> No problem.  First one: No idea +CC wolfram and i2c list.
>>>
>>> On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote:
>>>> On Mon, 30 Aug 2021 18:20:51 +0200  
>>>>>>> +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
>>>>>>> +{
>>>>>>> +	__be16 be_data = cpu_to_be16(data);
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	sunrise_wakeup(sunrise);  
>>>>>>
>>>>>> Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
>>>>>> between the wakeup and the following command.  That would make the device going back
>>>>>> to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
>>>>>> you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
>>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
>>>>>> gives an example.

Forgot to mention, regmap does let you do that. See e.g.
drivers/media/dvb-frontends/rtl2830.c which wraps regmap_update_bits,
regmap_bulk_write and regmap_bulk_read within a local I2C segment
lock along with a special regmap_bus that does unlocked I2C trasfers.

I think the driver does this because it has an I2C gate that needs to
be opened with a regmap interaction that in turn needs to be back to
back with the "real" regmap access, or else the gate closes again.

Cheers,
Peter

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

[Jacopo Mondi]

Hi Peter,
   thanks for your detailed answer

On Mon, Sep 06, 2021 at 01:03:52AM +0200, Peter Rosin wrote:
> On 2021-09-05 12:04, Jonathan Cameron wrote:
> > On Tue, 31 Aug 2021 09:40:11 +0200
> > Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> >> Hi Jonathan,
> >>    two more clarification requests, sorry to bother :)
> > No problem.  First one: No idea +CC wolfram and i2c list.
> >>
> >> On Mon, Aug 30, 2021 at 06:11:17PM +0100, Jonathan Cameron wrote:
> >>> On Mon, 30 Aug 2021 18:20:51 +0200
> >>>>>> +static int sunrise_write_word(struct sunrise_dev *sunrise, u8 reg, u16 data)
> >>>>>> +{
> >>>>>> +	__be16 be_data = cpu_to_be16(data);
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	sunrise_wakeup(sunrise);
> >>>>>
> >>>>> Hmm. Technically there isn't anything stopping another user of the i2c bus sneaking in
> >>>>> between the wakeup and the following command.  That would make the device going back
> >>>>> to sleep a lot more likely.  I can't off the top of my head remember if regmap lets
> >>>>> you lock the bus.  If not, you'll have to use the underlying i2c bus locking functions.
> >>>>> https://elixir.bootlin.com/linux/latest/source/drivers/iio/temperature/mlx90614.c#L432
> >>>>> gives an example.
> >>>>
> >>>> Right, there might be another call stealing the wakeup session!
> >>>>
> >>>> I should lock the underlying i2c bus, probably not root adapter like
> >>>> mlx90614 does but only the segment.
> >>>
> >>> Ideally only segment as you say as could be some muxes involved.
> >>
> >> If not that i2c_smbus_xfer() which is called by my wakeup and by the
> >> regmap_smbus_* calls tries to lock the segment as well, so we deadlock :)
> >>
> >> And actually, locking the underlying i2c segment seems even too
> >> strict, what we have to guarantee is that no other read/write function
> >> call from this driver interrupts a [wakeup-trasactions] sequence.
> >>
> >> Wouldn't it be better if I handle that with a dedicated mutex ?
> >
> > I'm not sure what best route is. +CC Wolfram, Peter and linux-i2c.
> >
> > Short story here is we have a device which autonomously goes to sleep.
> > Datasheet suggests waking it up with a failed xfer followed by what we
> > actually want to do (sufficiently quickly).
> >
> > Obviously we can't actually guarantee that will ever happen but it's a lot
> > more likely to succeed if we briefly stop anything else using he i2c bus.
> >
> > How should we handle this?
>
> 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.

>
> 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, ..).
}

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 ?

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

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

[Peter Rosin]

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

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

[Andy Shevchenko]

On Wed, Sep 8, 2021 at 6:29 PM Peter Rosin <peda@axentia.se> wrote:
> On 2021-09-08 13:00, Jacopo Mondi wrote:
> > On Mon, Sep 06, 2021 at 01:03:52AM +0200, Peter Rosin wrote:

...

>         struct i2c_msg msg[3] = {
>                 {       /* wakeup */
>                         .addr = 0x68,
>                         .flags = I2C_M_RD | I2C_M_IGNORE_NAK,
>                         .len = 0,
>                 }, {    /* write register number */

I'm wondering if device will have enough time in between to actualle
be woken up. I believe the waking up latency must be considered as
well as known 15ms suspending one.

>                         .addr = 0x68,
>                         .flags = 0,
>                         .len = 1,
>                         .buf = &reg,
>                 }, {    /* read register contents */
>                         .addr = 0x68,
>                         .flags = I2C_M_RD,
>                         .len = 17,
>                         .buf = buf,
>                 },
>         };

-- 
With Best Regards,
Andy Shevchenko

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

[Jacopo Mondi]

Hi Peter,

On Wed, Sep 08, 2021 at 05:29:02PM +0200, Peter Rosin wrote:
> 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.

In facts, I have just implemented this version using unlocked
__i2c_smbus_ functions in the regmap read/write overloads:

static int sunrise_regmap_read(void *context, const void *reg_buf,
			       size_t reg_size, void *val_buf, size_t val_size)
{
	struct i2c_client *client = context;
	union i2c_smbus_data data;
	int ret;

	memset(&data, 0, sizeof(data));
	data.block[0] = val_size;

	/*
	 * Wake up sensor by sending sensor address: START, sensor address,
	 * STOP. Sensor will not ACK this byte.
	 *
	 * The chip enters a low power state after 15msec without
	 * communications or after a complete read/write sequence.
	 */
	__i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
			 I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);

	ret = __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
			       I2C_SMBUS_READ, ((u8 *)reg_buf)[0],
			       I2C_SMBUS_I2C_BLOCK_DATA, &data);
	if (ret < 0)
		return ret;

	memcpy(val_buf, &data.block[1], data.block[0]);

	return 0;
}

static int sunrise_regmap_write(void *context, const void *val_buf, size_t count)
{
	struct i2c_client *client = context;
	union i2c_smbus_data data;

	/* Discard reg address from values count. */
	if (count < 1)
		return -EINVAL;
	count--;

	memset(&data, 0, sizeof(data));
	data.block[0] = count;
	memcpy(&data.block[1], (u8 *)val_buf + 1, count);

	__i2c_smbus_xfer(client->adapter, client->addr, I2C_M_IGNORE_NAK,
			 I2C_SMBUS_WRITE, 0, I2C_SMBUS_QUICK, NULL);

	return __i2c_smbus_xfer(client->adapter, client->addr, client->flags,
				I2C_SMBUS_WRITE, ((u8 *)val_buf)[0],
				I2C_SMBUS_I2C_BLOCK_DATA, &data);
}


Those will be wrapped by a segment lock in the driver's read/write
functions.

>
> > 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);

But this is interesting as well, as it seems the chip is flexible
enough to support repeated starts so this might works too.

Assuming this version works, which one is preferred in terms of
compatibility with the largest possible number of adapters (if it
makes any difference at all) ?

Documentation/i2c/smbus-protocol.rst seems to suggest smbus command
should be preferred:

If you write a driver for some I2C device, please try to use the SMBus
commands if at all possible (if the device uses only that subset of the
I2C protocol). This makes it possible to use the device driver on both
SMBus adapters and I2C adapters (the SMBus command set is automatically
translated to I2C on I2C adapters, but plain I2C commands can not be
handled at all on most pure SMBus adapters).

Thanks
   j

>
> 	...
>
> 	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