[v2] input: gpio-beeper: Simplify GPIO handling

Commit Message

Alexander Shiyan April 26, 2014, 5:46 a.m. UTC

This patch simplifies GPIO handling in the driver by using GPIO
functions based on descriptors. As a result this driver now can
be used for boards without DT support.

Signed-off-by: Alexander Shiyan <[e-mail]>
---
 drivers/input/misc/Kconfig       |  2 +-
 drivers/input/misc/gpio-beeper.c | 34 +++++++++++++++-------------------
 2 files changed, 16 insertions(+), 20 deletions(-)

Comments

Dmitry Torokhov April 29, 2014, 2:21 a.m. UTC | #1

Hi Alexander,

On Sat, Apr 26, 2014 at 09:46:54AM +0400, Alexander Shiyan wrote:
> +	beep->desc = devm_gpiod_get(&pdev->dev, NULL);
> +	if (!beep->desc)
> +		return -EINVAL;

As far as I can see devm_gpiod_get() never returns NULL, but either a
valid pointer or ERR_PTR-encoded error. Why do we check for NULL?

I can drop the check myself, no need to resubmit.

>  
> -static struct of_device_id gpio_beeper_of_match[] = {
> +static struct of_device_id __maybe_unused gpio_beeper_of_match[] = {
>  	{ .compatible = BEEPER_MODNAME, },
>  	{ }
>  };

Instead of marking it as __maybe_unused we should wrap it in
#ifdef CONFIG_OF. I'll do it as well.

Thanks.

Alexander Shiyan April 29, 2014, 2:55 a.m. UTC | #2

Mon, 28 Apr 2014 19:21:04 -0700 ?? Dmitry Torokhov <[e-mail]>:
> Hi Alexander,
> 
> On Sat, Apr 26, 2014 at 09:46:54AM +0400, Alexander Shiyan wrote:
> > +	beep->desc = devm_gpiod_get(&pdev->dev, NULL);
> > +	if (!beep->desc)
> > +		return -EINVAL;
> 
> As far as I can see devm_gpiod_get() never returns NULL, but either a
> valid pointer or ERR_PTR-encoded error. Why do we check for NULL?
> 
> I can drop the check myself, no need to resubmit.

OK.

...

One issue you keep value argument for gpio_beeper_toggle() as bool.
It should be "int" as in original patch, as well as "beeping" value in private struct.

---

Dmitry Torokhov April 29, 2014, 3:22 a.m. UTC | #3

On Tue, Apr 29, 2014 at 06:55:49AM +0400, Alexander Shiyan wrote:
> Mon, 28 Apr 2014 19:21:04 -0700 ?? Dmitry Torokhov <[e-mail]>:
> > Hi Alexander,
> > 
> > On Sat, Apr 26, 2014 at 09:46:54AM +0400, Alexander Shiyan wrote:
> > > +	beep->desc = devm_gpiod_get(&pdev->dev, NULL);
> > > +	if (!beep->desc)
> > > +		return -EINVAL;
> > 
> > As far as I can see devm_gpiod_get() never returns NULL, but either a
> > valid pointer or ERR_PTR-encoded error. Why do we check for NULL?
> > 
> > I can drop the check myself, no need to resubmit.
> 
> OK.
> 
> ...
> 
> One issue you keep value argument for gpio_beeper_toggle() as bool.
> It should be "int" as in original patch, as well as "beeping" value in
> private struct.

Not really, value of gpio_set_value_cansleep() is a boolean, even though
the type is int. I guess using 'int' is historical.  One can't really
set gpio value to let's say 5. 

Thanks.

Patch

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 5928ea7..2ff4425 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -224,7 +224,7 @@  config INPUT_GP2A
 
 config INPUT_GPIO_BEEPER
 	tristate "Generic GPIO Beeper support"
-	depends on OF_GPIO
+	depends on GPIOLIB
 	help
 	  Say Y here if you have a beeper connected to a GPIO pin.
 
diff --git a/drivers/input/misc/gpio-beeper.c b/drivers/input/misc/gpio-beeper.c
index b757435..4b90877 100644
--- a/drivers/input/misc/gpio-beeper.c
+++ b/drivers/input/misc/gpio-beeper.c
@@ -1,7 +1,7 @@ 
 /*
  * Generic GPIO beeper driver
  *
- * Copyright (C) 2013 Alexander Shiyan <[e-mail]>
+ * Copyright (C) 2013-2014 Alexander Shiyan <[e-mail]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -11,7 +11,7 @@ 
 
 #include <linux/input.h>
 #include <linux/module.h>
-#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 #include <linux/workqueue.h>
 #include <linux/platform_device.h>
 
@@ -19,14 +19,13 @@ 
 
 struct gpio_beeper {
 	struct work_struct	work;
-	int			gpio;
-	bool			active_low;
-	bool			beeping;
+	struct gpio_desc	*desc;
+	int			beeping;
 };
 
-static void gpio_beeper_toggle(struct gpio_beeper *beep, bool on)
+static void gpio_beeper_toggle(struct gpio_beeper *beep, int val)
 {
-	gpio_set_value_cansleep(beep->gpio, on ^ beep->active_low);
+	gpiod_set_value_cansleep(beep->desc, val);
 }
 
 static void gpio_beeper_work(struct work_struct *work)
@@ -59,24 +58,24 @@  static void gpio_beeper_close(struct input_dev *input)
 	struct gpio_beeper *beep = input_get_drvdata(input);
 
 	cancel_work_sync(&beep->work);
-	gpio_beeper_toggle(beep, false);
+	gpio_beeper_toggle(beep, 0);
 }
 
 static int gpio_beeper_probe(struct platform_device *pdev)
 {
 	struct gpio_beeper *beep;
-	enum of_gpio_flags flags;
 	struct input_dev *input;
-	unsigned long gflags;
 	int err;
 
 	beep = devm_kzalloc(&pdev->dev, sizeof(*beep), GFP_KERNEL);
 	if (!beep)
 		return -ENOMEM;
 
-	beep->gpio = of_get_gpio_flags(pdev->dev.of_node, 0, &flags);
-	if (!gpio_is_valid(beep->gpio))
-		return beep->gpio;
+	beep->desc = devm_gpiod_get(&pdev->dev, NULL);
+	if (!beep->desc)
+		return -EINVAL;
+	if (IS_ERR(beep->desc))
+		return PTR_ERR(beep->desc);
 
 	input = devm_input_allocate_device(&pdev->dev);
 	if (!input)
@@ -94,10 +93,7 @@  static int gpio_beeper_probe(struct platform_device *pdev)
 
 	input_set_capability(input, EV_SND, SND_BELL);
 
-	beep->active_low = flags & OF_GPIO_ACTIVE_LOW;
-	gflags = beep->active_low ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
-
-	err = devm_gpio_request_one(&pdev->dev, beep->gpio, gflags, pdev->name);
+	err = gpiod_direction_output(beep->desc, 0);
 	if (err)
 		return err;
 
@@ -106,7 +102,7 @@  static int gpio_beeper_probe(struct platform_device *pdev)
 	return input_register_device(input);
 }
 
-static struct of_device_id gpio_beeper_of_match[] = {
+static struct of_device_id __maybe_unused gpio_beeper_of_match[] = {
 	{ .compatible = BEEPER_MODNAME, },
 	{ }
 };
@@ -116,7 +112,7 @@  static struct platform_driver gpio_beeper_platform_driver = {
 	.driver	= {
 		.name		= BEEPER_MODNAME,
 		.owner		= THIS_MODULE,
-		.of_match_table	= gpio_beeper_of_match,
+		.of_match_table	= of_match_ptr(gpio_beeper_of_match),
 	},
 	.probe	= gpio_beeper_probe,
 };