Linux Input Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] RFT: iio: gp2ap002: Replace LUT with math
@ 2020-02-08 12:33 Linus Walleij
  2020-02-09 10:27 ` Gregor Riepl
  2020-02-10 22:53 ` Jonathan Bakker
  0 siblings, 2 replies; 9+ messages in thread
From: Linus Walleij @ 2020-02-08 12:33 UTC (permalink / raw)
  To: Jonathan Cameron, linux-iio
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-input, Linus Walleij, Jonathan Bakker

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));
+	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));
+
+	return (int)lux;
 }
 
 static int gp2ap002_read_raw(struct iio_dev *indio_dev,
-- 
2.21.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RFT: iio: gp2ap002: Replace LUT with math
  2020-02-08 12:33 [PATCH] RFT: iio: gp2ap002: Replace LUT with math Linus Walleij
@ 2020-02-09 10:27 ` Gregor Riepl
  2020-02-10 13:28   ` Linus Walleij
  2020-02-10 22:53 ` Jonathan Bakker
  1 sibling, 1 reply; 9+ messages in thread
From: Gregor Riepl @ 2020-02-09 10:27 UTC (permalink / raw)
  To: Linus Walleij, Jonathan Cameron, linux-iio
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-input, Jonathan Bakker


> -/*
> - * 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 },
> -};
...

> -	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;

That seems like a really, really contrived way to do a table lookup.
According to the table above, all successive input values between 5 and 47 are
covered, so shouldn't this be simple array indexing?

Something like:

#define gp2ap002_value_min 5
#define gp2ap002_value_max 47
static const unsigned int gp2ap002_value_to_illuminance_table[] = {
	3, 4, 5, 6, 8, ...... 39811, 50119
};
#define gp2ap002_table_size ARRAY_SIZE(gp2ap002_value_to_illuminance_table)
if (res < gp2ap002_value_min)
	return gp2ap002_value_to_illuminance_table[0];
if (res > gp2ap002_value_max)
	return gp2ap002_value_to_illuminance_table[gp2ap002_table_size - 1];
lux = gp2ap002_value_to_illuminance_table[res - gp2ap002_value_min];

And since res is linear, interpolation won't even be needed.

What am I missing?

> +	lux = int_pow(10, (res/10));
> +	if (lux > INT_MAX) {
> +		dev_err(gp2ap002->dev, "lux overflow, capped\n");
> +		lux = INT_MAX;
>  	}

This is certainly better, but I wonder if it's worth the computational cost.

Also: It looks like int_pow doesn't saturate, so even though it uses 64bit
integer math, it might be better to move the range check before the calculation.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RFT: iio: gp2ap002: Replace LUT with math
  2020-02-09 10:27 ` Gregor Riepl
@ 2020-02-10 13:28   ` Linus Walleij
  2020-02-10 19:33     ` Gregor Riepl
  0 siblings, 1 reply; 9+ messages in thread
From: Linus Walleij @ 2020-02-10 13:28 UTC (permalink / raw)
  To: Gregor Riepl
  Cc: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Linux Input, Jonathan Bakker

On Sun, Feb 9, 2020 at 11:27 AM Gregor Riepl <onitake@gmail.com> wrote:

> > -     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;
>
> That seems like a really, really contrived way to do a table lookup.
(...)
> And since res is linear, interpolation won't even be needed.

Sorry for my quick hack, patches welcome ;)

> > +     lux = int_pow(10, (res/10));
> > +     if (lux > INT_MAX) {
> > +             dev_err(gp2ap002->dev, "lux overflow, capped\n");
> > +             lux = INT_MAX;
> >       }
>
> This is certainly better, but I wonder if it's worth the computational cost.

We don't do this much so certainly the linecount shrink is
worth it.

> Also: It looks like int_pow doesn't saturate, so even though it uses 64bit
> integer math, it might be better to move the range check before the calculation.

