Re: [PATCH] HID: TWL4030: add HID Force Feedback vibra

[Jari Vanhala <ext-jari.vanhala@nokia.com>]

On Mon, 2010-02-22 at 19:38 +0100, ext Dmitry Torokhov wrote:
> On Mon, Feb 22, 2010 at 03:19:36PM +0200, Jari Vanhala wrote:
> > On Mon, 2010-02-22 at 07:40 +0100, ext Dmitry Torokhov wrote:
> > > > +struct vibra_info {
> > > > +	struct device		*dev;
> > > > +	struct input_dev	*input_dev;
> > > > +
> > > > +	struct workqueue_struct *workqueue;
> > > 
> > > Why do we need a separate workqueue? Can't keventd serve us?
> > 
> > There were problems with too long delays once in a while, own workqueue
> > made things much better. Also, you can change priority if needed.
> > 
> 
> OK, in this case you shoudl consider starting it only when input device
> is opened and shutting it down when it is closed.

Hum.. Delayed workqueue creating/destroying makes driver much more
complex. I create separate patch for it.

> > > > +static int vibra_play(struct input_dev *dev, void *data,
> > > > +		      struct ff_effect *effect)
> > > > +{
> > > > +	struct vibra_info *info = data;
> > > > +
> > > > +	if (!info->workqueue)
> > > > +		return 0;
> > > 
> > > How can workqueue be missig here?
> > 
> > It's missing when we remove driver. ff-memless wants to stop vibra (if
> > it was running) and destroys info. And also we really don't want to
> > start worker anymore.
> 
> I would expect the code to make sure workqueue is present while there
> are any outstanding playback requests. Otherwise you need more locking
> (what stops workqueue from being deleted right after you checked it?)

I think locking is kind of overkill. Vibra_play is run in atomic-context
and.. but I can add spinlock to protect work.

	++Jam