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

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

[atull]

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

Version 2 :
  * Get regulator_init_data from platform data or device tree.
  * One regulator per pmbus part.
  * Clean up Kconfig and #includes.

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 gating

 drivers/hwmon/pmbus/Kconfig      |    7 +++++
 drivers/hwmon/pmbus/ltc2978.c    |   60 ++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/pmbus/pmbus.h      |    5 ++++
 drivers/hwmon/pmbus/pmbus_core.c |   51 ++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    1 +
 5 files changed, 124 insertions(+)

-- 
1.7.9.5

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

[atull]

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

Version 2 :
  * Get regulator_init_data from platform data or device tree.
  * One regulator per pmbus part.
  * Clean up Kconfig and #includes.

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 gating

 drivers/hwmon/pmbus/Kconfig      |    7 +++++
 drivers/hwmon/pmbus/ltc2978.c    |   60 ++++++++++++++++++++++++++++++++++++++
 drivers/hwmon/pmbus/pmbus.h      |    5 ++++
 drivers/hwmon/pmbus/pmbus_core.c |   51 ++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    1 +
 5 files changed, 124 insertions(+)

-- 
1.7.9.5


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

[PATCH v2 1/2] pmbus: add regulator support

[atull]

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

To add a regulator, the pmbus device driver needs to add
regulator_desc information to its pmbus_driver_info struct.
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
---
 drivers/hwmon/pmbus/pmbus.h      |    5 ++++
 drivers/hwmon/pmbus/pmbus_core.c |   51 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    1 +
 3 files changed, 57 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index fa9beb3..93fadc3 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,9 @@ struct pmbus_driver_info {
 	 */
 	int (*identify)(struct i2c_client *client,
 			struct pmbus_driver_info *info);
+
+	/* Regulator functionality, if supported by this chip driver. */
+	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..472baff 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/of_regulator.h>
+#include <linux/regulator/driver.h>
 #include "pmbus.h"
 
 /*
@@ -1727,6 +1729,48 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int pmbus_regulator_register(struct pmbus_data *data,
+				    const struct pmbus_platform_data *pdata)
+{
+	struct device *dev = data->dev;
+	struct device_node *np = dev->of_node;
+	const struct pmbus_driver_info *info = data->info;
+	const struct regulator_desc *reg_desc = info->reg_desc;
+	struct regulator_dev *rdev;
+	struct regulator_config config = { };
+
+	if (!reg_desc)
+		return 0;
+
+	if (pdata && pdata->reg_init_data) {
+		config.init_data = pdata->reg_init_data;
+	} else {
+		config.init_data = of_get_regulator_init_data(dev, np);
+		if (!config.init_data)
+			return -ENOMEM;
+	}
+
+	config.dev = dev;
+	config.driver_data = data;
+
+	rdev = devm_regulator_register(dev, reg_desc, &config);
+	if (IS_ERR(rdev)) {
+		dev_err(dev, "failed to register regulator %s\n",
+			reg_desc->name);
+		return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+#else
+static int pmbus_regulator_register(struct pmbus_data *data,
+				    const struct pmbus_platform_data *pdata)
+{
+	return 0;
+}
+#endif
+
 int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		   struct pmbus_driver_info *info)
 {
@@ -1781,8 +1825,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, pdata);
+	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..15e08da 100644
--- a/include/linux/i2c/pmbus.h
+++ b/include/linux/i2c/pmbus.h
@@ -40,6 +40,7 @@
 
 struct pmbus_platform_data {
 	u32 flags;		/* Device specific flags */
+	struct regulator_init_data *reg_init_data;
 };
 
 #endif /* _PMBUS_H_ */
-- 
1.7.9.5

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

[atull]

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

