[PATCH v6 0/4] pmbus: ltc2978: add regulator support

[PATCH v6 0/4] pmbus: ltc2978: add regulator support

[atull]

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

Each output has individual on/off control.

New in v6:
  * Much cleanup of the bindings document
  * s/vout_en/vout/g

Hopefully this is getting closer.  We will still have the potential problem of
repeated node names on boards on boards that have many regulators of the same
kind.  If this can be handled in the regulator layer, that might be great.  
If we handle it here, whatever we come up with will have to keep the same
name over reboots and be predictable enough to serve as regulator node names
in the DT.  I'm open for suggestions here.

This patchset now uses "regulator: of: Provide simplified DT parsing method"
which are in the next-20141014 tag of linux-next.

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

Patch 1: 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.
  * Update list of parts supported in Kconfig.
  * add regulator_desc info.
  * use same structs for all parts; set num_regulators appropriately.

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          |   39 +++++++
 drivers/hwmon/pmbus/Kconfig                        |   11 +-
 drivers/hwmon/pmbus/ltc2978.c                      |   39 ++++++-
 drivers/hwmon/pmbus/pmbus.h                        |   30 +++++
 drivers/hwmon/pmbus/pmbus_core.c                   |  118 ++++++++++++++++++++
 include/linux/i2c/pmbus.h                          |    4 +
 6 files changed, 238 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/ltc2978.txt

-- 
1.7.9.5

[PATCH v6 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>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Mark Brown <broonie@kernel.org>
---
v2: clean whitespace

v3: list compatible strings in single column
    add vendor as lltc
    mention regulators.txt instead of documenting regulator-name
    s/vout_en/vout/
    remove size_cells, address-cells
    use node name to identify regulators
---
 .../devicetree/bindings/hwmon/ltc2978.txt          |   39 ++++++++++++++++++++
 1 file changed, 39 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..16c1227
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
@@ -0,0 +1,39 @@
+ltc2978
+
+Required properties:
+- compatible: should contain one of:
+  * "lltc,ltc2974"
+  * "lltc,ltc2977"
+  * "lltc,ltc2978"
+  * "lltc,ltc3880"
+  * "lltc,ltc3883"
+  * "lltc,ltm4676"
+- reg: I2C slave address
+
+Optional properties:
+- regulators: A node that houses a sub-node for each regulator controlled by
+  the device. Each sub-node is identified using the node's name, with valid
+  values listed below. The content of each sub-node is defined by the
+  standard binding for regulators; see regulator.txt.
+
+Valid names of regulators depend on number of supplies supported per device:
+  * ltc2974 : vout0 - vout3
+  * ltc2977 : vout0 - vout7
+  * ltc2978 : vout0 - vout7
+  * ltc3880 : vout0 - vout1
+  * ltc3883 : vout0
+  * ltm4676 : vout0 - vout1
+
+Example:
+ltc2978@5e {
+	compatible = "ltc2978";
+	reg = <0x5e>;
+	regulators {
+		vout0 {
+			regulator-name = "FPGA-2.5V";
+		};
+		vout2 {
+			regulator-name = "FPGA-1.5V";
+		};
+	};
+};
-- 
1.7.9.5

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

[atull]

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

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

Signed-off-by: Alan Tull <atull@opensource.altera.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Mark Brown <broonie@kernel.org>
---
v2: add prototypes for the two new functions
---
 drivers/hwmon/pmbus/pmbus.h      |    4 ++++
 drivers/hwmon/pmbus/pmbus_core.c |   31 +++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index fa9beb3..3ae79a7 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -375,6 +375,10 @@ int pmbus_read_word_data(struct i2c_client *client, u8 page, u8 reg);
 int pmbus_write_word_data(struct i2c_client *client, u8 page, u8 reg, u16 word);
 int pmbus_read_byte_data(struct i2c_client *client, int page, u8 reg);
 int pmbus_write_byte(struct i2c_client *client, int page, u8 value);
+int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg,
+			  u8 value);
+int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
+			   u8 mask, u8 value);
 void pmbus_clear_faults(struct i2c_client *client);
 bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
 bool pmbus_check_word_register(struct i2c_client *client, int page, int reg);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 291d11f..d6c3701 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -253,6 +253,37 @@ int pmbus_read_byte_data(struct i2c_client *client, int page, u8 reg)
 }
 EXPORT_SYMBOL_GPL(pmbus_read_byte_data);
 
+int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value)
+{
+	int rv;
+
+	rv = pmbus_set_page(client, page);
+	if (rv < 0)
+		return rv;
+
+	return i2c_smbus_write_byte_data(client, reg, value);
+}
+EXPORT_SYMBOL_GPL(pmbus_write_byte_data);
+
+int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
+			   u8 mask, u8 value)
+{
+	unsigned int tmp;
+	int rv;
+
+	rv = pmbus_read_byte_data(client, page, reg);
+	if (rv < 0)
+		return rv;
+
+	tmp = (rv & ~mask) | (value & mask);
+
+	if (tmp != rv)
+		rv = pmbus_write_byte_data(client, page, reg, tmp);
+
+	return rv;
+}
+EXPORT_SYMBOL_GPL(pmbus_update_byte_data);
+
 /*
  * _pmbus_read_byte_data() is similar to pmbus_read_byte_data(), but checks if
  * a device specific mapping function exists and calls it if necessary.
-- 
1.7.9.5

[PATCH v6 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 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 initialized from either
platform data or the device tree.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
Cc: Guenter Roeck <linux@roeck-us.net>
---
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

v5: use "regulator: of: Provide simplified DT parsing method"
    remove #include <linux/regulator/of_regulator.h>
    remove of_regulator_match structure in pmbus_driver_info
    add of_match and regulators_node for regulator_desc macro
    remove pmbus_regulator_parse_dt
    remove unneeded 'else' that set init_data and of_node
---
 drivers/hwmon/pmbus/pmbus.h      |   26 ++++++++++++
 drivers/hwmon/pmbus/pmbus_core.c |   87 ++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/pmbus.h        |    4 ++
 3 files changed, 117 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 3ae79a7..89a23ff 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
 
@@ -186,6 +188,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 +372,27 @@ 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;
 };
 
+/* 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),					\
+		.of_match = of_match_ptr(_name # _id),		\
+		.regulators_node = of_match_ptr("regulators"),	\
+		.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..f2e47c7 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"
 
 /*
@@ -1758,6 +1760,84 @@ 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);
+
+static int pmbus_regulator_register(struct pmbus_data *data)
+{
+	struct device *dev = data->dev;
+	const struct pmbus_driver_info *info = data->info;
+	const struct pmbus_platform_data *pdata = dev_get_platdata(dev);
+	struct regulator_dev *rdev;
+	int i;
+
+	for (i = 0; i < info->num_regulators; i++) {
+		struct regulator_config config = { };
+
+		config.dev = dev;
+		config.driver_data = data;
+
+		if (pdata && pdata->reg_init_data)
+			config.init_data = &pdata->reg_init_data[i];
+
+		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 +1892,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

[PATCH v6 4/4] pmbus: ltc2978: add regulator support

[atull]

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

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

Signed-off-by: Alan Tull <atull@opensource.altera.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Mark Brown <broonie@kernel.org>
---
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: fix a #endif comment
    simplify probe code, remove added switch statement
    remove BUG_ON(), add error message and fix num_regulators

v5: Kconfig: update list of supported chips
    use "regulator: of: Provide simplified DT parsing method"
    remove #include <linux/regulator/of_regulator.h>
    remove of_regulator_match

v6: add compatible strings with vendor code
    s/vout_en/vout/
---
 drivers/hwmon/pmbus/Kconfig   |   11 +++++++++--
 drivers/hwmon/pmbus/ltc2978.c |   39 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 6e1e493..a674cd8 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -47,15 +47,22 @@ config SENSORS_LM25066
 	  be called lm25066.
 
 config SENSORS_LTC2978
-	tristate "Linear Technologies LTC2974, LTC2978, LTC3880, and LTC3883"
+	tristate "Linear Technologies LTC2978 and compatibles"
 	default n
 	help
 	  If you say yes here you get hardware monitoring support for Linear
-	  Technology LTC2974, LTC2978, LTC3880, and LTC3883.
+	  Technology LTC2974, LTC2977, LTC2978, LTC3880, LTC3883, and LTM4676.
 
 	  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 LTC2978 and compatibles"
+	depends on SENSORS_LTC2978 && REGULATOR
+	help
+	  If you say yes here you get regulator support for Linear
+	  Technology LTC2974, LTC2977, LTC2978, LTC3880, LTC3883, and LTM4676.
+
 config SENSORS_MAX16064
 	tristate "Maxim MAX16064"
 	default n
diff --git a/drivers/hwmon/pmbus/ltc2978.c b/drivers/hwmon/pmbus/ltc2978.c
index e24ed52..0835050 100644
--- a/drivers/hwmon/pmbus/ltc2978.c
+++ b/drivers/hwmon/pmbus/ltc2978.c
@@ -22,6 +22,7 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
+#include <linux/regulator/driver.h>
 #include "pmbus.h"
 
 enum chips { ltc2974, ltc2977, ltc2978, ltc3880, ltc3883, ltm4676 };
@@ -374,6 +375,19 @@ static const struct i2c_device_id ltc2978_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ltc2978_id);
 
+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+static const struct regulator_desc ltc2978_reg_desc[] = {
+	PMBUS_REGULATOR("vout", 0),
+	PMBUS_REGULATOR("vout", 1),
+	PMBUS_REGULATOR("vout", 2),
+	PMBUS_REGULATOR("vout", 3),
+	PMBUS_REGULATOR("vout", 4),
+	PMBUS_REGULATOR("vout", 5),
+	PMBUS_REGULATOR("vout", 6),
+	PMBUS_REGULATOR("vout", 7),
+};
+#endif /* CONFIG_SENSORS_LTC2978_REGULATOR */
+
 static int ltc2978_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -487,13 +501,36 @@ static int ltc2978_probe(struct i2c_client *client,
 	default:
 		return -ENODEV;
 	}
