[PATCH 0/4] Add support for the TI TPS65086 PMIC.

[PATCH 0/4] Add support for the TI TPS65086 PMIC.

[Andrew F. Davis]

This series adds support for the TPS65086 PMIC. It is a MFD with an I2C
interface, several regulators and load switches, and a GPO controller.

Based on v4.3-rc1 and needs b4fe8ba ("regmap: Add generic macro to define regmap_irq")
from Lee Jones' tree to build.

Andrew F. Davis (4):
  Documentation: tps65086: Add DT bindings for the TPS65086 PMIC
  mfd: tps65086: Add driver for the TPS65086 PMIC
  regulator: tps65086: Add regulator driver for the TPS65086 PMIC
  gpio: tps65086: Add GPIO driver for the TPS65086 PMIC

 .../devicetree/bindings/gpio/gpio-tps65086.txt     |  17 ++
 Documentation/devicetree/bindings/mfd/tps65086.txt |  46 ++++
 .../bindings/regulator/tps65086-regulator.txt      |  36 +++
 drivers/gpio/Kconfig                               |   6 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-tps65086.c                       | 114 +++++++++
 drivers/mfd/Kconfig                                |  13 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/tps65086.c                             | 145 ++++++++++++
 drivers/regulator/Kconfig                          |   7 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/tps65086-regulator.c             | 263 +++++++++++++++++++++
 include/linux/mfd/tps65086.h                       | 120 ++++++++++
 13 files changed, 770 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt
 create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt
 create mode 100644 drivers/gpio/gpio-tps65086.c
 create mode 100644 drivers/mfd/tps65086.c
 create mode 100644 drivers/regulator/tps65086-regulator.c
 create mode 100644 include/linux/mfd/tps65086.h

-- 
1.9.1

[PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Andrew F. Davis]

The TPS65086 PMIC contains several regulators and a GPO controller.
Add bindings for the TPS65086 PMIC.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 .../devicetree/bindings/gpio/gpio-tps65086.txt     | 17 ++++++++
 Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++
 .../bindings/regulator/tps65086-regulator.txt      | 36 +++++++++++++++++
 3 files changed, 99 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt
 create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-tps65086.txt b/Documentation/devicetree/bindings/gpio/gpio-tps65086.txt
new file mode 100644
index 0000000..15e4063
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-tps65086.txt
@@ -0,0 +1,17 @@
+* TPS65086 GPO Controller bindings
+
+Required properties:
+ - compatible		: Should be "ti,tps65086-gpio".
+ - gpio-controller	: Marks the device node as a GPIO Controller.
+ - #gpio-cells		: Should be two.  The first cell is the pin number
+			  and the second cell is used to specify flags.
+			  See include/dt-bindings/gpio/gpio.h for possible
+			  values.
+
+Example:
+
+	gpio4: tps65086_gpio {
+		compatible = "ti,tps65086-gpio";
+		gpio-controller;
+		#gpio-cells = <2>;
+	};
diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt
new file mode 100644
index 0000000..4b6aeb4
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/tps65086.txt
@@ -0,0 +1,46 @@
+* TPS65086 Power Management Integrated Circuit bindings
+
+Required properties:
+ - compatible		: Should be "ti,tps65086".
+ - reg			: Slave address.
+ - interrupt-parent	: The parent interrupt controller.
+ - interrupts		: The interrupt line the device is connected to.
+ - interrupt-controller	: Marks the device node as an interrupt controller.
+ - #interrupt-cells	: The number of cells to describe an IRQ, this
+			  should be 2. The first cell is the IRQ number.
+			  The second cell is the flags, encoded as the trigger
+			  masks from ../interrupt-controller/interrupts.txt.
+
+Additional nodes defined in:
+ - Regulators	: ../regulator/tps65086-regulator.txt.
+ - GPIO		: ../gpio/gpio-tps65086.txt.
+
+Example:
+
+	pmic: tps65086@5e {
+		compatible = "ti,tps65086";
+		reg = <0x5e>;
+		interrupt-parent = <&gpio1>;
+		interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+
+		regulators {
+			compatible = "ti,tps65086-regulator";
+
+			buck1 {
+				regulator-name = "vcc1";
+				regulator-min-microvolt = <1600000>;
+				regulator-max-microvolt = <1600000>;
+				regulator-boot-on;
+				ti,regulator-decay;
+				ti,regulator-step-size-25mv;
+			};
+		};
+
+		gpio4: tps65086_gpio {
+			compatible = "ti,tps65086-gpio";
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+	};
diff --git a/Documentation/devicetree/bindings/regulator/tps65086-regulator.txt b/Documentation/devicetree/bindings/regulator/tps65086-regulator.txt
new file mode 100644
index 0000000..023cd73
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/tps65086-regulator.txt
@@ -0,0 +1,36 @@
+* TPS65086 regulator bindings
+
+Required properties:
+ - compatible	: Should be "ti,tps65086-regulator".
+ - list of regulators provided by this controller, must be named
+     after their hardware counterparts: buck[1-6], ldoa[1-3], swa1,
+     swb[1-2], and vtt.
+
+Optional properties:
+ - Per-regulator optional properties are defined in regulator.txt
+ - ti,regulator-step-size-25mv	: This is applicable for buck[1,2,6].
+				  Set this if the regulator is factory
+				  set with a 25mv step voltage mapping.
+ - ti,regulator-decay		: This is applicable for buck[1-6].
+				  Set this if the output needs to decay,
+				  default is for the output to slew down.
+
+Example:
+
+	regulators {
+		compatible = "ti,tps65086-regulator";
+
+		buck1 {
+			regulator-name = "vcc1";
+			regulator-min-microvolt = <1600000>;
+			regulator-max-microvolt = <1600000>;
+			regulator-boot-on;
+			ti,regulator-step-size-25mv;
+			ti,regulator-decay;
+		};
+
+		swa1 {
+			regulator-name = "ls1";
+			regulator-boot-on;
+		};
+	};
-- 
1.9.1

[PATCH 2/4] mfd: tps65086: Add driver for the TPS65086 PMIC

[Andrew F. Davis]

Add support for the TPS65912 device. It provides communication
through I2C and contains the following components:

 - Regulators
 - Load switches
 - GPO controller

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/mfd/Kconfig          |  13 ++++
 drivers/mfd/Makefile         |   1 +
 drivers/mfd/tps65086.c       | 145 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/tps65086.h | 120 +++++++++++++++++++++++++++++++++++
 4 files changed, 279 insertions(+)
 create mode 100644 drivers/mfd/tps65086.c
 create mode 100644 include/linux/mfd/tps65086.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 99d6367..ab3b392 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1093,6 +1093,19 @@ config TPS6507X
 	  This driver can also be built as a module.  If so, the module
 	  will be called tps6507x.
 
+config MFD_TPS65086
+	tristate "TI TPS65086x Power Management chips"
+	select REGMAP
+	select REGMAP_IRQ
+	select REGMAP_I2C
+	depends on I2C
+	depends on OF || COMPILE_TEST
+	help
+	  If you say yes here you get support for the TPS65086 series of
+	  Power Management chips.
+	  These include voltage regulators, GPO and other features
+	  that are often used in portable devices.
+
 config TPS65911_COMPARATOR
 	tristate
 
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a59e3fc..7adb902 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_MFD_WM8994)	+= wm8994-core.o wm8994-irq.o wm8994-regmap.o
 obj-$(CONFIG_TPS6105X)		+= tps6105x.o
 obj-$(CONFIG_TPS65010)		+= tps65010.o
 obj-$(CONFIG_TPS6507X)		+= tps6507x.o
+obj-$(CONFIG_MFD_TPS65086)	+= tps65086.o
 obj-$(CONFIG_MFD_TPS65217)	+= tps65217.o
 obj-$(CONFIG_MFD_TPS65218)	+= tps65218.o
 obj-$(CONFIG_MFD_TPS65910)	+= tps65910.o
diff --git a/drivers/mfd/tps65086.c b/drivers/mfd/tps65086.c
new file mode 100644
index 0000000..be6d678
--- /dev/null
+++ b/drivers/mfd/tps65086.c
@@ -0,0 +1,145 @@
+/*
+ * Driver for TI TPS65086x PMICs
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Author: Andrew F. Davis <afd@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether expressed or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License version 2 for more details.
+ *
+ * Based on the TPS65912 driver
+ */
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+
+#include <linux/mfd/tps65086.h>
+
+static const struct regmap_range tps65086_yes_ranges[] = {
+	regmap_reg_range(TPS65086_IRQ, TPS65086_IRQ),
+	regmap_reg_range(TPS65086_PMICSTAT, TPS65086_SHUTDNSRC),
+	regmap_reg_range(TPS65086_GPOCTRL, TPS65086_GPOCTRL),
+	regmap_reg_range(TPS65086_PG_STATUS1, TPS65086_OC_STATUS),
+};
+
+static const struct regmap_access_table tps65086_volatile_table = {
+	.yes_ranges = tps65086_yes_ranges,
+	.n_yes_ranges = ARRAY_SIZE(tps65086_yes_ranges),
+};
+
+static const struct regmap_config tps65086_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_RBTREE,
+	.volatile_table = &tps65086_volatile_table,
+};
+
+static const struct regmap_irq tps65086_irqs[] = {
+	REGMAP_IRQ_REG(TPS65086_IRQ_DIETEMP, 0, TPS65086_IRQ_DIETEMP_MASK),
+	REGMAP_IRQ_REG(TPS65086_IRQ_SHUTDN, 0, TPS65086_IRQ_SHUTDN_MASK),
+	REGMAP_IRQ_REG(TPS65086_IRQ_FAULT, 0, TPS65086_IRQ_FAULT_MASK),
+};
+
+static struct regmap_irq_chip tps65086_irq_chip = {
+	.name = "tps65086",
+	.status_base = TPS65086_IRQ,
+	.mask_base = TPS65086_IRQ_MASK,
+	.ack_base = TPS65086_IRQ,
+	.init_ack_masked = true,
+	.num_regs = 1,
+	.irqs = tps65086_irqs,
+	.num_irqs = ARRAY_SIZE(tps65086_irqs),
+};
+
+static const struct of_device_id tps65086_of_match_table[] = {
+	{ .compatible = "ti,tps65086", },
+	{ /* sentinel */ }
+};
+
+static int tps65086_probe(struct i2c_client *client,
+			  const struct i2c_device_id *ids)
+{
+	struct tps65086 *tps;
+	unsigned int version;
+	int ret;
+
+	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
+	if (!tps)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, tps);
+	tps->dev = &client->dev;
+	tps->irq = client->irq;
+
+	tps->regmap = devm_regmap_init_i2c(client, &tps65086_regmap_config);
+	if (IS_ERR(tps->regmap)) {
+		dev_err(tps->dev, "Failed to initialize register map\n");
+		return PTR_ERR(tps->regmap);
+	}
+
+	ret = regmap_read(tps->regmap, TPS65086_DEVICEID, &version);
+	if (ret < 0) {
+		dev_err(tps->dev, "Failed to read revision register\n");
+		return ret;
+	}
+
+	dev_info(tps->dev, "Device: TPS65086%01lX, OTP: %c, Rev: %ld\n",
+		 (version & TPS65086_DEVICEID_PART_MASK),
+		 (char)((version & TPS65086_DEVICEID_OTP_MASK) >> 4) + 'A',
+		 (version & TPS65086_DEVICEID_REV_MASK) >> 6);
+
+	ret = regmap_add_irq_chip(tps->regmap, tps->irq, IRQF_ONESHOT, 0,
+				  &tps65086_irq_chip, &tps->irq_data);
+	if (ret < 0) {
+		dev_err(tps->dev, "Failed to register IRQ chip\n");
+		return ret;
+	}
+
+	ret = of_platform_populate(tps->dev->of_node, NULL, NULL, tps->dev);
+	if (ret < 0) {
+		regmap_del_irq_chip(tps->irq, tps->irq_data);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tps65086_remove(struct i2c_client *client)
+{
+	struct tps65086 *tps = i2c_get_clientdata(client);
+
+	regmap_del_irq_chip(tps->irq, tps->irq_data);
+
+	return 0;
+}
+
+static const struct i2c_device_id tps65086_id_table[] = {
+	{ "tps65086", 0 },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i2c, tps65086_id_table);
+
+static struct i2c_driver tps65086_driver = {
+	.driver		= {
+		.name	= "tps65086",
+		.of_match_table = tps65086_of_match_table,
+	},
+	.probe		= tps65086_probe,
+	.remove		= tps65086_remove,
+	.id_table       = tps65086_id_table,
+};
+
+module_i2c_driver(tps65086_driver);
+
+MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
+MODULE_DESCRIPTION("TPS65086x PMIC Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/tps65086.h b/include/linux/mfd/tps65086.h
new file mode 100644
index 0000000..2a9e9dd
--- /dev/null
+++ b/include/linux/mfd/tps65086.h
@@ -0,0 +1,120 @@
+/*
+ * TI TPS65086x
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Author: Andrew F. Davis <afd@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether expressed or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License version 2 for more details.
+ *
+ * Based on the TPS65912 driver
+ */
+
+#ifndef __LINUX_MFD_TPS65086_H
+#define __LINUX_MFD_TPS65086_H
+
+#include <linux/device.h>
+#include <linux/regmap.h>
+
+/* List of registers for TPS65086 */
+#define TPS65086_DEVICEID		0x01
+#define TPS65086_IRQ			0x02
+#define TPS65086_IRQ_MASK		0x03
+#define TPS65086_PMICSTAT		0x04
+#define TPS65086_SHUTDNSRC		0x05
+#define TPS65086_BUCK1CTRL		0x20
+#define TPS65086_BUCK2CTRL		0x21
+#define TPS65086_BUCK3DECAY		0x22
+#define TPS65086_BUCK3VID		0x23
+#define TPS65086_BUCK3SLPCTRL		0x24
+#define TPS65086_BUCK4CTRL		0x25
+#define TPS65086_BUCK5CTRL		0x26
+#define TPS65086_BUCK6CTRL		0x27
+#define TPS65086_LDOA2CTRL		0x28
+#define TPS65086_LDOA3CTRL		0x29
+#define TPS65086_DISCHCTRL1		0x40
+#define TPS65086_DISCHCTRL2		0x41
+#define TPS65086_DISCHCTRL3		0x42
+#define TPS65086_PG_DELAY1		0x43
+#define TPS65086_FORCESHUTDN		0x91
+#define TPS65086_BUCK1SLPCTRL		0x92
+#define TPS65086_BUCK2SLPCTRL		0x93
+#define TPS65086_BUCK4VID		0x94
+#define TPS65086_BUCK4SLPVID		0x95
+#define TPS65086_BUCK5VID		0x96
+#define TPS65086_BUCK5SLPVID		0x97
+#define TPS65086_BUCK6VID		0x98
+#define TPS65086_BUCK6SLPVID		0x99
+#define TPS65086_LDOA2VID		0x9A
+#define TPS65086_LDOA3VID		0x9B
+#define TPS65086_BUCK123CTRL		0x9C
+#define TPS65086_PG_DELAY2		0x9D
+#define TPS65086_PIN_EN_MASK1		0x9E
+#define TPS65086_PIN_EN_MASK2		0x9F
+#define TPS65086_SWVTT_EN		0x9F
+#define TPS65086_PIN_EN_OVR1		0xA0
+#define TPS65086_PIN_EN_OVR2		0xA1
+#define TPS65086_GPOCTRL		0xA1
+#define TPS65086_PWR_FAULT_MASK1	0xA2
+#define TPS65086_PWR_FAULT_MASK2	0xA3
+#define TPS65086_GPO1PG_CTRL1		0xA4
+#define TPS65086_GPO1PG_CTRL2		0xA5
+#define TPS65086_GPO4PG_CTRL1		0xA6
+#define TPS65086_GPO4PG_CTRL2		0xA7
+#define TPS65086_GPO2PG_CTRL1		0xA8
+#define TPS65086_GPO2PG_CTRL2		0xA9
+#define TPS65086_GPO3PG_CTRL1		0xAA
+#define TPS65086_GPO3PG_CTRL2		0xAB
+#define TPS65086_LDOA1CTRL		0xAE
+#define TPS65086_PG_STATUS1		0xB0
+#define TPS65086_PG_STATUS2		0xB1
+#define TPS65086_PWR_FAULT_STATUS1	0xB2
+#define TPS65086_PWR_FAULT_STATUS2	0xB3
+#define TPS65086_TEMPCRIT		0xB4
+#define TPS65086_TEMPHOT		0xB5
+#define TPS65086_OC_STATUS		0xB6
+
+/* IRQ Register field definitions */
+#define TPS65086_IRQ_DIETEMP_MASK	BIT(0)
+#define TPS65086_IRQ_SHUTDN_MASK	BIT(3)
+#define TPS65086_IRQ_FAULT_MASK		BIT(7)
+
+/* DEVICEID Register field definitions */
+#define TPS65086_DEVICEID_PART_MASK	GENMASK(3, 0)
+#define TPS65086_DEVICEID_OTP_MASK	GENMASK(5, 4)
+#define TPS65086_DEVICEID_REV_MASK	GENMASK(7, 6)
+
+/* VID Masks */
+#define BUCK_VID_MASK			GENMASK(7, 1)
+#define VDOA1_VID_MASK			GENMASK(4, 1)
+#define VDOA23_VID_MASK			GENMASK(3, 0)
+
+/* Define the TPS65086 IRQ numbers */
+enum tps65086_irqs {
+	TPS65086_IRQ_DIETEMP,
+	TPS65086_IRQ_SHUTDN,
+	TPS65086_IRQ_FAULT,
+};
+
+/*
+ * struct tps65086 - state holder for the tps65086 driver
+ *
+ * Device data may be used to access the TPS65086 chip
+ */
+struct tps65086 {
+	struct device *dev;
+	struct regmap *regmap;
+
+	/* IRQ Data */
+	int irq;
+	struct regmap_irq_chip_data *irq_data;
+};
+
+#endif /*  __LINUX_MFD_TPS65086_H */
-- 
1.9.1

[PATCH 3/4] regulator: tps65086: Add regulator driver for the TPS65086 PMIC

[Andrew F. Davis]

Add support for TPS65086 PMIC regulators.

The regulators set consists of 3 Step-down Controllers, 3 Step-down
Converters, 3 LDOs, 3 Load Switches, and a Sink and Source LDO. The
output voltages are configurable and are meant to supply power to a
SoC and/or other components.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/regulator/Kconfig              |   7 +
 drivers/regulator/Makefile             |   1 +
 drivers/regulator/tps65086-regulator.c | 263 +++++++++++++++++++++++++++++++++
 3 files changed, 271 insertions(+)
 create mode 100644 drivers/regulator/tps65086-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 64bccff..cb5f04d 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -680,6 +680,13 @@ config REGULATOR_TPS6507X
 	  three step-down converters and two general-purpose LDO voltage regulators.
 	  It supports TI's software based Class-2 SmartReflex implementation.
 
+config REGULATOR_TPS65086
+	tristate "TI TPS65086 Power regulator"
+	depends on MFD_TPS65086
+	help
+	  This driver provides support for the voltage regulators on the
+	  TI TPS65086 PMIC.
+
 config REGULATOR_TPS65090
 	tristate "TI TPS65090 Power regulator"
 	depends on MFD_TPS65090
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 0f81749..945d8ec 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -85,6 +85,7 @@ obj-$(CONFIG_REGULATOR_TPS6105X) += tps6105x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS62360) += tps62360-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65023) += tps65023-regulator.o
 obj-$(CONFIG_REGULATOR_TPS6507X) += tps6507x-regulator.o
