Message-Id: <D71AW9R10XCO.D7ONB8ZU3M56@gmail.com>
Date: Mon, 13 Jan 2025 23:32:38 +0100
From: "Javier Carrasco" <javier.carrasco.cruz@...il.com>
To: "Matti Vaittinen" <mazziesaccount@...il.com>
Cc: "Jonathan Cameron" <jic23@...nel.org>, "Lars-Peter Clausen"
 <lars@...afoo.de>, "Rishi Gupta" <gupt21@...il.com>,
 <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Jonathan
 Cameron" <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH 2/2] iio: light: veml6030: fix scale to conform to ABI

On Mon Jan 13, 2025 at 8:52 PM CET, Matti Vaittinen wrote:
> ma 13.1.2025 klo 17.05 Javier Carrasco
> (javier.carrasco.cruz@...il.com) kirjoitti:
> >
> > On Mon Jan 13, 2025 at 12:56 PM CET, Matti Vaittinen wrote:
> > > On 07/01/2025 22:50, Javier Carrasco wrote:
> > > > The current scale is not ABI-compliant as it is just the sensor gain
> > > > instead of the value that acts as a multiplier to be applied to the raw
> > > > value (there is no offset).
> > > >
> > > > Use the iio-gts helpers to obtain the proper scale values according to
> > > > the gain and integration time to match the resolution tables from the
> > > > datasheet and drop dedicated variables to store the current values of
> > > > the integration time, gain and resolution. When at it, use 'scale'
> > > > instead of 'gain' consistently for the get/set functions to avoid
> > > > misunderstandings.
> > > >
> > > > Fixes: 7b779f573c48 ("iio: light: add driver for veml6030 ambient light sensor")
> > > > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@...il.com>
> > >
> > > Thanks for the patch Javier.
> > >
> > > And, sorry for a review which is more about questions than suggested
> > > improvements. I find it hard to focus on reading code today.
> > >
> > > > ---
> > > >   drivers/iio/light/Kconfig    |   1 +
> > > >   drivers/iio/light/veml6030.c | 499 ++++++++++++++++---------------------------
> > > >   2 files changed, 189 insertions(+), 311 deletions(-)
> > > >
> > >
> > > I like the diffstats of this Fix! :) It's nice you found gts-helpers
> > > helpful :)
> >
> > I wonder how painful the int. time/gain/scale issue in ALS was before
> > iio-gts existed :D
> >
> I don't really know. I wrote the gts-helpers as soon as I wrote my
> first light sensor driver :)
>
> > > ...
> > >
> > > > +
> > > > +static int veml6030_process_als(struct veml6030_data *data, int raw,
> > > > +                           int *val, int *val2)
> > > > +{
> > > > +   int ret, scale_int, scale_nano;
> > > > +   u64 tmp;
> > > > +
> > > > +   ret = veml6030_get_scale(data, &scale_int, &scale_nano);
> > > > +   if (ret < 0)
> > > > +           return ret;
> > > > +
> > > > +   tmp = (u64)raw * scale_nano;
> > > > +   *val = raw * scale_int + div_u64(tmp, NANO);
> > > > +   *val2 = div_u64(do_div(tmp, NANO), MILLI);
> > >
> > > do_div() is horrible on some platforms. Or, at least it used to be. Is
> > > there really no way to go without it? We're dealing with 16bit data and
> > > maximum of 512x total gain only, so maybe there was a way(?)
> > >
> > > Maybe a stupid question (in which case I blame mucus in my head) - could
> > > you just divide the raw value by the total gain?
> > >
> >
> > In its current form we need the 64-bit operations to handle the scale,
> > and it will probably have to stay like that for the reasons I give you
> > below.
>
> Still, I wonder if multiplying by scale really differs from dividing
> by the total gain? I think the scale is inversely proportional to the
> total gain, right?
>

I am sorry, but I am not sure if I got your point here. The scale is the
multiplier to get lux from raw, and for example it's not just 1/512 for
the maximum total gain, as that is not taking the intrinsic resolution
of the sensor. Maybe I am misunderstanding something, but I don't see the
way around raw * scale with the scale being one of the discrete values
provided in the application note.

I will give you a simple example, so you can tell me where my reasoning
fails:

raw = 100 counts
scale = 2.1504 lux/count (when IT=25ms and GAIN=1/8)
processed = 215.04 lux (raw * scale, ABI compliant for IIO_LIGHT)

The reply to your comment below explains why we have a PROCESSED
IIO_LIGHT in the first place.

> > > >   static irqreturn_t veml6030_event_handler(int irq, void *private)
> > > > @@ -1084,6 +968,13 @@ static int veml6030_hw_init(struct iio_dev *indio_dev, struct device *dev)
> > > >     int ret, val;
> > > >     struct veml6030_data *data = iio_priv(indio_dev);
> > > >
> > > > +   ret = devm_iio_init_iio_gts(dev, 2, 150400000,
> > >
> > > Can you please explain the seemingly odd choice for the max scale?
> > >
> > > Just a quick look at the sensor spec and this driver allows me to assume
> > > following:
> > >
> > > Maximum 'total gain' from both HWGAIN and integration time is 16 * 32 = 512.
> > >
> > > If we chose the 'total gain' 1x to match scale 1, then the smallest
> > > scale would be 1/512 = 0.001 953 125
> > >
> > > This is 1953125 NANOs. This would mean the max-scale 1X => gain 1X would
> > > not cause precision loss. (Well, I'm not at my sharpest as I've caught
> > > cold - but I _think_ this is true, right?)
> > >
> > > If I read this correctly, the only channel where the scale gets applied
> > > is the INTENSITY channel, right? Hence it should be possible to chose
> > > the scale as we see best. (Unless the idea of this seemingly odd scale
> > > is to maintain the old intensity / scale values in order to not shake
> > > userland any more - in which case this could be mentioned).
> > >
> >
> > The scale is applied to an IIO_LIGHT channel, not to the INTENSITY,
>
> Isn't the IIO_LIGHT channel a PROCESSED one? I thought the scale
> shouldn't be applied to it. (Driver may still apply scale internally,
> but users should not see it, right? And if the driver does it only
> internally, then the driver can also multiply values using (N *
> scale). Well, I suppose you can as well use this "fitted MAX SCALE" -
> but maybe it warrants a comment.

IIO_LIGHT provides RAW and PROCESSED values, which shouldn't have
happened in the first place as PROCESSED is just raw * scale, if scale
had been right from the beginning. Actually, when I took over this
driver, I was tempted to drop the PROCESSED value, but it was too late
for that without breaking ABI. My guess is that the processed value was
provided because in_illuminance_scale was not the right multiplier, only
the gain.
Note that in_illuminance_scale is also provided to the user, and that
must comply with the ABI definitions.

Thank you again,
Javier Carrasco

Powered by blists - more mailing lists