* [PATCH v1] hwmon: (pmbus/ltc2978) Set voltage resolution
@ 2022-05-30 14:34 Mårten Lindahl
2022-06-01 19:12 ` Guenter Roeck
0 siblings, 1 reply; 4+ messages in thread
From: Mårten Lindahl @ 2022-05-30 14:34 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 support for listing the range or else -EINVAL will be
returned.
This support does not exist for the LTC2977 regulator, so this patch
adds support for list voltage to the pmbus regulators by adding
regulator_list_voltage_linear to the pmbus_regulator_ops. It also
defines the voltage resolution for regulators ltc2972/LTC2974/LTC2975/
LTC2977/LTC2978/LTC2979/LTC2980/LTM2987 based on that they all have the
same stepwise 122.07uV resolution.
Since 122.07uV resolution is very small the resolution is set to a 1mV
resolution to be easier to handle.
Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
drivers/hwmon/pmbus/ltc2978.c | 57 +++++++++++++++++++++++++++++---
drivers/hwmon/pmbus/pmbus_core.c | 1 +
2 files changed, 54 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index 531aa674a928..cfb568c6c155 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -562,7 +562,37 @@ 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), \
+ .supply_name = "vin", \
+ .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 +869,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_err(&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_err(&client->dev, "num_regulators too large!");
+ info->num_regulators =
+ ARRAY_SIZE(ltc2978_reg_desc_default);
+ }
+ break;
}
#endif
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index f2cf0439da37..7d642b57c8b2 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2634,6 +2634,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 = regulator_list_voltage_linear,
};
EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1] hwmon: (pmbus/ltc2978) Set voltage resolution
2022-05-30 14:34 [PATCH v1] hwmon: (pmbus/ltc2978) Set voltage resolution Mårten Lindahl
@ 2022-06-01 19:12 ` Guenter Roeck
2022-06-02 14:40 ` Marten Lindahl
0 siblings, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2022-06-01 19:12 UTC (permalink / raw)
To: Mårten Lindahl; +Cc: Jean Delvare, linux-hwmon, kernel
On Mon, May 30, 2022 at 04:34:46PM +0200, Mårten Lindahl wrote:
> When checking if a regulator supports a voltage range, the regulator
> needs to have support for listing the range or else -EINVAL will be
> returned.
>
> This support does not exist for the LTC2977 regulator, so this patch
> adds support for list voltage to the pmbus regulators by adding
> regulator_list_voltage_linear to the pmbus_regulator_ops. It also
> defines the voltage resolution for regulators ltc2972/LTC2974/LTC2975/
> LTC2977/LTC2978/LTC2979/LTC2980/LTM2987 based on that they all have the
> same stepwise 122.07uV resolution.
>
> Since 122.07uV resolution is very small the resolution is set to a 1mV
> resolution to be easier to handle.
>
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
> drivers/hwmon/pmbus/ltc2978.c | 57 +++++++++++++++++++++++++++++---
> drivers/hwmon/pmbus/pmbus_core.c | 1 +
> 2 files changed, 54 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> index 531aa674a928..cfb568c6c155 100644
> --- a/drivers/hwmon/pmbus/ltc2978.c
> +++ b/drivers/hwmon/pmbus/ltc2978.c
> @@ -562,7 +562,37 @@ 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<space>DEFINE<tab>VALUE, please, and the () around 1000
is unnecessary.
Also, is the range really correct ? The valid / acceptable
voltages are in the range detected in pmbus_regulator_set_voltage(),
based on PMBUS_MFR_VOUT_MIN/PMBUS_VOUT_MARGIN_LOW and
PMBUS_MFR_VOUT_MAX/PMBUS_VOUT_MARGIN_HIGH, and that will likely differ
from the fixed number of voltages provided here.
That makes me wonder if it would make more sense to move this
functionality into the PMBus core code. Any thoughts on that ?
Thanks,
Guenter
> +
> +#define PMBUS_LTC2978_REGULATOR(_name, _id) \
> + [_id] = { \
> + .name = (_name # _id), \
> + .supply_name = "vin", \
> + .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 +869,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_err(&client->dev, "num_regulators too large!");
Let's make this a dev_warn(); it does not result in an error abort,
after all.
> + 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_err(&client->dev, "num_regulators too large!");
Same here.
> + info->num_regulators =
> + ARRAY_SIZE(ltc2978_reg_desc_default);
> + }
> + break;
> }
> #endif
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index f2cf0439da37..7d642b57c8b2 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2634,6 +2634,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 = regulator_list_voltage_linear,
> };
> EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] hwmon: (pmbus/ltc2978) Set voltage resolution
2022-06-01 19:12 ` Guenter Roeck
@ 2022-06-02 14:40 ` Marten Lindahl
2022-06-10 8:54 ` Marten Lindahl
0 siblings, 1 reply; 4+ messages in thread
From: Marten Lindahl @ 2022-06-02 14:40 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Mårten Lindahl, Jean Delvare, linux-hwmon, kernel
On Wed, Jun 01, 2022 at 09:12:56PM +0200, Guenter Roeck wrote:
> On Mon, May 30, 2022 at 04:34:46PM +0200, Mårten Lindahl wrote:
> > When checking if a regulator supports a voltage range, the regulator
> > needs to have support for listing the range or else -EINVAL will be
> > returned.
> >
> > This support does not exist for the LTC2977 regulator, so this patch
> > adds support for list voltage to the pmbus regulators by adding
> > regulator_list_voltage_linear to the pmbus_regulator_ops. It also
> > defines the voltage resolution for regulators ltc2972/LTC2974/LTC2975/
> > LTC2977/LTC2978/LTC2979/LTC2980/LTM2987 based on that they all have the
> > same stepwise 122.07uV resolution.
> >
> > Since 122.07uV resolution is very small the resolution is set to a 1mV
> > resolution to be easier to handle.
> >
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > ---
> > drivers/hwmon/pmbus/ltc2978.c | 57 +++++++++++++++++++++++++++++---
> > drivers/hwmon/pmbus/pmbus_core.c | 1 +
> > 2 files changed, 54 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> > index 531aa674a928..cfb568c6c155 100644
> > --- a/drivers/hwmon/pmbus/ltc2978.c
> > +++ b/drivers/hwmon/pmbus/ltc2978.c
> > @@ -562,7 +562,37 @@ 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)
>
Hi Guenter!
> #define<space>DEFINE<tab>VALUE, please, and the () around 1000
> is unnecessary.
Ok, will fix.
>
> Also, is the range really correct ? The valid / acceptable
> voltages are in the range detected in pmbus_regulator_set_voltage(),
> based on PMBUS_MFR_VOUT_MIN/PMBUS_VOUT_MARGIN_LOW and
> PMBUS_MFR_VOUT_MAX/PMBUS_VOUT_MARGIN_HIGH, and that will likely differ
> from the fixed number of voltages provided here.
>
> That makes me wonder if it would make more sense to move this
> functionality into the PMBus core code. Any thoughts on that ?
Yes, I did think about that, but to support regulator_count_voltages
the regulator_desc needs to have .n_voltages set and since the
regulator_desc is const I have to set .n_voltages to the full range
of the 16 bit adc resolution. And then I scale it to 1 mV units by the
defines above.
My understanding of the regulator core list_voltage implementation is
that it tests if a requested range fits within this range, using the
min_uV and max_uV limits (which can be specified in DT) to validate it.
Maybe a new pmbus_regulator_list_voltage function could read the voltage
upper and lower limits and then compare the requested range to that one?
But the regulator_desc .n_voltages still needs to be preset to
something. What should it be set to? Should I perhaps set it to the full
ADC resolution (0xFFFF) without scaling it?
Please note, I added the PMBUS_LTC2978_REGULATOR macro with included
voltages and steps only for the chips listed in the commit msg, based on
that they all have the same ADC resolution and ADC unit size (122 uV).
I would prefer to not add voltages and steps to the generic
PMBUS_REGULATOR macro in pmbus.h, as I don't have knowledge about the
other chips.
>
> Thanks,
> Guenter
>
> > +
> > +#define PMBUS_LTC2978_REGULATOR(_name, _id) \
> > + [_id] = { \
> > + .name = (_name # _id), \
> > + .supply_name = "vin", \
> > + .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 +869,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_err(&client->dev, "num_regulators too large!");
>
> Let's make this a dev_warn(); it does not result in an error abort,
> after all.
Ok, will fix.
>
> > + 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_err(&client->dev, "num_regulators too large!");
>
> Same here.
Ok, will fix.
Thanks!
Kind regards
Mårten
>
> > + info->num_regulators =
> > + ARRAY_SIZE(ltc2978_reg_desc_default);
> > + }
> > + break;
> > }
> > #endif
> >
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index f2cf0439da37..7d642b57c8b2 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -2634,6 +2634,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 = regulator_list_voltage_linear,
> > };
> > EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] hwmon: (pmbus/ltc2978) Set voltage resolution
2022-06-02 14:40 ` Marten Lindahl
@ 2022-06-10 8:54 ` Marten Lindahl
0 siblings, 0 replies; 4+ messages in thread
From: Marten Lindahl @ 2022-06-10 8:54 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Mårten Lindahl, Jean Delvare, linux-hwmon, kernel
On Thu, Jun 02, 2022 at 04:40:25PM +0200, Mårten Lindahl wrote:
> On Wed, Jun 01, 2022 at 09:12:56PM +0200, Guenter Roeck wrote:
> > On Mon, May 30, 2022 at 04:34:46PM +0200, Mårten Lindahl wrote:
> > > When checking if a regulator supports a voltage range, the regulator
> > > needs to have support for listing the range or else -EINVAL will be
> > > returned.
> > >
> > > This support does not exist for the LTC2977 regulator, so this patch
> > > adds support for list voltage to the pmbus regulators by adding
> > > regulator_list_voltage_linear to the pmbus_regulator_ops. It also
> > > defines the voltage resolution for regulators ltc2972/LTC2974/LTC2975/
> > > LTC2977/LTC2978/LTC2979/LTC2980/LTM2987 based on that they all have the
> > > same stepwise 122.07uV resolution.
> > >
> > > Since 122.07uV resolution is very small the resolution is set to a 1mV
> > > resolution to be easier to handle.
> > >
> > > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > > ---
> > > drivers/hwmon/pmbus/ltc2978.c | 57 +++++++++++++++++++++++++++++---
> > > drivers/hwmon/pmbus/pmbus_core.c | 1 +
> > > 2 files changed, 54 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> > > index 531aa674a928..cfb568c6c155 100644
> > > --- a/drivers/hwmon/pmbus/ltc2978.c
> > > +++ b/drivers/hwmon/pmbus/ltc2978.c
> > > @@ -562,7 +562,37 @@ 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)
> >
>
> Hi Guenter!
>
> > #define<space>DEFINE<tab>VALUE, please, and the () around 1000
> > is unnecessary.
>
> Ok, will fix.
>
> >
> > Also, is the range really correct ? The valid / acceptable
> > voltages are in the range detected in pmbus_regulator_set_voltage(),
> > based on PMBUS_MFR_VOUT_MIN/PMBUS_VOUT_MARGIN_LOW and
> > PMBUS_MFR_VOUT_MAX/PMBUS_VOUT_MARGIN_HIGH, and that will likely differ
> > from the fixed number of voltages provided here.
> >
> > That makes me wonder if it would make more sense to move this
> > functionality into the PMBus core code. Any thoughts on that ?
>
> Yes, I did think about that, but to support regulator_count_voltages
> the regulator_desc needs to have .n_voltages set and since the
> regulator_desc is const I have to set .n_voltages to the full range
> of the 16 bit adc resolution. And then I scale it to 1 mV units by the
> defines above.
>
> My understanding of the regulator core list_voltage implementation is
> that it tests if a requested range fits within this range, using the
> min_uV and max_uV limits (which can be specified in DT) to validate it.
>
> Maybe a new pmbus_regulator_list_voltage function could read the voltage
> upper and lower limits and then compare the requested range to that one?
>
Hi Guenter!
I implemented a new pmbus_regulator_list_voltage function that compares
a requested voltage to the lower/upper limits, and it seems to work fine.
There is a performance problem with reading the boundaries
(lots of smbus transmissions) for each call of a list_voltage iteration
from the regulator core, so I made a patch to cache the boundaries after
the first read.
I will send two patches shortly.
Kind regards
Mårten
> But the regulator_desc .n_voltages still needs to be preset to
> something. What should it be set to? Should I perhaps set it to the full
> ADC resolution (0xFFFF) without scaling it?
Answer to myself: I think it is correct to set .n_voltages to the full
range of the adc resolution, and then scale it.
>
> Please note, I added the PMBUS_LTC2978_REGULATOR macro with included
> voltages and steps only for the chips listed in the commit msg, based on
> that they all have the same ADC resolution and ADC unit size (122 uV).
> I would prefer to not add voltages and steps to the generic
> PMBUS_REGULATOR macro in pmbus.h, as I don't have knowledge about the
> other chips.
>
> >
> > Thanks,
> > Guenter
> >
> > > +
> > > +#define PMBUS_LTC2978_REGULATOR(_name, _id) \
> > > + [_id] = { \
> > > + .name = (_name # _id), \
> > > + .supply_name = "vin", \
> > > + .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 +869,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_err(&client->dev, "num_regulators too large!");
> >
> > Let's make this a dev_warn(); it does not result in an error abort,
> > after all.
>
> Ok, will fix.
>
> >
> > > + 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_err(&client->dev, "num_regulators too large!");
> >
> > Same here.
>
> Ok, will fix.
>
> Thanks!
>
> Kind regards
> Mårten
>
> >
> > > + info->num_regulators =
> > > + ARRAY_SIZE(ltc2978_reg_desc_default);
> > > + }
> > > + break;
> > > }
> > > #endif
> > >
> > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > > index f2cf0439da37..7d642b57c8b2 100644
> > > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > > @@ -2634,6 +2634,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 = regulator_list_voltage_linear,
> > > };
> > > EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
> > >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-06-10 8:55 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-30 14:34 [PATCH v1] hwmon: (pmbus/ltc2978) Set voltage resolution Mårten Lindahl
2022-06-01 19:12 ` Guenter Roeck
2022-06-02 14:40 ` Marten Lindahl
2022-06-10 8:54 ` Marten Lindahl
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.