+obj-$(CONFIG_REGULATOR_TPS65086) += tps65086-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65090) += tps65090-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65217) += tps65217-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65218) += tps65218-regulator.o
diff --git a/drivers/regulator/tps65086-regulator.c b/drivers/regulator/tps65086-regulator.c
new file mode 100644
index 0000000..5fd6cf4
--- /dev/null
+++ b/drivers/regulator/tps65086-regulator.c
@@ -0,0 +1,263 @@
+/*
+ * Regulator driver for TPS65086x PMIC
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Author: Andrew F. Davis <afd@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether expressed or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License version 2 for more details.
+ *
+ * Based on the TPS65912 driver
+ */
+
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+#include <linux/mfd/tps65086.h>
+
+enum tps65086_regulators { BUCK1, BUCK2, BUCK3, BUCK4, BUCK5, BUCK6, LDOA1,
+	LDOA2, LDOA3, SWA1, SWB1, SWB2, VTT };
+
+#define TPS65086_REGULATOR(_name, _id, _nv, _vr, _vm, _er, _em, _lr, _dr, _dm)\
+	[_id] = {							\
+		.desc = {						\
+			.name			= _name,		\
+			.id			= _id,			\
+			.ops			= &reg_ops,		\
+			.n_voltages		= _nv,			\
+			.type			= REGULATOR_VOLTAGE,	\
+			.owner			= THIS_MODULE,		\
+			.vsel_reg		= _vr,			\
+			.vsel_mask		= _vm,			\
+			.enable_reg		= _er,			\
+			.enable_mask		= _em,			\
+			.volt_table		= NULL,			\
+			.linear_ranges		= _lr,			\
+			.n_linear_ranges	= ARRAY_SIZE(_lr),	\
+		},							\
+		.decay_reg = _dr,					\
+		.decay_mask = _dm,					\
+	}
+
+#define TPS65086_SWITCH(_name, _id, _er, _em)				\
+	[_id] = {							\
+		.desc = {						\
+			.name			= _name,		\
+			.id			= _id,			\
+			.ops			= &switch_ops,		\
+			.type			= REGULATOR_VOLTAGE,	\
+			.owner			= THIS_MODULE,		\
+			.enable_reg		= _er,			\
+			.enable_mask		= _em,			\
+		},							\
+	}
+
+struct tps65086_regulator {
+	struct regulator_desc desc;
+	unsigned int decay_reg;
+	unsigned int decay_mask;
+};
+
+static const struct regulator_linear_range tps65086_buck126_10mv_ranges[] = {
+	REGULATOR_LINEAR_RANGE(0, 0x0, 0x0, 0),
+	REGULATOR_LINEAR_RANGE(410000, 0x1, 0x7F, 10000),
+};
+
+static const struct regulator_linear_range tps65086_buck126_25mv_ranges[] = {
+	REGULATOR_LINEAR_RANGE(0, 0x0, 0x0, 0),
+	REGULATOR_LINEAR_RANGE(1000000, 0x1, 0x18, 0),
+	REGULATOR_LINEAR_RANGE(1025000, 0x19, 0x7F, 25000),
+};
+
+static const struct regulator_linear_range tps65086_buck345_ranges[] = {
+	REGULATOR_LINEAR_RANGE(0, 0x0, 0x0, 0),
+	REGULATOR_LINEAR_RANGE(425000, 0x1, 0x7F, 25000),
+};
+
+static const struct regulator_linear_range tps65086_ldoa1_ranges[] = {
+	REGULATOR_LINEAR_RANGE(1350000, 0x0, 0x0, 0),
+	REGULATOR_LINEAR_RANGE(1500000, 0x1, 0x7, 100000),
+	REGULATOR_LINEAR_RANGE(2300000, 0x8, 0xA, 100000),
+	REGULATOR_LINEAR_RANGE(2700000, 0xB, 0xD, 150000),
+	REGULATOR_LINEAR_RANGE(3300000, 0xE, 0xE, 0),
+};
+
+static const struct regulator_linear_range tps65086_ldoa23_ranges[] = {
+	REGULATOR_LINEAR_RANGE(700000, 0x0, 0xD, 50000),
+	REGULATOR_LINEAR_RANGE(1400000, 0xE, 0xF, 100000),
+};
+
+/* Operations permitted on regulators */
+static struct regulator_ops reg_ops = {
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.map_voltage		= regulator_map_voltage_linear_range,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.list_voltage		= regulator_list_voltage_linear_range,
+};
+
+/* Operations permitted on load switches */
+static struct regulator_ops switch_ops = {
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+};
+
+static struct tps65086_regulator regulators[] = {
+	TPS65086_REGULATOR("BUCK1", BUCK1, 0x80, TPS65086_BUCK1CTRL,
+			   BUCK_VID_MASK, TPS65086_BUCK123CTRL, BIT(0),
+			   tps65086_buck126_10mv_ranges, TPS65086_BUCK1CTRL,
+			   BIT(0)),
+	TPS65086_REGULATOR("BUCK2", BUCK2, 0x80, TPS65086_BUCK2CTRL,
+			   BUCK_VID_MASK, TPS65086_BUCK123CTRL, BIT(1),
+			   tps65086_buck126_10mv_ranges, TPS65086_BUCK2CTRL,
+			   BIT(0)),
+	TPS65086_REGULATOR("BUCK3", BUCK3, 0x80, TPS65086_BUCK3VID,
+			   BUCK_VID_MASK, TPS65086_BUCK123CTRL, BIT(2),
+			   tps65086_buck345_ranges, TPS65086_BUCK3DECAY,
+			   BIT(0)),
+	TPS65086_REGULATOR("BUCK4", BUCK4, 0x80, TPS65086_BUCK4VID,
+			   BUCK_VID_MASK, TPS65086_BUCK4CTRL, BIT(0),
+			   tps65086_buck345_ranges, TPS65086_BUCK4VID,
+			   BIT(0)),
+	TPS65086_REGULATOR("BUCK5", BUCK5, 0x80, TPS65086_BUCK5VID,
+			   BUCK_VID_MASK, TPS65086_BUCK5CTRL, BIT(0),
+			   tps65086_buck345_ranges, TPS65086_BUCK5CTRL,
+			   BIT(0)),
+	TPS65086_REGULATOR("BUCK6", BUCK6, 0x80, TPS65086_BUCK6VID,
+			   BUCK_VID_MASK, TPS65086_BUCK6CTRL, BIT(0),
+			   tps65086_buck126_10mv_ranges, TPS65086_BUCK6CTRL,
+			   BIT(0)),
+	TPS65086_REGULATOR("LDOA1", LDOA1, 0xF, TPS65086_LDOA1CTRL,
+			   VDOA1_VID_MASK, TPS65086_LDOA1CTRL, BIT(0),
+			   tps65086_ldoa1_ranges, 0, 0),
+	TPS65086_REGULATOR("LDOA2", LDOA2, 0x10, TPS65086_LDOA2VID,
+			   VDOA23_VID_MASK, TPS65086_LDOA2CTRL, BIT(0),
+			   tps65086_ldoa23_ranges, 0, 0),
+	TPS65086_REGULATOR("LDOA3", LDOA3, 0x10, TPS65086_LDOA3VID,
+			   VDOA23_VID_MASK, TPS65086_LDOA3CTRL, BIT(0),
+			   tps65086_ldoa23_ranges, 0, 0),
+	TPS65086_SWITCH("SWA1", SWA1, TPS65086_SWVTT_EN, BIT(5)),
+	TPS65086_SWITCH("SWB1", SWB1, TPS65086_SWVTT_EN, BIT(6)),
+	TPS65086_SWITCH("SWB2", SWB2, TPS65086_SWVTT_EN, BIT(7)),
+	TPS65086_SWITCH("VTT", VTT, TPS65086_SWVTT_EN, BIT(4)),
+};
+
+static struct of_regulator_match tps65086_matches[] = {
+	{ .name = "buck1", .driver_data = &regulators[BUCK1], },
+	{ .name = "buck2", .driver_data = &regulators[BUCK2], },
+	{ .name = "buck3", .driver_data = &regulators[BUCK3], },
+	{ .name = "buck4", .driver_data = &regulators[BUCK4], },
+	{ .name = "buck5", .driver_data = &regulators[BUCK5], },
+	{ .name = "buck6", .driver_data = &regulators[BUCK6], },
+	{ .name = "ldoa1", .driver_data = &regulators[LDOA1], },
+	{ .name = "ldoa2", .driver_data = &regulators[LDOA2], },
+	{ .name = "ldoa3", .driver_data = &regulators[LDOA3], },
+	{ .name = "swa1", .driver_data = &regulators[SWA1], },
+	{ .name = "swb1", .driver_data = &regulators[SWB1], },
+	{ .name = "swb2", .driver_data = &regulators[SWB2], },
+	{ .name = "vtt", .driver_data = &regulators[VTT], },
+};
+
+static inline bool has_25mv_mode(int id)
+{
+	switch (id) {
+	case BUCK1:
+	case BUCK2:
+	case BUCK6:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static int tps65086_regulator_probe(struct platform_device *pdev)
+{
+	struct tps65086 *tps = dev_get_drvdata(pdev->dev.parent);
+	struct regulator_config config = { };
+	struct regulator_dev *rdev;
+	struct tps65086_regulator *regulator;
+
+	int i, ret;
+
+	platform_set_drvdata(pdev, tps);
+
+	config.dev = &pdev->dev;
+	config.driver_data = tps;
+	config.regmap = tps->regmap;
+
+	ret = of_regulator_match(&pdev->dev, pdev->dev.of_node,
+				 tps65086_matches,
+				 ARRAY_SIZE(tps65086_matches));
+	if (ret < 0) {
+		dev_err(tps->dev, "Error parsing regulator init data\n");
+		return ret;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(tps65086_matches); i++) {
+		config.init_data = tps65086_matches[i].init_data;
+		config.of_node = tps65086_matches[i].of_node;
+		regulator = tps65086_matches[i].driver_data;
+
+		/* Check for 25mV step mode */
+		if (has_25mv_mode(regulator->desc.id) &&
+				of_property_read_bool(config.of_node, "ti,regulator-step-size-25mv")) {
+			regulator->desc.linear_ranges = tps65086_buck126_25mv_ranges;
+			regulator->desc.n_linear_ranges = ARRAY_SIZE(tps65086_buck126_25mv_ranges);
+		}
+
+		/* Check for decay mode */
+		if (i <= BUCK6 && of_property_read_bool(config.of_node, "ti,regulator-decay")) {
+			ret = regmap_write_bits(tps->regmap,
+						regulator->decay_reg,
+						regulator->decay_mask,
+						regulator->decay_mask);
+			if (ret < 0) {
+				dev_err(tps->dev, "Error setting decay\n");
+				return ret;
+			}
+		}
+
+		rdev = devm_regulator_register(&pdev->dev, &regulator->desc,
+					       &config);
+		if (IS_ERR(rdev)) {
+			dev_err(tps->dev, "failed to register %s regulator\n",
+				pdev->name);
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}
+
+static const struct of_device_id tps65086_regulator_of_match_table[] = {
+	{ .compatible = "ti,tps65086-regulator", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, tps65086_regulator_of_match_table);
+
+static struct platform_driver tps65086_regulator_driver = {
+	.driver = {
+		.name = "tps65086-pmic",
+		.of_match_table = tps65086_regulator_of_match_table,
+	},
+	.probe = tps65086_regulator_probe,
+};
+
+module_platform_driver(tps65086_regulator_driver);
+
+MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
+MODULE_DESCRIPTION("TPS65086x voltage regulator driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH 4/4] gpio: tps65086: Add GPIO driver for the TPS65086 PMIC

[Andrew F. Davis]

Add support for the TPS65086 PMIC GPOs.

TPS65086 has four configurable GPOs that can be used for several
purposes.

Signed-off-by: Andrew F. Davis <afd@ti.com>
---
 drivers/gpio/Kconfig         |   6 +++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-tps65086.c | 114 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+)
 create mode 100644 drivers/gpio/gpio-tps65086.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b4fc9e4..c585d42 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -824,6 +824,12 @@ config GPIO_TIMBERDALE
 	---help---
 	Add support for the GPIO IP in the timberdale FPGA.
 
+config GPIO_TPS65086
+	tristate "TI TPS65086 GPO"
+	depends on MFD_TPS65086
+	help
+	  This driver supports the GPO on the TI TPS65086 PMIC.
+
 config GPIO_TPS6586X
 	bool "TPS6586X GPIO"
 	depends on MFD_TPS6586X
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index f79a7c4..2611c7e 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -94,6 +94,7 @@ obj-$(CONFIG_GPIO_TC3589X)	+= gpio-tc3589x.o
 obj-$(CONFIG_ARCH_TEGRA)	+= gpio-tegra.o
 obj-$(CONFIG_GPIO_TIMBERDALE)	+= gpio-timberdale.o
 obj-$(CONFIG_GPIO_PALMAS)	+= gpio-palmas.o
+obj-$(CONFIG_GPIO_TPS65086)	+= gpio-tps65086.o
 obj-$(CONFIG_GPIO_TPS6586X)	+= gpio-tps6586x.o
 obj-$(CONFIG_GPIO_TPS65910)	+= gpio-tps65910.o
 obj-$(CONFIG_GPIO_TPS65912)	+= gpio-tps65912.o
diff --git a/drivers/gpio/gpio-tps65086.c b/drivers/gpio/gpio-tps65086.c
new file mode 100644
index 0000000..9fba758
--- /dev/null
+++ b/drivers/gpio/gpio-tps65086.c
@@ -0,0 +1,114 @@
+/*
+ * TI TPS65086x GPIO Driver
+ *
+ * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * Author: Andrew F. Davis <afd@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether expressed or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License version 2 for more details.
+ *
+ * Based on the TPS65912 driver
+ */
+
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+
+#include <linux/mfd/tps65086.h>
+
+struct tps65086_gpio {
+	struct gpio_chip gpio_chip;
+	struct tps65086 *tps;
+};
+
+#define to_tps65086_gpio(gc) container_of(gc, struct tps65086_gpio, gpio_chip)
+
+static int tps65086_gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+	struct tps65086_gpio *gpio = to_tps65086_gpio(gc);
+	int ret, val;
+
+	ret = regmap_read(gpio->tps->regmap, TPS65086_GPOCTRL, &val);
+	if (ret < 0)
+		return ret;
+
+	return val & BIT(4 + offset);
+}
+
+static void tps65086_gpio_set(struct gpio_chip *gc, unsigned offset,
+			      int value)
+{
+	struct tps65086_gpio *gpio = to_tps65086_gpio(gc);
+
+	regmap_update_bits(gpio->tps->regmap, TPS65086_GPOCTRL,
+			   BIT(4 + offset), value ? BIT(4 + offset) : 0);
+}
+
+static struct gpio_chip template_chip = {
+	.label			= "tps65086-gpio",
+	.owner			= THIS_MODULE,
+	.get			= tps65086_gpio_get,
+	.set			= tps65086_gpio_set,
+	.can_sleep		= true,
+	.ngpio			= 4,
+	.base			= -1,
+};
+
+static int tps65086_gpio_probe(struct platform_device *pdev)
+{
+	struct tps65086_gpio *gpio;
+	int ret;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	gpio->tps = dev_get_drvdata(pdev->dev.parent);
+	gpio->gpio_chip = template_chip;
+	ret = gpiochip_add(&gpio->gpio_chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, gpio);
+
+	return 0;
+}
+
+static int tps65086_gpio_remove(struct platform_device *pdev)
+{
+	struct tps65086_gpio *gpio = platform_get_drvdata(pdev);
+
+	gpiochip_remove(&gpio->gpio_chip);
+
+	return 0;
+}
+
+static const struct of_device_id tps65086_gpio_of_match_table[] = {
+	{ .compatible = "ti,tps65086-gpio", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, tps65086_gpio_of_match_table);
+
+static struct platform_driver tps65086_gpio_driver = {
+	.driver = {
+		.name = "tps65086-gpio",
+		.of_match_table = tps65086_gpio_of_match_table,
+	},
+	.probe = tps65086_gpio_probe,
+	.remove = tps65086_gpio_remove,
+};
+
+module_platform_driver(tps65086_gpio_driver);
+
+MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
+MODULE_DESCRIPTION("TPS65086 GPIO driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Lee Jones]

On Fri, 16 Oct 2015, Andrew F. Davis wrote:

> The TPS65086 PMIC contains several regulators and a GPO controller.
> Add bindings for the TPS65086 PMIC.
> 
> Signed-off-by: Andrew F. Davis <afd@ti.com>
> ---
>  .../devicetree/bindings/gpio/gpio-tps65086.txt     | 17 ++++++++
>  Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++
>  .../bindings/regulator/tps65086-regulator.txt      | 36 +++++++++++++++++

Please split these up into separate patches.

There is no functional reason to bundle them up.

>  3 files changed, 99 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt
>  create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt
>  create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt

[...]

> diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt
> new file mode 100644
> index 0000000..4b6aeb4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/tps65086.txt
> @@ -0,0 +1,46 @@
> +* TPS65086 Power Management Integrated Circuit bindings
> +
> +Required properties:
> + - compatible		: Should be "ti,tps65086".

Any indication that it's a PMIC?

> + - reg			: Slave address.

I2C/SPI?

> + - interrupt-parent	: The parent interrupt controller.

Phandled to ...

> + - interrupts		: The interrupt line the device is connected to.
> + - interrupt-controller	: Marks the device node as an interrupt controller.
> + - #interrupt-cells	: The number of cells to describe an IRQ, this
> +			  should be 2. The first cell is the IRQ number.
> +			  The second cell is the flags, encoded as the trigger
> +			  masks from ../interrupt-controller/interrupts.txt.

Masks?  What masks?

Best to make a link to the header where the flags are defined here.

> +Additional nodes defined in:
> + - Regulators	: ../regulator/tps65086-regulator.txt.
> + - GPIO		: ../gpio/gpio-tps65086.txt.

I'd suggest removing the full stops from all of the lines above.

Just treat them as bullet points like we normally do.

> +Example:
> +
> +	pmic: tps65086@5e {
> +		compatible = "ti,tps65086";
> +		reg = <0x5e>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +
> +		regulators {
> +			compatible = "ti,tps65086-regulator";
> +
> +			buck1 {
> +				regulator-name = "vcc1";
> +				regulator-min-microvolt = <1600000>;
> +				regulator-max-microvolt = <1600000>;
> +				regulator-boot-on;
> +				ti,regulator-decay;
> +				ti,regulator-step-size-25mv;
> +			};
> +		};
> +
> +		gpio4: tps65086_gpio {
> +			compatible = "ti,tps65086-gpio";
> +			gpio-controller;
> +			#gpio-cells = <2>;
> +		};
> +	};

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] mfd: tps65086: Add driver for the TPS65086 PMIC

[Lee Jones]

On Fri, 16 Oct 2015, Andrew F. Davis wrote:

> Add support for the TPS65912 device. It provides communication
> through I2C and contains the following components:
> 
>  - Regulators
>  - Load switches
>  - GPO controller

What's a GPO controller?

> Signed-off-by: Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/mfd/Kconfig          |  13 ++++
>  drivers/mfd/Makefile         |   1 +
>  drivers/mfd/tps65086.c       | 145 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/tps65086.h | 120 +++++++++++++++++++++++++++++++++++
>  4 files changed, 279 insertions(+)
>  create mode 100644 drivers/mfd/tps65086.c
>  create mode 100644 include/linux/mfd/tps65086.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 99d6367..ab3b392 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1093,6 +1093,19 @@ config TPS6507X
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called tps6507x.
>  
> +config MFD_TPS65086
> +	tristate "TI TPS65086x Power Management chips"

Use the full name:

  "TI TPS65086x Power Management Integrated Chips (PMICs)"

> +	select REGMAP
> +	select REGMAP_IRQ
> +	select REGMAP_I2C
> +	depends on I2C

REGMAP_I2C already depends on I2C.

> +	depends on OF || COMPILE_TEST
> +	help
> +	  If you say yes here you get support for the TPS65086 series of
> +	  Power Management chips.

I'd remove the line break.

> +	  These include voltage regulators, GPO and other features

GPIO?

> +	  that are often used in portable devices.

This is meant to be a 'help', so instead of staying "and some other
stuff", please list them.

>  config TPS65911_COMPARATOR
>  	tristate
>  
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a59e3fc..7adb902 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_MFD_WM8994)	+= wm8994-core.o wm8994-irq.o wm8994-regmap.o
>  obj-$(CONFIG_TPS6105X)		+= tps6105x.o
>  obj-$(CONFIG_TPS65010)		+= tps65010.o
>  obj-$(CONFIG_TPS6507X)		+= tps6507x.o
> +obj-$(CONFIG_MFD_TPS65086)	+= tps65086.o
>  obj-$(CONFIG_MFD_TPS65217)	+= tps65217.o
>  obj-$(CONFIG_MFD_TPS65218)	+= tps65218.o
>  obj-$(CONFIG_MFD_TPS65910)	+= tps65910.o
> diff --git a/drivers/mfd/tps65086.c b/drivers/mfd/tps65086.c
> new file mode 100644
> index 0000000..be6d678
> --- /dev/null
> +++ b/drivers/mfd/tps65086.c
> @@ -0,0 +1,145 @@
> +/*
> + * Driver for TI TPS65086x PMICs

I'd suggest removing the driver part.  We know it's a driver.

> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * Author: Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether expressed or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License version 2 for more details.
> + *
> + * Based on the TPS65912 driver
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +#include <linux/mfd/tps65086.h>
> +
> +static const struct regmap_range tps65086_yes_ranges[] = {
> +	regmap_reg_range(TPS65086_IRQ, TPS65086_IRQ),
> +	regmap_reg_range(TPS65086_PMICSTAT, TPS65086_SHUTDNSRC),
> +	regmap_reg_range(TPS65086_GPOCTRL, TPS65086_GPOCTRL),
> +	regmap_reg_range(TPS65086_PG_STATUS1, TPS65086_OC_STATUS),
> +};
> +
> +static const struct regmap_access_table tps65086_volatile_table = {
> +	.yes_ranges = tps65086_yes_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(tps65086_yes_ranges),
> +};
> +
> +static const struct regmap_config tps65086_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.cache_type = REGCACHE_RBTREE,
> +	.volatile_table = &tps65086_volatile_table,
> +};
> +
> +static const struct regmap_irq tps65086_irqs[] = {
> +	REGMAP_IRQ_REG(TPS65086_IRQ_DIETEMP, 0, TPS65086_IRQ_DIETEMP_MASK),
> +	REGMAP_IRQ_REG(TPS65086_IRQ_SHUTDN, 0, TPS65086_IRQ_SHUTDN_MASK),
> +	REGMAP_IRQ_REG(TPS65086_IRQ_FAULT, 0, TPS65086_IRQ_FAULT_MASK),
> +};
> +
> +static struct regmap_irq_chip tps65086_irq_chip = {
> +	.name = "tps65086",
> +	.status_base = TPS65086_IRQ,
> +	.mask_base = TPS65086_IRQ_MASK,
> +	.ack_base = TPS65086_IRQ,
> +	.init_ack_masked = true,
> +	.num_regs = 1,
> +	.irqs = tps65086_irqs,
> +	.num_irqs = ARRAY_SIZE(tps65086_irqs),
> +};
> +
> +static const struct of_device_id tps65086_of_match_table[] = {
> +	{ .compatible = "ti,tps65086", },
> +	{ /* sentinel */ }
> +};
> +
> +static int tps65086_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *ids)
> +{
> +	struct tps65086 *tps;
> +	unsigned int version;
> +	int ret;
> +
> +	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
> +	if (!tps)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, tps);
> +	tps->dev = &client->dev;
> +	tps->irq = client->irq;
> +
> +	tps->regmap = devm_regmap_init_i2c(client, &tps65086_regmap_config);
> +	if (IS_ERR(tps->regmap)) {
> +		dev_err(tps->dev, "Failed to initialize register map\n");
> +		return PTR_ERR(tps->regmap);
> +	}
> +
> +	ret = regmap_read(tps->regmap, TPS65086_DEVICEID, &version);
> +	if (ret < 0) {
> +		dev_err(tps->dev, "Failed to read revision register\n");
> +		return ret;
> +	}
> +
> +	dev_info(tps->dev, "Device: TPS65086%01lX, OTP: %c, Rev: %ld\n",
> +		 (version & TPS65086_DEVICEID_PART_MASK),
> +		 (char)((version & TPS65086_DEVICEID_OTP_MASK) >> 4) + 'A',
> +		 (version & TPS65086_DEVICEID_REV_MASK) >> 6);
> +
> +	ret = regmap_add_irq_chip(tps->regmap, tps->irq, IRQF_ONESHOT, 0,
> +				  &tps65086_irq_chip, &tps->irq_data);
> +	if (ret < 0) {

if (ret)

> +		dev_err(tps->dev, "Failed to register IRQ chip\n");
> +		return ret;
> +	}
> +
> +	ret = of_platform_populate(tps->dev->of_node, NULL, NULL, tps->dev);
> +	if (ret < 0) {

if (ret)

> +		regmap_del_irq_chip(tps->irq, tps->irq_data);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tps65086_remove(struct i2c_client *client)
> +{
> +	struct tps65086 *tps = i2c_get_clientdata(client);
> +
> +	regmap_del_irq_chip(tps->irq, tps->irq_data);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id tps65086_id_table[] = {
> +	{ "tps65086", 0 },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(i2c, tps65086_id_table);
> +
> +static struct i2c_driver tps65086_driver = {
> +	.driver		= {
> +		.name	= "tps65086",
> +		.of_match_table = tps65086_of_match_table,
> +	},
> +	.probe		= tps65086_probe,
> +	.remove		= tps65086_remove,
> +	.id_table       = tps65086_id_table,
> +};
> +
> +module_i2c_driver(tps65086_driver);
> +
> +MODULE_AUTHOR("Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org>");
> +MODULE_DESCRIPTION("TPS65086x PMIC Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/tps65086.h b/include/linux/mfd/tps65086.h
> new file mode 100644
> index 0000000..2a9e9dd
> --- /dev/null
> +++ b/include/linux/mfd/tps65086.h
> @@ -0,0 +1,120 @@
> +/*
> + * TI TPS65086x
> + *
> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * Author: Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether expressed or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License version 2 for more details.
> + *
> + * Based on the TPS65912 driver
> + */
> +
> +#ifndef __LINUX_MFD_TPS65086_H
> +#define __LINUX_MFD_TPS65086_H
> +
> +#include <linux/device.h>
> +#include <linux/regmap.h>
> +
> +/* List of registers for TPS65086 */
> +#define TPS65086_DEVICEID		0x01
> +#define TPS65086_IRQ			0x02
> +#define TPS65086_IRQ_MASK		0x03
> +#define TPS65086_PMICSTAT		0x04
> +#define TPS65086_SHUTDNSRC		0x05
> +#define TPS65086_BUCK1CTRL		0x20
> +#define TPS65086_BUCK2CTRL		0x21
> +#define TPS65086_BUCK3DECAY		0x22
> +#define TPS65086_BUCK3VID		0x23
> +#define TPS65086_BUCK3SLPCTRL		0x24
> +#define TPS65086_BUCK4CTRL		0x25
> +#define TPS65086_BUCK5CTRL		0x26
> +#define TPS65086_BUCK6CTRL		0x27
> +#define TPS65086_LDOA2CTRL		0x28
> +#define TPS65086_LDOA3CTRL		0x29
> +#define TPS65086_DISCHCTRL1		0x40
> +#define TPS65086_DISCHCTRL2		0x41
> +#define TPS65086_DISCHCTRL3		0x42
> +#define TPS65086_PG_DELAY1		0x43
> +#define TPS65086_FORCESHUTDN		0x91
> +#define TPS65086_BUCK1SLPCTRL		0x92
> +#define TPS65086_BUCK2SLPCTRL		0x93
> +#define TPS65086_BUCK4VID		0x94
> +#define TPS65086_BUCK4SLPVID		0x95
> +#define TPS65086_BUCK5VID		0x96
> +#define TPS65086_BUCK5SLPVID		0x97
> +#define TPS65086_BUCK6VID		0x98
> +#define TPS65086_BUCK6SLPVID		0x99
> +#define TPS65086_LDOA2VID		0x9A
> +#define TPS65086_LDOA3VID		0x9B
> +#define TPS65086_BUCK123CTRL		0x9C
> +#define TPS65086_PG_DELAY2		0x9D
> +#define TPS65086_PIN_EN_MASK1		0x9E
> +#define TPS65086_PIN_EN_MASK2		0x9F
> +#define TPS65086_SWVTT_EN		0x9F
> +#define TPS65086_PIN_EN_OVR1		0xA0
> +#define TPS65086_PIN_EN_OVR2		0xA1
> +#define TPS65086_GPOCTRL		0xA1
> +#define TPS65086_PWR_FAULT_MASK1	0xA2
> +#define TPS65086_PWR_FAULT_MASK2	0xA3
> +#define TPS65086_GPO1PG_CTRL1		0xA4
> +#define TPS65086_GPO1PG_CTRL2		0xA5
> +#define TPS65086_GPO4PG_CTRL1		0xA6
> +#define TPS65086_GPO4PG_CTRL2		0xA7
> +#define TPS65086_GPO2PG_CTRL1		0xA8
> +#define TPS65086_GPO2PG_CTRL2		0xA9
> +#define TPS65086_GPO3PG_CTRL1		0xAA
> +#define TPS65086_GPO3PG_CTRL2		0xAB
> +#define TPS65086_LDOA1CTRL		0xAE
> +#define TPS65086_PG_STATUS1		0xB0
> +#define TPS65086_PG_STATUS2		0xB1
> +#define TPS65086_PWR_FAULT_STATUS1	0xB2
> +#define TPS65086_PWR_FAULT_STATUS2	0xB3
> +#define TPS65086_TEMPCRIT		0xB4
> +#define TPS65086_TEMPHOT		0xB5
> +#define TPS65086_OC_STATUS		0xB6
> +
> +/* IRQ Register field definitions */
> +#define TPS65086_IRQ_DIETEMP_MASK	BIT(0)
> +#define TPS65086_IRQ_SHUTDN_MASK	BIT(3)
> +#define TPS65086_IRQ_FAULT_MASK		BIT(7)
> +
> +/* DEVICEID Register field definitions */
> +#define TPS65086_DEVICEID_PART_MASK	GENMASK(3, 0)
> +#define TPS65086_DEVICEID_OTP_MASK	GENMASK(5, 4)
> +#define TPS65086_DEVICEID_REV_MASK	GENMASK(7, 6)
> +
> +/* VID Masks */
> +#define BUCK_VID_MASK			GENMASK(7, 1)
> +#define VDOA1_VID_MASK			GENMASK(4, 1)
> +#define VDOA23_VID_MASK			GENMASK(3, 0)
> +
> +/* Define the TPS65086 IRQ numbers */
> +enum tps65086_irqs {
> +	TPS65086_IRQ_DIETEMP,
> +	TPS65086_IRQ_SHUTDN,
> +	TPS65086_IRQ_FAULT,
> +};
> +
> +/*
> + * struct tps65086 - state holder for the tps65086 driver
> + *
> + * Device data may be used to access the TPS65086 chip
> + */
> +struct tps65086 {
> +	struct device *dev;
> +	struct regmap *regmap;
> +
> +	/* IRQ Data */
> +	int irq;
> +	struct regmap_irq_chip_data *irq_data;
> +};
> +
> +#endif /*  __LINUX_MFD_TPS65086_H */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Andrew F. Davis]

On 10/19/2015 04:13 AM, Lee Jones wrote:
> On Fri, 16 Oct 2015, Andrew F. Davis wrote:
>
>> The TPS65086 PMIC contains several regulators and a GPO controller.
>> Add bindings for the TPS65086 PMIC.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> ---
>>   .../devicetree/bindings/gpio/gpio-tps65086.txt     | 17 ++++++++
>>   Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++
>>   .../bindings/regulator/tps65086-regulator.txt      | 36 +++++++++++++++++
>
> Please split these up into separate patches.
>
> There is no functional reason to bundle them up.
>

ACK

>>   3 files changed, 99 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt
>>   create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt
>>   create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt
>
> [...]
>
>> diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt
>> new file mode 100644
>> index 0000000..4b6aeb4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mfd/tps65086.txt
>> @@ -0,0 +1,46 @@
>> +* TPS65086 Power Management Integrated Circuit bindings
>> +
>> +Required properties:
>> + - compatible		: Should be "ti,tps65086".
>
> Any indication that it's a PMIC?
>

In the compatible string?

>> + - reg			: Slave address.
>
> I2C/SPI?
>

ACK

>> + - interrupt-parent	: The parent interrupt controller.
>
> Phandled to ...
>

ACK

>> + - interrupts		: The interrupt line the device is connected to.
>> + - interrupt-controller	: Marks the device node as an interrupt controller.
>> + - #interrupt-cells	: The number of cells to describe an IRQ, this
>> +			  should be 2. The first cell is the IRQ number.
>> +			  The second cell is the flags, encoded as the trigger
>> +			  masks from ../interrupt-controller/interrupts.txt.
>
> Masks?  What masks?
>
> Best to make a link to the header where the flags are defined here.
>

ACK

>> +Additional nodes defined in:
>> + - Regulators	: ../regulator/tps65086-regulator.txt.
>> + - GPIO		: ../gpio/gpio-tps65086.txt.
>
> I'd suggest removing the full stops from all of the lines above.
>
> Just treat them as bullet points like we normally do.
>

ACK

>> +Example:
>> +
>> +	pmic: tps65086@5e {
>> +		compatible = "ti,tps65086";
>> +		reg = <0x5e>;
>> +		interrupt-parent = <&gpio1>;
>> +		interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
>> +		interrupt-controller;
>> +		#interrupt-cells = <2>;
>> +
>> +		regulators {
>> +			compatible = "ti,tps65086-regulator";
>> +
>> +			buck1 {
>> +				regulator-name = "vcc1";
>> +				regulator-min-microvolt = <1600000>;
>> +				regulator-max-microvolt = <1600000>;
>> +				regulator-boot-on;
>> +				ti,regulator-decay;
>> +				ti,regulator-step-size-25mv;
>> +			};
>> +		};
>> +
>> +		gpio4: tps65086_gpio {
>> +			compatible = "ti,tps65086-gpio";
>> +			gpio-controller;
>> +			#gpio-cells = <2>;
>> +		};
>> +	};
>
> [...]
>

-- 
Andrew F. Davis

Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Lee Jones]

On Mon, 19 Oct 2015, Andrew F. Davis wrote:

> On 10/19/2015 04:13 AM, Lee Jones wrote:
> >On Fri, 16 Oct 2015, Andrew F. Davis wrote:
> >
> >>The TPS65086 PMIC contains several regulators and a GPO controller.
> >>Add bindings for the TPS65086 PMIC.
> >>
> >>Signed-off-by: Andrew F. Davis <afd@ti.com>
> >>---
> >>  .../devicetree/bindings/gpio/gpio-tps65086.txt     | 17 ++++++++
> >>  Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++
> >>  .../bindings/regulator/tps65086-regulator.txt      | 36 +++++++++++++++++
> >
> >Please split these up into separate patches.
> >
> >There is no functional reason to bundle them up.
> >
> 
> ACK
> 
> >>  3 files changed, 99 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt
> >>  create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt
> >>  create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt
> >
> >[...]
> >
> >>diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt
> >>new file mode 100644
> >>index 0000000..4b6aeb4
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/mfd/tps65086.txt
> >>@@ -0,0 +1,46 @@
> >>+* TPS65086 Power Management Integrated Circuit bindings
> >>+
> >>+Required properties:
> >>+ - compatible		: Should be "ti,tps65086".
> >
> >Any indication that it's a PMIC?
> >
> 
> In the compatible string?

Ya.

> >>+ - reg			: Slave address.
> >
> >I2C/SPI?
> >
> 
> ACK
> 
> >>+ - interrupt-parent	: The parent interrupt controller.
> >
> >Phandled to ...
> >
> 
> ACK
> 
> >>+ - interrupts		: The interrupt line the device is connected to.
> >>+ - interrupt-controller	: Marks the device node as an interrupt controller.
> >>+ - #interrupt-cells	: The number of cells to describe an IRQ, this
> >>+			  should be 2. The first cell is the IRQ number.
> >>+			  The second cell is the flags, encoded as the trigger
> >>+			  masks from ../interrupt-controller/interrupts.txt.
> >
> >Masks?  What masks?
> >
> >Best to make a link to the header where the flags are defined here.
> >
> 
> ACK
> 
> >>+Additional nodes defined in:
> >>+ - Regulators	: ../regulator/tps65086-regulator.txt.
> >>+ - GPIO		: ../gpio/gpio-tps65086.txt.
> >
> >I'd suggest removing the full stops from all of the lines above.
> >
> >Just treat them as bullet points like we normally do.
> >
> 
> ACK
> 
> >>+Example:
> >>+
> >>+	pmic: tps65086@5e {
> >>+		compatible = "ti,tps65086";
> >>+		reg = <0x5e>;
> >>+		interrupt-parent = <&gpio1>;
> >>+		interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
> >>+		interrupt-controller;
> >>+		#interrupt-cells = <2>;
> >>+
> >>+		regulators {
> >>+			compatible = "ti,tps65086-regulator";
> >>+
> >>+			buck1 {
> >>+				regulator-name = "vcc1";
> >>+				regulator-min-microvolt = <1600000>;
> >>+				regulator-max-microvolt = <1600000>;
> >>+				regulator-boot-on;
> >>+				ti,regulator-decay;
> >>+				ti,regulator-step-size-25mv;
> >>+			};
> >>+		};
> >>+
> >>+		gpio4: tps65086_gpio {
> >>+			compatible = "ti,tps65086-gpio";
> >>+			gpio-controller;
> >>+			#gpio-cells = <2>;
> >>+		};
> >>+	};
> >
> >[...]
> >
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Andrew F. Davis]

On 10/19/2015 10:21 AM, Lee Jones wrote:
> On Mon, 19 Oct 2015, Andrew F. Davis wrote:
>
>> On 10/19/2015 04:13 AM, Lee Jones wrote:
>>> On Fri, 16 Oct 2015, Andrew F. Davis wrote:
>>>
>>>> The TPS65086 PMIC contains several regulators and a GPO controller.
>>>> Add bindings for the TPS65086 PMIC.
>>>>
>>>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>>>> ---
>>>>   .../devicetree/bindings/gpio/gpio-tps65086.txt     | 17 ++++++++
>>>>   Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++
>>>>   .../bindings/regulator/tps65086-regulator.txt      | 36 +++++++++++++++++
>>>
>>> Please split these up into separate patches.
>>>
>>> There is no functional reason to bundle them up.
>>>
>>
>> ACK
>>
>>>>   3 files changed, 99 insertions(+)
>>>>   create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt
>>>>   create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt
>>>>   create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt
>>>
>>> [...]
>>>
>>>> diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt
>>>> new file mode 100644
>>>> index 0000000..4b6aeb4
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mfd/tps65086.txt
>>>> @@ -0,0 +1,46 @@
>>>> +* TPS65086 Power Management Integrated Circuit bindings
>>>> +
>>>> +Required properties:
>>>> + - compatible		: Should be "ti,tps65086".
>>>
>>> Any indication that it's a PMIC?
>>>
>>
>> In the compatible string?
>
> Ya.
>

Not sure what you mean then?, no one else seems to be doing that,
"xx,xxxxxxx-pmic" is usually used for matching the regulator node,
not the device itself.

>>>> + - reg			: Slave address.
>>>
>>> I2C/SPI?
>>>
>>
>> ACK
>>
>>>> + - interrupt-parent	: The parent interrupt controller.
>>>
>>> Phandled to ...
>>>
>>
>> ACK
>>
>>>> + - interrupts		: The interrupt line the device is connected to.
>>>> + - interrupt-controller	: Marks the device node as an interrupt controller.
>>>> + - #interrupt-cells	: The number of cells to describe an IRQ, this
>>>> +			  should be 2. The first cell is the IRQ number.
>>>> +			  The second cell is the flags, encoded as the trigger
>>>> +			  masks from ../interrupt-controller/interrupts.txt.
>>>
>>> Masks?  What masks?
>>>
>>> Best to make a link to the header where the flags are defined here.
>>>
>>
>> ACK
>>
>>>> +Additional nodes defined in:
>>>> + - Regulators	: ../regulator/tps65086-regulator.txt.
>>>> + - GPIO		: ../gpio/gpio-tps65086.txt.
>>>
>>> I'd suggest removing the full stops from all of the lines above.
>>>
>>> Just treat them as bullet points like we normally do.
>>>
>>
>> ACK
>>
>>>> +Example:
>>>> +
>>>> +	pmic: tps65086@5e {
>>>> +		compatible = "ti,tps65086";
>>>> +		reg = <0x5e>;
>>>> +		interrupt-parent = <&gpio1>;
>>>> +		interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
>>>> +		interrupt-controller;
>>>> +		#interrupt-cells = <2>;
>>>> +
>>>> +		regulators {
>>>> +			compatible = "ti,tps65086-regulator";
>>>> +
>>>> +			buck1 {
>>>> +				regulator-name = "vcc1";
>>>> +				regulator-min-microvolt = <1600000>;
>>>> +				regulator-max-microvolt = <1600000>;
>>>> +				regulator-boot-on;
>>>> +				ti,regulator-decay;
>>>> +				ti,regulator-step-size-25mv;
>>>> +			};
>>>> +		};
>>>> +
>>>> +		gpio4: tps65086_gpio {
>>>> +			compatible = "ti,tps65086-gpio";
>>>> +			gpio-controller;
>>>> +			#gpio-cells = <2>;
>>>> +		};
>>>> +	};
>>>
>>> [...]
>>>
>>
>

-- 
Andrew F. Davis

Re: [PATCH 2/4] mfd: tps65086: Add driver for the TPS65086 PMIC

[Andrew F. Davis]

On 10/19/2015 04:23 AM, Lee Jones wrote:
> On Fri, 16 Oct 2015, Andrew F. Davis wrote:
>
>> Add support for the TPS65912 device. It provides communication
>> through I2C and contains the following components:
>>
>>   - Regulators
>>   - Load switches
>>   - GPO controller
>
> What's a GPO controller?
>

General Purpose Output, this controller only has output control, it's
listed in the data-sheet this way, and other drivers have used this term.

>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> ---
>>   drivers/mfd/Kconfig          |  13 ++++
>>   drivers/mfd/Makefile         |   1 +
>>   drivers/mfd/tps65086.c       | 145 +++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/mfd/tps65086.h | 120 +++++++++++++++++++++++++++++++++++
>>   4 files changed, 279 insertions(+)
>>   create mode 100644 drivers/mfd/tps65086.c
>>   create mode 100644 include/linux/mfd/tps65086.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 99d6367..ab3b392 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1093,6 +1093,19 @@ config TPS6507X
>>   	  This driver can also be built as a module.  If so, the module
>>   	  will be called tps6507x.
>>
>> +config MFD_TPS65086
>> +	tristate "TI TPS65086x Power Management chips"
>
> Use the full name:
>
>    "TI TPS65086x Power Management Integrated Chips (PMICs)"
>
>> +	select REGMAP
>> +	select REGMAP_IRQ
>> +	select REGMAP_I2C
>> +	depends on I2C
>
> REGMAP_I2C already depends on I2C.
>

So if I force REGMAP_I2C to =y without depending on I2C then couldn't
REGMAP_I2C get selected without I2C enabled? Selecting doesn't force
the selected item's dependencies still right?

>> +	depends on OF || COMPILE_TEST
>> +	help
>> +	  If you say yes here you get support for the TPS65086 series of
>> +	  Power Management chips.
>
> I'd remove the line break.
>
>> +	  These include voltage regulators, GPO and other features
>
> GPIO?
>
>> +	  that are often used in portable devices.
>
> This is meant to be a 'help', so instead of staying "and some other
> stuff", please list them.
>

This seems to be what other devices do, I'll change it to something
else then.

>>   config TPS65911_COMPARATOR
>>   	tristate
>>
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index a59e3fc..7adb902 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -66,6 +66,7 @@ obj-$(CONFIG_MFD_WM8994)	+= wm8994-core.o wm8994-irq.o wm8994-regmap.o
>>   obj-$(CONFIG_TPS6105X)		+= tps6105x.o
>>   obj-$(CONFIG_TPS65010)		+= tps65010.o
>>   obj-$(CONFIG_TPS6507X)		+= tps6507x.o
>> +obj-$(CONFIG_MFD_TPS65086)	+= tps65086.o
>>   obj-$(CONFIG_MFD_TPS65217)	+= tps65217.o
>>   obj-$(CONFIG_MFD_TPS65218)	+= tps65218.o
>>   obj-$(CONFIG_MFD_TPS65910)	+= tps65910.o
>> diff --git a/drivers/mfd/tps65086.c b/drivers/mfd/tps65086.c
>> new file mode 100644
>> index 0000000..be6d678
>> --- /dev/null
>> +++ b/drivers/mfd/tps65086.c
>> @@ -0,0 +1,145 @@
>> +/*
>> + * Driver for TI TPS65086x PMICs
>
> I'd suggest removing the driver part.  We know it's a driver.
>

Again this is what almost every other driver does, I'm not sure how
else to describe this file without using the word driver.

>> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
>> + *
>> + * Author: Andrew F. Davis <afd@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether expressed or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License version 2 for more details.
>> + *
>> + * Based on the TPS65912 driver
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +
>> +#include <linux/mfd/tps65086.h>
>> +
>> +static const struct regmap_range tps65086_yes_ranges[] = {
>> +	regmap_reg_range(TPS65086_IRQ, TPS65086_IRQ),
>> +	regmap_reg_range(TPS65086_PMICSTAT, TPS65086_SHUTDNSRC),
>> +	regmap_reg_range(TPS65086_GPOCTRL, TPS65086_GPOCTRL),
>> +	regmap_reg_range(TPS65086_PG_STATUS1, TPS65086_OC_STATUS),
>> +};
>> +
>> +static const struct regmap_access_table tps65086_volatile_table = {
>> +	.yes_ranges = tps65086_yes_ranges,
>> +	.n_yes_ranges = ARRAY_SIZE(tps65086_yes_ranges),
>> +};
>> +
>> +static const struct regmap_config tps65086_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.cache_type = REGCACHE_RBTREE,
>> +	.volatile_table = &tps65086_volatile_table,
>> +};
>> +
>> +static const struct regmap_irq tps65086_irqs[] = {
>> +	REGMAP_IRQ_REG(TPS65086_IRQ_DIETEMP, 0, TPS65086_IRQ_DIETEMP_MASK),
>> +	REGMAP_IRQ_REG(TPS65086_IRQ_SHUTDN, 0, TPS65086_IRQ_SHUTDN_MASK),
>> +	REGMAP_IRQ_REG(TPS65086_IRQ_FAULT, 0, TPS65086_IRQ_FAULT_MASK),
>> +};
>> +
>> +static struct regmap_irq_chip tps65086_irq_chip = {
>> +	.name = "tps65086",
>> +	.status_base = TPS65086_IRQ,
>> +	.mask_base = TPS65086_IRQ_MASK,
>> +	.ack_base = TPS65086_IRQ,
>> +	.init_ack_masked = true,
>> +	.num_regs = 1,
>> +	.irqs = tps65086_irqs,
>> +	.num_irqs = ARRAY_SIZE(tps65086_irqs),
>> +};
>> +
>> +static const struct of_device_id tps65086_of_match_table[] = {
>> +	{ .compatible = "ti,tps65086", },
>> +	{ /* sentinel */ }
>> +};
>> +
>> +static int tps65086_probe(struct i2c_client *client,
>> +			  const struct i2c_device_id *ids)
>> +{
>> +	struct tps65086 *tps;
>> +	unsigned int version;
>> +	int ret;
>> +
>> +	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
>> +	if (!tps)
>> +		return -ENOMEM;
>> +
>> +	i2c_set_clientdata(client, tps);
>> +	tps->dev = &client->dev;
>> +	tps->irq = client->irq;
>> +
>> +	tps->regmap = devm_regmap_init_i2c(client, &tps65086_regmap_config);
>> +	if (IS_ERR(tps->regmap)) {
>> +		dev_err(tps->dev, "Failed to initialize register map\n");
>> +		return PTR_ERR(tps->regmap);
>> +	}
>> +
>> +	ret = regmap_read(tps->regmap, TPS65086_DEVICEID, &version);
>> +	if (ret < 0) {
>> +		dev_err(tps->dev, "Failed to read revision register\n");
>> +		return ret;
>> +	}
>> +
>> +	dev_info(tps->dev, "Device: TPS65086%01lX, OTP: %c, Rev: %ld\n",
>> +		 (version & TPS65086_DEVICEID_PART_MASK),
>> +		 (char)((version & TPS65086_DEVICEID_OTP_MASK) >> 4) + 'A',
>> +		 (version & TPS65086_DEVICEID_REV_MASK) >> 6);
>> +
>> +	ret = regmap_add_irq_chip(tps->regmap, tps->irq, IRQF_ONESHOT, 0,
>> +				  &tps65086_irq_chip, &tps->irq_data);
>> +	if (ret < 0) {
>
> if (ret)
>

ACK

>> +		dev_err(tps->dev, "Failed to register IRQ chip\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_platform_populate(tps->dev->of_node, NULL, NULL, tps->dev);
>> +	if (ret < 0) {
>
> if (ret)
>

ACK

>> +		regmap_del_irq_chip(tps->irq, tps->irq_data);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int tps65086_remove(struct i2c_client *client)
>> +{
>> +	struct tps65086 *tps = i2c_get_clientdata(client);
>> +
>> +	regmap_del_irq_chip(tps->irq, tps->irq_data);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id tps65086_id_table[] = {
>> +	{ "tps65086", 0 },
>> +	{ /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(i2c, tps65086_id_table);
>> +
>> +static struct i2c_driver tps65086_driver = {
>> +	.driver		= {
>> +		.name	= "tps65086",
>> +		.of_match_table = tps65086_of_match_table,
>> +	},
>> +	.probe		= tps65086_probe,
>> +	.remove		= tps65086_remove,
>> +	.id_table       = tps65086_id_table,
>> +};
>> +
>> +module_i2c_driver(tps65086_driver);
>> +
>> +MODULE_AUTHOR("Andrew F. Davis <afd@ti.com>");
>> +MODULE_DESCRIPTION("TPS65086x PMIC Driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/mfd/tps65086.h b/include/linux/mfd/tps65086.h
>> new file mode 100644
>> index 0000000..2a9e9dd
>> --- /dev/null
>> +++ b/include/linux/mfd/tps65086.h
>> @@ -0,0 +1,120 @@
>> +/*
>> + * TI TPS65086x
>> + *
>> + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com/
>> + *
>> + * Author: Andrew F. Davis <afd@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether expressed or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License version 2 for more details.
>> + *
>> + * Based on the TPS65912 driver
>> + */
>> +
>> +#ifndef __LINUX_MFD_TPS65086_H
>> +#define __LINUX_MFD_TPS65086_H
>> +
>> +#include <linux/device.h>
>> +#include <linux/regmap.h>
>> +
>> +/* List of registers for TPS65086 */
>> +#define TPS65086_DEVICEID		0x01
>> +#define TPS65086_IRQ			0x02
>> +#define TPS65086_IRQ_MASK		0x03
>> +#define TPS65086_PMICSTAT		0x04
>> +#define TPS65086_SHUTDNSRC		0x05
>> +#define TPS65086_BUCK1CTRL		0x20
>> +#define TPS65086_BUCK2CTRL		0x21
>> +#define TPS65086_BUCK3DECAY		0x22
>> +#define TPS65086_BUCK3VID		0x23
>> +#define TPS65086_BUCK3SLPCTRL		0x24
>> +#define TPS65086_BUCK4CTRL		0x25
>> +#define TPS65086_BUCK5CTRL		0x26
>> +#define TPS65086_BUCK6CTRL		0x27
>> +#define TPS65086_LDOA2CTRL		0x28
>> +#define TPS65086_LDOA3CTRL		0x29
>> +#define TPS65086_DISCHCTRL1		0x40
>> +#define TPS65086_DISCHCTRL2		0x41
>> +#define TPS65086_DISCHCTRL3		0x42
>> +#define TPS65086_PG_DELAY1		0x43
>> +#define TPS65086_FORCESHUTDN		0x91
>> +#define TPS65086_BUCK1SLPCTRL		0x92
>> +#define TPS65086_BUCK2SLPCTRL		0x93
>> +#define TPS65086_BUCK4VID		0x94
>> +#define TPS65086_BUCK4SLPVID		0x95
>> +#define TPS65086_BUCK5VID		0x96
>> +#define TPS65086_BUCK5SLPVID		0x97
>> +#define TPS65086_BUCK6VID		0x98
>> +#define TPS65086_BUCK6SLPVID		0x99
>> +#define TPS65086_LDOA2VID		0x9A
>> +#define TPS65086_LDOA3VID		0x9B
>> +#define TPS65086_BUCK123CTRL		0x9C
>> +#define TPS65086_PG_DELAY2		0x9D
>> +#define TPS65086_PIN_EN_MASK1		0x9E
>> +#define TPS65086_PIN_EN_MASK2		0x9F
>> +#define TPS65086_SWVTT_EN		0x9F
>> +#define TPS65086_PIN_EN_OVR1		0xA0
>> +#define TPS65086_PIN_EN_OVR2		0xA1
>> +#define TPS65086_GPOCTRL		0xA1
>> +#define TPS65086_PWR_FAULT_MASK1	0xA2
>> +#define TPS65086_PWR_FAULT_MASK2	0xA3
>> +#define TPS65086_GPO1PG_CTRL1		0xA4
>> +#define TPS65086_GPO1PG_CTRL2		0xA5
>> +#define TPS65086_GPO4PG_CTRL1		0xA6
>> +#define TPS65086_GPO4PG_CTRL2		0xA7
>> +#define TPS65086_GPO2PG_CTRL1		0xA8
>> +#define TPS65086_GPO2PG_CTRL2		0xA9
>> +#define TPS65086_GPO3PG_CTRL1		0xAA
>> +#define TPS65086_GPO3PG_CTRL2		0xAB
>> +#define TPS65086_LDOA1CTRL		0xAE
>> +#define TPS65086_PG_STATUS1		0xB0
>> +#define TPS65086_PG_STATUS2		0xB1
>> +#define TPS65086_PWR_FAULT_STATUS1	0xB2
>> +#define TPS65086_PWR_FAULT_STATUS2	0xB3
>> +#define TPS65086_TEMPCRIT		0xB4
>> +#define TPS65086_TEMPHOT		0xB5
>> +#define TPS65086_OC_STATUS		0xB6
>> +
>> +/* IRQ Register field definitions */
>> +#define TPS65086_IRQ_DIETEMP_MASK	BIT(0)
>> +#define TPS65086_IRQ_SHUTDN_MASK	BIT(3)
>> +#define TPS65086_IRQ_FAULT_MASK		BIT(7)
>> +
>> +/* DEVICEID Register field definitions */
>> +#define TPS65086_DEVICEID_PART_MASK	GENMASK(3, 0)
>> +#define TPS65086_DEVICEID_OTP_MASK	GENMASK(5, 4)
>> +#define TPS65086_DEVICEID_REV_MASK	GENMASK(7, 6)
>> +
>> +/* VID Masks */
>> +#define BUCK_VID_MASK			GENMASK(7, 1)
>> +#define VDOA1_VID_MASK			GENMASK(4, 1)
>> +#define VDOA23_VID_MASK			GENMASK(3, 0)
>> +
>> +/* Define the TPS65086 IRQ numbers */
>> +enum tps65086_irqs {
>> +	TPS65086_IRQ_DIETEMP,
>> +	TPS65086_IRQ_SHUTDN,
>> +	TPS65086_IRQ_FAULT,
>> +};
>> +
>> +/*
>> + * struct tps65086 - state holder for the tps65086 driver
>> + *
>> + * Device data may be used to access the TPS65086 chip
>> + */
>> +struct tps65086 {
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +
>> +	/* IRQ Data */
>> +	int irq;
>> +	struct regmap_irq_chip_data *irq_data;
>> +};
>> +
>> +#endif /*  __LINUX_MFD_TPS65086_H */
>

-- 
Andrew F. Davis

Re: [PATCH 2/4] mfd: tps65086: Add driver for the TPS65086 PMIC

[Lee Jones]

On Mon, 19 Oct 2015, Andrew F. Davis wrote:
> On 10/19/2015 04:23 AM, Lee Jones wrote:
> >On Fri, 16 Oct 2015, Andrew F. Davis wrote:
> >
> >>Add support for the TPS65912 device. It provides communication
> >>through I2C and contains the following components:
> >>
> >>  - Regulators
> >>  - Load switches
> >>  - GPO controller
> >
> >What's a GPO controller?
> 
> General Purpose Output, this controller only has output control, it's
> listed in the data-sheet this way, and other drivers have used this term.

Okay.

> >>Signed-off-by: Andrew F. Davis <afd@ti.com>
> >>---
> >>  drivers/mfd/Kconfig          |  13 ++++
> >>  drivers/mfd/Makefile         |   1 +
> >>  drivers/mfd/tps65086.c       | 145 +++++++++++++++++++++++++++++++++++++++++++
> >>  include/linux/mfd/tps65086.h | 120 +++++++++++++++++++++++++++++++++++
> >>  4 files changed, 279 insertions(+)
> >>  create mode 100644 drivers/mfd/tps65086.c
> >>  create mode 100644 include/linux/mfd/tps65086.h
> >>
> >>diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >>index 99d6367..ab3b392 100644
> >>--- a/drivers/mfd/Kconfig
> >>+++ b/drivers/mfd/Kconfig
> >>@@ -1093,6 +1093,19 @@ config TPS6507X
> >>  	  This driver can also be built as a module.  If so, the module
> >>  	  will be called tps6507x.
> >>
> >>+config MFD_TPS65086
> >>+	tristate "TI TPS65086x Power Management chips"
> >
> >Use the full name:
> >
> >   "TI TPS65086x Power Management Integrated Chips (PMICs)"
> >
> >>+	select REGMAP
> >>+	select REGMAP_IRQ
> >>+	select REGMAP_I2C
> >>+	depends on I2C
> >
> >REGMAP_I2C already depends on I2C.
> >
> 
> So if I force REGMAP_I2C to =y without depending on I2C then couldn't
> REGMAP_I2C get selected without I2C enabled? Selecting doesn't force
> the selected item's dependencies still right?

Try it out. :)

> >>+	depends on OF || COMPILE_TEST
> >>+	help
> >>+	  If you say yes here you get support for the TPS65086 series of
> >>+	  Power Management chips.
> >
> >I'd remove the line break.
> >
> >>+	  These include voltage regulators, GPO and other features
> >
> >GPIO?
> >
> >>+	  that are often used in portable devices.
> >
> >This is meant to be a 'help', so instead of staying "and some other
> >stuff", please list them.
> >
> 
> This seems to be what other devices do, I'll change it to something
> else then.

If you know what the "other features" are, it's best to name them.

> >>  config TPS65911_COMPARATOR
> >>  	tristate
> >>
> >>diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> >>index a59e3fc..7adb902 100644
> >>--- a/drivers/mfd/Makefile
> >>+++ b/drivers/mfd/Makefile
> >>@@ -66,6 +66,7 @@ obj-$(CONFIG_MFD_WM8994)	+= wm8994-core.o wm8994-irq.o wm8994-regmap.o
> >>  obj-$(CONFIG_TPS6105X)		+= tps6105x.o
> >>  obj-$(CONFIG_TPS65010)		+= tps65010.o
> >>  obj-$(CONFIG_TPS6507X)		+= tps6507x.o
> >>+obj-$(CONFIG_MFD_TPS65086)	+= tps65086.o
> >>  obj-$(CONFIG_MFD_TPS65217)	+= tps65217.o
> >>  obj-$(CONFIG_MFD_TPS65218)	+= tps65218.o
> >>  obj-$(CONFIG_MFD_TPS65910)	+= tps65910.o
> >>diff --git a/drivers/mfd/tps65086.c b/drivers/mfd/tps65086.c
> >>new file mode 100644
> >>index 0000000..be6d678
> >>--- /dev/null
> >>+++ b/drivers/mfd/tps65086.c
> >>@@ -0,0 +1,145 @@
> >>+/*
> >>+ * Driver for TI TPS65086x PMICs
> >
> >I'd suggest removing the driver part.  We know it's a driver.
> 
> Again this is what almost every other driver does,

Almost every other driver.  That's some statement.

I count 42%. ;)

> I'm not sure how
> else to describe this file without using the word driver.

Perhaps I'm getting picky in my old age, but I think I'd prefer to see:

  TI TPS65086x PMIC support

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Lee Jones]

On Mon, 19 Oct 2015, Andrew F. Davis wrote:

> On 10/19/2015 10:21 AM, Lee Jones wrote:
> >On Mon, 19 Oct 2015, Andrew F. Davis wrote:
> >
> >>On 10/19/2015 04:13 AM, Lee Jones wrote:
> >>>On Fri, 16 Oct 2015, Andrew F. Davis wrote:
> >>>
> >>>>The TPS65086 PMIC contains several regulators and a GPO controller.
> >>>>Add bindings for the TPS65086 PMIC.
> >>>>
> >>>>Signed-off-by: Andrew F. Davis <afd@ti.com>
> >>>>---
> >>>>  .../devicetree/bindings/gpio/gpio-tps65086.txt     | 17 ++++++++
> >>>>  Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++
> >>>>  .../bindings/regulator/tps65086-regulator.txt      | 36 +++++++++++++++++
> >>>
> >>>Please split these up into separate patches.
> >>>
> >>>There is no functional reason to bundle them up.
> >>>
> >>
> >>ACK
> >>
> >>>>  3 files changed, 99 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt
> >>>>  create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt
> >>>>  create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt
> >>>
> >>>[...]
> >>>
> >>>>diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt
> >>>>new file mode 100644
> >>>>index 0000000..4b6aeb4
> >>>>--- /dev/null
> >>>>+++ b/Documentation/devicetree/bindings/mfd/tps65086.txt
> >>>>@@ -0,0 +1,46 @@
> >>>>+* TPS65086 Power Management Integrated Circuit bindings
> >>>>+
> >>>>+Required properties:
> >>>>+ - compatible		: Should be "ti,tps65086".
> >>>
> >>>Any indication that it's a PMIC?
> >>>
> >>
> >>In the compatible string?
> >
> >Ya.
> >
> 
> Not sure what you mean then?, no one else seems to be doing that,
> "xx,xxxxxxx-pmic" is usually used for matching the regulator node,
> not the device itself.

Either the driver is MFD is the PMIC or it's not.

If it is, the compatible should reflect that, if isn't not then the
description in the header comment and the one above is not correct.

IMO, 'pmic' should not be used in the regulator compatible strings, as
it's a general description of the overall device.  The regulators are
just a component of that device.

> >>>>+ - reg			: Slave address.
> >>>
> >>>I2C/SPI?
> >>>
> >>
> >>ACK
> >>
> >>>>+ - interrupt-parent	: The parent interrupt controller.
> >>>
> >>>Phandled to ...
> >>>
> >>
> >>ACK
> >>
> >>>>+ - interrupts		: The interrupt line the device is connected to.
> >>>>+ - interrupt-controller	: Marks the device node as an interrupt controller.
> >>>>+ - #interrupt-cells	: The number of cells to describe an IRQ, this
> >>>>+			  should be 2. The first cell is the IRQ number.
> >>>>+			  The second cell is the flags, encoded as the trigger
> >>>>+			  masks from ../interrupt-controller/interrupts.txt.
> >>>
> >>>Masks?  What masks?
> >>>
> >>>Best to make a link to the header where the flags are defined here.
> >>>
> >>
> >>ACK
> >>
> >>>>+Additional nodes defined in:
> >>>>+ - Regulators	: ../regulator/tps65086-regulator.txt.
> >>>>+ - GPIO		: ../gpio/gpio-tps65086.txt.
> >>>
> >>>I'd suggest removing the full stops from all of the lines above.
> >>>
> >>>Just treat them as bullet points like we normally do.
> >>>
> >>
> >>ACK
> >>
> >>>>+Example:
> >>>>+
> >>>>+	pmic: tps65086@5e {
> >>>>+		compatible = "ti,tps65086";
> >>>>+		reg = <0x5e>;
> >>>>+		interrupt-parent = <&gpio1>;
> >>>>+		interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
> >>>>+		interrupt-controller;
> >>>>+		#interrupt-cells = <2>;
> >>>>+
> >>>>+		regulators {
> >>>>+			compatible = "ti,tps65086-regulator";
> >>>>+
> >>>>+			buck1 {
> >>>>+				regulator-name = "vcc1";
> >>>>+				regulator-min-microvolt = <1600000>;
> >>>>+				regulator-max-microvolt = <1600000>;
> >>>>+				regulator-boot-on;
> >>>>+				ti,regulator-decay;
> >>>>+				ti,regulator-step-size-25mv;
> >>>>+			};
> >>>>+		};
> >>>>+
> >>>>+		gpio4: tps65086_gpio {
> >>>>+			compatible = "ti,tps65086-gpio";
> >>>>+			gpio-controller;
> >>>>+			#gpio-cells = <2>;
> >>>>+		};
> >>>>+	};
> >>>
> >>>[...]
> >>>
> >>
> >
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Andrew F. Davis]

On 10/20/2015 06:31 AM, Lee Jones wrote:
> On Mon, 19 Oct 2015, Andrew F. Davis wrote:
>
>> On 10/19/2015 10:21 AM, Lee Jones wrote:
>>> On Mon, 19 Oct 2015, Andrew F. Davis wrote:
>>>
>>>> On 10/19/2015 04:13 AM, Lee Jones wrote:
>>>>> On Fri, 16 Oct 2015, Andrew F. Davis wrote:
>>>>>
>>>>>> The TPS65086 PMIC contains several regulators and a GPO controller.
>>>>>> Add bindings for the TPS65086 PMIC.
>>>>>>
>>>>>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>>>>>> ---
>>>>>>   .../devicetree/bindings/gpio/gpio-tps65086.txt     | 17 ++++++++
>>>>>>   Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++
>>>>>>   .../bindings/regulator/tps65086-regulator.txt      | 36 +++++++++++++++++
>>>>>
>>>>> Please split these up into separate patches.
>>>>>
>>>>> There is no functional reason to bundle them up.
>>>>>
>>>>
>>>> ACK
>>>>
>>>>>>   3 files changed, 99 insertions(+)
>>>>>>   create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt
>>>>>>   create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt
>>>>>>   create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt
>>>>>
>>>>> [...]
>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt
>>>>>> new file mode 100644
>>>>>> index 0000000..4b6aeb4
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/mfd/tps65086.txt
>>>>>> @@ -0,0 +1,46 @@
>>>>>> +* TPS65086 Power Management Integrated Circuit bindings
>>>>>> +
>>>>>> +Required properties:
>>>>>> + - compatible		: Should be "ti,tps65086".
>>>>>
>>>>> Any indication that it's a PMIC?
>>>>>
>>>>
>>>> In the compatible string?
>>>
>>> Ya.
>>>
>>
>> Not sure what you mean then?, no one else seems to be doing that,
>> "xx,xxxxxxx-pmic" is usually used for matching the regulator node,
>> not the device itself.
>
> Either the driver is MFD is the PMIC or it's not.
>
> If it is, the compatible should reflect that, if isn't not then the
> description in the header comment and the one above is not correct.
>
> IMO, 'pmic' should not be used in the regulator compatible strings, as
> it's a general description of the overall device.  The regulators are
> just a component of that device.
>

I agree about not using compatible in regulator nodes (Mark Brown and I had
a discussion on this topic), what I mean with "xxxxxxx-pmic" is for the MFD
core to match sub-drivers (mfd_cells). For example drivers/mfd/tps80031.c,
matches the regulator with "tps80031-pmic", the clock with "tps80031-clock",
the charger with "tps80031-charger", etc..

The core device itself is just matched with "ti,tps65086" through the I2C bus
matching.

I could change the core to be "tps65086-pmic", then call the regulator
driver "tps65086-regulator" if this works for you, this seems to be the
way new drivers name the regulator driver (max77843.c).

>>>>>> + - reg			: Slave address.
>>>>>
>>>>> I2C/SPI?
>>>>>
>>>>
>>>> ACK
>>>>
>>>>>> + - interrupt-parent	: The parent interrupt controller.
>>>>>
>>>>> Phandled to ...
>>>>>
>>>>
>>>> ACK
>>>>
>>>>>> + - interrupts		: The interrupt line the device is connected to.
>>>>>> + - interrupt-controller	: Marks the device node as an interrupt controller.
>>>>>> + - #interrupt-cells	: The number of cells to describe an IRQ, this
>>>>>> +			  should be 2. The first cell is the IRQ number.
>>>>>> +			  The second cell is the flags, encoded as the trigger
>>>>>> +			  masks from ../interrupt-controller/interrupts.txt.
>>>>>
>>>>> Masks?  What masks?
>>>>>
>>>>> Best to make a link to the header where the flags are defined here.
>>>>>
>>>>
>>>> ACK
>>>>
>>>>>> +Additional nodes defined in:
>>>>>> + - Regulators	: ../regulator/tps65086-regulator.txt.
>>>>>> + - GPIO		: ../gpio/gpio-tps65086.txt.
>>>>>
>>>>> I'd suggest removing the full stops from all of the lines above.
>>>>>
>>>>> Just treat them as bullet points like we normally do.
>>>>>
>>>>
>>>> ACK
>>>>
>>>>>> +Example:
>>>>>> +
>>>>>> +	pmic: tps65086@5e {
>>>>>> +		compatible = "ti,tps65086";
>>>>>> +		reg = <0x5e>;
>>>>>> +		interrupt-parent = <&gpio1>;
>>>>>> +		interrupts = <28 IRQ_TYPE_LEVEL_LOW>;
>>>>>> +		interrupt-controller;
>>>>>> +		#interrupt-cells = <2>;
>>>>>> +
>>>>>> +		regulators {
>>>>>> +			compatible = "ti,tps65086-regulator";
>>>>>> +
>>>>>> +			buck1 {
>>>>>> +				regulator-name = "vcc1";
>>>>>> +				regulator-min-microvolt = <1600000>;
>>>>>> +				regulator-max-microvolt = <1600000>;
>>>>>> +				regulator-boot-on;
>>>>>> +				ti,regulator-decay;
>>>>>> +				ti,regulator-step-size-25mv;
>>>>>> +			};
>>>>>> +		};
>>>>>> +
>>>>>> +		gpio4: tps65086_gpio {
>>>>>> +			compatible = "ti,tps65086-gpio";
>>>>>> +			gpio-controller;
>>>>>> +			#gpio-cells = <2>;
>>>>>> +		};
>>>>>> +	};
>>>>>
>>>>> [...]
>>>>>
>>>>
>>>
>>
>

Re: [PATCH 2/4] mfd: tps65086: Add driver for the TPS65086 PMIC

[Andrew F. Davis]

On 10/20/2015 05:02 AM, Lee Jones wrote:
> On Mon, 19 Oct 2015, Andrew F. Davis wrote:
>> On 10/19/2015 04:23 AM, Lee Jones wrote:
>>> On Fri, 16 Oct 2015, Andrew F. Davis wrote:
>>>
>>>> Add support for the TPS65912 device. It provides communication
>>>> through I2C and contains the following components:
>>>>
>>>>   - Regulators
>>>>   - Load switches
>>>>   - GPO controller
>>>
>>> What's a GPO controller?
>>
>> General Purpose Output, this controller only has output control, it's
>> listed in the data-sheet this way, and other drivers have used this term.
>
> Okay.
>
>>>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>>>> ---
>>>>   drivers/mfd/Kconfig          |  13 ++++
>>>>   drivers/mfd/Makefile         |   1 +
>>>>   drivers/mfd/tps65086.c       | 145 +++++++++++++++++++++++++++++++++++++++++++
>>>>   include/linux/mfd/tps65086.h | 120 +++++++++++++++++++++++++++++++++++
>>>>   4 files changed, 279 insertions(+)
>>>>   create mode 100644 drivers/mfd/tps65086.c
>>>>   create mode 100644 include/linux/mfd/tps65086.h
>>>>
>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>> index 99d6367..ab3b392 100644
>>>> --- a/drivers/mfd/Kconfig
>>>> +++ b/drivers/mfd/Kconfig
>>>> @@ -1093,6 +1093,19 @@ config TPS6507X
>>>>   	  This driver can also be built as a module.  If so, the module
>>>>   	  will be called tps6507x.
>>>>
>>>> +config MFD_TPS65086
>>>> +	tristate "TI TPS65086x Power Management chips"
>>>
>>> Use the full name:
>>>
>>>    "TI TPS65086x Power Management Integrated Chips (PMICs)"
>>>
>>>> +	select REGMAP
>>>> +	select REGMAP_IRQ
>>>> +	select REGMAP_I2C
>>>> +	depends on I2C
>>>
>>> REGMAP_I2C already depends on I2C.
>>>
>>
>> So if I force REGMAP_I2C to =y without depending on I2C then couldn't
>> REGMAP_I2C get selected without I2C enabled? Selecting doesn't force
>> the selected item's dependencies still right?
>
> Try it out. :)
>

OK, I just tried a little test and it did as I expected, I was able to
select a symbol and it did not enable its dependency, so this would
break build. So, I'm still confused, why does REGMAP_I2C depending
on I2C matter? What are you suggesting my Kconfig should look like?

>>>> +	depends on OF || COMPILE_TEST
>>>> +	help
>>>> +	  If you say yes here you get support for the TPS65086 series of
>>>> +	  Power Management chips.
>>>
>>> I'd remove the line break.
>>>
>>>> +	  These include voltage regulators, GPO and other features
>>>
>>> GPIO?
>>>
>>>> +	  that are often used in portable devices.
>>>
>>> This is meant to be a 'help', so instead of staying "and some other
>>> stuff", please list them.
>>>
>>
>> This seems to be what other devices do, I'll change it to something
>> else then.
>
> If you know what the "other features" are, it's best to name them.
>

I was giving a generic description of a PMIC, I didn't really want to
list all the features and start marketing the part in the Kconfig, unless
you want, I'm sure TI won't mind the advertisement. :)

I'm not sure why we write so much about these driver parts at all in the
help, people who need them know what they are, it's not like some core
kernel config option that people might be undecided on and need explained
its function.

>>>>   config TPS65911_COMPARATOR
>>>>   	tristate
>>>>
>>>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>>>> index a59e3fc..7adb902 100644
>>>> --- a/drivers/mfd/Makefile
>>>> +++ b/drivers/mfd/Makefile
>>>> @@ -66,6 +66,7 @@ obj-$(CONFIG_MFD_WM8994)	+= wm8994-core.o wm8994-irq.o wm8994-regmap.o
>>>>   obj-$(CONFIG_TPS6105X)		+= tps6105x.o
>>>>   obj-$(CONFIG_TPS65010)		+= tps65010.o
>>>>   obj-$(CONFIG_TPS6507X)		+= tps6507x.o
>>>> +obj-$(CONFIG_MFD_TPS65086)	+= tps65086.o
>>>>   obj-$(CONFIG_MFD_TPS65217)	+= tps65217.o
>>>>   obj-$(CONFIG_MFD_TPS65218)	+= tps65218.o
>>>>   obj-$(CONFIG_MFD_TPS65910)	+= tps65910.o
>>>> diff --git a/drivers/mfd/tps65086.c b/drivers/mfd/tps65086.c
>>>> new file mode 100644
>>>> index 0000000..be6d678
>>>> --- /dev/null
>>>> +++ b/drivers/mfd/tps65086.c
>>>> @@ -0,0 +1,145 @@
>>>> +/*
>>>> + * Driver for TI TPS65086x PMICs
>>>
>>> I'd suggest removing the driver part.  We know it's a driver.
>>
>> Again this is what almost every other driver does,
>
> Almost every other driver.  That's some statement.
>
> I count 42%. ;)
>

I'm finding in drivers/mfd ~89% of drivers mention at some point that
they are a driver. But I do admit my original statement was based on a
quick monte carlo selection of a couple files. :)

>> I'm not sure how
>> else to describe this file without using the word driver.
>
> Perhaps I'm getting picky in my old age, but I think I'd prefer to see:
>
>    TI TPS65086x PMIC support
>

Still not sure this describes the file, maybe we could just drop that
line completely from files with an obvious purpose? Doesn't really bother
me ether way.

> [...]
>

Re: [PATCH 2/4] mfd: tps65086: Add driver for the TPS65086 PMIC

[Lee Jones]

On Tue, 20 Oct 2015, Andrew F. Davis wrote:

> On 10/20/2015 05:02 AM, Lee Jones wrote:
> >On Mon, 19 Oct 2015, Andrew F. Davis wrote:
> >>On 10/19/2015 04:23 AM, Lee Jones wrote:
> >>>On Fri, 16 Oct 2015, Andrew F. Davis wrote:
> >>>
> >>>>Add support for the TPS65912 device. It provides communication
> >>>>through I2C and contains the following components:
> >>>>
> >>>>  - Regulators
> >>>>  - Load switches
> >>>>  - GPO controller
> >>>
> >>>What's a GPO controller?
> >>
> >>General Purpose Output, this controller only has output control, it's
> >>listed in the data-sheet this way, and other drivers have used this term.
> >
> >Okay.
> >
> >>>>Signed-off-by: Andrew F. Davis <afd@ti.com>
> >>>>---
> >>>>  drivers/mfd/Kconfig          |  13 ++++
> >>>>  drivers/mfd/Makefile         |   1 +
> >>>>  drivers/mfd/tps65086.c       | 145 +++++++++++++++++++++++++++++++++++++++++++
> >>>>  include/linux/mfd/tps65086.h | 120 +++++++++++++++++++++++++++++++++++
> >>>>  4 files changed, 279 insertions(+)
> >>>>  create mode 100644 drivers/mfd/tps65086.c
> >>>>  create mode 100644 include/linux/mfd/tps65086.h
> >>>>
> >>>>diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >>>>index 99d6367..ab3b392 100644
> >>>>--- a/drivers/mfd/Kconfig
> >>>>+++ b/drivers/mfd/Kconfig
> >>>>@@ -1093,6 +1093,19 @@ config TPS6507X
> >>>>  	  This driver can also be built as a module.  If so, the module
> >>>>  	  will be called tps6507x.
> >>>>
> >>>>+config MFD_TPS65086
> >>>>+	tristate "TI TPS65086x Power Management chips"
> >>>
> >>>Use the full name:
> >>>
> >>>   "TI TPS65086x Power Management Integrated Chips (PMICs)"
> >>>
> >>>>+	select REGMAP
> >>>>+	select REGMAP_IRQ
> >>>>+	select REGMAP_I2C
> >>>>+	depends on I2C
> >>>
> >>>REGMAP_I2C already depends on I2C.
> >>>
> >>
> >>So if I force REGMAP_I2C to =y without depending on I2C then couldn't
> >>REGMAP_I2C get selected without I2C enabled? Selecting doesn't force
> >>the selected item's dependencies still right?
> >
> >Try it out. :)
> >
> 
> OK, I just tried a little test and it did as I expected, I was able to
> select a symbol and it did not enable its dependency, so this would
> break build. So, I'm still confused, why does REGMAP_I2C depending
> on I2C matter? What are you suggesting my Kconfig should look like?

That's fine, if you've tested it and it's a problem the above will do.

I'm still little curious as to why it's necessary to depend on the
same symbols your dependencies depend on.

How far up the chain is it possible to go? 

config DEPA

config DEPB
       select DEPA

config DEPC
       select DEPB
       depends on DEPA

config DEPD
       select DEPC
       depends on DEPA
       depends on DEPB

config YOU
       select DEPD
       depends on DEPA
       depends on DEPB
       depends on DEPC

Etc.

> >>>>+	depends on OF || COMPILE_TEST
> >>>>+	help
> >>>>+	  If you say yes here you get support for the TPS65086 series of
> >>>>+	  Power Management chips.
> >>>
> >>>I'd remove the line break.
> >>>
> >>>>+	  These include voltage regulators, GPO and other features
> >>>
> >>>GPIO?
> >>>
> >>>>+	  that are often used in portable devices.
> >>>
> >>>This is meant to be a 'help', so instead of staying "and some other
> >>>stuff", please list them.
> >>>
> >>
> >>This seems to be what other devices do, I'll change it to something
> >>else then.
> >
> >If you know what the "other features" are, it's best to name them.
> 
> I was giving a generic description of a PMIC, I didn't really want to
> list all the features and start marketing the part in the Kconfig, unless
> you want, I'm sure TI won't mind the advertisement. :)
> 
> I'm not sure why we write so much about these driver parts at all in the
> help, people who need them know what they are, it's not like some core
> kernel config option that people might be undecided on and need explained
> its function.

I'm sure when you've been trying to find answers to questions you've
found the help helpful (or maybe it was poorly underwritten and you
didn't find it useful at all).  Without the help, all these drivers
are just a selection of random letters, numbers and code/project
names.  It's the help which brings them to life, so to speak.

Please describe the device and what it provides.  No sales pitch
required.

> >>>>  config TPS65911_COMPARATOR
> >>>>  	tristate
> >>>>
> >>>>diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> >>>>index a59e3fc..7adb902 100644
> >>>>--- a/drivers/mfd/Makefile
> >>>>+++ b/drivers/mfd/Makefile
> >>>>@@ -66,6 +66,7 @@ obj-$(CONFIG_MFD_WM8994)	+= wm8994-core.o wm8994-irq.o wm8994-regmap.o
> >>>>  obj-$(CONFIG_TPS6105X)		+= tps6105x.o
> >>>>  obj-$(CONFIG_TPS65010)		+= tps65010.o
> >>>>  obj-$(CONFIG_TPS6507X)		+= tps6507x.o
> >>>>+obj-$(CONFIG_MFD_TPS65086)	+= tps65086.o
> >>>>  obj-$(CONFIG_MFD_TPS65217)	+= tps65217.o
> >>>>  obj-$(CONFIG_MFD_TPS65218)	+= tps65218.o
> >>>>  obj-$(CONFIG_MFD_TPS65910)	+= tps65910.o
> >>>>diff --git a/drivers/mfd/tps65086.c b/drivers/mfd/tps65086.c
> >>>>new file mode 100644
> >>>>index 0000000..be6d678
> >>>>--- /dev/null
> >>>>+++ b/drivers/mfd/tps65086.c
> >>>>@@ -0,0 +1,145 @@
> >>>>+/*
> >>>>+ * Driver for TI TPS65086x PMICs
> >>>
> >>>I'd suggest removing the driver part.  We know it's a driver.
> >>
> >>Again this is what almost every other driver does,
> >
> >Almost every other driver.  That's some statement.
> >
> >I count 42%. ;)
> >
> 
> I'm finding in drivers/mfd ~89% of drivers mention at some point that
> they are a driver. But I do admit my original statement was based on a
> quick monte carlo selection of a couple files. :)

I'm sure many drivers, mention the word driver at one point.  However,
this is specifically regarding the file title/header.

> >>I'm not sure how
> >>else to describe this file without using the word driver.
> >
> >Perhaps I'm getting picky in my old age, but I think I'd prefer to see:
> >
> >   TI TPS65086x PMIC support
> >
> 
> Still not sure this describes the file, maybe we could just drop that
> line completely from files with an obvious purpose? Doesn't really bother
> me ether way.

Sure.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Lee Jones]

On Tue, 20 Oct 2015, Andrew F. Davis wrote:

> On 10/20/2015 06:31 AM, Lee Jones wrote:
> >On Mon, 19 Oct 2015, Andrew F. Davis wrote:
> >
> >>On 10/19/2015 10:21 AM, Lee Jones wrote:
> >>>On Mon, 19 Oct 2015, Andrew F. Davis wrote:
> >>>
> >>>>On 10/19/2015 04:13 AM, Lee Jones wrote:
> >>>>>On Fri, 16 Oct 2015, Andrew F. Davis wrote:
> >>>>>
> >>>>>>The TPS65086 PMIC contains several regulators and a GPO controller.
> >>>>>>Add bindings for the TPS65086 PMIC.
> >>>>>>
> >>>>>>Signed-off-by: Andrew F. Davis <afd@ti.com>
> >>>>>>---
> >>>>>>  .../devicetree/bindings/gpio/gpio-tps65086.txt     | 17 ++++++++
> >>>>>>  Documentation/devicetree/bindings/mfd/tps65086.txt | 46 ++++++++++++++++++++++
> >>>>>>  .../bindings/regulator/tps65086-regulator.txt      | 36 +++++++++++++++++
> >>>>>
> >>>>>Please split these up into separate patches.
> >>>>>
> >>>>>There is no functional reason to bundle them up.
> >>>>>
> >>>>
> >>>>ACK
> >>>>
> >>>>>>  3 files changed, 99 insertions(+)
> >>>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-tps65086.txt
> >>>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/tps65086.txt
> >>>>>>  create mode 100644 Documentation/devicetree/bindings/regulator/tps65086-regulator.txt
> >>>>>
> >>>>>[...]
> >>>>>
> >>>>>>diff --git a/Documentation/devicetree/bindings/mfd/tps65086.txt b/Documentation/devicetree/bindings/mfd/tps65086.txt
> >>>>>>new file mode 100644
> >>>>>>index 0000000..4b6aeb4
> >>>>>>--- /dev/null
> >>>>>>+++ b/Documentation/devicetree/bindings/mfd/tps65086.txt
> >>>>>>@@ -0,0 +1,46 @@
> >>>>>>+* TPS65086 Power Management Integrated Circuit bindings
> >>>>>>+
> >>>>>>+Required properties:
> >>>>>>+ - compatible		: Should be "ti,tps65086".
> >>>>>
> >>>>>Any indication that it's a PMIC?
> >>>>>
> >>>>
> >>>>In the compatible string?
> >>>
> >>>Ya.
> >>>
> >>
> >>Not sure what you mean then?, no one else seems to be doing that,
> >>"xx,xxxxxxx-pmic" is usually used for matching the regulator node,
> >>not the device itself.
> >
> >Either the driver is MFD is the PMIC or it's not.
> >
> >If it is, the compatible should reflect that, if isn't not then the
> >description in the header comment and the one above is not correct.
> >
> >IMO, 'pmic' should not be used in the regulator compatible strings, as
> >it's a general description of the overall device.  The regulators are
> >just a component of that device.
> >
> 
> I agree about not using compatible in regulator nodes (Mark Brown and I had
> a discussion on this topic), what I mean with "xxxxxxx-pmic" is for the MFD
> core to match sub-drivers (mfd_cells). For example drivers/mfd/tps80031.c,
> matches the regulator with "tps80031-pmic", the clock with "tps80031-clock",
> the charger with "tps80031-charger", etc..
> 
> The core device itself is just matched with "ti,tps65086" through the I2C bus
> matching.
> 
> I could change the core to be "tps65086-pmic", then call the regulator
> driver "tps65086-regulator" if this works for you, this seems to be the
> way new drivers name the regulator driver (max77843.c).

Yes, this is what I was thinking of.

ti,tps65086 is uninformative/undescriptive and tells me nothing.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Mark Brown]

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

On Wed, Oct 21, 2015 at 09:46:33AM +0100, Lee Jones wrote:
> On Tue, 20 Oct 2015, Andrew F. Davis wrote:

> > I could change the core to be "tps65086-pmic", then call the regulator
> > driver "tps65086-regulator" if this works for you, this seems to be the
> > way new drivers name the regulator driver (max77843.c).

> Yes, this is what I was thinking of.

> ti,tps65086 is uninformative/undescriptive and tells me nothing.

It is however the normal way we write compatible strings - the class
information would normaly go in the node name (ie, i2c@7000c000 or
whatever).

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

Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Lee Jones]

On Wed, 21 Oct 2015, Mark Brown wrote:

> On Wed, Oct 21, 2015 at 09:46:33AM +0100, Lee Jones wrote:
> > On Tue, 20 Oct 2015, Andrew F. Davis wrote:
> 
> > > I could change the core to be "tps65086-pmic", then call the regulator
> > > driver "tps65086-regulator" if this works for you, this seems to be the
> > > way new drivers name the regulator driver (max77843.c).
> 
> > Yes, this is what I was thinking of.
> 
> > ti,tps65086 is uninformative/undescriptive and tells me nothing.
> 
> It is however the normal way we write compatible strings - the class
> information would normaly go in the node name (ie, i2c@7000c000 or
> whatever).

I didn't say it hasn't been done before, just that I didn't like it
for the aforementioned reasons.  I can also find 1000's of compatible
strings which do append "-<device_type>", so it's not exactly an
unheard of practice.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Mark Brown]

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

On Wed, Oct 21, 2015 at 12:18:32PM +0100, Lee Jones wrote:
> On Wed, 21 Oct 2015, Mark Brown wrote:
> > On Wed, Oct 21, 2015 at 09:46:33AM +0100, Lee Jones wrote:
> > > On Tue, 20 Oct 2015, Andrew F. Davis wrote:

> > It is however the normal way we write compatible strings - the class
> > information would normaly go in the node name (ie, i2c@7000c000 or
> > whatever).

> I didn't say it hasn't been done before, just that I didn't like it
> for the aforementioned reasons.  I can also find 1000's of compatible
> strings which do append "-<device_type>", so it's not exactly an
> unheard of practice.

It's a pretty substantial change in the way we make compatible strings
that we probably want to discuss more widely if we want to adopt it -
we've not been using that idiom and it's pretty surprising.  I'm not
really sure it help much and we do already have the pre-@ noise words
for this purpose (as well as comments in the DT).

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

Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Lee Jones]

On Wed, 21 Oct 2015, Mark Brown wrote:

> On Wed, Oct 21, 2015 at 12:18:32PM +0100, Lee Jones wrote:
> > On Wed, 21 Oct 2015, Mark Brown wrote:
> > > On Wed, Oct 21, 2015 at 09:46:33AM +0100, Lee Jones wrote:
> > > > On Tue, 20 Oct 2015, Andrew F. Davis wrote:
> 
> > > It is however the normal way we write compatible strings - the class
> > > information would normaly go in the node name (ie, i2c@7000c000 or
> > > whatever).
> 
> > I didn't say it hasn't been done before, just that I didn't like it
> > for the aforementioned reasons.  I can also find 1000's of compatible
> > strings which do append "-<device_type>", so it's not exactly an
> > unheard of practice.
> 
> It's a pretty substantial change in the way we make compatible strings
> that we probably want to discuss more widely if we want to adopt it -
> we've not been using that idiom and it's pretty surprising.  I'm not
> really sure it help much and we do already have the pre-@ noise words
> for this purpose (as well as comments in the DT).

I'm not *that* fussed about it to justify starting-up wider community
discussions.

My only point is that:

  compatible = "<vendor>,udw9283";

... is meaningless gibberish and I think it'd be better to be more
forthcoming which prevents having to dig around in DTS files for the
node name/label for true device/type identification.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] Documentation: tps65086: Add DT bindings for the TPS65086 PMIC

[Mark Brown]

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

On Wed, Oct 21, 2015 at 04:26:59PM +0100, Lee Jones wrote:
> On Wed, 21 Oct 2015, Mark Brown wrote:
> > On Wed, Oct 21, 2015 at 12:18:32PM +0100, Lee Jones wrote:

> > > I didn't say it hasn't been done before, just that I didn't like it
> > > for the aforementioned reasons.  I can also find 1000's of compatible
> > > strings which do append "-<device_type>", so it's not exactly an
> > > unheard of practice.

> > It's a pretty substantial change in the way we make compatible strings
> > that we probably want to discuss more widely if we want to adopt it -
> > we've not been using that idiom and it's pretty surprising.  I'm not
> > really sure it help much and we do already have the pre-@ noise words
> > for this purpose (as well as comments in the DT).

> I'm not *that* fussed about it to justify starting-up wider community
> discussions.

> My only point is that:

>   compatible = "<vendor>,udw9283";

> ... is meaningless gibberish and I think it'd be better to be more
> forthcoming which prevents having to dig around in DTS files for the
> node name/label for true device/type identification.

What I suspect most people actually do here is google the part name if
it's not obvious from context (presumably people with a compatible
string only are either looking at the driver or some DT source
instantiating it).

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

Re: [PATCH 2/4] mfd: tps65086: Add driver for the TPS65086 PMIC

[Andrew F. Davis]

On 10/21/2015 03:43 AM, Lee Jones wrote:
> On Tue, 20 Oct 2015, Andrew F. Davis wrote:
>
>> On 10/20/2015 05:02 AM, Lee Jones wrote:
>>> On Mon, 19 Oct 2015, Andrew F. Davis wrote:
>>>> On 10/19/2015 04:23 AM, Lee Jones wrote:
>>>>> On Fri, 16 Oct 2015, Andrew F. Davis wrote:
>>>>>
>>>>>> Add support for the TPS65912 device. It provides communication
>>>>>> through I2C and contains the following components:
>>>>>>
>>>>>>   - Regulators
>>>>>>   - Load switches
>>>>>>   - GPO controller
>>>>>
>>>>> What's a GPO controller?
>>>>
>>>> General Purpose Output, this controller only has output control, it's
>>>> listed in the data-sheet this way, and other drivers have used this term.
>>>
>>> Okay.
>>>
>>>>>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>>>>>> ---
>>>>>>   drivers/mfd/Kconfig          |  13 ++++
>>>>>>   drivers/mfd/Makefile         |   1 +
>>>>>>   drivers/mfd/tps65086.c       | 145 +++++++++++++++++++++++++++++++++++++++++++
>>>>>>   include/linux/mfd/tps65086.h | 120 +++++++++++++++++++++++++++++++++++
>>>>>>   4 files changed, 279 insertions(+)
>>>>>>   create mode 100644 drivers/mfd/tps65086.c
>>>>>>   create mode 100644 include/linux/mfd/tps65086.h
>>>>>>
>>>>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>>>>> index 99d6367..ab3b392 100644
>>>>>> --- a/drivers/mfd/Kconfig
>>>>>> +++ b/drivers/mfd/Kconfig
>>>>>> @@ -1093,6 +1093,19 @@ config TPS6507X
>>>>>>   	  This driver can also be built as a module.  If so, the module
>>>>>>   	  will be called tps6507x.
>>>>>>
>>>>>> +config MFD_TPS65086
>>>>>> +	tristate "TI TPS65086x Power Management chips"
>>>>>
>>>>> Use the full name:
>>>>>
>>>>>    "TI TPS65086x Power Management Integrated Chips (PMICs)"
>>>>>
>>>>>> +	select REGMAP
>>>>>> +	select REGMAP_IRQ
>>>>>> +	select REGMAP_I2C
>>>>>> +	depends on I2C
>>>>>
>>>>> REGMAP_I2C already depends on I2C.
>>>>>
>>>>
>>>> So if I force REGMAP_I2C to =y without depending on I2C then couldn't
>>>> REGMAP_I2C get selected without I2C enabled? Selecting doesn't force
>>>> the selected item's dependencies still right?
>>>
>>> Try it out. :)
>>>
>>
>> OK, I just tried a little test and it did as I expected, I was able to
>> select a symbol and it did not enable its dependency, so this would
>> break build. So, I'm still confused, why does REGMAP_I2C depending
>> on I2C matter? What are you suggesting my Kconfig should look like?
>
> That's fine, if you've tested it and it's a problem the above will do.
>
> I'm still little curious as to why it's necessary to depend on the
> same symbols your dependencies depend on.
>

Because I don't 'depend' on REGMAP_I2C, I 'select' it. So none of its
dependencies need be met, so I have to depend on them for REGMAP_I2C.

'Depending' on REGMAP_I2C would also be undesirable as my symbol would
only be visible when REGMAP_I2C is =y, this would obscure my symbol as
no one would know to turn on REGMAP_I2C to make my driver appear. I
only need I2C and then I can go manually enable all my random helper
symbols myself if I'm selected.

> How far up the chain is it possible to go?
>
> config DEPA
>
> config DEPB
>         select DEPA
>
> config DEPC
>         select DEPB
>         depends on DEPA
>
> config DEPD
>         select DEPC
>         depends on DEPA
>         depends on DEPB
>
> config YOU
>         select DEPD
>         depends on DEPA
>         depends on DEPB
>         depends on DEPC
>
> Etc.
>

To me it seems like a fundamental issue with how the Kconfig system
was originally designed (a quick check shows it existed in some form
20 years ago...) that leads to this kind of thing, if they knew how
it would be used today they probably would have made it more
directed-tree like, without the reverse dependencies and all.

>>>>>> +	depends on OF || COMPILE_TEST
>>>>>> +	help
>>>>>> +	  If you say yes here you get support for the TPS65086 series of
>>>>>> +	  Power Management chips.
>>>>>
>>>>> I'd remove the line break.
>>>>>
>>>>>> +	  These include voltage regulators, GPO and other features
>>>>>
>>>>> GPIO?
>>>>>
>>>>>> +	  that are often used in portable devices.
>>>>>
>>>>> This is meant to be a 'help', so instead of staying "and some other
>>>>> stuff", please list them.
>>>>>
>>>>
>>>> This seems to be what other devices do, I'll change it to something
>>>> else then.
>>>
>>> If you know what the "other features" are, it's best to name them.
>>
>> I was giving a generic description of a PMIC, I didn't really want to
>> list all the features and start marketing the part in the Kconfig, unless
>> you want, I'm sure TI won't mind the advertisement. :)
>>
>> I'm not sure why we write so much about these driver parts at all in the
>> help, people who need them know what they are, it's not like some core
>> kernel config option that people might be undecided on and need explained
>> its function.
>
> I'm sure when you've been trying to find answers to questions you've
> found the help helpful (or maybe it was poorly underwritten and you
> didn't find it useful at all).  Without the help, all these drivers
> are just a selection of random letters, numbers and code/project
> names.  It's the help which brings them to life, so to speak.
>

Well sure, they have been a lot of help for core config where I might
not know what CONFIG_PERF_EVENTS means and a long explanation is nice,
but I know what a TPS65086 is, if I didn't I wouldn't be trying to
enable the driver for it.

> Please describe the device and what it provides.  No sales pitch
> required.
>

I'll see what I can think up. "These include voltage regulators, GPIO
and other features that are often used in portable devices." Really
does give the gist of it for uninformed users who might want to know
why this is enabled for their computer/board.

Honestly I don't think the part actually does anything else, but saying
that when other similar parts add the "other features" line is kinda
odd and might single out this part as looking less useful.

>>>>>>   config TPS65911_COMPARATOR
>>>>>>   	tristate
>>>>>>
>>>>>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>>>>>> index a59e3fc..7adb902 100644
>>>>>> --- a/drivers/mfd/Makefile
>>>>>> +++ b/drivers/mfd/Makefile
>>>>>> @@ -66,6 +66,7 @@ obj-$(CONFIG_MFD_WM8994)	+= wm8994-core.o wm8994-irq.o wm8994-regmap.o
>>>>>>   obj-$(CONFIG_TPS6105X)		+= tps6105x.o
>>>>>>   obj-$(CONFIG_TPS65010)		+= tps65010.o
>>>>>>   obj-$(CONFIG_TPS6507X)		+= tps6507x.o
>>>>>> +obj-$(CONFIG_MFD_TPS65086)	+= tps65086.o
>>>>>>   obj-$(CONFIG_MFD_TPS65217)	+= tps65217.o
>>>>>>   obj-$(CONFIG_MFD_TPS65218)	+= tps65218.o
>>>>>>   obj-$(CONFIG_MFD_TPS65910)	+= tps65910.o
>>>>>> diff --git a/drivers/mfd/tps65086.c b/drivers/mfd/tps65086.c
>>>>>> new file mode 100644
>>>>>> index 0000000..be6d678
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/mfd/tps65086.c
>>>>>> @@ -0,0 +1,145 @@
>>>>>> +/*
>>>>>> + * Driver for TI TPS65086x PMICs
>>>>>
>>>>> I'd suggest removing the driver part.  We know it's a driver.
>>>>
>>>> Again this is what almost every other driver does,
>>>
>>> Almost every other driver.  That's some statement.
>>>
>>> I count 42%. ;)
>>>
>>
>> I'm finding in drivers/mfd ~89% of drivers mention at some point that
>> they are a driver. But I do admit my original statement was based on a
>> quick monte carlo selection of a couple files. :)
>
> I'm sure many drivers, mention the word driver at one point.  However,
> this is specifically regarding the file title/header.
>

Got it, just felt like hairsplitting :)

>>>> I'm not sure how
>>>> else to describe this file without using the word driver.
>>>
>>> Perhaps I'm getting picky in my old age, but I think I'd prefer to see:
>>>
>>>    TI TPS65086x PMIC support
>>>
>>
>> Still not sure this describes the file, maybe we could just drop that
>> line completely from files with an obvious purpose? Doesn't really bother
>> me ether way.
>
> Sure.
>

ACK

-- 
Andrew F. Davis

Re: [PATCH 2/4] mfd: tps65086: Add driver for the TPS65086 PMIC

[Lee Jones]

On Wed, 21 Oct 2015, Andrew F. Davis wrote:
> On 10/21/2015 03:43 AM, Lee Jones wrote:
> >On Tue, 20 Oct 2015, Andrew F. Davis wrote:
> >
> >>On 10/20/2015 05:02 AM, Lee Jones wrote:
> >>>On Mon, 19 Oct 2015, Andrew F. Davis wrote:
> >>>>On 10/19/2015 04:23 AM, Lee Jones wrote:
> >>>>>On Fri, 16 Oct 2015, Andrew F. Davis wrote:
> >>>>>
> >>>>>>Add support for the TPS65912 device. It provides communication
> >>>>>>through I2C and contains the following components:
> >>>>>>
> >>>>>>  - Regulators
> >>>>>>  - Load switches
> >>>>>>  - GPO controller
> >>>>>
> >>>>>What's a GPO controller?
> >>>>
> >>>>General Purpose Output, this controller only has output control, it's
> >>>>listed in the data-sheet this way, and other drivers have used this term.
> >>>
> >>>Okay.
> >>>
> >>>>>>Signed-off-by: Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org>
> >>>>>>---
> >>>>>>  drivers/mfd/Kconfig          |  13 ++++
> >>>>>>  drivers/mfd/Makefile         |   1 +
> >>>>>>  drivers/mfd/tps65086.c       | 145 +++++++++++++++++++++++++++++++++++++++++++
> >>>>>>  include/linux/mfd/tps65086.h | 120 +++++++++++++++++++++++++++++++++++
> >>>>>>  4 files changed, 279 insertions(+)
> >>>>>>  create mode 100644 drivers/mfd/tps65086.c
> >>>>>>  create mode 100644 include/linux/mfd/tps65086.h
> >>>>>>
> >>>>>>diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> >>>>>>index 99d6367..ab3b392 100644
> >>>>>>--- a/drivers/mfd/Kconfig
> >>>>>>+++ b/drivers/mfd/Kconfig
> >>>>>>@@ -1093,6 +1093,19 @@ config TPS6507X
> >>>>>>  	  This driver can also be built as a module.  If so, the module
> >>>>>>  	  will be called tps6507x.
> >>>>>>
> >>>>>>+config MFD_TPS65086
> >>>>>>+	tristate "TI TPS65086x Power Management chips"
> >>>>>
> >>>>>Use the full name:
> >>>>>
> >>>>>   "TI TPS65086x Power Management Integrated Chips (PMICs)"
> >>>>>
> >>>>>>+	select REGMAP
> >>>>>>+	select REGMAP_IRQ
> >>>>>>+	select REGMAP_I2C
> >>>>>>+	depends on I2C
> >>>>>
> >>>>>REGMAP_I2C already depends on I2C.
> >>>>>
> >>>>
> >>>>So if I force REGMAP_I2C to =y without depending on I2C then couldn't
> >>>>REGMAP_I2C get selected without I2C enabled? Selecting doesn't force
> >>>>the selected item's dependencies still right?
> >>>
> >>>Try it out. :)
> >>>
> >>
> >>OK, I just tried a little test and it did as I expected, I was able to
> >>select a symbol and it did not enable its dependency, so this would
> >>break build. So, I'm still confused, why does REGMAP_I2C depending
> >>on I2C matter? What are you suggesting my Kconfig should look like?
> >
> >That's fine, if you've tested it and it's a problem the above will do.
> >
> >I'm still little curious as to why it's necessary to depend on the
> >same symbols your dependencies depend on.
> >
> 
> Because I don't 'depend' on REGMAP_I2C, I 'select' it. So none of its
> dependencies need be met, so I have to depend on them for REGMAP_I2C.
> 
> 'Depending' on REGMAP_I2C would also be undesirable as my symbol would
> only be visible when REGMAP_I2C is =y, this would obscure my symbol as
> no one would know to turn on REGMAP_I2C to make my driver appear. I
> only need I2C and then I can go manually enable all my random helper
> symbols myself if I'm selected.

Right.  I understand the fundamentals of how Kconfig works, I guess I
just hoped it would be better in cases like this.

> >How far up the chain is it possible to go?
> >
> >config DEPA
> >
> >config DEPB
> >        select DEPA
> >
> >config DEPC
> >        select DEPB
> >        depends on DEPA
> >
> >config DEPD
> >        select DEPC
> >        depends on DEPA
> >        depends on DEPB
> >
> >config YOU
> >        select DEPD
> >        depends on DEPA
> >        depends on DEPB
> >        depends on DEPC
> >
> >Etc.
> >
> 
> To me it seems like a fundamental issue with how the Kconfig system
> was originally designed (a quick check shows it existed in some form
> 20 years ago...) that leads to this kind of thing, if they knew how
> it would be used today they probably would have made it more
> directed-tree like, without the reverse dependencies and all.

Yes, perhaps.

> >>>>>>+	depends on OF || COMPILE_TEST
> >>>>>>+	help
> >>>>>>+	  If you say yes here you get support for the TPS65086 series of
> >>>>>>+	  Power Management chips.
> >>>>>
> >>>>>I'd remove the line break.
> >>>>>
> >>>>>>+	  These include voltage regulators, GPO and other features
> >>>>>
> >>>>>GPIO?
> >>>>>
> >>>>>>+	  that are often used in portable devices.
> >>>>>
> >>>>>This is meant to be a 'help', so instead of staying "and some other
> >>>>>stuff", please list them.
> >>>>>
> >>>>
> >>>>This seems to be what other devices do, I'll change it to something
> >>>>else then.
> >>>
> >>>If you know what the "other features" are, it's best to name them.
> >>
> >>I was giving a generic description of a PMIC, I didn't really want to
> >>list all the features and start marketing the part in the Kconfig, unless
> >>you want, I'm sure TI won't mind the advertisement. :)
> >>
> >>I'm not sure why we write so much about these driver parts at all in the
> >>help, people who need them know what they are, it's not like some core
> >>kernel config option that people might be undecided on and need explained
> >>its function.
> >
> >I'm sure when you've been trying to find answers to questions you've
> >found the help helpful (or maybe it was poorly underwritten and you
> >didn't find it useful at all).  Without the help, all these drivers
> >are just a selection of random letters, numbers and code/project
> >names.  It's the help which brings them to life, so to speak.
> >
> 
> Well sure, they have been a lot of help for core config where I might
> not know what CONFIG_PERF_EVENTS means and a long explanation is nice,
> but I know what a TPS65086 is, if I didn't I wouldn't be trying to
> enable the driver for it.

That's a very single minded view.  The help isn't just for people
looking to enable drivers.  Think about the maintenance aspect and
people who are either just curious or wish to learn more.  The help
should be useful (helpful), rather than each of them saying "enable
this to add support for <enter crappy non-descriptive model number
here> and all of it's other features".

> >Please describe the device and what it provides.  No sales pitch
> >required.
> >
> 
> I'll see what I can think up. "These include voltage regulators, GPIO
> and other features that are often used in portable devices." Really
> does give the gist of it for uninformed users who might want to know
> why this is enabled for their computer/board.

If there really are "other features" and you know what they are, then
list them.  If there aren't or *might be*, but you don't know what
they are just omit the phrase.  Saying "and some other stuff" is a
cop-out.

> Honestly I don't think the part actually does anything else, but saying
> that when other similar parts add the "other features" line is kinda
> odd and might single out this part as looking less useful.

Ah ha!  Exactly my point.  So if it's fluff just remove it.

> >>>>>>  config TPS65911_COMPARATOR
> >>>>>>  	tristate
> >>>>>>
> >>>>>>diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> >>>>>>index a59e3fc..7adb902 100644
> >>>>>>--- a/drivers/mfd/Makefile
> >>>>>>+++ b/drivers/mfd/Makefile
> >>>>>>@@ -66,6 +66,7 @@ obj-$(CONFIG_MFD_WM8994)	+= wm8994-core.o wm8994-irq.o wm8994-regmap.o
> >>>>>>  obj-$(CONFIG_TPS6105X)		+= tps6105x.o
> >>>>>>  obj-$(CONFIG_TPS65010)		+= tps65010.o
> >>>>>>  obj-$(CONFIG_TPS6507X)		+= tps6507x.o
> >>>>>>+obj-$(CONFIG_MFD_TPS65086)	+= tps65086.o
> >>>>>>  obj-$(CONFIG_MFD_TPS65217)	+= tps65217.o
> >>>>>>  obj-$(CONFIG_MFD_TPS65218)	+= tps65218.o
> >>>>>>  obj-$(CONFIG_MFD_TPS65910)	+= tps65910.o
> >>>>>>diff --git a/drivers/mfd/tps65086.c b/drivers/mfd/tps65086.c
> >>>>>>new file mode 100644
> >>>>>>index 0000000..be6d678
> >>>>>>--- /dev/null
> >>>>>>+++ b/drivers/mfd/tps65086.c
> >>>>>>@@ -0,0 +1,145 @@
> >>>>>>+/*
> >>>>>>+ * Driver for TI TPS65086x PMICs
> >>>>>
> >>>>>I'd suggest removing the driver part.  We know it's a driver.
> >>>>
> >>>>Again this is what almost every other driver does,
> >>>
> >>>Almost every other driver.  That's some statement.
> >>>
> >>>I count 42%. ;)
> >>>
> >>
> >>I'm finding in drivers/mfd ~89% of drivers mention at some point that
> >>they are a driver. But I do admit my original statement was based on a
> >>quick monte carlo selection of a couple files. :)
> >
> >I'm sure many drivers, mention the word driver at one point.  However,
> >this is specifically regarding the file title/header.
> >
> 
> Got it, just felt like hairsplitting :)
> 
> >>>>I'm not sure how
> >>>>else to describe this file without using the word driver.
> >>>
> >>>Perhaps I'm getting picky in my old age, but I think I'd prefer to see:
> >>>
> >>>   TI TPS65086x PMIC support
> >>>
> >>
> >>Still not sure this describes the file, maybe we could just drop that
> >>line completely from files with an obvious purpose? Doesn't really bother
> >>me ether way.
> >
> >Sure.
> >
> 
> ACK
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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