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

[PATCH 0/2] 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 functions for setting and getting voltage are missing 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

Mårten Lindahl (2):
  hwmon: (pmbus/ltc2978) Add driver specific regulator ops
  hwmon: (pmbus/ltc2978) Add get_voltage/set_voltage ops

 drivers/hwmon/pmbus/ltc2978.c | 133 ++++++++++++++++++++++++++++++++--
 1 file changed, 125 insertions(+), 8 deletions(-)

-- 
2.30.2

[PATCH 1/2] hwmon: (pmbus/ltc2978) Add driver specific regulator ops

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

But this driver uses the default pmbus_regulator_ops for the enable/
disable/is_enabled functions. By using these functions it bypasses the
wait time recommendations for several of the devices managed by the
driver (ltc3880/ltc3882/ltc3883/ltc3884/ltc3886/ltc3887/ltc3889/ltm4664/
ltm4675/ltm4676/ltm4677/ltm4678/ltm4680/ltm4686/ltm4700/ltc7880).

Lets add driver specific regulator enable/disable/is_enabled ops which
takes the wait time into consideration for the specified devices, by
overriding pmbus_read_byte_data with internal ltc_read_byte_data.

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

diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index 0127273883f0..822edec33ba7 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -551,15 +551,78 @@ static const struct i2c_device_id ltc2978_id[] = {
 MODULE_DEVICE_TABLE(i2c, ltc2978_id);
 
 #if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+static int ltc2978_regulator_is_enabled(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 = ltc_read_byte_data(client, page, PMBUS_OPERATION);
+	if (ret < 0)
+		return ret;
+
+	return !!(ret & PB_OPERATION_CONTROL_ON);
+}
+
+static int ltc2978_regulator_on_off(struct regulator_dev *rdev, bool enable)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	u8 page = rdev_get_id(rdev);
+	unsigned int tmp;
+	int rv;
+
+	rv = ltc_read_byte_data(client, page, PMBUS_OPERATION);
+	if (rv < 0)
+		return rv;
+
+	tmp = (rv & ~PB_OPERATION_CONTROL_ON) |
+	    (enable ? PB_OPERATION_CONTROL_ON : 0);
+
+	if (tmp != rv)
+		rv = pmbus_write_byte_data(client, page, PMBUS_OPERATION, tmp);
+
+	return rv;
+}
+
+static int ltc2978_regulator_enable(struct regulator_dev *rdev)
+{
+	return ltc2978_regulator_on_off(rdev, 1);
+}
+
+static int ltc2978_regulator_disable(struct regulator_dev *rdev)
+{
+	return ltc2978_regulator_on_off(rdev, 0);
+}
+
+static const struct regulator_ops ltc2978_regulator_ops = {
+	.enable = ltc2978_regulator_enable,
+	.disable = ltc2978_regulator_disable,
+	.is_enabled = ltc2978_regulator_is_enabled,
+};
+
+/* Macro for filling in array of struct regulator_desc */
+#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 = &ltc2978_regulator_ops,			\
+		.type = REGULATOR_VOLTAGE,			\
+		.owner = THIS_MODULE,				\
+	}
+
 static const struct regulator_desc ltc2978_reg_desc[] = {
-	PMBUS_REGULATOR("vout", 0),
-	PMBUS_REGULATOR("vout", 1),
-	PMBUS_REGULATOR("vout", 2),
-	PMBUS_REGULATOR("vout", 3),
-	PMBUS_REGULATOR("vout", 4),
-	PMBUS_REGULATOR("vout", 5),
-	PMBUS_REGULATOR("vout", 6),
-	PMBUS_REGULATOR("vout", 7),
+	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),
 };
 #endif /* CONFIG_SENSORS_LTC2978_REGULATOR */
 
-- 
2.30.2

[PATCH 2/2] hwmon: (pmbus/ltc2978) Add get_voltage/set_voltage ops

[Mårten Lindahl]

