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

[Gregor Riepl <onitake@gmail.com>]

> -/*
> - * 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.