* [PATCH v2 1/3] hwmon: (pmbus) Introduce and use cached vout margins
2022-06-10 11:47 [PATCH v2 0/3] hwmon: (pmbus/ltc2978) Set voltage resolution Mårten Lindahl
@ 2022-06-10 11:47 ` Mårten Lindahl
2022-06-10 11:47 ` [PATCH v2 2/3] hwmon: (pmbus) Add list_voltage to pmbus ops Mårten Lindahl
2022-06-10 11:47 ` [PATCH v2 3/3] hwmon: (pmbus/ltc2978) Set voltage resolution Mårten Lindahl
2 siblings, 0 replies; 8+ messages in thread
From: Mårten Lindahl @ 2022-06-10 11:47 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare; +Cc: linux-hwmon, kernel, Mårten Lindahl
When setting a new voltage the voltage boundaries are read every time to
check that the new voltage is within the proper range. Checking these
voltage boundaries consists of reading one of PMBUS_MFR_VOUT_MIN/
PMBUS_VOUT_MARGIN_LOW registers and then PMBUS_MFR_VOUT_MAX/
PMBUS_VOUT_MARGIN_HIGH together with writing the PMBUS_CLEAR_FAULTS
register.
Since these boundaries are never being changed, it can be cached and
thus saving unnecessary smbus transmissions.
Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
drivers/hwmon/pmbus/pmbus_core.c | 54 +++++++++++++++++++-------------
1 file changed, 32 insertions(+), 22 deletions(-)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 02912022853d..478dda49a45f 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -104,6 +104,9 @@ struct pmbus_data {
s16 currpage; /* current page, -1 for unknown/unset */
s16 currphase; /* current phase, 0xff for all, -1 for unknown/unset */
+
+ int vout_low[PMBUS_PAGES]; /* voltage low margin */
+ int vout_high[PMBUS_PAGES]; /* voltage high margin */
};
struct pmbus_debugfs_entry {
@@ -2667,34 +2670,41 @@ static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uv,
.data = -1,
};
int val = DIV_ROUND_CLOSEST(min_uv, 1000); /* convert to mV */
- int low, high;
*selector = 0;
- if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MIN))
- s.data = _pmbus_read_word_data(client, s.page, 0xff, PMBUS_MFR_VOUT_MIN);
- if (s.data < 0) {
- s.data = _pmbus_read_word_data(client, s.page, 0xff, PMBUS_VOUT_MARGIN_LOW);
- if (s.data < 0)
- return s.data;
- }
- low = pmbus_reg2data(data, &s);
-
- s.data = -1;
- if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MAX))
- s.data = _pmbus_read_word_data(client, s.page, 0xff, PMBUS_MFR_VOUT_MAX);
- if (s.data < 0) {
- s.data = _pmbus_read_word_data(client, s.page, 0xff, PMBUS_VOUT_MARGIN_HIGH);
- if (s.data < 0)
- return s.data;
+ if (!data->vout_low[s.page]) {
+ if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MIN))
+ s.data = _pmbus_read_word_data(client, s.page, 0xff,
+ PMBUS_MFR_VOUT_MIN);
+ if (s.data < 0) {
+ s.data = _pmbus_read_word_data(client, s.page, 0xff,
+ PMBUS_VOUT_MARGIN_LOW);
+ if (s.data < 0)
+ return s.data;
+ }
+ data->vout_low[s.page] = pmbus_reg2data(data, &s);
+ }
+
+ if (!data->vout_high[s.page]) {
+ s.data = -1;
+ if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MAX))
+ s.data = _pmbus_read_word_data(client, s.page, 0xff,
+ PMBUS_MFR_VOUT_MAX);
+ if (s.data < 0) {
+ s.data = _pmbus_read_word_data(client, s.page, 0xff,
+ PMBUS_VOUT_MARGIN_HIGH);
+ if (s.data < 0)
+ return s.data;
+ }
+ data->vout_high[s.page] = pmbus_reg2data(data, &s);
}
- high = pmbus_reg2data(data, &s);
/* Make sure we are within margins */
- if (low > val)
- val = low;
- if (high < val)
- val = high;
+ if (data->vout_low[s.page] > val)
+ val = data->vout_low[s.page];
+ if (data->vout_high[s.page] < val)
+ val = data->vout_high[s.page];
val = pmbus_data2reg(data, &s, val);
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v2 2/3] hwmon: (pmbus) Add list_voltage to pmbus ops
2022-06-10 11:47 [PATCH v2 0/3] hwmon: (pmbus/ltc2978) Set voltage resolution Mårten Lindahl
2022-06-10 11:47 ` [PATCH v2 1/3] hwmon: (pmbus) Introduce and use cached vout margins Mårten Lindahl
@ 2022-06-10 11:47 ` Mårten Lindahl
2022-06-10 14:16 ` Guenter Roeck
2022-06-10 11:47 ` [PATCH v2 3/3] hwmon: (pmbus/ltc2978) Set voltage resolution Mårten Lindahl
2 siblings, 1 reply; 8+ messages in thread
From: Mårten Lindahl @ 2022-06-10 11:47 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare; +Cc: linux-hwmon, kernel, Mårten Lindahl
When checking if a regulator supports a voltage range, the regulator
needs to have a list_voltage callback set to the regulator_ops or else
-EINVAL will be returned. This support does not exist for the pmbus
regulators, so this patch adds pmbus_regulator_list_voltage to the
pmbus_regulator_ops.
Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
drivers/hwmon/pmbus/pmbus_core.c | 50 ++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 478dda49a45f..24ba4b2b03d4 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2711,6 +2711,55 @@ static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uv,
return _pmbus_write_word_data(client, s.page, PMBUS_VOUT_COMMAND, (u16)val);
}
+static int pmbus_regulator_list_voltage(struct regulator_dev *rdev,
+ unsigned int selector)
+{
+ struct device *dev = rdev_get_dev(rdev);
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct pmbus_data *data = i2c_get_clientdata(client);
+ struct pmbus_sensor s = {
+ .page = rdev_get_id(rdev),
+ .class = PSC_VOLTAGE_OUT,
+ .convert = true,
+ .data = -1,
+ };
+ int val = DIV_ROUND_CLOSEST(rdev->desc->min_uV +
+ (rdev->desc->uV_step * selector),
+ 1000); /* convert to mV */
+
+ if (!data->vout_low[s.page]) {
+ if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MIN))
+ s.data = _pmbus_read_word_data(client, s.page, 0xff,
+ PMBUS_MFR_VOUT_MIN);
+ if (s.data < 0) {
+ s.data = _pmbus_read_word_data(client, s.page, 0xff,
+ PMBUS_VOUT_MARGIN_LOW);
+ if (s.data < 0)
+ return s.data;
+ }
+ data->vout_low[s.page] = pmbus_reg2data(data, &s);
+ }
+
+ if (!data->vout_high[s.page]) {
+ s.data = -1;
+ if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MAX))
+ s.data = _pmbus_read_word_data(client, s.page, 0xff,
+ PMBUS_MFR_VOUT_MAX);
+ if (s.data < 0) {
+ s.data = _pmbus_read_word_data(client, s.page, 0xff,
+ PMBUS_VOUT_MARGIN_HIGH);
+ if (s.data < 0)
+ return s.data;
+ }
+ data->vout_high[s.page] = pmbus_reg2data(data, &s);
+ }
+
+ if (val >= data->vout_low[s.page] && val <= data->vout_high[s.page])
+ return val * 1000; /* unit is uV */
+
+ return 0;
+}
+
const struct regulator_ops pmbus_regulator_ops = {
.enable = pmbus_regulator_enable,
.disable = pmbus_regulator_disable,
@@ -2718,6 +2767,7 @@ const struct regulator_ops pmbus_regulator_ops = {
.get_error_flags = pmbus_regulator_get_error_flags,
.get_voltage = pmbus_regulator_get_voltage,
.set_voltage = pmbus_regulator_set_voltage,
+ .list_voltage = pmbus_regulator_list_voltage,
};
EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 2/3] hwmon: (pmbus) Add list_voltage to pmbus ops
2022-06-10 11:47 ` [PATCH v2 2/3] hwmon: (pmbus) Add list_voltage to pmbus ops Mårten Lindahl
@ 2022-06-10 14:16 ` Guenter Roeck
2022-06-13 20:27 ` Marten Lindahl
0 siblings, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2022-06-10 14:16 UTC (permalink / raw)
To: Mårten Lindahl, Jean Delvare; +Cc: linux-hwmon, kernel
On 6/10/22 04:47, Mårten Lindahl wrote:
> When checking if a regulator supports a voltage range, the regulator
> needs to have a list_voltage callback set to the regulator_ops or else
> -EINVAL will be returned. This support does not exist for the pmbus
> regulators, so this patch adds pmbus_regulator_list_voltage to the
> pmbus_regulator_ops.
>
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
> drivers/hwmon/pmbus/pmbus_core.c | 50 ++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 478dda49a45f..24ba4b2b03d4 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2711,6 +2711,55 @@ static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uv,
> return _pmbus_write_word_data(client, s.page, PMBUS_VOUT_COMMAND, (u16)val);
> }
>
> +static int pmbus_regulator_list_voltage(struct regulator_dev *rdev,
> + unsigned int selector)
> +{
> + struct device *dev = rdev_get_dev(rdev);
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct pmbus_data *data = i2c_get_clientdata(client);
> + struct pmbus_sensor s = {
> + .page = rdev_get_id(rdev),
> + .class = PSC_VOLTAGE_OUT,
> + .convert = true,
> + .data = -1,
> + };
> + int val = DIV_ROUND_CLOSEST(rdev->desc->min_uV +
> + (rdev->desc->uV_step * selector),
> + 1000); /* convert to mV */
> +
> + if (!data->vout_low[s.page]) {
> + if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MIN))
> + s.data = _pmbus_read_word_data(client, s.page, 0xff,
> + PMBUS_MFR_VOUT_MIN);
> + if (s.data < 0) {
> + s.data = _pmbus_read_word_data(client, s.page, 0xff,
> + PMBUS_VOUT_MARGIN_LOW);
> + if (s.data < 0)
> + return s.data;
> + }
> + data->vout_low[s.page] = pmbus_reg2data(data, &s);
> + }
> +
> + if (!data->vout_high[s.page]) {
> + s.data = -1;
> + if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MAX))
> + s.data = _pmbus_read_word_data(client, s.page, 0xff,
> + PMBUS_MFR_VOUT_MAX);
> + if (s.data < 0) {
> + s.data = _pmbus_read_word_data(client, s.page, 0xff,
> + PMBUS_VOUT_MARGIN_HIGH);
> + if (s.data < 0)
> + return s.data;
> + }
> + data->vout_high[s.page] = pmbus_reg2data(data, &s);
> + }
> +
The code above is similar to the same code in the first patch. Please
move it into a function (in the first patch).
> + if (val >= data->vout_low[s.page] && val <= data->vout_high[s.page])
> + return val * 1000; /* unit is uV */
> +
> + return 0;
Other drivers return -EINVAL here. Should this be returned as well
if rdev->desc->min_uV or rdev->desc->uV_step is 0, if selector
is out of range, or if data->vout_low[s.page] / data->vout_high[s.page]
is 0 ?
Thanks,
Guenter
> +}
> +
> const struct regulator_ops pmbus_regulator_ops = {
> .enable = pmbus_regulator_enable,
> .disable = pmbus_regulator_disable,
> @@ -2718,6 +2767,7 @@ const struct regulator_ops pmbus_regulator_ops = {
> .get_error_flags = pmbus_regulator_get_error_flags,
> .get_voltage = pmbus_regulator_get_voltage,
> .set_voltage = pmbus_regulator_set_voltage,
> + .list_voltage = pmbus_regulator_list_voltage,
> };
> EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 2/3] hwmon: (pmbus) Add list_voltage to pmbus ops
2022-06-10 14:16 ` Guenter Roeck
@ 2022-06-13 20:27 ` Marten Lindahl
0 siblings, 0 replies; 8+ messages in thread
From: Marten Lindahl @ 2022-06-13 20:27 UTC (permalink / raw)
To: Guenter Roeck
Cc: Mårten Lindahl, Jean Delvare, linux-hwmon@vger.kernel.org,
kernel
On Fri, Jun 10, 2022 at 04:16:46PM +0200, Guenter Roeck wrote:
> On 6/10/22 04:47, Mårten Lindahl wrote:
> > When checking if a regulator supports a voltage range, the regulator
> > needs to have a list_voltage callback set to the regulator_ops or else
> > -EINVAL will be returned. This support does not exist for the pmbus
> > regulators, so this patch adds pmbus_regulator_list_voltage to the
> > pmbus_regulator_ops.
> >
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > ---
> > drivers/hwmon/pmbus/pmbus_core.c | 50 ++++++++++++++++++++++++++++++++
> > 1 file changed, 50 insertions(+)
> >
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index 478dda49a45f..24ba4b2b03d4 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -2711,6 +2711,55 @@ static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uv,
> > return _pmbus_write_word_data(client, s.page, PMBUS_VOUT_COMMAND, (u16)val);
> > }
> >
> > +static int pmbus_regulator_list_voltage(struct regulator_dev *rdev,
> > + unsigned int selector)
> > +{
> > + struct device *dev = rdev_get_dev(rdev);
> > + struct i2c_client *client = to_i2c_client(dev->parent);
> > + struct pmbus_data *data = i2c_get_clientdata(client);
> > + struct pmbus_sensor s = {
> > + .page = rdev_get_id(rdev),
> > + .class = PSC_VOLTAGE_OUT,
> > + .convert = true,
> > + .data = -1,
> > + };
> > + int val = DIV_ROUND_CLOSEST(rdev->desc->min_uV +
> > + (rdev->desc->uV_step * selector),
> > + 1000); /* convert to mV */
> > +
> > + if (!data->vout_low[s.page]) {
> > + if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MIN))
> > + s.data = _pmbus_read_word_data(client, s.page, 0xff,
> > + PMBUS_MFR_VOUT_MIN);
> > + if (s.data < 0) {
> > + s.data = _pmbus_read_word_data(client, s.page, 0xff,
> > + PMBUS_VOUT_MARGIN_LOW);
> > + if (s.data < 0)
> > + return s.data;
> > + }
> > + data->vout_low[s.page] = pmbus_reg2data(data, &s);
> > + }
> > +
> > + if (!data->vout_high[s.page]) {
> > + s.data = -1;
> > + if (pmbus_check_word_register(client, s.page, PMBUS_MFR_VOUT_MAX))
> > + s.data = _pmbus_read_word_data(client, s.page, 0xff,
> > + PMBUS_MFR_VOUT_MAX);
> > + if (s.data < 0) {
> > + s.data = _pmbus_read_word_data(client, s.page, 0xff,
> > + PMBUS_VOUT_MARGIN_HIGH);
> > + if (s.data < 0)
> > + return s.data;
> > + }
> > + data->vout_high[s.page] = pmbus_reg2data(data, &s);
> > + }
> > +
Hi Guenter!
>
> The code above is similar to the same code in the first patch. Please
> move it into a function (in the first patch).
Ok, I will do that. I think it will be one function for low_margin and
one for high_margin.
>
> > + if (val >= data->vout_low[s.page] && val <= data->vout_high[s.page])
> > + return val * 1000; /* unit is uV */
> > +
> > + return 0;
>
> Other drivers return -EINVAL here. Should this be returned as well
> if rdev->desc->min_uV or rdev->desc->uV_step is 0, if selector
> is out of range, or if data->vout_low[s.page] / data->vout_high[s.page]
> is 0 ?
I will add some more checks. I will try to follow what the description
of list_voltages says in include/linux/regulator/driver.h, but it's a
bit free for interpretation, thinking of what counts as unusable
voltages :)
* @list_voltage: Return one of the supported voltages, in microvolts; zero
* if the selector indicates a voltage that is unusable on this system;
* or negative errno. Selectors range from zero to one less than
* regulator_desc.n_voltages. Voltages may be reported in any order.
I guess, as long as a voltage is inside of the low/high margins it is a
valid voltage (even though high and/or low is 0).
Kind regards
Mårten
>
> Thanks,
> Guenter
>
> > +}
> > +
> > const struct regulator_ops pmbus_regulator_ops = {
> > .enable = pmbus_regulator_enable,
> > .disable = pmbus_regulator_disable,
> > @@ -2718,6 +2767,7 @@ const struct regulator_ops pmbus_regulator_ops = {
> > .get_error_flags = pmbus_regulator_get_error_flags,
> > .get_voltage = pmbus_regulator_get_voltage,
> > .set_voltage = pmbus_regulator_set_voltage,
> > + .list_voltage = pmbus_regulator_list_voltage,
> > };
> > EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] hwmon: (pmbus/ltc2978) Set voltage resolution
2022-06-10 11:47 [PATCH v2 0/3] hwmon: (pmbus/ltc2978) Set voltage resolution Mårten Lindahl
2022-06-10 11:47 ` [PATCH v2 1/3] hwmon: (pmbus) Introduce and use cached vout margins Mårten Lindahl
2022-06-10 11:47 ` [PATCH v2 2/3] hwmon: (pmbus) Add list_voltage to pmbus ops Mårten Lindahl
@ 2022-06-10 11:47 ` Mårten Lindahl
2022-06-10 14:27 ` Guenter Roeck
2 siblings, 1 reply; 8+ messages in thread
From: Mårten Lindahl @ 2022-06-10 11:47 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare; +Cc: linux-hwmon, kernel, Mårten Lindahl
The LTC2977 regulator does not set the regulator_desc .n_voltages value
which is needed in order to let the regulator core list the regulator
voltage range.
This patch defines a regulator_desc with a voltage range, and uses it
for defining voltage resolution for regulators LTC2972/LTC2974/LTC2975/
LTC2977/LTC2978/LTC2979/LTC2980/LTM2987 based on that they all have a 16
bit ADC with the same stepwise 122.07uV resolution. It also scales the
resolution to a 1mV resolution which is easier to handle.
Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
drivers/hwmon/pmbus/ltc2978.c | 56 ++++++++++++++++++++++++++++++++---
1 file changed, 52 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index 531aa674a928..7d44e64c61c1 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -562,7 +562,36 @@ static const struct i2c_device_id ltc2978_id[] = {
MODULE_DEVICE_TABLE(i2c, ltc2978_id);
#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+#define LTC2978_ADC_RES 0xFFFF
+#define LTC2978_N_ADC 122
+#define LTC2978_MAX_UV (LTC2978_ADC_RES * LTC2978_N_ADC)
+#define LTC2978_UV_STEP 1000
+
+#define PMBUS_LTC2978_REGULATOR(_name, _id) \
+ [_id] = { \
+ .name = (_name # _id), \
+ .id = (_id), \
+ .of_match = of_match_ptr(_name # _id), \
+ .regulators_node = of_match_ptr("regulators"), \
+ .ops = &pmbus_regulator_ops, \
+ .type = REGULATOR_VOLTAGE, \
+ .owner = THIS_MODULE, \
+ .n_voltages = (LTC2978_MAX_UV / LTC2978_UV_STEP) + 1, \
+ .uV_step = LTC2978_UV_STEP, \
+ }
+
static const struct regulator_desc ltc2978_reg_desc[] = {
+ PMBUS_LTC2978_REGULATOR("vout", 0),
+ PMBUS_LTC2978_REGULATOR("vout", 1),
+ PMBUS_LTC2978_REGULATOR("vout", 2),
+ PMBUS_LTC2978_REGULATOR("vout", 3),
+ PMBUS_LTC2978_REGULATOR("vout", 4),
+ PMBUS_LTC2978_REGULATOR("vout", 5),
+ PMBUS_LTC2978_REGULATOR("vout", 6),
+ PMBUS_LTC2978_REGULATOR("vout", 7),
+};
+
+static const struct regulator_desc ltc2978_reg_desc_default[] = {
PMBUS_REGULATOR("vout", 0),
PMBUS_REGULATOR("vout", 1),
PMBUS_REGULATOR("vout", 2),
@@ -839,10 +868,29 @@ static int ltc2978_probe(struct i2c_client *client)
#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
info->num_regulators = info->pages;
- info->reg_desc = ltc2978_reg_desc;
- if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc)) {
- dev_err(&client->dev, "num_regulators too large!");
- info->num_regulators = ARRAY_SIZE(ltc2978_reg_desc);
+ switch (data->id) {
+ case ltc2972:
+ case ltc2974:
+ case ltc2975:
+ case ltc2977:
+ case ltc2978:
+ case ltc2979:
+ case ltc2980:
+ case ltm2987:
+ info->reg_desc = ltc2978_reg_desc;
+ if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc)) {
+ dev_warn(&client->dev, "num_regulators too large!");
+ info->num_regulators = ARRAY_SIZE(ltc2978_reg_desc);
+ }
+ break;
+ default:
+ info->reg_desc = ltc2978_reg_desc_default;
+ if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc_default)) {
+ dev_warn(&client->dev, "num_regulators too large!");
+ info->num_regulators =
+ ARRAY_SIZE(ltc2978_reg_desc_default);
+ }
+ break;
}
#endif
--
2.30.2
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 3/3] hwmon: (pmbus/ltc2978) Set voltage resolution
2022-06-10 11:47 ` [PATCH v2 3/3] hwmon: (pmbus/ltc2978) Set voltage resolution Mårten Lindahl
@ 2022-06-10 14:27 ` Guenter Roeck
2022-06-13 20:34 ` Marten Lindahl
0 siblings, 1 reply; 8+ messages in thread
From: Guenter Roeck @ 2022-06-10 14:27 UTC (permalink / raw)
To: Mårten Lindahl, Jean Delvare; +Cc: linux-hwmon, kernel
On 6/10/22 04:47, Mårten Lindahl wrote:
> The LTC2977 regulator does not set the regulator_desc .n_voltages value
> which is needed in order to let the regulator core list the regulator
> voltage range.
>
> This patch defines a regulator_desc with a voltage range, and uses it
> for defining voltage resolution for regulators LTC2972/LTC2974/LTC2975/
> LTC2977/LTC2978/LTC2979/LTC2980/LTM2987 based on that they all have a 16
> bit ADC with the same stepwise 122.07uV resolution. It also scales the
> resolution to a 1mV resolution which is easier to handle.
>
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
> drivers/hwmon/pmbus/ltc2978.c | 56 ++++++++++++++++++++++++++++++++---
> 1 file changed, 52 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> index 531aa674a928..7d44e64c61c1 100644
> --- a/drivers/hwmon/pmbus/ltc2978.c
> +++ b/drivers/hwmon/pmbus/ltc2978.c
> @@ -562,7 +562,36 @@ static const struct i2c_device_id ltc2978_id[] = {
> MODULE_DEVICE_TABLE(i2c, ltc2978_id);
>
> #if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
> +#define LTC2978_ADC_RES 0xFFFF
> +#define LTC2978_N_ADC 122
> +#define LTC2978_MAX_UV (LTC2978_ADC_RES * LTC2978_N_ADC)
> +#define LTC2978_UV_STEP 1000
> +
> +#define PMBUS_LTC2978_REGULATOR(_name, _id) \
> + [_id] = { \
> + .name = (_name # _id), \
This needs 'supply_name = "vin"'. See commit 54cc3dbfc10dc ("hwmon:
(pmbus) Add regulator supply into macro").
> + .id = (_id), \
> + .of_match = of_match_ptr(_name # _id), \
> + .regulators_node = of_match_ptr("regulators"), \
> + .ops = &pmbus_regulator_ops, \
> + .type = REGULATOR_VOLTAGE, \
> + .owner = THIS_MODULE, \
> + .n_voltages = (LTC2978_MAX_UV / LTC2978_UV_STEP) + 1, \
> + .uV_step = LTC2978_UV_STEP, \
> + }
Please introduce a new generic macro PMBUS_REGULATOR_STEP()
with additional parameters n_voltages and uV_step and use it here.
PMBUS_REGULATOR() can then be redefined as
#define PMBUS_REGULATOR(n, i) PMBUS_REGULATOR_STEP(n, i, 0, 0)
Thanks,
Guenter
> +
> static const struct regulator_desc ltc2978_reg_desc[] = {
> + PMBUS_LTC2978_REGULATOR("vout", 0),
> + PMBUS_LTC2978_REGULATOR("vout", 1),
> + PMBUS_LTC2978_REGULATOR("vout", 2),
> + PMBUS_LTC2978_REGULATOR("vout", 3),
> + PMBUS_LTC2978_REGULATOR("vout", 4),
> + PMBUS_LTC2978_REGULATOR("vout", 5),
> + PMBUS_LTC2978_REGULATOR("vout", 6),
> + PMBUS_LTC2978_REGULATOR("vout", 7),
> +};
> +
> +static const struct regulator_desc ltc2978_reg_desc_default[] = {
> PMBUS_REGULATOR("vout", 0),
> PMBUS_REGULATOR("vout", 1),
> PMBUS_REGULATOR("vout", 2),
> @@ -839,10 +868,29 @@ static int ltc2978_probe(struct i2c_client *client)
>
> #if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
> info->num_regulators = info->pages;
> - info->reg_desc = ltc2978_reg_desc;
> - if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc)) {
> - dev_err(&client->dev, "num_regulators too large!");
> - info->num_regulators = ARRAY_SIZE(ltc2978_reg_desc);
> + switch (data->id) {
> + case ltc2972:
> + case ltc2974:
> + case ltc2975:
> + case ltc2977:
> + case ltc2978:
> + case ltc2979:
> + case ltc2980:
> + case ltm2987:
> + info->reg_desc = ltc2978_reg_desc;
> + if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc)) {
> + dev_warn(&client->dev, "num_regulators too large!");
> + info->num_regulators = ARRAY_SIZE(ltc2978_reg_desc);
> + }
> + break;
> + default:
> + info->reg_desc = ltc2978_reg_desc_default;
> + if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc_default)) {
> + dev_warn(&client->dev, "num_regulators too large!");
> + info->num_regulators =
> + ARRAY_SIZE(ltc2978_reg_desc_default);
> + }
> + break;
> }
> #endif
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 3/3] hwmon: (pmbus/ltc2978) Set voltage resolution
2022-06-10 14:27 ` Guenter Roeck
@ 2022-06-13 20:34 ` Marten Lindahl
0 siblings, 0 replies; 8+ messages in thread
From: Marten Lindahl @ 2022-06-13 20:34 UTC (permalink / raw)
To: Guenter Roeck
Cc: Mårten Lindahl, Jean Delvare, linux-hwmon@vger.kernel.org,
kernel
On Fri, Jun 10, 2022 at 04:27:55PM +0200, Guenter Roeck wrote:
> On 6/10/22 04:47, Mårten Lindahl wrote:
> > The LTC2977 regulator does not set the regulator_desc .n_voltages value
> > which is needed in order to let the regulator core list the regulator
> > voltage range.
> >
> > This patch defines a regulator_desc with a voltage range, and uses it
> > for defining voltage resolution for regulators LTC2972/LTC2974/LTC2975/
> > LTC2977/LTC2978/LTC2979/LTC2980/LTM2987 based on that they all have a 16
> > bit ADC with the same stepwise 122.07uV resolution. It also scales the
> > resolution to a 1mV resolution which is easier to handle.
> >
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > ---
> > drivers/hwmon/pmbus/ltc2978.c | 56 ++++++++++++++++++++++++++++++++---
> > 1 file changed, 52 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> > index 531aa674a928..7d44e64c61c1 100644
> > --- a/drivers/hwmon/pmbus/ltc2978.c
> > +++ b/drivers/hwmon/pmbus/ltc2978.c
> > @@ -562,7 +562,36 @@ static const struct i2c_device_id ltc2978_id[] = {
> > MODULE_DEVICE_TABLE(i2c, ltc2978_id);
> >
> > #if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
> > +#define LTC2978_ADC_RES 0xFFFF
> > +#define LTC2978_N_ADC 122
> > +#define LTC2978_MAX_UV (LTC2978_ADC_RES * LTC2978_N_ADC)
> > +#define LTC2978_UV_STEP 1000
> > +
> > +#define PMBUS_LTC2978_REGULATOR(_name, _id) \
> > + [_id] = { \
> > + .name = (_name # _id), \
>
> This needs 'supply_name = "vin"'. See commit 54cc3dbfc10dc ("hwmon:
> (pmbus) Add regulator supply into macro").
>
> > + .id = (_id), \
> > + .of_match = of_match_ptr(_name # _id), \
> > + .regulators_node = of_match_ptr("regulators"), \
> > + .ops = &pmbus_regulator_ops, \
> > + .type = REGULATOR_VOLTAGE, \
> > + .owner = THIS_MODULE, \
> > + .n_voltages = (LTC2978_MAX_UV / LTC2978_UV_STEP) + 1, \
> > + .uV_step = LTC2978_UV_STEP, \
> > + }
>
> Please introduce a new generic macro PMBUS_REGULATOR_STEP()
> with additional parameters n_voltages and uV_step and use it here.
> PMBUS_REGULATOR() can then be redefined as
>
> #define PMBUS_REGULATOR(n, i) PMBUS_REGULATOR_STEP(n, i, 0, 0)
Hi Guenter!
Yes, that's a good idea. I will do that, and then there wont be any risk
of missing attributes like this new 'supply_name' as it is only defined at
one place.
Kind regards
Mårten
>
> Thanks,
> Guenter
>
> > +
> > static const struct regulator_desc ltc2978_reg_desc[] = {
> > + PMBUS_LTC2978_REGULATOR("vout", 0),
> > + PMBUS_LTC2978_REGULATOR("vout", 1),
> > + PMBUS_LTC2978_REGULATOR("vout", 2),
> > + PMBUS_LTC2978_REGULATOR("vout", 3),
> > + PMBUS_LTC2978_REGULATOR("vout", 4),
> > + PMBUS_LTC2978_REGULATOR("vout", 5),
> > + PMBUS_LTC2978_REGULATOR("vout", 6),
> > + PMBUS_LTC2978_REGULATOR("vout", 7),
> > +};
> > +
> > +static const struct regulator_desc ltc2978_reg_desc_default[] = {
> > PMBUS_REGULATOR("vout", 0),
> > PMBUS_REGULATOR("vout", 1),
> > PMBUS_REGULATOR("vout", 2),
> > @@ -839,10 +868,29 @@ static int ltc2978_probe(struct i2c_client *client)
> >
> > #if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
> > info->num_regulators = info->pages;
> > - info->reg_desc = ltc2978_reg_desc;
> > - if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc)) {
> > - dev_err(&client->dev, "num_regulators too large!");
> > - info->num_regulators = ARRAY_SIZE(ltc2978_reg_desc);
> > + switch (data->id) {
> > + case ltc2972:
> > + case ltc2974:
> > + case ltc2975:
> > + case ltc2977:
> > + case ltc2978:
> > + case ltc2979:
> > + case ltc2980:
> > + case ltm2987:
> > + info->reg_desc = ltc2978_reg_desc;
> > + if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc)) {
> > + dev_warn(&client->dev, "num_regulators too large!");
> > + info->num_regulators = ARRAY_SIZE(ltc2978_reg_desc);
> > + }
> > + break;
> > + default:
> > + info->reg_desc = ltc2978_reg_desc_default;
> > + if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc_default)) {
> > + dev_warn(&client->dev, "num_regulators too large!");
> > + info->num_regulators =
> > + ARRAY_SIZE(ltc2978_reg_desc_default);
> > + }
> > + break;
> > }
> > #endif
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread