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

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

On Fri, Jan 24, 2020 at 1:47 AM Jonathan Bakker <xc-racer2@live.ca> wrote:

> Thanks for the driver, I've tested it on a first-gen Galaxy S
> device with a GP2AP002A00F.  I have a few comments that I've put inline
> based on my experiences.

Thanks a lot!

> Both shortly after probe (when runtime pm timeout occurs?) and after
> manually disabling the proximity event, this
> irq handler is called.  Since the chip is in low power state, it obviously
> fails to read the proximity value and write to the OCON register below, eg
>
> [    7.215875] gp2ap002 11-0044: error reading proximity
> [    8.105878] gp2ap002 11-0044: error setting up VOUT control 1
>
> Can we do something like disable_irq() in the runtime pm function to prevent
> this?

I added that in v6, I hope this solves your problem.

> The gp2ap002s00f_regmap_i2c_read function works on the gp2ap002a00f as well,
> so this can be simplified/dropped.

Fixed this too in v6.

> > +             if (ch_type != IIO_CURRENT) {
> > +                     dev_err(dev,
> > +                             "wrong type of IIO channel specified for ALSOUT\n");
> > +                     return -EINVAL;
> > +             }
>
> This enforces a current ADC, while several devices besides mine (eg Galaxy Nexus)
> use a resistor and a voltage ADC.  For this case, could we add a device property such as
> sharp,adc-adjustment-ratio to convert from the raw ADC values to a "current" that
> could be used in the lookup table?  So the "current" would be the raw ADC divided
> by that special value.
>
> Instructions for converting the ADC back to the current can be found eg at
> https://android.googlesource.com/device/samsung/crespo/+/2e0ab7265e3039fee787c2216e0c98d92ea0b49e%5E%21/#F0

I'd like to keep that as a future enhancement unless someone is very eager
to get it and has a device they can test it on. Otherwise it will be
just dry-coding
on my part.

I think the property we would add in the device tree in that case should
be the resistance value. This is based on the following assumption
which is indeed a bit of speculation since there is no proper datasheet
for the light sensor part of the component:

- The light sensor part is simply a photodiode
- This emits a nonlinear current in relation to how many
  photons hit the photodiode in a time interval, the relationship
  is described in the curren->lux table we have
- Some vendors do not have any current ADC, so they connect
  this to a resistor, and measure the voltage over the
  resistor because the have a voltage ADC

Since current is linear to the voltage over the resistor, we should
include the resistance in the device tree, then using that the
corresponding current can be calculated and we use the same
look-up table to find the lux. Probably each system may need
to also subtract some bias voltage or so.

But we definately need something to test on to do this right.

Yours,
Linus Walleij