[PATCH next 0/2] net: rfkill: gpio: make better use of gpiod API

[PATCH next 0/2] net: rfkill: gpio: make better use of gpiod API

[Uwe Kleine-König]

Hello,

this series adapts the rfkill-gpio driver to make use of the flags
parameter of the gpiod_get* family of functions. Currently this is
optional (with a crude cpp hack) but this should change.

The second patch was split out because it probably makes davem break out
in spots. Still I send it because I consider it a sensible change. Up to
you if you want the discussion. (See
http://mid.gmane.org/20150331.140700.1574593248925827837.davem@davemloft.net
for a similar previous discussion.) For other subsystems I did similar
changes in a single patch without any resulting criticism.

Best regards
Uwe

Uwe Kleine-König (2):
  net: rfkill: gpio: make better use of gpiod API
  net: rfkill: gpio: simplify code flow

 net/rfkill/rfkill-gpio.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

-- 
2.1.4

[PATCH next 1/2] net: rfkill: gpio: make better use of gpiod API

[Uwe Kleine-König]

Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions)
which appeared in v3.17-rc1, the gpiod_get* functions take an additional
parameter that allows to specify direction and initial value for output.

Furthermore there is devm_gpiod_get_optional which is designed to get
optional gpios.

Simplify driver accordingly.

Note this makes error checking more strict because only -ENOENT is
ignored when searching for the GPIOs which is good.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 net/rfkill/rfkill-gpio.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index d978f2f46ff3..d5d58d919552 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -112,21 +112,17 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
 
 	rfkill->clk = devm_clk_get(&pdev->dev, NULL);
 
-	gpio = devm_gpiod_get(&pdev->dev, "reset");
-	if (!IS_ERR(gpio)) {
-		ret = gpiod_direction_output(gpio, 0);
-		if (ret)
-			return ret;
-		rfkill->reset_gpio = gpio;
-	}
+	gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
 
-	gpio = devm_gpiod_get(&pdev->dev, "shutdown");
-	if (!IS_ERR(gpio)) {
-		ret = gpiod_direction_output(gpio, 0);
-		if (ret)
-			return ret;
-		rfkill->shutdown_gpio = gpio;
-	}
+	rfkill->reset_gpio = gpio;
+
+	gpio = devm_gpiod_get_optional(&pdev->dev, "shutdown", GPIOD_OUT_LOW);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+
+	rfkill->shutdown_gpio = gpio;
 
 	/* Make sure at-least one of the GPIO is defined and that
 	 * a name is specified for this instance
-- 
2.1.4

[PATCH RFC next 2/2] net: rfkill: gpio: simplify code flow

[Uwe Kleine-König]

When assigning directly to the pointer contained in the driver data the
local variable can be dropped together with the extra assignment to it.

The downside is that davem doesn't like writing error pointers to
dynamically allocated memory as this might result in bugs like:

	if (driverdata->gpio)
		free_resources(driverdata->gpio);

which is probably wrong for error values. In this case however there is
no cleanup code for the gpios as everything is managed by devm_* and
furthermore already without this change there is an error path further
down in rfkill_gpio_probe that potentially keeps a valid gpio descriptor
in driver data which must not be used. For this eventually calling some
cleanup function is hardly better than on an error pointer.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 net/rfkill/rfkill-gpio.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c
index d5d58d919552..fd540024c506 100644
--- a/net/rfkill/rfkill-gpio.c
+++ b/net/rfkill/rfkill-gpio.c
@@ -92,7 +92,6 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
 {
 	struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data;
 	struct rfkill_gpio_data *rfkill;
-	struct gpio_desc *gpio;
 	int ret;
 
 	rfkill = devm_kzalloc(&pdev->dev, sizeof(*rfkill), GFP_KERNEL);
@@ -112,17 +111,15 @@ static int rfkill_gpio_probe(struct platform_device *pdev)
 
 	rfkill->clk = devm_clk_get(&pdev->dev, NULL);
 
-	gpio = devm_gpiod_get_optional(&pdev->dev, "reset", GPIOD_OUT_LOW);
-	if (IS_ERR(gpio))
-		return PTR_ERR(gpio);
+	rfkill->reset_gpio = devm_gpiod_get_optional(&pdev->dev, "reset",
+						     GPIOD_OUT_LOW);
+	if (IS_ERR(rfkill->reset_gpio))
+		return PTR_ERR(rfkill->reset_gpio);
 
-	rfkill->reset_gpio = gpio;
-
-	gpio = devm_gpiod_get_optional(&pdev->dev, "shutdown", GPIOD_OUT_LOW);
-	if (IS_ERR(gpio))
-		return PTR_ERR(gpio);
-
-	rfkill->shutdown_gpio = gpio;
+	rfkill->shutdown_gpio = devm_gpiod_get_optional(&pdev->dev, "shutdown",
+							GPIOD_OUT_LOW);
+	if (IS_ERR(rfkill->shutdown_gpio))
+		return PTR_ERR(rfkill->shutdown_gpio);
 
 	/* Make sure at-least one of the GPIO is defined and that
 	 * a name is specified for this instance
-- 
2.1.4

Re: [PATCH next 1/2] net: rfkill: gpio: make better use of gpiod API

[Johannes Berg]

On Thu, 2015-05-28 at 11:46 +0200, Uwe Kleine-König wrote:
> Since 39b2bbe3d715 (gpio: add flags argument to gpiod_get*() functions)
> which appeared in v3.17-rc1, the gpiod_get* functions take an additional
> parameter that allows to specify direction and initial value for output.
> 
> Furthermore there is devm_gpiod_get_optional which is designed to get
> optional gpios.
> 
> Simplify driver accordingly.
> 
> Note this makes error checking more strict because only -ENOENT is
> ignored when searching for the GPIOs which is good.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied.

johannes

Re: [PATCH RFC next 2/2] net: rfkill: gpio: simplify code flow

[Johannes Berg]

On Thu, 2015-05-28 at 11:46 +0200, Uwe Kleine-König wrote:
> When assigning directly to the pointer contained in the driver data the
> local variable can be dropped together with the extra assignment to it.

I'm not really sure I see the big benefit of this? It doesn't really
seem to make the code much easier to read/follow.

I also don't really see the (perceived) objection with "dynamic memory"
though since that memory is freed pretty much immediately as soon as we
return a non-zero value from this function ... the function itself
allocates the memory, and clearly we return without it ever being able
to use it, so ...

Anyway - I might apply this for the few removed lines of code, but only
with a better commit log.

johannes

Re: [PATCH RFC next 2/2] net: rfkill: gpio: simplify code flow

[Uwe Kleine-König]

On Fri, May 29, 2015 at 01:17:04PM +0200, Johannes Berg wrote:
> On Thu, 2015-05-28 at 11:46 +0200, Uwe Kleine-König wrote:
> > When assigning directly to the pointer contained in the driver data the
> > local variable can be dropped together with the extra assignment to it.
> 
> I'm not really sure I see the big benefit of this? It doesn't really
> seem to make the code much easier to read/follow.
> 
> I also don't really see the (perceived) objection with "dynamic memory"
> though since that memory is freed pretty much immediately as soon as we
> return a non-zero value from this function ... the function itself
> allocates the memory, and clearly we return without it ever being able
> to use it, so ...
> 
> Anyway - I might apply this for the few removed lines of code, but only
> with a better commit log.
Alternatively squash it into patch 1/2 and add:

	While touching the code simplify it a bit to not need a local
	variable for the gpio descriptors.

?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH RFC next 2/2] net: rfkill: gpio: simplify code flow

[Johannes Berg]

On Fri, 2015-05-29 at 14:42 +0200, Uwe Kleine-König wrote:
> On Fri, May 29, 2015 at 01:17:04PM +0200, Johannes Berg wrote:
> > On Thu, 2015-05-28 at 11:46 +0200, Uwe Kleine-König wrote:
> > > When assigning directly to the pointer contained in the driver data the
> > > local variable can be dropped together with the extra assignment to it.
> > 
> > I'm not really sure I see the big benefit of this? It doesn't really
> > seem to make the code much easier to read/follow.
> > 
> > I also don't really see the (perceived) objection with "dynamic memory"
> > though since that memory is freed pretty much immediately as soon as we
> > return a non-zero value from this function ... the function itself
> > allocates the memory, and clearly we return without it ever being able
> > to use it, so ...
> > 
> > Anyway - I might apply this for the few removed lines of code, but only
> > with a better commit log.
> Alternatively squash it into patch 1/2 and add:
> 
> 	While touching the code simplify it a bit to not need a local
> 	variable for the gpio descriptors.

Too late, already applied & pushed out the other one.

johannes

Re: [PATCH RFC next 2/2] net: rfkill: gpio: simplify code flow

[Uwe Kleine-König]

Hello,

On Fri, May 29, 2015 at 02:45:10PM +0200, Johannes Berg wrote:
> On Fri, 2015-05-29 at 14:42 +0200, Uwe Kleine-König wrote:
> > On Fri, May 29, 2015 at 01:17:04PM +0200, Johannes Berg wrote:
> > > On Thu, 2015-05-28 at 11:46 +0200, Uwe Kleine-König wrote:
> > > > When assigning directly to the pointer contained in the driver data the
> > > > local variable can be dropped together with the extra assignment to it.
> > > 
> > > I'm not really sure I see the big benefit of this? It doesn't really
> > > seem to make the code much easier to read/follow.
> > > 
> > > I also don't really see the (perceived) objection with "dynamic memory"
> > > though since that memory is freed pretty much immediately as soon as we
> > > return a non-zero value from this function ... the function itself
> > > allocates the memory, and clearly we return without it ever being able
> > > to use it, so ...
> > > 
> > > Anyway - I might apply this for the few removed lines of code, but only
> > > with a better commit log.
> > Alternatively squash it into patch 1/2 and add:
> > 
> > 	While touching the code simplify it a bit to not need a local
> > 	variable for the gpio descriptors.
> 
> Too late, already applied & pushed out the other one.
Ah, I thought you wanted a better commit log, didn't understand that you
looked into that yourself.

Best regards and thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH RFC next 2/2] net: rfkill: gpio: simplify code flow

[Uwe Kleine-König]

Hello again,

On Fri, May 29, 2015 at 02:46:37PM +0200, Uwe Kleine-König wrote:
> On Fri, May 29, 2015 at 02:45:10PM +0200, Johannes Berg wrote:
> > On Fri, 2015-05-29 at 14:42 +0200, Uwe Kleine-König wrote:
> > > On Fri, May 29, 2015 at 01:17:04PM +0200, Johannes Berg wrote:
> > > > On Thu, 2015-05-28 at 11:46 +0200, Uwe Kleine-König wrote:
> > > > > When assigning directly to the pointer contained in the driver data the
> > > > > local variable can be dropped together with the extra assignment to it.
> > > > 
> > > > I'm not really sure I see the big benefit of this? It doesn't really
> > > > seem to make the code much easier to read/follow.
> > > > 
> > > > I also don't really see the (perceived) objection with "dynamic memory"
> > > > though since that memory is freed pretty much immediately as soon as we
> > > > return a non-zero value from this function ... the function itself
> > > > allocates the memory, and clearly we return without it ever being able
> > > > to use it, so ...
> > > > 
> > > > Anyway - I might apply this for the few removed lines of code, but only
> > > > with a better commit log.
> > > Alternatively squash it into patch 1/2 and add:
> > > 
> > > 	While touching the code simplify it a bit to not need a local
> > > 	variable for the gpio descriptors.
> > 
> > Too late, already applied & pushed out the other one.
> Ah, I thought you wanted a better commit log, didn't understand that you
> looked into that yourself.
Ah, misunderstood once more. Will take a look for a better commit log.

Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |