[PATCH v4 0/4] hwmon: (pmbus/ltc2978) Add regulator ops

[PATCH v4 0/4] hwmon: (pmbus/ltc2978) Add regulator ops

[Mårten Lindahl]

Hi!

The LTC2978 driver supports a wide range of power regulator chips, but it
has limited functionality for using it in a dynamic regulator framework.
Since standard functions for setting and getting voltage are missing as
pmbus core operations this patchset adds it.

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

Kind regards
Mårten Lindahl

Changes in v2:
 - Add pmbus core _pmbus_write_byte_data to check for driver specific callback
 - Change pmbus_update_byte_data to use _pmbus_read/write_byte_data
 - Change pmbus_regulator_is_enabled to use _pmbus_read_byte_data
 - Export pmbus core functions enable/disable/is_enabled

Changes in v3:
 - Split patch "hwmon: (pmbus/ltc2978) Use driver specific ops if they exist"
   into two patches: (1) pmbus core, (2) ltc2978.
 - Move ltc2978_regulator_get/set_voltage functions to pmbus core.

Changes in v4:
 - Split (and rename) patch "hwmon: (pmbus) Use driver specific ops if they exist"
   into two patches where the first handle _pmbus_write_byte_data, and the
   second handle _pmbus_read_byte_data.
 - Use voltage conversion functions in pmbus_regulator_get/set_voltage.

Mårten Lindahl (4):
  hwmon: (pmbus) Introduce and use write_byte_data callback
  hwmon: (pmbus) Use _pmbus_read_byte_data with callback
  hwmon: (pmbus/ltc2978) Add chip specific write_byte_data
  hwmon: (pmbus) Add get_voltage/set_voltage ops

 drivers/hwmon/pmbus/ltc2978.c    |  12 +++
 drivers/hwmon/pmbus/pmbus.h      |   2 +
 drivers/hwmon/pmbus/pmbus_core.c | 133 +++++++++++++++++++++++++------
 3 files changed, 121 insertions(+), 26 deletions(-)

-- 
2.30.2

[PATCH v4 1/4] hwmon: (pmbus) Introduce and use write_byte_data callback

[Mårten Lindahl]

Some of the pmbus core functions uses pmbus_write_byte_data, which does
not support driver callbacks for chip specific write operations. This
could potentially influence some specific regulator chips that for
example need a time delay before each data access.

