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

[Jacopo Mondi <jacopo@jmondi.org>]

Hi Jonathan,
   two more clarification requests, sorry to bother :)

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 ?

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

Thanks
   j