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

[PATCH V2 0/6] 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.


Changes V1 -> V2:
- Remove the unnecessariy header inclusion in the gpio-tps6586x.
- To avoid bisect functionality break, grep the user of tps6586x and
  found that tegra_defconfig is only using this. Added the
  GPIO_TPS6586x in config variable on this series.
  This results 6 patch on this series.

Laxman Dewangan (6):
  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
  ARM: tegra: defconfig: enable tps6586x gpio
  mfd: tps6586x: remove gpio support from core driver

 arch/arm/configs/tegra_defconfig |    1 +
 drivers/gpio/Kconfig             |    7 +
 drivers/gpio/Makefile            |    1 +
 drivers/gpio/gpio-tps6586x.c     |  158 ++++++++++++++++++++
 drivers/mfd/Kconfig              |    3 +-
 drivers/mfd/tps6586x.c           |  301 +++++++++++---------------------------
 6 files changed, 256 insertions(+), 215 deletions(-)
 create mode 100644 drivers/gpio/gpio-tps6586x.c

[PATCH V2 1/6] 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>
---
Changes V1 -> V2:
No change, generated new patch for V2 series.

 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

Re: [PATCH V2 1/6] mfd: tps6586x:use devm managed resources

[Mark Brown]

On Mon, Jul 16, 2012 at 12:21:45PM +0530, Laxman Dewangan wrote:

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

Are you sure this is safe - what guarantees that we can't get an
interrupt while tearing the device down?

Re: [PATCH V2 1/6] mfd: tps6586x:use devm managed resources

[Laxman Dewangan]

On Tuesday 17 July 2012 01:31 AM, Mark Brown wrote:
> On Mon, Jul 16, 2012 at 12:21:45PM +0530, Laxman Dewangan wrote:
>
>> -	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);
> Are you sure this is safe - what guarantees that we can't get an
> interrupt while tearing the device down?

I think device_remove will get called before the managed resource get 
freed.
So do we need to call disable_irq() in remove() to avoid any interrupts?

Re: [PATCH V2 1/6] mfd: tps6586x:use devm managed resources

[Mark Brown]

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

On Tue, Jul 17, 2012 at 10:12:39AM +0530, Laxman Dewangan wrote:
> On Tuesday 17 July 2012 01:31 AM, Mark Brown wrote:
> >On Mon, Jul 16, 2012 at 12:21:45PM +0530, Laxman Dewangan wrote:

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

> >Are you sure this is safe - what guarantees that we can't get an
> >interrupt while tearing the device down?

> I think device_remove will get called before the managed resource
> get freed.
> So do we need to call disable_irq() in remove() to avoid any interrupts?

That would be very rude if the IRQ happened to get shared, though that's
not possible at the moment.  In general it's pretty hard to use devm_
IRQs safely, it's normally safest to avoid them due to the issues with
ensuring that the interrupt handler is safe to be called even while the
device is being torn down.

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

Re: [PATCH V2 1/6] mfd: tps6586x:use devm managed resources

[Laxman Dewangan]

On Tuesday 17 July 2012 10:23 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Jul 17, 2012 at 10:12:39AM +0530, Laxman Dewangan wrote:
>> On Tuesday 17 July 2012 01:31 AM, Mark Brown wrote:
>>> On Mon, Jul 16, 2012 at 12:21:45PM +0530, Laxman Dewangan wrote:
>
>> I think device_remove will get called before the managed resource
>> get freed.
>> So do we need to call disable_irq() in remove() to avoid any interrupts?
> That would be very rude if the IRQ happened to get shared, though that's
> not possible at the moment.  In general it's pretty hard to use devm_
> IRQs safely, it's normally safest to avoid them due to the issues with
> ensuring that the interrupt handler is safe to be called even while the
> device is being torn down.

OK, then I will revert this specific change if it is not safe. I will 
send the another patch (part of series v3 1/6) for this change only.

[PATCH V2 2/6] 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>
---
Changes V1 -> V2:
No change, generated new patch for V2 series.

 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

Re: [PATCH V2 2/6] mfd: Use regmap for tps6586x register access.

