Skip to contentSkip to conversation

add regulator support for pmbus and ltc2978

PATCH Superseded [email protected] 3 participants 9 messages Aug 21, 2014 – Aug 22, 2014 Hardware monitoring

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

cover add regulator support for pmbus and ltc2978

From: Alan Tull <[email protected]>

This set of patches adds regulator support to pmbus_core.c and to ltc2978.c.

Other pmbus parts can use this to add their own regulator support.

Current ltc2978 support is limited to enable/disable.

Tested on ltc2978. Looking at the datasheets for the other parts supported by ltc2978.c, it looks like it should work.

Thanks,
Alan

Alan Tull (2):
pmbus: add regulator support
pmbus: ltc2978: add regulator enable disable

 drivers/hwmon/pmbus/Kconfig      |    9 +++++
 drivers/hwmon/pmbus/ltc2978.c    |   69 ++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/pmbus/pmbus.h      |    7 ++++
 drivers/hwmon/pmbus/pmbus_core.c |   40 ++++++++++++++++++++++
 4 files changed, 125 insertions(+)
Signature · 7 lines
1.7.9.5


_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
author
#1 lore raw

PATCH 1/2 pmbus: add regulator support

From: Alan Tull <[email protected]>

Enable pmbus device drivers to add regulator functionality.

To add a regulator, it's pretty straightforward. The pmbus device driver needs to add regulator information to its pmbus_driver_info struct:

* num_regulators : number of regulators
* reg_init : array of struct regulator_init_data
* reg_desc : array of struct regulator_desc

Signed-off-by Alan Tull <[email protected]>
---
 drivers/hwmon/pmbus/pmbus.h      |    7 +++++++
 drivers/hwmon/pmbus/pmbus_core.c |   40 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)
drivers/hwmon/pmbus/pmbus.h +7 -0
index fa9beb3..a45722f 100644
@@ -19,6 +19,8 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/regulator/driver.h>
+
 #ifndef PMBUS_H
 #define PMBUS_H
 

@@ -365,6 +367,11 @@ struct pmbus_driver_info {
 	 */
 	int (*identify)(struct i2c_client *client,
 			struct pmbus_driver_info *info);
+
+	/* Regulator functionality, if supported by this chip driver. */
+	int num_regulators;
+	struct regulator_init_data *reg_init;
+	const struct regulator_desc *reg_desc;
 };
 
 /* Function declarations */
