* [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 related [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-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
* 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
end of thread, other threads:[~2020-02-11 8:09 UTC | newest] 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).