> 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 <[email protected]>
>
> 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 <[email protected]>
> ---
> 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?
---
