diff mbox

regulator: core: Still free GPIOs using gpio_free()

Message ID 1404215726-2480-1-git-send-email-broonie@kernel.org
State New
Headers show

Commit Message

Mark Brown July 1, 2014, 11:55 a.m. UTC 
From: Mark Brown <broonie@linaro.org>

Even though we mostly use GPIO descriptors internally we still use
gpio_request_one() to request so we need to pair that with gpio_free() to
release the GPIO.

Reported-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Mark Brown <broonie@linaro.org>
---
 drivers/regulator/core.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Russell King - ARM Linux July 1, 2014, 12:06 p.m. UTC | #1 
On Tue, Jul 01, 2014 at 12:55:26PM +0100, Mark Brown wrote:
> From: Mark Brown <broonie@linaro.org>
> 
> Even though we mostly use GPIO descriptors internally we still use
> gpio_request_one() to request so we need to pair that with gpio_free() to
> release the GPIO.
> 
> Reported-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Mark Brown <broonie@linaro.org>

This doesn't explain the reason...  I see the current GPIO code doing this:

void gpio_free(unsigned gpio)
{
        gpiod_free(gpio_to_desc(gpio));
}
EXPORT_SYMBOL_GPL(gpio_free);
Mark Brown July 1, 2014, 12:11 p.m. UTC | #2 
On Tue, Jul 01, 2014 at 01:06:22PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 01, 2014 at 12:55:26PM +0100, Mark Brown wrote:

> > Even though we mostly use GPIO descriptors internally we still use
> > gpio_request_one() to request so we need to pair that with gpio_free() to
> > release the GPIO.

> This doesn't explain the reason...  I see the current GPIO code doing this:

> void gpio_free(unsigned gpio)
> {
>         gpiod_free(gpio_to_desc(gpio));
> }
> EXPORT_SYMBOL_GPL(gpio_free);

Huh, good point (though it still seems a bit neater to match up the
request and release functions I guess).  Linus?
Alexandre Courbot July 1, 2014, 12:24 p.m. UTC | #3 
On Tue, Jul 1, 2014 at 9:11 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 01, 2014 at 01:06:22PM +0100, Russell King - ARM Linux wrote:
>> On Tue, Jul 01, 2014 at 12:55:26PM +0100, Mark Brown wrote:
>
>> > Even though we mostly use GPIO descriptors internally we still use
>> > gpio_request_one() to request so we need to pair that with gpio_free() to
>> > release the GPIO.
>
>> This doesn't explain the reason...  I see the current GPIO code doing this:
>
>> void gpio_free(unsigned gpio)
>> {
>>         gpiod_free(gpio_to_desc(gpio));
>> }
>> EXPORT_SYMBOL_GPL(gpio_free);
>
> Huh, good point (though it still seems a bit neater to match up the
> request and release functions I guess).  Linus?

Yeah it's a nice habit to free the GPIOs using the same interface as
the one used to obtain them. Although if we are to allow the regulator
framework to request GPIO through both the integer and descriptor
interfaces, we certainly don't want to track that information forever.

On top of that we want to limit the use of gpio_to_desc() and
desc_to_gpio() in drivers as much as possible, so I'd say I prefer to
see gpiod_free() used no matter where the GPIO came from.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mark Brown July 1, 2014, 12:27 p.m. UTC | #4 
On Tue, Jul 01, 2014 at 09:24:22PM +0900, Alexandre Courbot wrote:
> On Tue, Jul 1, 2014 at 9:11 PM, Mark Brown <broonie@kernel.org> wrote:

> > Huh, good point (though it still seems a bit neater to match up the
> > request and release functions I guess).  Linus?

> Yeah it's a nice habit to free the GPIOs using the same interface as
> the one used to obtain them. Although if we are to allow the regulator
> framework to request GPIO through both the integer and descriptor
> interfaces, we certainly don't want to track that information forever.

> On top of that we want to limit the use of gpio_to_desc() and
> desc_to_gpio() in drivers as much as possible, so I'd say I prefer to
> see gpiod_free() used no matter where the GPIO came from.

Right, but Linus said we didn't need this conversion to descriptors so
I'm a bit confused as to what the issue he was raising was.
Linus Walleij July 8, 2014, 8:33 a.m. UTC | #5 
On Tue, Jul 1, 2014 at 2:27 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Jul 01, 2014 at 09:24:22PM +0900, Alexandre Courbot wrote:
>> On Tue, Jul 1, 2014 at 9:11 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > Huh, good point (though it still seems a bit neater to match up the
>> > request and release functions I guess).  Linus?
>
>> Yeah it's a nice habit to free the GPIOs using the same interface as
>> the one used to obtain them. Although if we are to allow the regulator
>> framework to request GPIO through both the integer and descriptor
>> interfaces, we certainly don't want to track that information forever.
>
>> On top of that we want to limit the use of gpio_to_desc() and
>> desc_to_gpio() in drivers as much as possible, so I'd say I prefer to
>> see gpiod_free() used no matter where the GPIO came from.
>
> Right, but Linus said we didn't need this conversion to descriptors so
> I'm a bit confused as to what the issue he was raising was.

Now I'm probably confusing myself...

All drivers should as far as possible use descriptors internally, doing
things like gpio_to_desc() only for backward-compatibility where
necessary, with the intent of eventually ridding the integer interfaces
entirely.

It may be an eternal project, but hey, we are atleast two of us
pursuing it.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Mark Brown July 8, 2014, 11:53 a.m. UTC | #6 
On Tue, Jul 08, 2014 at 10:33:23AM +0200, Linus Walleij wrote:
> On Tue, Jul 1, 2014 at 2:27 PM, Mark Brown <broonie@kernel.org> wrote:

> > Right, but Linus said we didn't need this conversion to descriptors so
> > I'm a bit confused as to what the issue he was raising was.

> Now I'm probably confusing myself...

> All drivers should as far as possible use descriptors internally, doing
> things like gpio_to_desc() only for backward-compatibility where
> necessary, with the intent of eventually ridding the integer interfaces
> entirely.

> It may be an eternal project, but hey, we are atleast two of us
> pursuing it.

OK, so in that case Russell's code should be fine then and we don't need
to do anything - great!
diff mbox

Patch

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index c563d93125cd..80381409f856 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1710,7 +1710,12 @@  static void regulator_ena_gpio_free(struct regulator_dev *rdev)
 		if (pin->gpiod == rdev->ena_pin->gpiod) {
 			if (pin->request_count <= 1) {
 				pin->request_count = 0;
-				gpiod_put(pin->gpiod);
+				/*
+				 * Since we requested with gpio_request_one()
+				 * we still need to free with gpio_free()
+				 * for now.
+				 */
+				gpio_free(desc_to_gpio(pin->gpiod));
 				list_del(&pin->list);
 				kfree(pin);
 			} else {