To add a regulator, the pmbus device driver needs to add
regulator_desc information to its pmbus_driver_info struct.
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
---
 drivers/hwmon/pmbus/pmbus.h      |    5 ++++
 drivers/hwmon/pmbus/pmbus_core.c |   51 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    1 +
 3 files changed, 57 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index fa9beb3..93fadc3 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,9 @@ struct pmbus_driver_info {
 	 */
 	int (*identify)(struct i2c_client *client,
 			struct pmbus_driver_info *info);
+
+	/* Regulator functionality, if supported by this chip driver. */
+	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..472baff 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/of_regulator.h>
+#include <linux/regulator/driver.h>
 #include "pmbus.h"
 
 /*
@@ -1727,6 +1729,48 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int pmbus_regulator_register(struct pmbus_data *data,
+				    const struct pmbus_platform_data *pdata)
+{
+	struct device *dev = data->dev;
+	struct device_node *np = dev->of_node;
+	const struct pmbus_driver_info *info = data->info;
+	const struct regulator_desc *reg_desc = info->reg_desc;
+	struct regulator_dev *rdev;
+	struct regulator_config config = { };
+
+	if (!reg_desc)
+		return 0;
+
+	if (pdata && pdata->reg_init_data) {
+		config.init_data = pdata->reg_init_data;
+	} else {
+		config.init_data = of_get_regulator_init_data(dev, np);
+		if (!config.init_data)
+			return -ENOMEM;
+	}
+
+	config.dev = dev;
+	config.driver_data = data;
+
+	rdev = devm_regulator_register(dev, reg_desc, &config);
+	if (IS_ERR(rdev)) {
+		dev_err(dev, "failed to register regulator %s\n",
+			reg_desc->name);
+		return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+#else
+static int pmbus_regulator_register(struct pmbus_data *data,
+				    const struct pmbus_platform_data *pdata)
+{
+	return 0;
+}
+#endif
+
 int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		   struct pmbus_driver_info *info)
 {
@@ -1781,8 +1825,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, pdata);
+	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..15e08da 100644
--- a/include/linux/i2c/pmbus.h
+++ b/include/linux/i2c/pmbus.h
@@ -40,6 +40,7 @@
 
 struct pmbus_platform_data {
 	u32 flags;		/* Device specific flags */
+	struct regulator_init_data *reg_init_data;
 };
 
 #endif /* _PMBUS_H_ */
-- 
1.7.9.5


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

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

[atull]

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

Add regulator with support for enabling or disabling all
supplies.

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

v2:  Remove '#include <linux/regulator/machine.h>'
     Kconfig fixes
     Remove hardwired regulator_init_data
---
 drivers/hwmon/pmbus/Kconfig   |    7 +++++
 drivers/hwmon/pmbus/ltc2978.c |   60 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 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..2bdcfe2 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/err.h>
+#include <linux/regulator/driver.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include "pmbus.h"
@@ -151,6 +152,60 @@ 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,
+};
+#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 +542,11 @@ static int ltc2978_probe(struct i2c_client *client,
 	default:
 		return -ENODEV;
 	}
+
+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+	info->reg_desc = &ltc2978_reg_desc;
+#endif
+
 	return pmbus_do_probe(client, id, info);
 }
 
-- 
1.7.9.5

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

[atull]

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

Add regulator with support for enabling or disabling all
supplies.

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

v2:  Remove '#include <linux/regulator/machine.h>'
     Kconfig fixes
     Remove hardwired regulator_init_data
---
 drivers/hwmon/pmbus/Kconfig   |    7 +++++
 drivers/hwmon/pmbus/ltc2978.c |   60 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 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..2bdcfe2 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/err.h>
+#include <linux/regulator/driver.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include "pmbus.h"
@@ -151,6 +152,60 @@ 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,
+};
+#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 +542,11 @@ static int ltc2978_probe(struct i2c_client *client,
 	default:
 		return -ENODEV;
 	}
+
+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+	info->reg_desc = &ltc2978_reg_desc;
+#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

Re: [PATCH v2 1/2] pmbus: add regulator support

[Mark Brown]

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

On Fri, Aug 22, 2014 at 04:11:33PM -0500, atull@opensource.altera.com wrote:

> +	if (pdata && pdata->reg_init_data) {
> +		config.init_data = pdata->reg_init_data;
> +	} else {
> +		config.init_data = of_get_regulator_init_data(dev, np);
> +		if (!config.init_data)
> +			return -ENOMEM;
> +	}

It should be fine to start with no init data - just let the regulator
core worry about it.  This will allow users to read back the state even
if they can't change anything which is useful for system bringup or
general debugging.

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

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

[Mark Brown]

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

On Fri, Aug 22, 2014 at 04:11:33PM -0500, atull@opensource.altera.com wrote:

> +	if (pdata && pdata->reg_init_data) {
> +		config.init_data = pdata->reg_init_data;
> +	} else {
> +		config.init_data = of_get_regulator_init_data(dev, np);
> +		if (!config.init_data)
> +			return -ENOMEM;
> +	}

It should be fine to start with no init data - just let the regulator
core worry about it.  This will allow users to read back the state even
if they can't change anything which is useful for system bringup or
general debugging.

[-- 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 v2 2/2] pmbus: ltc2978: add regulator gating

[Mark Brown]

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

On Fri, Aug 22, 2014 at 04:11:34PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add regulator with support for enabling or disabling all
> supplies.

Reviwed-by: Mark Brown <broonie@linaro.org>

though it still looks like you should be able to create generic
functions for the operations.

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

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

[Mark Brown]

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

On Fri, Aug 22, 2014 at 04:11:34PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add regulator with support for enabling or disabling all
> supplies.

Reviwed-by: Mark Brown <broonie@linaro.org>

though it still looks like you should be able to create generic
functions for the operations.

[-- 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 v2 1/2] pmbus: add regulator support

[atull]

On Fri, 22 Aug 2014, Mark Brown wrote:

> On Fri, Aug 22, 2014 at 04:11:33PM -0500, atull@opensource.altera.com wrote:
> 
> > +	if (pdata && pdata->reg_init_data) {
> > +		config.init_data = pdata->reg_init_data;
> > +	} else {
> > +		config.init_data = of_get_regulator_init_data(dev, np);
> > +		if (!config.init_data)
> > +			return -ENOMEM;
> > +	}
> 
> It should be fine to start with no init data - just let the regulator
> core worry about it.  This will allow users to read back the state even
> if they can't change anything which is useful for system bringup or
> general debugging.
> 

of_get_regulator_init_data() will only have an error if it cannot alloc 
the regulator_init_data struct.  That's why I did -ENOMEM.  If there
is no platform data and nothing about the regulator in the device tree, we 
should end up with a zeroed out regulator_init_data.

Thanks for the review.

Alan

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

[atull]

On Fri, 22 Aug 2014, Mark Brown wrote:

> On Fri, Aug 22, 2014 at 04:11:33PM -0500, atull@opensource.altera.com wrote:
> 
> > +	if (pdata && pdata->reg_init_data) {
> > +		config.init_data = pdata->reg_init_data;
> > +	} else {
> > +		config.init_data = of_get_regulator_init_data(dev, np);
> > +		if (!config.init_data)
> > +			return -ENOMEM;
> > +	}
> 
> It should be fine to start with no init data - just let the regulator
> core worry about it.  This will allow users to read back the state even
> if they can't change anything which is useful for system bringup or
> general debugging.
> 

of_get_regulator_init_data() will only have an error if it cannot alloc 
the regulator_init_data struct.  That's why I did -ENOMEM.  If there
is no platform data and nothing about the regulator in the device tree, we 
should end up with a zeroed out regulator_init_data.

Thanks for the review.

Alan

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

Re: [PATCH v2 1/2] pmbus: add regulator support

[Guenter Roeck]

On 08/22/2014 05:31 PM, atull wrote:
> On Fri, 22 Aug 2014, Mark Brown wrote:
>
>> On Fri, Aug 22, 2014 at 04:11:33PM -0500, atull@opensource.altera.com wrote:
>>
>>> +	if (pdata && pdata->reg_init_data) {
>>> +		config.init_data = pdata->reg_init_data;
>>> +	} else {
>>> +		config.init_data = of_get_regulator_init_data(dev, np);
>>> +		if (!config.init_data)
>>> +			return -ENOMEM;
>>> +	}
>>
>> It should be fine to start with no init data - just let the regulator
>> core worry about it.  This will allow users to read back the state even
>> if they can't change anything which is useful for system bringup or
>> general debugging.
>>
>
> of_get_regulator_init_data() will only have an error if it cannot alloc
> the regulator_init_data struct.  That's why I did -ENOMEM.  If there
> is no platform data and nothing about the regulator in the device tree, we
> should end up with a zeroed out regulator_init_data.
>
Yes, but if OF is not defined it will return NULL as well. Unless I am
missing something, that means that the code will now fail if there is
no platform init data and OF is not configured.

Guenter

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

[Guenter Roeck]

On 08/22/2014 05:31 PM, atull wrote:
> On Fri, 22 Aug 2014, Mark Brown wrote:
>
>> On Fri, Aug 22, 2014 at 04:11:33PM -0500, atull@opensource.altera.com wrote:
>>
>>> +	if (pdata && pdata->reg_init_data) {
>>> +		config.init_data = pdata->reg_init_data;
>>> +	} else {
>>> +		config.init_data = of_get_regulator_init_data(dev, np);
>>> +		if (!config.init_data)
>>> +			return -ENOMEM;
>>> +	}
>>
>> It should be fine to start with no init data - just let the regulator
>> core worry about it.  This will allow users to read back the state even
>> if they can't change anything which is useful for system bringup or
>> general debugging.
>>
>
> of_get_regulator_init_data() will only have an error if it cannot alloc
> the regulator_init_data struct.  That's why I did -ENOMEM.  If there
> is no platform data and nothing about the regulator in the device tree, we
> should end up with a zeroed out regulator_init_data.
>
Yes, but if OF is not defined it will return NULL as well. Unless I am
missing something, that means that the code will now fail if there is
no platform init data and OF is not configured.