+
+#if IS_ENABLED(CONFIG_SENSORS_LTC2978_REGULATOR)
+	info->num_regulators = info->pages;
+	info->reg_desc = ltc2978_reg_desc;
+	if (info->num_regulators > ARRAY_SIZE(ltc2978_reg_desc)) {
+		dev_err(&client->dev, "num_regulators too large!");
+		info->num_regulators = ARRAY_SIZE(ltc2978_reg_desc);
+	}
+#endif
+
 	return pmbus_do_probe(client, id, info);
 }
 
-/* This is the driver that will be inserted */
+#ifdef CONFIG_OF
+static const struct of_device_id ltc2978_of_match[] = {
+	{ .compatible = "lltc,ltc2974" },
+	{ .compatible = "lltc,ltc2977" },
+	{ .compatible = "lltc,ltc2978" },
+	{ .compatible = "lltc,ltc3880" },
+	{ .compatible = "lltc,ltc3883" },
+	{ .compatible = "lltc,ltm4676" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ltc2978_of_match);
+#endif
+
 static struct i2c_driver ltc2978_driver = {
 	.driver = {
 		   .name = "ltc2978",
+		   .of_match_table = of_match_ptr(ltc2978_of_match),
 		   },
 	.probe = ltc2978_probe,
 	.remove = pmbus_do_remove,
-- 
1.7.9.5

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

[Guenter Roeck]

On Wed, Oct 15, 2014 at 01:55:09PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add support for simple on/off control of each channel.
> 
> To add regulator support, the pmbus part driver needs to add
> regulator_desc information 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 initialized from either
> platform data or the device tree.
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> Reviewed-by: Mark Brown <broonie@kernel.org>
> Cc: Guenter Roeck <linux@roeck-us.net>

Hi Alan,

I am still seeing lots of the following:

vout0: Failed to create debugfs directory
vout1: Failed to create debugfs directory
vout2: Failed to create debugfs directory
vout3: Failed to create debugfs directory
vout4: Failed to create debugfs directory
vout5: Failed to create debugfs directory
vout6: Failed to create debugfs directory
vout7: Failed to create debugfs directory

I thought there was a problem in the regulator core, but after looking
into it concluded that the regulator core _should_ prepend the names
with the device name when creating the debugfs entries, unless no device
name is specified. So something must be missing. We'll need to sort
this out before I can accept the code.

Thanks,
Guenter

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

[atull]

On Thu, 16 Oct 2014, Guenter Roeck wrote:

> On Wed, Oct 15, 2014 at 01:55:09PM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > Add support for simple on/off control of each channel.
> > 
> > To add regulator support, the pmbus part driver needs to add
> > regulator_desc information 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 initialized from either
> > platform data or the device tree.
> > 
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > Reviewed-by: Mark Brown <broonie@kernel.org>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> 
> Hi Alan,
> 
> I am still seeing lots of the following:
> 
> vout0: Failed to create debugfs directory
> vout1: Failed to create debugfs directory
> vout2: Failed to create debugfs directory
> vout3: Failed to create debugfs directory
> vout4: Failed to create debugfs directory
> vout5: Failed to create debugfs directory
> vout6: Failed to create debugfs directory
> vout7: Failed to create debugfs directory
> 
> I thought there was a problem in the regulator core, but after looking
> into it concluded that the regulator core _should_ prepend the names
> with the device name when creating the debugfs entries, unless no device
> name is specified. So something must be missing. We'll need to sort
> this out before I can accept the code.
> 
> Thanks,
> Guenter
> 

Hi Guenter,

OK, I will look into it.

Alan

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

[Guenter Roeck]

On Thu, Oct 16, 2014 at 01:40:23PM -0500, atull wrote:
> On Thu, 16 Oct 2014, Guenter Roeck wrote:
> 
> > On Wed, Oct 15, 2014 at 01:55:09PM -0500, atull@opensource.altera.com wrote:
> > > From: Alan Tull <atull@opensource.altera.com>
> > > 
> > > Add support for simple on/off control of each channel.
> > > 
> > > To add regulator support, the pmbus part driver needs to add
> > > regulator_desc information 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 initialized from either
> > > platform data or the device tree.
> > > 
> > > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > > Reviewed-by: Mark Brown <broonie@kernel.org>
> > > Cc: Guenter Roeck <linux@roeck-us.net>
> > 
> > Hi Alan,
> > 
> > I am still seeing lots of the following:
> > 
> > vout0: Failed to create debugfs directory
> > vout1: Failed to create debugfs directory
> > vout2: Failed to create debugfs directory
> > vout3: Failed to create debugfs directory
> > vout4: Failed to create debugfs directory
> > vout5: Failed to create debugfs directory
> > vout6: Failed to create debugfs directory
> > vout7: Failed to create debugfs directory
> > 
> > I thought there was a problem in the regulator core, but after looking
> > into it concluded that the regulator core _should_ prepend the names
> > with the device name when creating the debugfs entries, unless no device
> > name is specified. So something must be missing. We'll need to sort
> > this out before I can accept the code.
> > 
> > Thanks,
> > Guenter
> > 
> 
> Hi Guenter,
> 
> OK, I will look into it.
> 
Hi Alan,

I tracked it down a bit further. It is a regulator core problem after all.
Turns out the parent device name is not _always_ used when creating
debugfs directories. The culprit is rdev_init_debugfs, which just takes
rdev_get_name() to create the name. I am currently playing with it.

Problem is that if I just prepend the parent device name (if available)
in rdev_init_debugfs, I get something like the following.

2-005c-pmb-vout0     5-005d-vout6  5-005f-vout5  5-0061-vout4
2-005c-pmb-vout1     5-005d-vout7  5-005f-vout6  5-0061-vout5
2-005c-pmb-vout2     5-005e-vout0  5-005f-vout7  5-0061-vout6
2-005c-vout3         5-005e-vout1  5-0060-vout0  5-0061-vout7
2-005c-vout4         5-005e-vout2  5-0060-vout1  5-0062-vout0
2-005c-vout5         5-005e-vout3  5-0060-vout2  5-0062-vout1
2-005c-vout6         5-005e-vout4  5-0060-vout3  5-0062-vout2
2-005c-vout7         5-005e-vout5  5-0060-vout4  5-0062-vout3
3p3v.11-3P3V         5-005e-vout6  5-0060-vout5  5-0062-vout4
5-005d-fpc-5d-vout0  5-005e-vout7  5-0060-vout6  5-0062-vout5
5-005d-fpc-5d-vout1  5-005f-vout0  5-0060-vout7  5-0062-vout6
5-005d-vout2         5-005f-vout1  5-0061-vout0  5-0062-vout7
5-005d-vout3         5-005f-vout2  5-0061-vout1  reg-dummy-regulator-dummy
5-005d-vout4         5-005f-vout3  5-0061-vout2  supply_map
5-005d-vout5         5-005f-vout4  5-0061-vout3

This is a bit better but creates names such as "3p3v.11-3P3V"
and "reg-dummy-regulator-dummy" which doesn't make much sense
either. So I think we'll need something better than that.

Guenter

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

[Guenter Roeck]

On Wed, Oct 15, 2014 at 01:55:07PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add device tree bindings documentation for ltc2978.
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Mark Brown <broonie@kernel.org>
> ---
> v2: clean whitespace
> 
> v3: list compatible strings in single column
>     add vendor as lltc
>     mention regulators.txt instead of documenting regulator-name
>     s/vout_en/vout/
>     remove size_cells, address-cells
>     use node name to identify regulators
> ---
>  .../devicetree/bindings/hwmon/ltc2978.txt          |   39 ++++++++++++++++++++
>  1 file changed, 39 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..16c1227
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/ltc2978.txt
> @@ -0,0 +1,39 @@
> +ltc2978
> +
> +Required properties:
> +- compatible: should contain one of:
> +  * "lltc,ltc2974"
> +  * "lltc,ltc2977"
> +  * "lltc,ltc2978"
> +  * "lltc,ltc3880"
> +  * "lltc,ltc3883"
> +  * "lltc,ltm4676"
> +- reg: I2C slave address
> +
> +Optional properties:
> +- regulators: A node that houses a sub-node for each regulator controlled by
> +  the device. Each sub-node is identified using the node's name, with valid
> +  values listed below. The content of each sub-node is defined by the
> +  standard binding for regulators; see regulator.txt.
> +
> +Valid names of regulators depend on number of supplies supported per device:
> +  * ltc2974 : vout0 - vout3
> +  * ltc2977 : vout0 - vout7
> +  * ltc2978 : vout0 - vout7
> +  * ltc3880 : vout0 - vout1
> +  * ltc3883 : vout0
> +  * ltm4676 : vout0 - vout1
> +
> +Example:
> +ltc2978@5e {
> +	compatible = "ltc2978";

Nitpick, but still:

		"lltc,ltc2978"

> +	reg = <0x5e>;
> +	regulators {
> +		vout0 {
> +			regulator-name = "FPGA-2.5V";
> +		};
> +		vout2 {
> +			regulator-name = "FPGA-1.5V";
> +		};
> +	};
> +};
> -- 
> 1.7.9.5
> 

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

[Guenter Roeck]

On Thu, Oct 16, 2014 at 01:40:23PM -0500, atull wrote:
> On Thu, 16 Oct 2014, Guenter Roeck wrote:
> 
> > On Wed, Oct 15, 2014 at 01:55:09PM -0500, atull@opensource.altera.com wrote:
> > > From: Alan Tull <atull@opensource.altera.com>
> > > 
> > > Add support for simple on/off control of each channel.
> > > 
> > > To add regulator support, the pmbus part driver needs to add
> > > regulator_desc information 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 initialized from either
> > > platform data or the device tree.
> > > 
> > > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > > Reviewed-by: Mark Brown <broonie@kernel.org>
> > > Cc: Guenter Roeck <linux@roeck-us.net>
> > 
> > Hi Alan,
> > 
> > I am still seeing lots of the following:
> > 
> > vout0: Failed to create debugfs directory
> > vout1: Failed to create debugfs directory
> > vout2: Failed to create debugfs directory
> > vout3: Failed to create debugfs directory
> > vout4: Failed to create debugfs directory
> > vout5: Failed to create debugfs directory
> > vout6: Failed to create debugfs directory
> > vout7: Failed to create debugfs directory
> > 
> > I thought there was a problem in the regulator core, but after looking
> > into it concluded that the regulator core _should_ prepend the names
> > with the device name when creating the debugfs entries, unless no device
> > name is specified. So something must be missing. We'll need to sort
> > this out before I can accept the code.
> > 
> > Thanks,
> > Guenter
> > 
> 
> Hi Guenter,
> 
> OK, I will look into it.
> 

Can you try the following patch ? It may not be perfect, but it does
the job for me.

Mark, would that patch be acceptable ?

Thanks,
Guenter
---
From: Guenter Roeck <linux@roeck-us.net>
Date: Thu, 16 Oct 2014 13:08:35 -0700
Subject: [PATCH] regulator: Ensure unique regulator debugfs directory names

If multiple regulator devices of the same type exist in a system,
the regulator driver assigns generic names for the regulators it
provides, and debugfs is enabled, the regulator subsystem attempts
to create multiple entries with the same name in the regulator debugfs
directory. This fails for all but the first regulator, resulting in
multiple "Failed to create debugfs directory" log entries.

To avoid the problem, prepend the debugfs directory name for a regulator
with its parent device name if available, but only if no explicit
regulator name was provided.

Cc: Alan Tull <atull@opensource.altera.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/regulator/core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index cd87c0c..92f7a53 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3538,7 +3538,18 @@ static int add_regulator_attributes(struct regulator_dev *rdev)
 
 static void rdev_init_debugfs(struct regulator_dev *rdev)
 {
-	rdev->debugfs = debugfs_create_dir(rdev_get_name(rdev), debugfs_root);
+	struct device *parent = rdev->dev.parent;
+	const char *rname = rdev_get_name(rdev);
+	char name[NAME_MAX];
+
+	/* Avoid duplicate debugfs directory names */
+	if (parent && rname == rdev->desc->name) {
+		snprintf(name, sizeof(name), "%s-%s", dev_name(parent),
+			 rname);
+		rname = name;
+	}
+
+	rdev->debugfs = debugfs_create_dir(rname, debugfs_root);
 	if (!rdev->debugfs) {
 		rdev_warn(rdev, "Failed to create debugfs directory\n");
 		return;
-- 
1.9.1

Re: [PATCH v6 0/4] pmbus: ltc2978: add regulator support

[Guenter Roeck]

On Wed, Oct 15, 2014 at 01:55:06PM -0500, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Each output has individual on/off control.
> 
> New in v6:
>   * Much cleanup of the bindings document
>   * s/vout_en/vout/g
> 
> Hopefully this is getting closer.  We will still have the potential problem of
> repeated node names on boards on boards that have many regulators of the same
> kind.  If this can be handled in the regulator layer, that might be great.  
> If we handle it here, whatever we come up with will have to keep the same
> name over reboots and be predictable enough to serve as regulator node names
> in the DT.  I'm open for suggestions here.
> 
> This patchset now uses "regulator: of: Provide simplified DT parsing method"
> which are in the next-20141014 tag of linux-next.
> 
Hi Alan,

Since the problems we have seen are in the regulator core code and not
in your patches, I added the series to -next. I fixed up the 'compatible'
example in the devicetree description, so there is no need to resubmit
the patches.

Thanks,
Guenter

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

[atull]

On Thu, 16 Oct 2014, Guenter Roeck wrote:

> On Thu, Oct 16, 2014 at 01:40:23PM -0500, atull wrote:
> > On Thu, 16 Oct 2014, Guenter Roeck wrote:
> > 
> > > On Wed, Oct 15, 2014 at 01:55:09PM -0500, atull@opensource.altera.com wrote:
> > > > From: Alan Tull <atull@opensource.altera.com>
> > > > 
> > > > Add support for simple on/off control of each channel.
> > > > 
> > > > To add regulator support, the pmbus part driver needs to add
> > > > regulator_desc information 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 initialized from either
> > > > platform data or the device tree.
> > > > 
> > > > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> > > > Reviewed-by: Mark Brown <broonie@kernel.org>
> > > > Cc: Guenter Roeck <linux@roeck-us.net>
> > > 
> > > Hi Alan,
> > > 
> > > I am still seeing lots of the following:
> > > 
> > > vout0: Failed to create debugfs directory
> > > vout1: Failed to create debugfs directory
> > > vout2: Failed to create debugfs directory
> > > vout3: Failed to create debugfs directory
> > > vout4: Failed to create debugfs directory
> > > vout5: Failed to create debugfs directory
> > > vout6: Failed to create debugfs directory
> > > vout7: Failed to create debugfs directory
> > > 
> > > I thought there was a problem in the regulator core, but after looking
> > > into it concluded that the regulator core _should_ prepend the names
> > > with the device name when creating the debugfs entries, unless no device
> > > name is specified. So something must be missing. We'll need to sort
> > > this out before I can accept the code.
> > > 
> > > Thanks,
> > > Guenter
> > > 
> > 
> > Hi Guenter,
> > 
> > OK, I will look into it.
> > 
> 
> Can you try the following patch ? It may not be perfect, but it does
> the job for me.
> 
> Mark, would that patch be acceptable ?
> 
> Thanks,
> Guenter
> ---
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Thu, 16 Oct 2014 13:08:35 -0700
> Subject: [PATCH] regulator: Ensure unique regulator debugfs directory names
> 
> If multiple regulator devices of the same type exist in a system,
> the regulator driver assigns generic names for the regulators it
> provides, and debugfs is enabled, the regulator subsystem attempts
> to create multiple entries with the same name in the regulator debugfs
> directory. This fails for all but the first regulator, resulting in
> multiple "Failed to create debugfs directory" log entries.
> 
> To avoid the problem, prepend the debugfs directory name for a regulator
> with its parent device name if available, but only if no explicit
> regulator name was provided.
> 
> Cc: Alan Tull <atull@opensource.altera.com>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/regulator/core.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index cd87c0c..92f7a53 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -3538,7 +3538,18 @@ static int add_regulator_attributes(struct regulator_dev *rdev)
>  
>  static void rdev_init_debugfs(struct regulator_dev *rdev)
>  {
> -	rdev->debugfs = debugfs_create_dir(rdev_get_name(rdev), debugfs_root);
> +	struct device *parent = rdev->dev.parent;
> +	const char *rname = rdev_get_name(rdev);
> +	char name[NAME_MAX];
> +
> +	/* Avoid duplicate debugfs directory names */
> +	if (parent && rname == rdev->desc->name) {
> +		snprintf(name, sizeof(name), "%s-%s", dev_name(parent),
> +			 rname);
> +		rname = name;
> +	}
> +
> +	rdev->debugfs = debugfs_create_dir(rname, debugfs_root);
>  	if (!rdev->debugfs) {
>  		rdev_warn(rdev, "Failed to create debugfs directory\n");
>  		return;
> -- 
> 1.9.1
> 

Hi Guenter,
 
I tried it out on a board with two ltc2978's.  It looked really good to 
me.  It solves the debugfs issue without affecting the DT.

root@socfpga_cyclone5:~# ls /sys/kernel/debug/regulator/
0-005c-vout0               0-005c-vout6               0-005e-vout7
0-005c-vout1               0-005c-vout7               FPGA-1.1V
0-005c-vout2               0-005e-vout1               FPGA-1.5V
0-005c-vout3               0-005e-vout3               FPGA-2.5V
0-005c-vout4               0-005e-vout5               
reg-dummy-regulator-dummy
0-005c-vout5               0-005e-vout6               supply_map

Alan

Re: [PATCH v6 0/4] pmbus: ltc2978: add regulator support

[atull]

On Thu, 16 Oct 2014, Guenter Roeck wrote:

> On Wed, Oct 15, 2014 at 01:55:06PM -0500, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> > 
> > Each output has individual on/off control.
> > 
> > New in v6:
> >   * Much cleanup of the bindings document
> >   * s/vout_en/vout/g
> > 
> > Hopefully this is getting closer.  We will still have the potential problem of
> > repeated node names on boards on boards that have many regulators of the same
> > kind.  If this can be handled in the regulator layer, that might be great.  
> > If we handle it here, whatever we come up with will have to keep the same
> > name over reboots and be predictable enough to serve as regulator node names
> > in the DT.  I'm open for suggestions here.
> > 
> > This patchset now uses "regulator: of: Provide simplified DT parsing method"
> > which are in the next-20141014 tag of linux-next.
> > 
> Hi Alan,
> 
> Since the problems we have seen are in the regulator core code and not
> in your patches, I added the series to -next. I fixed up the 'compatible'
> example in the devicetree description, so there is no need to resubmit
> the patches.
> 
> Thanks,
> Guenter
> 

Hi Guenter,

Thanks!

Alan

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

[Guenter Roeck]

On 10/17/2014 07:48 AM, atull wrote:
> On Thu, 16 Oct 2014, Guenter Roeck wrote:
>
>> On Thu, Oct 16, 2014 at 01:40:23PM -0500, atull wrote:
>>> On Thu, 16 Oct 2014, Guenter Roeck wrote:
>>>
>>>> On Wed, Oct 15, 2014 at 01:55:09PM -0500, atull@opensource.altera.com wrote:
>>>>> From: Alan Tull <atull@opensource.altera.com>
>>>>>
>>>>> Add support for simple on/off control of each channel.
>>>>>
>>>>> To add regulator support, the pmbus part driver needs to add
>>>>> regulator_desc information 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 initialized from either
>>>>> platform data or the device tree.
>>>>>
>>>>> Signed-off-by: Alan Tull <atull@opensource.altera.com>
>>>>> Reviewed-by: Mark Brown <broonie@kernel.org>
>>>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>>>
>>>> Hi Alan,
>>>>
>>>> I am still seeing lots of the following:
>>>>
>>>> vout0: Failed to create debugfs directory
>>>> vout1: Failed to create debugfs directory
>>>> vout2: Failed to create debugfs directory
>>>> vout3: Failed to create debugfs directory
>>>> vout4: Failed to create debugfs directory
>>>> vout5: Failed to create debugfs directory
>>>> vout6: Failed to create debugfs directory
>>>> vout7: Failed to create debugfs directory
>>>>
>>>> I thought there was a problem in the regulator core, but after looking
>>>> into it concluded that the regulator core _should_ prepend the names
>>>> with the device name when creating the debugfs entries, unless no device
>>>> name is specified. So something must be missing. We'll need to sort
>>>> this out before I can accept the code.
>>>>
>>>> Thanks,
>>>> Guenter
>>>>
>>>
>>> Hi Guenter,
>>>
>>> OK, I will look into it.
>>>
>>
>> Can you try the following patch ? It may not be perfect, but it does
>> the job for me.
>>
>> Mark, would that patch be acceptable ?
>>
>> Thanks,
>> Guenter
>> ---
>> From: Guenter Roeck <linux@roeck-us.net>
>> Date: Thu, 16 Oct 2014 13:08:35 -0700
>> Subject: [PATCH] regulator: Ensure unique regulator debugfs directory names
>>
>> If multiple regulator devices of the same type exist in a system,
>> the regulator driver assigns generic names for the regulators it
>> provides, and debugfs is enabled, the regulator subsystem attempts
>> to create multiple entries with the same name in the regulator debugfs
>> directory. This fails for all but the first regulator, resulting in
>> multiple "Failed to create debugfs directory" log entries.
>>
>> To avoid the problem, prepend the debugfs directory name for a regulator
>> with its parent device name if available, but only if no explicit
>> regulator name was provided.
>>
>> Cc: Alan Tull <atull@opensource.altera.com>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/regulator/core.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
>> index cd87c0c..92f7a53 100644
>> --- a/drivers/regulator/core.c
>> +++ b/drivers/regulator/core.c
>> @@ -3538,7 +3538,18 @@ static int add_regulator_attributes(struct regulator_dev *rdev)
>>
>>   static void rdev_init_debugfs(struct regulator_dev *rdev)
>>   {
>> -	rdev->debugfs = debugfs_create_dir(rdev_get_name(rdev), debugfs_root);
>> +	struct device *parent = rdev->dev.parent;
>> +	const char *rname = rdev_get_name(rdev);
>> +	char name[NAME_MAX];
>> +
>> +	/* Avoid duplicate debugfs directory names */
>> +	if (parent && rname == rdev->desc->name) {
>> +		snprintf(name, sizeof(name), "%s-%s", dev_name(parent),
>> +			 rname);
>> +		rname = name;
>> +	}
>> +
>> +	rdev->debugfs = debugfs_create_dir(rname, debugfs_root);
>>   	if (!rdev->debugfs) {
>>   		rdev_warn(rdev, "Failed to create debugfs directory\n");
>>   		return;
>> --
>> 1.9.1
>>
>
> Hi Guenter,
>
> I tried it out on a board with two ltc2978's.  It looked really good to
> me.  It solves the debugfs issue without affecting the DT.
>
> root@socfpga_cyclone5:~# ls /sys/kernel/debug/regulator/
> 0-005c-vout0               0-005c-vout6               0-005e-vout7
> 0-005c-vout1               0-005c-vout7               FPGA-1.1V
> 0-005c-vout2               0-005e-vout1               FPGA-1.5V
> 0-005c-vout3               0-005e-vout3               FPGA-2.5V
> 0-005c-vout4               0-005e-vout5
> reg-dummy-regulator-dummy
> 0-005c-vout5               0-005e-vout6               supply_map
>
> Alan
>
Great, thanks a lot for testing.

I'll submit the patch officially. Let's see if Mark accepts it.

Guenter