regulator support for pmbus and ltc2978

From PMBus_Specification_Part_II_Rev_1-3_20140318.pdf:

From: Alan Tull <[email protected]>

Add support for simple on/off control of each channel.

To add regulator support, the pmbus part driver needs to add regulator_desc information, of_regulator_match information, and number of regulators to its pmbus_driver_info struct.

regulator_desc can be declared using default macro for a regulator (PMBUS_REGULATOR) that is in pmbus.h

The regulator_init_data can be intialized from either platform data or the device tree.

Signed-off-by Alan Tull <[email protected]>

v2: Remove '#include <linux/regulator/machine.h>'
Only one regulator per pmbus device
Get regulator_init_data from pdata or device tree

v3: Support multiple regulators for each chip
Move most code to pmbus_core.c
fixed values for on/off

---
 drivers/hwmon/pmbus/pmbus.h      |   27 ++++++++
 drivers/hwmon/pmbus/pmbus_core.c |  133 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    4 ++
 3 files changed, 164 insertions(+)

drivers/hwmon/pmbus/pmbus.h +27 -0

index fa9beb3..74aa382 100644
@@ -19,6 +19,9 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
 #ifndef PMBUS_H
 #define PMBUS_H
 

@@ -186,6 +189,12 @@
 #define PMBUS_VIRT_STATUS_VMON		(PMBUS_VIRT_BASE + 35)
 
 /*
+ * OPERATION
+ */
+#define PB_OPERATION_CONTROL_ON		(1<<7)
+#define PB_OPERATION_CONTROL_SEQ_OFF	(1<<6)
+
+/*
  * CAPABILITY
  */
 #define PB_CAPABILITY_SMBALERT		(1<<4)

@@ -365,8 +374,26 @@ 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;
+	const struct regulator_desc *reg_desc;
+	struct of_regulator_match *reg_matches;
 };
 
+/* Regulator ops */
+
+extern struct regulator_ops pmbus_regulator_regulator_ops;
+
+/* Macro for filling in array of struct regulator_desc */
+#define PMBUS_REGULATOR(_name, _id)				\
+	[_id] = {						\
+		.name = (_name # _id),				\
+		.id = (_id),					\
+		.ops = &pmbus_regulator_regulator_ops,		\
+		.owner = THIS_MODULE,				\
+	}
+
 /* Function declarations */
 
 void pmbus_clear_cache(struct i2c_client *client);

drivers/hwmon/pmbus/pmbus_core.c +72 -0

index d6c3701..9ab8bd4 100644
@@ -29,6 +29,9 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/jiffies.h>
 #include <linux/i2c/pmbus.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include "pmbus.h"
 
 /*

@@ -1758,6 +1761,125 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int pmbus_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 = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
+	if (ret < 0)
+		return ret;
+
+	return !!(ret & PB_OPERATION_CONTROL_ON);
+}
+
+static int _pmbus_regulator_enable(struct regulator_dev *rdev, bool enable)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	u8 val, page = rdev_get_id(rdev);
+
+	if (enable)
+		val = PB_OPERATION_CONTROL_ON;
+	else
+		val = 0;
+
+	return pmbus_update_byte_data(client, page, PMBUS_OPERATION,
+				      PB_OPERATION_CONTROL_ON, val);
+}
+
+static int pmbus_regulator_enable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_enable(rdev, 1);
+}
+
+static int pmbus_regulator_disable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_enable(rdev, 0);
+}
+
+struct regulator_ops pmbus_regulator_regulator_ops = {
+	.enable = pmbus_regulator_enable,
+	.disable = pmbus_regulator_disable,
+	.is_enabled = pmbus_regulator_is_enabled,
+};
+EXPORT_SYMBOL_GPL(pmbus_regulator_regulator_ops);
+
+#if IS_ENABLED(CONFIG_OF)
+static int pmbus_regulator_parse_dt(struct device *dev,
+				    const struct pmbus_driver_info *info)
+{
+	struct device_node *np_regulators;
+	int ret;
+
+	if (!info->num_regulators)
+		return 0;
+
+	if (!info->reg_matches || !info->reg_desc)
+		return -EINVAL;
+
+	np_regulators = of_get_child_by_name(dev->of_node, "regulators");
+	if (!np_regulators)
+		return 0;
+
+	ret = of_regulator_match(dev, np_regulators, info->reg_matches,
+				 info->num_regulators);
+	of_node_put(np_regulators);
+	if (ret < 0)
+		return ret;

Hi Alan,

Quoted 170 lines · On 9/24/14, 12:57 PM, [email protected] wrote:

From: Alan Tull <[email protected]>

Add support for simple on/off control of each channel.

To add regulator support, the pmbus part driver needs to add regulator_desc information, of_regulator_match information, and number of regulators to its pmbus_driver_info struct.

regulator_desc can be declared using default macro for a regulator (PMBUS_REGULATOR) that is in pmbus.h

The regulator_init_data can be intialized from either platform data or the device tree.

Signed-off-by Alan Tull <[email protected]>

v2: Remove '#include <linux/regulator/machine.h>'
Only one regulator per pmbus device
Get regulator_init_data from pdata or device tree

v3: Support multiple regulators for each chip
Move most code to pmbus_core.c
fixed values for on/off

---
 drivers/hwmon/pmbus/pmbus.h      |   27 ++++++++
 drivers/hwmon/pmbus/pmbus_core.c |  133 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    4 ++
 3 files changed, 164 insertions(+)

drivers/hwmon/pmbus/pmbus.h +27 -0

index fa9beb3..74aa382 100644
@@ -19,6 +19,9 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
 #ifndef PMBUS_H
 #define PMBUS_H
 

@@ -186,6 +189,12 @@
 #define PMBUS_VIRT_STATUS_VMON		(PMBUS_VIRT_BASE + 35)
 
 /*
+ * OPERATION
+ */
+#define PB_OPERATION_CONTROL_ON		(1<<7)
+#define PB_OPERATION_CONTROL_SEQ_OFF	(1<<6)
+
+/*
  * CAPABILITY
  */
 #define PB_CAPABILITY_SMBALERT		(1<<4)

@@ -365,8 +374,26 @@ 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;
+	const struct regulator_desc *reg_desc;
+	struct of_regulator_match *reg_matches;
 };
 
