On Tue, 31 Aug 2021 09:40:11 +0200
Jacopo Mondi <jacopo@xxxxxxxxxx> 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
