[PATCH v3 0/3] hwmon: (pmbus/ltc2978) Add regulator ops

[PATCH v3 0/3] 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.

Mårten Lindahl (3):
  hwmon: (pmbus) Use driver specific ops if they exist
  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 | 101 +++++++++++++++++++++++++------
 3 files changed, 95 insertions(+), 20 deletions(-)

-- 
2.30.2

[PATCH v3 1/3] hwmon: (pmbus) Use driver specific ops if they exist

[Mårten Lindahl]

Pmbus drivers using the default pmbus_regulator_ops for the enable/
disable/is_enabled functions will use the standard pmbus core functions
pmbus_read/write_byte_data. This could potentially influence some
specific regulator chips that for example need a time delay before each
data access.

Lets add support for drivers to use chip specific read/write operations
when using the standard pmbus_regulator_ops.

Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
---
 drivers/hwmon/pmbus/pmbus.h      |  2 ++
 drivers/hwmon/pmbus/pmbus_core.c | 58 +++++++++++++++++++++-----------
 2 files changed, 40 insertions(+), 20 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..1b0728c3c7d8 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -384,25 +384,6 @@ int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value)
 }
 EXPORT_SYMBOL_NS_GPL(pmbus_write_byte_data, PMBUS);
 
-int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
-			   u8 mask, u8 value)
-{
-	unsigned int tmp;
-	int rv;
-
-	rv = pmbus_read_byte_data(client, page, reg);
-	if (rv < 0)
-		return rv;
-
-	tmp = (rv & ~mask) | (value & mask);
-
-	if (tmp != rv)
-		rv = pmbus_write_byte_data(client, page, reg, tmp);
-
-	return rv;
-}
-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.
@@ -421,6 +402,43 @@ static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)
 	return pmbus_read_byte_data(client, page, reg);
 }
 