+/* Regulator ops */
+
+extern struct regulator_ops pmbus_regulator_regulator_ops;
+
+/* Macro for filling in array of struct regulator_desc */
+#define PMBUS_REGULATOR(_name, _id)				\
+	[_id] = {						\
+		.name = (_name # _id),				\
+		.id = (_id),					\
+		.ops = &pmbus_regulator_regulator_ops,		\
+		.owner = THIS_MODULE,				\
+	}
+
 /* Function declarations */
 
 void pmbus_clear_cache(struct i2c_client *client);

drivers/hwmon/pmbus/pmbus_core.c +72 -0

index d6c3701..9ab8bd4 100644
@@ -29,6 +29,9 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/jiffies.h>
 #include <linux/i2c/pmbus.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include "pmbus.h"
 
 /*

@@ -1758,6 +1761,125 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int pmbus_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 = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
+	if (ret < 0)
+		return ret;
+
+	return !!(ret & PB_OPERATION_CONTROL_ON);
+}
+
+static int _pmbus_regulator_enable(struct regulator_dev *rdev, bool enable)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	u8 val, page = rdev_get_id(rdev);
+
+	if (enable)
+		val = PB_OPERATION_CONTROL_ON;
+	else
+		val = 0;
+
+	return pmbus_update_byte_data(client, page, PMBUS_OPERATION,
+				      PB_OPERATION_CONTROL_ON, val);
+}
+
+static int pmbus_regulator_enable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_enable(rdev, 1);
+}
+
+static int pmbus_regulator_disable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_enable(rdev, 0);
+}
+
+struct regulator_ops pmbus_regulator_regulator_ops = {
+	.enable = pmbus_regulator_enable,
+	.disable = pmbus_regulator_disable,
+	.is_enabled = pmbus_regulator_is_enabled,
+};
+EXPORT_SYMBOL_GPL(pmbus_regulator_regulator_ops);
+
+#if IS_ENABLED(CONFIG_OF)
+static int pmbus_regulator_parse_dt(struct device *dev,
+				    const struct pmbus_driver_info *info)
+{
+	struct device_node *np_regulators;
+	int ret;
+
+	if (!info->num_regulators)
+		return 0;
+
+	if (!info->reg_matches || !info->reg_desc)
+		return -EINVAL;
+
+	np_regulators = of_get_child_by_name(dev->of_node, "regulators");
+	if (!np_regulators)
+		return 0;
+
+	ret = of_regulator_match(dev, np_regulators, info->reg_matches,
+				 info->num_regulators);
+	of_node_put(np_regulators);
+	if (ret < 0)
+		return ret;

drivers/hwmon/pmbus/pmbus_core.c static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, · line 1842

Quoted 10 lines

drivers/hwmon/pmbus/pmbus_core.c +10 -0

+
+	return 0;
+}
+#else
+static int pmbus_regulator_parse_dt(struct device *dev,
+				    const struct pmbus_driver_info *info)
+{
+	return 0;
+}
+#endif

This breaks the build if CONFIG_REGULATOR is not enabled:

drivers/hwmon/pmbus/pmbus_core.c: In function âpmbus_do_probeâ:
drivers/hwmon/pmbus/pmbus_core.c:1894:2: error: implicit declaration of function âpmbus_regulator_parse_dtâ [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors

Dinh

From: Alan Tull <[email protected]>

Add support for simple on/off control of each channel.

To add regulator support, the pmbus part driver needs to add regulator_desc information, of_regulator_match information, and number of regulators to its pmbus_driver_info struct.

regulator_desc can be declared using default macro for a regulator (PMBUS_REGULATOR) that is in pmbus.h

The regulator_init_data can be intialized from either platform data or the device tree.

Signed-off-by Alan Tull <[email protected]>

v2: Remove '#include <linux/regulator/machine.h>'
Only one regulator per pmbus device
Get regulator_init_data from pdata or device tree

v3: Support multiple regulators for each chip
Move most code to pmbus_core.c
fixed values for on/off

---
 drivers/hwmon/pmbus/pmbus.h      |   27 ++++++++
 drivers/hwmon/pmbus/pmbus_core.c |  133 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    4 ++
 3 files changed, 164 insertions(+)

drivers/hwmon/pmbus/pmbus.h +3 -0

index fa9beb3..74aa382 100644
@@ -19,6 +19,9 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
 #ifndef PMBUS_H
 #define PMBUS_H
 

drivers/hwmon/pmbus/pmbus.h +3 -0

+
+/*
  * CAPABILITY
  */
 #define PB_CAPABILITY_SMBALERT		(1<<4)

@@ -365,8 +374,26 @@ struct pmbus_driver_info {
 	 */
 	int (*identify)(struct i2c_client *client,
 			struct pmbus_driver_info *info);
+

drivers/hwmon/pmbus/pmbus.h +0 -0

 /* Function declarations */
 
 void pmbus_clear_cache(struct i2c_client *client);

drivers/hwmon/pmbus/pmbus_core.c +10 -0

index d6c3701..9ab8bd4 100644
@@ -29,6 +29,9 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/jiffies.h>
 #include <linux/i2c/pmbus.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include "pmbus.h"
 
 /*

@@ -1758,6 +1761,125 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int pmbus_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;

drivers/hwmon/pmbus/pmbus_core.c +6 -0

+}
+
+static int pmbus_regulator_enable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_enable(rdev, 1);
+}

drivers/hwmon/pmbus/pmbus_core.c +75 -0

+};
+EXPORT_SYMBOL_GPL(pmbus_regulator_regulator_ops);
+
+#if IS_ENABLED(CONFIG_OF)
+static int pmbus_regulator_parse_dt(struct device *dev,
+				    const struct pmbus_driver_info *info)
+{
+	struct device_node *np_regulators;
+	int ret;
+
+	if (!info->num_regulators)
+		return 0;
+
+	if (!info->reg_matches || !info->reg_desc)
+		return -EINVAL;
+
+	np_regulators = of_get_child_by_name(dev->of_node, "regulators");
+	if (!np_regulators)
+		return 0;
+
+	ret = of_regulator_match(dev, np_regulators, info->reg_matches,
+				 info->num_regulators);
+	of_node_put(np_regulators);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+#else
+static int pmbus_regulator_parse_dt(struct device *dev,
+				    const struct pmbus_driver_info *info)
+{
+	return 0;
+}
+#endif
+
+static int pmbus_regulator_register(struct pmbus_data *data)
+{
+	struct device *dev = data->dev;
+	const struct pmbus_driver_info *info = data->info;
+	const struct pmbus_platform_data *pdata = dev_get_platdata(dev);
+	struct regulator_dev *rdev;
+	int i;
+
+	for (i = 0; i < info->num_regulators; i++) {
+		struct regulator_config config = { };
+
+		config.dev = dev;
+		config.driver_data = data;
+
+		if (pdata && pdata->reg_init_data) {
+			config.init_data = &pdata->reg_init_data[i];
+		} else {
+			config.init_data = info->reg_matches[i].init_data;
+			config.of_node = info->reg_matches[i].of_node;
+		}
+
+		rdev = devm_regulator_register(dev, &info->reg_desc[i],
+					       &config);
+		if (IS_ERR(rdev)) {
+			dev_err(dev, "Failed to register %s regulator\n",
+				info->reg_desc[i].name);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}
+#else
+static int pmbus_regulator_register(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)
 {

drivers/hwmon/pmbus/pmbus_core.c +7 -0

 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WRITE_BYTE
 				     | I2C_FUNC_SMBUS_BYTE_DATA
 				     | I2C_FUNC_SMBUS_WORD_DATA))

@@ -1812,8 +1938,15 @@ 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_regulator_register(data);
+	if (ret)
+		goto out_unregister;
+
 	return 0;
 
+out_unregister:
+	hwmon_device_unregister(data->hwmon_dev);
 out_kfree:
 	kfree(data->group.attrs);
 	return ret;

include/linux/i2c/pmbus.h +4 -0

index 69280db..ee3c2ab 100644
@@ -40,6 +40,10 @@
 
 struct pmbus_platform_data {
 	u32 flags;		/* Device specific flags */
+
+	/* regulator support */
+	int num_regulators;
+	struct regulator_init_data *reg_init_data;
 };
 
 #endif /* _PMBUS_H_ */

Signature · 1 line

1.7.9.5

Quoted 14 lines · On Wed, Sep 24, 2014 at 12:57:55PM -0500, [email protected] wrote:

From: Alan Tull <[email protected]>

Add support for simple on/off control of each channel.

To add regulator support, the pmbus part driver needs to add regulator_desc information, of_regulator_match information, and number of regulators to its pmbus_driver_info struct.

regulator_desc can be declared using default macro for a regulator (PMBUS_REGULATOR) that is in pmbus.h

The regulator_init_data can be intialized from either platform data or the device tree.

Quoted 2 lines

Signed-off-by Alan Tull <[email protected]>

Hi Alan,

Overall looks pretty good. Couple of comments inline.

Quoted 30 lines

v2: Remove '#include <linux/regulator/machine.h>'
Only one regulator per pmbus device
Get regulator_init_data from pdata or device tree

v3: Support multiple regulators for each chip
Move most code to pmbus_core.c
fixed values for on/off

---
 drivers/hwmon/pmbus/pmbus.h      |   27 ++++++++
 drivers/hwmon/pmbus/pmbus_core.c |  133 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    4 ++
 3 files changed, 164 insertions(+)

drivers/hwmon/pmbus/pmbus.h +3 -0

index fa9beb3..74aa382 100644
@@ -19,6 +19,9 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
 #ifndef PMBUS_H
 #define PMBUS_H
 

@@ -186,6 +189,12 @@
 #define PMBUS_VIRT_STATUS_VMON		(PMBUS_VIRT_BASE + 35)
 

drivers/hwmon/pmbus/pmbus.h line 195

Quoted 5 lines

drivers/hwmon/pmbus/pmbus.h +4 -0

 /*
+ * OPERATION
+ */
+#define PB_OPERATION_CONTROL_ON		(1<<7)
+#define PB_OPERATION_CONTROL_SEQ_OFF	(1<<6)

Can those defines be more consistent ? Does it really need SEQ_OFF or can it just be OFF ?

Quoted 17 lines

drivers/hwmon/pmbus/pmbus.h +7 -0

+
+/*
  * CAPABILITY
  */
 #define PB_CAPABILITY_SMBALERT		(1<<4)

@@ -365,8 +374,26 @@ 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;
+	const struct regulator_desc *reg_desc;
+	struct of_regulator_match *reg_matches;
 };
 