How do you mean I should be doing that without actually
doing the power calculation? (Maybe a dumb question but
math was never my best subject.)

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RFT: iio: gp2ap002: Replace LUT with math
  2020-02-10 13:28   ` Linus Walleij
@ 2020-02-10 19:33     ` Gregor Riepl
  2020-02-10 23:19       ` Jonathan Bakker
  0 siblings, 1 reply; 9+ messages in thread
From: Gregor Riepl @ 2020-02-10 19:33 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Linux Input, Jonathan Bakker

>> Also: It looks like int_pow doesn't saturate, so even though it uses 64bit
>> integer math, it might be better to move the range check before the calculation.
> 
> How do you mean I should be doing that without actually
> doing the power calculation? (Maybe a dumb question but
> math was never my best subject.)

Well, if you clamp the input value to a valid range, there is no risk of
under- or overflow:

#define GP2AP002_ADC_MIN 5
#define GP2AP002_ADC_MAX 47
/* ensure lux stays in a valid range
   lux > 10^(5/10)
   lux < 10^(47/10)
 */
clamp(res, GP2AP002_ADC_MIN, GP2AP002_ADC_MAX);
lux = int_pow(10, (res/10));

However, there is another problem with this solution:
If you divide the input value by 10 before raising it to the power of 10, you
lose a lot of precision. Keep in mind that you're doing integer math here.
The input range is very limited, so reducing it further will also reduce the
number of lux steps: int((47-5)/10) = 4, so you will end up with only 4
luminance steps.

Instead of messing with the precision, I propose simplifying the original code
to a simple table lookup.
This will reduce constant memory usage to 42 values * 16 bit = 84 bytes and
computational complexity to one single memory reference.
While I'm sure there is a more optimal solution, I think it's the easiest to
understand with the least impact on accuracy and performance:

#define GP2AP002_ADC_MIN 5
#define GP2AP002_ADC_MAX 47

/*
 * 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[mA] / 10)
 */
static const u16 gp2ap002_illuminance_table[] = {
	3, 4, 5, 6, 8, 10, 12, 16, 20, 25, 32, 40, 50, 63, 79, 100, 126, 158,
	200, 251, 316, 398, 501, 631, 794, 1000, 1259, 1585, 1995, 2512, 3162,
	3981, 5012, 6310, 7943, 10000, 12589, 15849, 19953, 25119, 31623,
	39811, 50119,
};

static int gp2ap002_get_lux(struct gp2ap002 *gp2ap002)
{
	const struct gp2ap002_illuminance *ill1;
	const struct gp2ap002_illuminance *ill2;
	int ret, res;
	u16 lux;

	ret = iio_read_channel_processed(gp2ap002->alsout, &res);
	if (ret < 0)
		return ret;

	dev_dbg(gp2ap002->dev, "read %d mA from ADC\n", res);

	/* ensure we're staying inside the boundaries of the lookup table */
	clamp(res, GP2AP002_ADC_MIN, GP2AP002_ADC_MAX);
	lux = gp2ap002_illuminance_table[res - GP2AP002_ADC_MIN];

	return (int)lux;
}

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RFT: iio: gp2ap002: Replace LUT with math
  2020-02-08 12:33 [PATCH] RFT: iio: gp2ap002: Replace LUT with math Linus Walleij
  2020-02-09 10:27 ` Gregor Riepl
@ 2020-02-10 22:53 ` Jonathan Bakker
  1 sibling, 0 replies; 9+ messages in thread
From: Jonathan Bakker @ 2020-02-10 22:53 UTC (permalink / raw)
  To: Linus Walleij, Jonathan Cameron, linux-iio
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, linux-input

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RFT: iio: gp2ap002: Replace LUT with math
  2020-02-10 19:33     ` Gregor Riepl
@ 2020-02-10 23:19       ` Jonathan Bakker
  2020-02-11  7:15         ` Gregor Riepl
  0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Bakker @ 2020-02-10 23:19 UTC (permalink / raw)
  To: Gregor Riepl, Linus Walleij
  Cc: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Linux Input

Just an FYI - the ADC_MIN should probably be 0 for full darkness,
but I understand the concept and like it :)

I believe the light sensor part to be a Sharp GA1A light detector or similar,
based on the fact that DigiKey had a page (1) that
mentions both together and that the specs for the GA1A1S202WP (2) line up quite well with
those of the GP2AP002.  Note that the datasheet for the GA1A1S202WP even mentions the
illuminance = 10 ^ (current / 10) formula, re-arranged as
current = 10 * log(illuminance), although it specifies uA as opposed to mA which is the same
as the Android libsensors (both the Nexus S (crespo) (3) and Galaxy Nexus (tuna) (4)) versions.
I suspect that this should be adjusted after the call to iio_read_channel_processed().