[Mark Brown]

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

On Mon, Jul 16, 2012 at 12:21:46PM +0530, Laxman Dewangan wrote:
> Using regmap apis for accessing the device registers.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

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

[PATCH V2 3/6] 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>
---
Changes V1 -> V2:
No change, generated new patch for V2 series.

 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

Re: [PATCH V2 3/6] mfd: tps6586x: cache register through regmap

[Mark Brown]

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

On Mon, Jul 16, 2012 at 12:21:47PM +0530, Laxman Dewangan wrote:
> To cache the interrupt mask register, use the regmap RB_TREE
> cache-ing mechanism in place of implementing it locally.

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Can you use regmap-irq?

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

Re: [PATCH V2 3/6] mfd: tps6586x: cache register through regmap

[Laxman Dewangan]

On Tuesday 17 July 2012 01:33 AM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Mon, Jul 16, 2012 at 12:21:47PM +0530, Laxman Dewangan wrote:
>> To cache the interrupt mask register, use the regmap RB_TREE
>> cache-ing mechanism in place of implementing it locally.
> Reviewed-by: Mark Brown<broonie@opensource.wolfsonmicro.com>
>
> Can you use regmap-irq?
>
Sure, I will convert it to regmap_irq and will send new patch after this 
series in linux-next.
Thanks,
Laxman

[PATCH V2 4/6] 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes V1 -> V2:
Remove non-require header inclusion.

 drivers/gpio/Kconfig         |    7 ++
 drivers/gpio/Makefile        |    1 +
 drivers/gpio/gpio-tps6586x.c |  158 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 166 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..2526b3b
--- /dev/null
+++ b/drivers/gpio/gpio-tps6586x.c
@@ -0,0 +1,158 @@
+/*
+ * TI TPS6586x GPIO driver
+ *
+ * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
+ * Author: Laxman dewangan <ldewangan@nvidia.com>
+ *
+ * Based on tps6586x.c
+ * Copyright (c) 2010 CompuLab Ltd.
+ * Mike Rapoport <mike@compulab.co.il>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/errno.h>
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mfd/tps6586x.h>
+#include <linux/of_device.h>
+#include <linux/platform_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("Laxman Dewangan <ldewangan@nvidia.com>");
+MODULE_LICENSE("GPL");
-- 
1.7.1.1

[PATCH V2 5/6] ARM: tegra: defconfig: enable tps6586x gpio

[Laxman Dewangan]

Enable GPIO_TPS6586X as the gpio functionality of
this device moved as platform driver.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes V1 -> V2:
New change in V2 series. To avoid bisect, added change in defconfig.

 arch/arm/configs/tegra_defconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/configs/tegra_defconfig b/arch/arm/configs/tegra_defconfig
index 4be9c1e..5ae3889 100644
--- a/arch/arm/configs/tegra_defconfig
+++ b/arch/arm/configs/tegra_defconfig
@@ -105,6 +105,7 @@ CONFIG_I2C_MUX_PINCTRL=y
 CONFIG_I2C_TEGRA=y
 CONFIG_SPI=y
 CONFIG_SPI_TEGRA=y
+CONFIG_GPIO_TPS6586X=y
 CONFIG_GPIO_TPS65910=y
 CONFIG_POWER_SUPPLY=y
 CONFIG_BATTERY_SBS=y
-- 
1.7.1.1

Re: [PATCH V2 5/6] ARM: tegra: defconfig: enable tps6586x gpio

[Linus Walleij]

On Mon, Jul 16, 2012 at 8:51 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:

> Enable GPIO_TPS6586X as the gpio functionality of
> this device moved as platform driver.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

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

Yours,
Linus Walleij

[PATCH V2 6/6] 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>
---
Changes from V1->V2
No changes in code, just patch become 6th of series as change in defconfig
got added.

 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 V2 6/6] mfd: tps6586x: remove gpio support from core driver

[Linus Walleij]

On Mon, Jul 16, 2012 at 8:51 AM, 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>
> ---
> Changes from V1->V2
> No changes in code, just patch become 6th of series as change in defconfig
> got added.

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

Yours,
Linus Walleij