* Re: [PATCH v2] iio: light: tsl2591: simplify tsl2591_persist functions via lookup table
[not found] ` <CAOWnv8HjVGVsg2d8kDt9LyUVp-HbnaWzAxZ_6egw+KtNmEpv7A@mail.gmail.com>
@ 2026-04-26 19:29 ` David Lechner
0 siblings, 0 replies; only message in thread
From: David Lechner @ 2026-04-26 19:29 UTC (permalink / raw)
To: LucasRabaquim, Andy Shevchenko
Cc: Jonathan Cameron, nuno.sa, andy, Matheus Feitosa, linux-iio
On 4/25/26 2:24 PM, LucasRabaquim wrote:
> Thank you, Jonathan, David, and Andy, for the feedback, and apologies
> for our eagerness to send a new patch right after the first one.
Another lesson to learn. :-)
Please don't top-post. Make your reply inline in the context where it
make sense and trim out everything else. (There wasn't much to trim in
this one though since it was already trimmed.)
>> ...
>>
>>>> +static const struct tsl2591_persist_entry tsl2591_persist_table[] = {
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_ANY, 1 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_2, 2 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_3, 3 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_5, 5 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_10, 10 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_15, 15 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_20, 20 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_25, 25 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_30, 30 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_35, 35 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_40, 40 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_45, 45 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_50, 50 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_55, 55 },
>>>> + { TSL2591_PRST_ALS_INT_CYCLE_60, 60 },
>>>> +};
>>
>> Instead, make this to be an index of the just an array of u8:s.
>>
>> ...
>> [TSL2591_PRST_ALS_INT_CYCLE_30] = 30,
>> [TSL2591_PRST_ALS_INT_CYCLE_35] = 35,
>> ...
>>
>> ...
>>
>>>> static int tsl2591_persist_cycle_to_lit(const u8 als_persist)
>>>> {
>>>> - switch (als_persist) {
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_ANY:
>>>> - return 1;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_2:
>>>> - return 2;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_3:
>>>> - return 3;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_5:
>>>> - return 5;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_10:
>>>> - return 10;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_15:
>>>> - return 15;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_20:
>>>> - return 20;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_25:
>>>> - return 25;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_30:
>>>> - return 30;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_35:
>>>> - return 35;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_40:
>>>> - return 40;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_45:
>>>> - return 45;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_50:
>>>> - return 50;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_55:
>>>> - return 55;
>>>> - case TSL2591_PRST_ALS_INT_CYCLE_60:
>>>> - return 60;
>>>> - default:
>>>> - return -EINVAL;
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(tsl2591_persist_table); i++) {
>>
>> Besides the style of
>>
>> for (unsigned int i = 0; i < ARRAY_SIZE(tsl2591_persist_table); i++) {
>>
>>>> + if (als_persist == tsl2591_persist_table[i].cycle)
>>>> + return tsl2591_persist_table[i].lit;
>>>> }
>>
>> ...this loop now not needed at all.
>>
>> if (als_persist < 1 || als_persist >= ARRAY_SIZE(tsl2591_persist_table))
>> return -EINVAL;
>>
>> return tsl2591_persist_table[als_persist];
>>
>>>> + return -EINVAL;
>>>> }
>>
>> ...
>>
>>>> static int tsl2591_persist_lit_to_cycle(const u8 als_persist)
>>>> {
>>>> - switch (als_persist) {
>>>> - case 1:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_ANY;
>>>> - case 2:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_2;
>>>> - case 3:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_3;
>>>> - case 5:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_5;
>>>> - case 10:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_10;
>>>> - case 15:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_15;
>>>> - case 20:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_20;
>>>> - case 25:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_25;
>>>> - case 30:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_30;
>>>> - case 35:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_35;
>>>> - case 40:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_40;
>>>> - case 45:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_45;
>>>> - case 50:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_50;
>>>> - case 55:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_55;
>>>> - case 60:
>>>> - return TSL2591_PRST_ALS_INT_CYCLE_60;
>>>> - default:
>>>> - return -EINVAL;
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(tsl2591_persist_table); i++) {
>>>> + if (als_persist == tsl2591_persist_table[i].lit)
>>>> + return tsl2591_persist_table[i].cycle;
>>>> }
>>>> +
>>>> + return -EINVAL;
>>>> }
>>
>> Ditto.
>>
Here is where putting the reply makes sense where we can see the
relevant previous discussion above and the new discussion below.
>
> Regarding the use of a u8 array indexed by
> TSL2591_PRST_ALS_INT_CYCLE_*: we see it would work very well in
> tsl2591_persist_cycle_to_lit(). However, we don't think the solution
> applies to tsl2591_persist_lit_to_cycle(), since it would require
> another array that, to use the index initialization, would allocate 61
> memory positions with only 15 relevant values actually stored.
>
> We thought of the following, albeit possibly fragile, solution to
> follow Andy's suggestion:
>
> static int tsl2591_persist_lit_to_cycle(const u8 als_persist)
> {
> if(als_persist > 60 || als_persist < 1)
> return -EINVAL;
> if(als_persist % 5 == 0) // Valid values between 5 and 60 inclusive
> return (als_persist/5) + 3;
> if(als_persist < 4) // Cases for 1, 2 and 3
> return als_persist;
> return -EINVAL;
> }
> Otherwise, to keep the array indexed by TSL2591_PRST_ALS_INT_CYCLE_*
> approach we think it would still need a for-loop based solution for
> tsl2591_persist_lit_to_cycle(). We would like to hear your thoughts
> before committing to any direction for a possible new patch.
>
> With Best Regards,
> Lucas Rabaquim
>
Personally, I would go with looping over the lookup array since there
isn't a simple formula that works for all values.
^ permalink raw reply [flat|nested] only message in thread