1. https://web.archive.org/web/20130303022051/http://www.digikey.com/catalog/en/partgroup/ga1a-and-gp2a/26402
2. https://web.archive.org/web/20121221163708/http://www.sharpsma.com/download/GA1A1S202WP-Specpdf
3. https://android.googlesource.com/device/samsung/crespo/+/refs/heads/jb-release/libsensors/LightSensor.cpp
4. https://android.googlesource.com/device/samsung/tuna/+/refs/tags/android-4.3_r3/libsensors/LightSensor.cpp

Thanks,
Jonathan

On 2020-02-10 11:33 a.m., Gregor Riepl wrote:
>>> Also: It looks like int_pow doesn't saturate, so even though it uses 64bit
>>> integer math, it might be better to move the range check before the calculation.
>>
>> How do you mean I should be doing that without actually
>> doing the power calculation? (Maybe a dumb question but
>> math was never my best subject.)
> 
> Well, if you clamp the input value to a valid range, there is no risk of
> under- or overflow:
> 
> #define GP2AP002_ADC_MIN 5
> #define GP2AP002_ADC_MAX 47
> /* ensure lux stays in a valid range
>    lux > 10^(5/10)
>    lux < 10^(47/10)
>  */
> clamp(res, GP2AP002_ADC_MIN, GP2AP002_ADC_MAX);
> lux = int_pow(10, (res/10));
> 
> However, there is another problem with this solution:
> If you divide the input value by 10 before raising it to the power of 10, you
> lose a lot of precision. Keep in mind that you're doing integer math here.
> The input range is very limited, so reducing it further will also reduce the
> number of lux steps: int((47-5)/10) = 4, so you will end up with only 4
> luminance steps.
> 
> Instead of messing with the precision, I propose simplifying the original code
> to a simple table lookup.
> This will reduce constant memory usage to 42 values * 16 bit = 84 bytes and
> computational complexity to one single memory reference.
> While I'm sure there is a more optimal solution, I think it's the easiest to
> understand with the least impact on accuracy and performance:
> 
> #define GP2AP002_ADC_MIN 5
> #define GP2AP002_ADC_MAX 47
> 
> /*
>  * 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[mA] / 10)
>  */
> static const u16 gp2ap002_illuminance_table[] = {
> 	3, 4, 5, 6, 8, 10, 12, 16, 20, 25, 32, 40, 50, 63, 79, 100, 126, 158,
> 	200, 251, 316, 398, 501, 631, 794, 1000, 1259, 1585, 1995, 2512, 3162,
> 	3981, 5012, 6310, 7943, 10000, 12589, 15849, 19953, 25119, 31623,
> 	39811, 50119,
> };
> 
> static int gp2ap002_get_lux(struct gp2ap002 *gp2ap002)
> {
> 	const struct gp2ap002_illuminance *ill1;
> 	const struct gp2ap002_illuminance *ill2;
> 	int ret, res;
> 	u16 lux;
> 
> 	ret = iio_read_channel_processed(gp2ap002->alsout, &res);
> 	if (ret < 0)
> 		return ret;
> 
> 	dev_dbg(gp2ap002->dev, "read %d mA from ADC\n", res);
> 
> 	/* ensure we're staying inside the boundaries of the lookup table */
> 	clamp(res, GP2AP002_ADC_MIN, GP2AP002_ADC_MAX);
> 	lux = gp2ap002_illuminance_table[res - GP2AP002_ADC_MIN];
> 
> 	return (int)lux;
> }
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RFT: iio: gp2ap002: Replace LUT with math
  2020-02-10 23:19       ` Jonathan Bakker
@ 2020-02-11  7:15         ` Gregor Riepl
  2020-02-11  7:51           ` Linus Walleij
  0 siblings, 1 reply; 9+ messages in thread
From: Gregor Riepl @ 2020-02-11  7:15 UTC (permalink / raw)
  To: Jonathan Bakker, Linus Walleij
  Cc: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, Linux Input

> Just an FYI - the ADC_MIN should probably be 0 for full darkness,
> but I understand the concept and like it :)
> 
> I believe the light sensor part to be a Sharp GA1A light detector or similar,
> based on the fact that DigiKey had a page (1) that
> mentions both together and that the specs for the GA1A1S202WP (2) line up quite well with
> those of the GP2AP002.  Note that the datasheet for the GA1A1S202WP even mentions the
> illuminance = 10 ^ (current / 10) formula, re-arranged as
> current = 10 * log(illuminance), although it specifies uA as opposed to mA which is the same
> as the Android libsensors (both the Nexus S (crespo) (3) and Galaxy Nexus (tuna) (4)) versions.
> I suspect that this should be adjusted after the call to iio_read_channel_processed().

How about this, then?
With a full-range lookup table (47 values), it's even possible to avoid
additional constants and simply clamp to the size of the table.


/*
 * 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[mA] / 10)
 *
 * When the ADC measures 0, return 0 lux.
 */
static const u16 gp2ap002_illuminance_table[] = {
	0, 1, 1, 2, 2, 3, 4, 5, 6, 8, 10, 12, 16, 20, 25, 32, 40, 50, 63, 79,
	100, 126, 158, 200, 251, 316, 398, 501, 631, 794, 1000, 1259, 1585,
	1995, 2512, 3162, 3981, 5012, 6310, 7943, 10000, 12589, 15849, 19953,
	25119, 31623, 39811, 50119,
};

static int gp2ap002_get_lux(struct gp2ap002 *gp2ap002)
{
	const struct gp2ap002_illuminance *ill1;
	const struct gp2ap002_illuminance *ill2;
	int ret, res;
	u16 lux;

	ret = iio_read_channel_processed(gp2ap002->alsout, &res);
	if (ret < 0)
		return ret;

	dev_dbg(gp2ap002->dev, "read %d mA from ADC\n", res);

	/* ensure we don't under/overflow */
	clamp(res, 0, ARRAY_SIZE(gp2ap002_illuminance_table) - 1);
	lux = gp2ap002_illuminance_table[res];

	return (int)lux;
}

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RFT: iio: gp2ap002: Replace LUT with math
  2020-02-11  7:15         ` Gregor Riepl