Lets add support for driver callback with _pmbus_write_byte_data.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/hwmon/pmbus/pmbus.h      |  2 ++
 drivers/hwmon/pmbus/pmbus_core.c | 24 +++++++++++++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index e74b6ef070f3..c031a9700ace 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -438,6 +438,8 @@ struct pmbus_driver_info {
 	int (*read_byte_data)(struct i2c_client *client, int page, int reg);
 	int (*read_word_data)(struct i2c_client *client, int page, int phase,
 			      int reg);
+	int (*write_byte_data)(struct i2c_client *client, int page, int reg,
+			      u8 byte);
 	int (*write_word_data)(struct i2c_client *client, int page, int reg,
 			       u16 word);
 	int (*write_byte)(struct i2c_client *client, int page, u8 value);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index b2618b1d529e..98da2db3f831 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -276,6 +276,24 @@ static int _pmbus_write_word_data(struct i2c_client *client, int page, int reg,
 	return pmbus_write_word_data(client, page, reg, word);
 }
 
+/*
+ * _pmbus_write_byte_data() is similar to pmbus_write_byte_data(), but checks if
+ * a device specific mapping function exists and calls it if necessary.
+ */
+static int _pmbus_write_byte_data(struct i2c_client *client, int page, int reg, u8 value)
+{
+	struct pmbus_data *data = i2c_get_clientdata(client);
+	const struct pmbus_driver_info *info = data->info;
+	int status;
+
+	if (info->write_byte_data) {
+		status = info->write_byte_data(client, page, reg, value);
+		if (status != -ENODATA)
+			return status;
+	}
+	return pmbus_write_byte_data(client, page, reg, value);
+}
+
 int pmbus_update_fan(struct i2c_client *client, int page, int id,
 		     u8 config, u8 mask, u16 command)
 {
@@ -290,7 +308,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
 
 	to = (from & ~mask) | (config & mask);
 	if (to != from) {
-		rv = pmbus_write_byte_data(client, page,
+		rv = _pmbus_write_byte_data(client, page,
 					   pmbus_fan_config_registers[id], to);
 		if (rv < 0)
 			return rv;
@@ -397,7 +415,7 @@ int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
 	tmp = (rv & ~mask) | (value & mask);
 
 	if (tmp != rv)
-		rv = pmbus_write_byte_data(client, page, reg, tmp);
+		rv = _pmbus_write_byte_data(client, page, reg, tmp);
 
 	return rv;
 }
@@ -912,7 +930,7 @@ static int pmbus_get_boolean(struct i2c_client *client, struct pmbus_boolean *b,
 
 	regval = status & mask;
 	if (regval) {
-		ret = pmbus_write_byte_data(client, page, reg, regval);
+		ret = _pmbus_write_byte_data(client, page, reg, regval);
 		if (ret)
 			goto unlock;
 	}
-- 
2.30.2

[PATCH v4 2/4] hwmon: (pmbus) Use _pmbus_read_byte_data with callback

[Mårten Lindahl]

Some of the pmbus core functions uses pmbus_read_byte_data, which does
not support driver callbacks for chip specific write operations. This
could potentially influence some specific regulator chips that for
example need a time delay before each data access.

Lets use _pmbus_read_byte_data with callback check.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 46 ++++++++++++++++----------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 98da2db3f831..bd143ca0c320 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -294,6 +294,24 @@ static int _pmbus_write_byte_data(struct i2c_client *client, int page, int reg,
 	return pmbus_write_byte_data(client, page, reg, value);
 }
 
+/*
+ * _pmbus_read_byte_data() is similar to pmbus_read_byte_data(), but checks if
+ * a device specific mapping function exists and calls it if necessary.
+ */
+static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)
+{
+	struct pmbus_data *data = i2c_get_clientdata(client);
+	const struct pmbus_driver_info *info = data->info;
+	int status;
+
+	if (info->read_byte_data) {
+		status = info->read_byte_data(client, page, reg);
+		if (status != -ENODATA)
+			return status;
+	}
+	return pmbus_read_byte_data(client, page, reg);
+}
+
 int pmbus_update_fan(struct i2c_client *client, int page, int id,
 		     u8 config, u8 mask, u16 command)
 {
@@ -301,7 +319,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
 	int rv;
 	u8 to;
 
-	from = pmbus_read_byte_data(client, page,
+	from = _pmbus_read_byte_data(client, page,
 				    pmbus_fan_config_registers[id]);
 	if (from < 0)
 		return from;
@@ -408,7 +426,7 @@ int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
 	unsigned int tmp;
 	int rv;
 
-	rv = pmbus_read_byte_data(client, page, reg);
+	rv = _pmbus_read_byte_data(client, page, reg);
 	if (rv < 0)
 		return rv;
 
@@ -421,24 +439,6 @@ int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
 }
 EXPORT_SYMBOL_NS_GPL(pmbus_update_byte_data, PMBUS);
 
-/*
- * _pmbus_read_byte_data() is similar to pmbus_read_byte_data(), but checks if
- * a device specific mapping function exists and calls it if necessary.
- */
-static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)
-{
-	struct pmbus_data *data = i2c_get_clientdata(client);
-	const struct pmbus_driver_info *info = data->info;
-	int status;
-
-	if (info->read_byte_data) {
-		status = info->read_byte_data(client, page, reg);
-		if (status != -ENODATA)
-			return status;
-	}
-	return pmbus_read_byte_data(client, page, reg);
-}
-
 static struct pmbus_sensor *pmbus_find_sensor(struct pmbus_data *data, int page,
 					      int reg)
 {
@@ -473,7 +473,7 @@ static int pmbus_get_fan_rate(struct i2c_client *client, int page, int id,
 		return s->data;
 	}
 
-	config = pmbus_read_byte_data(client, page,
+	config = _pmbus_read_byte_data(client, page,
 				      pmbus_fan_config_registers[id]);
 	if (config < 0)
 		return config;
@@ -2414,7 +2414,7 @@ static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
 	int ret;
 
 	mutex_lock(&data->update_lock);
-	ret = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
+	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
 	mutex_unlock(&data->update_lock);
 
 	if (ret < 0)
@@ -2513,7 +2513,7 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
 		if (!(func & cat->func))
 			continue;
 
-		status = pmbus_read_byte_data(client, page, cat->reg);
+		status = _pmbus_read_byte_data(client, page, cat->reg);
 		if (status < 0) {
 			mutex_unlock(&data->update_lock);
 			return status;
-- 
2.30.2

[PATCH v4 3/4] hwmon: (pmbus/ltc2978) Add chip specific write_byte_data

[Mårten Lindahl]

Several of the manuals for devices supported by this driver describes
the need for a minimum wait time before the chip is ready to receive
next command.

This wait time is already implemented in the driver as a ltc_wait_ready
function with a driver defined wait time of 100 ms, and is considered
for specific devices before reading/writing data on the pmbus.

Since this driver uses the default pmbus_regulator_ops for the enable/
disable/is_enabled functions we should add a driver specific callback
for write_byte_data to prevent bypassing the wait time recommendations
for the following devices: ltc3880/ltc3882/ltc3883/ltc3884/ltc3886/
ltc3887/ltc3889/ltm4664/ltm4675/ltm4676/ltm4677/ltm4678/ltm4680/ltm4686/
ltm4700/ltc7880.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/hwmon/pmbus/ltc2978.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index 0127273883f0..531aa674a928 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -196,6 +196,17 @@ static int ltc_read_byte_data(struct i2c_client *client, int page, int reg)
 	return pmbus_read_byte_data(client, page, reg);
 }
 
+static int ltc_write_byte_data(struct i2c_client *client, int page, int reg, u8 value)
+{
+	int ret;
+
+	ret = ltc_wait_ready(client);
+	if (ret < 0)
+		return ret;
+
+	return pmbus_write_byte_data(client, page, reg, value);
+}
+
 static int ltc_write_byte(struct i2c_client *client, int page, u8 byte)
 {
 	int ret;
@@ -681,6 +692,7 @@ static int ltc2978_probe(struct i2c_client *client)
 	info = &data->info;
 	info->write_word_data = ltc2978_write_word_data;
 	info->write_byte = ltc_write_byte;
+	info->write_byte_data = ltc_write_byte_data;
 	info->read_word_data = ltc_read_word_data;
 	info->read_byte_data = ltc_read_byte_data;
 
-- 
2.30.2

[PATCH v4 4/4] hwmon: (pmbus) Add get_voltage/set_voltage ops

[Mårten Lindahl]

The pmbus core does not have operations for getting or setting voltage.
Add functions get/set voltage for the dynamic regulator framework.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 63 ++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index bd143ca0c320..fe7dbb496e3b 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -1531,6 +1531,11 @@ static const struct pmbus_sensor_attr voltage_attributes[] = {
 		.gbit = PB_STATUS_VOUT_OV,
 		.limit = vout_limit_attrs,
 		.nlimit = ARRAY_SIZE(vout_limit_attrs),
+	}, {
+		.reg = PMBUS_VOUT_COMMAND,
+		.class = PSC_VOLTAGE_OUT,
+		.paged = true,
+		.func = PMBUS_HAVE_VOUT,
 	}
 };
 
@@ -2563,11 +2568,69 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
 	return 0;
 }
 
+static int pmbus_regulator_get_voltage(struct regulator_dev *rdev)
+{
+	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 *sensor;
+	u8 page = rdev_get_id(rdev);
+	int ret;
+
+	sensor = pmbus_find_sensor(data, page, PMBUS_READ_VOUT);
+	if (IS_ERR(sensor))
+		return -ENODATA;
+
+	mutex_lock(&data->update_lock);
+	pmbus_update_sensor_data(client, sensor);
+	if (sensor->data < 0)
+		ret = sensor->data;
+	else
+		ret = (int)pmbus_reg2data(data, sensor) * 1000; /* unit is uV */
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uV,
+					 int max_uV, 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 *sensor;
+	u8 page = rdev_get_id(rdev);
+	s64 tmp = DIV_ROUND_CLOSEST_ULL(min_uV, 1000); /* convert to mV */
+	u16 val;
+	int ret;
+	*selector = 0;
+
+	sensor = pmbus_find_sensor(data, page, PMBUS_VOUT_COMMAND);
+	if (IS_ERR(sensor))
+		return -ENODATA;
+
+	ret = _pmbus_read_word_data(client, page, 0xff, PMBUS_VOUT_MARGIN_LOW);
+	if (ret < 0)
+		return ret;
+
+	val = pmbus_data2reg(data, sensor, tmp);
+
+	/* Do not fall shorter than low margin */
+	if (ret > val)
+		val = ret;
+
+	ret = _pmbus_write_word_data(client, page, PMBUS_VOUT_COMMAND, val);
+
+	return ret;
+}
+
 const struct regulator_ops pmbus_regulator_ops = {
 	.enable = pmbus_regulator_enable,
 	.disable = pmbus_regulator_disable,
 	.is_enabled = pmbus_regulator_is_enabled,
 	.get_error_flags = pmbus_regulator_get_error_flags,
+	.get_voltage = pmbus_regulator_get_voltage,
+	.set_voltage = pmbus_regulator_set_voltage,
 };
 EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
 
-- 
2.30.2

Re: [PATCH v4 4/4] hwmon: (pmbus) Add get_voltage/set_voltage ops

[Guenter Roeck]

On 4/28/22 07:40, Mårten Lindahl wrote:
> The pmbus core does not have operations for getting or setting voltage.
> Add functions get/set voltage for the dynamic regulator framework.
> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 63 ++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index bd143ca0c320..fe7dbb496e3b 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -1531,6 +1531,11 @@ static const struct pmbus_sensor_attr voltage_attributes[] = {
>   		.gbit = PB_STATUS_VOUT_OV,
>   		.limit = vout_limit_attrs,
>   		.nlimit = ARRAY_SIZE(vout_limit_attrs),
> +	}, {
> +		.reg = PMBUS_VOUT_COMMAND,
> +		.class = PSC_VOLTAGE_OUT,
> +		.paged = true,
> +		.func = PMBUS_HAVE_VOUT,
>   	}

Ok, you lost me here. This adds an inX_input attribute. Why ? This is completely
unrelated to the intended scope of this patch. It also doesn't report a measured
voltage, but a configuration value. If anything, it would have to be a separate
patch, and you'd have to argue hard why it makes sense to report it as measured
voltage.

>   };
>   
> @@ -2563,11 +2568,69 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
>   	return 0;
>   }
>   
> +static int pmbus_regulator_get_voltage(struct regulator_dev *rdev)
> +{
> +	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 *sensor;
> +	u8 page = rdev_get_id(rdev);
> +	int ret;
> +
> +	sensor = pmbus_find_sensor(data, page, PMBUS_READ_VOUT);
> +	if (IS_ERR(sensor))
> +		return -ENODATA;
> +
> +	mutex_lock(&data->update_lock);
> +	pmbus_update_sensor_data(client, sensor);
> +	if (sensor->data < 0)
> +		ret = sensor->data;
> +	else
> +		ret = (int)pmbus_reg2data(data, sensor) * 1000; /* unit is uV */
> +	mutex_unlock(&data->update_lock);
> +

Same question. Why ?

> +	return ret;
> +}
> +
> +static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uV,
> +					 int max_uV, 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 *sensor;
> +	u8 page = rdev_get_id(rdev);
> +	s64 tmp = DIV_ROUND_CLOSEST_ULL(min_uV, 1000); /* convert to mV */
> +	u16 val;
> +	int ret;
> +	*selector = 0;
> +
> +	sensor = pmbus_find_sensor(data, page, PMBUS_VOUT_COMMAND);
> +	if (IS_ERR(sensor))
> +		return -ENODATA;
> +
> +	ret = _pmbus_read_word_data(client, page, 0xff, PMBUS_VOUT_MARGIN_LOW);
> +	if (ret < 0)
> +		return ret;
> +
That actually makes me wonder: What about VOUT_MARGIN_HIGH ?
Also, there are optional MFR_VOUT_MIN and MFR_VOUT_MAX registers.
Would it possibly make sense to determine the valid range once
during probe and then compare against it ?

Thanks,
Guenter

> +	val = pmbus_data2reg(data, sensor, tmp);
> +
> +	/* Do not fall shorter than low margin */
> +	if (ret > val)
> +		val = ret;
> +
> +	ret = _pmbus_write_word_data(client, page, PMBUS_VOUT_COMMAND, val);
> +
> +	return ret;
> +}
> +
>   const struct regulator_ops pmbus_regulator_ops = {
>   	.enable = pmbus_regulator_enable,
>   	.disable = pmbus_regulator_disable,
>   	.is_enabled = pmbus_regulator_is_enabled,
>   	.get_error_flags = pmbus_regulator_get_error_flags,
> +	.get_voltage = pmbus_regulator_get_voltage,
> +	.set_voltage = pmbus_regulator_set_voltage,
>   };
>   EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
>   

Re: [PATCH v4 4/4] hwmon: (pmbus) Add get_voltage/set_voltage ops

[Marten Lindahl]

On Thu, Apr 28, 2022 at 06:49:21PM +0200, Guenter Roeck wrote:
> On 4/28/22 07:40, Mårten Lindahl wrote:
> > The pmbus core does not have operations for getting or setting voltage.
> > Add functions get/set voltage for the dynamic regulator framework.
> > 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > ---
> >   drivers/hwmon/pmbus/pmbus_core.c | 63 ++++++++++++++++++++++++++++++++
> >   1 file changed, 63 insertions(+)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index bd143ca0c320..fe7dbb496e3b 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -1531,6 +1531,11 @@ static const struct pmbus_sensor_attr voltage_attributes[] = {
> >   		.gbit = PB_STATUS_VOUT_OV,
> >   		.limit = vout_limit_attrs,
> >   		.nlimit = ARRAY_SIZE(vout_limit_attrs),
> > +	}, {
> > +		.reg = PMBUS_VOUT_COMMAND,
> > +		.class = PSC_VOLTAGE_OUT,
> > +		.paged = true,
> > +		.func = PMBUS_HAVE_VOUT,
> >   	}
> 
> Ok, you lost me here. This adds an inX_input attribute. Why ? This is completely
> unrelated to the intended scope of this patch. It also doesn't report a measured
> voltage, but a configuration value. If anything, it would have to be a separate
> patch, and you'd have to argue hard why it makes sense to report it as measured
> voltage.

I see. The reason for adding this is as simple as I now understand it is wrong.
Please remember, my first version of the set/get_voltage functions where hardcoded
with L16 input/output. Then in order to use the already existing convertion functions
pmbus_data2reg and pmbus_reg2data I added this only for the need of a sensor object,
as those functions are tailored for a sensor object.

So now I have to ask you for advice. Should I use the existing convertion
functions, or do you suggest new variants of them? If reusing them, I guess I have
two options:
 1: Modify them to take class, page, and data outside of a sensor object as input.
 2: Use them as they are, but create a local 'dummy' sensor object with class, page,
    and data to use when calling the convertion functions.

I hope I made it more clear for you now how I was thinking. There is
absolutely no intention of having sensor inX_input attributes for
reading the setpoint values. This was just an unwanted sideeffect, and I
will glady remove it again.
> 
> >   };
> >   
> > @@ -2563,11 +2568,69 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
> >   	return 0;
> >   }
> >   
> > +static int pmbus_regulator_get_voltage(struct regulator_dev *rdev)
> > +{
> > +	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 *sensor;
> > +	u8 page = rdev_get_id(rdev);
> > +	int ret;
> > +
> > +	sensor = pmbus_find_sensor(data, page, PMBUS_READ_VOUT);
> > +	if (IS_ERR(sensor))
> > +		return -ENODATA;
> > +
> > +	mutex_lock(&data->update_lock);
> > +	pmbus_update_sensor_data(client, sensor);
> > +	if (sensor->data < 0)
> > +		ret = sensor->data;
> > +	else
> > +		ret = (int)pmbus_reg2data(data, sensor) * 1000; /* unit is uV */
> > +	mutex_unlock(&data->update_lock);
> > +
> 
> Same question. Why ?

Same reason as above. Only to get the sensor object for pmbus_reg2data.

> 
> > +	return ret;
> > +}
> > +
> > +static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uV,
> > +					 int max_uV, 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 *sensor;
> > +	u8 page = rdev_get_id(rdev);
> > +	s64 tmp = DIV_ROUND_CLOSEST_ULL(min_uV, 1000); /* convert to mV */
> > +	u16 val;
> > +	int ret;
> > +	*selector = 0;
> > +
> > +	sensor = pmbus_find_sensor(data, page, PMBUS_VOUT_COMMAND);
> > +	if (IS_ERR(sensor))
> > +		return -ENODATA;
> > +
> > +	ret = _pmbus_read_word_data(client, page, 0xff, PMBUS_VOUT_MARGIN_LOW);
> > +	if (ret < 0)
> > +		return ret;
> > +
> That actually makes me wonder: What about VOUT_MARGIN_HIGH ?

