* [PATCH v2] iio: light: tsl2591: simplify tsl2591_persist functions via lookup table
@ 2026-04-19 0:35 LucasRabaquim
2026-04-19 11:36 ` Jonathan Cameron
0 siblings, 1 reply; 5+ messages in thread
From: LucasRabaquim @ 2026-04-19 0:35 UTC (permalink / raw)
To: jic23, dlechner, nuno.sa, andy; +Cc: LucasRabaquim, Matheus Feitosa, linux-iio
Replace the switch-case logic in tsl2591_persist_cycle_to_lit()
and tsl2591_persist_lit_to_cycle() with a bidirectional
lookup table tsl2591_persist_table.
These functions have redundant switch-cases. Now they use a for-loop,
like in tsl2591_als_time_to_fval(). The change simplifies
addition or modification of TSL2591_PRST_ALS_INT_CYCLE_* values by
associating cycle and lit values in a single entry defined by
tsl2591_persist_entry type, reducing code duplication and verbosity.
Signed-off-by: LucasRabaquim <lucas.rabaquim@usp.br>
Co-developed-by: Matheus Feitosa <matheus.feitosa@usp.br>
Signed-off-by: Matheus Feitosa <matheus.feitosa@usp.br>
---
drivers/iio/light/tsl2591.c | 104 +++++++++++++-----------------------
1 file changed, 38 insertions(+), 66 deletions(-)
diff --git a/drivers/iio/light/tsl2591.c b/drivers/iio/light/tsl2591.c
index c5557867ea43..3fe34a48ec2b 100644
--- a/drivers/iio/light/tsl2591.c
+++ b/drivers/iio/light/tsl2591.c
@@ -170,6 +170,12 @@ struct tsl2591_chip {
bool events_enabled;
};
+/* Persist cycle conversion table mapping register cycles to lit values */
+struct tsl2591_persist_entry {
+ u8 cycle;
+ u8 lit;
+};
+
/*
* Period table is ALS persist cycle x integration time setting
* Integration times: 100ms, 200ms, 300ms, 400ms, 500ms, 600ms
@@ -229,80 +235,46 @@ static int tsl2591_multiplier_to_gain(const u32 multiplier)
}
}
+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 },
+};
+
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++) {
+ if (als_persist == tsl2591_persist_table[i].cycle)
+ return tsl2591_persist_table[i].lit;
}
+
+ 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;
}
static int tsl2591_compatible_int_time(struct tsl2591_chip *chip,
--
2.47.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] iio: light: tsl2591: simplify tsl2591_persist functions via lookup table
2026-04-19 0:35 [PATCH v2] iio: light: tsl2591: simplify tsl2591_persist functions via lookup table LucasRabaquim
@ 2026-04-19 11:36 ` Jonathan Cameron
2026-04-20 7:44 ` Andy Shevchenko
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2026-04-19 11:36 UTC (permalink / raw)
To: LucasRabaquim; +Cc: dlechner, nuno.sa, andy, Matheus Feitosa, linux-iio
On Sat, 18 Apr 2026 21:35:43 -0300
LucasRabaquim <lucas.rabaquim@usp.br> wrote:
> Replace the switch-case logic in tsl2591_persist_cycle_to_lit()
> and tsl2591_persist_lit_to_cycle() with a bidirectional
> lookup table tsl2591_persist_table.
>
> These functions have redundant switch-cases. Now they use a for-loop,
> like in tsl2591_als_time_to_fval(). The change simplifies
> addition or modification of TSL2591_PRST_ALS_INT_CYCLE_* values by
> associating cycle and lit values in a single entry defined by
> tsl2591_persist_entry type, reducing code duplication and verbosity.
>
> Signed-off-by: LucasRabaquim <lucas.rabaquim@usp.br>
> Co-developed-by: Matheus Feitosa <matheus.feitosa@usp.br>
> Signed-off-by: Matheus Feitosa <matheus.feitosa@usp.br>
Hi.
First a general process thing. Slow down! You might well have
gotten more feedback on v1 if it had been on list without being
replaced for more than a day. I'd advise generally not sending new
versions for at least a week so multiple reviewers have time to look
at the changes.
In general the change looks good, but I'll leave it on list for
a while before considering picking it up.
Thanks,
Jonathan
> ---
> drivers/iio/light/tsl2591.c | 104 +++++++++++++-----------------------
> 1 file changed, 38 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/iio/light/tsl2591.c b/drivers/iio/light/tsl2591.c
> index c5557867ea43..3fe34a48ec2b 100644
> --- a/drivers/iio/light/tsl2591.c
> +++ b/drivers/iio/light/tsl2591.c
> @@ -170,6 +170,12 @@ struct tsl2591_chip {
> bool events_enabled;
> };
>
> +/* Persist cycle conversion table mapping register cycles to lit values */
> +struct tsl2591_persist_entry {
> + u8 cycle;
> + u8 lit;
> +};
> +
> /*
> * Period table is ALS persist cycle x integration time setting
> * Integration times: 100ms, 200ms, 300ms, 400ms, 500ms, 600ms
> @@ -229,80 +235,46 @@ static int tsl2591_multiplier_to_gain(const u32 multiplier)
> }
> }
>
> +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 },
> +};
> +
> 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++) {
> + if (als_persist == tsl2591_persist_table[i].cycle)
> + return tsl2591_persist_table[i].lit;
> }
> +
> + 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;
> }
>
> static int tsl2591_compatible_int_time(struct tsl2591_chip *chip,
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] iio: light: tsl2591: simplify tsl2591_persist functions via lookup table
2026-04-19 11:36 ` Jonathan Cameron
@ 2026-04-20 7:44 ` Andy Shevchenko
2026-04-25 19:24 ` LucasRabaquim
0 siblings, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2026-04-20 7:44 UTC (permalink / raw)
To: Jonathan Cameron
Cc: LucasRabaquim, dlechner, nuno.sa, andy, Matheus Feitosa,
linux-iio
On Sun, Apr 19, 2026 at 12:36:04PM +0100, Jonathan Cameron wrote:
> On Sat, 18 Apr 2026 21:35:43 -0300
> LucasRabaquim <lucas.rabaquim@usp.br> wrote:
> First a general process thing. Slow down! You might well have
> gotten more feedback on v1 if it had been on list without being
> replaced for more than a day. I'd advise generally not sending new
> versions for at least a week so multiple reviewers have time to look
> at the changes.
>
> In general the change looks good, but I'll leave it on list for
> a while before considering picking it up.
Indeed.
...
> > +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.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] iio: light: tsl2591: simplify tsl2591_persist functions via lookup table
2026-04-20 7:44 ` Andy Shevchenko
@ 2026-04-25 19:24 ` LucasRabaquim
2026-04-26 19:29 ` David Lechner
0 siblings, 1 reply; 5+ messages in thread
From: LucasRabaquim @ 2026-04-25 19:24 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, dlechner, nuno.sa, andy, Matheus Feitosa,
linux-iio
Thank you, Jonathan, David, and Andy, for the feedback, and apologies
for our eagerness to send a new patch right after the first one.
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
Em seg., 20 de abr. de 2026 às 04:44, Andy Shevchenko
<andriy.shevchenko@intel.com> escreveu:
>
> On Sun, Apr 19, 2026 at 12:36:04PM +0100, Jonathan Cameron wrote:
> > On Sat, 18 Apr 2026 21:35:43 -0300
> > LucasRabaquim <lucas.rabaquim@usp.br> wrote:
>
> > First a general process thing. Slow down! You might well have
> > gotten more feedback on v1 if it had been on list without being
> > replaced for more than a day. I'd advise generally not sending new
> > versions for at least a week so multiple reviewers have time to look
> > at the changes.
> >
> > In general the change looks good, but I'll leave it on list for
> > a while before considering picking it up.
>
> Indeed.
>
> ...
>
> > > +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.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] iio: light: tsl2591: simplify tsl2591_persist functions via lookup table
2026-04-25 19:24 ` LucasRabaquim
@ 2026-04-26 19:29 ` David Lechner
0 siblings, 0 replies; 5+ messages 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] 5+ messages in thread
end of thread, other threads:[~2026-04-26 19:29 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-19 0:35 [PATCH v2] iio: light: tsl2591: simplify tsl2591_persist functions via lookup table LucasRabaquim
2026-04-19 11:36 ` Jonathan Cameron
2026-04-20 7:44 ` Andy Shevchenko
2026-04-25 19:24 ` LucasRabaquim
2026-04-26 19:29 ` David Lechner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox