[PATCH v2 0/3] gpio: twl4030: Correct status reporting for outputs

[PATCH v2 0/3] gpio: twl4030: Correct status reporting for outputs

[Peter Ujfalusi]

Hi,

As Grant commneted on the first version:
https://lkml.org/lkml/2012/12/5/53

Introduce bitfields to cache the directionand output status of the pins so we
can report them correctly.
To do this I did some cleanup within the driver to get rid of the global
variables and moved them under a private structure, changed the locking as well
to protect the bitfield operation.
As a last patch I added a note that the PWMA/B handling should not be in this
driver at all.

Regards,
Peter
---
Peter Ujfalusi (3):
  gpio: twl4030: Introduce private structure to store variables needed
    runtime
  gpio: twl4030: Cache the direction and output states in private data
  gpio: twl4030: TODO comment to remove the PWMA/B (LEDA/B) handling

 drivers/gpio/gpio-twl4030.c | 177 ++++++++++++++++++++++++++++----------------
 1 file changed, 115 insertions(+), 62 deletions(-)

-- 
1.8.0

[PATCH v2 1/3] gpio: twl4030: Introduce private structure to store variables needed runtime

[Peter Ujfalusi]

Move most of the global variables inside a private structure and allocate
it dynamically.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpio/gpio-twl4030.c | 82 +++++++++++++++++++++++++++------------------
 1 file changed, 50 insertions(+), 32 deletions(-)

diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
index 55b4fe4..c092739 100644
--- a/drivers/gpio/gpio-twl4030.c
+++ b/drivers/gpio/gpio-twl4030.c
@@ -49,11 +49,6 @@
  * There are also two LED pins used sometimes as output-only GPIOs.
  */
 
-
-static struct gpio_chip twl_gpiochip;
-static int twl4030_gpio_base;
-static int twl4030_gpio_irq_base;
-
 /* genirq interfaces are not available to modules */
 #ifdef MODULE
 #define is_module()	true
@@ -72,11 +67,20 @@ static int twl4030_gpio_irq_base;
 /* Data structures */
 static DEFINE_MUTEX(gpio_lock);
 
-/* store usage of each GPIO. - each bit represents one GPIO */
-static unsigned int gpio_usage_count;
+struct gpio_twl4030_priv {
+	struct gpio_chip gpio_chip;
+	int irq_base;
+
+	unsigned int usage_count;
+};
 
 /*----------------------------------------------------------------------*/
 
+static inline struct gpio_twl4030_priv *to_gpio_twl4030(struct gpio_chip *chip)
+{
+	return container_of(chip, struct gpio_twl4030_priv, gpio_chip);
+}
+
 /*
  * To configure TWL4030 GPIO module registers
  */
@@ -193,10 +197,6 @@ static int twl4030_get_gpio_datain(int gpio)
 	u8 base = 0;
 	int ret = 0;
 
-	if (unlikely((gpio >= TWL4030_GPIO_MAX)
-		|| !(gpio_usage_count & BIT(gpio))))
-		return -EPERM;
-
 	base = REG_GPIODATAIN1 + d_bnk;
 	ret = gpio_twl4030_read(base);
 	if (ret > 0)
@@ -209,6 +209,7 @@ static int twl4030_get_gpio_datain(int gpio)
 
 static int twl_request(struct gpio_chip *chip, unsigned offset)
 {
+	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
 	int status = 0;
 
 	mutex_lock(&gpio_lock);
@@ -252,7 +253,7 @@ static int twl_request(struct gpio_chip *chip, unsigned offset)
 	}
 
 	/* on first use, turn GPIO module "on" */
-	if (!gpio_usage_count) {
+	if (!priv->usage_count) {
 		struct twl4030_gpio_platform_data *pdata;
 		u8 value = MASK_GPIO_CTRL_GPIO_ON;
 
@@ -266,16 +267,18 @@ static int twl_request(struct gpio_chip *chip, unsigned offset)
 		status = gpio_twl4030_write(REG_GPIO_CTRL, value);
 	}
 
+done:
 	if (!status)
-		gpio_usage_count |= (0x1 << offset);
+		priv->usage_count |= BIT(offset);
 
-done:
 	mutex_unlock(&gpio_lock);
 	return status;
 }
 
 static void twl_free(struct gpio_chip *chip, unsigned offset)
 {
+	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
+
 	if (offset >= TWL4030_GPIO_MAX) {
 		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, 1);
 		return;
@@ -283,10 +286,10 @@ static void twl_free(struct gpio_chip *chip, unsigned offset)
 
 	mutex_lock(&gpio_lock);
 
-	gpio_usage_count &= ~BIT(offset);
+	priv->usage_count &= ~BIT(offset);
 
 	/* on last use, switch off GPIO module */
-	if (!gpio_usage_count)
+	if (!priv->usage_count)
 		gpio_twl4030_write(REG_GPIO_CTRL, 0x0);
 
 	mutex_unlock(&gpio_lock);
@@ -301,14 +304,19 @@ static int twl_direction_in(struct gpio_chip *chip, unsigned offset)
 
 static int twl_get(struct gpio_chip *chip, unsigned offset)
 {
+	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
 	int status = 0;
 
+	if (!(priv->usage_count & BIT(offset)))
+		return -EPERM;
+
 	if (offset < TWL4030_GPIO_MAX)
 		status = twl4030_get_gpio_datain(offset);
 	else if (offset == TWL4030_GPIO_MAX)
 		status = cached_leden & LEDEN_LEDAON;
 	else
 		status = cached_leden & LEDEN_LEDBON;
+
 	return (status < 0) ? 0 : status;
 }
 
@@ -333,12 +341,14 @@ static void twl_set(struct gpio_chip *chip, unsigned offset, int value)
 
 static int twl_to_irq(struct gpio_chip *chip, unsigned offset)
 {
-	return (twl4030_gpio_irq_base && (offset < TWL4030_GPIO_MAX))
-		? (twl4030_gpio_irq_base + offset)
+	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
+
+	return (priv->irq_base && (offset < TWL4030_GPIO_MAX))
+		? (priv->irq_base + offset)
 		: -EINVAL;
 }
 
-static struct gpio_chip twl_gpiochip = {
+static struct gpio_chip template_chip = {
 	.label			= "twl4030",
 	.owner			= THIS_MODULE,
 	.request		= twl_request,
@@ -424,8 +434,14 @@ static int __devinit gpio_twl4030_probe(struct platform_device *pdev)
 {
 	struct twl4030_gpio_platform_data *pdata = pdev->dev.platform_data;
 	struct device_node *node = pdev->dev.of_node;
+	struct gpio_twl4030_priv *priv;
 	int ret, irq_base;
 
+	priv = devm_kzalloc(&pdev->dev, sizeof(struct gpio_twl4030_priv),
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
 	/* maybe setup IRQs */
 	if (is_module()) {
 		dev_err(&pdev->dev, "can't dispatch IRQs from modules\n");
@@ -445,12 +461,13 @@ static int __devinit gpio_twl4030_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 
-	twl4030_gpio_irq_base = irq_base;
+	priv->irq_base = irq_base;
 
 no_irqs:
-	twl_gpiochip.base = -1;
-	twl_gpiochip.ngpio = TWL4030_GPIO_MAX;
-	twl_gpiochip.dev = &pdev->dev;
+	priv->gpio_chip = template_chip;
+	priv->gpio_chip.base = -1;
+	priv->gpio_chip.ngpio = TWL4030_GPIO_MAX;
+	priv->gpio_chip.dev = &pdev->dev;
 
 	if (node)
 		pdata = of_gpio_twl4030(&pdev->dev);
@@ -481,23 +498,23 @@ no_irqs:
 	 * is (still) clear if use_leds is set.
 	 */
 	if (pdata->use_leds)
-		twl_gpiochip.ngpio += 2;
+		priv->gpio_chip.ngpio += 2;
 
-	ret = gpiochip_add(&twl_gpiochip);
+	ret = gpiochip_add(&priv->gpio_chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "could not register gpiochip, %d\n", ret);
-		twl_gpiochip.ngpio = 0;
+		priv->gpio_chip.ngpio = 0;
 		gpio_twl4030_remove(pdev);
 		goto out;
 	}
 
-	twl4030_gpio_base = twl_gpiochip.base;
+	platform_set_drvdata(pdev, priv);
 
 	if (pdata && pdata->setup) {
 		int status;
 
-		status = pdata->setup(&pdev->dev,
-				twl4030_gpio_base, TWL4030_GPIO_MAX);
+		status = pdata->setup(&pdev->dev, priv->gpio_chip.base,
+				      TWL4030_GPIO_MAX);
 		if (status)
 			dev_dbg(&pdev->dev, "setup --> %d\n", status);
 	}
@@ -510,18 +527,19 @@ out:
 static int gpio_twl4030_remove(struct platform_device *pdev)
 {
 	struct twl4030_gpio_platform_data *pdata = pdev->dev.platform_data;
+	struct gpio_twl4030_priv *priv = platform_get_drvdata(pdev);
 	int status;
 
 	if (pdata && pdata->teardown) {
-		status = pdata->teardown(&pdev->dev,
-				twl4030_gpio_base, TWL4030_GPIO_MAX);
+		status = pdata->teardown(&pdev->dev, priv->gpio_chip.base,
+					 TWL4030_GPIO_MAX);
 		if (status) {
 			dev_dbg(&pdev->dev, "teardown --> %d\n", status);
 			return status;
 		}
 	}
 
-	status = gpiochip_remove(&twl_gpiochip);
+	status = gpiochip_remove(&priv->gpio_chip);
 	if (status < 0)
 		return status;
 
-- 
1.8.0

[PATCH v2 2/3] gpio: twl4030: Cache the direction and output states in private data

[Peter Ujfalusi]

Use more coherent locking in the driver. Use bitfield to store the GPIO
direction and if the pin is configured as output store the status also in a
bitfiled.
In this way we can just look at these bitfields when we need information
about the pin status and only reach out to the chip when it is needed.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpio/gpio-twl4030.c | 99 ++++++++++++++++++++++++++++++---------------
 1 file changed, 66 insertions(+), 33 deletions(-)

diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
index c092739..a38e6e9c 100644
--- a/drivers/gpio/gpio-twl4030.c
+++ b/drivers/gpio/gpio-twl4030.c
@@ -64,14 +64,15 @@
 /* Mask for GPIO registers when aggregated into a 32-bit integer */
 #define GPIO_32_MASK			0x0003ffff
 
-/* Data structures */
-static DEFINE_MUTEX(gpio_lock);
-
 struct gpio_twl4030_priv {
 	struct gpio_chip gpio_chip;
+	struct mutex mutex;
 	int irq_base;
 
+	/* Bitfields for state caching */
 	unsigned int usage_count;
+	unsigned int direction;
+	unsigned int out_state;
 };
 
 /*----------------------------------------------------------------------*/
@@ -130,7 +131,7 @@ static inline int gpio_twl4030_read(u8 address)
 
 /*----------------------------------------------------------------------*/
 
-static u8 cached_leden;		/* protected by gpio_lock */
+static u8 cached_leden;
 
 /* The LED lines are open drain outputs ... a FET pulls to GND, so an
  * external pullup is needed.  We could also expose the integrated PWM
@@ -144,14 +145,12 @@ static void twl4030_led_set_value(int led, int value)
 	if (led)
 		mask <<= 1;
 
-	mutex_lock(&gpio_lock);
 	if (value)
 		cached_leden &= ~mask;
 	else
 		cached_leden |= mask;
 	status = twl_i2c_write_u8(TWL4030_MODULE_LED, cached_leden,
 				  TWL4030_LED_LEDEN_REG);
-	mutex_unlock(&gpio_lock);
 }
 
 static int twl4030_set_gpio_direction(int gpio, int is_input)
@@ -162,7 +161,6 @@ static int twl4030_set_gpio_direction(int gpio, int is_input)
 	u8 base = REG_GPIODATADIR1 + d_bnk;
 	int ret = 0;
 
-	mutex_lock(&gpio_lock);
 	ret = gpio_twl4030_read(base);
 	if (ret >= 0) {
 		if (is_input)
@@ -172,7 +170,6 @@ static int twl4030_set_gpio_direction(int gpio, int is_input)
 
 		ret = gpio_twl4030_write(base, reg);
 	}
-	mutex_unlock(&gpio_lock);
 	return ret;
 }
 
@@ -212,7 +209,7 @@ static int twl_request(struct gpio_chip *chip, unsigned offset)
 	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
 	int status = 0;
 
-	mutex_lock(&gpio_lock);
+	mutex_lock(&priv->mutex);
 
 	/* Support the two LED outputs as output-only GPIOs. */
 	if (offset >= TWL4030_GPIO_MAX) {
@@ -271,7 +268,7 @@ done:
 	if (!status)
 		priv->usage_count |= BIT(offset);
 
-	mutex_unlock(&gpio_lock);
+	mutex_unlock(&priv->mutex);
 	return status;
 }
 
@@ -279,64 +276,98 @@ static void twl_free(struct gpio_chip *chip, unsigned offset)
 {
 	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
 
+	mutex_lock(&priv->mutex);
 	if (offset >= TWL4030_GPIO_MAX) {
 		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, 1);
 		return;
 	}
 
-	mutex_lock(&gpio_lock);
-
 	priv->usage_count &= ~BIT(offset);
 
 	/* on last use, switch off GPIO module */
 	if (!priv->usage_count)
 		gpio_twl4030_write(REG_GPIO_CTRL, 0x0);
 
-	mutex_unlock(&gpio_lock);
+	mutex_unlock(&priv->mutex);
 }
 
 static int twl_direction_in(struct gpio_chip *chip, unsigned offset)
 {
-	return (offset < TWL4030_GPIO_MAX)
-		? twl4030_set_gpio_direction(offset, 1)
-		: -EINVAL;
+	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
+	int ret;
+
+	mutex_lock(&priv->mutex);
+	if (offset < TWL4030_GPIO_MAX)
+		ret = twl4030_set_gpio_direction(offset, 1);
+	else
+		ret = -EINVAL;
+
+	if (!ret)
+		priv->direction &= ~BIT(offset);
+
+	mutex_unlock(&priv->mutex);
+
+	return ret;
 }
 
 static int twl_get(struct gpio_chip *chip, unsigned offset)
 {
 	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
+	int ret;
 	int status = 0;
 
-	if (!(priv->usage_count & BIT(offset)))
-		return -EPERM;
+	mutex_lock(&priv->mutex);
+	if (!(priv->usage_count & BIT(offset))) {
+		ret = -EPERM;
+		goto out;
+	}
 
-	if (offset < TWL4030_GPIO_MAX)
-		status = twl4030_get_gpio_datain(offset);
-	else if (offset == TWL4030_GPIO_MAX)
-		status = cached_leden & LEDEN_LEDAON;
+	if (priv->direction & BIT(offset))
+		status = priv->out_state & BIT(offset);
 	else
-		status = cached_leden & LEDEN_LEDBON;
+		status = twl4030_get_gpio_datain(offset);
 
-	return (status < 0) ? 0 : status;
+	ret = (status <= 0) ? 0 : 1;
+out:
+	mutex_unlock(&priv->mutex);
+	dev_err(chip->dev, "%s: offset %d value %d\n", __func__, offset, ret);
+	return ret;
 }
 
-static int twl_direction_out(struct gpio_chip *chip, unsigned offset, int value)
+static void twl_set(struct gpio_chip *chip, unsigned offset, int value)
 {
-	if (offset < TWL4030_GPIO_MAX) {
+	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
+
+	dev_err(chip->dev, "%s: offset %d value %d\n", __func__, offset, value);
+	mutex_lock(&priv->mutex);
+	if (offset < TWL4030_GPIO_MAX)
 		twl4030_set_gpio_dataout(offset, value);
-		return twl4030_set_gpio_direction(offset, 0);
-	} else {
+	else
 		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, value);
-		return 0;
-	}
+
+	if (value)
+		priv->out_state |= BIT(offset);
+	else
+		priv->out_state &= ~BIT(offset);
+
+	mutex_unlock(&priv->mutex);
 }
 
-static void twl_set(struct gpio_chip *chip, unsigned offset, int value)
+static int twl_direction_out(struct gpio_chip *chip, unsigned offset, int value)
 {
+	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
+
+	dev_err(chip->dev, "%s: offset %d value %d\n", __func__, offset, value);
+	mutex_lock(&priv->mutex);
 	if (offset < TWL4030_GPIO_MAX)
 		twl4030_set_gpio_dataout(offset, value);
-	else
-		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, value);
+
+	priv->direction |= BIT(offset);
+	mutex_unlock(&priv->mutex);
+
+	twl_set(chip, offset, value);
+
+	return 0;
 }
 
 static int twl_to_irq(struct gpio_chip *chip, unsigned offset)
@@ -469,6 +500,8 @@ no_irqs:
 	priv->gpio_chip.ngpio = TWL4030_GPIO_MAX;
 	priv->gpio_chip.dev = &pdev->dev;
 
+	mutex_init(&priv->mutex);
+
 	if (node)
 		pdata = of_gpio_twl4030(&pdev->dev);
 
-- 
1.8.0

[PATCH v2 3/3] gpio: twl4030: TODO comment to remove the PWMA/B (LEDA/B) handling

[Peter Ujfalusi]

This GPIO driver should not configure anything else then GPIOs.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpio/gpio-twl4030.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
index a38e6e9c..1e9f08c4 100644
--- a/drivers/gpio/gpio-twl4030.c
+++ b/drivers/gpio/gpio-twl4030.c
@@ -47,6 +47,7 @@
  * intended to support multiple hosts.
  *
  * There are also two LED pins used sometimes as output-only GPIOs.
+ * TODO: Handling of PWMA/B (LEDA/B) should be removed from this GPIO driver!
  */
 
 /* genirq interfaces are not available to modules */
@@ -131,6 +132,7 @@ static inline int gpio_twl4030_read(u8 address)
 
 /*----------------------------------------------------------------------*/
 
+/* TODO: Handling of PWMA/B (LEDA/B) should be removed from this GPIO driver! */
 static u8 cached_leden;
 
 /* The LED lines are open drain outputs ... a FET pulls to GND, so an
-- 
1.8.0

Re: [PATCH v2 0/3] gpio: twl4030: Correct status reporting for outputs

[Linus Walleij]

On Thu, Dec 6, 2012 at 11:52 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> As Grant commneted on the first version:
> https://lkml.org/lkml/2012/12/5/53
>
> Introduce bitfields to cache the directionand output status of the pins so we
> can report them correctly.
> To do this I did some cleanup within the driver to get rid of the global
> variables and moved them under a private structure, changed the locking as well
> to protect the bitfield operation.
> As a last patch I added a note that the PWMA/B handling should not be in this
> driver at all.

This looks good to me:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Since Grant was requesting the changes I'll let him decide to merge.

Yours,
Linus Walleij

Re: [PATCH v2 0/3] gpio: twl4030: Correct status reporting for outputs

[Peter Ujfalusi]

Hi Grant,

On 12/07/2012 09:09 AM, Linus Walleij wrote:
> On Thu, Dec 6, 2012 at 11:52 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> 
>> As Grant commneted on the first version:
>> https://lkml.org/lkml/2012/12/5/53
>>
>> Introduce bitfields to cache the directionand output status of the pins so we
>> can report them correctly.
>> To do this I did some cleanup within the driver to get rid of the global
>> variables and moved them under a private structure, changed the locking as well
>> to protect the bitfield operation.
>> As a last patch I added a note that the PWMA/B handling should not be in this
>> driver at all.
> 
> This looks good to me:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Since Grant was requesting the changes I'll let him decide to merge.

Would you have time to take a look at this set?

Thanks,
Péter

Re: [PATCH v2 0/3] gpio: twl4030: Correct status reporting for outputs

[Grant Likely]

On Wed, Dec 12, 2012 at 11:12 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> Hi Grant,
>
> On 12/07/2012 09:09 AM, Linus Walleij wrote:
>> On Thu, Dec 6, 2012 at 11:52 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>>
>>> As Grant commneted on the first version:
>>> https://lkml.org/lkml/2012/12/5/53
>>>
>>> Introduce bitfields to cache the directionand output status of the pins so we
>>> can report them correctly.
>>> To do this I did some cleanup within the driver to get rid of the global
>>> variables and moved them under a private structure, changed the locking as well
>>> to protect the bitfield operation.
>>> As a last patch I added a note that the PWMA/B handling should not be in this
>>> driver at all.
>>
>> This looks good to me:
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> Since Grant was requesting the changes I'll let him decide to merge.
>
> Would you have time to take a look at this set?

I will take a look at it this week, but I cannot pick it up for v3.8
unless it is a regression bug fix from v3.6. It will have to wait for
v3.9 and it can be merged into linux-next after the v3.8 merge window
closes.

g.

Re: [PATCH v2 0/3] gpio: twl4030: Correct status reporting for outputs

[Peter Ujfalusi]

On 12/12/2012 12:45 PM, Grant Likely wrote:
> I will take a look at it this week

Thanks

> but I cannot pick it up for v3.8 unless it is a regression bug fix from
> v3.6. It will have to wait for v3.9 and it can be merged into linux-next
> after the v3.8 merge window closes.

3.9 is fine. the gpio-twl4030 never reported the output state correctly, I
just found it out while doing some cleanups around the twl-core and board files.

Thanks,
Péter

Re: [PATCH v2 1/3] gpio: twl4030: Introduce private structure to store variables needed runtime

[Grant Likely]

On Thu, 6 Dec 2012 11:52:05 +0100, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> Move most of the global variables inside a private structure and allocate
> it dynamically.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Applied, thanks

g.

> ---
>  drivers/gpio/gpio-twl4030.c | 82 +++++++++++++++++++++++++++------------------
>  1 file changed, 50 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
> index 55b4fe4..c092739 100644
> --- a/drivers/gpio/gpio-twl4030.c
> +++ b/drivers/gpio/gpio-twl4030.c
> @@ -49,11 +49,6 @@
>   * There are also two LED pins used sometimes as output-only GPIOs.
>   */
>  
> -
> -static struct gpio_chip twl_gpiochip;
> -static int twl4030_gpio_base;
> -static int twl4030_gpio_irq_base;
> -
>  /* genirq interfaces are not available to modules */
>  #ifdef MODULE
>  #define is_module()	true
> @@ -72,11 +67,20 @@ static int twl4030_gpio_irq_base;
>  /* Data structures */
>  static DEFINE_MUTEX(gpio_lock);
>  
> -/* store usage of each GPIO. - each bit represents one GPIO */
> -static unsigned int gpio_usage_count;
> +struct gpio_twl4030_priv {
> +	struct gpio_chip gpio_chip;
> +	int irq_base;
> +
> +	unsigned int usage_count;
> +};
>  
>  /*----------------------------------------------------------------------*/
>  
> +static inline struct gpio_twl4030_priv *to_gpio_twl4030(struct gpio_chip *chip)
> +{
> +	return container_of(chip, struct gpio_twl4030_priv, gpio_chip);
> +}
> +
>  /*
>   * To configure TWL4030 GPIO module registers
>   */
> @@ -193,10 +197,6 @@ static int twl4030_get_gpio_datain(int gpio)
>  	u8 base = 0;
>  	int ret = 0;
>  
> -	if (unlikely((gpio >= TWL4030_GPIO_MAX)
> -		|| !(gpio_usage_count & BIT(gpio))))
> -		return -EPERM;
> -
>  	base = REG_GPIODATAIN1 + d_bnk;
>  	ret = gpio_twl4030_read(base);
>  	if (ret > 0)
> @@ -209,6 +209,7 @@ static int twl4030_get_gpio_datain(int gpio)
>  
>  static int twl_request(struct gpio_chip *chip, unsigned offset)
>  {
> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>  	int status = 0;
>  
>  	mutex_lock(&gpio_lock);
> @@ -252,7 +253,7 @@ static int twl_request(struct gpio_chip *chip, unsigned offset)
>  	}
>  
>  	/* on first use, turn GPIO module "on" */
> -	if (!gpio_usage_count) {
> +	if (!priv->usage_count) {
>  		struct twl4030_gpio_platform_data *pdata;
>  		u8 value = MASK_GPIO_CTRL_GPIO_ON;
>  
> @@ -266,16 +267,18 @@ static int twl_request(struct gpio_chip *chip, unsigned offset)
>  		status = gpio_twl4030_write(REG_GPIO_CTRL, value);
>  	}
>  
> +done:
>  	if (!status)
> -		gpio_usage_count |= (0x1 << offset);
> +		priv->usage_count |= BIT(offset);
>  
> -done:
>  	mutex_unlock(&gpio_lock);
>  	return status;
>  }
>  
>  static void twl_free(struct gpio_chip *chip, unsigned offset)
>  {
> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
> +
>  	if (offset >= TWL4030_GPIO_MAX) {
>  		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, 1);
>  		return;
> @@ -283,10 +286,10 @@ static void twl_free(struct gpio_chip *chip, unsigned offset)
>  
>  	mutex_lock(&gpio_lock);
>  
> -	gpio_usage_count &= ~BIT(offset);
> +	priv->usage_count &= ~BIT(offset);
>  
>  	/* on last use, switch off GPIO module */
> -	if (!gpio_usage_count)
> +	if (!priv->usage_count)
>  		gpio_twl4030_write(REG_GPIO_CTRL, 0x0);
>  
>  	mutex_unlock(&gpio_lock);
> @@ -301,14 +304,19 @@ static int twl_direction_in(struct gpio_chip *chip, unsigned offset)
>  
>  static int twl_get(struct gpio_chip *chip, unsigned offset)
>  {
> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>  	int status = 0;
>  
> +	if (!(priv->usage_count & BIT(offset)))
> +		return -EPERM;
> +
>  	if (offset < TWL4030_GPIO_MAX)
>  		status = twl4030_get_gpio_datain(offset);
>  	else if (offset == TWL4030_GPIO_MAX)
>  		status = cached_leden & LEDEN_LEDAON;
>  	else
>  		status = cached_leden & LEDEN_LEDBON;
> +
>  	return (status < 0) ? 0 : status;
>  }
>  
> @@ -333,12 +341,14 @@ static void twl_set(struct gpio_chip *chip, unsigned offset, int value)
>  
>  static int twl_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
> -	return (twl4030_gpio_irq_base && (offset < TWL4030_GPIO_MAX))
> -		? (twl4030_gpio_irq_base + offset)
> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
> +
> +	return (priv->irq_base && (offset < TWL4030_GPIO_MAX))
> +		? (priv->irq_base + offset)
>  		: -EINVAL;
>  }
>  
> -static struct gpio_chip twl_gpiochip = {
> +static struct gpio_chip template_chip = {
>  	.label			= "twl4030",
>  	.owner			= THIS_MODULE,
>  	.request		= twl_request,
> @@ -424,8 +434,14 @@ static int __devinit gpio_twl4030_probe(struct platform_device *pdev)
>  {
>  	struct twl4030_gpio_platform_data *pdata = pdev->dev.platform_data;
>  	struct device_node *node = pdev->dev.of_node;
> +	struct gpio_twl4030_priv *priv;
>  	int ret, irq_base;
>  
> +	priv = devm_kzalloc(&pdev->dev, sizeof(struct gpio_twl4030_priv),
> +			    GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
>  	/* maybe setup IRQs */
>  	if (is_module()) {
>  		dev_err(&pdev->dev, "can't dispatch IRQs from modules\n");
> @@ -445,12 +461,13 @@ static int __devinit gpio_twl4030_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		return ret;
>  
> -	twl4030_gpio_irq_base = irq_base;
> +	priv->irq_base = irq_base;
>  
>  no_irqs:
> -	twl_gpiochip.base = -1;
> -	twl_gpiochip.ngpio = TWL4030_GPIO_MAX;
> -	twl_gpiochip.dev = &pdev->dev;
> +	priv->gpio_chip = template_chip;
> +	priv->gpio_chip.base = -1;
> +	priv->gpio_chip.ngpio = TWL4030_GPIO_MAX;
> +	priv->gpio_chip.dev = &pdev->dev;
>  
>  	if (node)
>  		pdata = of_gpio_twl4030(&pdev->dev);
> @@ -481,23 +498,23 @@ no_irqs:
>  	 * is (still) clear if use_leds is set.
>  	 */
>  	if (pdata->use_leds)
> -		twl_gpiochip.ngpio += 2;
> +		priv->gpio_chip.ngpio += 2;
>  
> -	ret = gpiochip_add(&twl_gpiochip);
> +	ret = gpiochip_add(&priv->gpio_chip);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "could not register gpiochip, %d\n", ret);
> -		twl_gpiochip.ngpio = 0;
> +		priv->gpio_chip.ngpio = 0;
>  		gpio_twl4030_remove(pdev);
>  		goto out;
>  	}
>  
> -	twl4030_gpio_base = twl_gpiochip.base;
> +	platform_set_drvdata(pdev, priv);
>  
>  	if (pdata && pdata->setup) {
>  		int status;
>  
> -		status = pdata->setup(&pdev->dev,
> -				twl4030_gpio_base, TWL4030_GPIO_MAX);
> +		status = pdata->setup(&pdev->dev, priv->gpio_chip.base,
> +				      TWL4030_GPIO_MAX);
>  		if (status)
>  			dev_dbg(&pdev->dev, "setup --> %d\n", status);
>  	}
> @@ -510,18 +527,19 @@ out:
>  static int gpio_twl4030_remove(struct platform_device *pdev)
>  {
>  	struct twl4030_gpio_platform_data *pdata = pdev->dev.platform_data;
> +	struct gpio_twl4030_priv *priv = platform_get_drvdata(pdev);
>  	int status;
>  
>  	if (pdata && pdata->teardown) {
> -		status = pdata->teardown(&pdev->dev,
> -				twl4030_gpio_base, TWL4030_GPIO_MAX);
> +		status = pdata->teardown(&pdev->dev, priv->gpio_chip.base,
> +					 TWL4030_GPIO_MAX);
>  		if (status) {
>  			dev_dbg(&pdev->dev, "teardown --> %d\n", status);
>  			return status;
>  		}
>  	}
>  
> -	status = gpiochip_remove(&twl_gpiochip);
> +	status = gpiochip_remove(&priv->gpio_chip);
>  	if (status < 0)
>  		return status;
>  
> -- 
> 1.8.0
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Re: [PATCH v2 2/3] gpio: twl4030: Cache the direction and output states in private data

[Grant Likely]

On Thu, 6 Dec 2012 11:52:06 +0100, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> Use more coherent locking in the driver. Use bitfield to store the GPIO
> direction and if the pin is configured as output store the status also in a
> bitfiled.
> In this way we can just look at these bitfields when we need information
> about the pin status and only reach out to the chip when it is needed.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Applied, thanks

g.

> ---
>  drivers/gpio/gpio-twl4030.c | 99 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 66 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
> index c092739..a38e6e9c 100644
> --- a/drivers/gpio/gpio-twl4030.c
> +++ b/drivers/gpio/gpio-twl4030.c
> @@ -64,14 +64,15 @@
>  /* Mask for GPIO registers when aggregated into a 32-bit integer */
>  #define GPIO_32_MASK			0x0003ffff
>  
> -/* Data structures */
> -static DEFINE_MUTEX(gpio_lock);
> -
>  struct gpio_twl4030_priv {
>  	struct gpio_chip gpio_chip;
> +	struct mutex mutex;
>  	int irq_base;
>  
> +	/* Bitfields for state caching */
>  	unsigned int usage_count;
> +	unsigned int direction;
> +	unsigned int out_state;
>  };
>  
>  /*----------------------------------------------------------------------*/
> @@ -130,7 +131,7 @@ static inline int gpio_twl4030_read(u8 address)
>  
>  /*----------------------------------------------------------------------*/
>  
> -static u8 cached_leden;		/* protected by gpio_lock */
> +static u8 cached_leden;
>  
>  /* The LED lines are open drain outputs ... a FET pulls to GND, so an
>   * external pullup is needed.  We could also expose the integrated PWM
> @@ -144,14 +145,12 @@ static void twl4030_led_set_value(int led, int value)
>  	if (led)
>  		mask <<= 1;
>  
> -	mutex_lock(&gpio_lock);
>  	if (value)
>  		cached_leden &= ~mask;
>  	else
>  		cached_leden |= mask;
>  	status = twl_i2c_write_u8(TWL4030_MODULE_LED, cached_leden,
>  				  TWL4030_LED_LEDEN_REG);
> -	mutex_unlock(&gpio_lock);
>  }
>  
>  static int twl4030_set_gpio_direction(int gpio, int is_input)
> @@ -162,7 +161,6 @@ static int twl4030_set_gpio_direction(int gpio, int is_input)
>  	u8 base = REG_GPIODATADIR1 + d_bnk;
>  	int ret = 0;
>  
> -	mutex_lock(&gpio_lock);
>  	ret = gpio_twl4030_read(base);
>  	if (ret >= 0) {
>  		if (is_input)
> @@ -172,7 +170,6 @@ static int twl4030_set_gpio_direction(int gpio, int is_input)
>  
>  		ret = gpio_twl4030_write(base, reg);
>  	}
> -	mutex_unlock(&gpio_lock);
>  	return ret;
>  }
>  
> @@ -212,7 +209,7 @@ static int twl_request(struct gpio_chip *chip, unsigned offset)
>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>  	int status = 0;
>  
> -	mutex_lock(&gpio_lock);
> +	mutex_lock(&priv->mutex);
>  
>  	/* Support the two LED outputs as output-only GPIOs. */
>  	if (offset >= TWL4030_GPIO_MAX) {
> @@ -271,7 +268,7 @@ done:
>  	if (!status)
>  		priv->usage_count |= BIT(offset);
>  
> -	mutex_unlock(&gpio_lock);
> +	mutex_unlock(&priv->mutex);
>  	return status;
>  }
>  
> @@ -279,64 +276,98 @@ static void twl_free(struct gpio_chip *chip, unsigned offset)
>  {
>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>  
> +	mutex_lock(&priv->mutex);
>  	if (offset >= TWL4030_GPIO_MAX) {
>  		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, 1);
>  		return;
>  	}
>  
> -	mutex_lock(&gpio_lock);
> -
>  	priv->usage_count &= ~BIT(offset);
>  
>  	/* on last use, switch off GPIO module */
>  	if (!priv->usage_count)
>  		gpio_twl4030_write(REG_GPIO_CTRL, 0x0);
>  
> -	mutex_unlock(&gpio_lock);
> +	mutex_unlock(&priv->mutex);
>  }
>  
>  static int twl_direction_in(struct gpio_chip *chip, unsigned offset)
>  {
> -	return (offset < TWL4030_GPIO_MAX)
> -		? twl4030_set_gpio_direction(offset, 1)
> -		: -EINVAL;
> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
> +	int ret;
> +
> +	mutex_lock(&priv->mutex);
> +	if (offset < TWL4030_GPIO_MAX)
> +		ret = twl4030_set_gpio_direction(offset, 1);
> +	else
> +		ret = -EINVAL;
> +
> +	if (!ret)
> +		priv->direction &= ~BIT(offset);
> +
> +	mutex_unlock(&priv->mutex);
> +
> +	return ret;
>  }
>  
>  static int twl_get(struct gpio_chip *chip, unsigned offset)
>  {
>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
> +	int ret;
>  	int status = 0;
>  
> -	if (!(priv->usage_count & BIT(offset)))
> -		return -EPERM;
> +	mutex_lock(&priv->mutex);
> +	if (!(priv->usage_count & BIT(offset))) {
> +		ret = -EPERM;
> +		goto out;
> +	}
>  
> -	if (offset < TWL4030_GPIO_MAX)
> -		status = twl4030_get_gpio_datain(offset);
> -	else if (offset == TWL4030_GPIO_MAX)
> -		status = cached_leden & LEDEN_LEDAON;
> +	if (priv->direction & BIT(offset))
> +		status = priv->out_state & BIT(offset);
>  	else
> -		status = cached_leden & LEDEN_LEDBON;
> +		status = twl4030_get_gpio_datain(offset);
>  
> -	return (status < 0) ? 0 : status;
> +	ret = (status <= 0) ? 0 : 1;
> +out:
> +	mutex_unlock(&priv->mutex);
> +	dev_err(chip->dev, "%s: offset %d value %d\n", __func__, offset, ret);
> +	return ret;
>  }
>  
> -static int twl_direction_out(struct gpio_chip *chip, unsigned offset, int value)
> +static void twl_set(struct gpio_chip *chip, unsigned offset, int value)
>  {
> -	if (offset < TWL4030_GPIO_MAX) {
> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
> +
> +	dev_err(chip->dev, "%s: offset %d value %d\n", __func__, offset, value);
> +	mutex_lock(&priv->mutex);
> +	if (offset < TWL4030_GPIO_MAX)
>  		twl4030_set_gpio_dataout(offset, value);
> -		return twl4030_set_gpio_direction(offset, 0);
> -	} else {
> +	else
>  		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, value);
> -		return 0;
> -	}
> +
> +	if (value)
> +		priv->out_state |= BIT(offset);
> +	else
> +		priv->out_state &= ~BIT(offset);
> +
> +	mutex_unlock(&priv->mutex);
>  }
>  
> -static void twl_set(struct gpio_chip *chip, unsigned offset, int value)
> +static int twl_direction_out(struct gpio_chip *chip, unsigned offset, int value)
>  {
> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
> +
> +	dev_err(chip->dev, "%s: offset %d value %d\n", __func__, offset, value);
> +	mutex_lock(&priv->mutex);
>  	if (offset < TWL4030_GPIO_MAX)
>  		twl4030_set_gpio_dataout(offset, value);
> -	else
> -		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, value);
> +
> +	priv->direction |= BIT(offset);
> +	mutex_unlock(&priv->mutex);
> +
> +	twl_set(chip, offset, value);
> +
> +	return 0;
>  }
>  
>  static int twl_to_irq(struct gpio_chip *chip, unsigned offset)
> @@ -469,6 +500,8 @@ no_irqs:
>  	priv->gpio_chip.ngpio = TWL4030_GPIO_MAX;
>  	priv->gpio_chip.dev = &pdev->dev;
>  
> +	mutex_init(&priv->mutex);
> +
>  	if (node)
>  		pdata = of_gpio_twl4030(&pdev->dev);
>  
> -- 
> 1.8.0
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Re: [PATCH v2 3/3] gpio: twl4030: TODO comment to remove the PWMA/B (LEDA/B) handling

[Grant Likely]

On Thu, 6 Dec 2012 11:52:07 +0100, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> This GPIO driver should not configure anything else then GPIOs.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

I'm not sure if this is the right direction. I actually have no problem
with a single driver that registers itself with multiple interfaces (ie.
GPIO and PWM) if it makes sense for it to do so. I suspec that a lot of
the multifunction device drivers break things up more than is strictly
necessary.

I'll still apply this if you think it is the right direction, but I
wanted to throw that though out there for consideration.

g.

> ---
>  drivers/gpio/gpio-twl4030.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-twl4030.c b/drivers/gpio/gpio-twl4030.c
> index a38e6e9c..1e9f08c4 100644
> --- a/drivers/gpio/gpio-twl4030.c
> +++ b/drivers/gpio/gpio-twl4030.c
> @@ -47,6 +47,7 @@
>   * intended to support multiple hosts.
>   *
>   * There are also two LED pins used sometimes as output-only GPIOs.
> + * TODO: Handling of PWMA/B (LEDA/B) should be removed from this GPIO driver!
>   */
>  
>  /* genirq interfaces are not available to modules */
> @@ -131,6 +132,7 @@ static inline int gpio_twl4030_read(u8 address)
>  
>  /*----------------------------------------------------------------------*/
>  
> +/* TODO: Handling of PWMA/B (LEDA/B) should be removed from this GPIO driver! */
>  static u8 cached_leden;
>  
>  /* The LED lines are open drain outputs ... a FET pulls to GND, so an
> -- 
> 1.8.0
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Re: [PATCH v2 2/3] gpio: twl4030: Cache the direction and output states in private data

[Michael Trimarchi]

Hi

Grant Likely <grant.likely@secretlab.ca> wrote:

>On Thu, 6 Dec 2012 11:52:06 +0100, Peter Ujfalusi
><peter.ujfalusi@ti.com> wrote:
>> Use more coherent locking in the driver. Use bitfield to store the
>GPIO
>> direction and if the pin is configured as output store the status
>also in a
>> bitfiled.
>> In this way we can just look at these bitfields when we need
>information
>> about the pin status and only reach out to the chip when it is
>needed.
>> 
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>
>Applied, thanks
>
>g.
>
>> ---
>>  drivers/gpio/gpio-twl4030.c | 99
>++++++++++++++++++++++++++++++---------------
>>  1 file changed, 66 insertions(+), 33 deletions(-)
>> 
>> diff --git a/drivers/gpio/gpio-twl4030.c
>b/drivers/gpio/gpio-twl4030.c
>> index c092739..a38e6e9c 100644
>> --- a/drivers/gpio/gpio-twl4030.c
>> +++ b/drivers/gpio/gpio-twl4030.c
>> @@ -64,14 +64,15 @@
>>  /* Mask for GPIO registers when aggregated into a 32-bit integer */
>>  #define GPIO_32_MASK			0x0003ffff
>>  
>> -/* Data structures */
>> -static DEFINE_MUTEX(gpio_lock);
>> -
>>  struct gpio_twl4030_priv {
>>  	struct gpio_chip gpio_chip;
>> +	struct mutex mutex;
>>  	int irq_base;
>>  
>> +	/* Bitfields for state caching */
>>  	unsigned int usage_count;
>> +	unsigned int direction;
>> +	unsigned int out_state;
>>  };
>>  
>> 
>/*----------------------------------------------------------------------*/
>> @@ -130,7 +131,7 @@ static inline int gpio_twl4030_read(u8 address)
>>  
>> 
>/*----------------------------------------------------------------------*/
>>  
>> -static u8 cached_leden;		/* protected by gpio_lock */
>> +static u8 cached_leden;
>>  
>>  /* The LED lines are open drain outputs ... a FET pulls to GND, so
>an
>>   * external pullup is needed.  We could also expose the integrated
>PWM
>> @@ -144,14 +145,12 @@ static void twl4030_led_set_value(int led, int
>value)
>>  	if (led)
>>  		mask <<= 1;
>>  
>> -	mutex_lock(&gpio_lock);
>>  	if (value)
>>  		cached_leden &= ~mask;
>>  	else
>>  		cached_leden |= mask;
>>  	status = twl_i2c_write_u8(TWL4030_MODULE_LED, cached_leden,
>>  				  TWL4030_LED_LEDEN_REG);
>> -	mutex_unlock(&gpio_lock);
>>  }
>>  
>>  static int twl4030_set_gpio_direction(int gpio, int is_input)
>> @@ -162,7 +161,6 @@ static int twl4030_set_gpio_direction(int gpio,
>int is_input)
>>  	u8 base = REG_GPIODATADIR1 + d_bnk;
>>  	int ret = 0;
>>  
>> -	mutex_lock(&gpio_lock);
>>  	ret = gpio_twl4030_read(base);
>>  	if (ret >= 0) {
>>  		if (is_input)
>> @@ -172,7 +170,6 @@ static int twl4030_set_gpio_direction(int gpio,
>int is_input)
>>  
>>  		ret = gpio_twl4030_write(base, reg);
>>  	}
>> -	mutex_unlock(&gpio_lock);
>>  	return ret;
>>  }
>>  
>> @@ -212,7 +209,7 @@ static int twl_request(struct gpio_chip *chip,
>unsigned offset)
>>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>>  	int status = 0;
>>  
>> -	mutex_lock(&gpio_lock);
>> +	mutex_lock(&priv->mutex);
>>  
>>  	/* Support the two LED outputs as output-only GPIOs. */
>>  	if (offset >= TWL4030_GPIO_MAX) {
>> @@ -271,7 +268,7 @@ done:
>>  	if (!status)
>>  		priv->usage_count |= BIT(offset);
>>  
>> -	mutex_unlock(&gpio_lock);
>> +	mutex_unlock(&priv->mutex);
>>  	return status;
>>  }
>>  
>> @@ -279,64 +276,98 @@ static void twl_free(struct gpio_chip *chip,
>unsigned offset)
>>  {
>>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>>  
>> +	mutex_lock(&priv->mutex);
>>  	if (offset >= TWL4030_GPIO_MAX) {
>>  		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, 1);

I have the mobile but where is the unlock here?


>>  		return;
>>  	}
>>  
>> -	mutex_lock(&gpio_lock);
>> -
>>  	priv->usage_count &= ~BIT(offset);
>>  
>>  	/* on last use, switch off GPIO module */
>>  	if (!priv->usage_count)
>>  		gpio_twl4030_write(REG_GPIO_CTRL, 0x0);
>>  
>> -	mutex_unlock(&gpio_lock);
>> +	mutex_unlock(&priv->mutex);
>>  }
>>  
>>  static int twl_direction_in(struct gpio_chip *chip, unsigned offset)
>>  {
>> -	return (offset < TWL4030_GPIO_MAX)
>> -		? twl4030_set_gpio_direction(offset, 1)
>> -		: -EINVAL;
>> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>> +	int ret;
>> +
>> +	mutex_lock(&priv->mutex);
>> +	if (offset < TWL4030_GPIO_MAX)
>> +		ret = twl4030_set_gpio_direction(offset, 1);
>> +	else
>> +		ret = -EINVAL;
>> +
>> +	if (!ret)
>> +		priv->direction &= ~BIT(offset);
>> +
>> +	mutex_unlock(&priv->mutex);
>> +
>> +	return ret;
>>  }
>>  
>>  static int twl_get(struct gpio_chip *chip, unsigned offset)
>>  {
>>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>> +	int ret;
>>  	int status = 0;
>>  
>> -	if (!(priv->usage_count & BIT(offset)))
>> -		return -EPERM;
>> +	mutex_lock(&priv->mutex);
>> +	if (!(priv->usage_count & BIT(offset))) {
>> +		ret = -EPERM;
>> +		goto out;
>> +	}
>>  
>> -	if (offset < TWL4030_GPIO_MAX)
>> -		status = twl4030_get_gpio_datain(offset);
>> -	else if (offset == TWL4030_GPIO_MAX)
>> -		status = cached_leden & LEDEN_LEDAON;
>> +	if (priv->direction & BIT(offset))
>> +		status = priv->out_state & BIT(offset);
>>  	else
>> -		status = cached_leden & LEDEN_LEDBON;
>> +		status = twl4030_get_gpio_datain(offset);
>>  
>> -	return (status < 0) ? 0 : status;
>> +	ret = (status <= 0) ? 0 : 1;
>> +out:
>> +	mutex_unlock(&priv->mutex);
>> +	dev_err(chip->dev, "%s: offset %d value %d\n", __func__, offset,
>ret);
>> +	return ret;
>>  }
>>  
>> -static int twl_direction_out(struct gpio_chip *chip, unsigned
>offset, int value)
>> +static void twl_set(struct gpio_chip *chip, unsigned offset, int
>value)
>>  {
>> -	if (offset < TWL4030_GPIO_MAX) {
>> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>> +
>> +	dev_err(chip->dev, "%s: offset %d value %d\n", __func__, offset,
>value);
>> +	mutex_lock(&priv->mutex);
>> +	if (offset < TWL4030_GPIO_MAX)
>>  		twl4030_set_gpio_dataout(offset, value);
>> -		return twl4030_set_gpio_direction(offset, 0);
>> -	} else {
>> +	else
>>  		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, value);
>> -		return 0;
>> -	}
>> +
>> +	if (value)
>> +		priv->out_state |= BIT(offset);
>> +	else
>> +		priv->out_state &= ~BIT(offset);
>> +
>> +	mutex_unlock(&priv->mutex);
>>  }
>>  
>> -static void twl_set(struct gpio_chip *chip, unsigned offset, int
>value)
>> +static int twl_direction_out(struct gpio_chip *chip, unsigned
>offset, int value)
>>  {
>> +	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
>> +
>> +	dev_err(chip->dev, "%s: offset %d value %d\n", __func__, offset,
>value);
>> +	mutex_lock(&priv->mutex);
>>  	if (offset < TWL4030_GPIO_MAX)
>>  		twl4030_set_gpio_dataout(offset, value);
>> -	else
>> -		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, value);
>> +
>> +	priv->direction |= BIT(offset);
>> +	mutex_unlock(&priv->mutex);
>> +
>> +	twl_set(chip, offset, value);
>> +
>> +	return 0;
>>  }
>>  
>>  static int twl_to_irq(struct gpio_chip *chip, unsigned offset)
>> @@ -469,6 +500,8 @@ no_irqs:
>>  	priv->gpio_chip.ngpio = TWL4030_GPIO_MAX;
>>  	priv->gpio_chip.dev = &pdev->dev;
>>  
>> +	mutex_init(&priv->mutex);
>> +
>>  	if (node)
>>  		pdata = of_gpio_twl4030(&pdev->dev);
>>  
>> -- 
>> 1.8.0
>> 

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH v2 2/3] gpio: twl4030: Cache the direction and output states in private data