Guenter


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

Re: [PATCH v2 1/2] pmbus: add regulator support

[Mark Brown]

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

On Sat, Aug 23, 2014 at 07:00:44AM -0700, Guenter Roeck wrote:
> On 08/22/2014 05:31 PM, atull wrote:

> >of_get_regulator_init_data() will only have an error if it cannot alloc
> >the regulator_init_data struct.  That's why I did -ENOMEM.  If there
> >is no platform data and nothing about the regulator in the device tree, we
> >should end up with a zeroed out regulator_init_data.

> Yes, but if OF is not defined it will return NULL as well. Unless I am
> missing something, that means that the code will now fail if there is
> no platform init data and OF is not configured.

Indeed, and that's the more interesting case than running out of memory.

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

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

[Mark Brown]

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

On Sat, Aug 23, 2014 at 07:00:44AM -0700, Guenter Roeck wrote:
> On 08/22/2014 05:31 PM, atull wrote:

> >of_get_regulator_init_data() will only have an error if it cannot alloc
> >the regulator_init_data struct.  That's why I did -ENOMEM.  If there
> >is no platform data and nothing about the regulator in the device tree, we
> >should end up with a zeroed out regulator_init_data.

> Yes, but if OF is not defined it will return NULL as well. Unless I am
> missing something, that means that the code will now fail if there is
> no platform init data and OF is not configured.

Indeed, and that's the more interesting case than running out of memory.

[-- 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 v2 2/2] pmbus: ltc2978: add regulator gating

[Guenter Roeck]

On 08/22/2014 02:45 PM, Mark Brown wrote:
> On Fri, Aug 22, 2014 at 04:11:34PM -0500, atull@opensource.altera.com wrote:
>> From: Alan Tull <atull@opensource.altera.com>
>>
>> Add regulator with support for enabling or disabling all
>> supplies.
>
> Reviwed-by: Mark Brown <broonie@linaro.org>
>
> though it still looks like you should be able to create generic
> functions for the operations.
>
Sorry I didn't have time to review the code myself. I'll have
to check the datasheet about turning regulators on and off.
Using page 0xff for the lm2978 looks wrong, as the chip supports
up to 8 channels which should be controlled separately
(I would assume) instead of turning them all on and off in
one go. Maybe I am missing something, but my assumption would
have been to have a separate regulator for each channel, and
that each channel would have its own regulator which would be
turned on and off separately. So I don't really understand the
change between v1 and v2 of the core patch, which dropped the
per-channel regulators. Someone will have to explain to me
why that makes sense, especially since it means that I won't
be able to use the regulator expansion in my system (which
would require per-channel regulators, and which does not always
have all channels enabled on a given chip).

In respect to generic functions, that really depends on the scope
of the regulators. As written, where all regulators are turned on
in a single operation, per-chip functions are needed. I thought
we would only need per-chip configuration values, but that only
applies if regulators are turned on one by one, not all in one go.

Either case, a wrapper for ltc2978_write_pmbus_operation needs to
be added to pmbus_core.c as pmbus_write_byte_data and exported
(as a separate patch). For paged reads, the existing
pmbus_read_byte_data should be used. In general, avoid direct
accesses to paged registers and use API functions instead
if possible.

Thanks,
Guenter

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

[Guenter Roeck]

On 08/22/2014 02:45 PM, Mark Brown wrote:
> On Fri, Aug 22, 2014 at 04:11:34PM -0500, atull@opensource.altera.com wrote:
>> From: Alan Tull <atull@opensource.altera.com>
>>
>> Add regulator with support for enabling or disabling all
>> supplies.
>
> Reviwed-by: Mark Brown <broonie@linaro.org>
>
> though it still looks like you should be able to create generic
> functions for the operations.
>
Sorry I didn't have time to review the code myself. I'll have
to check the datasheet about turning regulators on and off.
Using page 0xff for the lm2978 looks wrong, as the chip supports
up to 8 channels which should be controlled separately
(I would assume) instead of turning them all on and off in
one go. Maybe I am missing something, but my assumption would
have been to have a separate regulator for each channel, and
that each channel would have its own regulator which would be
turned on and off separately. So I don't really understand the
change between v1 and v2 of the core patch, which dropped the
per-channel regulators. Someone will have to explain to me
why that makes sense, especially since it means that I won't
be able to use the regulator expansion in my system (which
would require per-channel regulators, and which does not always
have all channels enabled on a given chip).

In respect to generic functions, that really depends on the scope
of the regulators. As written, where all regulators are turned on
in a single operation, per-chip functions are needed. I thought
we would only need per-chip configuration values, but that only
applies if regulators are turned on one by one, not all in one go.

Either case, a wrapper for ltc2978_write_pmbus_operation needs to
be added to pmbus_core.c as pmbus_write_byte_data and exported
(as a separate patch). For paged reads, the existing
pmbus_read_byte_data should be used. In general, avoid direct
accesses to paged registers and use API functions instead
if possible.

Thanks,
Guenter


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

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

[Mark Brown]

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

On Sat, Aug 23, 2014 at 08:10:16AM -0700, Guenter Roeck wrote:

> Sorry I didn't have time to review the code myself. I'll have
> to check the datasheet about turning regulators on and off.
> Using page 0xff for the lm2978 looks wrong, as the chip supports
> up to 8 channels which should be controlled separately
> (I would assume) instead of turning them all on and off in
> one go. Maybe I am missing something, but my assumption would
> have been to have a separate regulator for each channel, and
> that each channel would have its own regulator which would be

BTW I should point out that my review is just for the regulator API
aspects of the change, I have no knowledge of this hardware.  If what
you're saying matches the hardware I'd definitely expect to see one
regulator registered per physical regulator rather than a single
regulator for everything on the device.

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

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

[Mark Brown]

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

On Sat, Aug 23, 2014 at 08:10:16AM -0700, Guenter Roeck wrote:

> Sorry I didn't have time to review the code myself. I'll have
> to check the datasheet about turning regulators on and off.
> Using page 0xff for the lm2978 looks wrong, as the chip supports
> up to 8 channels which should be controlled separately
> (I would assume) instead of turning them all on and off in
> one go. Maybe I am missing something, but my assumption would
> have been to have a separate regulator for each channel, and
> that each channel would have its own regulator which would be

BTW I should point out that my review is just for the regulator API
aspects of the change, I have no knowledge of this hardware.  If what
you're saying matches the hardware I'd definitely expect to see one
regulator registered per physical regulator rather than a single
regulator for everything on the device.

[-- 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 v2 1/2] pmbus: add regulator support

[Alan Tull]

On Sat, Aug 23, 2014 at 9:54 AM, Mark Brown <broonie@kernel.org> wrote:
> On Sat, Aug 23, 2014 at 07:00:44AM -0700, Guenter Roeck wrote:
>> On 08/22/2014 05:31 PM, atull wrote:
>
>> >of_get_regulator_init_data() will only have an error if it cannot alloc
>> >the regulator_init_data struct.  That's why I did -ENOMEM.  If there
>> >is no platform data and nothing about the regulator in the device tree, we
>> >should end up with a zeroed out regulator_init_data.
>
>> Yes, but if OF is not defined it will return NULL as well. Unless I am
>> missing something, that means that the code will now fail if there is
>> no platform init data and OF is not configured.
>
> Indeed, and that's the more interesting case than running out of memory.

Thanks for catching this.  I'll remove the unnecessary check for NULL
and the brackets here.

Alan

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

[Alan Tull]

On Sat, Aug 23, 2014 at 9:54 AM, Mark Brown <broonie@kernel.org> wrote:
> On Sat, Aug 23, 2014 at 07:00:44AM -0700, Guenter Roeck wrote:
>> On 08/22/2014 05:31 PM, atull wrote:
>
>> >of_get_regulator_init_data() will only have an error if it cannot alloc
>> >the regulator_init_data struct.  That's why I did -ENOMEM.  If there
>> >is no platform data and nothing about the regulator in the device tree, we
>> >should end up with a zeroed out regulator_init_data.
>
>> Yes, but if OF is not defined it will return NULL as well. Unless I am
>> missing something, that means that the code will now fail if there is
>> no platform init data and OF is not configured.
>
> Indeed, and that's the more interesting case than running out of memory.

Thanks for catching this.  I'll remove the unnecessary check for NULL
and the brackets here.

Alan

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

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

[Alan Tull]

On Sat, Aug 23, 2014 at 10:10 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 08/22/2014 02:45 PM, Mark Brown wrote:
>>
>> On Fri, Aug 22, 2014 at 04:11:34PM -0500, atull@opensource.altera.com
>> wrote:
>>>
>>> From: Alan Tull <atull@opensource.altera.com>
>>>
>>> Add regulator with support for enabling or disabling all
>>> supplies.
>>
>>
>> Reviwed-by: Mark Brown <broonie@linaro.org>
>>
>> though it still looks like you should be able to create generic
>> functions for the operations.
>>
> Sorry I didn't have time to review the code myself. I'll have
> to check the datasheet about turning regulators on and off.
> Using page 0xff for the lm2978 looks wrong, as the chip supports
> up to 8 channels which should be controlled separately
> (I would assume) instead of turning them all on and off in
> one go. Maybe I am missing something, but my assumption would
> have been to have a separate regulator for each channel, and
> that each channel would have its own regulator which would be
> turned on and off separately. So I don't really understand the
> change between v1 and v2 of the core patch, which dropped the
> per-channel regulators. Someone will have to explain to me
> why that makes sense, especially since it means that I won't
> be able to use the regulator expansion in my system (which
> would require per-channel regulators, and which does not always
> have all channels enabled on a given chip).

The LTC2978 spec says that the OPERATION command register "responds to
the global page command (PAGE=0xFF)."  I originally took it to mean
that it *only* responds to 0xFF.  Now I get that yes I could turn
on/off each of 8 channels separately.

I will make the change have one regulator for each supply.  It would
be useful to still have a global regulator for turning them all on/off
together if that is not completely odious.

>
> In respect to generic functions, that really depends on the scope
> of the regulators. As written, where all regulators are turned on
> in a single operation, per-chip functions are needed. I thought
> we would only need per-chip configuration values, but that only
> applies if regulators are turned on one by one, not all in one go.

For all the parts supported by ltc2978.c, a banked write of 0x80 to
the OPERATION register turns a regulator on, writing 0x00 turns it
off.  I'm not sure how universal that is for other PMBUS parts.  I
will look at the PMBUS specs when I get back to the office Tuesday.

I'll look into it and see if I can push almost all of this code into
pmbus_core.c and just have per-chip config values in
pmbus_driver_info.   If many PMBUS parts have the same scheme (0x80 or
0x00 written to OPERATION register as a banked write) then  a flag bit
that is turned on in pmbus_driver_info could enable this scheme for
this chip.  If it is a different register or different on/off values,
then I'll need to add those to pmbus_driver_info.
>
> Either case, a wrapper for ltc2978_write_pmbus_operation needs to
> be added to pmbus_core.c as pmbus_write_byte_data and exported
> (as a separate patch). For paged reads, the existing
> pmbus_read_byte_data should be used. In general, avoid direct
> accesses to paged registers and use API functions instead
> if possible.

OK.  Will add pmbus_write_byte_data as a separate patch and use the
existing pmbus_read_byte_data.

Thanks for the review!

Alan

>
> Thanks,
> Guenter
>

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

[Alan Tull]

On Sat, Aug 23, 2014 at 10:10 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 08/22/2014 02:45 PM, Mark Brown wrote:
>>
>> On Fri, Aug 22, 2014 at 04:11:34PM -0500, atull@opensource.altera.com
>> wrote:
>>>
>>> From: Alan Tull <atull@opensource.altera.com>
>>>
>>> Add regulator with support for enabling or disabling all
>>> supplies.
>>
>>
>> Reviwed-by: Mark Brown <broonie@linaro.org>
>>
>> though it still looks like you should be able to create generic
>> functions for the operations.
>>
> Sorry I didn't have time to review the code myself. I'll have
> to check the datasheet about turning regulators on and off.
> Using page 0xff for the lm2978 looks wrong, as the chip supports
> up to 8 channels which should be controlled separately
> (I would assume) instead of turning them all on and off in
> one go. Maybe I am missing something, but my assumption would
> have been to have a separate regulator for each channel, and
> that each channel would have its own regulator which would be
> turned on and off separately. So I don't really understand the
> change between v1 and v2 of the core patch, which dropped the
> per-channel regulators. Someone will have to explain to me
> why that makes sense, especially since it means that I won't
> be able to use the regulator expansion in my system (which
> would require per-channel regulators, and which does not always
> have all channels enabled on a given chip).

The LTC2978 spec says that the OPERATION command register "responds to
the global page command (PAGE=0xFF)."  I originally took it to mean
that it *only* responds to 0xFF.  Now I get that yes I could turn
on/off each of 8 channels separately.

I will make the change have one regulator for each supply.  It would
be useful to still have a global regulator for turning them all on/off
together if that is not completely odious.

>
> In respect to generic functions, that really depends on the scope
> of the regulators. As written, where all regulators are turned on
> in a single operation, per-chip functions are needed. I thought
> we would only need per-chip configuration values, but that only
> applies if regulators are turned on one by one, not all in one go.

For all the parts supported by ltc2978.c, a banked write of 0x80 to
the OPERATION register turns a regulator on, writing 0x00 turns it
off.  I'm not sure how universal that is for other PMBUS parts.  I
will look at the PMBUS specs when I get back to the office Tuesday.

I'll look into it and see if I can push almost all of this code into
pmbus_core.c and just have per-chip config values in
pmbus_driver_info.   If many PMBUS parts have the same scheme (0x80 or
0x00 written to OPERATION register as a banked write) then  a flag bit
that is turned on in pmbus_driver_info could enable this scheme for
this chip.  If it is a different register or different on/off values,
then I'll need to add those to pmbus_driver_info.
>
> Either case, a wrapper for ltc2978_write_pmbus_operation needs to
> be added to pmbus_core.c as pmbus_write_byte_data and exported
> (as a separate patch). For paged reads, the existing
> pmbus_read_byte_data should be used. In general, avoid direct
> accesses to paged registers and use API functions instead
> if possible.

OK.  Will add pmbus_write_byte_data as a separate patch and use the
existing pmbus_read_byte_data.

Thanks for the review!

Alan

>
> Thanks,
> Guenter
>

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

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

[Guenter Roeck]

On Sun, Aug 24, 2014 at 08:30:16AM -0500, Alan Tull wrote:
> On Sat, Aug 23, 2014 at 10:10 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 08/22/2014 02:45 PM, Mark Brown wrote:
> >>
> >> On Fri, Aug 22, 2014 at 04:11:34PM -0500, atull@opensource.altera.com
> >> wrote:
> >>>
> >>> From: Alan Tull <atull@opensource.altera.com>
> >>>
> >>> Add regulator with support for enabling or disabling all
> >>> supplies.
> >>
> >>
> >> Reviwed-by: Mark Brown <broonie@linaro.org>
> >>
> >> though it still looks like you should be able to create generic
> >> functions for the operations.
> >>
> > Sorry I didn't have time to review the code myself. I'll have
> > to check the datasheet about turning regulators on and off.
> > Using page 0xff for the lm2978 looks wrong, as the chip supports
> > up to 8 channels which should be controlled separately
> > (I would assume) instead of turning them all on and off in
> > one go. Maybe I am missing something, but my assumption would
> > have been to have a separate regulator for each channel, and
> > that each channel would have its own regulator which would be
> > turned on and off separately. So I don't really understand the
> > change between v1 and v2 of the core patch, which dropped the
> > per-channel regulators. Someone will have to explain to me
> > why that makes sense, especially since it means that I won't
> > be able to use the regulator expansion in my system (which
> > would require per-channel regulators, and which does not always
> > have all channels enabled on a given chip).
> 
> The LTC2978 spec says that the OPERATION command register "responds to
> the global page command (PAGE=0xFF)."  I originally took it to mean
> that it *only* responds to 0xFF.  Now I get that yes I could turn
> on/off each of 8 channels separately.
> 
> I will make the change have one regulator for each supply.  It would
> be useful to still have a global regulator for turning them all on/off
> together if that is not completely odious.
> 
> >
> > In respect to generic functions, that really depends on the scope
> > of the regulators. As written, where all regulators are turned on
> > in a single operation, per-chip functions are needed. I thought
> > we would only need per-chip configuration values, but that only
> > applies if regulators are turned on one by one, not all in one go.
> 
> For all the parts supported by ltc2978.c, a banked write of 0x80 to
> the OPERATION register turns a regulator on, writing 0x00 turns it
> off.  I'm not sure how universal that is for other PMBUS parts.  I
> will look at the PMBUS specs when I get back to the office Tuesday.
> 
It isn't. Other parts need a different value to be written for on/off.
However, that does not mean we need a chip specific _function_ to write
the values. All we should need is the values to write for on/off.

> I'll look into it and see if I can push almost all of this code into
> pmbus_core.c and just have per-chip config values in
> pmbus_driver_info.   If many PMBUS parts have the same scheme (0x80 or
> 0x00 written to OPERATION register as a banked write) then  a flag bit
> that is turned on in pmbus_driver_info could enable this scheme for
> this chip.  If it is a different register or different on/off values,
> then I'll need to add those to pmbus_driver_info.

