[PATCH 0/5] mfd: tp6586x: enhancements in the driver

[PATCH 0/5] mfd: tp6586x: enhancements in the driver

[Laxman Dewangan]

Doing some enhancements in the tps6586x core driver which is
based on some recent driver framework enhancements.
Following are highlights of changes:
- Use devm for allocation to remove the code for freeing it.
- Use regmap i2c for register access in place of direct i2c apis.
  This will give the debug fs and cache functionality through regmap
  framework.
- Use the regmap caching for some fo register in place of local
  implementation.
- Move the gpio support driver to the drivers/gpio and implement it as
  platform driver. The registration will be done as mfd sub devices.

Laxman Dewangan (5):
  mfd: tps6586x:use devm managed resources
  mfd: Use regmap for tps6586x register access.
  mfd: tps6586x: cache register through regmap
  gpio: tps6586x: add gpio support through platform driver
  mfd: tps6586x: remove gpio support from core driver

 drivers/gpio/Kconfig         |    7 +
 drivers/gpio/Makefile        |    1 +
 drivers/gpio/gpio-tps6586x.c |  158 ++++++++++++++++++++++
 drivers/mfd/Kconfig          |    3 +-
 drivers/mfd/tps6586x.c       |  301 ++++++++++++------------------------------
 5 files changed, 255 insertions(+), 215 deletions(-)
 create mode 100644 drivers/gpio/gpio-tps6586x.c

[PATCH 1/5] mfd: tps6586x:use devm managed resources

[Laxman Dewangan]

Allocate memory for device state using devm_kzalloc(), get the
IRQ using devm_request_irq().
All to simplify accounting and letting the kernel do the
garbage-collection.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/mfd/tps6586x.c |   23 ++++++++---------------
 1 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index c84b550..1256b7f 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -432,8 +432,8 @@ static int __devinit tps6586x_irq_init(struct tps6586x *tps6586x, int irq,
 #endif
 	}
 
-	ret = request_threaded_irq(irq, NULL, tps6586x_irq, IRQF_ONESHOT,
-				   "tps6586x", tps6586x);
+	ret = devm_request_threaded_irq(tps6586x->dev, irq, NULL, tps6586x_irq,
+			IRQF_ONESHOT, "tps6586x", tps6586x);
 
 	if (!ret) {
 		device_init_wakeup(tps6586x->dev, 1);
@@ -579,9 +579,11 @@ static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 
 	dev_info(&client->dev, "VERSIONCRC is %02x\n", ret);
 
-	tps6586x = kzalloc(sizeof(struct tps6586x), GFP_KERNEL);
-	if (tps6586x == NULL)
+	tps6586x = devm_kzalloc(&client->dev, sizeof(*tps6586x), GFP_KERNEL);
+	if (tps6586x == NULL) {
+		dev_err(&client->dev, "memory for tps6586x alloc failed\n");
 		return -ENOMEM;
+	}
 
 	tps6586x->client = client;
 	tps6586x->dev = &client->dev;
@@ -594,14 +596,14 @@ static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 					pdata->irq_base);
 		if (ret) {
 			dev_err(&client->dev, "IRQ init failed: %d\n", ret);
-			goto err_irq_init;
+			return ret;
 		}
 	}
 
 	ret = tps6586x_gpio_init(tps6586x, pdata->gpio_base);
 	if (ret) {
 		dev_err(&client->dev, "GPIO registration failed: %d\n", ret);
-		goto err_gpio_init;
+		return ret;
 	}
 
 	ret = tps6586x_add_subdevs(tps6586x, pdata);
@@ -619,11 +621,6 @@ err_add_devs:
 			dev_err(&client->dev, "Can't remove gpio chip: %d\n",
 				ret);
 	}
-err_gpio_init:
-	if (client->irq)
-		free_irq(client->irq, tps6586x);
-err_irq_init:
-	kfree(tps6586x);
 	return ret;
 }
 
@@ -633,9 +630,6 @@ static int __devexit tps6586x_i2c_remove(struct i2c_client *client)
 	struct tps6586x_platform_data *pdata = client->dev.platform_data;
 	int ret;
 
-	if (client->irq)
-		free_irq(client->irq, tps6586x);
-
 	if (pdata->gpio_base) {
 		ret = gpiochip_remove(&tps6586x->gpio);
 		if (ret)
@@ -644,7 +638,6 @@ static int __devexit tps6586x_i2c_remove(struct i2c_client *client)
 	}
 
 	tps6586x_remove_subdevs(tps6586x);
-	kfree(tps6586x);
 	return 0;
 }
 
-- 
1.7.1.1

[PATCH 2/5] mfd: Use regmap for tps6586x register access.

[Laxman Dewangan]

Using regmap apis for accessing the device registers.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/mfd/Kconfig    |    1 +
 drivers/mfd/tps6586x.c |  157 ++++++++++++++----------------------------------
 2 files changed, 47 insertions(+), 111 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index fa267d4..ec885a9 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -200,6 +200,7 @@ config MFD_TPS6586X
 	bool "TPS6586x Power Management chips"
 	depends on I2C=y && GPIOLIB && GENERIC_HARDIRQS
 	select MFD_CORE
+	select REGMAP_I2C
 	depends on REGULATOR
 	help
 	  If you say yes here you get support for the TPS6586X series of
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index 1256b7f..fb0c109 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -21,8 +21,10 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/i2c.h>
+#include <linux/regmap.h>
 #include <linux/regulator/of_regulator.h>
 
 #include <linux/mfd/core.h>
@@ -48,6 +50,9 @@
 /* device id */
 #define TPS6586X_VERSIONCRC	0xcd
 
+/* Maximum register */
+#define TPS6586X_MAX_REGISTER	(TPS6586X_VERSIONCRC + 1)
+
 struct tps6586x_irq_data {
 	u8	mask_reg;
 	u8	mask_mask;
@@ -90,9 +95,9 @@ static const struct tps6586x_irq_data tps6586x_irqs[] = {
 };
 
 struct tps6586x {
-	struct mutex		lock;
 	struct device		*dev;
 	struct i2c_client	*client;
+	struct regmap		*regmap;
 
 	struct gpio_chip	gpio;
 	struct irq_chip		irq_chip;
@@ -103,152 +108,69 @@ struct tps6586x {
 	u8			mask_reg[5];
 };
 
-static inline int __tps6586x_read(struct i2c_client *client,
-				  int reg, uint8_t *val)
+static inline struct tps6586x *dev_to_tps6586x(struct device *dev)
 {
-	int ret;
-
-	ret = i2c_smbus_read_byte_data(client, reg);
-	if (ret < 0) {
-		dev_err(&client->dev, "failed reading at 0x%02x\n", reg);
-		return ret;
-	}
-
-	*val = (uint8_t)ret;
-
-	return 0;
-}
-
-static inline int __tps6586x_reads(struct i2c_client *client, int reg,
-				   int len, uint8_t *val)
-{
-	int ret;
-
-	ret = i2c_smbus_read_i2c_block_data(client, reg, len, val);
-	if (ret < 0) {
-		dev_err(&client->dev, "failed reading from 0x%02x\n", reg);
-		return ret;
-	}
-
-	return 0;
-}
-
-static inline int __tps6586x_write(struct i2c_client *client,
-				 int reg, uint8_t val)
-{
-	int ret;
-
-	ret = i2c_smbus_write_byte_data(client, reg, val);
-	if (ret < 0) {
-		dev_err(&client->dev, "failed writing 0x%02x to 0x%02x\n",
-				val, reg);
-		return ret;
-	}
-
-	return 0;
-}
-
-static inline int __tps6586x_writes(struct i2c_client *client, int reg,
-				  int len, uint8_t *val)
-{
-	int ret, i;
-
-	for (i = 0; i < len; i++) {
-		ret = __tps6586x_write(client, reg + i, *(val + i));
-		if (ret < 0)
-			return ret;
-	}
-
-	return 0;
+	return i2c_get_clientdata(to_i2c_client(dev));
 }
 
 int tps6586x_write(struct device *dev, int reg, uint8_t val)
 {
-	return __tps6586x_write(to_i2c_client(dev), reg, val);
+	struct tps6586x *tps6586x = dev_to_tps6586x(dev);
+
+	return regmap_write(tps6586x->regmap, reg, val);
 }
 EXPORT_SYMBOL_GPL(tps6586x_write);
 
 int tps6586x_writes(struct device *dev, int reg, int len, uint8_t *val)
 {
-	return __tps6586x_writes(to_i2c_client(dev), reg, len, val);
+	struct tps6586x *tps6586x = dev_to_tps6586x(dev);
+
+	return regmap_bulk_write(tps6586x->regmap, reg, val, len);
 }
 EXPORT_SYMBOL_GPL(tps6586x_writes);
 
 int tps6586x_read(struct device *dev, int reg, uint8_t *val)
 {
-	return __tps6586x_read(to_i2c_client(dev), reg, val);
+	struct tps6586x *tps6586x = dev_to_tps6586x(dev);
+	unsigned int rval;
+	int ret;
+
+	ret = regmap_read(tps6586x->regmap, reg, &rval);
+	if (!ret)
+		*val = rval;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(tps6586x_read);
 
 int tps6586x_reads(struct device *dev, int reg, int len, uint8_t *val)
 {
-	return __tps6586x_reads(to_i2c_client(dev), reg, len, val);
+	struct tps6586x *tps6586x = dev_to_tps6586x(dev);
+
+	return regmap_bulk_read(tps6586x->regmap, reg, val, len);
 }
 EXPORT_SYMBOL_GPL(tps6586x_reads);
 
 int tps6586x_set_bits(struct device *dev, int reg, uint8_t bit_mask)
 {
-	struct tps6586x *tps6586x = dev_get_drvdata(dev);
-	uint8_t reg_val;
-	int ret = 0;
-
-	mutex_lock(&tps6586x->lock);
-
-	ret = __tps6586x_read(to_i2c_client(dev), reg, &reg_val);
-	if (ret)
-		goto out;
+	struct tps6586x *tps6586x = dev_to_tps6586x(dev);
 
-	if ((reg_val & bit_mask) != bit_mask) {
-		reg_val |= bit_mask;
-		ret = __tps6586x_write(to_i2c_client(dev), reg, reg_val);
-	}
-out:
-	mutex_unlock(&tps6586x->lock);
-	return ret;
+	return regmap_update_bits(tps6586x->regmap, reg, bit_mask, bit_mask);
 }
 EXPORT_SYMBOL_GPL(tps6586x_set_bits);
 
 int tps6586x_clr_bits(struct device *dev, int reg, uint8_t bit_mask)
 {
-	struct tps6586x *tps6586x = dev_get_drvdata(dev);
-	uint8_t reg_val;
-	int ret = 0;
+	struct tps6586x *tps6586x = dev_to_tps6586x(dev);
 
-	mutex_lock(&tps6586x->lock);
-
-	ret = __tps6586x_read(to_i2c_client(dev), reg, &reg_val);
-	if (ret)
-		goto out;
-
-	if (reg_val & bit_mask) {
-		reg_val &= ~bit_mask;
-		ret = __tps6586x_write(to_i2c_client(dev), reg, reg_val);
-	}
-out:
-	mutex_unlock(&tps6586x->lock);
-	return ret;
+	return regmap_update_bits(tps6586x->regmap, reg, bit_mask, 0);
 }
 EXPORT_SYMBOL_GPL(tps6586x_clr_bits);
 
 int tps6586x_update(struct device *dev, int reg, uint8_t val, uint8_t mask)
 {
-	struct tps6586x *tps6586x = dev_get_drvdata(dev);
-	uint8_t reg_val;
-	int ret = 0;
-
-	mutex_lock(&tps6586x->lock);
-
-	ret = __tps6586x_read(tps6586x->client, reg, &reg_val);
-	if (ret)
-		goto out;
+	struct tps6586x *tps6586x = dev_to_tps6586x(dev);
 
-	if ((reg_val & mask) != val) {
-		reg_val = (reg_val & ~mask) | val;
-		ret = __tps6586x_write(tps6586x->client, reg, reg_val);
-	}
-out:
-	mutex_unlock(&tps6586x->lock);
-	return ret;
+	return regmap_update_bits(tps6586x->regmap, reg, mask, val);
 }
 EXPORT_SYMBOL_GPL(tps6586x_update);
 
@@ -258,7 +180,7 @@ static int tps6586x_gpio_get(struct gpio_chip *gc, unsigned offset)
 	uint8_t val;
 	int ret;
 
-	ret = __tps6586x_read(tps6586x->client, TPS6586X_GPIOSET2, &val);
+	ret = tps6586x_read(tps6586x->dev, TPS6586X_GPIOSET2, &val);
 	if (ret)
 		return ret;
 
@@ -556,6 +478,12 @@ static struct tps6586x_platform_data *tps6586x_parse_dt(struct i2c_client *clien
 }
 #endif
 
+static const struct regmap_config tps6586x_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = TPS6586X_MAX_REGISTER - 1,
+};
+
 static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 					const struct i2c_device_id *id)
 {
@@ -589,7 +517,14 @@ static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 	tps6586x->dev = &client->dev;
 	i2c_set_clientdata(client, tps6586x);
 
-	mutex_init(&tps6586x->lock);
+	tps6586x->regmap = devm_regmap_init_i2c(client,
+					&tps6586x_regmap_config);
+	if (IS_ERR(tps6586x->regmap)) {
+		ret = PTR_ERR(tps6586x->regmap);
+		dev_err(&client->dev, "regmap init failed: %d\n", ret);
+		return ret;
+	}
+
 
 	if (client->irq) {
 		ret = tps6586x_irq_init(tps6586x, client->irq,
-- 
1.7.1.1

[PATCH 3/5] mfd: tps6586x: cache register through regmap

[Laxman Dewangan]

To cache the interrupt mask register, use the regmap RB_TREE
cache-ing mechanism in place of implementing it locally.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/mfd/tps6586x.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index fb0c109..7210fa7 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -104,7 +104,6 @@ struct tps6586x {
 	struct mutex		irq_lock;
 	int			irq_base;
 	u32			irq_en;
-	u8			mask_cache[5];
 	u8			mask_reg[5];
 };
 
@@ -276,12 +275,11 @@ static void tps6586x_irq_sync_unlock(struct irq_data *data)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(tps6586x->mask_reg); i++) {
-		if (tps6586x->mask_reg[i] != tps6586x->mask_cache[i]) {
-			if (!WARN_ON(tps6586x_write(tps6586x->dev,
-						    TPS6586X_INT_MASK1 + i,
-						    tps6586x->mask_reg[i])))
-				tps6586x->mask_cache[i] = tps6586x->mask_reg[i];
-		}
+		int ret;
+		ret = tps6586x_write(tps6586x->dev,
+					    TPS6586X_INT_MASK1 + i,
+					    tps6586x->mask_reg[i]);
+		WARN_ON(ret);
 	}
 
 	mutex_unlock(&tps6586x->irq_lock);
@@ -328,7 +326,6 @@ static int __devinit tps6586x_irq_init(struct tps6586x *tps6586x, int irq,
 
 	mutex_init(&tps6586x->irq_lock);
 	for (i = 0; i < 5; i++) {
-		tps6586x->mask_cache[i] = 0xff;
 		tps6586x->mask_reg[i] = 0xff;
 		tps6586x_write(tps6586x->dev, TPS6586X_INT_MASK1 + i, 0xff);
 	}
@@ -478,10 +475,21 @@ static struct tps6586x_platform_data *tps6586x_parse_dt(struct i2c_client *clien
 }
 #endif
 
+static bool is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	/* Cache all interrupt mask register */
+	if ((reg >= TPS6586X_INT_MASK1) && (reg <= TPS6586X_INT_MASK5))
+		return false;
+
+	return true;
+}
+
 static const struct regmap_config tps6586x_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 	.max_register = TPS6586X_MAX_REGISTER - 1,
+	.volatile_reg = is_volatile_reg,
+	.cache_type = REGCACHE_RBTREE,
 };
 
 static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
-- 
1.7.1.1

[PATCH 4/5] gpio: tps6586x: add gpio support through platform driver

[Laxman Dewangan]

Converting the gpio driver of tps6586x to a platform
driver in place of registering the gpio through core
driver.
The motivation of the change is:
- This is inline with the mfd drivers implementation.
- This will move the related gpio support to gpio driver
  folder where all gpio related drivers are available.
  This will be easy the maintenance and enhancement is
  anything done for gpio.
- The gpio functionality can be selected through config
  variable.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/gpio/Kconfig         |    7 ++
 drivers/gpio/Makefile        |    1 +
 drivers/gpio/gpio-tps6586x.c |  159 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+), 0 deletions(-)
 create mode 100644 drivers/gpio/gpio-tps6586x.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 502b5ea..b16c8a7 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -597,6 +597,13 @@ config GPIO_AB8500
 	help
 	  Select this to enable the AB8500 IC GPIO driver
 
+config GPIO_TPS6586X
+	bool "TPS6586X GPIO"
+	depends on MFD_TPS6586X
+	help
+	  Select this option to enable GPIO driver for the TPS6586X
+	  chip family.
+
 config GPIO_TPS65910
 	bool "TPS65910 GPIO"
 	depends on MFD_TPS65910
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d370481..153cace 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -63,6 +63,7 @@ obj-$(CONFIG_GPIO_TC3589X)	+= gpio-tc3589x.o
 obj-$(CONFIG_ARCH_TEGRA)	+= gpio-tegra.o
 obj-$(CONFIG_GPIO_TIMBERDALE)	+= gpio-timberdale.o
 obj-$(CONFIG_ARCH_DAVINCI_TNETV107X) += gpio-tnetv107x.o
+obj-$(CONFIG_GPIO_TPS6586X)	+= gpio-tps6586x.o
 obj-$(CONFIG_GPIO_TPS65910)	+= gpio-tps65910.o
 obj-$(CONFIG_GPIO_TPS65912)	+= gpio-tps65912.o
 obj-$(CONFIG_GPIO_TWL4030)	+= gpio-twl4030.o
diff --git a/drivers/gpio/gpio-tps6586x.c b/drivers/gpio/gpio-tps6586x.c
new file mode 100644
index 0000000..aa2ca99
--- /dev/null
+++ b/drivers/gpio/gpio-tps6586x.c
@@ -0,0 +1,159 @@
+/*
+ * TI TPS6586x GPIO driver
+ *
+ * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
+ * Laxman Dewangan <ldewangan@nvidia.com>
+ *
+ * Copyright (c) 2010 CompuLab Ltd.
+ * Mike Rapoport <mike@compulab.co.il>
+ *
+ * Based on da903x.c.
+ * Copyright (C) 2008 Compulab, Ltd.
+ * Mike Rapoport <mike@compulab.co.il>
+ * Copyright (C) 2006-2008 Marvell International Ltd.
+ * Eric Miao <eric.miao@marvell.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify it
+ *  under  the terms of the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the License, or (at your
+ *  option) any later version.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/tps6586x.h>
+#include <linux/of_device.h>
+
+/* GPIO control registers */
+#define TPS6586X_GPIOSET1	0x5d
+#define TPS6586X_GPIOSET2	0x5e
+
+struct tps6586x_gpio {
+	struct gpio_chip gpio_chip;
+	struct device *parent;
+};
+
+static inline struct tps6586x_gpio *to_tps6586x_gpio(struct gpio_chip *chip)
+{
+	return container_of(chip, struct tps6586x_gpio, gpio_chip);
+}
+
+static int tps6586x_gpio_get(struct gpio_chip *gc, unsigned offset)
+{
+	struct tps6586x_gpio *tps6586x_gpio = to_tps6586x_gpio(gc);
+	uint8_t val;
+	int ret;
+
+	ret = tps6586x_read(tps6586x_gpio->parent, TPS6586X_GPIOSET2, &val);
+	if (ret)
+		return ret;
+
+	return !!(val & (1 << offset));
+}
+
+static void tps6586x_gpio_set(struct gpio_chip *gc, unsigned offset,
+			      int value)
+{
+	struct tps6586x_gpio *tps6586x_gpio = to_tps6586x_gpio(gc);
+
+	tps6586x_update(tps6586x_gpio->parent, TPS6586X_GPIOSET2,
+			value << offset, 1 << offset);
+}
+
+static int tps6586x_gpio_output(struct gpio_chip *gc, unsigned offset,
+				int value)
+{
+	struct tps6586x_gpio *tps6586x_gpio = to_tps6586x_gpio(gc);
+	uint8_t val, mask;
+
+	tps6586x_gpio_set(gc, offset, value);
+
+	val = 0x1 << (offset * 2);
+	mask = 0x3 << (offset * 2);
+
+	return tps6586x_update(tps6586x_gpio->parent, TPS6586X_GPIOSET1,
+				val, mask);
+}
+
+static int __devinit tps6586x_gpio_probe(struct platform_device *pdev)
+{
+	struct tps6586x_platform_data *pdata;
+	struct tps6586x_gpio *tps6586x_gpio;
+	int ret;
+
+	pdata = dev_get_platdata(pdev->dev.parent);
+	tps6586x_gpio = devm_kzalloc(&pdev->dev,
+				sizeof(*tps6586x_gpio), GFP_KERNEL);
+	if (!tps6586x_gpio) {
+		dev_err(&pdev->dev, "Could not allocate tps6586x_gpio\n");
+		return -ENOMEM;
+	}
+
+	tps6586x_gpio->parent = pdev->dev.parent;
+
+	tps6586x_gpio->gpio_chip.owner = THIS_MODULE;
+	tps6586x_gpio->gpio_chip.label = pdev->name;
+	tps6586x_gpio->gpio_chip.dev = &pdev->dev;
+	tps6586x_gpio->gpio_chip.ngpio = 4;
+	tps6586x_gpio->gpio_chip.can_sleep = 1;
+
+	/* FIXME: add handling of GPIOs as dedicated inputs */
+	tps6586x_gpio->gpio_chip.direction_output = tps6586x_gpio_output;
+	tps6586x_gpio->gpio_chip.set	= tps6586x_gpio_set;
+	tps6586x_gpio->gpio_chip.get	= tps6586x_gpio_get;
+
+#ifdef CONFIG_OF_GPIO
+	tps6586x_gpio->gpio_chip.of_node = pdev->dev.parent->of_node;
+#endif
+	if (pdata && pdata->gpio_base)
+		tps6586x_gpio->gpio_chip.base = pdata->gpio_base;
+	else
+		tps6586x_gpio->gpio_chip.base = -1;
+
+	ret = gpiochip_add(&tps6586x_gpio->gpio_chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, tps6586x_gpio);
+
+	return ret;
+}
+
+static int __devexit tps6586x_gpio_remove(struct platform_device *pdev)
+{
+	struct tps6586x_gpio *tps6586x_gpio = platform_get_drvdata(pdev);
+
+	return gpiochip_remove(&tps6586x_gpio->gpio_chip);
+}
+
+static struct platform_driver tps6586x_gpio_driver = {
+	.driver.name	= "tps6586x-gpio",
+	.driver.owner	= THIS_MODULE,
+	.probe		= tps6586x_gpio_probe,
+	.remove		= __devexit_p(tps6586x_gpio_remove),
+};
+
+static int __init tps6586x_gpio_init(void)
+{
+	return platform_driver_register(&tps6586x_gpio_driver);
+}
+subsys_initcall(tps6586x_gpio_init);
+
+static void __exit tps6586x_gpio_exit(void)
+{
+	platform_driver_unregister(&tps6586x_gpio_driver);
+}
+module_exit(tps6586x_gpio_exit);
+
+MODULE_ALIAS("platform:tps6586x-gpio");
+MODULE_DESCRIPTION("GPIO interface for TPS6586X PMIC");
+MODULE_AUTHOR("Mike Rapoport <mike@compulab.co.il>");
+MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
+MODULE_LICENSE("GPL");
-- 
1.7.1.1

Re: [PATCH 4/5] gpio: tps6586x: add gpio support through platform driver

[Linus Walleij]

On Fri, Jul 13, 2012 at 12:39 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:

> Converting the gpio driver of tps6586x to a platform
> driver in place of registering the gpio through core
> driver.
> The motivation of the change is:
> - This is inline with the mfd drivers implementation.
> - This will move the related gpio support to gpio driver
>   folder where all gpio related drivers are available.
>   This will be easy the maintenance and enhancement is
>   anything done for gpio.
> - The gpio functionality can be selected through config
>   variable.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

Overall this is very, very good and you're doing the right thing.

> --- /dev/null
> +++ b/drivers/gpio/gpio-tps6586x.c

> +#include <linux/i2c.h>

Why?

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 4/5] gpio: tps6586x: add gpio support through platform driver

[Laxman Dewangan]

On Sunday 15 July 2012 03:48 AM, Linus Walleij wrote:
> On Fri, Jul 13, 2012 at 12:39 PM, Laxman Dewangan<ldewangan@nvidia.com>  wrote:
>
>> Converting the gpio driver of tps6586x to a platform
>> driver in place of registering the gpio through core
>> driver.
>> The motivation of the change is:
>> - This is inline with the mfd drivers implementation.
>> - This will move the related gpio support to gpio driver
>>    folder where all gpio related drivers are available.
>>    This will be easy the maintenance and enhancement is
>>    anything done for gpio.
>> - The gpio functionality can be selected through config
>>    variable.
>>
>> Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
> Overall this is very, very good and you're doing the right thing.
>
>> --- /dev/null
>> +++ b/drivers/gpio/gpio-tps6586x.c
>> +#include<linux/i2c.h>
> Why?
>

Removed this inclusion as it is not require.

> Reviewed-by: Linus Walleij<linus.walleij@linaro.org>

I will add your reviewed-by in my next patch for easyness.

[PATCH 5/5] mfd: tps6586x: remove gpio support from core driver

[Laxman Dewangan]

The GPIO functionality of device tps6586x is added through
platform gpio driver and it can be register as the mfd sub
device and hence removing the duplicates code which register
the gpio functionality from core driver.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/mfd/Kconfig    |    2 +-
 drivers/mfd/tps6586x.c |   99 +++++++++---------------------------------------
 2 files changed, 19 insertions(+), 82 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ec885a9..872dfe4 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -198,7 +198,7 @@ config MFD_TPS65217
 
 config MFD_TPS6586X
 	bool "TPS6586x Power Management chips"
-	depends on I2C=y && GPIOLIB && GENERIC_HARDIRQS
+	depends on I2C=y && GENERIC_HARDIRQS
 	select MFD_CORE
 	select REGMAP_I2C
 	depends on REGULATOR
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index 7210fa7..0b76fd5 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -22,7 +22,6 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/err.h>
-#include <linux/gpio.h>
 #include <linux/i2c.h>
 #include <linux/regmap.h>
 #include <linux/regulator/of_regulator.h>
@@ -30,10 +29,6 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/tps6586x.h>
 
-/* GPIO control registers */
-#define TPS6586X_GPIOSET1	0x5d
-#define TPS6586X_GPIOSET2	0x5e
-
 /* interrupt control registers */
 #define TPS6586X_INT_ACK1	0xb5
 #define TPS6586X_INT_ACK2	0xb6
@@ -94,12 +89,23 @@ static const struct tps6586x_irq_data tps6586x_irqs[] = {
 	[TPS6586X_INT_RTC_ALM2] = TPS6586X_IRQ(TPS6586X_INT_MASK4, 1 << 1),
 };
 
+static struct mfd_cell tps6586x_cell[] = {
+	{
+		.name = "tps6586x-gpio",
+	},
+	{
+		.name = "tps6586x-rtc",
+	},
+	{
+		.name = "tps6586x-onkey",
+	},
+};
+
 struct tps6586x {
 	struct device		*dev;
 	struct i2c_client	*client;
 	struct regmap		*regmap;
 
-	struct gpio_chip	gpio;
 	struct irq_chip		irq_chip;
 	struct mutex		irq_lock;
 	int			irq_base;
@@ -173,63 +179,6 @@ int tps6586x_update(struct device *dev, int reg, uint8_t val, uint8_t mask)
 }
 EXPORT_SYMBOL_GPL(tps6586x_update);
 
