Re: [PATCH 2/2 v5] iio: light: Add a driver for Sharp GP2AP002x00F

[Linus Walleij <linus.walleij@linaro.org>]

On Sun, Feb 2, 2020 at 4:08 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 26 Jan 2020 20:27:22 +0000 Jonathan Bakker <xc-racer2@live.ca> wrote:

> > -             if (ch_type != IIO_CURRENT) {
> > +             if (ch_type == IIO_VOLTAGE) {
> > +                     ret = device_property_read_u8(dev,
> > +                             "sharp,adc-adjustment-ratio", &val);
> > +                     if (ret) {
> > +                             dev_err(dev,
> > +                                     "failed to obtain adc conversion\n");
> > +                             return -EINVAL;
> > +                     }
> > +                     gp2ap002->adc_adj = val;
> > +             } else if (ch_type != IIO_CURRENT) {
> >                       dev_err(dev,
> >                               "wrong type of IIO channel specified for ALSOUT\n");
> >                       return -EINVAL;
> >
> > Alternatively, you could collect the resistor value, the ADC precision (this doesn't
> > appear to be queryable via the IIO layer), and the reference voltage level - but I'm
> > not sure how you'd do the inverse calculation in the kernel.
>
> An alternative to doing this is to represent the resistor circuit explicitly.
>
> You end up with a really small ADC driver that is a consumer of a voltage
> and provides a current channel.  That has all the properties of the
> circuit via DT.

That is indeed a lot better, more modular and more reusable.
It also becomes its own node in the device tree with very
generic bindings for resistance and ADC bias/offset.

> In general I would prefer we handle this sort of conversion generically
> rather than bolting it into a light sensor driver like you are doing here,
> even if it comes at the cost of a bit more complexity.

Agreed.

There are however two improvements that can be done as separate
patches to the code in this driver, but preferably by someone with access
to the right hardware so they can verify the result.

The Google Android code pointed to by Mr. Bakker:
https://android.googlesource.com/device/samsung/crespo/+/2e0ab7265e3039fee787c2216e0c98d92ea0b49e%5E%21/#F0

+                // Convert adc value to lux assuming:
+                // I = 10 * log(Ev) uA
+                // R = 47kOhm
+                // Max adc value 4095 = 3.3V
+                // 1/4 of light reaches sensor
+                mPendingEvent.light = powf(10, event->value * (330.0f
/ 4095.0f / 47.0f)) * 4;

contains:

- A logarithmic formula based on the datasheet which we
  don't have but presumably correct. A patch converting the
  crude interpolated look-up table to proper floating point maths
  expressing the curve would be much appreciated and cuts
  down the size of the driver. This should be one simple
  patch with nothing else needing to be changed. According
  to the formula it should be lux = 10^(mA/10) which corresponds
  to the values in the table. I verified the values with a spreadsheet,
  then I sent a patch like this, please test!

- A device tree property to compensate for the attenuation by
  the glass in front of the sensor. In the example the
  attenuation is 75%, only 1/4 of the light actually hits the
  sensor. I am uncertain about the physics of this, should
  that really be expressed as fraction or percentage?
  Should it rather be in dB? This should be another patch
  adding the DT property and maths for the attenuation.

Yours,
Linus Walleij