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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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