-static int tps6586x_gpio_get(struct gpio_chip *gc, unsigned offset)
-{
-	struct tps6586x *tps6586x = container_of(gc, struct tps6586x, gpio);
-	uint8_t val;
-	int ret;
-
-	ret = tps6586x_read(tps6586x->dev, TPS6586X_GPIOSET2, &val);
-	if (ret)
-		return ret;
-
-	return !!(val & (1 << offset));
-}
-
-
-static void tps6586x_gpio_set(struct gpio_chip *chip, unsigned offset,
-			      int value)
-{
-	struct tps6586x *tps6586x = container_of(chip, struct tps6586x, gpio);
-
-	tps6586x_update(tps6586x->dev, TPS6586X_GPIOSET2,
-			value << offset, 1 << offset);
-}
-
-static int tps6586x_gpio_output(struct gpio_chip *gc, unsigned offset,
-				int value)
-{
-	struct tps6586x *tps6586x = container_of(gc, struct tps6586x, gpio);
-	uint8_t val, mask;
-
-	tps6586x_gpio_set(gc, offset, value);
-
-	val = 0x1 << (offset * 2);
-	mask = 0x3 << (offset * 2);
-
-	return tps6586x_update(tps6586x->dev, TPS6586X_GPIOSET1, val, mask);
-}
-
-static int tps6586x_gpio_init(struct tps6586x *tps6586x, int gpio_base)
-{
-	if (!gpio_base)
-		return 0;
-
-	tps6586x->gpio.owner		= THIS_MODULE;
-	tps6586x->gpio.label		= tps6586x->client->name;
-	tps6586x->gpio.dev		= tps6586x->dev;
-	tps6586x->gpio.base		= gpio_base;
-	tps6586x->gpio.ngpio		= 4;
-	tps6586x->gpio.can_sleep	= 1;
-
-	/* FIXME: add handling of GPIOs as dedicated inputs */
-	tps6586x->gpio.direction_output	= tps6586x_gpio_output;
-	tps6586x->gpio.set		= tps6586x_gpio_set;
-	tps6586x->gpio.get		= tps6586x_gpio_get;
-
-	return gpiochip_add(&tps6586x->gpio);
-}
-
 static int __remove_subdev(struct device *dev, void *unused)
 {
 	platform_device_unregister(to_platform_device(dev));
@@ -543,9 +492,10 @@ static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 		}
 	}
 
-	ret = tps6586x_gpio_init(tps6586x, pdata->gpio_base);
-	if (ret) {
-		dev_err(&client->dev, "GPIO registration failed: %d\n", ret);
+	ret = mfd_add_devices(tps6586x->dev, -1,
+			tps6586x_cell, ARRAY_SIZE(tps6586x_cell), NULL, 0);
+	if (ret < 0) {
+		dev_err(&client->dev, "mfd_add_devices failed: %d\n", ret);
 		return ret;
 	}
 
@@ -558,29 +508,16 @@ static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
 	return 0;
 
 err_add_devs:
-	if (pdata->gpio_base) {
-		ret = gpiochip_remove(&tps6586x->gpio);
-		if (ret)
-			dev_err(&client->dev, "Can't remove gpio chip: %d\n",
-				ret);
-	}
+	mfd_remove_devices(tps6586x->dev);
 	return ret;
 }
 
 static int __devexit tps6586x_i2c_remove(struct i2c_client *client)
 {
 	struct tps6586x *tps6586x = i2c_get_clientdata(client);
-	struct tps6586x_platform_data *pdata = client->dev.platform_data;
-	int ret;
-
-	if (pdata->gpio_base) {
-		ret = gpiochip_remove(&tps6586x->gpio);
-		if (ret)
-			dev_err(&client->dev, "Can't remove gpio chip: %d\n",
-				ret);
-	}
 
 	tps6586x_remove_subdevs(tps6586x);
+	mfd_remove_devices(tps6586x->dev);
 	return 0;
 }
 
