Re: [PATCH] Input: add touchscreen driver for MELFAS MCS-5000 controller

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

On Thursday 14 May 2009 17:49:40 Joonyoung Shim wrote:
> >>>> +	case INPUT_TYPE_SINGLE:
> >>>> +		x = (buffer[READ_X_POS_UPPER] << 8) | buffer[READ_X_POS_LOWER];
> >>>> +		y = (buffer[READ_Y_POS_UPPER] << 8) | buffer[READ_Y_POS_LOWER];
> >>>> +
> >>>> +		input_report_key(data->input_dev, BTN_TOUCH, 1);
> >>>> +		input_report_abs(data->input_dev, ABS_X, x);
> >>>> +		input_report_abs(data->input_dev, ABS_Y, y);
> >>>> +		input_report_abs(data->input_dev, ABS_PRESSURE,
> >>>> +				DEFAULT_PRESSURE);
> >>>
> >>> If the hardware does not support pressure reading don't fake it.
> >>> BTN_TOUCH should be enough to signal touch.
> >>
> >> MCS-5000 supports pressure reading, but the value of pressure is
> >> unstable in my target, so i used the static value defined.
> >> I will add pressure reading after more test.
> >
> > OK. Alternatively you may indicate in the platform data if pressure
> > reading is supported and set ABS_PRESSURE bit and report pressure only
> > when you know it works well.
>
> OK, i will add the thing about pressure in the platform data.
>
> >> signal touch? Do you mean single touch?
> >
> > I meant "signal" as "convey", "indicate".
>
> I see :)
>
> >>>> +
> >>>> +	if (request_irq(data->irq, mcs5000_ts_interrupt,
> >>>> +			IRQF_TRIGGER_LOW, "mcs5000_ts_input", data)) {
> >>>> +		dev_err(&data->client->dev, "Failed to register interrupt\n");
> >>>> +		ret = -EINVAL;
> >>>
> >>> Why EINVAL and not EBUSY? Or better yet, why don't you propagate what
> >>> reqiest_irq returned?
> >>
> >> Hmm, EINVAL is used in wm97xx-core.c file, but you are right.
> >
> > I am always taking patches ;))
>
> OK, i will send the patch about it.
>
> >>>> +{
> >>>> +	struct mcs5000_ts_data *data = i2c_get_clientdata(client);
> >>>> +
> >>>> +	cancel_work_sync(&data->ts_event_work);
> >>>
> >>> There is a race here, IRQ handler may resubmit work after
> >>> cancel_work_sync() returns. You need to  make sure that
> >>> IRQ handler does not resubmit work while device is being shut down.
> >>
> >> ok, how about doing free_irq() before cancel_work_sync() call?
> >
> > Then there is a risk that your work will try to enable_irq() on irq that
> > was freed. I am not sure if IRQ core will be happy with it,
>
> Oh, i didn't think about that, then, how about the following patch?
> The work handler decides trying to enable_irq() by whether irq is NULL or
> not.
>
> @@ -179,7 +179,8 @@ static void mcs5000_ts_irq_worker(struct work_struct
> *work)
>
>  	mcs5000_ts_input_read(data);
>
> -	enable_irq(data->irq);
> +	if (data->irq)
> +		enable_irq(data->irq);
>  }
>
>  static irqreturn_t mcs5000_ts_interrupt(int irq, void *dev_id)
> @@ -307,8 +308,9 @@ static int mcs5000_ts_remove(struct i2c_client *client)
>  {
>  	struct mcs5000_ts_data *data = i2c_get_clientdata(client);
>
> -	cancel_work_sync(&data->ts_event_work);
>  	free_irq(data->irq, data);
> +	data->irq = 0;

Not if you want to enable IRQ 0 every once in a while. I think a separate 
flag, the check in the IRQ handler as opposed to workqueue handler and a 
memory barrier should work though.

> +	cancel_work_sync(&data->ts_event_work);
>  	input_unregister_device(data->input_dev);
>  	input_free_device(data->input_dev);
>  	kfree(data);

-- 
Dmitry