Ok, I will add a check for VOUT_MARGIN_HIGH also.

> Also, there are optional MFR_VOUT_MIN and MFR_VOUT_MAX registers.
> Would it possibly make sense to determine the valid range once
> during probe and then compare against it ?

Maybe this could be a good thing, so that we don't need to read both
margins every time. But I guess that would need a new kind of page list
with margins added to the pmbus_driver_info struct?
I would prefer to make that change in a separate patch if it's ok with
you?

Kind regards
Mårten
> 
> Thanks,
> Guenter
> 
> > +	val = pmbus_data2reg(data, sensor, tmp);
> > +
> > +	/* Do not fall shorter than low margin */
> > +	if (ret > val)
> > +		val = ret;
> > +
> > +	ret = _pmbus_write_word_data(client, page, PMBUS_VOUT_COMMAND, val);
> > +
> > +	return ret;
> > +}
> > +
> >   const struct regulator_ops pmbus_regulator_ops = {
> >   	.enable = pmbus_regulator_enable,
> >   	.disable = pmbus_regulator_disable,
> >   	.is_enabled = pmbus_regulator_is_enabled,
> >   	.get_error_flags = pmbus_regulator_get_error_flags,
> > +	.get_voltage = pmbus_regulator_get_voltage,
> > +	.set_voltage = pmbus_regulator_set_voltage,
> >   };
> >   EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
> >   
> 