[Grant Likely]

On Wed, 19 Dec 2012 21:53:23 +0100, Michael Trimarchi <michael@amarulasolutions.com> wrote:
> Hi
> 
> Grant Likely <grant.likely@secretlab.ca> wrote:
> 
> >On Thu, 6 Dec 2012 11:52:06 +0100, Peter Ujfalusi
> ><peter.ujfalusi@ti.com> wrote:
> >> Use more coherent locking in the driver. Use bitfield to store the
> >GPIO
> >> direction and if the pin is configured as output store the status
> >also in a
> >> bitfiled.
> >> In this way we can just look at these bitfields when we need
> >information
> >> about the pin status and only reach out to the chip when it is
> >needed.
> >> 
> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> >
> >Applied, thanks
> >
> >g.
> >
> >> @@ -279,64 +276,98 @@ static void twl_free(struct gpio_chip *chip,
> >unsigned offset)
> >>  {
> >>  	struct gpio_twl4030_priv *priv = to_gpio_twl4030(chip);
> >>  
> >> +	mutex_lock(&priv->mutex);
> >>  	if (offset >= TWL4030_GPIO_MAX) {
> >>  		twl4030_led_set_value(offset - TWL4030_GPIO_MAX, 1);
> 
> I have the mobile but where is the unlock here?

Good catch. I've dropped the patch. Peter, please resend a fixed-up version.

g.

Re: [PATCH v2 3/3] gpio: twl4030: TODO comment to remove the PWMA/B (LEDA/B) handling

[Peter Ujfalusi]

On 12/19/2012 06:07 PM, Grant Likely wrote:
> On Thu, 6 Dec 2012 11:52:07 +0100, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>> This GPIO driver should not configure anything else then GPIOs.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> I'm not sure if this is the right direction. I actually have no problem
> with a single driver that registers itself with multiple interfaces (ie.
> GPIO and PWM) if it makes sense for it to do so. I suspec that a lot of
> the multifunction device drivers break things up more than is strictly
> necessary.

We have PWM drivers for these IPs. As you remember this is the reason I
started to work on the gpio-pwm driver so we can have cleaner, more generic
way to map a PWM as a gpio. I really don't like the idea of having the same
PWM code sitting in various places in the kernel just because it was easier to
hack it like that rather then to make an effort for a clean implementation.
The PWM handling in the gpio-twl4030 driver is a prime example of this IMHO.
It is just a shortcut, nothing else.

> I'll still apply this if you think it is the right direction, but I
> wanted to throw that though out there for consideration.

-- 
Péter

Re: [PATCH v2 3/3] gpio: twl4030: TODO comment to remove the PWMA/B (LEDA/B) handling

[Grant Likely]

On Thu, Dec 20, 2012 at 9:23 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 12/19/2012 06:07 PM, Grant Likely wrote:
>> On Thu, 6 Dec 2012 11:52:07 +0100, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>>> This GPIO driver should not configure anything else then GPIOs.
>>>
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>
>> I'm not sure if this is the right direction. I actually have no problem
>> with a single driver that registers itself with multiple interfaces (ie.
>> GPIO and PWM) if it makes sense for it to do so. I suspec that a lot of
>> the multifunction device drivers break things up more than is strictly
>> necessary.
>
> We have PWM drivers for these IPs. As you remember this is the reason I
> started to work on the gpio-pwm driver so we can have cleaner, more generic
> way to map a PWM as a gpio. I really don't like the idea of having the same
> PWM code sitting in various places in the kernel just because it was easier to
> hack it like that rather then to make an effort for a clean implementation.
> The PWM handling in the gpio-twl4030 driver is a prime example of this IMHO.
> It is just a shortcut, nothing else.

Ah, right. (there's nothing wrong with my memory, it's just short)  :-p

g.