drivers/hwmon/pmbus/pmbus.h struct pmbus_driver_info { · line 387

Quoted 4 lines

drivers/hwmon/pmbus/pmbus.h +4 -0

+/* Regulator ops */
+
+extern struct regulator_ops pmbus_regulator_regulator_ops;
+

How about just pmbus_regulator_ops ? I don't see a double regulator_ variable name anywhere else in the code, and I don't really see the benefit of it.

drivers/hwmon/pmbus/pmbus.h struct pmbus_driver_info { · line 396

Quoted 10 lines

drivers/hwmon/pmbus/pmbus.h +9 -0

+/* Macro for filling in array of struct regulator_desc */
+#define PMBUS_REGULATOR(_name, _id)				\
+	[_id] = {						\
+		.name = (_name # _id),				\
+		.id = (_id),					\
+		.ops = &pmbus_regulator_regulator_ops,		\
+		.owner = THIS_MODULE,				\
+	}
+

Any idea how/if we can get rid of the resulting checkpatch error ?

Quoted 34 lines

drivers/hwmon/pmbus/pmbus.h +0 -0

 /* Function declarations */
 
 void pmbus_clear_cache(struct i2c_client *client);

drivers/hwmon/pmbus/pmbus_core.c +15 -0

index d6c3701..9ab8bd4 100644
@@ -29,6 +29,9 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/jiffies.h>
 #include <linux/i2c/pmbus.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include "pmbus.h"
 
 /*

@@ -1758,6 +1761,125 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int pmbus_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 = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
+	if (ret < 0)
+		return ret;
+

drivers/hwmon/pmbus/pmbus_core.c static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, · line 1780

Quoted 5 lines

drivers/hwmon/pmbus/pmbus_core.c +5 -0

+	return !!(ret & PB_OPERATION_CONTROL_ON);
+}
+
+static int _pmbus_regulator_enable(struct regulator_dev *rdev, bool enable)
+{

Can you find a better name for this function ? After all, it doesn't just enable the regulator, it also disables it.

drivers/hwmon/pmbus/pmbus_core.c static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, · line 1791

Quoted 12 lines

drivers/hwmon/pmbus/pmbus_core.c +11 -0

+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	u8 val, page = rdev_get_id(rdev);
+
+	if (enable)
+		val = PB_OPERATION_CONTROL_ON;
+	else
+		val = 0;
+
+	return pmbus_update_byte_data(client, page, PMBUS_OPERATION,
+				      PB_OPERATION_CONTROL_ON, val);

enable ? PB_OPERATION_CONTROL_ON : 0

would be much simpler here.

Quoted 7 lines

drivers/hwmon/pmbus/pmbus_core.c +6 -0

+}
+
+static int pmbus_regulator_enable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_enable(rdev, 1);
+}

drivers/hwmon/pmbus/pmbus_core.c static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, · line 1807

Quoted 10 lines

drivers/hwmon/pmbus/pmbus_core.c +10 -0

+
+static int pmbus_regulator_disable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_enable(rdev, 0);
+}
+
+struct regulator_ops pmbus_regulator_regulator_ops = {
+	.enable = pmbus_regulator_enable,
+	.disable = pmbus_regulator_disable,
+	.is_enabled = pmbus_regulator_is_enabled,

No get_voltage support ?

[ Guess it isn't mandatory. We can add it later to get this going. ]

Quoted 80 lines

drivers/hwmon/pmbus/pmbus_core.c +75 -0

+};
+EXPORT_SYMBOL_GPL(pmbus_regulator_regulator_ops);
+
+#if IS_ENABLED(CONFIG_OF)
+static int pmbus_regulator_parse_dt(struct device *dev,
+				    const struct pmbus_driver_info *info)
+{
+	struct device_node *np_regulators;
+	int ret;
+
+	if (!info->num_regulators)
+		return 0;
+
+	if (!info->reg_matches || !info->reg_desc)
+		return -EINVAL;
+
+	np_regulators = of_get_child_by_name(dev->of_node, "regulators");
+	if (!np_regulators)
+		return 0;
+
+	ret = of_regulator_match(dev, np_regulators, info->reg_matches,
+				 info->num_regulators);
+	of_node_put(np_regulators);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+#else
+static int pmbus_regulator_parse_dt(struct device *dev,
+				    const struct pmbus_driver_info *info)
+{
+	return 0;
+}
+#endif
+
+static int pmbus_regulator_register(struct pmbus_data *data)
+{
+	struct device *dev = data->dev;
+	const struct pmbus_driver_info *info = data->info;
+	const struct pmbus_platform_data *pdata = dev_get_platdata(dev);
+	struct regulator_dev *rdev;
+	int i;
+
+	for (i = 0; i < info->num_regulators; i++) {
+		struct regulator_config config = { };
+
+		config.dev = dev;
+		config.driver_data = data;
+
+		if (pdata && pdata->reg_init_data) {
+			config.init_data = &pdata->reg_init_data[i];
+		} else {
+			config.init_data = info->reg_matches[i].init_data;
+			config.of_node = info->reg_matches[i].of_node;
+		}
+
+		rdev = devm_regulator_register(dev, &info->reg_desc[i],
+					       &config);
+		if (IS_ERR(rdev)) {
+			dev_err(dev, "Failed to register %s regulator\n",
+				info->reg_desc[i].name);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}
+#else
+static int pmbus_regulator_register(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)
 {

drivers/hwmon/pmbus/pmbus_core.c int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, · line 1897

Quoted 7 lines

drivers/hwmon/pmbus/pmbus_core.c +4 -0

@@ -1769,6 +1891,10 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 	if (!info)
 		return -ENODEV;
 
+	ret = pmbus_regulator_parse_dt(dev, info);
+	if (ret)
+		return ret;
+

You have the conditions wrong above.

If CONFIG_REGULATOR is not enabled, this will fail to build, since pmbus_regulator_parse_dt is not declared at all in this case.

I can understand that you want to parse the dt early, but it would be simpler to just parse it from pmbus_regulator_register(). It is only relevant if regulators are configured anyway, and we don't really need to optimize the code for the error case.

Quoted 38 lines

drivers/hwmon/pmbus/pmbus_core.c +7 -0

 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WRITE_BYTE
 				     | I2C_FUNC_SMBUS_BYTE_DATA
 				     | I2C_FUNC_SMBUS_WORD_DATA))

@@ -1812,8 +1938,15 @@ 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_regulator_register(data);
+	if (ret)
+		goto out_unregister;
+
 	return 0;
 
+out_unregister:
+	hwmon_device_unregister(data->hwmon_dev);
 out_kfree:
 	kfree(data->group.attrs);
 	return ret;

include/linux/i2c/pmbus.h +4 -0

index 69280db..ee3c2ab 100644
@@ -40,6 +40,10 @@
 
 struct pmbus_platform_data {
 	u32 flags;		/* Device specific flags */
+
+	/* regulator support */
+	int num_regulators;
+	struct regulator_init_data *reg_init_data;
 };
 
 #endif /* _PMBUS_H_ */

Signature · 1 line

1.7.9.5

Quoted 15 lines · Guenter Roeck

Quoted 14 lines · On Wed, Sep 24, 2014 at 12:57:55PM -0500, [email protected] wrote:

From: Alan Tull <[email protected]>

Add support for simple on/off control of each channel.

To add regulator support, the pmbus part driver needs to add regulator_desc information, of_regulator_match information, and number of regulators to its pmbus_driver_info struct.

regulator_desc can be declared using default macro for a regulator (PMBUS_REGULATOR) that is in pmbus.h

The regulator_init_data can be intialized from either platform data or the device tree.

Quoted 7 lines

Quoted 2 lines

Signed-off-by Alan Tull <[email protected]>

Hi Alan,

Overall looks pretty good. Couple of comments inline.

Hi Guenter,

Quoted 30 lines

Quoted 30 lines

v2: Remove '#include <linux/regulator/machine.h>'
Only one regulator per pmbus device
Get regulator_init_data from pdata or device tree

v3: Support multiple regulators for each chip
Move most code to pmbus_core.c
fixed values for on/off

---
 drivers/hwmon/pmbus/pmbus.h      |   27 ++++++++
 drivers/hwmon/pmbus/pmbus_core.c |  133 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    4 ++
 3 files changed, 164 insertions(+)

drivers/hwmon/pmbus/pmbus.h +3 -0

index fa9beb3..74aa382 100644
@@ -19,6 +19,9 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
 #ifndef PMBUS_H
 #define PMBUS_H
 

@@ -186,6 +189,12 @@
 #define PMBUS_VIRT_STATUS_VMON		(PMBUS_VIRT_BASE + 35)
 

Quoted 8 lines

drivers/hwmon/pmbus/pmbus.h line 195

Quoted 5 lines

drivers/hwmon/pmbus/pmbus.h +4 -0

 /*
+ * OPERATION
+ */
+#define PB_OPERATION_CONTROL_ON		(1<<7)
+#define PB_OPERATION_CONTROL_SEQ_OFF	(1<<6)

Can those defines be more consistent ? Does it really need SEQ_OFF or can it just be OFF ?

PB_OPERATION_CONTROL_SEQ_OFF is not used, so I will eliminate it.

Quoted 17 lines

Quoted 17 lines

drivers/hwmon/pmbus/pmbus.h +7 -0

+
+/*
  * CAPABILITY
  */
 #define PB_CAPABILITY_SMBALERT		(1<<4)

@@ -365,8 +374,26 @@ 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;
+	const struct regulator_desc *reg_desc;
+	struct of_regulator_match *reg_matches;
 };
 

Quoted 7 lines

drivers/hwmon/pmbus/pmbus.h struct pmbus_driver_info { · line 387

Quoted 4 lines

drivers/hwmon/pmbus/pmbus.h +4 -0

+/* Regulator ops */
+
+extern struct regulator_ops pmbus_regulator_regulator_ops;
+

How about just pmbus_regulator_ops ? I don't see a double regulator_ variable name anywhere else in the code, and I don't really see the benefit of it.

That was a mistake. No need for double regulators here.

Quoted 12 lines

drivers/hwmon/pmbus/pmbus.h struct pmbus_driver_info { · line 396

Quoted 10 lines

drivers/hwmon/pmbus/pmbus.h +9 -0

+/* Macro for filling in array of struct regulator_desc */
+#define PMBUS_REGULATOR(_name, _id)				\
+	[_id] = {						\
+		.name = (_name # _id),				\
+		.id = (_id),					\
+		.ops = &pmbus_regulator_regulator_ops,		\
+		.owner = THIS_MODULE,				\
+	}
+

Any idea how/if we can get rid of the resulting checkpatch error ?

I banged my head on that for a while. I'll try some more.

Quoted 34 lines

Quoted 34 lines

drivers/hwmon/pmbus/pmbus.h +0 -0

 /* Function declarations */
 
 void pmbus_clear_cache(struct i2c_client *client);

drivers/hwmon/pmbus/pmbus_core.c +15 -0

index d6c3701..9ab8bd4 100644
@@ -29,6 +29,9 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/jiffies.h>
 #include <linux/i2c/pmbus.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include "pmbus.h"
 
 /*

@@ -1758,6 +1761,125 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int pmbus_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 = pmbus_read_byte_data(client, page, PMBUS_OPERATION);
+	if (ret < 0)
+		return ret;
+

Quoted 8 lines

drivers/hwmon/pmbus/pmbus_core.c static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, · line 1780

Quoted 5 lines

drivers/hwmon/pmbus/pmbus_core.c +5 -0

+	return !!(ret & PB_OPERATION_CONTROL_ON);
+}
+
+static int _pmbus_regulator_enable(struct regulator_dev *rdev, bool enable)
+{

Can you find a better name for this function ? After all, it doesn't just enable the regulator, it also disables it.

_pmbus_regulator_on_off?

Quoted 15 lines

drivers/hwmon/pmbus/pmbus_core.c static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, · line 1791

Quoted 12 lines

drivers/hwmon/pmbus/pmbus_core.c +11 -0

+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	u8 val, page = rdev_get_id(rdev);
+
+	if (enable)
+		val = PB_OPERATION_CONTROL_ON;
+	else
+		val = 0;
+
+	return pmbus_update_byte_data(client, page, PMBUS_OPERATION,
+				      PB_OPERATION_CONTROL_ON, val);

enable ? PB_OPERATION_CONTROL_ON : 0

Quoted 1 line

would be much simpler here.

OK

Quoted 20 lines

Quoted 7 lines

drivers/hwmon/pmbus/pmbus_core.c +6 -0

+}
+
+static int pmbus_regulator_enable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_enable(rdev, 1);
+}

drivers/hwmon/pmbus/pmbus_core.c static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data, · line 1807

Quoted 10 lines

drivers/hwmon/pmbus/pmbus_core.c +10 -0

+
+static int pmbus_regulator_disable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_enable(rdev, 0);
+}
+
+struct regulator_ops pmbus_regulator_regulator_ops = {
+	.enable = pmbus_regulator_enable,
+	.disable = pmbus_regulator_disable,
+	.is_enabled = pmbus_regulator_is_enabled,

No get_voltage support ?

Quoted 1 line

[ Guess it isn't mandatory. We can add it later to get this going. ]

Yep, no voltage support for now. But it will be straightforward for someone to insert here and probably won't require rewriting any of this.

Quoted 90 lines

Quoted 80 lines

drivers/hwmon/pmbus/pmbus_core.c +75 -0

+};
+EXPORT_SYMBOL_GPL(pmbus_regulator_regulator_ops);
+
+#if IS_ENABLED(CONFIG_OF)
+static int pmbus_regulator_parse_dt(struct device *dev,
+				    const struct pmbus_driver_info *info)
+{
+	struct device_node *np_regulators;
+	int ret;
+
+	if (!info->num_regulators)
+		return 0;
+
+	if (!info->reg_matches || !info->reg_desc)
+		return -EINVAL;
+
+	np_regulators = of_get_child_by_name(dev->of_node, "regulators");
+	if (!np_regulators)
+		return 0;
+
+	ret = of_regulator_match(dev, np_regulators, info->reg_matches,
+				 info->num_regulators);
+	of_node_put(np_regulators);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+#else
+static int pmbus_regulator_parse_dt(struct device *dev,
+				    const struct pmbus_driver_info *info)
+{
+	return 0;
+}
+#endif
+
+static int pmbus_regulator_register(struct pmbus_data *data)
+{
+	struct device *dev = data->dev;
+	const struct pmbus_driver_info *info = data->info;
+	const struct pmbus_platform_data *pdata = dev_get_platdata(dev);
+	struct regulator_dev *rdev;
+	int i;
+
+	for (i = 0; i < info->num_regulators; i++) {
+		struct regulator_config config = { };
+
+		config.dev = dev;
+		config.driver_data = data;
+
+		if (pdata && pdata->reg_init_data) {
+			config.init_data = &pdata->reg_init_data[i];
+		} else {
+			config.init_data = info->reg_matches[i].init_data;
+			config.of_node = info->reg_matches[i].of_node;
+		}
+
+		rdev = devm_regulator_register(dev, &info->reg_desc[i],
+					       &config);
+		if (IS_ERR(rdev)) {
+			dev_err(dev, "Failed to register %s regulator\n",
+				info->reg_desc[i].name);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}
+#else
+static int pmbus_regulator_register(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)
 {

drivers/hwmon/pmbus/pmbus_core.c int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id, · line 1897

Quoted 7 lines

drivers/hwmon/pmbus/pmbus_core.c +4 -0

@@ -1769,6 +1891,10 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 	if (!info)
 		return -ENODEV;
 
+	ret = pmbus_regulator_parse_dt(dev, info);
+	if (ret)
+		return ret;
+

You have the conditions wrong above.

Quoted 7 lines

If CONFIG_REGULATOR is not enabled, this will fail to build, since pmbus_regulator_parse_dt is not declared at all in this case.

I can understand that you want to parse the dt early, but it would be simpler to just parse it from pmbus_regulator_register(). It is only relevant if regulators are configured anyway, and we don't really need to optimize the code for the error case.

I was thinking of adding the flags to the device tree parsing code. That is the only other thing this driver is taking from the platform data. If I do that, this driver will be completely done for device tree. I could do
that by adding a 'pmbus-skip-status-check' device tree property. That
would be a small change, but I would still need to parse the dt early. Otherwise I can redo the code as you are recommending above.

What do you think?

From: Alan Tull <[email protected]>

Add simple on/off regulator support for ltc2978 and other pmbus parts supported by ltc2978.c

Signed-off-by Alan Tull <[email protected]>

v2: Remove '#include <linux/regulator/machine.h>'
Only one regulator per pmbus device
Get regulator_init_data from pdata or device tree

v3: Support multiple regulators for each chip
Move most code to pmbus_core.c
fixed values for on/off

---
 drivers/hwmon/pmbus/Kconfig   |    7 ++++++
 drivers/hwmon/pmbus/ltc2978.c |   51 +++++++++++++++++++++++++++++++++++++++++

 2 files changed, 58 insertions(+)

drivers/hwmon/pmbus/Kconfig +7 -0

index 6e1e493..79117b7 100644
@@ -56,6 +56,13 @@ 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"
+	depends on SENSORS_LTC2978 && 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 +2 -0

index e24ed52..7d4dcd7 100644
@@ -22,6 +22,8 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
 #include "pmbus.h"
 
 enum chips { ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676 };

@@ -374,6 +376,30 @@ static const struct i2c_device_id ltc2978_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ltc2978_id);
 

drivers/hwmon/pmbus/ltc2978.c +15 -0

+
 static int ltc2978_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {

@@ -487,6 +513,31 @@ static int ltc2978_probe(struct i2c_client *client,
 	default:
 		return -ENODEV;
 	}
+
+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+	info->reg_desc = ltc2978_reg_desc;
+	info->reg_matches = ltc2978_reg_matches;
+
+	switch (data->id) {
+	case ltc2974:
+		info->num_regulators = LTC2974_NUM_PAGES;
+		break;
+	case ltc2977:
+	case ltc2978:
+		info->num_regulators = LTC2978_NUM_PAGES;
+		break;
+	case ltc3880:

Quoted 11 lines

From: Alan Tull <[email protected]>

Add simple on/off regulator support for ltc2978 and other pmbus parts supported by ltc2978.c

Signed-off-by Alan Tull <[email protected]>

v2: Remove '#include <linux/regulator/machine.h>'
Only one regulator per pmbus device
Get regulator_init_data from pdata or device tree

Quoted 6 lines

v3: Support multiple regulators for each chip
Move most code to pmbus_core.c
fixed values for on/off

---
 drivers/hwmon/pmbus/Kconfig   |    7 ++++++
 drivers/hwmon/pmbus/ltc2978.c |   51 +++++++++++++++++++++++++++++++++++++++++

This will also require devicetree documentation describing the device nodes.

Quoted 38 lines

 2 files changed, 58 insertions(+)

drivers/hwmon/pmbus/Kconfig +7 -0

index 6e1e493..79117b7 100644
@@ -56,6 +56,13 @@ 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"
+	depends on SENSORS_LTC2978 && 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 +2 -0

index e24ed52..7d4dcd7 100644
@@ -22,6 +22,8 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
 #include "pmbus.h"
 
 enum chips { ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676 };

@@ -374,6 +376,30 @@ static const struct i2c_device_id ltc2978_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ltc2978_id);
 

drivers/hwmon/pmbus/ltc2978.c static const struct i2c_device_id ltc2978_id[] = { · line 388

Quoted 10 lines

drivers/hwmon/pmbus/ltc2978.c +10 -0

+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+static const struct regulator_desc ltc2978_reg_desc[] = {
+	PMBUS_REGULATOR("vout_en", 0),
+	PMBUS_REGULATOR("vout_en", 1),
+	PMBUS_REGULATOR("vout_en", 2),
+	PMBUS_REGULATOR("vout_en", 3),
+	PMBUS_REGULATOR("vout_en", 4),
+	PMBUS_REGULATOR("vout_en", 5),
+	PMBUS_REGULATOR("vout_en", 6),
+	PMBUS_REGULATOR("vout_en", 7),

How about just vout[0-7] ? I don't see a value in "_en".

drivers/hwmon/pmbus/ltc2978.c static const struct i2c_device_id ltc2978_id[] = { · line 399

Quoted 12 lines

drivers/hwmon/pmbus/ltc2978.c +11 -0

+};
+
+static struct of_regulator_match ltc2978_reg_matches[] = {
+	{ .name = "vout_en0" },
+	{ .name = "vout_en1" },
+	{ .name = "vout_en2" },
+	{ .name = "vout_en3" },
+	{ .name = "vout_en4" },
+	{ .name = "vout_en5" },
+	{ .name = "vout_en6" },
+	{ .name = "vout_en7" },

If there are multiple LTC chips in the system, this will result in duplicate regulator names. Does that matter ? Any ideas how other regulators handle this ?

Example on my test system:

root@localhost:/sys/class/regulator# grep vout_en0 */name
regulator.15/name:vout_en0
regulator.2/name:vout_en0
regulator.23/name:vout_en0
regulator.31/name:vout_en0
regulator.39/name:vout_en0
regulator.47/name:vout_en0

drivers/hwmon/pmbus/ltc2978.c static const struct i2c_device_id ltc2978_id[] = { · line 401

Quoted 3 lines

drivers/hwmon/pmbus/ltc2978.c +2 -0

+};
+#endif /* CONFIG_REGULATOR */

Nitpick, but

CONFIG_SENSORS_LTC2978_REGULATOR

Quoted 30 lines

drivers/hwmon/pmbus/ltc2978.c +23 -0

+
 static int ltc2978_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {

@@ -487,6 +513,31 @@ static int ltc2978_probe(struct i2c_client *client,
 	default:
 		return -ENODEV;
 	}
+
+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+	info->reg_desc = ltc2978_reg_desc;
+	info->reg_matches = ltc2978_reg_matches;
+
+	switch (data->id) {
+	case ltc2974:
+		info->num_regulators = LTC2974_NUM_PAGES;
+		break;
+	case ltc2977:
+	case ltc2978:
+		info->num_regulators = LTC2978_NUM_PAGES;
+		break;
+	case ltc3880:
+	case ltm4676:
+		info->num_regulators = LTC3880_NUM_PAGES;
+		break;
+	case ltc3883:
+		info->num_regulators = LTC3883_NUM_PAGES;
+		break;
+	default:
+		return -ENODEV;

drivers/hwmon/pmbus/ltc2978.c static int ltc2978_probe(struct i2c_client *client, · line 539

Quoted 2 lines

drivers/hwmon/pmbus/ltc2978.c +2 -0

+	}
+	BUG_ON(info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc));

How about an error message and reducing info->num_regulators to ARRAY_SIZE(ltc2978_reg_desc) if that happens ? I am not really a friend of BUG_ON() as it seems a bit drastic. Sure, one can argue that the programmer doesn't deserve better, but the idea behind BUG_ON is that the kernel can not continue to operate, and that is not really the case here.

Also, please drop the ifdef here, and merge the initialization into the first switch statement. The few saved bytes of code are not really worth it. You can use defines for ltc2978_reg_desc and ltc2978_reg_matches and initialize with NULL if CONFIG_SENSORS_LTC2978_REGULATOR is not defined.

Thanks,
Guenter

On Wed, 24 Sep 2014, Guenter Roeck wrote:

Hi Guenter,

Quoted 19 lines · On Wed, Sep 24, 2014 at 12:57:56PM -0500, [email protected] wrote:

Quoted 17 lines

From: Alan Tull <[email protected]>

Add simple on/off regulator support for ltc2978 and other pmbus parts supported by ltc2978.c

Signed-off-by Alan Tull <[email protected]>

v2: Remove '#include <linux/regulator/machine.h>'
Only one regulator per pmbus device
Get regulator_init_data from pdata or device tree

v3: Support multiple regulators for each chip
Move most code to pmbus_core.c
fixed values for on/off

---
 drivers/hwmon/pmbus/Kconfig   |    7 ++++++
 drivers/hwmon/pmbus/ltc2978.c |   51 +++++++++++++++++++++++++++++++++++++++++

Quoted 1 line

This will also require devicetree documentation describing the device nodes.

Yes, I'll add that as a separate patch to v4. It will be a new file since there currently isn't any pmbus or ltc2978 bindings documentation that I could find.

Quoted 38 lines

Quoted 38 lines

 2 files changed, 58 insertions(+)

drivers/hwmon/pmbus/Kconfig +7 -0

index 6e1e493..79117b7 100644
@@ -56,6 +56,13 @@ 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"
+	depends on SENSORS_LTC2978 && 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 +2 -0

index e24ed52..7d4dcd7 100644
@@ -22,6 +22,8 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
 #include "pmbus.h"
 
 enum chips { ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676 };

@@ -374,6 +376,30 @@ static const struct i2c_device_id ltc2978_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ltc2978_id);
 

Quoted 12 lines

drivers/hwmon/pmbus/ltc2978.c static const struct i2c_device_id ltc2978_id[] = { · line 388

Quoted 10 lines

drivers/hwmon/pmbus/ltc2978.c +10 -0

+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+static const struct regulator_desc ltc2978_reg_desc[] = {
+	PMBUS_REGULATOR("vout_en", 0),
+	PMBUS_REGULATOR("vout_en", 1),
+	PMBUS_REGULATOR("vout_en", 2),
+	PMBUS_REGULATOR("vout_en", 3),
+	PMBUS_REGULATOR("vout_en", 4),
+	PMBUS_REGULATOR("vout_en", 5),
+	PMBUS_REGULATOR("vout_en", 6),
+	PMBUS_REGULATOR("vout_en", 7),

How about just vout[0-7] ? I don't see a value in "_en".

That's cool. I'll do it.

Quoted 18 lines

drivers/hwmon/pmbus/ltc2978.c static const struct i2c_device_id ltc2978_id[] = { · line 399

Quoted 12 lines

drivers/hwmon/pmbus/ltc2978.c +11 -0

+};
+
+static struct of_regulator_match ltc2978_reg_matches[] = {
+	{ .name = "vout_en0" },
+	{ .name = "vout_en1" },
+	{ .name = "vout_en2" },
+	{ .name = "vout_en3" },
+	{ .name = "vout_en4" },
+	{ .name = "vout_en5" },
+	{ .name = "vout_en6" },
+	{ .name = "vout_en7" },

If there are multiple LTC chips in the system, this will result in duplicate regulator names. Does that matter ? Any ideas how other regulators handle this ?

Example on my test system:

Quoted 7 lines

root@localhost:/sys/class/regulator# grep vout_en0 */name
regulator.15/name:vout_en0
regulator.2/name:vout_en0
regulator.23/name:vout_en0
regulator.31/name:vout_en0
regulator.39/name:vout_en0
regulator.47/name:vout_en0

These are just default names, but I think I could make the name better. How about <part #>-<i2c address>-vout<#> such as "ltc2978-5c-vout0"

Quoted 4 lines · atull

Quoted 1 line

regulator.47/name:vout_en0

These are just default names, but I think I could make the name better. How about <part #>-<i2c address>-vout<#> such as "ltc2978-5c-vout0"

Problem with that is that i2c bus numbers are not static, which would make it quite difficult to describe the regulator in devicetree data.

Any idea what other regulators do in such situations ?

Quoted 6 lines · Guenter Roeck

Quoted 4 lines · atull

Quoted 1 line

regulator.47/name:vout_en0

These are just default names, but I think I could make the name better. How about <part #>-<i2c address>-vout<#> such as "ltc2978-5c-vout0"

Quoted 4 lines

Problem with that is that i2c bus numbers are not static, which would make it quite difficult to describe the regulator in devicetree data.

Any idea what other regulators do in such situations ?

What's there is correct (modulo the _en), the names are local to the chip. If the user actually cares they will be providing the name of the supply on the board anyway.