-- 
1.7.1.1

Re: [PATCH 5/5] mfd: tps6586x: remove gpio support from core driver

[Linus Walleij]

On Fri, Jul 13, 2012 at 12:39 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:

> The GPIO functionality of device tps6586x is added through
> platform gpio driver and it can be register as the mfd sub
> device and hence removing the duplicates code which register
> the gpio functionality from core driver.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

The only thing I worry about is if old defconfig files are now
missing this config option and then this patch becomes an
unbisectable showstopper for them because they don't get
this driver by default anymore.

So please make sure you've grepped around and checked
that of some platforms defconfig was previously selecting
the core driver, make sure it selects this driver too for
the moment, it's better if they later deactivate it
if they don't need it.

Yours,
Linus Walleij

Re: [PATCH 5/5] mfd: tps6586x: remove gpio support from core driver

[Laxman Dewangan]

On Sunday 15 July 2012 03:42 AM, Linus Walleij wrote:
> On Fri, Jul 13, 2012 at 12:39 PM, Laxman Dewangan<ldewangan@nvidia.com>  wrote:
>
>> The GPIO functionality of device tps6586x is added through
>> platform gpio driver and it can be register as the mfd sub
>> device and hence removing the duplicates code which register
>> the gpio functionality from core driver.
>>
>> Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
> The only thing I worry about is if old defconfig files are now
> missing this config option and then this patch becomes an
> unbisectable showstopper for them because they don't get
> this driver by default anymore.
>
> So please make sure you've grepped around and checked
> that of some platforms defconfig was previously selecting
> the core driver, make sure it selects this driver too for
> the moment, it's better if they later deactivate it
> if they don't need it.

Grep shows that only arm based platform tegra_defconfig is using this 
device.
I have added the change in tegra_defconfig and will send as part of this 
series.