[lm-sensors] [PATCH 0/2] add regulator support for pmbus and ltc2978

[lm-sensors] [PATCH 0/2] add regulator support for pmbus and ltc2978

[atull]

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

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

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

Current ltc2978 support is limited to enable/disable.

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

Thanks,
Alan

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

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

-- 
1.7.9.5


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH 0/2] add regulator support for pmbus and ltc2978

[atull]

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

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

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

Current ltc2978 support is limited to enable/disable.

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

Thanks,
Alan

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

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

-- 
1.7.9.5

[lm-sensors] [PATCH 1/2] pmbus: add regulator support

[atull]

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

Enable pmbus device drivers to add regulator functionality.

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

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

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 drivers/hwmon/pmbus/pmbus.h      |    7 +++++++
 drivers/hwmon/pmbus/pmbus_core.c |   40 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index fa9beb3..a45722f 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -19,6 +19,8 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/regulator/driver.h>
+
 #ifndef PMBUS_H
 #define PMBUS_H
 
@@ -365,6 +367,11 @@ struct pmbus_driver_info {
 	 */
 	int (*identify)(struct i2c_client *client,
 			struct pmbus_driver_info *info);
+
+	/* Regulator functionality, if supported by this chip driver. */
+	int num_regulators;
+	struct regulator_init_data *reg_init;
+	const struct regulator_desc *reg_desc;
 };
 
 /* Function declarations */
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 291d11f..10315ee 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -29,6 +29,8 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/jiffies.h>
 #include <linux/i2c/pmbus.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include "pmbus.h"
 
 /*
@@ -1727,6 +1729,39 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int pmbus_register_regulators(struct pmbus_data *data)
+{
+	struct regulator_dev *rdev;
+	struct regulator_config config = { };
+	const struct pmbus_driver_info *info = data->info;
+	struct device *dev = data->dev;
+	int i;
+
+	config.dev = dev;
+	config.driver_data = data;
+
+	for (i = 0; i < info->num_regulators; i++) {
+		config.init_data = &info->reg_init[i];
+		rdev = devm_regulator_register(dev, &info->reg_desc[i],
+					       &config);
+		if (IS_ERR(rdev)) {
+			dev_err(dev, "failed to register regulator %s\n",
+				info->reg_desc[i].name);
+			hwmon_device_unregister(data->hwmon_dev);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}
+#else
+static int pmbus_register_regulators(struct pmbus_data *data)
+{
+	return 0;
+}
+#endif
+
 int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		   struct pmbus_driver_info *info)
 {
@@ -1781,6 +1816,11 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		dev_err(dev, "Failed to register hwmon device\n");
 		goto out_kfree;
 	}
+
+	ret = pmbus_register_regulators(data);
+	if (ret)
+		goto out_kfree;
+
 	return 0;
 
 out_kfree:
-- 
1.7.9.5


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH 1/2] pmbus: add regulator support

[atull]

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

Enable pmbus device drivers to add regulator functionality.

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

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

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 drivers/hwmon/pmbus/pmbus.h      |    7 +++++++
 drivers/hwmon/pmbus/pmbus_core.c |   40 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index fa9beb3..a45722f 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -19,6 +19,8 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/regulator/driver.h>
+
 #ifndef PMBUS_H
 #define PMBUS_H
 
@@ -365,6 +367,11 @@ struct pmbus_driver_info {
 	 */
 	int (*identify)(struct i2c_client *client,
 			struct pmbus_driver_info *info);
