[PATCH v4 0/4] regulator support for pmbus and ltc2978

[PATCH v4 0/4] regulator support for pmbus and ltc2978

[atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx]

From: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>

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.

Fixes in v4 are not huge:
  Add device tree bindings documentation for ltc2978.
  rename _pmbus_regulator_enable to _pmbus_regulator_on_off
  simplify _pmbus_regulator_on_off code
  s/regulator_regulator/regulator/
  fix build break when !CONFIG_REGULATOR
  remove unused #define PB_OPERATION_CONTROL_SEQ_OFF
  fix a #endif comment
  simplify probe code, remove added switch statement
  remove BUG_ON(), add error message and fix num_regulators

Patch 1: document device tree bindings for ltc2978

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

Patch 3: regulator support added in 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 4: 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

Alan Tull (4):
  hwmon: ltc2978: device tree bindings documentation
  pmbus: core: add helpers for byte write and read modify write
  pmbus: add regulator support
  pmbus: ltc2978: add regulator support

 .../devicetree/bindings/hwmon/ltc2978.txt          |   42 +++++
 drivers/hwmon/pmbus/Kconfig                        |    7 +
 drivers/hwmon/pmbus/ltc2978.c                      |   37 +++++
 drivers/hwmon/pmbus/pmbus.h                        |   26 ++++
 drivers/hwmon/pmbus/pmbus_core.c                   |  160 ++++++++++++++++++++
 include/linux/i2c/pmbus.h                          |    4 +
 6 files changed, 276 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/4] hwmon: ltc2978: device tree bindings documentation

[atull]

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

Add device tree bindings documentation for ltc2978.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
 .../devicetree/bindings/hwmon/ltc2978.txt          |   42 ++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt

diff --git a/Documentation/devicetree/bindings/hwmon/ltc2978.txt b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
new file mode 100644
index 0000000..60fadc2
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
@@ -0,0 +1,42 @@
+ltc2978
+
+Required properties:
+ - compatible: one of: ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676
+ - reg: I2C address
+
+Optional properties:
+ Name of the optional regulator subnode must be "regulators".
+  - #address-cells must be 1.
+  - #size-cells must be 0.
+
+ For each regulator:
+  - reg: regulator number
+  - regulator-compatible: must be vout_en<regulator number> such as vout_en3
+    valid range is:
+      ltc2977, ltc2978 : vout_en0 - vout_en7
+      ltc2974          : vout_en0 - vout_en3
+      ltc3880, ltm4676 : vout_en0 - vout_en1
+      ltc3883          : vout_en0 only
+  - regulator-name: arbitrary name for regulator
+
+Example:
+ltc2978@5e {
+	compatible = "ltc2978";
+	reg = <0x5e>;
+	regulators {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		vdacp0_reg: regulator@0 {
+			reg = <0>;
+			regulator-compatible = "vout_en0";
+			regulator-name = "FPGA-2.5V";
+		};
+		vdacp2_reg: regulator@2 {
+			reg = <2>;
+			regulator-compatible = "vout_en2";
+			regulator-name = "FPGA-1.5V";
+		};
+	};
+};
+
-- 
1.7.9.5

[PATCH v4 2/4] pmbus: core: add helpers for byte write and read modify write

[atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx]

From: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>

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-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/4] pmbus: core: add helpers for byte write and read modify write

[Guenter Roeck]

On 10/01/2014 12:05 PM, atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
> From: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>
> 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-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> ---
>   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.
>

Results in:

drivers/hwmon/pmbus/pmbus_core.c:259:5: warning: symbol 'pmbus_write_byte_data' was not declared. Should it be static?
drivers/hwmon/pmbus/pmbus_core.c:271:5: warning: symbol 'pmbus_update_byte_data' was not declared. Should it be static?
drivers/hwmon/pmbus/pmbus_core.c:259:5: warning: no previous prototype for 'pmbus_write_byte_data' [-Wmissing-prototypes]
drivers/hwmon/pmbus/pmbus_core.c:271:5: warning: no previous prototype for 'pmbus_update_byte_data' [-Wmissing-prototypes]

No need to resubmit. I added a patch to un-export the functions
and merged patch 2 and 3 together in order to avoid a warning
after patch 2. We can always export the new helper functions
later if needed.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/4] regulator support for pmbus and ltc2978

[Guenter Roeck]

On Wed, Oct 01, 2014 at 02:05:46PM -0500, atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org wrote:
> From: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
> 
> 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.
> 
> Fixes in v4 are not huge: Add device tree bindings documentation for ltc2978.
> rename _pmbus_regulator_enable to _pmbus_regulator_on_off simplify
> _pmbus_regulator_on_off code s/regulator_regulator/regulator/ fix build break
> when !CONFIG_REGULATOR remove unused #define PB_OPERATION_CONTROL_SEQ_OFF fix
> a #endif comment simplify probe code, remove added switch statement remove
> BUG_ON(), add error message and fix num_regulators
> 
> Patch 1: document device tree bindings for ltc2978
> 
> Patch 2: add two helper functions for byte pmbus byte operations * byte write
> and byte read/modify/write
> 
> Patch 3: regulator support added in 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 4: 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.
> 
Looks like this patch got lost. I did not get it, and I don't see it on lkml
either.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/4] regulator support for pmbus and ltc2978

[atull]

On Wed, 1 Oct 2014, Guenter Roeck wrote:

> On Wed, Oct 01, 2014 at 02:05:46PM -0500, atull@opensource.altera.com wrote:
> > 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.
> > 
> > Fixes in v4 are not huge: Add device tree bindings documentation for ltc2978.
> > rename _pmbus_regulator_enable to _pmbus_regulator_on_off simplify
> > _pmbus_regulator_on_off code s/regulator_regulator/regulator/ fix build break
> > when !CONFIG_REGULATOR remove unused #define PB_OPERATION_CONTROL_SEQ_OFF fix
> > a #endif comment simplify probe code, remove added switch statement remove
> > BUG_ON(), add error message and fix num_regulators
> > 
> > Patch 1: document device tree bindings for ltc2978
> > 
> > Patch 2: add two helper functions for byte pmbus byte operations * byte write
> > and byte read/modify/write
> > 
> > Patch 3: regulator support added in 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 4: 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.
> > 
> Looks like this patch got lost. I did not get it, and I don't see it on lkml
> either.
> 
> Guenter

Yes I missed it.  Just sent patch 0004 out.

Alan


> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH v4 3/4] 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

v4: rename _pmbus_regulator_enable to _pmbus_regulator_on_off
    simplify _pmbus_regulator_on_off code
    s/regulator_regulator/regulator/
    fix break when !CONFIG_REGULATOR
    remove unused #define PB_OPERATION_CONTROL_SEQ_OFF
---
 drivers/hwmon/pmbus/pmbus.h      |   26 ++++++++
 drivers/hwmon/pmbus/pmbus_core.c |  129 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    4 ++
 3 files changed, 159 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index fa9beb3..64b44a1 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,11 @@
 #define PMBUS_VIRT_STATUS_VMON		(PMBUS_VIRT_BASE + 35)
 
 /*
+ * OPERATION
+ */
+#define PB_OPERATION_CONTROL_ON		(1<<7)
+
+/*
  * CAPABILITY
  */
 #define PB_CAPABILITY_SMBALERT		(1<<4)
@@ -365,8 +373,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_ops;
+
+/* Macro for filling in array of struct regulator_desc */
+#define PMBUS_REGULATOR(_name, _id)				\
+	[_id] = {						\
+		.name = (_name # _id),				\
+		.id = (_id),					\
+		.ops = &pmbus_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..a8a5cee 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_on_off(struct regulator_dev *rdev, bool enable)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	u8 page = rdev_get_id(rdev);
+
+	return pmbus_update_byte_data(client, page, PMBUS_OPERATION,
+				      PB_OPERATION_CONTROL_ON,
+				      enable ? PB_OPERATION_CONTROL_ON : 0);
+}
+
+static int pmbus_regulator_enable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_on_off(rdev, 1);
+}
+
+static int pmbus_regulator_disable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_on_off(rdev, 0);
+}
+
+struct regulator_ops pmbus_regulator_ops = {
+	.enable = pmbus_regulator_enable,
+	.disable = pmbus_regulator_disable,
+	.is_enabled = pmbus_regulator_is_enabled,
+};
+EXPORT_SYMBOL_GPL(pmbus_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 ret, i;
+
+	ret = pmbus_regulator_parse_dt(dev, info);
+	if (ret)
+		return ret;
+
+	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)
 {
@@ -1812,8 +1934,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 v4 3/4] pmbus: add regulator support

[Mark Brown]

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

On Wed, Oct 01, 2014 at 02:05:49PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add support for simple on/off control of each channel.

This is basically fine but the regulator API has moved on a bit with the
DT handling over the time you've been working on this.

> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> 
> v2: Remove '#include <linux/regulator/machine.h>'
>     Only one regulator per pmbus device

Put this after the --- as covered in SubmittingPatches.

> +static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)
> +{
> +	struct device *dev = rdev_get_dev(rdev);
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	u8 page = rdev_get_id(rdev);
> +
> +	return pmbus_update_byte_data(client, page, PMBUS_OPERATION,
> +				      PB_OPERATION_CONTROL_ON,
> +				      enable ? PB_OPERATION_CONTROL_ON : 0);
> +}
> +
> +static int pmbus_regulator_enable(struct regulator_dev *rdev)
> +{
> +	return _pmbus_regulator_on_off(rdev, 1);
> +}

I'm not sure factoring out the code actually won much here.

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

We now have helpers in the regulator core for this - set of_match and
regulators_node in the regulator descriptor and the core will resolve
this stuff for you and...

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

...you can just drop the else case (and check for reg_init_data) here.

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

Re: [PATCH v4 3/4] pmbus: add regulator support

[Guenter Roeck]

On 10/02/2014 04:20 AM, Mark Brown wrote:
> On Wed, Oct 01, 2014 at 02:05:49PM -0500, atull@opensource.altera.com wrote:
>> From: Alan Tull <atull@opensource.altera.com>
>>
>> Add support for simple on/off control of each channel.
>
> This is basically fine but the regulator API has moved on a bit with the
> DT handling over the time you've been working on this.
>
>> Signed-off-by: Alan Tull <atull@opensource.altera.com>
>>
>> v2: Remove '#include <linux/regulator/machine.h>'
>>      Only one regulator per pmbus device
>
> Put this after the --- as covered in SubmittingPatches.
>
>> +static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)
>> +{
>> +	struct device *dev = rdev_get_dev(rdev);
>> +	struct i2c_client *client = to_i2c_client(dev->parent);
>> +	u8 page = rdev_get_id(rdev);
>> +
>> +	return pmbus_update_byte_data(client, page, PMBUS_OPERATION,
>> +				      PB_OPERATION_CONTROL_ON,
>> +				      enable ? PB_OPERATION_CONTROL_ON : 0);
>> +}
>> +
>> +static int pmbus_regulator_enable(struct regulator_dev *rdev)
>> +{
>> +	return _pmbus_regulator_on_off(rdev, 1);
>> +}
>
> I'm not sure factoring out the code actually won much here.
>
>> +	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);
>
> We now have helpers in the regulator core for this - set of_match and
> regulators_node in the regulator descriptor and the core will resolve
> this stuff for you and...
>
>> +		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;
>> +		}
>
> ...you can just drop the else case (and check for reg_init_data) here.
>

Guess we'll need another version after all.

Alan, in that case please provide prototyles for the new API functions
introduced in patch 2/4.

Thanks,
Guenter

Re: [PATCH v4 3/4] pmbus: add regulator support

[atull]

On Thu, 2 Oct 2014, Guenter Roeck wrote:

> On 10/02/2014 04:20 AM, Mark Brown wrote:
> > On Wed, Oct 01, 2014 at 02:05:49PM -0500, atull@opensource.altera.com wrote:
> > > From: Alan Tull <atull@opensource.altera.com>
> > > 
> > > Add support for simple on/off control of each channel.
> > 
> > This is basically fine but the regulator API has moved on a bit with the
> > DT handling over the time you've been working on this.
> > 
> > > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > > 
> > > v2: Remove '#include <linux/regulator/machine.h>'
> > >      Only one regulator per pmbus device
> > 
> > Put this after the --- as covered in SubmittingPatches.
> > 
> > > +static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool
> > > enable)
> > > +{
> > > +	struct device *dev = rdev_get_dev(rdev);
> > > +	struct i2c_client *client = to_i2c_client(dev->parent);
> > > +	u8 page = rdev_get_id(rdev);
> > > +
> > > +	return pmbus_update_byte_data(client, page, PMBUS_OPERATION,
> > > +				      PB_OPERATION_CONTROL_ON,
> > > +				      enable ? PB_OPERATION_CONTROL_ON : 0);
> > > +}
> > > +
> > > +static int pmbus_regulator_enable(struct regulator_dev *rdev)
> > > +{
> > > +	return _pmbus_regulator_on_off(rdev, 1);
> > > +}
> > 
> > I'm not sure factoring out the code actually won much here.
> > 
> > > +	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);
> > 
> > We now have helpers in the regulator core for this - set of_match and
> > regulators_node in the regulator descriptor and the core will resolve
> > this stuff for you and...
> > 
> > > +		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;
> > > +		}
> > 
> > ...you can just drop the else case (and check for reg_init_data) here.
> > 
> 
> Guess we'll need another version after all.
> 
> Alan, in that case please provide prototyles for the new API functions
> introduced in patch 2/4.
> 
> Thanks,
> Guenter

OK I have sent out v5 which uses the simplified DT parsing method.

I added the prototypes and made the other fixes requested.

Alan


> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH v4 0/4] regulator support for pmbus and ltc2978

[Guenter Roeck]

On 10/01/2014 12:05 PM, atull@opensource.altera.com wrote:
> 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.
>
> Fixes in v4 are not huge:
>    Add device tree bindings documentation for ltc2978.
>    rename _pmbus_regulator_enable to _pmbus_regulator_on_off
>    simplify _pmbus_regulator_on_off code
>    s/regulator_regulator/regulator/
>    fix build break when !CONFIG_REGULATOR
>    remove unused #define PB_OPERATION_CONTROL_SEQ_OFF
>    fix a #endif comment
>    simplify probe code, remove added switch statement
>    remove BUG_ON(), add error message and fix num_regulators
>
> Patch 1: document device tree bindings for ltc2978
>
> Patch 2: add two helper functions for byte pmbus byte operations
>    * byte write and byte read/modify/write
>
> Patch 3: regulator support added in 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 4: 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
>
> Alan Tull (4):
>    hwmon: ltc2978: device tree bindings documentation
>    pmbus: core: add helpers for byte write and read modify write
>    pmbus: add regulator support
>    pmbus: ltc2978: add regulator support
>
>   .../devicetree/bindings/hwmon/ltc2978.txt          |   42 +++++
>   drivers/hwmon/pmbus/Kconfig                        |    7 +
>   drivers/hwmon/pmbus/ltc2978.c                      |   37 +++++
>   drivers/hwmon/pmbus/pmbus.h                        |   26 ++++
>   drivers/hwmon/pmbus/pmbus_core.c                   |  160 ++++++++++++++++++++
>   include/linux/i2c/pmbus.h                          |    4 +
>   6 files changed, 276 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt
>

Overall looks ok. There is a whitespace error in patch #1, and the revision history
should be after --- since we don't want it in the commit log. I fixed those up,
and applied the series to my local hwmon-next branch. I'll do some more testing
on a real system and then push it into -next. That should happen tomorrow
unless I encounter some problems.

We can take care of the Kconfig messages with a follow-up patch.

Thanks,
Guenter