Re: [PATCH v4 4/4] hwmon: (pmbus) Add get_voltage/set_voltage ops

[Guenter Roeck]

On 4/29/22 02:52, Marten Lindahl wrote:
> On Thu, Apr 28, 2022 at 06:49:21PM +0200, Guenter Roeck wrote:
>> On 4/28/22 07:40, Mårten Lindahl wrote:
>>> The pmbus core does not have operations for getting or setting voltage.
>>> Add functions get/set voltage for the dynamic regulator framework.
>>>
>>> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
>>> ---
>>>    drivers/hwmon/pmbus/pmbus_core.c | 63 ++++++++++++++++++++++++++++++++
>>>    1 file changed, 63 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>>> index bd143ca0c320..fe7dbb496e3b 100644
>>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>>> @@ -1531,6 +1531,11 @@ static const struct pmbus_sensor_attr voltage_attributes[] = {
>>>    		.gbit = PB_STATUS_VOUT_OV,
>>>    		.limit = vout_limit_attrs,
>>>    		.nlimit = ARRAY_SIZE(vout_limit_attrs),
>>> +	}, {
>>> +		.reg = PMBUS_VOUT_COMMAND,
>>> +		.class = PSC_VOLTAGE_OUT,
>>> +		.paged = true,
>>> +		.func = PMBUS_HAVE_VOUT,
>>>    	}
>>
>> Ok, you lost me here. This adds an inX_input attribute. Why ? This is completely
>> unrelated to the intended scope of this patch. It also doesn't report a measured
>> voltage, but a configuration value. If anything, it would have to be a separate
>> patch, and you'd have to argue hard why it makes sense to report it as measured
>> voltage.
> 
> I see. The reason for adding this is as simple as I now understand it is wrong.
> Please remember, my first version of the set/get_voltage functions where hardcoded
> with L16 input/output. Then in order to use the already existing convertion functions
> pmbus_data2reg and pmbus_reg2data I added this only for the need of a sensor object,
> as those functions are tailored for a sensor object.
> 
> So now I have to ask you for advice. Should I use the existing convertion
> functions, or do you suggest new variants of them? If reusing them, I guess I have
> two options:
>   1: Modify them to take class, page, and data outside of a sensor object as input.
>   2: Use them as they are, but create a local 'dummy' sensor object with class, page,
>      and data to use when calling the convertion functions.
> 

I think 2) is the easier and less complex solution for now.

> I hope I made it more clear for you now how I was thinking. There is
> absolutely no intention of having sensor inX_input attributes for
> reading the setpoint values. This was just an unwanted sideeffect, and I
> will glady remove it again.

No problem. Thanks for the explanation.

>>
>>>    };
>>>    
>>> @@ -2563,11 +2568,69 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
>>>    	return 0;
>>>    }
>>>    
>>> +static int pmbus_regulator_get_voltage(struct regulator_dev *rdev)
>>> +{
>>> +	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 *sensor;
>>> +	u8 page = rdev_get_id(rdev);
>>> +	int ret;
>>> +
>>> +	sensor = pmbus_find_sensor(data, page, PMBUS_READ_VOUT);
>>> +	if (IS_ERR(sensor))
>>> +		return -ENODATA;
>>> +
>>> +	mutex_lock(&data->update_lock);
>>> +	pmbus_update_sensor_data(client, sensor);
>>> +	if (sensor->data < 0)
>>> +		ret = sensor->data;
>>> +	else
>>> +		ret = (int)pmbus_reg2data(data, sensor) * 1000; /* unit is uV */
>>> +	mutex_unlock(&data->update_lock);
>>> +
>>
>> Same question. Why ?
> 
> Same reason as above. Only to get the sensor object for pmbus_reg2data.
> 
>>
>>> +	return ret;
>>> +}
>>> +
>>> +static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uV,
>>> +					 int max_uV, 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 *sensor;
>>> +	u8 page = rdev_get_id(rdev);
>>> +	s64 tmp = DIV_ROUND_CLOSEST_ULL(min_uV, 1000); /* convert to mV */
>>> +	u16 val;
>>> +	int ret;
>>> +	*selector = 0;
>>> +
>>> +	sensor = pmbus_find_sensor(data, page, PMBUS_VOUT_COMMAND);
>>> +	if (IS_ERR(sensor))
>>> +		return -ENODATA;
>>> +
>>> +	ret = _pmbus_read_word_data(client, page, 0xff, PMBUS_VOUT_MARGIN_LOW);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>> That actually makes me wonder: What about VOUT_MARGIN_HIGH ?
> 
> Ok, I will add a check for VOUT_MARGIN_HIGH also.
> 
>> Also, there are optional MFR_VOUT_MIN and MFR_VOUT_MAX registers.
>> Would it possibly make sense to determine the valid range once
>> during probe and then compare against it ?
> 
> Maybe this could be a good thing, so that we don't need to read both
> margins every time. But I guess that would need a new kind of page list
> with margins added to the pmbus_driver_info struct?
> I would prefer to make that change in a separate patch if it's ok with
> you?
> 