@ 2020-02-11  7:51           ` Linus Walleij
  2020-02-11  8:09             ` Gregor Riepl
  0 siblings, 1 reply; 9+ messages in thread
From: Linus Walleij @ 2020-02-11  7:51 UTC (permalink / raw)
  To: Gregor Riepl
  Cc: Jonathan Bakker, Jonathan Cameron, linux-iio, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Linux Input

On Tue, Feb 11, 2020 at 8:15 AM Gregor Riepl <onitake@gmail.com> wrote:

> How about this, then?
> With a full-range lookup table (47 values), it's even possible to avoid
> additional constants and simply clamp to the size of the table.

This looks VERY good!

Can you provide a patch against upstream (Jonathan's tree)
or do you want me to pick the method and send a patch (I can
add in your Signed-off-by in that case, if you approve).

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] RFT: iio: gp2ap002: Replace LUT with math
  2020-02-11  7:51           ` Linus Walleij
@ 2020-02-11  8:09             ` Gregor Riepl
  0 siblings, 0 replies; 9+ messages in thread
From: Gregor Riepl @ 2020-02-11  8:09 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Jonathan Bakker, Jonathan Cameron, linux-iio, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, Linux Input


> This looks VERY good!
> 
> Can you provide a patch against upstream (Jonathan's tree)
> or do you want me to pick the method and send a patch (I can
> add in your Signed-off-by in that case, if you approve).

I think it will be easier if you just pick the method.

You have my Signed-off.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-08 12:33 [PATCH] RFT: iio: gp2ap002: Replace LUT with math Linus Walleij
2020-02-09 10:27 ` Gregor Riepl
2020-02-10 13:28   ` Linus Walleij
2020-02-10 19:33     ` Gregor Riepl
2020-02-10 23:19       ` Jonathan Bakker
2020-02-11  7:15         ` Gregor Riepl
2020-02-11  7:51           ` Linus Walleij
2020-02-11  8:09             ` Gregor Riepl
2020-02-10 22:53 ` Jonathan Bakker

Linux Input Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-input/0 linux-input/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-input linux-input/ https://lore.kernel.org/linux-input \
		linux-input@vger.kernel.org
	public-inbox-index linux-input

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-input


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git