[PATCH 0/4] mfd:tps65910: use devm_* and register gpio as platform driver

[PATCH 0/4] mfd:tps65910: use devm_* and register gpio as platform driver

[Laxman Dewangan]

This series does cleanup in the mfd/tps65910 as follows:
- Do not cache the register when initailizing regmap. Cache when
  actually when we need it.
- Convert the allocation to use devm_* apis.
- Move the gpio as platform driver and register it as mfd sub devices.

Laxman Dewangan (4):
  mfd: tps65910: cache register when we need it
	Remove the chaching of register in regmap initialization.

  mfd: tps65910: convert all allocation to devm_*
	Convert the allocation to use devm_* apis.

  mfd: tps65910: register gpio as mfd device
  gpio: tps65910: move this as platform driver
	Above two patch to make gpio driver as platform driver and register
	as mfd sub device from core driver.
	

 drivers/gpio/gpio-tps65910.c |  140 ++++++++++++++++++++++++++++++-----------
 drivers/mfd/Kconfig          |    1 -
 drivers/mfd/tps65910.c       |   37 ++++-------
 include/linux/mfd/tps65910.h |    6 --
 4 files changed, 115 insertions(+), 69 deletions(-)

[PATCH 1/4] mfd: tps65910: cache register when we need it

[Laxman Dewangan]

During regmap initialization, we do not provide the default value and
hence in place of caching register during regmap_init(), cache it
when actually we need it i.e. after reading of that register.

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

diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index 7a55af9..0f95ddf 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -85,8 +85,7 @@ static const struct regmap_config tps65910_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 	.volatile_reg = is_volatile_reg,
-	.max_register = TPS65910_MAX_REGISTER,
-	.num_reg_defaults_raw = TPS65910_MAX_REGISTER,
+	.max_register = TPS65910_MAX_REGISTER - 1,
 	.cache_type = REGCACHE_RBTREE,
 };
 
-- 
1.7.1.1

Re: [PATCH 1/4] mfd: tps65910: cache register when we need it

[Mark Brown]

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

On Wed, May 09, 2012 at 06:40:54PM +0530, Laxman Dewangan wrote:
> During regmap initialization, we do not provide the default value and
> hence in place of caching register during regmap_init(), cache it
> when actually we need it i.e. after reading of that register.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

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

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

[PATCH 2/4] mfd: tps65910: convert all allocation to devm_*

[Laxman Dewangan]

Convert memory allocation and regmap initialization to
use devm_* functions.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/mfd/tps65910.c |   28 +++++++++-------------------
 1 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index 0f95ddf..d0d8ae9 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -161,15 +161,13 @@ static __devinit int tps65910_i2c_probe(struct i2c_client *i2c,
 	if (!pmic_plat_data)
 		return -EINVAL;
 
-	init_data = kzalloc(sizeof(struct tps65910_platform_data), GFP_KERNEL);
+	init_data = devm_kzalloc(&i2c->dev, sizeof(*init_data), GFP_KERNEL);
 	if (init_data == NULL)
 		return -ENOMEM;
 
-	tps65910 = kzalloc(sizeof(struct tps65910), GFP_KERNEL);
-	if (tps65910 == NULL) {
-		kfree(init_data);
+	tps65910 = devm_kzalloc(&i2c->dev, sizeof(*tps65910), GFP_KERNEL);
+	if (tps65910 == NULL)
 		return -ENOMEM;
-	}
 
 	i2c_set_clientdata(i2c, tps65910);
 	tps65910->dev = &i2c->dev;
@@ -179,18 +177,20 @@ static __devinit int tps65910_i2c_probe(struct i2c_client *i2c,
 	tps65910->write = tps65910_i2c_write;
 	mutex_init(&tps65910->io_mutex);
 
-	tps65910->regmap = regmap_init_i2c(i2c, &tps65910_regmap_config);
+	tps65910->regmap = devm_regmap_init_i2c(i2c, &tps65910_regmap_config);
 	if (IS_ERR(tps65910->regmap)) {
 		ret = PTR_ERR(tps65910->regmap);
 		dev_err(&i2c->dev, "regmap initialization failed: %d\n", ret);
-		goto regmap_err;
+		return ret;
 	}
 
 	ret = mfd_add_devices(tps65910->dev, -1,
 			      tps65910s, ARRAY_SIZE(tps65910s),
 			      NULL, 0);
-	if (ret < 0)
-		goto err;
+	if (ret < 0) {
+		dev_err(&i2c->dev, "mfd_add_devices failed: %d\n", ret);
+		return ret;
+	}
 
 	init_data->irq = pmic_plat_data->irq;
 	init_data->irq_base = pmic_plat_data->irq_base;
@@ -201,14 +201,6 @@ static __devinit int tps65910_i2c_probe(struct i2c_client *i2c,
 
 	tps65910_sleepinit(tps65910, pmic_plat_data);
 
-	kfree(init_data);
-	return ret;
-
-err:
-	regmap_exit(tps65910->regmap);
-regmap_err:
-	kfree(tps65910);
-	kfree(init_data);
 	return ret;
 }
 
@@ -218,8 +210,6 @@ static __devexit int tps65910_i2c_remove(struct i2c_client *i2c)
 
 	tps65910_irq_exit(tps65910);
 	mfd_remove_devices(tps65910->dev);
-	regmap_exit(tps65910->regmap);
-	kfree(tps65910);
 
 	return 0;
 }
-- 
1.7.1.1

Re: [PATCH 2/4] mfd: tps65910: convert all allocation to devm_*

[Mark Brown]

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

On Wed, May 09, 2012 at 06:40:55PM +0530, Laxman Dewangan wrote:
> Convert memory allocation and regmap initialization to
> use devm_* functions.

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

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

[PATCH 3/4] mfd: tps65910: register gpio as mfd device

[Laxman Dewangan]

As gpio support for tps65910 is on gpio driver, registering
gpio support as the mfd sub devices instead of calling gpio_init()
from the core probe.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/mfd/Kconfig          |    1 -
 drivers/mfd/tps65910.c       |    6 +++---
 include/linux/mfd/tps65910.h |    6 ------
 3 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 77873d3..5e8481c 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -176,7 +176,6 @@ config MFD_TPS65910
 	bool "TPS65910 Power Management chip"
 	depends on I2C=y && GPIOLIB
 	select MFD_CORE
-	select GPIO_TPS65910
 	select REGMAP_I2C
 	help
 	  if you say yes here you get support for the TPS65910 series of
diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index d0d8ae9..f51ab30 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -19,13 +19,15 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
-#include <linux/gpio.h>
 #include <linux/mfd/core.h>
 #include <linux/regmap.h>
 #include <linux/mfd/tps65910.h>
 
 static struct mfd_cell tps65910s[] = {
 	{
+		.name = "tps65910-gpio",
+	},
+	{
 		.name = "tps65910-pmic",
 	},
 	{
@@ -195,8 +197,6 @@ static __devinit int tps65910_i2c_probe(struct i2c_client *i2c,
 	init_data->irq = pmic_plat_data->irq;
 	init_data->irq_base = pmic_plat_data->irq_base;
 
-	tps65910_gpio_init(tps65910, pmic_plat_data->gpio_base);
-
 	tps65910_irq_init(tps65910, init_data->irq, init_data);
 
 	tps65910_sleepinit(tps65910, pmic_plat_data);
diff --git a/include/linux/mfd/tps65910.h b/include/linux/mfd/tps65910.h
index 56903ad..7884f0b 100644
--- a/include/linux/mfd/tps65910.h
+++ b/include/linux/mfd/tps65910.h
@@ -17,8 +17,6 @@
 #ifndef __LINUX_MFD_TPS65910_H
 #define __LINUX_MFD_TPS65910_H
 
-#include <linux/gpio.h>
-
 /* TPS chip id list */
 #define TPS65910			0
 #define TPS65911			1
@@ -831,9 +829,6 @@ struct tps65910 {
 	struct tps65910_rtc *rtc;
 	struct tps65910_power *power;
 
-	/* GPIO Handling */
-	struct gpio_chip gpio;
-
 	/* IRQ Handling */
 	struct mutex irq_lock;
 	int chip_irq;
@@ -849,7 +844,6 @@ struct tps65910_platform_data {
 
 int tps65910_set_bits(struct tps65910 *tps65910, u8 reg, u8 mask);
 int tps65910_clear_bits(struct tps65910 *tps65910, u8 reg, u8 mask);
-void tps65910_gpio_init(struct tps65910 *tps65910, int gpio_base);
 int tps65910_irq_init(struct tps65910 *tps65910, int irq,
 		struct tps65910_platform_data *pdata);
 int tps65910_irq_exit(struct tps65910 *tps65910);
-- 
1.7.1.1

[PATCH 4/4] gpio: tps65910: move this as platform driver

[Laxman Dewangan]

Make the gpio-tps65910 as platform driver and register
this from tps65910 core driver as mfd sub device.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/gpio/gpio-tps65910.c |  140 ++++++++++++++++++++++++++++++-----------
 1 files changed, 102 insertions(+), 38 deletions(-)

diff --git a/drivers/gpio/gpio-tps65910.c b/drivers/gpio/gpio-tps65910.c
index 7eef648..3db5357 100644
--- a/drivers/gpio/gpio-tps65910.c
+++ b/drivers/gpio/gpio-tps65910.c
@@ -18,11 +18,23 @@
 #include <linux/errno.h>
 #include <linux/gpio.h>
 #include <linux/i2c.h>
+#include <linux/platform_device.h>
 #include <linux/mfd/tps65910.h>
 
+struct tps65910_gpio {
+	struct gpio_chip gpio_chip;
+	struct tps65910 *tps65910;
+};
+
+static inline struct tps65910_gpio *to_tps65910_gpio(struct gpio_chip *chip)
+{
+	return container_of(chip, struct tps65910_gpio, gpio_chip);
+}
+
 static int tps65910_gpio_get(struct gpio_chip *gc, unsigned offset)
 {
-	struct tps65910 *tps65910 = container_of(gc, struct tps65910, gpio);
+	struct tps65910_gpio *tps65910_gpio = to_tps65910_gpio(gc);
+	struct tps65910 *tps65910 = tps65910_gpio->tps65910;
 	uint8_t val;
 
 	tps65910->read(tps65910, TPS65910_GPIO0 + offset, 1, &val);
@@ -36,7 +48,8 @@ static int tps65910_gpio_get(struct gpio_chip *gc, unsigned offset)
 static void tps65910_gpio_set(struct gpio_chip *gc, unsigned offset,
 			      int value)
 {
-	struct tps65910 *tps65910 = container_of(gc, struct tps65910, gpio);
+	struct tps65910_gpio *tps65910_gpio = to_tps65910_gpio(gc);
+	struct tps65910 *tps65910 = tps65910_gpio->tps65910;
 
 	if (value)
 		tps65910_set_bits(tps65910, TPS65910_GPIO0 + offset,
@@ -49,7 +62,8 @@ static void tps65910_gpio_set(struct gpio_chip *gc, unsigned offset,
 static int tps65910_gpio_output(struct gpio_chip *gc, unsigned offset,
 				int value)
 {
-	struct tps65910 *tps65910 = container_of(gc, struct tps65910, gpio);
+	struct tps65910_gpio *tps65910_gpio = to_tps65910_gpio(gc);
+	struct tps65910 *tps65910 = tps65910_gpio->tps65910;
 
 	/* Set the initial value */
 	tps65910_gpio_set(gc, offset, value);
@@ -60,59 +74,109 @@ static int tps65910_gpio_output(struct gpio_chip *gc, unsigned offset,
 
 static int tps65910_gpio_input(struct gpio_chip *gc, unsigned offset)
 {
-	struct tps65910 *tps65910 = container_of(gc, struct tps65910, gpio);
+	struct tps65910_gpio *tps65910_gpio = to_tps65910_gpio(gc);
+	struct tps65910 *tps65910 = tps65910_gpio->tps65910;
 
 	return tps65910_clear_bits(tps65910, TPS65910_GPIO0 + offset,
 						GPIO_CFG_MASK);
 }
 
-void tps65910_gpio_init(struct tps65910 *tps65910, int gpio_base)
+static int __devinit tps65910_gpio_probe(struct platform_device *pdev)
 {
+	struct tps65910 *tps65910 = dev_get_drvdata(pdev->dev.parent);
+	struct tps65910_board *pdata = dev_get_platdata(tps65910->dev);
+	struct tps65910_gpio *tps65910_gpio;
 	int ret;
-	struct tps65910_board *board_data;
+	int i;
+
+	tps65910_gpio = devm_kzalloc(&pdev->dev,
+				sizeof(*tps65910_gpio), GFP_KERNEL);
+	if (!tps65910_gpio) {
+		dev_err(&pdev->dev, "Could not allocate tps65910_gpio\n");
+		return -ENOMEM;
+	}
 
-	if (!gpio_base)
-		return;
+	tps65910_gpio->tps65910 = tps65910;
 
-	tps65910->gpio.owner		= THIS_MODULE;
-	tps65910->gpio.label		= tps65910->i2c_client->name;
-	tps65910->gpio.dev		= tps65910->dev;
-	tps65910->gpio.base		= gpio_base;
+	tps65910_gpio->gpio_chip.owner = THIS_MODULE;
+	tps65910_gpio->gpio_chip.label = tps65910->i2c_client->name;
 
 	switch(tps65910_chip_id(tps65910)) {
 	case TPS65910:
-		tps65910->gpio.ngpio	= TPS65910_NUM_GPIO;
+		tps65910_gpio->gpio_chip.ngpio = TPS65910_NUM_GPIO;
 		break;
 	case TPS65911:
-		tps65910->gpio.ngpio	= TPS65911_NUM_GPIO;
+		tps65910_gpio->gpio_chip.ngpio = TPS65911_NUM_GPIO;
 		break;
 	default:
-		return;
+		return -EINVAL;
 	}
-	tps65910->gpio.can_sleep	= 1;
-
-	tps65910->gpio.direction_input	= tps65910_gpio_input;
-	tps65910->gpio.direction_output	= tps65910_gpio_output;
-	tps65910->gpio.set		= tps65910_gpio_set;
-	tps65910->gpio.get		= tps65910_gpio_get;
-
-	/* Configure sleep control for gpios */
-	board_data = dev_get_platdata(tps65910->dev);
-	if (board_data) {
-		int i;
-		for (i = 0; i < tps65910->gpio.ngpio; ++i) {
-			if (board_data->en_gpio_sleep[i]) {
-				ret = tps65910_set_bits(tps65910,
-					TPS65910_GPIO0 + i, GPIO_SLEEP_MASK);
-				if (ret < 0)
-					dev_warn(tps65910->dev,
-						"GPIO Sleep setting failed\n");
-			}
-		}
+	tps65910_gpio->gpio_chip.can_sleep = 1;
+	tps65910_gpio->gpio_chip.direction_input = tps65910_gpio_input;
+	tps65910_gpio->gpio_chip.direction_output = tps65910_gpio_output;
+	tps65910_gpio->gpio_chip.set	= tps65910_gpio_set;
+	tps65910_gpio->gpio_chip.get	= tps65910_gpio_get;
+	tps65910_gpio->gpio_chip.dev = &pdev->dev;
+	if (pdata && pdata->gpio_base)
+		tps65910_gpio->gpio_chip.base = pdata->gpio_base;
+	else
+		tps65910_gpio->gpio_chip.base = -1;
+
+	if (!pdata)
+		goto skip_init;
+
+	/* Configure sleep control for gpios if provided */
+	for (i = 0; i < tps65910_gpio->gpio_chip.ngpio; ++i) {
+		if (!pdata->en_gpio_sleep[i])
+			continue;
+
+		ret = tps65910_set_bits(tps65910,
+			TPS65910_GPIO0 + i, GPIO_SLEEP_MASK);
+		if (ret < 0)
+			dev_warn(tps65910->dev,
+				"GPIO Sleep setting failed with err %d\n", ret);
+	}
+
+skip_init:
+	ret = gpiochip_add(&tps65910_gpio->gpio_chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
+		return ret;
 	}
 
-	ret = gpiochip_add(&tps65910->gpio);
+	platform_set_drvdata(pdev, tps65910_gpio);
 
-	if (ret)
-		dev_warn(tps65910->dev, "GPIO registration failed: %d\n", ret);
+	return ret;
 }
+
+static int __devexit tps65910_gpio_remove(struct platform_device *pdev)
+{
+	struct tps65910_gpio *tps65910_gpio = platform_get_drvdata(pdev);
+
+	return gpiochip_remove(&tps65910_gpio->gpio_chip);
+}
+
+static struct platform_driver tps65910_gpio_driver = {
+	.driver.name    = "tps65910-gpio",
+	.driver.owner   = THIS_MODULE,
+	.probe		= tps65910_gpio_probe,
+	.remove		= __devexit_p(tps65910_gpio_remove),
+};
+
+static int __init tps65910_gpio_init(void)
+{
+	return platform_driver_register(&tps65910_gpio_driver);
+}
+subsys_initcall(tps65910_gpio_init);
+
+static void __exit tps65910_gpio_exit(void)
+{
+	platform_driver_unregister(&tps65910_gpio_driver);
+}
+module_exit(tps65910_gpio_exit);
+
+MODULE_AUTHOR("Graeme Gregory <gg@slimlogic.co.uk>");
+MODULE_AUTHOR("Jorge Eduardo Candelaria jedu@slimlogic.co.uk>");
+MODULE_DESCRIPTION("GPIO interface for TPS65910/TPS6511 PMICs");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:tps65910-gpio");
-- 
1.7.1.1

Re: [PATCH 4/4] gpio: tps65910: move this as platform driver

[Grant Likely]

On Wed,  9 May 2012 18:40:57 +0530, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> Make the gpio-tps65910 as platform driver and register
> this from tps65910 core driver as mfd sub device.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

Minor comment below, but otherwise:

Acked-by: Grant Likely <grant.likely@secretlab.ca>
> +static struct platform_driver tps65910_gpio_driver = {
> +	.driver.name    = "tps65910-gpio",
> +	.driver.owner   = THIS_MODULE,
> +	.probe		= tps65910_gpio_probe,
> +	.remove		= __devexit_p(tps65910_gpio_remove),
> +};
> +
> +static int __init tps65910_gpio_init(void)
> +{
> +	return platform_driver_register(&tps65910_gpio_driver);
> +}
> +subsys_initcall(tps65910_gpio_init);
> +
> +static void __exit tps65910_gpio_exit(void)
> +{
> +	platform_driver_unregister(&tps65910_gpio_driver);
> +}
> +module_exit(tps65910_gpio_exit);

module_platform_driver()

Re: [PATCH 4/4] gpio: tps65910: move this as platform driver

[Mark Brown]

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

On Fri, May 11, 2012 at 11:40:11AM -0600, Grant Likely wrote:
> > +static int __init tps65910_gpio_init(void)
> > +{
> > +	return platform_driver_register(&tps65910_gpio_driver);
> > +}
> > +subsys_initcall(tps65910_gpio_init);

> module_platform_driver()

This seems a little premature when we've not got many (any?) drivers
implementing -EPROBE_DEFER.  If we got the change I posted to make
gpio_request() return -EPROBE_DEFER if there was no GPIO registered
that'd get most cases covered though, the drivers would hopefully pass
the error through.

I've not done this for regulators since the cpufreq drivers are still
deviceless so couldn't cope.

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

Re: [PATCH 0/4] mfd:tps65910: use devm_* and register gpio as platform driver

[Samuel Ortiz]

Hi Laxman,

On Wed, May 09, 2012 at 06:40:53PM +0530, Laxman Dewangan wrote:
> This series does cleanup in the mfd/tps65910 as follows:
> - Do not cache the register when initailizing regmap. Cache when
>   actually when we need it.
> - Convert the allocation to use devm_* apis.
> - Move the gpio as platform driver and register it as mfd sub devices.
> 
> Laxman Dewangan (4):
>   mfd: tps65910: cache register when we need it
> 	Remove the chaching of register in regmap initialization.
> 
>   mfd: tps65910: convert all allocation to devm_*
> 	Convert the allocation to use devm_* apis.
> 
>   mfd: tps65910: register gpio as mfd device
>   gpio: tps65910: move this as platform driver
> 	Above two patch to make gpio driver as platform driver and register
> 	as mfd sub device from core driver.
> 	
I applied patches 1, 2 and 3. Patch #4 does not apply cleanly to my for-next
branch, could you please rebase it ?
Also, having Grant's ACK for it would be nice.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 0/4] mfd:tps65910: use devm_* and register gpio as platform driver

[Laxman Dewangan]

On Friday 11 May 2012 06:44 PM, Samuel Ortiz wrote:
> Hi Laxman,
>
> On Wed, May 09, 2012 at 06:40:53PM +0530, Laxman Dewangan wrote:
>> This series does cleanup in the mfd/tps65910 as follows:
>> - Do not cache the register when initailizing regmap. Cache when
>>    actually when we need it.
>> - Convert the allocation to use devm_* apis.
>> - Move the gpio as platform driver and register it as mfd sub devices.
>>
>> Laxman Dewangan (4):
>>    mfd: tps65910: cache register when we need it
>> 	Remove the chaching of register in regmap initialization.
>>
>>    mfd: tps65910: convert all allocation to devm_*
>> 	Convert the allocation to use devm_* apis.
>>
>>    mfd: tps65910: register gpio as mfd device
>>    gpio: tps65910: move this as platform driver
>> 	Above two patch to make gpio driver as platform driver and register
>> 	as mfd sub device from core driver.
>> 	
> I applied patches 1, 2 and 3. Patch #4 does not apply cleanly to my for-next
> branch, could you please rebase it ?
> Also, having Grant's ACK for it would be nice.
Thanks for taking care.
I will create a new patch based on your tree if Grant is OK with this.