I think you need to check for four values right now:

- Try to read MFR_VOUT_MIN. If that does not work, read VOUT_MARGIN_LOW.
- Same for high values.

Ultimately, yes, I think we should add a list of limits. I think it
would make more sense though to add the limits to a new regulator
specific data structure. Maybe we should create a separate data structure
for regulators. Right now we pass struct pmbus_data. Maybe we need
struct pmbus_regulator_data which would contain a pointer to
struct pmbus_data as well as additional information needed for
regulators.

Thanks,
Guenter

Re: [PATCH v4 1/4] hwmon: (pmbus) Introduce and use write_byte_data callback

[Guenter Roeck]

On Thu, Apr 28, 2022 at 04:40:36PM +0200, Mårten Lindahl wrote:
> Some of the pmbus core functions uses pmbus_write_byte_data, which does
> not support driver callbacks for chip specific write operations. This
> could potentially influence some specific regulator chips that for
> example need a time delay before each data access.
> 
> Lets add support for driver callback with _pmbus_write_byte_data.
> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/pmbus.h      |  2 ++
>  drivers/hwmon/pmbus/pmbus_core.c | 24 +++++++++++++++++++++---
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index e74b6ef070f3..c031a9700ace 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -438,6 +438,8 @@ struct pmbus_driver_info {
>  	int (*read_byte_data)(struct i2c_client *client, int page, int reg);
>  	int (*read_word_data)(struct i2c_client *client, int page, int phase,
>  			      int reg);
> +	int (*write_byte_data)(struct i2c_client *client, int page, int reg,
> +			      u8 byte);
>  	int (*write_word_data)(struct i2c_client *client, int page, int reg,
>  			       u16 word);
>  	int (*write_byte)(struct i2c_client *client, int page, u8 value);
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index b2618b1d529e..98da2db3f831 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -276,6 +276,24 @@ static int _pmbus_write_word_data(struct i2c_client *client, int page, int reg,
>  	return pmbus_write_word_data(client, page, reg, word);
>  }
>  
> +/*
> + * _pmbus_write_byte_data() is similar to pmbus_write_byte_data(), but checks if
> + * a device specific mapping function exists and calls it if necessary.
> + */
> +static int _pmbus_write_byte_data(struct i2c_client *client, int page, int reg, u8 value)
> +{
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +	const struct pmbus_driver_info *info = data->info;
> +	int status;
> +
> +	if (info->write_byte_data) {
> +		status = info->write_byte_data(client, page, reg, value);
> +		if (status != -ENODATA)
> +			return status;
> +	}
> +	return pmbus_write_byte_data(client, page, reg, value);
> +}
> +
>  int pmbus_update_fan(struct i2c_client *client, int page, int id,
>  		     u8 config, u8 mask, u16 command)
>  {
> @@ -290,7 +308,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
>  
>  	to = (from & ~mask) | (config & mask);
>  	if (to != from) {
> -		rv = pmbus_write_byte_data(client, page,
> +		rv = _pmbus_write_byte_data(client, page,
>  					   pmbus_fan_config_registers[id], to);
>  		if (rv < 0)
>  			return rv;
> @@ -397,7 +415,7 @@ int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
>  	tmp = (rv & ~mask) | (value & mask);
>  
>  	if (tmp != rv)
> -		rv = pmbus_write_byte_data(client, page, reg, tmp);
> +		rv = _pmbus_write_byte_data(client, page, reg, tmp);
>  
>  	return rv;
>  }
> @@ -912,7 +930,7 @@ static int pmbus_get_boolean(struct i2c_client *client, struct pmbus_boolean *b,
>  
>  	regval = status & mask;
>  	if (regval) {
> -		ret = pmbus_write_byte_data(client, page, reg, regval);
> +		ret = _pmbus_write_byte_data(client, page, reg, regval);
>  		if (ret)
>  			goto unlock;
>  	}

Re: [PATCH v4 2/4] hwmon: (pmbus) Use _pmbus_read_byte_data with callback

[Guenter Roeck]

On Thu, Apr 28, 2022 at 04:40:37PM +0200, Mårten Lindahl wrote:
> Some of the pmbus core functions uses pmbus_read_byte_data, which does
> not support driver callbacks for chip specific write operations. This
> could potentially influence some specific regulator chips that for
> example need a time delay before each data access.
> 
> Lets use _pmbus_read_byte_data with callback check.
> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 46 ++++++++++++++++----------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 98da2db3f831..bd143ca0c320 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -294,6 +294,24 @@ static int _pmbus_write_byte_data(struct i2c_client *client, int page, int reg,
>  	return pmbus_write_byte_data(client, page, reg, value);
>  }
>  
> +/*
> + * _pmbus_read_byte_data() is similar to pmbus_read_byte_data(), but checks if
> + * a device specific mapping function exists and calls it if necessary.
> + */
> +static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)
> +{
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +	const struct pmbus_driver_info *info = data->info;
> +	int status;
> +
> +	if (info->read_byte_data) {
> +		status = info->read_byte_data(client, page, reg);
> +		if (status != -ENODATA)
> +			return status;
> +	}
> +	return pmbus_read_byte_data(client, page, reg);
> +}
> +
>  int pmbus_update_fan(struct i2c_client *client, int page, int id,
>  		     u8 config, u8 mask, u16 command)
>  {
> @@ -301,7 +319,7 @@ int pmbus_update_fan(struct i2c_client *client, int page, int id,
>  	int rv;
>  	u8 to;
>  
> -	from = pmbus_read_byte_data(client, page,
> +	from = _pmbus_read_byte_data(client, page,
>  				    pmbus_fan_config_registers[id]);
>  	if (from < 0)
>  		return from;
> @@ -408,7 +426,7 @@ int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
>  	unsigned int tmp;
>  	int rv;
>  
> -	rv = pmbus_read_byte_data(client, page, reg);
> +	rv = _pmbus_read_byte_data(client, page, reg);
>  	if (rv < 0)
>  		return rv;
>  
> @@ -421,24 +439,6 @@ int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
>  }
>  EXPORT_SYMBOL_NS_GPL(pmbus_update_byte_data, PMBUS);
>  
> -/*
> - * _pmbus_read_byte_data() is similar to pmbus_read_byte_data(), but checks if
> - * a device specific mapping function exists and calls it if necessary.
> - */
> -static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)
> -{
> -	struct pmbus_data *data = i2c_get_clientdata(client);
> -	const struct pmbus_driver_info *info = data->info;
> -	int status;
> -
> -	if (info->read_byte_data) {
> -		status = info->read_byte_data(client, page, reg);
> -		if (status != -ENODATA)
> -			return status;
> -	}
> -	return pmbus_read_byte_data(client, page, reg);
> -}
> -
>  static struct pmbus_sensor *pmbus_find_sensor(struct pmbus_data *data, int page,
>  					      int reg)
>  {
> @@ -473,7 +473,7 @@ static int pmbus_get_fan_rate(struct i2c_client *client, int page, int id,
>  		return s->data;
>  	}
>  
> -	config = pmbus_read_byte_data(client, page,
> +	config = _pmbus_read_byte_data(client, page,
>  				      pmbus_fan_config_registers[id]);
>  	if (config < 0)
>  		return config;
> @@ -2414,7 +2414,7 @@ static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
>  	int ret;
>  
>  	mutex_lock(&data->update_lock);
> -	ret = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> +	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
>  	mutex_unlock(&data->update_lock);
>  
>  	if (ret < 0)
> @@ -2513,7 +2513,7 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
>  		if (!(func & cat->func))
>  			continue;
>  
> -		status = pmbus_read_byte_data(client, page, cat->reg);
> +		status = _pmbus_read_byte_data(client, page, cat->reg);
>  		if (status < 0) {
>  			mutex_unlock(&data->update_lock);
>  			return status;

Re: [PATCH v4 3/4] hwmon: (pmbus/ltc2978) Add chip specific write_byte_data

[Guenter Roeck]

On Thu, Apr 28, 2022 at 04:40:38PM +0200, Mårten Lindahl wrote:
> Several of the manuals for devices supported by this driver describes
> the need for a minimum wait time before the chip is ready to receive
> next command.
> 
> This wait time is already implemented in the driver as a ltc_wait_ready
> function with a driver defined wait time of 100 ms, and is considered
> for specific devices before reading/writing data on the pmbus.
> 
> Since this driver uses the default pmbus_regulator_ops for the enable/
> disable/is_enabled functions we should add a driver specific callback
> for write_byte_data to prevent bypassing the wait time recommendations
> for the following devices: ltc3880/ltc3882/ltc3883/ltc3884/ltc3886/
> ltc3887/ltc3889/ltm4664/ltm4675/ltm4676/ltm4677/ltm4678/ltm4680/ltm4686/
> ltm4700/ltc7880.
> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/ltc2978.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> index 0127273883f0..531aa674a928 100644
> --- a/drivers/hwmon/pmbus/ltc2978.c
> +++ b/drivers/hwmon/pmbus/ltc2978.c
> @@ -196,6 +196,17 @@ static int ltc_read_byte_data(struct i2c_client *client, int page, int reg)
>  	return pmbus_read_byte_data(client, page, reg);
>  }
>  
> +static int ltc_write_byte_data(struct i2c_client *client, int page, int reg, u8 value)
> +{
> +	int ret;
> +
> +	ret = ltc_wait_ready(client);
> +	if (ret < 0)
> +		return ret;
> +
> +	return pmbus_write_byte_data(client, page, reg, value);
> +}
> +
>  static int ltc_write_byte(struct i2c_client *client, int page, u8 byte)
>  {
>  	int ret;
> @@ -681,6 +692,7 @@ static int ltc2978_probe(struct i2c_client *client)
>  	info = &data->info;
>  	info->write_word_data = ltc2978_write_word_data;
>  	info->write_byte = ltc_write_byte;
> +	info->write_byte_data = ltc_write_byte_data;
>  	info->read_word_data = ltc_read_word_data;
>  	info->read_byte_data = ltc_read_byte_data;
>  

Re: [PATCH v4 4/4] hwmon: (pmbus) Add get_voltage/set_voltage ops

[Marten Lindahl]

On Fri, Apr 29, 2022 at 07:00:27PM +0200, Guenter Roeck wrote:
> On 4/29/22 02:52, Marten Lindahl wrote:
> > On Thu, Apr 28, 2022 at 06:49:21PM +0200, Guenter Roeck wrote:
> >> On 4/28/22 07:40, Mårten Lindahl wrote:
> >>> The pmbus core does not have operations for getting or setting voltage.
> >>> Add functions get/set voltage for the dynamic regulator framework.
> >>>
> >>> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> >>> ---
> >>>    drivers/hwmon/pmbus/pmbus_core.c | 63 ++++++++++++++++++++++++++++++++
> >>>    1 file changed, 63 insertions(+)
> >>>
> >>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> >>> index bd143ca0c320..fe7dbb496e3b 100644
> >>> --- a/drivers/hwmon/pmbus/pmbus_core.c
> >>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> >>> @@ -1531,6 +1531,11 @@ static const struct pmbus_sensor_attr voltage_attributes[] = {
> >>>    		.gbit = PB_STATUS_VOUT_OV,
> >>>    		.limit = vout_limit_attrs,
> >>>    		.nlimit = ARRAY_SIZE(vout_limit_attrs),
> >>> +	}, {
> >>> +		.reg = PMBUS_VOUT_COMMAND,
> >>> +		.class = PSC_VOLTAGE_OUT,
> >>> +		.paged = true,
> >>> +		.func = PMBUS_HAVE_VOUT,
> >>>    	}
> >>
> >> Ok, you lost me here. This adds an inX_input attribute. Why ? This is completely
> >> unrelated to the intended scope of this patch. It also doesn't report a measured
> >> voltage, but a configuration value. If anything, it would have to be a separate
> >> patch, and you'd have to argue hard why it makes sense to report it as measured
> >> voltage.
> > 
> > I see. The reason for adding this is as simple as I now understand it is wrong.
> > Please remember, my first version of the set/get_voltage functions where hardcoded
> > with L16 input/output. Then in order to use the already existing convertion functions
> > pmbus_data2reg and pmbus_reg2data I added this only for the need of a sensor object,
> > as those functions are tailored for a sensor object.
> > 
> > So now I have to ask you for advice. Should I use the existing convertion
> > functions, or do you suggest new variants of them? If reusing them, I guess I have
> > two options:
> >   1: Modify them to take class, page, and data outside of a sensor object as input.
> >   2: Use them as they are, but create a local 'dummy' sensor object with class, page,
> >      and data to use when calling the convertion functions.
> > 
> 
> I think 2) is the easier and less complex solution for now.

Hi Guenter!
This seems to work fine.

> 
> > I hope I made it more clear for you now how I was thinking. There is
> > absolutely no intention of having sensor inX_input attributes for
> > reading the setpoint values. This was just an unwanted sideeffect, and I
> > will glady remove it again.
> 
> No problem. Thanks for the explanation.
> 
> >>
> >>>    };
> >>>    
> >>> @@ -2563,11 +2568,69 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
> >>>    	return 0;
> >>>    }
> >>>    
> >>> +static int pmbus_regulator_get_voltage(struct regulator_dev *rdev)
> >>> +{
> >>> +	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 *sensor;
> >>> +	u8 page = rdev_get_id(rdev);
> >>> +	int ret;
> >>> +
> >>> +	sensor = pmbus_find_sensor(data, page, PMBUS_READ_VOUT);
> >>> +	if (IS_ERR(sensor))
> >>> +		return -ENODATA;
> >>> +
> >>> +	mutex_lock(&data->update_lock);
> >>> +	pmbus_update_sensor_data(client, sensor);
> >>> +	if (sensor->data < 0)
> >>> +		ret = sensor->data;
> >>> +	else
> >>> +		ret = (int)pmbus_reg2data(data, sensor) * 1000; /* unit is uV */
> >>> +	mutex_unlock(&data->update_lock);
> >>> +
> >>
> >> Same question. Why ?
> > 
> > Same reason as above. Only to get the sensor object for pmbus_reg2data.
> > 
> >>
> >>> +	return ret;
> >>> +}
> >>> +
> >>> +static int pmbus_regulator_set_voltage(struct regulator_dev *rdev, int min_uV,
> >>> +					 int max_uV, 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 *sensor;
> >>> +	u8 page = rdev_get_id(rdev);
> >>> +	s64 tmp = DIV_ROUND_CLOSEST_ULL(min_uV, 1000); /* convert to mV */
> >>> +	u16 val;
> >>> +	int ret;
> >>> +	*selector = 0;
> >>> +
> >>> +	sensor = pmbus_find_sensor(data, page, PMBUS_VOUT_COMMAND);
> >>> +	if (IS_ERR(sensor))
> >>> +		return -ENODATA;
> >>> +
> >>> +	ret = _pmbus_read_word_data(client, page, 0xff, PMBUS_VOUT_MARGIN_LOW);
> >>> +	if (ret < 0)
> >>> +		return ret;
> >>> +
> >> That actually makes me wonder: What about VOUT_MARGIN_HIGH ?
> > 
> > Ok, I will add a check for VOUT_MARGIN_HIGH also.
> > 
> >> Also, there are optional MFR_VOUT_MIN and MFR_VOUT_MAX registers.
> >> Would it possibly make sense to determine the valid range once
> >> during probe and then compare against it ?
> > 
> > Maybe this could be a good thing, so that we don't need to read both
> > margins every time. But I guess that would need a new kind of page list
> > with margins added to the pmbus_driver_info struct?
> > I would prefer to make that change in a separate patch if it's ok with
> > you?
> > 
> 
> I think you need to check for four values right now:
> 
> - Try to read MFR_VOUT_MIN. If that does not work, read VOUT_MARGIN_LOW.
> - Same for high values.

Ok, I think I first need to check the MFR_VOUT_MIN/MFR_VOUT_MAX
registers to make sure they are supported. On LTC2977 they are not
supported, so I need to write PMBUS_CLEAR_FAULTS after trying to access
them.

I'll send a new patch for this.

Kind regards
Mårten

> 
> Ultimately, yes, I think we should add a list of limits. I think it
> would make more sense though to add the limits to a new regulator
> specific data structure. Maybe we should create a separate data structure
> for regulators. Right now we pass struct pmbus_data. Maybe we need
> struct pmbus_regulator_data which would contain a pointer to
> struct pmbus_data as well as additional information needed for
> regulators.
> 
> Thanks,
> Guenter