+/*
+ * _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_byte_data(struct i2c_client *client, int page, u8 reg,
+			   u8 mask, u8 value)
+{
+	unsigned int tmp;
+	int rv;
+
+	rv = _pmbus_read_byte_data(client, page, reg);
+	if (rv < 0)
+		return rv;
+
+	tmp = (rv & ~mask) | (value & mask);
+
+	if (tmp != rv)
+		rv = _pmbus_write_byte_data(client, page, reg, tmp);
+
+	return rv;
+}
+EXPORT_SYMBOL_NS_GPL(pmbus_update_byte_data, PMBUS);
+
 static struct pmbus_sensor *pmbus_find_sensor(struct pmbus_data *data, int page,
 					      int reg)
 {
@@ -2396,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)
-- 
2.30.2

Re: [PATCH v3 1/3] hwmon: (pmbus) Use driver specific ops if they exist

[Guenter Roeck]

On Wed, Apr 27, 2022 at 03:02:11PM +0200, Mårten Lindahl wrote:
> Pmbus drivers using the default pmbus_regulator_ops for the enable/
> disable/is_enabled functions will use the standard pmbus core functions
> pmbus_read/write_byte_data. This could potentially influence some
> specific regulator chips that for example need a time delay before each
> data access.
> 
> Lets add support for drivers to use chip specific read/write operations
> when using the standard pmbus_regulator_ops.

The subject is misleading. It should be something like "introduce and
use write_byte_data callback". Also, existing code calling
pmbus_write_byte_data() should call _pmbus_write_byte_data() instead.
This applies to pmbus_update_fan() and pmbus_get_boolean().

Thanks,
Guenter

> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> ---
>  drivers/hwmon/pmbus/pmbus.h      |  2 ++
>  drivers/hwmon/pmbus/pmbus_core.c | 58 +++++++++++++++++++++-----------
>  2 files changed, 40 insertions(+), 20 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..1b0728c3c7d8 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -384,25 +384,6 @@ int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value)
>  }
>  EXPORT_SYMBOL_NS_GPL(pmbus_write_byte_data, PMBUS);
>  
> -int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
> -			   u8 mask, u8 value)
> -{
> -	unsigned int tmp;
> -	int rv;
> -
> -	rv = pmbus_read_byte_data(client, page, reg);
> -	if (rv < 0)
> -		return rv;
> -
> -	tmp = (rv & ~mask) | (value & mask);
> -
> -	if (tmp != rv)
> -		rv = pmbus_write_byte_data(client, page, reg, tmp);
> -
> -	return rv;
> -}
> -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.
> @@ -421,6 +402,43 @@ static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)
>  	return pmbus_read_byte_data(client, page, reg);
>  }
>  
> +/*
> + * _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_byte_data(struct i2c_client *client, int page, u8 reg,
> +			   u8 mask, u8 value)
> +{
> +	unsigned int tmp;
> +	int rv;
> +
> +	rv = _pmbus_read_byte_data(client, page, reg);
> +	if (rv < 0)
> +		return rv;
> +
> +	tmp = (rv & ~mask) | (value & mask);
> +
> +	if (tmp != rv)
> +		rv = _pmbus_write_byte_data(client, page, reg, tmp);
> +
> +	return rv;
> +}
> +EXPORT_SYMBOL_NS_GPL(pmbus_update_byte_data, PMBUS);
> +
>  static struct pmbus_sensor *pmbus_find_sensor(struct pmbus_data *data, int page,
>  					      int reg)
>  {
> @@ -2396,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)

Re: [PATCH v3 1/3] hwmon: (pmbus) Use driver specific ops if they exist

[Marten Lindahl]

On Wed, Apr 27, 2022 at 03:26:56PM +0200, Guenter Roeck wrote:
> On Wed, Apr 27, 2022 at 03:02:11PM +0200, Mårten Lindahl wrote:
> > Pmbus drivers using the default pmbus_regulator_ops for the enable/
> > disable/is_enabled functions will use the standard pmbus core functions
> > pmbus_read/write_byte_data. This could potentially influence some
> > specific regulator chips that for example need a time delay before each
> > data access.
> > 
> > Lets add support for drivers to use chip specific read/write operations
> > when using the standard pmbus_regulator_ops.
> 
> The subject is misleading. It should be something like "introduce and
> use write_byte_data callback". Also, existing code calling
> pmbus_write_byte_data() should call _pmbus_write_byte_data() instead.
> This applies to pmbus_update_fan() and pmbus_get_boolean().

Ok, I will change the subject to your suggestion, and change those two
functions. Thanks!

Kind regards
Mårten
> 
> Thanks,
> Guenter
> 
> > 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>
> > ---
> >  drivers/hwmon/pmbus/pmbus.h      |  2 ++
> >  drivers/hwmon/pmbus/pmbus_core.c | 58 +++++++++++++++++++++-----------
> >  2 files changed, 40 insertions(+), 20 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..1b0728c3c7d8 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -384,25 +384,6 @@ int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value)
> >  }
> >  EXPORT_SYMBOL_NS_GPL(pmbus_write_byte_data, PMBUS);
> >  
> > -int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
> > -			   u8 mask, u8 value)
> > -{
> > -	unsigned int tmp;
> > -	int rv;
> > -
> > -	rv = pmbus_read_byte_data(client, page, reg);
> > -	if (rv < 0)
> > -		return rv;
> > -
> > -	tmp = (rv & ~mask) | (value & mask);
> > -
> > -	if (tmp != rv)
> > -		rv = pmbus_write_byte_data(client, page, reg, tmp);
> > -
> > -	return rv;
> > -}
> > -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.
> > @@ -421,6 +402,43 @@ static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)
> >  	return pmbus_read_byte_data(client, page, reg);
> >  }
> >  
> > +/*
> > + * _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_byte_data(struct i2c_client *client, int page, u8 reg,
> > +			   u8 mask, u8 value)
> > +{
> > +	unsigned int tmp;
> > +	int rv;
> > +
> > +	rv = _pmbus_read_byte_data(client, page, reg);
> > +	if (rv < 0)
> > +		return rv;
> > +
> > +	tmp = (rv & ~mask) | (value & mask);
> > +
> > +	if (tmp != rv)
> > +		rv = _pmbus_write_byte_data(client, page, reg, tmp);
> > +
> > +	return rv;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(pmbus_update_byte_data, PMBUS);
> > +
> >  static struct pmbus_sensor *pmbus_find_sensor(struct pmbus_data *data, int page,
> >  					      int reg)
> >  {
> > @@ -2396,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)

[PATCH v3 2/3] 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 v3 3/3] 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 | 43 ++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 1b0728c3c7d8..afc238faa8ce 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2563,11 +2563,54 @@ 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);
+	u8 page = rdev_get_id(rdev);
+	int ret;
+
+	ret = _pmbus_read_word_data(client, page, 0xff, PMBUS_READ_VOUT);
+	if (ret < 0)
+		return ret;
+
+	ret *= 1000;
+
+	return ((ret >> 13) * 1000);
+}
+
+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);
+	u8 page = rdev_get_id(rdev);
+	long tmp = DIV_ROUND_CLOSEST(min_uV, 1000);
+	u32 val = DIV_ROUND_CLOSEST(tmp << 13, 1000);
+	int ret;
+	*selector = 0;
+
+	ret = _pmbus_read_word_data(client, page, 0xff, PMBUS_VOUT_MARGIN_LOW);
+	if (ret < 0)
+		return ret;
+
+	/* Select the voltage closest to min_uV */
+	if (ret > val)
+		val = ret;
+
+	ret = _pmbus_write_word_data(client, page, PMBUS_VOUT_COMMAND,
+				    (u16)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 v3 3/3] hwmon: (pmbus) Add get_voltage/set_voltage ops

