[PATCH v2 0/3] hwmon: (pmbus/ltc2978) Set voltage resolution

[PATCH v2 0/3] hwmon: (pmbus/ltc2978) Set voltage resolution

[Mårten Lindahl]

Hi!

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 change
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, and scales the resolution to a 1mV
resolution which is easier to handle.

These patches have been tested on an ARTPEC-8 developer board with a group
of LTC2977 power regulators.

Kind regards
Mårten Lindahl

Mårten Lindahl (3):
  hwmon: (pmbus) Introduce and use cached vout margins
  hwmon: (pmbus) Add list_voltage to pmbus ops
  hwmon: (pmbus/ltc2978) Set voltage resolution

 drivers/hwmon/pmbus/ltc2978.c    |  56 +++++++++++++++--
 drivers/hwmon/pmbus/pmbus_core.c | 104 ++++++++++++++++++++++++-------
 2 files changed, 134 insertions(+), 26 deletions(-)

-- 
2.30.2

[PATCH v2 1/3] hwmon: (pmbus) Introduce and use cached vout margins

[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

[PATCH v2 2/3] hwmon: (pmbus) Add list_voltage to pmbus ops

[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

[PATCH v2 3/3] hwmon: (pmbus/ltc2978) Set voltage resolution

[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

Re: [PATCH v2 2/3] hwmon: (pmbus) Add list_voltage to pmbus ops

[Guenter Roeck]

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);
>   

Re: [PATCH v2 3/3] hwmon: (pmbus/ltc2978) Set voltage resolution

[Guenter Roeck]

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
>   

Re: [PATCH v2 2/3] hwmon: (pmbus) Add list_voltage to pmbus ops

[Marten Lindahl]

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);
> >   
> 

Re: [PATCH v2 3/3] hwmon: (pmbus/ltc2978) Set voltage resolution

[Marten Lindahl]

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
> >   
>