This driver does not have regulator specific 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/ltc2978.c | 55 +++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index 822edec33ba7..fce08c33e66a 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -596,10 +596,65 @@ static int ltc2978_regulator_disable(struct regulator_dev *rdev)
 	return ltc2978_regulator_on_off(rdev, 0);
 }
 
+static int ltc2978_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 = ltc_wait_ready(client);
+	if (ret < 0)
+		return 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 ltc2978_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 = ltc_wait_ready(client);
+	if (ret < 0)
+		return ret;
+
+	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 = ltc_wait_ready(client);
+	if (ret < 0)
+		return ret;
+
+	ret = pmbus_write_word_data(client, page, PMBUS_VOUT_COMMAND,
+				    (u16)val);
+
+	return ret;
+}
+
 static const struct regulator_ops ltc2978_regulator_ops = {
 	.enable = ltc2978_regulator_enable,
 	.disable = ltc2978_regulator_disable,
 	.is_enabled = ltc2978_regulator_is_enabled,
+	.get_voltage = ltc2978_regulator_get_voltage,
+	.set_voltage = ltc2978_regulator_set_voltage,
 };
 
 /* Macro for filling in array of struct regulator_desc */
-- 
2.30.2

Re: [PATCH 1/2] hwmon: (pmbus/ltc2978) Add driver specific regulator ops

[Guenter Roeck]

On Wed, Apr 06, 2022 at 02:43:20PM +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.
> 
> But this driver uses the default pmbus_regulator_ops for the enable/
> disable/is_enabled functions. By using these functions it bypasses the
> wait time recommendations for several of the devices managed by the
> driver (ltc3880/ltc3882/ltc3883/ltc3884/ltc3886/ltc3887/ltc3889/ltm4664/
> ltm4675/ltm4676/ltm4677/ltm4678/ltm4680/ltm4686/ltm4700/ltc7880).
> 
> Lets add driver specific regulator enable/disable/is_enabled ops which
> takes the wait time into consideration for the specified devices, by
> overriding pmbus_read_byte_data with internal ltc_read_byte_data.
> 
> Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>

This patch solves the wrong problem. The real problem is that the
regulator code in the PMBus core writes direcetly into the chip
and doesn't use the driver functions to do it if needed, and that
the PMBus core does not support a chip-specific write_byte_data function
(because so far it wasn't needed). That needs to get fixed, and then
we won't need chip specific regulator functions.

Thanks,
Guenter

Re: [PATCH 1/2] hwmon: (pmbus/ltc2978) Add driver specific regulator ops

[Marten Lindahl]

On Sun, Apr 24, 2022 at 06:49:10PM +0200, Guenter Roeck wrote:
> On Wed, Apr 06, 2022 at 02:43:20PM +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.
> > 
> > But this driver uses the default pmbus_regulator_ops for the enable/
> > disable/is_enabled functions. By using these functions it bypasses the
> > wait time recommendations for several of the devices managed by the
> > driver (ltc3880/ltc3882/ltc3883/ltc3884/ltc3886/ltc3887/ltc3889/ltm4664/
> > ltm4675/ltm4676/ltm4677/ltm4678/ltm4680/ltm4686/ltm4700/ltc7880).
> > 
> > Lets add driver specific regulator enable/disable/is_enabled ops which
> > takes the wait time into consideration for the specified devices, by
> > overriding pmbus_read_byte_data with internal ltc_read_byte_data.
> > 
> > Signed-off-by: Mårten Lindahl <marten.lindahl@axis.com>

Hi Guenter!
Thanks for your comments.
> 
> This patch solves the wrong problem. The real problem is that the
> regulator code in the PMBus core writes direcetly into the chip
> and doesn't use the driver functions to do it if needed, and that
> the PMBus core does not support a chip-specific write_byte_data function
> (because so far it wasn't needed). That needs to get fixed, and then
> we won't need chip specific regulator functions.

Good point. I will add support for driver callback functions in pmbus
core and remove the ltc2978 enable/disable/is_enabled functions.

Kind regards
Mårten
> 
> Thanks,
> Guenter