From: Grant Likely <grant.likely@secretlab.ca>
To: Peter Ujfalusi <peter.ujfalusi@ti.com>,
Linus Walleij <linus.walleij@linaro.org>
Cc: linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org
Subject: Re: [PATCH v2 2/3] gpio: twl4030: Cache the direction and output states in private data
Date: Wed, 19 Dec 2012 17:03:56 +0000 [thread overview]
Message-ID: <20121219170357.0BA403E0C4A@localhost> (raw)
In-Reply-To: <1354791127-20545-3-git-send-email-peter.ujfalusi@ti.com>
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.
next prev parent reply other threads:[~2012-12-19 20:35 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-06 10:52 [PATCH v2 0/3] gpio: twl4030: Correct status reporting for outputs Peter Ujfalusi
2012-12-06 10:52 ` [PATCH v2 1/3] gpio: twl4030: Introduce private structure to store variables needed runtime Peter Ujfalusi
2012-12-19 17:02 ` Grant Likely
2012-12-06 10:52 ` [PATCH v2 2/3] gpio: twl4030: Cache the direction and output states in private data Peter Ujfalusi
2012-12-19 17:03 ` Grant Likely [this message]
2012-12-19 20:53 ` Michael Trimarchi
2012-12-19 22:17 ` Grant Likely
2012-12-06 10:52 ` [PATCH v2 3/3] gpio: twl4030: TODO comment to remove the PWMA/B (LEDA/B) handling Peter Ujfalusi
2012-12-19 17:07 ` Grant Likely
2012-12-20 9:23 ` Peter Ujfalusi
2012-12-20 9:45 ` Grant Likely
2012-12-07 8:09 ` [PATCH v2 0/3] gpio: twl4030: Correct status reporting for outputs Linus Walleij
2012-12-12 11:12 ` Peter Ujfalusi
2012-12-12 11:45 ` Grant Likely
2012-12-12 12:47 ` Peter Ujfalusi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20121219170357.0BA403E0C4A@localhost \
--to=grant.likely@secretlab.ca \
--cc=linus.walleij@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=peter.ujfalusi@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox