Re: [PATCH] RFT: iio: gp2ap002: Replace LUT with math

[Jonathan Bakker <xc-racer2@live.ca>]

Hi Linus,

After implementing a small shim from the voltage ADC on my device, I was able to test this.
Please see my inline comments.

On 2020-02-08 4:33 a.m., Linus Walleij wrote:
> This replaces the current-to-lux look up table with some
> integer math using int_pow() from <linux/kernel.h>.
> Drop the unused <linux/math64.h> in the process.
> 
> Cc: Jonathan Bakker <xc-racer2@live.ca>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I would appreciate testing that this give the right
> result. Using math does look a lot better.
> ---
>  drivers/iio/light/gp2ap002.c | 94 +++---------------------------------
>  1 file changed, 7 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/iio/light/gp2ap002.c b/drivers/iio/light/gp2ap002.c
> index 433907619268..153ae0a163ab 100644
> --- a/drivers/iio/light/gp2ap002.c
> +++ b/drivers/iio/light/gp2ap002.c
> @@ -33,7 +33,6 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/interrupt.h>
>  #include <linux/bits.h>
> -#include <linux/math64.h>
>  #include <linux/pm.h>
>  
>  #define GP2AP002_PROX_CHANNEL 0
> @@ -205,71 +204,10 @@ static irqreturn_t gp2ap002_prox_irq(int irq, void *d)
>  	return IRQ_HANDLED;
>  }
>  
> -struct gp2ap002_illuminance {
> -	unsigned int curr;
> -	unsigned int lux;
> -};
> -
> -/*
> - * This array maps current and lux.
> - *
> - * Ambient light sensing range is 3 to 55000 lux.
> - *
> - * This mapping is based on the following formula.
> - * illuminance = 10 ^ (current / 10)
> - */
> -static const struct gp2ap002_illuminance gp2ap002_illuminance_table[] = {
> -	{ .curr		= 5, .lux	= 3 },
> -	{ .curr		= 6, .lux	= 4 },
> -	{ .curr		= 7, .lux	= 5 },
> -	{ .curr		= 8, .lux	= 6 },
> -	{ .curr		= 9, .lux	= 8 },
> -	{ .curr		= 10, .lux	= 10 },
> -	{ .curr		= 11, .lux	= 12 },
> -	{ .curr		= 12, .lux	= 16 },
> -	{ .curr		= 13, .lux	= 20 },
> -	{ .curr		= 14, .lux	= 25 },
> -	{ .curr		= 15, .lux	= 32 },
> -	{ .curr		= 16, .lux	= 40 },
> -	{ .curr		= 17, .lux	= 50 },
> -	{ .curr		= 18, .lux	= 63 },
> -	{ .curr		= 19, .lux	= 79 },
> -	{ .curr		= 20, .lux	= 100 },
> -	{ .curr		= 21, .lux	= 126 },
> -	{ .curr		= 22, .lux	= 158 },
> -	{ .curr		= 23, .lux	= 200 },
> -	{ .curr		= 24, .lux	= 251 },
> -	{ .curr		= 25, .lux	= 316 },
> -	{ .curr		= 26, .lux	= 398 },
> -	{ .curr		= 27, .lux	= 501 },
> -	{ .curr		= 28, .lux	= 631 },
> -	{ .curr		= 29, .lux	= 794 },
> -	{ .curr		= 30, .lux	= 1000 },
> -	{ .curr		= 31, .lux	= 1259 },
> -	{ .curr		= 32, .lux	= 1585 },
> -	{ .curr		= 33, .lux	= 1995 },
> -	{ .curr		= 34, .lux	= 2512 },
> -	{ .curr		= 35, .lux	= 3162 },
> -	{ .curr		= 36, .lux	= 3981 },
> -	{ .curr		= 37, .lux	= 5012 },
> -	{ .curr		= 38, .lux	= 6310 },
> -	{ .curr		= 39, .lux	= 7943 },
> -	{ .curr		= 40, .lux	= 10000 },
> -	{ .curr		= 41, .lux	= 12589 },
> -	{ .curr		= 42, .lux	= 15849 },
> -	{ .curr		= 43, .lux	= 19953 },
> -	{ .curr		= 44, .lux	= 25119 },
> -	{ .curr		= 45, .lux	= 31623 },
> -	{ .curr		= 46, .lux	= 39811 },
> -	{ .curr		= 47, .lux	= 50119 },
> -};
> -
>  static int gp2ap002_get_lux(struct gp2ap002 *gp2ap002)
>  {
> -	const struct gp2ap002_illuminance *ill1;
> -	const struct gp2ap002_illuminance *ill2;
>  	int ret, res;
> -	int i;
> +	u64 lux;
>  
>  	ret = iio_read_channel_processed(gp2ap002->alsout, &res);
>  	if (ret < 0)
> @@ -277,31 +215,13 @@ static int gp2ap002_get_lux(struct gp2ap002 *gp2ap002)
>  
>  	dev_dbg(gp2ap002->dev, "read %d mA from ADC\n", res);
>  
> -	ill1 = &gp2ap002_illuminance_table[0];
> -	if (res < ill1->curr) {
> -		dev_dbg(gp2ap002->dev, "total darkness\n");
> -		return 0;
> -	}
> -	for (i = 0; i < ARRAY_SIZE(gp2ap002_illuminance_table) - 1; i++) {
> -		ill1 = &gp2ap002_illuminance_table[i];
> -		ill2 = &gp2ap002_illuminance_table[i + 1];
> -
> -		if (res > ill2->curr)
> -			continue;
> -		if ((res <= ill1->curr) && (res >= ill2->curr))
> -			break;
> +	lux = int_pow(10, (res/10));

Unfortunately, this doesn't seem to work.  Since it's integer math, it means we
only have an output of 100, 1000, or 10000 depending on the reading.  A LUT is
probably a much better solution.

> +	if (lux > INT_MAX) {
> +		dev_err(gp2ap002->dev, "lux overflow, capped\n");
> +		lux = INT_MAX;
>  	}
> -	if (res > ill2->curr) {
> -		dev_info_once(gp2ap002->dev, "max current overflow\n");
> -		return ill2->curr;
> -	}
> -	/* Interpolate and return */
> -	dev_dbg(gp2ap002->dev, "interpolate index %d and %d\n", i, i + 1);
> -	/* How many steps along the curve */
> -	i = res - ill1->curr; /* x - x0 */
> -	/* Linear interpolation */
> -	return ill1->lux + i *
> -		((ill2->lux - ill1->lux) / (ill2->curr - ill1->curr));

However, this also apparently didn't really work either as I was having what appeared to be
overflow errors (ie when a raw ADC value of 37, this calculation pulls out -52961).

Here's a few entries after add a debug print right after the gp2ap002_get_lux() call in
gp2ap002_read_raw():

gp2ap002 11-0044: read 33 mA from ADC
iio iio:device4: read value of -94193
gp2ap002 11-0044: read 37 mA from ADC
iio iio:device4: read value of -52961
gp2ap002 11-0044: read 39 mA from ADC
iio iio:device4: read value of -32345
gp2ap002 11-0044: read 42 mA from ADC
iio iio:device4: read value of -1421
gp2ap002 11-0044: read 39 mA from ADC
iio iio:device4: read value of -32345
gp2ap002 11-0044: read 29 mA from ADC
iio iio:device4: read value of -135425
gp2ap002 11-0044: read 48 mA from ADC
gp2ap002 11-0044: max current overflow
iio iio:device4: read value of 47
gp2ap002 11-0044: read 33 mA from ADC

My apologies for not catching this first time around.

> +
> +	return (int)lux;
>  }
>  
>  static int gp2ap002_read_raw(struct iio_dev *indio_dev,
> 

Thanks,
Jonathan Bakker