Values should work, and the writes should all be banked, but a flag is
unfortunately insufficient. You should include a per-page flag to enable
the functionality (something like PMBUS_HAVE_REGULATOR). Some PMBus chips
don't have regulators on all pages.

> >
> > Either case, a wrapper for ltc2978_write_pmbus_operation needs to
> > be added to pmbus_core.c as pmbus_write_byte_data and exported
> > (as a separate patch). For paged reads, the existing
> > pmbus_read_byte_data should be used. In general, avoid direct
> > accesses to paged registers and use API functions instead
> > if possible.
> 
> OK.  Will add pmbus_write_byte_data as a separate patch and use the
> existing pmbus_read_byte_data.
> 
If you use per-page flags and values, you should not need those
at all.

Thanks,
Guenter

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

[Guenter Roeck]

On Sun, Aug 24, 2014 at 08:30:16AM -0500, Alan Tull wrote:
> On Sat, Aug 23, 2014 at 10:10 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > On 08/22/2014 02:45 PM, Mark Brown wrote:
> >>
> >> On Fri, Aug 22, 2014 at 04:11:34PM -0500, atull@opensource.altera.com
> >> wrote:
> >>>
> >>> From: Alan Tull <atull@opensource.altera.com>
> >>>
> >>> Add regulator with support for enabling or disabling all
> >>> supplies.
> >>
> >>
> >> Reviwed-by: Mark Brown <broonie@linaro.org>
> >>
> >> though it still looks like you should be able to create generic
> >> functions for the operations.
> >>
> > Sorry I didn't have time to review the code myself. I'll have
> > to check the datasheet about turning regulators on and off.
> > Using page 0xff for the lm2978 looks wrong, as the chip supports
> > up to 8 channels which should be controlled separately
> > (I would assume) instead of turning them all on and off in
> > one go. Maybe I am missing something, but my assumption would
> > have been to have a separate regulator for each channel, and
> > that each channel would have its own regulator which would be
> > turned on and off separately. So I don't really understand the
> > change between v1 and v2 of the core patch, which dropped the
> > per-channel regulators. Someone will have to explain to me
> > why that makes sense, especially since it means that I won't
> > be able to use the regulator expansion in my system (which
> > would require per-channel regulators, and which does not always
> > have all channels enabled on a given chip).
> 
> The LTC2978 spec says that the OPERATION command register "responds to
> the global page command (PAGE=0xFF)."  I originally took it to mean
> that it *only* responds to 0xFF.  Now I get that yes I could turn
> on/off each of 8 channels separately.
> 
> I will make the change have one regulator for each supply.  It would
> be useful to still have a global regulator for turning them all on/off
> together if that is not completely odious.
> 
> >
> > In respect to generic functions, that really depends on the scope
> > of the regulators. As written, where all regulators are turned on
> > in a single operation, per-chip functions are needed. I thought
> > we would only need per-chip configuration values, but that only
> > applies if regulators are turned on one by one, not all in one go.
> 
> For all the parts supported by ltc2978.c, a banked write of 0x80 to
> the OPERATION register turns a regulator on, writing 0x00 turns it
> off.  I'm not sure how universal that is for other PMBUS parts.  I
> will look at the PMBUS specs when I get back to the office Tuesday.
> 
It isn't. Other parts need a different value to be written for on/off.
However, that does not mean we need a chip specific _function_ to write
the values. All we should need is the values to write for on/off.

> I'll look into it and see if I can push almost all of this code into
> pmbus_core.c and just have per-chip config values in
> pmbus_driver_info.   If many PMBUS parts have the same scheme (0x80 or
> 0x00 written to OPERATION register as a banked write) then  a flag bit
> that is turned on in pmbus_driver_info could enable this scheme for
> this chip.  If it is a different register or different on/off values,
> then I'll need to add those to pmbus_driver_info.

Values should work, and the writes should all be banked, but a flag is
unfortunately insufficient. You should include a per-page flag to enable
the functionality (something like PMBUS_HAVE_REGULATOR). Some PMBus chips
don't have regulators on all pages.

> >
> > Either case, a wrapper for ltc2978_write_pmbus_operation needs to
> > be added to pmbus_core.c as pmbus_write_byte_data and exported
> > (as a separate patch). For paged reads, the existing
> > pmbus_read_byte_data should be used. In general, avoid direct
> > accesses to paged registers and use API functions instead
> > if possible.
> 
> OK.  Will add pmbus_write_byte_data as a separate patch and use the
> existing pmbus_read_byte_data.
> 
If you use per-page flags and values, you should not need those
at all.

Thanks,
Guenter

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