On 3/15/06, Shaun Jackman <[email protected]> wrote:
> Comments, please?
>
Hi Shaun,
Sorry it took me long time to respond... Overall it looks good, I have
couple of comments though.
> >
> > * Use the touch status bit rather than the pressure bits to
> > distinguish a BTN_TOUCH event. Non-pressure-sensitive touchscreens
> > always report full pressure.
> > * Report ABS_PRESSURE information only if the touchscreen supports it.
We should not only omit reporting pressure if toucscreen does not
support it but also not set ABS_PRESSURE bit in input device.
> > * Implement the checksum calculation correctly, and verify that the
> > transmitted checksum is correct.
> > * Use dev_dbg to log errors in the protocol.
> >
> > Signed-off-by: Shaun Jackman <[email protected]>
> >
> > diff --git a/drivers/input/touchscreen/elo.c b/drivers/input/touchscreen/elo.c
> > index c86a2eb..23b269a 100644
> > --- a/drivers/input/touchscreen/elo.c
> > +++ b/drivers/input/touchscreen/elo.c
> > @@ -35,6 +35,8 @@ MODULE_LICENSE("GPL");
> > */
> >
> > #define ELO_MAX_LENGTH 10
> > +#define ELO10_TOUCH 0x03
> > +#define ELO10_PRESSURE 0x80
> >
> > /*
> > * Per-touchscreen data.
> > @@ -53,38 +55,40 @@ struct elo {
> > static void elo_process_data_10(struct elo* elo, unsigned char data,
> > struct pt_regs *regs)
> > {
> > struct input_dev *dev = elo->dev;
> > + struct device *dbg = &elo->serio->dev;
> >
> > - elo->csum += elo->data[elo->idx] = data;
> > -
> > + elo->data[elo->idx] = data;
> > switch (elo->idx++) {
> > -
> > case 0:
> > + elo->csum = 0xaa;
> > if (data != 'U') {
> > + dev_dbg(dbg, "unsynchronized data: 0x%02x\n", data);
> > elo->idx = 0;
> > - elo->csum = 0;
> > }
> > break;
> > -
> > - case 1:
> > - if (data != 'T') {
> > - elo->idx = 0;
> > - elo->csum = 0;
> > - }
> > - break;
> > -
> > case 9:
> > - if (elo->csum) {
> > - input_regs(dev, regs);
> > - input_report_abs(dev, ABS_X, (elo->data[4] << 8) | elo->data[3]);
> > - input_report_abs(dev, ABS_Y, (elo->data[6] << 8) | elo->data[5]);
> > - input_report_abs(dev, ABS_PRESSURE, (elo->data[8] << 8) | elo->data[7]);
> > - input_report_key(dev, BTN_TOUCH, elo->data[8] || elo->data[7]);
> > - input_sync(dev);
> > - }
> > elo->idx = 0;
> > - elo->csum = 0;
> > + if (elo->csum != elo->data[9]) {
> > + dev_dbg(dbg, "bad checksum: 0x%02x, expected 0x%02x\n",
> > + elo->data[9], elo->csum);
> > + break;
> > + }
> > + if (elo->data[1] != 'T') {
> > + dev_dbg(dbg, "unexpected packet: 0x%02x\n",
> > + elo->data[1]);
> > + break;
> > + }
What is the reason for postponing checking for 'T' until full packet
is assembled? Did you actually see packets with valid checksum but
without 'T'?
> > + input_regs(dev, regs);
> > + input_report_abs(dev, ABS_X, (elo->data[4] << 8) | elo->data[3]);
> > + input_report_abs(dev, ABS_Y, (elo->data[6] << 8) | elo->data[5]);
> > + if (elo->data[2] & ELO10_PRESSURE)
> > + input_report_abs(dev, ABS_PRESSURE,
> > + (elo->data[8] << 8) | elo->data[7]);
> > + input_report_key(dev, BTN_TOUCH, elo->data[2] & ELO10_TOUCH);
> > + input_sync(dev);
> > break;
> > }
> > + elo->csum += data;
> > }
> >
> > static void elo_process_data_6(struct elo* elo, unsigned char data,
> > struct pt_regs *regs)
> > @@ -189,6 +193,7 @@ static void elo_disconnect(struct serio
> > {
> > struct elo* elo = serio_get_drvdata(serio);
> >
> > + dev_dbg(&serio->dev, "disconnect\n");
I am not sure if we want to have this logging here. Serio core or
maybe even driver core might be better suited for this.
> > input_unregister_device(elo->dev);
> > serio_close(serio);
> > serio_set_drvdata(serio, NULL);
> > @@ -207,6 +212,7 @@ static int elo_connect(struct serio *ser
> > struct input_dev *input_dev;
> > int err;
> >
> > + dev_dbg(&serio->dev, "connect\n");
Same here.
> > elo = kzalloc(sizeof(struct elo), GFP_KERNEL);
> > input_dev = input_allocate_device();
> > if (!elo || !input_dev) {
> >
>
--
Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
