[PATCH v3 0/3] regulator support for pmbus and ltc2978

[PATCH v3 0/3] regulator support for pmbus and ltc2978

[atull]

From: Alan Tull <atull@opensource.altera.com>

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

Each output has individual on/off control.

>From PMBus_Specification_Part_II_Rev_1-3_20140318.pdf:
12.1.1. OPERATION Command Bit [7]
  Bit [7] controls whether the PMBus device output is on or off.
   If bit [7] is cleared (equals 0), then the output is off.
   If bit [7] is set (equals 1), then the output is on.

Patch 1: add two helper functions for byte pmbus byte operations
  * byte write and byte read/modify/write

Patch 2: changes for pmbus_core.c and pmbus.h
  * regulator_ops functions (is_enabled, enable, and disable)
  * gets regulator init data from device tree or platform data
  * registers the regulators
  * header has a macro for chip drivers to build their
    regulator_desc data

Patch 3: changes for ltc2978.c
  * Add Kconfig to enable/disable ltc2978 regulator functionality
  * add regulator_desc and of_regulator_match info
  * use same structs for all parts; set num_regulators appropriately.

Alan Tull (3):
  pmbus: core: add helpers for byte write and read modify write
  pmbus: add regulator support
  pmbus: ltc2978: add regulator support

 drivers/hwmon/pmbus/Kconfig      |    7 ++
 drivers/hwmon/pmbus/ltc2978.c    |   51 ++++++++++++
 drivers/hwmon/pmbus/pmbus.h      |   27 +++++++
 drivers/hwmon/pmbus/pmbus_core.c |  164 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    4 +
 5 files changed, 253 insertions(+)

-- 
1.7.9.5

[PATCH v3 1/3] pmbus: core: add helpers for byte write and read modify write

[atull]

From: Alan Tull <atull@opensource.altera.com>

Add two helper functions:
 * pmbus_write_byte_data  = paged byte write
 * pmbus_update_byte_data = paged byte read/modify/write

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 drivers/hwmon/pmbus/pmbus_core.c |   31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 291d11f..d6c3701 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -253,6 +253,37 @@ int pmbus_read_byte_data(struct i2c_client *client, int page, u8 reg)
 }
 EXPORT_SYMBOL_GPL(pmbus_read_byte_data);
 
+int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value)
+{
+	int rv;
+
+	rv = pmbus_set_page(client, page);
+	if (rv < 0)
+		return rv;
+
+	return i2c_smbus_write_byte_data(client, reg, value);
+}
+EXPORT_SYMBOL_GPL(pmbus_write_byte_data);
+
+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_GPL(pmbus_update_byte_data);
+
 /*
  * _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.
-- 
1.7.9.5

[PATCH v3 2/3] pmbus: add regulator support

[atull]

From: Alan Tull <atull@opensource.altera.com>

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 <atull@opensource.altera.com>

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(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index fa9beb3..74aa382 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -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);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index d6c3701..9ab8bd4 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -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;
+
+	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)
 {
@@ -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;
+
 	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;
diff --git a/include/linux/i2c/pmbus.h b/include/linux/i2c/pmbus.h
index 69280db..ee3c2ab 100644
--- a/include/linux/i2c/pmbus.h
+++ b/include/linux/i2c/pmbus.h
@@ -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_ */
-- 
1.7.9.5

Re: [PATCH v3 2/3] pmbus: add regulator support

[Dinh Nguyen]

Hi Alan,

On 9/24/14, 12:57 PM, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> 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 <atull@opensource.altera.com>
> 
> 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(+)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index fa9beb3..74aa382 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -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);
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index d6c3701..9ab8bd4 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -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;
> +
> +	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

Re: [PATCH v3 2/3] pmbus: add regulator support

[atull]

[-- Attachment #1: Type: text/plain, Size: 6164 bytes --]

On Wed, 24 Sep 2014, Dinh Nguyen wrote:

> Hi Alan,
> 
> On 9/24/14, 12:57 PM, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > 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 <atull@opensource.altera.com>
> > 
> > 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(+)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index fa9beb3..74aa382 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -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);
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index d6c3701..9ab8bd4 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -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;
> > +
> > +	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
> 

Hi Dinh,

Thanks, I will fix it.  I had that working earlier, but obviously broke it again...

Alan

Re: [PATCH v3 2/3] pmbus: add regulator support

[Guenter Roeck]

On Wed, Sep 24, 2014 at 12:57:55PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> 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 <atull@opensource.altera.com>
> 

Hi Alan,

Overall looks pretty good. Couple of comments inline.

> 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(+)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index fa9beb3..74aa382 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -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)

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

> +
> +/*
>   * 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;
> +
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.

> +/* 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 ?

>  /* Function declarations */
>  
>  void pmbus_clear_cache(struct i2c_client *client);
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index d6c3701..9ab8bd4 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -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)
> +{

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

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

> +}
> +
> +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,

No get_voltage support ?

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

> +};
> +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)
>  {
> @@ -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.

>  	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;
> diff --git a/include/linux/i2c/pmbus.h b/include/linux/i2c/pmbus.h
> index 69280db..ee3c2ab 100644
> --- a/include/linux/i2c/pmbus.h
> +++ b/include/linux/i2c/pmbus.h
> @@ -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_ */
> -- 
> 1.7.9.5
> 

Re: [PATCH v3 2/3] pmbus: add regulator support

[atull]

On Wed, 24 Sep 2014, Guenter Roeck wrote:

> On Wed, Sep 24, 2014 at 12:57:55PM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > 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 <atull@opensource.altera.com>
> > 
> 
> Hi Alan,
> 
> Overall looks pretty good. Couple of comments inline.
> 

Hi Guenter,

> > 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(+)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index fa9beb3..74aa382 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -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)
> 
> 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.

> 
> > +
> > +/*
> >   * 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;
> > +
> 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.

> 
> > +/* 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.

> 
> >  /* Function declarations */
> >  
> >  void pmbus_clear_cache(struct i2c_client *client);
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index d6c3701..9ab8bd4 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -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)
> > +{
> 
> 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?

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

OK

> 
> > +}
> > +
> > +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,
> 
> No get_voltage support ?
> 
> [ 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.

> 
> > +};
> > +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)
> >  {
> > @@ -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.

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? 

Thanks for the review,

Alan

> 
> >  	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;
> > diff --git a/include/linux/i2c/pmbus.h b/include/linux/i2c/pmbus.h
> > index 69280db..ee3c2ab 100644
> > --- a/include/linux/i2c/pmbus.h
> > +++ b/include/linux/i2c/pmbus.h
> > @@ -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_ */
> > -- 
> > 1.7.9.5
> > 
> 

Re: [PATCH v3 2/3] pmbus: add regulator support

[Guenter Roeck]

On Wed, Sep 24, 2014 at 04:06:11PM -0500, atull wrote:
> On Wed, 24 Sep 2014, Guenter Roeck wrote:
> 
> > On Wed, Sep 24, 2014 at 12:57:55PM -0500, atull@opensource.altera.com wrote:
> > > From: Alan Tull <atull@opensource.altera.com>
> > > 
> > > 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 <atull@opensource.altera.com>
> > > 
> > 
> > Hi Alan,
> > 
> > Overall looks pretty good. Couple of comments inline.
> > 
> 
> Hi Guenter,
> 
> > > 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(+)
> > > 
> > > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > > index fa9beb3..74aa382 100644
> > > --- a/drivers/hwmon/pmbus/pmbus.h
> > > +++ b/drivers/hwmon/pmbus/pmbus.h
> > > @@ -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)
> > 
> > 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.
> 
Even better ;)

> > 
> > > +
> > > +/*
> > >   * 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;
> > > +
> > 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.
> 
> > 
> > > +/* 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.
> 
Don't spend too much time on it. I'll accept it either way.

> > 
> > >  /* Function declarations */
> > >  
> > >  void pmbus_clear_cache(struct i2c_client *client);
> > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > > index d6c3701..9ab8bd4 100644
> > > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > > @@ -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)
> > > +{
> > 
> > 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?
> 
Ok.

> > 
> > > +	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.
> 
> OK
> 
> > 
> > > +}
> > > +
> > > +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,
> > 
> > No get_voltage support ?
> > 
> > [ 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.
> 
That is true.

> > 
> > > +};
> > > +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)
> > >  {
> > > @@ -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.
> 
> 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.
> 
Guess we can do that if/when it is needed. So far there is only one flag bit,
and that isn't widely needed. I am not even sure if it is needed anymore in the
first place - I'll have to go back to my notes to find out which chips actually
need it. We may have other means today to accomplish the same, via an explicit
chip driver.

> What do you think? 
> 
I'd keep it simple for now, and only parse reglator data.

Guenter

[PATCH v3 3/3] pmbus: ltc2978: add regulator support

[atull]

From: Alan Tull <atull@opensource.altera.com>

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

Signed-off-by: Alan Tull <atull@opensource.altera.com>

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(+)

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 6e1e493..79117b7 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -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
diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index e24ed52..7d4dcd7 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -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);
 
+#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),
+};
+
+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" },
+};
+#endif /* CONFIG_REGULATOR */
+
 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;
+	}
+	BUG_ON(info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc));
+#endif
 	return pmbus_do_probe(client, id, info);
 }
 
-- 
1.7.9.5

Re: [PATCH v3 3/3] pmbus: ltc2978: add regulator support

[Guenter Roeck]

