* [PATCH] iio: adc: sc27xx: Refactor sc27xx_adc_scale_init to reduce duplication
@ 2025-04-28 3:42 Gabriel Geraldino
2025-05-05 16:24 ` Jonathan Cameron
0 siblings, 1 reply; 3+ messages in thread
From: Gabriel Geraldino @ 2025-04-28 3:42 UTC (permalink / raw)
To: jic23, orsonzhai, Baolin Wang
Cc: gabrielgeraldinosouza, Nicolas Borsari, linux-iio
Simplify the sc27xx_adc_scale_init() function by refactoring scale ratio
initialization logic for SC2720, SC2730, and SC2731 PMICs. This removes
code duplication by using a data structure to represent channel/scale
mapping.
This change improves readability and maintainability without changing
functional behavior.
Signed-off-by: Gabriel Geraldino <gabrielgeraldinosouza@gmail.com>
Co-developed-by: Nicolas Borsari <nicborsari@usp.br>
Signed-off-by: Nicolas Borsari <nicborsari@usp.br>
---
drivers/iio/adc/sc27xx_adc.c | 90 +++++++++++++-----------------------
1 file changed, 32 insertions(+), 58 deletions(-)
diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
index 2535c2c3e..a8312859d 100644
--- a/drivers/iio/adc/sc27xx_adc.c
+++ b/drivers/iio/adc/sc27xx_adc.c
@@ -407,78 +407,52 @@ static int sc2731_adc_get_ratio(int channel, int scale)
/*
* According to the datasheet set specific value on some channel.
*/
+struct adc_channel_scale {
+ int channel;
+ int scale;
+};
+
+static void sc27xx_adc_scale_init(struct sc27xx_adc_data *data,
+ const struct adc_channel_scale *table,
+ int table_len,
+ int default_scale)
+{
+ int j;
+
+ for (j = 0; j < sizeof(data->channel_scale); j++)
+ data->channel_scale[j] = default_scale;
+
+ for (j = 0; j < table_len; j++)
+ data->channel_scale[table[j].channel] = table[j].scale;
+}
+
static void sc2720_adc_scale_init(struct sc27xx_adc_data *data)
{
- int i;
-
- for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
- switch (i) {
- case 5:
- data->channel_scale[i] = 3;
- break;
- case 7:
- case 9:
- data->channel_scale[i] = 2;
- break;
- case 13:
- data->channel_scale[i] = 1;
- break;
- case 19:
- case 30:
- case 31:
- data->channel_scale[i] = 3;
- break;
- default:
- data->channel_scale[i] = 0;
- break;
- }
- }
+ static const struct adc_channel_scale sc2720_table[] = {
+ {5, 3}, {7, 2}, {9, 2}, {13, 1}, {19, 3}, {30, 3}, {31, 3}
+ };
+ sc27xx_adc_scale_init(data, sc2720_table, ARRAY_SIZE(sc2720_table), 0);
}
static void sc2730_adc_scale_init(struct sc27xx_adc_data *data)
{
- int i;
-
- for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
- switch (i) {
- case 5:
- case 10:
- case 19:
- case 30:
- case 31:
- data->channel_scale[i] = 3;
- break;
- case 7:
- case 9:
- data->channel_scale[i] = 2;
- break;
- case 13:
- data->channel_scale[i] = 1;
- break;
- default:
- data->channel_scale[i] = 0;
- break;
- }
- }
+ static const struct adc_channel_scale sc2730_table[] = {
+ {5, 3}, {10, 3}, {19, 3}, {30, 3}, {31, 3},
+ {7, 2}, {9, 2}, {13, 1}
+ };
+ sc27xx_adc_scale_init(data, sc2730_table, ARRAY_SIZE(sc2730_table), 0);
}
static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
{
- int i;
/*
* In the current software design, SC2731 support 2 scales,
* channels 5 uses big scale, others use smale.
*/
- for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
- switch (i) {
- case 5:
- data->channel_scale[i] = 1;
- break;
- default:
- data->channel_scale[i] = 0;
- break;
- }
- }
+ static const struct adc_channel_scale sc2731_table[] = {
+ {5, 1}
+ };
+ sc27xx_adc_scale_init(data, sc2731_table, ARRAY_SIZE(sc2731_table), 0);
}
static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
--
2.39.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] iio: adc: sc27xx: Refactor sc27xx_adc_scale_init to reduce duplication
2025-04-28 3:42 [PATCH] iio: adc: sc27xx: Refactor sc27xx_adc_scale_init to reduce duplication Gabriel Geraldino
@ 2025-05-05 16:24 ` Jonathan Cameron
2025-06-01 18:57 ` Gabriel Geraldino
0 siblings, 1 reply; 3+ messages in thread
From: Jonathan Cameron @ 2025-05-05 16:24 UTC (permalink / raw)
To: Gabriel Geraldino; +Cc: orsonzhai, Baolin Wang, Nicolas Borsari, linux-iio
On Mon, 28 Apr 2025 05:42:22 +0200
Gabriel Geraldino <gabrielgeraldinosouza@gmail.com> wrote:
> Simplify the sc27xx_adc_scale_init() function by refactoring scale ratio
> initialization logic for SC2720, SC2730, and SC2731 PMICs. This removes
> code duplication by using a data structure to represent channel/scale
> mapping.
>
> This change improves readability and maintainability without changing
> functional behavior.
>
> Signed-off-by: Gabriel Geraldino <gabrielgeraldinosouza@gmail.com>
> Co-developed-by: Nicolas Borsari <nicborsari@usp.br>
> Signed-off-by: Nicolas Borsari <nicborsari@usp.br>
Hi Gabriel, Nicolas,
A few comments inline, but overall I'm not sure the code reduction
is sufficiently to cover the resulting loss of readability.
Sometimes a switch is simply clear than a partial look up table.
So nice idea, but I'm not seeing it as being a good move for this particular code.
I'd be slightly interested to see the optimized output of the two approaches, but
this is far from a high performance path so we care a lot more about readability here.
Thanks,
Jonathan
> ---
> drivers/iio/adc/sc27xx_adc.c | 90 +++++++++++++-----------------------
> 1 file changed, 32 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> index 2535c2c3e..a8312859d 100644
> --- a/drivers/iio/adc/sc27xx_adc.c
> +++ b/drivers/iio/adc/sc27xx_adc.c
> @@ -407,78 +407,52 @@ static int sc2731_adc_get_ratio(int channel, int scale)
> /*
> * According to the datasheet set specific value on some channel.
> */
> +struct adc_channel_scale {
> + int channel;
> + int scale;
> +};
> +
> +static void sc27xx_adc_scale_init(struct sc27xx_adc_data *data,
> + const struct adc_channel_scale *table,
> + int table_len,
> + int default_scale)
Style wise, I'd prefer these aligned after the (
I don't think this will even go over 80 chars but if it did that would
be fine if it doesn't go over by much.
> +{
> + int j;
> +
> + for (j = 0; j < sizeof(data->channel_scale); j++)
> + data->channel_scale[j] = default_scale;
> +
> + for (j = 0; j < table_len; j++)
> + data->channel_scale[table[j].channel] = table[j].scale;
> +}
> +
> static void sc2720_adc_scale_init(struct sc27xx_adc_data *data)
> {
> - int i;
> -
> - for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> - switch (i) {
> - case 5:
> - data->channel_scale[i] = 3;
> - break;
> - case 7:
> - case 9:
> - data->channel_scale[i] = 2;
> - break;
> - case 13:
> - data->channel_scale[i] = 1;
> - break;
> - case 19:
> - case 30:
> - case 31:
> - data->channel_scale[i] = 3;
> - break;
> - default:
> - data->channel_scale[i] = 0;
> - break;
> - }
> - }
> + static const struct adc_channel_scale sc2720_table[] = {
> + {5, 3}, {7, 2}, {9, 2}, {13, 1}, {19, 3}, {30, 3}, {31, 3}
Spaces after { and before }
> + };
> + sc27xx_adc_scale_init(data, sc2720_table, ARRAY_SIZE(sc2720_table), 0);
> }
>
> static void sc2730_adc_scale_init(struct sc27xx_adc_data *data)
> {
> - int i;
> -
> - for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> - switch (i) {
> - case 5:
> - case 10:
> - case 19:
> - case 30:
> - case 31:
> - data->channel_scale[i] = 3;
> - break;
> - case 7:
> - case 9:
> - data->channel_scale[i] = 2;
> - break;
> - case 13:
> - data->channel_scale[i] = 1;
> - break;
> - default:
> - data->channel_scale[i] = 0;
> - break;
> - }
> - }
> + static const struct adc_channel_scale sc2730_table[] = {
> + {5, 3}, {10, 3}, {19, 3}, {30, 3}, {31, 3},
> + {7, 2}, {9, 2}, {13, 1}
> + };
> + sc27xx_adc_scale_init(data, sc2730_table, ARRAY_SIZE(sc2730_table), 0);
> }
>
> static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> {
> - int i;
> /*
> * In the current software design, SC2731 support 2 scales,
> * channels 5 uses big scale, others use smale.
> */
> - for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> - switch (i) {
> - case 5:
> - data->channel_scale[i] = 1;
> - break;
> - default:
> - data->channel_scale[i] = 0;
> - break;
> - }
> - }
> + static const struct adc_channel_scale sc2731_table[] = {
> + {5, 1}
> + };
> + sc27xx_adc_scale_init(data, sc2731_table, ARRAY_SIZE(sc2731_table), 0);
> }
>
> static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] iio: adc: sc27xx: Refactor sc27xx_adc_scale_init to reduce duplication
2025-05-05 16:24 ` Jonathan Cameron
@ 2025-06-01 18:57 ` Gabriel Geraldino
0 siblings, 0 replies; 3+ messages in thread
From: Gabriel Geraldino @ 2025-06-01 18:57 UTC (permalink / raw)
Cc: Nicolas Borsari, linux-iio
Hi Jonathan,
Apologies for the late reply, and thanks for your thoughtful feedback.
We understand your concerns, and we agree that in this case,
readability should take precedence.
We'll drop this idea and focus our efforts elsewhere.
Sorry for resending — the previous email was rejected due to HTML formatting.
Best regards,
Gabriel & Nicolas
On Mon, May 5, 2025 at 1:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 28 Apr 2025 05:42:22 +0200
> Gabriel Geraldino <gabrielgeraldinosouza@gmail.com> wrote:
>
> > Simplify the sc27xx_adc_scale_init() function by refactoring scale ratio
> > initialization logic for SC2720, SC2730, and SC2731 PMICs. This removes
> > code duplication by using a data structure to represent channel/scale
> > mapping.
> >
> > This change improves readability and maintainability without changing
> > functional behavior.
> >
> > Signed-off-by: Gabriel Geraldino <gabrielgeraldinosouza@gmail.com>
> > Co-developed-by: Nicolas Borsari <nicborsari@usp.br>
> > Signed-off-by: Nicolas Borsari <nicborsari@usp.br>
> Hi Gabriel, Nicolas,
>
> A few comments inline, but overall I'm not sure the code reduction
> is sufficiently to cover the resulting loss of readability.
> Sometimes a switch is simply clear than a partial look up table.
>
> So nice idea, but I'm not seeing it as being a good move for this particular code.
> I'd be slightly interested to see the optimized output of the two approaches, but
> this is far from a high performance path so we care a lot more about readability here.
>
>
> Thanks,
>
> Jonathan
>
>
> > ---
> > drivers/iio/adc/sc27xx_adc.c | 90 +++++++++++++-----------------------
> > 1 file changed, 32 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/iio/adc/sc27xx_adc.c b/drivers/iio/adc/sc27xx_adc.c
> > index 2535c2c3e..a8312859d 100644
> > --- a/drivers/iio/adc/sc27xx_adc.c
> > +++ b/drivers/iio/adc/sc27xx_adc.c
> > @@ -407,78 +407,52 @@ static int sc2731_adc_get_ratio(int channel, int scale)
> > /*
> > * According to the datasheet set specific value on some channel.
> > */
> > +struct adc_channel_scale {
> > + int channel;
> > + int scale;
> > +};
> > +
> > +static void sc27xx_adc_scale_init(struct sc27xx_adc_data *data,
> > + const struct adc_channel_scale *table,
> > + int table_len,
> > + int default_scale)
>
> Style wise, I'd prefer these aligned after the (
> I don't think this will even go over 80 chars but if it did that would
> be fine if it doesn't go over by much.
>
> > +{
> > + int j;
> > +
> > + for (j = 0; j < sizeof(data->channel_scale); j++)
> > + data->channel_scale[j] = default_scale;
> > +
> > + for (j = 0; j < table_len; j++)
> > + data->channel_scale[table[j].channel] = table[j].scale;
> > +}
> > +
> > static void sc2720_adc_scale_init(struct sc27xx_adc_data *data)
> > {
> > - int i;
> > -
> > - for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> > - switch (i) {
> > - case 5:
> > - data->channel_scale[i] = 3;
> > - break;
> > - case 7:
> > - case 9:
> > - data->channel_scale[i] = 2;
> > - break;
> > - case 13:
> > - data->channel_scale[i] = 1;
> > - break;
> > - case 19:
> > - case 30:
> > - case 31:
> > - data->channel_scale[i] = 3;
> > - break;
> > - default:
> > - data->channel_scale[i] = 0;
> > - break;
> > - }
> > - }
> > + static const struct adc_channel_scale sc2720_table[] = {
> > + {5, 3}, {7, 2}, {9, 2}, {13, 1}, {19, 3}, {30, 3}, {31, 3}
>
> Spaces after { and before }
>
> > + };
> > + sc27xx_adc_scale_init(data, sc2720_table, ARRAY_SIZE(sc2720_table), 0);
> > }
> >
> > static void sc2730_adc_scale_init(struct sc27xx_adc_data *data)
> > {
> > - int i;
> > -
> > - for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> > - switch (i) {
> > - case 5:
> > - case 10:
> > - case 19:
> > - case 30:
> > - case 31:
> > - data->channel_scale[i] = 3;
> > - break;
> > - case 7:
> > - case 9:
> > - data->channel_scale[i] = 2;
> > - break;
> > - case 13:
> > - data->channel_scale[i] = 1;
> > - break;
> > - default:
> > - data->channel_scale[i] = 0;
> > - break;
> > - }
> > - }
> > + static const struct adc_channel_scale sc2730_table[] = {
> > + {5, 3}, {10, 3}, {19, 3}, {30, 3}, {31, 3},
> > + {7, 2}, {9, 2}, {13, 1}
> > + };
> > + sc27xx_adc_scale_init(data, sc2730_table, ARRAY_SIZE(sc2730_table), 0);
> > }
> >
> > static void sc2731_adc_scale_init(struct sc27xx_adc_data *data)
> > {
> > - int i;
> > /*
> > * In the current software design, SC2731 support 2 scales,
> > * channels 5 uses big scale, others use smale.
> > */
> > - for (i = 0; i < SC27XX_ADC_CHANNEL_MAX; i++) {
> > - switch (i) {
> > - case 5:
> > - data->channel_scale[i] = 1;
> > - break;
> > - default:
> > - data->channel_scale[i] = 0;
> > - break;
> > - }
> > - }
> > + static const struct adc_channel_scale sc2731_table[] = {
> > + {5, 1}
> > + };
> > + sc27xx_adc_scale_init(data, sc2731_table, ARRAY_SIZE(sc2731_table), 0);
> > }
> >
> > static int sc27xx_adc_read(struct sc27xx_adc_data *data, int channel,
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-06-01 18:57 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-28 3:42 [PATCH] iio: adc: sc27xx: Refactor sc27xx_adc_scale_init to reduce duplication Gabriel Geraldino
2025-05-05 16:24 ` Jonathan Cameron
2025-06-01 18:57 ` Gabriel Geraldino
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).