[PATCH RFC v2] input: Add new driver for GPIO beeper

[PATCH RFC v2] input: Add new driver for GPIO beeper

[Alexander Shiyan]

This patch adds a new driver for the beeper controlled via GPIO pin.
The driver does not depend on the architecture and is positioned as
a replacement for the specific drivers that are used for this function,
for example drivers/input/misc/ixp4xx-beeper.
Since this patch is RFC only, inline comments are welcome.
---
 drivers/input/misc/Kconfig                      |    6 +
 drivers/input/misc/Makefile                     |    1 +
 drivers/input/misc/gpio-beeper.c                |  156 +++++++++++++++++++++++
 include/linux/platform_data/input-gpio-beeper.h |   20 +++
 4 files changed, 183 insertions(+), 0 deletions(-)
 create mode 100644 drivers/input/misc/gpio-beeper.c
 create mode 100644 include/linux/platform_data/input-gpio-beeper.h

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 259ef31..549af80 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -222,6 +222,12 @@ config INPUT_GP2A
 	  To compile this driver as a module, choose M here: the
 	  module will be called gp2ap002a00f.
 
+config INPUT_GPIO_BEEPER
+	tristate "Generic GPIO beeper support"
+	depends on GPIOLIB
+	help
+	  Say Y here if you have a beeper connected to the GPIO pin.
+
 config INPUT_GPIO_TILT_POLLED
 	tristate "Polled GPIO tilt switch"
 	depends on GENERIC_GPIO
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 1f1e1b1..8234854 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_INPUT_DA9052_ONKEY)	+= da9052_onkey.o
 obj-$(CONFIG_INPUT_DA9055_ONKEY)	+= da9055_onkey.o
 obj-$(CONFIG_INPUT_DM355EVM)		+= dm355evm_keys.o
 obj-$(CONFIG_INPUT_GP2A)		+= gp2ap002a00f.o
+obj-$(CONFIG_INPUT_GPIO_BEEPER)		+= gpio-beeper.o
 obj-$(CONFIG_INPUT_GPIO_TILT_POLLED)	+= gpio_tilt_polled.o
 obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_IXP4XX_BEEPER)	+= ixp4xx-beeper.o
diff --git a/drivers/input/misc/gpio-beeper.c b/drivers/input/misc/gpio-beeper.c
new file mode 100644
index 0000000..6bc2a2c
--- /dev/null
+++ b/drivers/input/misc/gpio-beeper.c
@@ -0,0 +1,156 @@
+/*
+ *  Generic GPIO beeper driver
+ *
+ *  Copyright (C) 2012 Alexander Shiyan <shc_work@mail.ru>
+ *
+ * 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
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/gpio.h>
+#include <linux/input.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/workqueue.h>
+#include <linux/platform_device.h>
+
+#include <linux/platform_data/input-gpio-beeper.h>
+
+struct gpio_beeper_priv {
+	struct input_dev	*input;
+	struct delayed_work	work;
+	char			phys[32];
+	int			gpio_nr;
+	int			active_low:1;
+};
+
+static void gpio_beeper_change(struct gpio_beeper_priv *s, int set)
+{
+	gpio_set_value_cansleep(s->gpio_nr, set ^ s->active_low);
+}
+
+static void gpio_beeper_work(struct work_struct *work)
+{
+	struct gpio_beeper_priv *s = container_of(work, struct gpio_beeper_priv,
+						  work.work);
+
+	gpio_beeper_change(s, 0);
+}
+
+static int gpio_beeper_event(struct input_dev *dev, unsigned int type,
+			     unsigned int code, int value)
+{
+	struct gpio_beeper_priv *s = input_get_drvdata(dev);
+
+	if ((type != EV_SND) || (code != SND_BELL))
+		return -ENOTSUPP;
+
+	if (value < 0)
+		return -EINVAL;
+
+	if (!value)
+		value = 1000;
+
+	/* Turning beeper ON */
+	gpio_beeper_change(s, 1);
+	/* Schedule work for turning OFF */
+	mod_delayed_work(system_wq, &s->work, msecs_to_jiffies(value));
+
+	return 0;
+}
+
+static int gpio_beeper_probe(struct platform_device *pdev)
+{
+	struct gpio_beeper_pdata *pdata = dev_get_platdata(&pdev->dev);
+	struct gpio_beeper_priv *s;
+
+	if (!pdata) {
+		dev_err(&pdev->dev, "Missing platform data\n");
+		return -EINVAL;
+	}
+
+	if (!gpio_is_valid(pdata->gpio_nr)) {
+		dev_err(&pdev->dev, "Invalid gpio %i\n", pdata->gpio_nr);
+		return -EINVAL;
+	}
+
+	s = devm_kzalloc(&pdev->dev, sizeof(struct gpio_beeper_priv),
+			 GFP_KERNEL);
+	if (!s) {
+		dev_err(&pdev->dev, "Memory allocate error\n");
+		return -ENOMEM;
+	}
+
+	s->gpio_nr = pdata->gpio_nr;
+	s->active_low = pdata->active_low;
+
+	s->input = devm_input_allocate_device(&pdev->dev);
+	if (!s->input) {
+		dev_err(&pdev->dev, "Input device allocate error\n");
+		return -ENOMEM;
+	}
+
+	snprintf(s->phys, sizeof(s->phys), "%s/input0",
+		 pdata->name ? pdata->name : dev_name(&pdev->dev));
+	s->input->dev.parent	= &pdev->dev;
+	s->input->name		= dev_name(&pdev->dev);
+	s->input->phys		= s->phys;
+	s->input->id.bustype	= BUS_HOST;
+	s->input->id.vendor	= 0x0001;
+	s->input->id.product	= 0x0001;
+	s->input->id.version	= 0x0100;
+	s->input->evbit[0]	= BIT(EV_SND);
+	s->input->sndbit[0]	= BIT(SND_BELL);
+	s->input->event		= gpio_beeper_event;
+
+	if (devm_gpio_request(&pdev->dev, s->gpio_nr, s->input->name)) {
+		dev_err(&pdev->dev, "Unable to claim gpio %i\n", s->gpio_nr);
+		return -EBUSY;
+	}
+
+	gpio_direction_output(s->gpio_nr, s->active_low);
+
+	input_set_drvdata(s->input, s);
+	platform_set_drvdata(pdev, s);
+
+	INIT_DELAYED_WORK(&s->work, gpio_beeper_work);
+
+	return input_register_device(s->input);
+}
+
+static int gpio_beeper_remove(struct platform_device *pdev)
+{
+	struct gpio_beeper_priv *s = platform_get_drvdata(pdev);
+
+	input_unregister_device(s->input);
+	cancel_delayed_work_sync(&s->work);
+	gpio_beeper_change(s, 0);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static void gpio_beeper_shutdown(struct platform_device *pdev)
+{
+	struct gpio_beeper_priv *s = platform_get_drvdata(pdev);
+
+	/* Turning OFF immediately */
+	flush_delayed_work(&s->work);
+}
+
+static struct platform_driver gpio_beeper_platform_driver = {
+	.driver	= {
+		.name	= "gpio-beeper",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= gpio_beeper_probe,
+	.remove		= gpio_beeper_remove,
+	.shutdown	= gpio_beeper_shutdown,
+};
+module_platform_driver(gpio_beeper_platform_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
+MODULE_DESCRIPTION("Generic GPIO beeper driver");
diff --git a/include/linux/platform_data/input-gpio-beeper.h b/include/linux/platform_data/input-gpio-beeper.h
new file mode 100644
index 0000000..9eb4362
--- /dev/null
+++ b/include/linux/platform_data/input-gpio-beeper.h
@@ -0,0 +1,20 @@
+/*
+ *  Copyright (C) 2012 Alexander Shiyan <shc_work@mail.ru>
+ *
+ * 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
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __PLATFORM_DATA_INPUT_GPIO_BEEPER_H_
+#define __PLATFORM_DATA_INPUT_GPIO_BEEPER_H_
+
+/* gpio-beeper platform data structure */
+struct gpio_beeper_pdata {
+	const char	*name;		/* Name of device (Optional) */
+	int		gpio_nr;	/* GPIO number */
+	int		active_low:1;	/* Set if active level is LOW */
+};
+
+#endif
-- 
1.7.8.6

Re: [PATCH RFC v2] input: Add new driver for GPIO beeper

[Dmitry Torokhov]

Hi Alexander,

On Thu, Nov 29, 2012 at 09:28:59PM +0400, Alexander Shiyan wrote:
> This patch adds a new driver for the beeper controlled via GPIO pin.
> The driver does not depend on the architecture and is positioned as
> a replacement for the specific drivers that are used for this function,
> for example drivers/input/misc/ixp4xx-beeper.
> Since this patch is RFC only, inline comments are welcome.

Your signed-off-by is missing...

> +
> +static int gpio_beeper_event(struct input_dev *dev, unsigned int type,
> +			     unsigned int code, int value)
> +{
> +	struct gpio_beeper_priv *s = input_get_drvdata(dev);
> +
> +	if ((type != EV_SND) || (code != SND_BELL))
> +		return -ENOTSUPP;
> +
> +	if (value < 0)
> +		return -EINVAL;
> +
> +	if (!value)
> +		value = 1000;
> +
> +	/* Turning beeper ON */
> +	gpio_beeper_change(s, 1);

You are running under spinlock, you can't touch GPIO here using
"cansleep" operations.

> +	/* Schedule work for turning OFF */
> +	mod_delayed_work(system_wq, &s->work, msecs_to_jiffies(value));
> +

This is incorrect. The "value" here is pitch, not the duration of sound.
The callers are responsible to shut off the sound. The difference
between SND_BELL and SND_TONE is that latter allows controlling pitch
while the former uses driver- and platform-specific pitch.

I think the driver should look like in the patch below. Can you please
tell me if it works for you?

Thanks!

-- 
Dmitry


Input: Add new driver for GPIO beeper

From: Alexander Shiyan <shc_work@mail.ru>

This patch adds a new driver for the beeper controlled via GPIO pin.
The driver does not depend on the architecture and is positioned as
a replacement for the specific drivers that are used for this function,
for example drivers/input/misc/ixp4xx-beeper.
Since this patch is RFC only, inline comments are welcome.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/Kconfig                      |    9 +
 drivers/input/misc/Makefile                     |    1 
 drivers/input/misc/gpio-beeper.c                |  154 +++++++++++++++++++++++
 include/linux/platform_data/input-gpio-beeper.h |   20 +++
 4 files changed, 184 insertions(+)
 create mode 100644 drivers/input/misc/gpio-beeper.c
 create mode 100644 include/linux/platform_data/input-gpio-beeper.h

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 6d2bf0c..0d9b0eb 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -219,6 +219,15 @@ config INPUT_GP2A
 	  To compile this driver as a module, choose M here: the
 	  module will be called gp2ap002a00f.
 
+config INPUT_GPIO_BEEPER
+	tristate "Generic GPIO beeper support"
+	depends on GPIOLIB
+	help
+	  Say Y here if you have a beeper connected to a GPIO pin.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gpio_beeper.
+
 config INPUT_GPIO_TILT_POLLED
 	tristate "Polled GPIO tilt switch"
 	depends on GENERIC_GPIO
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 1f874af..662b39a 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_INPUT_DA9052_ONKEY)	+= da9052_onkey.o
 obj-$(CONFIG_INPUT_DA9055_ONKEY)	+= da9055_onkey.o
 obj-$(CONFIG_INPUT_DM355EVM)		+= dm355evm_keys.o
 obj-$(CONFIG_INPUT_GP2A)		+= gp2ap002a00f.o
+obj-$(CONFIG_INPUT_GPIO_BEEPER)		+= gpio-beeper.o
 obj-$(CONFIG_INPUT_GPIO_TILT_POLLED)	+= gpio_tilt_polled.o
 obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_IXP4XX_BEEPER)	+= ixp4xx-beeper.o
diff --git a/drivers/input/misc/gpio-beeper.c b/drivers/input/misc/gpio-beeper.c
new file mode 100644
index 0000000..3408d43
--- /dev/null
+++ b/drivers/input/misc/gpio-beeper.c
@@ -0,0 +1,154 @@
+/*
+ *  Generic GPIO beeper driver
+ *
+ *  Copyright (C) 2012 Alexander Shiyan <shc_work@mail.ru>
+ *
+ * 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
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/gpio.h>
+#include <linux/input.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/workqueue.h>
+#include <linux/platform_device.h>
+
+#include <linux/platform_data/input-gpio-beeper.h>
+
+struct gpio_beeper {
+	struct input_dev	*input;
+	struct work_struct	work;
+	char			phys[32];
+	int			gpio_nr;
+	bool			active_low;
+	bool			beeping;
+};
+
+static void gpio_beeper_toggle(struct gpio_beeper *beeper, bool on)
+{
+	gpio_set_value_cansleep(beeper->gpio_nr, on ^ beeper->active_low);
+}
+
+static void gpio_beeper_work(struct work_struct *work)
+{
+	struct gpio_beeper *beeper =
+				container_of(work, struct gpio_beeper, work);
+
+	gpio_beeper_toggle(beeper, beeper->beeping);
+}
+
+static int gpio_beeper_event(struct input_dev *dev, unsigned int type,
+			     unsigned int code, int value)
+{
+	struct gpio_beeper *beeper = input_get_drvdata(dev);
+
+	if (type != EV_SND || code != SND_BELL)
+		return -ENOTSUPP;
+
+	if (value < 0)
+		return -EINVAL;
+
+	beeper->beeping = value;
+	/* Schedule work to actually turn the beeper on or off */
+	schedule_work(&beeper->work);
+
+	return 0;
+}
+
+static void gpio_beeper_close(struct input_dev *input)
+{
+	struct gpio_beeper *beeper = input_get_drvdata(input);
+
+	cancel_work_sync(&beeper->work);
+	gpio_beeper_toggle(beeper, false);
+}
+
+static int gpio_beeper_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct gpio_beeper_pdata *pdata = dev_get_platdata(dev);
+	struct gpio_beeper *beeper;
+	struct input_dev *input;
+	int error;
+
+	if (!pdata) {
+		dev_err(dev, "Missing platform data\n");
+		return -EINVAL;
+	}
+
+	if (!gpio_is_valid(pdata->gpio_nr)) {
+		dev_err(dev, "Invalid gpio %i\n", pdata->gpio_nr);
+		return -EINVAL;
+	}
+
+	beeper = devm_kzalloc(&pdev->dev, sizeof(struct gpio_beeper),
+				GFP_KERNEL);
+	if (!beeper) {
+		dev_err(dev, "Memory allocate error\n");
+		return -ENOMEM;
+	}
+
+	beeper->gpio_nr = pdata->gpio_nr;
+	beeper->active_low = pdata->active_low;
+
+	INIT_WORK(&beeper->work, gpio_beeper_work);
+	snprintf(beeper->phys, sizeof(beeper->phys),
+		 "%s/input0", dev_name(dev));
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(&pdev->dev, "Input device allocate error\n");
+		return -ENOMEM;
+	}
+
+	input->name		= pdata->name ?: "GPIO Beeper";
+	input->phys		= beeper->phys;
+	input->id.bustype	= BUS_HOST;
+	input->id.vendor	= 0x0001;
+	input->id.product	= 0x0001;
+	input->id.version	= 0x0100;
+
+	input->close		= gpio_beeper_close;
+	input->event		= gpio_beeper_event;
+
+	input_set_capability(input, EV_SND, SND_BELL);
+
+	error = devm_gpio_request(dev, beeper->gpio_nr, input->name);
+	if (error) {
+		dev_err(dev, "Unable to claim gpio %i\n", beeper->gpio_nr);
+		return error;
+	}
+
+	gpio_direction_output(beeper->gpio_nr, beeper->active_low);
+
+	beeper->input = input;
+	input_set_drvdata(input, beeper);
+	platform_set_drvdata(pdev, beeper);
+
+	return input_register_device(beeper->input);
+}
+
+static void gpio_beeper_shutdown(struct platform_device *pdev)
+{
+	struct gpio_beeper *beeper = platform_get_drvdata(pdev);
+
+	/* Turning OFF immediately */
+	gpio_beeper_close(beeper->input);
+}
+
+static struct platform_driver gpio_beeper_platform_driver = {
+	.driver	= {
+		.name	= "gpio-beeper",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= gpio_beeper_probe,
+	.shutdown	= gpio_beeper_shutdown,
+};
+module_platform_driver(gpio_beeper_platform_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
+MODULE_DESCRIPTION("Generic GPIO beeper driver");
diff --git a/include/linux/platform_data/input-gpio-beeper.h b/include/linux/platform_data/input-gpio-beeper.h
new file mode 100644
index 0000000..9eb4362
--- /dev/null
+++ b/include/linux/platform_data/input-gpio-beeper.h
@@ -0,0 +1,20 @@
+/*
+ *  Copyright (C) 2012 Alexander Shiyan <shc_work@mail.ru>
+ *
+ * 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
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __PLATFORM_DATA_INPUT_GPIO_BEEPER_H_
+#define __PLATFORM_DATA_INPUT_GPIO_BEEPER_H_
+
+/* gpio-beeper platform data structure */
+struct gpio_beeper_pdata {
+	const char	*name;		/* Name of device (Optional) */
+	int		gpio_nr;	/* GPIO number */
+	bool		active_low;	/* Set if active level is LOW */
+};
+
+#endif

Re[2]: [PATCH RFC v2] input: Add new driver for GPIO beeper

[Alexander Shiyan]

...
> I think the driver should look like in the patch below. Can you please
> tell me if it works for you?
...

Can you please send me patch as attachment?

---

Re: [PATCH RFC v2] input: Add new driver for GPIO beeper

[Dmitry Torokhov]

[-- Attachment #1: Type: text/plain, Size: 407 bytes --]

On Fri, Nov 30, 2012 at 10:46:07AM +0400, Alexander Shiyan wrote:
> ...
> > I think the driver should look like in the patch below. Can you please
> > tell me if it works for you?
> ...
> 
> Can you please send me patch as attachment?
> 

Sure, however do you know that if you were to simply save the original
email and fed it to "patch" command it would pick up the diff and apply
it?

Thanks.

-- 
Dmitry

[-- Attachment #2: input-add-new-driver-for-gpio-beeper.patch --]
[-- Type: text/plain, Size: 7166 bytes --]

Input: Add new driver for GPIO beeper

From: Alexander Shiyan <shc_work@mail.ru>

This patch adds a new driver for the beeper controlled via GPIO pin.
The driver does not depend on the architecture and is positioned as
a replacement for the specific drivers that are used for this function,
for example drivers/input/misc/ixp4xx-beeper.
Since this patch is RFC only, inline comments are welcome.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/Kconfig                      |    9 +
 drivers/input/misc/Makefile                     |    1 
 drivers/input/misc/gpio-beeper.c                |  154 +++++++++++++++++++++++
 include/linux/platform_data/input-gpio-beeper.h |   20 +++
 4 files changed, 184 insertions(+)
 create mode 100644 drivers/input/misc/gpio-beeper.c
 create mode 100644 include/linux/platform_data/input-gpio-beeper.h

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 6d2bf0c..0d9b0eb 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -219,6 +219,15 @@ config INPUT_GP2A
 	  To compile this driver as a module, choose M here: the
 	  module will be called gp2ap002a00f.
 
+config INPUT_GPIO_BEEPER
+	tristate "Generic GPIO beeper support"
+	depends on GPIOLIB
+	help
+	  Say Y here if you have a beeper connected to a GPIO pin.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gpio_beeper.
+
 config INPUT_GPIO_TILT_POLLED
 	tristate "Polled GPIO tilt switch"
 	depends on GENERIC_GPIO
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 1f874af..662b39a 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -26,6 +26,7 @@ obj-$(CONFIG_INPUT_DA9052_ONKEY)	+= da9052_onkey.o
 obj-$(CONFIG_INPUT_DA9055_ONKEY)	+= da9055_onkey.o
 obj-$(CONFIG_INPUT_DM355EVM)		+= dm355evm_keys.o
 obj-$(CONFIG_INPUT_GP2A)		+= gp2ap002a00f.o
+obj-$(CONFIG_INPUT_GPIO_BEEPER)		+= gpio-beeper.o
 obj-$(CONFIG_INPUT_GPIO_TILT_POLLED)	+= gpio_tilt_polled.o
 obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_IXP4XX_BEEPER)	+= ixp4xx-beeper.o
diff --git a/drivers/input/misc/gpio-beeper.c b/drivers/input/misc/gpio-beeper.c
new file mode 100644
index 0000000..3408d43
--- /dev/null
+++ b/drivers/input/misc/gpio-beeper.c
@@ -0,0 +1,154 @@
+/*
+ *  Generic GPIO beeper driver
+ *
+ *  Copyright (C) 2012 Alexander Shiyan <shc_work@mail.ru>
+ *
+ * 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
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/gpio.h>
+#include <linux/input.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/workqueue.h>
+#include <linux/platform_device.h>
+
+#include <linux/platform_data/input-gpio-beeper.h>
+
+struct gpio_beeper {
+	struct input_dev	*input;
+	struct work_struct	work;
+	char			phys[32];
+	int			gpio_nr;
+	bool			active_low;
+	bool			beeping;
+};
+
+static void gpio_beeper_toggle(struct gpio_beeper *beeper, bool on)
+{
+	gpio_set_value_cansleep(beeper->gpio_nr, on ^ beeper->active_low);
+}
+
+static void gpio_beeper_work(struct work_struct *work)
+{
+	struct gpio_beeper *beeper =
+				container_of(work, struct gpio_beeper, work);
+
+	gpio_beeper_toggle(beeper, beeper->beeping);
+}
+
+static int gpio_beeper_event(struct input_dev *dev, unsigned int type,
+			     unsigned int code, int value)
+{
+	struct gpio_beeper *beeper = input_get_drvdata(dev);
+
+	if (type != EV_SND || code != SND_BELL)
+		return -ENOTSUPP;
+
+	if (value < 0)
+		return -EINVAL;
+
+	beeper->beeping = value;
+	/* Schedule work to actually turn the beeper on or off */
+	schedule_work(&beeper->work);
+
+	return 0;
+}
+
+static void gpio_beeper_close(struct input_dev *input)
+{
+	struct gpio_beeper *beeper = input_get_drvdata(input);
+
+	cancel_work_sync(&beeper->work);
+	gpio_beeper_toggle(beeper, false);
+}
+
+static int gpio_beeper_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct gpio_beeper_pdata *pdata = dev_get_platdata(dev);
+	struct gpio_beeper *beeper;
+	struct input_dev *input;
+	int error;
+
+	if (!pdata) {
+		dev_err(dev, "Missing platform data\n");
+		return -EINVAL;
+	}
+
+	if (!gpio_is_valid(pdata->gpio_nr)) {
+		dev_err(dev, "Invalid gpio %i\n", pdata->gpio_nr);
+		return -EINVAL;
+	}
+
+	beeper = devm_kzalloc(&pdev->dev, sizeof(struct gpio_beeper),
+				GFP_KERNEL);
+	if (!beeper) {
+		dev_err(dev, "Memory allocate error\n");
+		return -ENOMEM;
+	}
+
+	beeper->gpio_nr = pdata->gpio_nr;
+	beeper->active_low = pdata->active_low;
+
+	INIT_WORK(&beeper->work, gpio_beeper_work);
+	snprintf(beeper->phys, sizeof(beeper->phys),
+		 "%s/input0", dev_name(dev));
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(&pdev->dev, "Input device allocate error\n");
+		return -ENOMEM;
+	}
+
+	input->name		= pdata->name ?: "GPIO Beeper";
+	input->phys		= beeper->phys;
+	input->id.bustype	= BUS_HOST;
+	input->id.vendor	= 0x0001;
+	input->id.product	= 0x0001;
+	input->id.version	= 0x0100;
+
+	input->close		= gpio_beeper_close;
+	input->event		= gpio_beeper_event;
+
+	input_set_capability(input, EV_SND, SND_BELL);
+
+	error = devm_gpio_request(dev, beeper->gpio_nr, input->name);
+	if (error) {
+		dev_err(dev, "Unable to claim gpio %i\n", beeper->gpio_nr);
+		return error;
+	}
+
+	gpio_direction_output(beeper->gpio_nr, beeper->active_low);
+
+	beeper->input = input;
+	input_set_drvdata(input, beeper);
+	platform_set_drvdata(pdev, beeper);
+
+	return input_register_device(beeper->input);
+}
+
+static void gpio_beeper_shutdown(struct platform_device *pdev)
+{
+	struct gpio_beeper *beeper = platform_get_drvdata(pdev);
+
+	/* Turning OFF immediately */
+	gpio_beeper_close(beeper->input);
+}
+
+static struct platform_driver gpio_beeper_platform_driver = {
+	.driver	= {
+		.name	= "gpio-beeper",
+		.owner	= THIS_MODULE,
+	},
+	.probe		= gpio_beeper_probe,
+	.shutdown	= gpio_beeper_shutdown,
+};
+module_platform_driver(gpio_beeper_platform_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
+MODULE_DESCRIPTION("Generic GPIO beeper driver");
diff --git a/include/linux/platform_data/input-gpio-beeper.h b/include/linux/platform_data/input-gpio-beeper.h
new file mode 100644
index 0000000..9eb4362
--- /dev/null
+++ b/include/linux/platform_data/input-gpio-beeper.h
@@ -0,0 +1,20 @@
+/*
+ *  Copyright (C) 2012 Alexander Shiyan <shc_work@mail.ru>
+ *
+ * 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
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __PLATFORM_DATA_INPUT_GPIO_BEEPER_H_
+#define __PLATFORM_DATA_INPUT_GPIO_BEEPER_H_
+
+/* gpio-beeper platform data structure */
+struct gpio_beeper_pdata {
+	const char	*name;		/* Name of device (Optional) */
+	int		gpio_nr;	/* GPIO number */
+	int		active_low:1;	/* Set if active level is LOW */
+};
+
+#endif

Re[2]: [PATCH RFC v2] input: Add new driver for GPIO beeper

[Alexander Shiyan]

> On Thu, Nov 29, 2012 at 09:28:59PM +0400, Alexander Shiyan wrote:
> > This patch adds a new driver for the beeper controlled via GPIO pin.
> > The driver does not depend on the architecture and is positioned as
> > a replacement for the specific drivers that are used for this function,
> > for example drivers/input/misc/ixp4xx-beeper.
> > Since this patch is RFC only, inline comments are welcome.
...
> > +static int gpio_beeper_event(struct input_dev *dev, unsigned int type,
> > +			     unsigned int code, int value)
> > +{
> > +	struct gpio_beeper_priv *s = input_get_drvdata(dev);
> > +
> > +	if ((type != EV_SND) || (code != SND_BELL))
> > +		return -ENOTSUPP;
> > +
> > +	if (value < 0)
> > +		return -EINVAL;
> > +
> > +	if (!value)
> > +		value = 1000;
> > +
> > +	/* Turning beeper ON */
> > +	gpio_beeper_change(s, 1);
> 
> You are running under spinlock, you can't touch GPIO here using
> "cansleep" operations.
> 
> > +	/* Schedule work for turning OFF */
> > +	mod_delayed_work(system_wq, &s->work, msecs_to_jiffies(value));
> > +
> 
> This is incorrect. The "value" here is pitch, not the duration of sound.
> The callers are responsible to shut off the sound. The difference
> between SND_BELL and SND_TONE is that latter allows controlling pitch
> while the former uses driver- and platform-specific pitch.
As it turned out, I understand the purpose of the parameter is incorrect.
My mistake.

> I think the driver should look like in the patch below. Can you please
> tell me if it works for you?
Works. The test was on ARM CLPS711X board with the driver is compiled
into the kernel, ie without modules. Non-critical comments inlined.


> Input: Add new driver for GPIO beeper
> 
> From: Alexander Shiyan <shc_work@mail.ru>
> 
> This patch adds a new driver for the beeper controlled via GPIO pin.
> The driver does not depend on the architecture and is positioned as
> a replacement for the specific drivers that are used for this function,
> for example drivers/input/misc/ixp4xx-beeper.
> Since this patch is RFC only, inline comments are welcome.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/misc/Kconfig                      |    9 +
>  drivers/input/misc/Makefile                     |    1 
>  drivers/input/misc/gpio-beeper.c                |  154 +++++++++++++++++++++++
>  include/linux/platform_data/input-gpio-beeper.h |   20 +++

> +static int gpio_beeper_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct gpio_beeper_pdata *pdata = dev_get_platdata(dev);
> +	struct gpio_beeper *beeper;
> +	struct input_dev *input;
> +	int error;
> +
> +	if (!pdata) {
> +		dev_err(dev, "Missing platform data\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!gpio_is_valid(pdata->gpio_nr)) {
> +		dev_err(dev, "Invalid gpio %i\n", pdata->gpio_nr);
> +		return -EINVAL;
> +	}
> +
> +	beeper = devm_kzalloc(&pdev->dev, sizeof(struct gpio_beeper),
> +				GFP_KERNEL);
I would prefer to do when moving padding on brackets.

> +	if (!beeper) {
> +		dev_err(dev, "Memory allocate error\n");
> +		return -ENOMEM;
> +	}
> +
> +	beeper->gpio_nr = pdata->gpio_nr;
> +	beeper->active_low = pdata->active_low;
> +
> +	INIT_WORK(&beeper->work, gpio_beeper_work);
> +	snprintf(beeper->phys, sizeof(beeper->phys),
> +		 "%s/input0", dev_name(dev));
> +
> +	input = devm_input_allocate_device(dev);
I am temporaty replace this function to input_allocate_device() because
I am tested this driver on 2.6.8.

> +	if (!input) {
> +		dev_err(&pdev->dev, "Input device allocate error\n");
> +		return -ENOMEM;
> +	}
> +
> +	input->name		= pdata->name ?: "GPIO Beeper";
> +	input->phys		= beeper->phys;
> +	input->id.bustype	= BUS_HOST;
> +	input->id.vendor	= 0x0001;
> +	input->id.product	= 0x0001;
> +	input->id.version	= 0x0100;
> +
IMHO, no empty line needed here.

> +	input->close		= gpio_beeper_close;
> +	input->event		= gpio_beeper_event;
...
> +
> +	input_set_capability(input, EV_SND, SND_BELL);
> +
> +	error = devm_gpio_request(dev, beeper->gpio_nr, input->name);
> +	if (error) {
> +		dev_err(dev, "Unable to claim gpio %i\n", beeper->gpio_nr);
> +		return error;
> +	}
> +
> +	gpio_direction_output(beeper->gpio_nr, beeper->active_low);
> +
> +	beeper->input = input;
> +	input_set_drvdata(input, beeper);
> +	platform_set_drvdata(pdev, beeper);
> +
> +	return input_register_device(beeper->input);
> +}
> +
> +static void gpio_beeper_shutdown(struct platform_device *pdev)
> +{
> +	struct gpio_beeper *beeper = platform_get_drvdata(pdev);
> +
> +	/* Turning OFF immediately */
> +	gpio_beeper_close(beeper->input);
> +}
> +
> +static struct platform_driver gpio_beeper_platform_driver = {
> +	.driver	= {
> +		.name	= "gpio-beeper",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= gpio_beeper_probe,
> +	.shutdown	= gpio_beeper_shutdown,
> +};
> +module_platform_driver(gpio_beeper_platform_driver);
No "remove" function?

---

Re: [PATCH RFC v2] input: Add new driver for GPIO beeper

[Dmitry Torokhov]

Hi Alexander,

On Fri, Nov 30, 2012 at 02:18:00PM +0400, Alexander Shiyan wrote:
> > On Thu, Nov 29, 2012 at 09:28:59PM +0400, Alexander Shiyan wrote:
> > > This patch adds a new driver for the beeper controlled via GPIO pin.
> > > The driver does not depend on the architecture and is positioned as
> > > a replacement for the specific drivers that are used for this function,
> > > for example drivers/input/misc/ixp4xx-beeper.
> > > Since this patch is RFC only, inline comments are welcome.
> ...
> > > +static int gpio_beeper_event(struct input_dev *dev, unsigned int type,
> > > +			     unsigned int code, int value)
> > > +{
> > > +	struct gpio_beeper_priv *s = input_get_drvdata(dev);
> > > +
> > > +	if ((type != EV_SND) || (code != SND_BELL))
> > > +		return -ENOTSUPP;
> > > +
> > > +	if (value < 0)
> > > +		return -EINVAL;
> > > +
> > > +	if (!value)
> > > +		value = 1000;
> > > +
> > > +	/* Turning beeper ON */
> > > +	gpio_beeper_change(s, 1);
> > 
> > You are running under spinlock, you can't touch GPIO here using
> > "cansleep" operations.
> > 
> > > +	/* Schedule work for turning OFF */
> > > +	mod_delayed_work(system_wq, &s->work, msecs_to_jiffies(value));
> > > +
> > 
> > This is incorrect. The "value" here is pitch, not the duration of sound.
> > The callers are responsible to shut off the sound. The difference
> > between SND_BELL and SND_TONE is that latter allows controlling pitch
> > while the former uses driver- and platform-specific pitch.
> As it turned out, I understand the purpose of the parameter is incorrect.
> My mistake.
> 
> > I think the driver should look like in the patch below. Can you please
> > tell me if it works for you?
> Works. The test was on ARM CLPS711X board with the driver is compiled
> into the kernel, ie without modules. Non-critical comments inlined.
> 

Excellent, thank you.

> 
> > Input: Add new driver for GPIO beeper
> > 
> > From: Alexander Shiyan <shc_work@mail.ru>
> > 
> > This patch adds a new driver for the beeper controlled via GPIO pin.
> > The driver does not depend on the architecture and is positioned as
> > a replacement for the specific drivers that are used for this function,
> > for example drivers/input/misc/ixp4xx-beeper.
> > Since this patch is RFC only, inline comments are welcome.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/misc/Kconfig                      |    9 +
> >  drivers/input/misc/Makefile                     |    1 
> >  drivers/input/misc/gpio-beeper.c                |  154 +++++++++++++++++++++++
> >  include/linux/platform_data/input-gpio-beeper.h |   20 +++
> 
> > +static int gpio_beeper_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	const struct gpio_beeper_pdata *pdata = dev_get_platdata(dev);
> > +	struct gpio_beeper *beeper;
> > +	struct input_dev *input;
> > +	int error;
> > +
> > +	if (!pdata) {
> > +		dev_err(dev, "Missing platform data\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!gpio_is_valid(pdata->gpio_nr)) {
> > +		dev_err(dev, "Invalid gpio %i\n", pdata->gpio_nr);
> > +		return -EINVAL;
> > +	}
> > +
> > +	beeper = devm_kzalloc(&pdev->dev, sizeof(struct gpio_beeper),
> > +				GFP_KERNEL);
> I would prefer to do when moving padding on brackets.

I am not sure what you were trying to say here... Are you saying
GFP_KERNEL should be aligned with opening parenthesis?

> 
> > +	if (!beeper) {
> > +		dev_err(dev, "Memory allocate error\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	beeper->gpio_nr = pdata->gpio_nr;
> > +	beeper->active_low = pdata->active_low;
> > +
> > +	INIT_WORK(&beeper->work, gpio_beeper_work);
> > +	snprintf(beeper->phys, sizeof(beeper->phys),
> > +		 "%s/input0", dev_name(dev));
> > +
> > +	input = devm_input_allocate_device(dev);
> I am temporaty replace this function to input_allocate_device() because
> I am tested this driver on 2.6.8.

Surely not 2.6? Anyway for devm_input_allocate_device() support you need
the following commit from my 'next' branch in
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git

commit 2be975c6d920de989ff5e4bc09ffe87e59d94662
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Date:   Sat Nov 3 12:16:12 2012 -0700

    Input: introduce managed input devices (add devres support)

> 
> > +	if (!input) {
> > +		dev_err(&pdev->dev, "Input device allocate error\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	input->name		= pdata->name ?: "GPIO Beeper";
> > +	input->phys		= beeper->phys;
> > +	input->id.bustype	= BUS_HOST;
> > +	input->id.vendor	= 0x0001;
> > +	input->id.product	= 0x0001;
> > +	input->id.version	= 0x0100;
> > +
> IMHO, no empty line needed here.
> 
> > +	input->close		= gpio_beeper_close;
> > +	input->event		= gpio_beeper_event;
> ...
> > +
> > +	input_set_capability(input, EV_SND, SND_BELL);
> > +
> > +	error = devm_gpio_request(dev, beeper->gpio_nr, input->name);
> > +	if (error) {
> > +		dev_err(dev, "Unable to claim gpio %i\n", beeper->gpio_nr);
> > +		return error;
> > +	}
> > +
> > +	gpio_direction_output(beeper->gpio_nr, beeper->active_low);
> > +
> > +	beeper->input = input;
> > +	input_set_drvdata(input, beeper);
> > +	platform_set_drvdata(pdev, beeper);
> > +
> > +	return input_register_device(beeper->input);
> > +}
> > +
> > +static void gpio_beeper_shutdown(struct platform_device *pdev)
> > +{
> > +	struct gpio_beeper *beeper = platform_get_drvdata(pdev);
> > +
> > +	/* Turning OFF immediately */
> > +	gpio_beeper_close(beeper->input);
> > +}
> > +
> > +static struct platform_driver gpio_beeper_platform_driver = {
> > +	.driver	= {
> > +		.name	= "gpio-beeper",
> > +		.owner	= THIS_MODULE,
> > +	},
> > +	.probe		= gpio_beeper_probe,
> > +	.shutdown	= gpio_beeper_shutdown,
> > +};
> > +module_platform_driver(gpio_beeper_platform_driver);
> No "remove" function?

With all resources being devm_* managed (and shuttign off beeper
happening in gpio_keys_close()) the remove function would be just an
empty stub. As far as I can see platform devices not have to have a
remove function, but testing this by unloading the driver or unbinding
it via sysfs would be nice.

Thanks.

-- 
Dmitry

Re[2]: [PATCH RFC v2] input: Add new driver for GPIO beeper

[Alexander Shiyan]

...
> On Fri, Nov 30, 2012 at 02:18:00PM +0400, Alexander Shiyan wrote:
> > > On Thu, Nov 29, 2012 at 09:28:59PM +0400, Alexander Shiyan wrote:
> > > > This patch adds a new driver for the beeper controlled via GPIO pin.
> > > > The driver does not depend on the architecture and is positioned as
> > > > a replacement for the specific drivers that are used for this function,
> > > > for example drivers/input/misc/ixp4xx-beeper.
...
> > > I think the driver should look like in the patch below. Can you please
> > > tell me if it works for you?
> > Works. The test was on ARM CLPS711X board with the driver is compiled
> > into the kernel, ie without modules. Non-critical comments inlined.
> Excellent, thank you.

> > > +	beeper = devm_kzalloc(&pdev->dev, sizeof(struct gpio_beeper),
> > > +				GFP_KERNEL);
> > I would prefer to do when moving padding on brackets.
> 
> I am not sure what you were trying to say here... Are you saying
> GFP_KERNEL should be aligned with opening parenthesis?
Yes.  This not important, but just looks better.  :)

> > > +	input = devm_input_allocate_device(dev);
> > I am temporaty replace this function to input_allocate_device() because
> > I am tested this driver on 2.6.8.
> 
> Surely not 2.6? Anyway for devm_input_allocate_device() support you need
> the following commit from my 'next' branch in
> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
Sorry. Of course I meant 3.6.8.

> > > +static struct platform_driver gpio_beeper_platform_driver = {
> > > +	.driver	= {
> > > +		.name	= "gpio-beeper",
> > > +		.owner	= THIS_MODULE,
> > > +	},
> > > +	.probe		= gpio_beeper_probe,
> > > +	.shutdown	= gpio_beeper_shutdown,
> > > +};
> > > +module_platform_driver(gpio_beeper_platform_driver);
> > No "remove" function?
> 
> With all resources being devm_* managed (and shuttign off beeper
> happening in gpio_keys_close()) the remove function would be just an
> empty stub. As far as I can see platform devices not have to have a
> remove function, but testing this by unloading the driver or unbinding
> it via sysfs would be nice.
Unfortunately, I am unable to check in as a module.
Otherwise, you're right, because there will be no input_unregister_device,
we can not use the "remove".

You commit the result? Or I should remade complete non-RFC patch?

---

Re: [PATCH RFC v2] input: Add new driver for GPIO beeper

[Dmitry Torokhov]

On Friday, November 30, 2012 09:03:57 PM Alexander Shiyan wrote:
> ...
> 
> > On Fri, Nov 30, 2012 at 02:18:00PM +0400, Alexander Shiyan wrote:
> > > > On Thu, Nov 29, 2012 at 09:28:59PM +0400, Alexander Shiyan wrote:
> > > > > This patch adds a new driver for the beeper controlled via GPIO pin.
> > > > > The driver does not depend on the architecture and is positioned as
> > > > > a replacement for the specific drivers that are used for this
> > > > > function,
> > > > > for example drivers/input/misc/ixp4xx-beeper.
> 
> ...
> 
> > > > I think the driver should look like in the patch below. Can you please
> > > > tell me if it works for you?
> > > 
> > > Works. The test was on ARM CLPS711X board with the driver is compiled
> > > into the kernel, ie without modules. Non-critical comments inlined.
> > 
> > Excellent, thank you.
> > 
> > > > +	beeper = devm_kzalloc(&pdev->dev, sizeof(struct gpio_beeper),
> > > > +				GFP_KERNEL);
> > > 
> > > I would prefer to do when moving padding on brackets.
> > 
> > I am not sure what you were trying to say here... Are you saying
> > GFP_KERNEL should be aligned with opening parenthesis?
> 
> Yes.  This not important, but just looks better.  :)
> 
> > > > +	input = devm_input_allocate_device(dev);
> > > 
> > > I am temporaty replace this function to input_allocate_device() because
> > > I am tested this driver on 2.6.8.
> > 
> > Surely not 2.6? Anyway for devm_input_allocate_device() support you need
> > the following commit from my 'next' branch in
> > git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
> 
> Sorry. Of course I meant 3.6.8.
> 
> > > > +static struct platform_driver gpio_beeper_platform_driver = {
> > > > +	.driver	= {
> > > > +		.name	= "gpio-beeper",
> > > > +		.owner	= THIS_MODULE,
> > > > +	},
> > > > +	.probe		= gpio_beeper_probe,
> > > > +	.shutdown	= gpio_beeper_shutdown,
> > > > +};
> > > > +module_platform_driver(gpio_beeper_platform_driver);
> > > 
> > > No "remove" function?
> > 
> > With all resources being devm_* managed (and shuttign off beeper
> > happening in gpio_keys_close()) the remove function would be just an
> > empty stub. As far as I can see platform devices not have to have a
> > remove function, but testing this by unloading the driver or unbinding
> > it via sysfs would be nice.
> 
> Unfortunately, I am unable to check in as a module.
> Otherwise, you're right, because there will be no input_unregister_device,
> we can not use the "remove".

You can still use input_unregister_device() even if you used
devm_input_allocate_device() to allocate it. But most often you do not
have to, as with this driver.

> 
> You commit the result? Or I should remade complete non-RFC patch?

I will commit, I just need your "Signed-off-by: ..." as you are the author.

Thanks.

-- 
Dmitry