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 <[email protected]>
---
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),
};
-static const unsigned int bd718xx_ldo1_volt_range_sel[] = { 0x0, 0x20 };
+static const unsigned int bd718xx_ldo1_volt_range_sel[] = { 0x0, 0x1 };
/*
* LDO2
@@ -403,7 +403,7 @@ static const struct linear_range bd71847_ldo5_volts[] = {
REGULATOR_LINEAR_RANGE(800000, 0x00, 0x0F, 100000),
};
-static const unsigned int bd71847_ldo5_volt_range_sel[] = { 0x0, 0x20 };
+static const unsigned int bd71847_ldo5_volt_range_sel[] = { 0x0, 0x1 };
/*
* LDO6
diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index 586f42e378ee..8bdafea70d36 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -5,13 +5,14 @@
// Copyright 2007, 2008 Wolfson Microelectronics PLC.
// Copyright 2008 SlimLogic Ltd.
-#include <linux/kernel.h>
-#include <linux/err.h>
+#include <linux/bitops.h>
#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/kernel.h>
#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
#include <linux/regulator/driver.h>
-#include <linux/module.h>
#include "internal.h"
@@ -108,6 +109,7 @@ static int regulator_range_selector_to_index(struct regulator_dev *rdev,
return -EINVAL;
rval &= rdev->desc->vsel_range_mask;
+ rval >>= ffs(rdev->desc->vsel_range_mask) - 1;
for (i = 0; i < rdev->desc->n_linear_ranges; i++) {
if (rdev->desc->linear_range_selectors[i] == rval)
@@ -195,6 +197,7 @@ int regulator_set_voltage_sel_pickable_regmap(struct regulator_dev *rdev,
sel += rdev->desc->linear_ranges[i].min_sel;
range = rdev->desc->linear_range_selectors[i];
+ range <<= ffs(rdev->desc->vsel_mask) - 1;
if (rdev->desc->vsel_reg == rdev->desc->vsel_range_reg) {
ret = regmap_update_bits(rdev->regmap,
diff --git a/drivers/regulator/tps6287x-regulator.c b/drivers/regulator/tps6287x-regulator.c
index 870e63ce3ff2..3139a0cbb6f7 100644
--- a/drivers/regulator/tps6287x-regulator.c
+++ b/drivers/regulator/tps6287x-regulator.c
@@ -41,7 +41,7 @@ static const struct linear_range tps6287x_voltage_ranges[] = {
};
static const unsigned int tps6287x_voltage_range_sel[] = {
- 0x0, 0x4, 0x8, 0xC
+ 0x0, 0x1, 0x2, 0x3
};
static const unsigned int tps6287x_ramp_table[] = {
--
2.41.0.rc0.172.g3f132b7071-goog
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 <[email protected]>
> ---
> 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
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
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 <[email protected]>
For the helpers.c and bd718x7
Reviewed-by: Matti Vaittinen <[email protected]>
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Fri, Jun 09, 2023 at 01:45:15PM +0300, Matti Vaittinen wrote:
> For the helpers.c and bd718x7
> Reviewed-by: Matti Vaittinen <[email protected]>
There's a v2: https://lore.kernel.org/r/[email protected]
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 <[email protected]>
>
> There's a v2: https://lore.kernel.org/r/[email protected]
Right, thanks! I replied to a wrong mail!
I'll send the tag to the correct version.
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
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 <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
>
> From: Chen-Yu Tsai
> > Sent: 12 June 2023 04:39
> >
> > On Sun, Jun 11, 2023 at 1:10 AM David Laight <[email protected]> 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 <[email protected]>
> Cc: Mark Brown <[email protected]>; Liam Girdwood <[email protected]>; Manivannan Sadhasivam
> <[email protected]>; Cristian Ciocaltea <[email protected]>; Matti Vaittinen
> <[email protected]>; [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH] regulator: Use bitfield values for range selectors
>
> On Mon, Jun 12, 2023 at 3:48 PM David Laight <[email protected]> wrote:
> >
> > From: Chen-Yu Tsai
> > > Sent: 12 June 2023 04:39
> > >
> > > On Sun, Jun 11, 2023 at 1:10 AM David Laight <[email protected]> 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)