[Guenter Roeck]

On 4/27/22 06:02, 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 | 43 ++++++++++++++++++++++++++++++++
>   1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 1b0728c3c7d8..afc238faa8ce 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2563,11 +2563,54 @@ 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);
> +	u8 page = rdev_get_id(rdev);
> +	int ret;
> +
> +	ret = _pmbus_read_word_data(client, page, 0xff, PMBUS_READ_VOUT);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret *= 1000;
> +
> +	return ((ret >> 13) * 1000);

That will need to use voltage conversion functions. VOUT isn't the same
for all chips.

> +}
> +
> +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);
> +	u8 page = rdev_get_id(rdev);
> +	long tmp = DIV_ROUND_CLOSEST(min_uV, 1000);
> +	u32 val = DIV_ROUND_CLOSEST(tmp << 13, 1000);

Same as above.

> +	int ret;
> +	*selector = 0;
> +
> +	ret = _pmbus_read_word_data(client, page, 0xff, PMBUS_VOUT_MARGIN_LOW);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Select the voltage closest to min_uV */
> +	if (ret > val)
> +		val = ret;
> +
> +	ret = _pmbus_write_word_data(client, page, PMBUS_VOUT_COMMAND,
> +				    (u16)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 v3 3/3] hwmon: (pmbus) Add get_voltage/set_voltage ops

[Marten Lindahl]

On Wed, Apr 27, 2022 at 03:10:30PM +0200, Guenter Roeck wrote:
> On 4/27/22 06:02, 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 | 43 ++++++++++++++++++++++++++++++++
> >   1 file changed, 43 insertions(+)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index 1b0728c3c7d8..afc238faa8ce 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -2563,11 +2563,54 @@ 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);
> > +	u8 page = rdev_get_id(rdev);
> > +	int ret;
> > +
> > +	ret = _pmbus_read_word_data(client, page, 0xff, PMBUS_READ_VOUT);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret *= 1000;
> > +
> > +	return ((ret >> 13) * 1000);
> 
> That will need to use voltage conversion functions. VOUT isn't the same
> for all chips.
> 
Ok, I will fix that. Will change it to use pmbus_reg2data()

> > +}
> > +
> > +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);
> > +	u8 page = rdev_get_id(rdev);
> > +	long tmp = DIV_ROUND_CLOSEST(min_uV, 1000);
> > +	u32 val = DIV_ROUND_CLOSEST(tmp << 13, 1000);
> 
> Same as above.

I will make it use pmbus_data2reg() for conversion.

Kind regards
Mårten
> 
> > +	int ret;
> > +	*selector = 0;
> > +
> > +	ret = _pmbus_read_word_data(client, page, 0xff, PMBUS_VOUT_MARGIN_LOW);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Select the voltage closest to min_uV */
> > +	if (ret > val)
> > +		val = ret;
> > +
> > +	ret = _pmbus_write_word_data(client, page, PMBUS_VOUT_COMMAND,
> > +				    (u16)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);
> >   
>