On Tue, Jan 25, 2011 at 07:34:23PM +0000, Mark Brown wrote:
> On Tue, Jan 25, 2011 at 09:07:29AM -0800, Dmitry Torokhov wrote:
> > On Tue, Jan 25, 2011 at 02:09:16PM +0000, Mark Brown wrote:
>
> > > + int data_irq;
> > > + int pd_irq;
>
> > Maybe make irqs unsigned?
>
> I guess.
>
> > > +static void wm831x_ts_input_close(struct input_dev *idev)
> > > +{
> > > + struct wm831x_ts *wm831x_ts = input_get_drvdata(idev);
> > > + struct wm831x *wm831x = wm831x_ts->wm831x;
> > > +
> > > + wm831x_set_bits(wm831x, WM831X_TOUCH_CONTROL_1,
> > > + WM831X_TCH_ENA | WM831X_TCH_CVT_ENA |
> > > + WM831X_TCH_X_ENA | WM831X_TCH_Y_ENA |
> > > + WM831X_TCH_Z_ENA, 0);
> > > +
> > > + if (wm831x_ts->pen_down)
> > > + disable_irq_nosync(wm831x_ts->data_irq);
>
> > Why nosync?
>
> Cut'n'paste from the pen up handling in the data IRQ (which obviously
> does need to be nosync due to the fact that it runs from IRQ context).
>
> > > + input_dev = input_allocate_device();
>
> > Are we 100% sure it is impossible for the 2 irqs above to fire up before
> > we get here? Like if we happen to have hardware in half-active state
> > while binding?
>
> It's spectacularly unlikely - in the sort of hardware this device is
> useful for nothing below Linux would realistically want to talk to the
> touchscreen and since the IRQs are threaded they can't be shared.
Would you please move input allocation still - even if it works fine
with your device the code might be used as an example by other
authors... and there should be no penalties for your driver if you
allocate just a tad earlier.
Thanks a lot!
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