drivers/hwmon/pmbus/pmbus_core.c +40 -0
index 291d11f..10315ee 100644
@@ -29,6 +29,8 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/jiffies.h>
 #include <linux/i2c/pmbus.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include "pmbus.h"
 
 /*

@@ -1727,6 +1729,39 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int pmbus_register_regulators(struct pmbus_data *data)
+{
+	struct regulator_dev *rdev;
+	struct regulator_config config = { };
+	const struct pmbus_driver_info *info = data->info;
+	struct device *dev = data->dev;
+	int i;
+
+	config.dev = dev;
+	config.driver_data = data;
+
+	for (i = 0; i < info->num_regulators; i++) {
+		config.init_data = &info->reg_init[i];
+		rdev = devm_regulator_register(dev, &info->reg_desc[i],
+					       &config);
+		if (IS_ERR(rdev)) {
+			dev_err(dev, "failed to register regulator %s\n",
+				info->reg_desc[i].name);
+			hwmon_device_unregister(data->hwmon_dev);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}
+#else
+static int pmbus_register_regulators(struct pmbus_data *data)
+{
+	return 0;
+}
+#endif
+
 int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		   struct pmbus_driver_info *info)
 {

@@ -1781,6 +1816,11 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		dev_err(dev, "Failed to register hwmon device\n");
 		goto out_kfree;
 	}
+
+	ret = pmbus_register_regulators(data);
+	if (ret)
+		goto out_kfree;
+
 	return 0;
 
 out_kfree:
Signature · 7 lines
1.7.9.5


_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
author
#2 lore raw

PATCH 2/2 pmbus: ltc2978: add regulator gating

In reply to [email protected]

From: Alan Tull <[email protected]>

Add support for enabling or disabling all supplies.

Signed-off-by Alan Tull <[email protected]>
---
 drivers/hwmon/pmbus/Kconfig   |    9 ++++++
 drivers/hwmon/pmbus/ltc2978.c |   69 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)
drivers/hwmon/pmbus/Kconfig +9 -0
index 6e1e493..c0d98f2 100644
@@ -56,6 +56,15 @@ config SENSORS_LTC2978
 	  This driver can also be built as a module. If so, the module will
 	  be called ltc2978.
 
+config SENSORS_LTC2978_REGULATOR
+	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
+	default n
+	depends on SENSORS_LTC2978
+	select REGULATOR
+	help
+	  If you say yes here you get regulator support for Linear
+	  Technology LTC2974, LTC2978, LTC3880, and LTC3883.
+
 config SENSORS_MAX16064
 	tristate "Maxim MAX16064"
 	default n
drivers/hwmon/pmbus/ltc2978.c +69 -0

87 lines not on this page · raw message

Signature · 7 lines
1.7.9.5


_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
#3 lore raw

Re: [PATCH 2/2] pmbus: ltc2978: add regulator gating

drivers/hwmon/pmbus/Kconfig config SENSORS_LTC2978 · line 61

Quoted 3 lines · On Thu, Aug 21, 2014 at 05:21:26PM -0500, [email protected] wrote:
drivers/hwmon/pmbus/Kconfig +3 -0
+config SENSORS_LTC2978_REGULATOR
+	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
+	default n

No need to say default n here, it's the default default.

drivers/hwmon/pmbus/Kconfig config SENSORS_LTC2978 · line 63

Quoted 2 lines
drivers/hwmon/pmbus/Kconfig +2 -0
+	depends on SENSORS_LTC2978
+	select REGULATOR

I'd expect a depends here.

drivers/hwmon/pmbus/ltc2978.c line 24

Quoted 2 lines
drivers/hwmon/pmbus/ltc2978.c +2 -0
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>

If you need machine.h that's suspicious... why do you need it?

drivers/hwmon/pmbus/ltc2978.c static int ltc2978_read_word_data_common(struct i2c_client *client, int page, · line 168

Quoted 12 lines
drivers/hwmon/pmbus/ltc2978.c +12 -0
+static int ltc2978_write_pmbus_operation(struct regulator_dev *rdev, u8 value)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret;
+
+	ret = pmbus_set_page(client, 0xff);
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_write_byte_data(client, PMBUS_OPERATION, value);
+}

This all looks very much like pmbus could use regmap and then the regmap helpers. I'd not insist on it though. What I would however suggest is that these functions should all be helpers which read the specific page, addresses and bits to write from the driver structure - I bet the code is going to be identical for most pmbus using regulators and so it makes sense to share it like we do with the generic regmap functions.

That means that any good practice can be deployed more easily and any API updates only need to update the helpers.

drivers/hwmon/pmbus/ltc2978.c static int ltc2978_read_word_data_common(struct i2c_client *client, int page, · line 213

Quoted 5 lines
drivers/hwmon/pmbus/ltc2978.c +5 -0
+static struct regulator_init_data ltc2978_regulator_init = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+};

You should not be forcing this on, you don't know what's safe on any given board. Allow the board to specify constraints then it has control.

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

maintainer
#4 lore raw

Re: [PATCH 2/2] pmbus: ltc2978: add regulator gating

Quoted 18 lines · Mark Brown

drivers/hwmon/pmbus/Kconfig config SENSORS_LTC2978 · line 61

Quoted 3 lines · On Thu, Aug 21, 2014 at 05:21:26PM -0500, [email protected] wrote:
drivers/hwmon/pmbus/Kconfig +3 -0
+config SENSORS_LTC2978_REGULATOR
+	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
+	default n

No need to say default n here, it's the default default.

drivers/hwmon/pmbus/Kconfig config SENSORS_LTC2978 · line 63

Quoted 2 lines
drivers/hwmon/pmbus/Kconfig +2 -0
+	depends on SENSORS_LTC2978
+	select REGULATOR

I'd expect a depends here.

drivers/hwmon/pmbus/ltc2978.c line 24

Quoted 2 lines
drivers/hwmon/pmbus/ltc2978.c +2 -0
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>

If you need machine.h that's suspicious... why do you need it?

Quoted 15 lines

drivers/hwmon/pmbus/ltc2978.c static int ltc2978_read_word_data_common(struct i2c_client *client, int page, · line 168

Quoted 12 lines
drivers/hwmon/pmbus/ltc2978.c +12 -0
+static int ltc2978_write_pmbus_operation(struct regulator_dev *rdev, u8 value)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret;
+
+	ret = pmbus_set_page(client, 0xff);
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_write_byte_data(client, PMBUS_OPERATION, value);
+}

This all looks very much like pmbus could use regmap and then the regmap helpers. I'd not insist on it though. What I would however suggest is

Not unless regmap got extended recently to support quick, byte, and word smbus accesses at the same time.

Guenter

Quoted 8 lines

that these functions should all be helpers which read the specific page, addresses and bits to write from the driver structure - I bet the code is going to be identical for most pmbus using regulators and so it makes sense to share it like we do with the generic regmap functions.

That means that any good practice can be deployed more easily and any API updates only need to update the helpers.

Quoted 9 lines

drivers/hwmon/pmbus/ltc2978.c static int ltc2978_read_word_data_common(struct i2c_client *client, int page, · line 213

Quoted 5 lines
drivers/hwmon/pmbus/ltc2978.c +5 -0
+static struct regulator_init_data ltc2978_regulator_init = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+};

You should not be forcing this on, you don't know what's safe on any given board. Allow the board to specify constraints then it has control.

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#5 lore raw

Re: [PATCH 2/2] pmbus: ltc2978: add regulator gating

Quoted 8 lines · Guenter Roeck
Quoted 4 lines · Mark Brown

On Thu, Aug 21, 2014 at 05:21:26PM -0500, [email protected] wrote:

This all looks very much like pmbus could use regmap and then the regmap helpers. I'd not insist on it though. What I would however suggest is

Not unless regmap got extended recently to support quick, byte, and word smbus accesses at the same time.

Depending on how you decide which it quite possibly does - if it's based on the register number that'd work.

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

maintainer
#6 lore raw

Re: [PATCH 2/2] pmbus: ltc2978: add regulator gating

Quoted 4 lines · Mark Brown
Quoted 2 lines · Guenter Roeck
Quoted 1 line · Mark Brown

On Thu, Aug 21, 2014 at 05:21:26PM -0500, [email protected] wrote:

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

This all looks very much like pmbus could use regmap and then the regmap helpers. I'd not insist on it though. What I would however suggest is

Not unless regmap got extended recently to support quick, byte, and word smbus accesses at the same time.

Depending on how you decide which it quite possibly does - if it's based on the register number that'd work.

Mostly per register, but also per chip (for manufacturing specific registers the register size is determined by the chip type). Also, there are block registers. Plus, the scope of each register (ie if it is paged or not) is chip specific. The same register may be paged on one chip, and unpaged on another. I'll have another look to see if that all can be mapped into the regmap model. If yes, it might actually be quite helpful and might simplify the pmbus code quite a bit.

Either case, even if regmap now supports all the PMBus oddities, converting the pmbus drivers to use regmap should be a separate patch set and not be tied together.

Tnanks,
Guenter

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

author
#7 lore raw

Re: [PATCH 2/2] pmbus: ltc2978: add regulator gating

In reply to Mark Brown
Quoted 8 lines · Mark Brown

drivers/hwmon/pmbus/Kconfig config SENSORS_LTC2978 · line 61

Quoted 3 lines · On Thu, Aug 21, 2014 at 05:21:26PM -0500, [email protected] wrote:
drivers/hwmon/pmbus/Kconfig +3 -0
+config SENSORS_LTC2978_REGULATOR
+	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
+	default n

No need to say default n here, it's the default default.

Quoted 4 lines

drivers/hwmon/pmbus/Kconfig config SENSORS_LTC2978 · line 63

Quoted 2 lines
drivers/hwmon/pmbus/Kconfig +2 -0
+	depends on SENSORS_LTC2978
+	select REGULATOR

I'd expect a depends here.

Hi Mark,

Thanks for the review. I'll fix these two Kconfig issues.

Quoted 5 lines

drivers/hwmon/pmbus/ltc2978.c line 24

Quoted 3 lines
drivers/hwmon/pmbus/ltc2978.c +2 -0
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>

If you need machine.h that's suspicious... why do you need it?

It was to define 'struct regulator_init_data' which I had hardwired here as you noted below. Will fix.

Quoted 24 lines

drivers/hwmon/pmbus/ltc2978.c static int ltc2978_read_word_data_common(struct i2c_client *client, int page, · line 168

Quoted 13 lines
drivers/hwmon/pmbus/ltc2978.c +12 -0
+static int ltc2978_write_pmbus_operation(struct regulator_dev *rdev, u8 value)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret;
+
+	ret = pmbus_set_page(client, 0xff);
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_write_byte_data(client, PMBUS_OPERATION, value);
+}

This all looks very much like pmbus could use regmap and then the regmap helpers. I'd not insist on it though. What I would however suggest is that these functions should all be helpers which read the specific page, addresses and bits to write from the driver structure - I bet the code is going to be identical for most pmbus using regulators and so it makes sense to share it like we do with the generic regmap functions.

That means that any good practice can be deployed more easily and any API updates only need to update the helpers.

Quoted 10 lines

drivers/hwmon/pmbus/ltc2978.c static int ltc2978_read_word_data_common(struct i2c_client *client, int page, · line 213

Quoted 5 lines
drivers/hwmon/pmbus/ltc2978.c +5 -0
+static struct regulator_init_data ltc2978_regulator_init = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+};

You should not be forcing this on, you don't know what's safe on any given board. Allow the board to specify constraints then it has control.

Yes, this info should be from board info or device tree. I'll fix this.

Thanks!

Alan

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

author
#8 lore raw
In reply to [email protected]
Quoted 19 lines · On Thu, 21 Aug 2014, [email protected] wrote:

From: Alan Tull <[email protected]>

This set of patches adds regulator support to pmbus_core.c and to ltc2978.c.

Other pmbus parts can use this to add their own regulator support.

Current ltc2978 support is limited to enable/disable.

Tested on ltc2978. Looking at the datasheets for the other parts supported by ltc2978.c, it looks like it should work.

Thanks,
Alan

Alan Tull (2):
pmbus: add regulator support
pmbus: ltc2978: add regulator enable disable

Quoted 10 lines
 drivers/hwmon/pmbus/Kconfig      |    9 +++++
 drivers/hwmon/pmbus/ltc2978.c    |   69 ++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/pmbus/pmbus.h      |    7 ++++
 drivers/hwmon/pmbus/pmbus_core.c |   40 ++++++++++++++++++++++
 4 files changed, 125 insertions(+)
Signature · 1 line
1.7.9.5

I've posted V2 of these patches.

Alan

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors