Re: [PATCH] input: driver for touchscreen on iPaq h3xxx

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Jul 13, 2014 at 4:59 AM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
> Hi Linus, Dmitry,
>
> On Sat, Jul 12, 2014 at 11:31:21AM +0200, Linus Walleij wrote:
>> From: Dmitry Artamonow <mad_soft@xxxxxxxx>
>
> So is this Dmitry's or Alessandro's work?

Multiple authors. Alessandro, Dmitry and me. In order of sign-off...
The only viable rule I have for multiple authors is to attribute it
to the author who wrote the bulk of the code, and that comes
from Dmitry.

>> +#include <linux/mfd/ipaq-micro.h>
>
> Has this been merged already?

Yes. v3.16 merge window.

>> +static void micro_ts_receive(void *data, int len, unsigned char *msg)
>> +{
>> +     struct touchscreen_data *ts = data;
>> +
>> +     if (len == 4) {
>> +             input_report_abs(ts->input, ABS_X, (msg[2]<<8)+msg[3]);
>
> be16_to_cpup()?

Yes! Replacing everywhere.

>> +             input_report_abs(ts->input, ABS_Y, (msg[0]<<8)+msg[1]);
>> +             input_report_abs(ts->input, ABS_PRESSURE, 1);
>
> Please no fake pressure events, update your tslib instead.
>
>> +             input_report_key(ts->input, BTN_TOUCH, 0);
>
> This seems completely backwards.

Yeah... I wonder what happened here.

>> +     ts->input = input_allocate_device();
>
> Error handling please; also consider using devm_input_allocate_device().
>
>> +
>> +     ts->input->evbit[0] = BIT(EV_ABS);
>> +     ts->input->absbit[0] = BIT(ABS_X) | BIT(ABS_Y) | BIT(ABS_PRESSURE);
>> +     input_set_abs_params(ts->input, ABS_X, 0, 1023, 0, 0);
>> +     input_set_abs_params(ts->input, ABS_Y, 0, 1023, 0, 0);
>> +     input_set_abs_params(ts->input, ABS_PRESSURE, 0x0, 0x1, 0, 0);
>> +
>
> You are missing setting BTN_TOUCH in capabilities so events you are
> trying to send in micro_ts_receive will not go anywhere.

Yeah. And I switched this to using just input_set_capability().

>> +     spin_lock(&ts->micro->lock);
>> +     ts->micro->ts = NULL;
>> +     ts->micro->ts_data = NULL;
>> +     spin_unlock(&ts->micro->lock);
>
> What is the point here?

Unregistering the callbacks so the MFD core device will stop
calling them. Or we will crash the kernel when removing the
driver as next event arrives.

Sending v2 soon.

Yours,
Linus Walleij
--
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




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux