Re: [PATCH v2] iio: light: tsl2591: simplify tsl2591_persist functions via lookup table

Re: [PATCH v2] iio: light: tsl2591: simplify tsl2591_persist functions via lookup table

[David Lechner]

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.