Message ID | 20121130062815.GA27232@core.coreip.homeip.net (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
> 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? ---
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.
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.
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