On Wed, Sep 24, 2014 at 12:57:56PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add simple on/off regulator support for ltc2978 and
> other pmbus parts supported by ltc2978.c
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> 
> 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 +++++++++++++++++++++++++++++++++++++++++

This will also require devicetree documentation describing the device nodes.

>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 6e1e493..79117b7 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -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
> diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> index e24ed52..7d4dcd7 100644
> --- a/drivers/hwmon/pmbus/ltc2978.c
> +++ b/drivers/hwmon/pmbus/ltc2978.c
> @@ -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);
>  
> +#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".

> +};
> +
> +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

> +};
> +#endif /* CONFIG_REGULATOR */

Nitpick, but

	CONFIG_SENSORS_LTC2978_REGULATOR
> +
>  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;
> +	}
> +	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

Re: [PATCH v3 3/3] pmbus: ltc2978: add regulator support

[atull]

On Wed, 24 Sep 2014, Guenter Roeck wrote:

Hi Guenter,

> On Wed, Sep 24, 2014 at 12:57:56PM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > Add simple on/off regulator support for ltc2978 and
> > other pmbus parts supported by ltc2978.c
> > 
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > 
> > 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 +++++++++++++++++++++++++++++++++++++++++
> 
> 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.

> 
> >  2 files changed, 58 insertions(+)
> > 
> > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > index 6e1e493..79117b7 100644
> > --- a/drivers/hwmon/pmbus/Kconfig
> > +++ b/drivers/hwmon/pmbus/Kconfig
> > @@ -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
> > diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> > index e24ed52..7d4dcd7 100644
> > --- a/drivers/hwmon/pmbus/ltc2978.c
> > +++ b/drivers/hwmon/pmbus/ltc2978.c
> > @@ -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);
> >  
> > +#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.

> 
> > +};
> > +
> > +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

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"

If the board has regulator_init_data, then these default names get overwritten.
In my case, I'm just using 3 supplies so those 3 get overwritten:

root@socfpga_cyclone5:/sys/class/regulator# cat */name
regulator-dummy
FPGA-2.5V
vout_en1
FPGA-1.5V
vout_en3
FPGA-1.1V
vout_en5
vout_en6
vout_en7

> 
> > +};
> > +#endif /* CONFIG_REGULATOR */
> 
> Nitpick, but
> 
> 	CONFIG_SENSORS_LTC2978_REGULATOR

I'll change it.

> > +
> >  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;
> > +	}
> > +	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.

That sounds right to me.  I'll do that.

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

OK, that will be cleaner.

> 
> Thanks,
> Guenter
> 

Thanks for the feedback,
Alan

Re: [PATCH v3 3/3] pmbus: ltc2978: add regulator support

[Guenter Roeck]

On Wed, Sep 24, 2014 at 03:48:40PM -0500, atull wrote:
> On Wed, 24 Sep 2014, Guenter Roeck wrote:
> 
> Hi Guenter,
> 
> > On Wed, Sep 24, 2014 at 12:57:56PM -0500, atull@opensource.altera.com wrote:
> > > From: Alan Tull <atull@opensource.altera.com>
> > > 
> > > Add simple on/off regulator support for ltc2978 and
> > > other pmbus parts supported by ltc2978.c
> > > 
> > > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > > 
> > > 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 +++++++++++++++++++++++++++++++++++++++++
> > 
> > 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.
> 

Hi Alan,

It wasn't needed so far... just describe the bindings for ltc2978.

> > 
> > >  2 files changed, 58 insertions(+)
> > > 
> > > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> > > index 6e1e493..79117b7 100644
> > > --- a/drivers/hwmon/pmbus/Kconfig
> > > +++ b/drivers/hwmon/pmbus/Kconfig
> > > @@ -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
> > > diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
> > > index e24ed52..7d4dcd7 100644
> > > --- a/drivers/hwmon/pmbus/ltc2978.c
> > > +++ b/drivers/hwmon/pmbus/ltc2978.c
> > > @@ -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);
> > >  
> > > +#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.
> 
> > 
> > > +};
> > > +
> > > +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
> 
> 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 ?

Guenter

Re: [PATCH v3 3/3] pmbus: ltc2978: add regulator support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 677 bytes --]

On Wed, Sep 24, 2014 at 02:13:32PM -0700, Guenter Roeck wrote:
> On Wed, Sep 24, 2014 at 03:48:40PM -0500, atull wrote:

> > > 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 ?

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3 3/3] pmbus: ltc2978: add regulator support

[Guenter Roeck]

On 09/24/2014 03:33 PM, Mark Brown wrote:
> On Wed, Sep 24, 2014 at 02:13:32PM -0700, Guenter Roeck wrote:
>> On Wed, Sep 24, 2014 at 03:48:40PM -0500, atull wrote:
>
>>>> 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 ?
>
> 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.
>

Great, thanks a lot.

Guenter