regulator: Use bitfield values for range selectors

  Right now the regulator helpers expect raw register values for the range
selectors. This is different from the voltage selectors, which are
normalized as bitfield values. This leads to a bit of confusion. Also,
raw values are harder to copy from datasheets or match up with them,
as datasheets will typically have bitfield values.

Make the helpers expect bitfield values, and convert existing users.
Include bitops.h explicitly for ffs(), and reorder the header include
statements. While at it, also replace module.h with export.h, since the
only use is EXPORT_SYMBOL_GPL.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 drivers/regulator/atc260x-regulator.c  | 2 +-
 drivers/regulator/bd718x7-regulator.c  | 8 ++++----
 drivers/regulator/helpers.c            | 9 ++++++---
 drivers/regulator/tps6287x-regulator.c | 2 +-
 4 files changed, 12 insertions(+), 9 deletions(-)
  

On 6/8/23 10:56, Chen-Yu Tsai wrote:
> Right now the regulator helpers expect raw register values for the range
> selectors. This is different from the voltage selectors, which are
> normalized as bitfield values. This leads to a bit of confusion. Also,
> raw values are harder to copy from datasheets or match up with them,
> as datasheets will typically have bitfield values.
> 
> Make the helpers expect bitfield values, and convert existing users.
> Include bitops.h explicitly for ffs(), and reorder the header include
> statements. While at it, also replace module.h with export.h, since the
> only use is EXPORT_SYMBOL_GPL.
> 
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>   drivers/regulator/atc260x-regulator.c  | 2 +-
>   drivers/regulator/bd718x7-regulator.c  | 8 ++++----
>   drivers/regulator/helpers.c            | 9 ++++++---
>   drivers/regulator/tps6287x-regulator.c | 2 +-
>   4 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/regulator/atc260x-regulator.c b/drivers/regulator/atc260x-regulator.c
> index 87e237d740bc..0bba33955a1a 100644
> --- a/drivers/regulator/atc260x-regulator.c
> +++ b/drivers/regulator/atc260x-regulator.c
> @@ -37,7 +37,7 @@ static const struct linear_range atc2609a_ldo_voltage_ranges1[] = {
>   };
>   
>   static const unsigned int atc260x_ldo_voltage_range_sel[] = {
> -	0x0, 0x20,
> +	0x0, 0x1,
>   };
>   
>   static int atc260x_dcdc_set_voltage_time_sel(struct regulator_dev *rdev,
> diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
> index b0b9938c20a1..da1eea1207e5 100644
> --- a/drivers/regulator/bd718x7-regulator.c
> +++ b/drivers/regulator/bd718x7-regulator.c
> @@ -289,7 +289,7 @@ static const struct linear_range bd71837_buck5_volts[] = {
>    * and 0x1 for last 3 ranges.
>    */
>   static const unsigned int bd71837_buck5_volt_range_sel[] = {
> -	0x0, 0x0, 0x0, 0x80, 0x80, 0x80
> +	0x0, 0x0, 0x0, 0x1, 0x1, 0x1
>   };
>   
>   /*
> @@ -309,7 +309,7 @@ static const struct linear_range bd71847_buck3_volts[] = {
>   };
>   
>   static const unsigned int bd71847_buck3_volt_range_sel[] = {
> -	0x0, 0x0, 0x0, 0x40, 0x80, 0x80, 0x80
> +	0x0, 0x0, 0x0, 0x1, 0x2, 0x2, 0x2
>   };
>   
>   static const struct linear_range bd71847_buck4_volts[] = {
> @@ -360,7 +360,7 @@ static const struct linear_range bd718xx_ldo1_volts[] = {
>   	REGULATOR_LINEAR_RANGE(1600000, 0x00, 0x03, 100000),
>   };

Shouldn't the
static const unsigned int bd71847_buck4_volt_range_sel[] = { 0x0, 0x40 };

be also converted to { 0x0, 0x1 }? The range mask seems to be:
#define BD71847_BUCK4_RANGE_MASK        0x40

Other than that - the helpers + bd718x7 look good to me.

Yours,
	-- Matti
  

On 6/8/23 10:56, Chen-Yu Tsai wrote:
> Right now the regulator helpers expect raw register values for the range
> selectors. This is different from the voltage selectors, which are
> normalized as bitfield values. This leads to a bit of confusion. Also,
> raw values are harder to copy from datasheets or match up with them,
> as datasheets will typically have bitfield values.
> 
> Make the helpers expect bitfield values, and convert existing users.
> Include bitops.h explicitly for ffs(), and reorder the header include
> statements. While at it, also replace module.h with export.h, since the
> only use is EXPORT_SYMBOL_GPL.
> 
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

For the helpers.c and bd718x7
Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

Yours,
	-- Matti
  

On Fri, Jun 09, 2023 at 01:45:15PM +0300, Matti Vaittinen wrote:

> For the helpers.c and bd718x7
> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>

There's a v2: https://lore.kernel.org/r/20230609075032.2804554-1-wenst@chromium.org
  

On 6/9/23 13:53, Mark Brown wrote:
> On Fri, Jun 09, 2023 at 01:45:15PM +0300, Matti Vaittinen wrote:
> 
>> For the helpers.c and bd718x7
>> Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> There's a v2: https://lore.kernel.org/r/20230609075032.2804554-1-wenst@chromium.org

Right, thanks! I replied to a wrong mail!

I'll send the tag to the correct version.
  

From: Chen-Yu Tsai
> Sent: 08 June 2023 08:57
> 
> Right now the regulator helpers expect raw register values for the range
> selectors. This is different from the voltage selectors, which are
> normalized as bitfield values. This leads to a bit of confusion. Also,
> raw values are harder to copy from datasheets or match up with them,
> as datasheets will typically have bitfield values.
> 
> Make the helpers expect bitfield values, and convert existing users.
> Include bitops.h explicitly for ffs(), and reorder the header include
> statements. While at it, also replace module.h with export.h, since the
> only use is EXPORT_SYMBOL_GPL.
> 
...
>  static const unsigned int atc260x_ldo_voltage_range_sel[] = {
> -	0x0, 0x20,
> +	0x0, 0x1,
>  };

Is there any way the change can be done so that un-edited
modules fail to compile?
Otherwise the whole thing is an accident waiting to happen.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Sun, Jun 11, 2023 at 1:10 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Chen-Yu Tsai
> > Sent: 08 June 2023 08:57
> >
> > Right now the regulator helpers expect raw register values for the range
> > selectors. This is different from the voltage selectors, which are
> > normalized as bitfield values. This leads to a bit of confusion. Also,
> > raw values are harder to copy from datasheets or match up with them,
> > as datasheets will typically have bitfield values.
> >
> > Make the helpers expect bitfield values, and convert existing users.
> > Include bitops.h explicitly for ffs(), and reorder the header include
> > statements. While at it, also replace module.h with export.h, since the
> > only use is EXPORT_SYMBOL_GPL.
> >
> ...
> >  static const unsigned int atc260x_ldo_voltage_range_sel[] = {
> > -     0x0, 0x20,
> > +     0x0, 0x1,
> >  };
>
> Is there any way the change can be done so that un-edited
> modules fail to compile?
> Otherwise the whole thing is an accident waiting to happen.

I think we could change the field name in the regulator description?
But unsuspecting end users / developers might just edit the name and not
see that the scheme has changed.

Or we could add a sanity check at runtime that checks the values during
regulator registration. How does that sound?

Mark, is this something you'd like?


ChenYu
  

From: Chen-Yu Tsai
> Sent: 12 June 2023 04:39
> 
> On Sun, Jun 11, 2023 at 1:10 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Chen-Yu Tsai
> > > Sent: 08 June 2023 08:57
> > >
> > > Right now the regulator helpers expect raw register values for the range
> > > selectors. This is different from the voltage selectors, which are
> > > normalized as bitfield values. This leads to a bit of confusion. Also,
> > > raw values are harder to copy from datasheets or match up with them,
> > > as datasheets will typically have bitfield values.
> > >
> > > Make the helpers expect bitfield values, and convert existing users.
> > > Include bitops.h explicitly for ffs(), and reorder the header include
> > > statements. While at it, also replace module.h with export.h, since the
> > > only use is EXPORT_SYMBOL_GPL.
> > >
> > ...
> > >  static const unsigned int atc260x_ldo_voltage_range_sel[] = {
> > > -     0x0, 0x20,
> > > +     0x0, 0x1,
> > >  };
> >
> > Is there any way the change can be done so that un-edited
> > modules fail to compile?
> > Otherwise the whole thing is an accident waiting to happen.
> 
> I think we could change the field name in the regulator description?
> But unsuspecting end users / developers might just edit the name and not
> see that the scheme has changed.
> 
> Or we could add a sanity check at runtime that checks the values during
> regulator registration. How does that sound?
> 
> Mark, is this something you'd like?

Can you change the name of the function the values are passed to?
Or maybe change the type to 'unsigned char' (assuming bit numbers
are small).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  

On Mon, Jun 12, 2023 at 3:48 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Chen-Yu Tsai
> > Sent: 12 June 2023 04:39
> >
> > On Sun, Jun 11, 2023 at 1:10 AM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Chen-Yu Tsai
> > > > Sent: 08 June 2023 08:57
> > > >
> > > > Right now the regulator helpers expect raw register values for the range
> > > > selectors. This is different from the voltage selectors, which are
> > > > normalized as bitfield values. This leads to a bit of confusion. Also,
> > > > raw values are harder to copy from datasheets or match up with them,
> > > > as datasheets will typically have bitfield values.
> > > >
> > > > Make the helpers expect bitfield values, and convert existing users.
> > > > Include bitops.h explicitly for ffs(), and reorder the header include
> > > > statements. While at it, also replace module.h with export.h, since the
> > > > only use is EXPORT_SYMBOL_GPL.
> > > >
> > > ...
> > > >  static const unsigned int atc260x_ldo_voltage_range_sel[] = {
> > > > -     0x0, 0x20,
> > > > +     0x0, 0x1,
> > > >  };
> > >
> > > Is there any way the change can be done so that un-edited
> > > modules fail to compile?
> > > Otherwise the whole thing is an accident waiting to happen.
> >
> > I think we could change the field name in the regulator description?
> > But unsuspecting end users / developers might just edit the name and not
> > see that the scheme has changed.
> >
> > Or we could add a sanity check at runtime that checks the values during
> > regulator registration. How does that sound?
> >
> > Mark, is this something you'd like?
>
> Can you change the name of the function the values are passed to?

    .set_voltage_sel = regulator_set_voltage_sel_pickable_regmap,

This one, and

    .get_voltage_sel = regulator_get_voltage_sel_pickable_regmap,

which calls the helper regulator_range_selector_to_index() that actually
uses the values.

Not sure how changing the helper names would help, or how it is different
from changing the field name? The names are already quite long but spot
on.

> Or maybe change the type to 'unsigned char' (assuming bit numbers
> are small).

That doesn't quite work if the range selector is in the upper four bits.
The values would fit nevertheless.

ChenYu
  

From: Chen-Yu Tsai
> Sent: 12 June 2023 10:27
> To: David Laight <David.Laight@ACULAB.COM>
> Cc: Mark Brown <broonie@kernel.org>; Liam Girdwood <lgirdwood@gmail.com>; Manivannan Sadhasivam
> <mani@kernel.org>; Cristian Ciocaltea <cristian.ciocaltea@gmail.com>; Matti Vaittinen
> <mazziesaccount@gmail.com>; linux-actions@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
> arm-kernel@lists.infradead.org
> Subject: Re: [PATCH] regulator: Use bitfield values for range selectors
> 
> On Mon, Jun 12, 2023 at 3:48 PM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Chen-Yu Tsai
> > > Sent: 12 June 2023 04:39
> > >
> > > On Sun, Jun 11, 2023 at 1:10 AM David Laight <David.Laight@aculab.com> wrote:
> > > >
> > > > From: Chen-Yu Tsai
> > > > > Sent: 08 June 2023 08:57
> > > > >
> > > > > Right now the regulator helpers expect raw register values for the range
> > > > > selectors. This is different from the voltage selectors, which are
> > > > > normalized as bitfield values. This leads to a bit of confusion. Also,
> > > > > raw values are harder to copy from datasheets or match up with them,
> > > > > as datasheets will typically have bitfield values.
> > > > >
> > > > > Make the helpers expect bitfield values, and convert existing users.
> > > > > Include bitops.h explicitly for ffs(), and reorder the header include
> > > > > statements. While at it, also replace module.h with export.h, since the
> > > > > only use is EXPORT_SYMBOL_GPL.
> > > > >
> > > > ...
> > > > >  static const unsigned int atc260x_ldo_voltage_range_sel[] = {
> > > > > -     0x0, 0x20,
> > > > > +     0x0, 0x1,
> > > > >  };
> > > >
> > > > Is there any way the change can be done so that un-edited
> > > > modules fail to compile?
> > > > Otherwise the whole thing is an accident waiting to happen.
> > >
> > > I think we could change the field name in the regulator description?
> > > But unsuspecting end users / developers might just edit the name and not
> > > see that the scheme has changed.

Looking at it, that would seem to be needed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)