+
+	/* Regulator functionality, if supported by this chip driver. */
+	int num_regulators;
+	struct regulator_init_data *reg_init;
+	const struct regulator_desc *reg_desc;
 };
 
 /* Function declarations */
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 291d11f..10315ee 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -29,6 +29,8 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/jiffies.h>
 #include <linux/i2c/pmbus.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include "pmbus.h"
 
 /*
@@ -1727,6 +1729,39 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int pmbus_register_regulators(struct pmbus_data *data)
+{
+	struct regulator_dev *rdev;
+	struct regulator_config config = { };
+	const struct pmbus_driver_info *info = data->info;
+	struct device *dev = data->dev;
+	int i;
+
+	config.dev = dev;
+	config.driver_data = data;
+
+	for (i = 0; i < info->num_regulators; i++) {
+		config.init_data = &info->reg_init[i];
+		rdev = devm_regulator_register(dev, &info->reg_desc[i],
+					       &config);
+		if (IS_ERR(rdev)) {
+			dev_err(dev, "failed to register regulator %s\n",
+				info->reg_desc[i].name);
+			hwmon_device_unregister(data->hwmon_dev);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}
+#else
+static int pmbus_register_regulators(struct pmbus_data *data)
+{
+	return 0;
+}
+#endif
+
 int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		   struct pmbus_driver_info *info)
 {
@@ -1781,6 +1816,11 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		dev_err(dev, "Failed to register hwmon device\n");
 		goto out_kfree;
 	}
+
+	ret = pmbus_register_regulators(data);
+	if (ret)
+		goto out_kfree;
+
 	return 0;
 
 out_kfree:
-- 
1.7.9.5

[lm-sensors] [PATCH 2/2] pmbus: ltc2978: add regulator gating

[atull]

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

Add support for enabling or disabling all supplies.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 drivers/hwmon/pmbus/Kconfig   |    9 ++++++
 drivers/hwmon/pmbus/ltc2978.c |   69 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 6e1e493..c0d98f2 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -56,6 +56,15 @@ config SENSORS_LTC2978
 	  This driver can also be built as a module. If so, the module will
 	  be called ltc2978.
 
+config SENSORS_LTC2978_REGULATOR
+	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
+	default n
+	depends on SENSORS_LTC2978
+	select REGULATOR
+	help
+	  If you say yes here you get regulator support for Linear
+	  Technology LTC2974, LTC2978, LTC3880, and LTC3883.
+
 config SENSORS_MAX16064
 	tristate "Maxim MAX16064"
 	default n
diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index e24ed52..1e25346 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -20,6 +20,8 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/err.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include "pmbus.h"
@@ -151,6 +153,66 @@ static int ltc2978_read_word_data_common(struct i2c_client *client, int page,
 	return ret;
 }
 
+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+static int ltc2978_write_pmbus_operation(struct regulator_dev *rdev, u8 value)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret;
+
+	ret = pmbus_set_page(client, 0xff);
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_write_byte_data(client, PMBUS_OPERATION, value);
+}
+
+static int ltc2978_enable_all(struct regulator_dev *rdev)
+{
+	return ltc2978_write_pmbus_operation(rdev, 0x80);
+}
+
+static int ltc2978_disable_all(struct regulator_dev *rdev)
+{
+	return ltc2978_write_pmbus_operation(rdev, 0);
+}
+
+static int ltc2978_is_enabled(struct regulator_dev *rdev)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret;
+
+	ret = pmbus_set_page(client, 0xff);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_read_byte_data(client, PMBUS_OPERATION);
+	if (ret < 0)
+		return ret;
+
+	return !!(ret & 0x80);
+}
+
+static struct regulator_ops ltc2978_regulator_ops = {
+	.enable = ltc2978_enable_all,
+	.disable = ltc2978_disable_all,
+	.is_enabled = ltc2978_is_enabled,
+};
+
+static const struct regulator_desc ltc2978_reg_desc = {
+	.name = "ltc2978",
+	.ops = &ltc2978_regulator_ops,
+	.owner = THIS_MODULE,
+};
+
+static struct regulator_init_data ltc2978_regulator_init = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+};
+#endif
+
 static int ltc2978_read_word_data(struct i2c_client *client, int page, int reg)
 {
 	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
@@ -487,6 +549,13 @@ static int ltc2978_probe(struct i2c_client *client,
 	default:
 		return -ENODEV;
 	}
+
+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+	info->num_regulators = 1;
+	info->reg_desc = &ltc2978_reg_desc;
+	info->reg_init = &ltc2978_regulator_init;
+#endif
+
 	return pmbus_do_probe(client, id, info);
 }
 
-- 
1.7.9.5


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[atull]

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

Add support for enabling or disabling all supplies.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 drivers/hwmon/pmbus/Kconfig   |    9 ++++++
 drivers/hwmon/pmbus/ltc2978.c |   69 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 6e1e493..c0d98f2 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -56,6 +56,15 @@ config SENSORS_LTC2978
 	  This driver can also be built as a module. If so, the module will
 	  be called ltc2978.
 
+config SENSORS_LTC2978_REGULATOR
+	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
+	default n
+	depends on SENSORS_LTC2978
+	select REGULATOR
+	help
+	  If you say yes here you get regulator support for Linear
+	  Technology LTC2974, LTC2978, LTC3880, and LTC3883.
+
 config SENSORS_MAX16064
 	tristate "Maxim MAX16064"
 	default n
diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index e24ed52..1e25346 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -20,6 +20,8 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/err.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include "pmbus.h"
@@ -151,6 +153,66 @@ static int ltc2978_read_word_data_common(struct i2c_client *client, int page,
 	return ret;
 }
 
+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+static int ltc2978_write_pmbus_operation(struct regulator_dev *rdev, u8 value)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret;
+
+	ret = pmbus_set_page(client, 0xff);
+	if (ret < 0)
+		return ret;
+
+	return i2c_smbus_write_byte_data(client, PMBUS_OPERATION, value);
+}
+
+static int ltc2978_enable_all(struct regulator_dev *rdev)
+{
+	return ltc2978_write_pmbus_operation(rdev, 0x80);
+}
+
+static int ltc2978_disable_all(struct regulator_dev *rdev)
+{
+	return ltc2978_write_pmbus_operation(rdev, 0);
+}
+
+static int ltc2978_is_enabled(struct regulator_dev *rdev)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret;
+
+	ret = pmbus_set_page(client, 0xff);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_read_byte_data(client, PMBUS_OPERATION);
+	if (ret < 0)
+		return ret;
+
+	return !!(ret & 0x80);
+}
+
+static struct regulator_ops ltc2978_regulator_ops = {
+	.enable = ltc2978_enable_all,
+	.disable = ltc2978_disable_all,
+	.is_enabled = ltc2978_is_enabled,
+};
+
+static const struct regulator_desc ltc2978_reg_desc = {
+	.name = "ltc2978",
+	.ops = &ltc2978_regulator_ops,
+	.owner = THIS_MODULE,
+};
+
+static struct regulator_init_data ltc2978_regulator_init = {
+	.constraints = {
+		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
+	},
+};
+#endif
+
 static int ltc2978_read_word_data(struct i2c_client *client, int page, int reg)
 {
 	const struct pmbus_driver_info *info = pmbus_get_driver_info(client);
@@ -487,6 +549,13 @@ static int ltc2978_probe(struct i2c_client *client,
 	default:
 		return -ENODEV;
 	}
+
+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+	info->num_regulators = 1;
+	info->reg_desc = &ltc2978_reg_desc;
+	info->reg_init = &ltc2978_regulator_init;
+#endif
+
 	return pmbus_do_probe(client, id, info);
 }
 
-- 
1.7.9.5

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1684 bytes --]

On Thu, Aug 21, 2014 at 05:21:26PM -0500, atull@opensource.altera.com wrote:

> +config SENSORS_LTC2978_REGULATOR
> +	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
> +	default n

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

> +	depends on SENSORS_LTC2978
> +	select REGULATOR

I'd expect a depends here.

> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>

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

> +static int ltc2978_write_pmbus_operation(struct regulator_dev *rdev, u8 value)
> +{
> +	struct device *dev = rdev_get_dev(rdev);
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	int ret;
> +
> +	ret = pmbus_set_page(client, 0xff);
> +	if (ret < 0)
> +		return ret;
> +
> +	return i2c_smbus_write_byte_data(client, PMBUS_OPERATION, value);
> +}

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

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

> +static struct regulator_init_data ltc2978_regulator_init = {
> +	.constraints = {
> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> +	},
> +};

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Mark Brown]

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

On Thu, Aug 21, 2014 at 05:21:26PM -0500, atull@opensource.altera.com wrote:

> +config SENSORS_LTC2978_REGULATOR
> +	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
> +	default n

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

> +	depends on SENSORS_LTC2978
> +	select REGULATOR

I'd expect a depends here.

> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>

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

> +static int ltc2978_write_pmbus_operation(struct regulator_dev *rdev, u8 value)
> +{
> +	struct device *dev = rdev_get_dev(rdev);
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	int ret;
> +
> +	ret = pmbus_set_page(client, 0xff);
> +	if (ret < 0)
> +		return ret;
> +
> +	return i2c_smbus_write_byte_data(client, PMBUS_OPERATION, value);
> +}

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

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

> +static struct regulator_init_data ltc2978_regulator_init = {
> +	.constraints = {
> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> +	},
> +};

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

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

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

[Guenter Roeck]

On Thu, Aug 21, 2014 at 07:36:50PM -0500, Mark Brown wrote:
> On Thu, Aug 21, 2014 at 05:21:26PM -0500, atull@opensource.altera.com wrote:
> 
> > +config SENSORS_LTC2978_REGULATOR
> > +	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
> > +	default n
> 
> No need to say default n here, it's the default default.
> 
> > +	depends on SENSORS_LTC2978
> > +	select REGULATOR
> 
> I'd expect a depends here.
> 
> > +#include <linux/regulator/driver.h>
> > +#include <linux/regulator/machine.h>
> 
> If you need machine.h that's suspicious...  why do you need it?
> 
> > +static int ltc2978_write_pmbus_operation(struct regulator_dev *rdev, u8 value)
> > +{
> > +	struct device *dev = rdev_get_dev(rdev);
> > +	struct i2c_client *client = to_i2c_client(dev->parent);
> > +	int ret;
> > +
> > +	ret = pmbus_set_page(client, 0xff);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return i2c_smbus_write_byte_data(client, PMBUS_OPERATION, value);
> > +}
> 
> This all looks very much like pmbus could use regmap and then the regmap
> helpers.  I'd not insist on it though.  What I would however suggest is

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

Guenter

> that these functions should all be helpers which read the specific
> page, addresses and bits to write from the driver structure - I bet the
> code is going to be identical for most pmbus using regulators and so it
> makes sense to share it like we do with the generic regmap functions.
> 
> That means that any good practice can be deployed more easily and any
> API updates only need to update the helpers.
> 
> > +static struct regulator_init_data ltc2978_regulator_init = {
> > +	.constraints = {
> > +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> > +	},
> > +};
> 
> You should not be forcing this on, you don't know what's safe on any
> given board.  Allow the board to specify constraints then it has
> control.



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Guenter Roeck]

On Thu, Aug 21, 2014 at 07:36:50PM -0500, Mark Brown wrote:
> On Thu, Aug 21, 2014 at 05:21:26PM -0500, atull@opensource.altera.com wrote:
> 
> > +config SENSORS_LTC2978_REGULATOR
> > +	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
> > +	default n
> 
> No need to say default n here, it's the default default.
> 
> > +	depends on SENSORS_LTC2978
> > +	select REGULATOR
> 
> I'd expect a depends here.
> 
> > +#include <linux/regulator/driver.h>
> > +#include <linux/regulator/machine.h>
> 
> If you need machine.h that's suspicious...  why do you need it?
> 
> > +static int ltc2978_write_pmbus_operation(struct regulator_dev *rdev, u8 value)
> > +{
> > +	struct device *dev = rdev_get_dev(rdev);
> > +	struct i2c_client *client = to_i2c_client(dev->parent);
> > +	int ret;
> > +
> > +	ret = pmbus_set_page(client, 0xff);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return i2c_smbus_write_byte_data(client, PMBUS_OPERATION, value);
> > +}
> 
> This all looks very much like pmbus could use regmap and then the regmap
> helpers.  I'd not insist on it though.  What I would however suggest is

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

Guenter

> that these functions should all be helpers which read the specific
> page, addresses and bits to write from the driver structure - I bet the
> code is going to be identical for most pmbus using regulators and so it
> makes sense to share it like we do with the generic regmap functions.
> 
> That means that any good practice can be deployed more easily and any
> API updates only need to update the helpers.
> 
> > +static struct regulator_init_data ltc2978_regulator_init = {
> > +	.constraints = {
> > +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> > +	},
> > +};
> 
> You should not be forcing this on, you don't know what's safe on any
> given board.  Allow the board to specify constraints then it has
> control.

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 581 bytes --]

On Thu, Aug 21, 2014 at 06:18:10PM -0700, Guenter Roeck wrote:
> On Thu, Aug 21, 2014 at 07:36:50PM -0500, Mark Brown wrote:
> > On Thu, Aug 21, 2014 at 05:21:26PM -0500, atull@opensource.altera.com wrote:

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

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

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Mark Brown]

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

On Thu, Aug 21, 2014 at 06:18:10PM -0700, Guenter Roeck wrote:
> On Thu, Aug 21, 2014 at 07:36:50PM -0500, Mark Brown wrote:
> > On Thu, Aug 21, 2014 at 05:21:26PM -0500, atull@opensource.altera.com wrote:

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

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

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

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

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

[Guenter Roeck]

On Thu, Aug 21, 2014 at 08:26:22PM -0500, Mark Brown wrote:
> On Thu, Aug 21, 2014 at 06:18:10PM -0700, Guenter Roeck wrote:
> > On Thu, Aug 21, 2014 at 07:36:50PM -0500, Mark Brown wrote:
> > > On Thu, Aug 21, 2014 at 05:21:26PM -0500, atull@opensource.altera.com wrote:
> 
> > > This all looks very much like pmbus could use regmap and then the regmap
> > > helpers.  I'd not insist on it though.  What I would however suggest is
> 
> > Not unless regmap got extended recently to support quick, byte, and word
> > smbus accesses at the same time.
> 
> Depending on how you decide which it quite possibly does - if it's based
> on the register number that'd work.

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

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

Tnanks,
Guenter

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Guenter Roeck]

On Thu, Aug 21, 2014 at 08:26:22PM -0500, Mark Brown wrote:
> On Thu, Aug 21, 2014 at 06:18:10PM -0700, Guenter Roeck wrote:
> > On Thu, Aug 21, 2014 at 07:36:50PM -0500, Mark Brown wrote:
> > > On Thu, Aug 21, 2014 at 05:21:26PM -0500, atull@opensource.altera.com wrote:
> 
> > > This all looks very much like pmbus could use regmap and then the regmap
> > > helpers.  I'd not insist on it though.  What I would however suggest is
> 
> > Not unless regmap got extended recently to support quick, byte, and word
> > smbus accesses at the same time.
> 
> Depending on how you decide which it quite possibly does - if it's based
> on the register number that'd work.

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

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

Tnanks,
Guenter

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

[atull]

On Fri, 22 Aug 2014, Mark Brown wrote:

> On Thu, Aug 21, 2014 at 05:21:26PM -0500, atull@opensource.altera.com wrote:
> 
> > +config SENSORS_LTC2978_REGULATOR
> > +	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
> > +	default n
> 
> No need to say default n here, it's the default default.
> 
> > +	depends on SENSORS_LTC2978
> > +	select REGULATOR
> 
> I'd expect a depends here.

Hi Mark,

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

> 
> > +#include <linux/regulator/driver.h>
> > +#include <linux/regulator/machine.h>
> 
> If you need machine.h that's suspicious...  why do you need it?

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

> 
> > +static int ltc2978_write_pmbus_operation(struct regulator_dev *rdev, u8 value)
> > +{
> > +	struct device *dev = rdev_get_dev(rdev);
> > +	struct i2c_client *client = to_i2c_client(dev->parent);
> > +	int ret;
> > +
> > +	ret = pmbus_set_page(client, 0xff);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return i2c_smbus_write_byte_data(client, PMBUS_OPERATION, value);
> > +}
> 
> This all looks very much like pmbus could use regmap and then the regmap
> helpers.  I'd not insist on it though.  What I would however suggest is
> that these functions should all be helpers which read the specific
> page, addresses and bits to write from the driver structure - I bet the
> code is going to be identical for most pmbus using regulators and so it
> makes sense to share it like we do with the generic regmap functions.
> 
> That means that any good practice can be deployed more easily and any
> API updates only need to update the helpers.
> 
> > +static struct regulator_init_data ltc2978_regulator_init = {
> > +	.constraints = {
> > +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> > +	},
> > +};
> 
> You should not be forcing this on, you don't know what's safe on any
> given board.  Allow the board to specify constraints then it has
> control.
> 

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

Thanks!

Alan

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[atull]

On Fri, 22 Aug 2014, Mark Brown wrote:

> On Thu, Aug 21, 2014 at 05:21:26PM -0500, atull@opensource.altera.com wrote:
> 
> > +config SENSORS_LTC2978_REGULATOR
> > +	boolean "Regulator support for LTC2974, LTC2978, LTC3880, and LTC3883"
> > +	default n
> 
> No need to say default n here, it's the default default.
> 
> > +	depends on SENSORS_LTC2978
> > +	select REGULATOR
> 
> I'd expect a depends here.

Hi Mark,

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

> 
> > +#include <linux/regulator/driver.h>
> > +#include <linux/regulator/machine.h>
> 
> If you need machine.h that's suspicious...  why do you need it?

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

> 
> > +static int ltc2978_write_pmbus_operation(struct regulator_dev *rdev, u8 value)
> > +{
> > +	struct device *dev = rdev_get_dev(rdev);
> > +	struct i2c_client *client = to_i2c_client(dev->parent);
> > +	int ret;
> > +
> > +	ret = pmbus_set_page(client, 0xff);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return i2c_smbus_write_byte_data(client, PMBUS_OPERATION, value);
> > +}
> 
> This all looks very much like pmbus could use regmap and then the regmap
> helpers.  I'd not insist on it though.  What I would however suggest is
> that these functions should all be helpers which read the specific
> page, addresses and bits to write from the driver structure - I bet the
> code is going to be identical for most pmbus using regulators and so it
> makes sense to share it like we do with the generic regmap functions.
> 
> That means that any good practice can be deployed more easily and any
> API updates only need to update the helpers.
> 
> > +static struct regulator_init_data ltc2978_regulator_init = {
> > +	.constraints = {
> > +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
> > +	},
> > +};
> 
> You should not be forcing this on, you don't know what's safe on any
> given board.  Allow the board to specify constraints then it has
> control.
> 

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

Thanks!

Alan

Re: [lm-sensors] [PATCH 0/2] add regulator support for pmbus and ltc2978

[atull]

On Thu, 21 Aug 2014, atull@opensource.altera.com wrote:

> From: Alan Tull <atull@opensource.altera.com>
> 
> This set of patches adds regulator support to pmbus_core.c and to
> ltc2978.c.
> 
> Other pmbus parts can use this to add their own regulator support.
> 
> Current ltc2978 support is limited to enable/disable.
> 
> Tested on ltc2978.  Looking at the datasheets for the other parts
> supported by ltc2978.c, it looks like it should work.
> 
> Thanks,
> Alan
> 
> Alan Tull (2):
>   pmbus: add regulator support
>   pmbus: ltc2978: add regulator enable disable
> 
>  drivers/hwmon/pmbus/Kconfig      |    9 +++++
>  drivers/hwmon/pmbus/ltc2978.c    |   69 ++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/pmbus/pmbus.h      |    7 ++++
>  drivers/hwmon/pmbus/pmbus_core.c |   40 ++++++++++++++++++++++
>  4 files changed, 125 insertions(+)
> 
> -- 
> 1.7.9.5
> 
> 

I've posted V2 of these patches.

Alan


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [PATCH 0/2] add regulator support for pmbus and ltc2978

[atull]

On Thu, 21 Aug 2014, atull@opensource.altera.com wrote:

> From: Alan Tull <atull@opensource.altera.com>
> 
> This set of patches adds regulator support to pmbus_core.c and to
> ltc2978.c.
> 
> Other pmbus parts can use this to add their own regulator support.
> 
> Current ltc2978 support is limited to enable/disable.
> 
> Tested on ltc2978.  Looking at the datasheets for the other parts
> supported by ltc2978.c, it looks like it should work.
> 
> Thanks,
> Alan
> 
> Alan Tull (2):
>   pmbus: add regulator support
>   pmbus: ltc2978: add regulator enable disable
> 
>  drivers/hwmon/pmbus/Kconfig      |    9 +++++
>  drivers/hwmon/pmbus/ltc2978.c    |   69 ++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/pmbus/pmbus.h      |    7 ++++
>  drivers/hwmon/pmbus/pmbus_core.c |   40 ++++++++++++++++++++++
>  4 files changed, 125 insertions(+)
> 
> -- 
> 1.7.9.5
> 
> 

I've posted V2 of these patches.

Alan