Am Dienstag, den 15.02.2005, 14:43 +0100 schrieb Vojtech Pavlik:
> On Tue, Feb 15, 2005 at 09:57:59AM +0100, Kenan Esau wrote:
> > Am Freitag, den 11.02.2005, 21:10 +0100 schrieb Vojtech Pavlik:
>
> > Here are my changes. I have tested everything on my lifebook B2175 and
> > it works fine for me. I have used DMI for probing. Does anyone have an
> > Idea what devices we have to add to the DMI-probing?
> >
> > Please comment on the code.
>
> > diff -Naur -X dontdiff
> > linux-2.6.11-rc3-vanilla/drivers/input/mouse/lifebook.c
> > linux-2.6.11-rc3-kenan/drivers/input/mouse/lifebook.c
> > --- linux-2.6.11-rc3-vanilla/drivers/input/mouse/lifebook.c 1970-01-01
> > 01:00:00.000000000 +0100
> > +++ linux-2.6.11-rc3-kenan/drivers/input/mouse/lifebook.c 2005-02-14
> > 19:09:37.000000000 +0100
> > @@ -0,0 +1,150 @@
> > +/*
> > + * Fujitsu B-series Lifebook PS/2 TouchScreen driver
> > + *
> > + * Copyright (c) 2005 Vojtech Pavlik <[EMAIL PROTECTED]>
> > + *
> > + * Copyright (c) 2005 Kenan Esau <[EMAIL PROTECTED]>
> > + *
> > + * TouchScreen detection, absolute mode setting and packet layout is taken
> > from
> > + * Harald Hoyer's description of the device.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as
> > published by
> > + * the Free Software Foundation.
> > + */
> > +
> > +#include <linux/input.h>
> > +#include <linux/serio.h>
> > +#include <linux/libps2.h>
> > +#include <linux/dmi.h>
> > +
> > +#include "psmouse.h"
> > +#include "lifebook.h"
> > +
> > +#define LBTOUCH_TOUCHED 0x04
> > +#define LBTOUCH_X_HIGH 0x30
> > +#define LBTOUCH_Y_HIGH 0xC0
> > +#define LBTOUCH_LB 0x01
> > +#define LBTOUCH_RB 0x02
> > +
> > +static int max_y = 937;
>
> This doesn't look correct. I think the correct value here is 1024,
> because that's what is the maximum possible value transfered in the
> packet. With 937 you can get negative values in your code.
Since the input_event-structure takes signed values that does not really
matter. But you are right it looks a little bit strange and I will
change it to 1024. It's 937 at the moment since this is the "ideal"
value for my touchscreen where y_max=937. ;-)
> > +static struct dmi_system_id lifebook_dmi_table[] = {
> > + {
> > + .ident = "Fujitsu Siemens Lifebook B-Sereis",
> > + .matches = {
> > + DMI_MATCH(DMI_PRODUCT_NAME, "LIFEBOOK B Series"),
> > + },
> > + },
> > + { }
> > +};
>
> This might be a bit too much generic. Are you sure there are no B Series
> lifebooks without a touchscreen?
>
> > +static psmouse_ret_t lifebook_process_byte(struct psmouse *psmouse, struct
> > pt_regs *regs)
> > +{
> > + unsigned char *packet = psmouse->packet;
> > + struct input_dev *dev = &psmouse->dev;
> > +
> > + unsigned long x = 0;
> > + unsigned long y = 0;
> > + static uint8_t pkt_lst_touch = 0;
> > + static uint8_t pkt_cur_touch = 0;
> > + uint8_t pkt_lb = packet[0] & LBTOUCH_LB;
> > + uint8_t pkt_rb = packet[0] & LBTOUCH_RB;
>
> Tab/space damage here. Do we really need constants for everything? They
> don't carry any information value, because we already know what the mask
> means from the left side of the assignment.
>
> Another use for constants is where the value would possibly change,
> which again isn't the case with masks.
I put the constants there since I think it is more readable but if you
don't like them I'll throw em out.
> Also, input_regs() is missing here.
>
> > + pkt_cur_touch = packet[0] & LBTOUCH_TOUCHED;
> > +
> > + if ( psmouse->pktcnt != 3 )
> > + return PSMOUSE_GOOD_DATA;
> > +
> > + /* calculate X and Y */
> > + if (pkt_cur_touch) {
> > + x = (packet[1] | ((packet[0] & LBTOUCH_X_HIGH) << 4 ));
> > + y = max_y -
> > + (packet[2] | ((packet[0] & LBTOUCH_Y_HIGH) << 2 ));
> > + } else {
> > + x = ((packet[0] & 0x10) ? packet[1]-256 : packet[1]);
> > + y = - ((packet[0] & 0x20) ? packet[2]-256 : packet[2]);
> > + }
>
> This doesn't make sense. As far as I know, there is bit 3 in byte 0
> which signifies a relative packet. We don't need to take the decision
> how to interpret the axis values based on the touch bit!
I will check this.
>
> > + input_report_key(dev, BTN_LEFT, pkt_lb);
> > + input_report_key(dev, BTN_RIGHT, pkt_rb);
> > + input_report_key(dev, BTN_TOUCH, pkt_cur_touch);
> > +
> > + /* currently touched */
> > + if (pkt_cur_touch) {
> > + input_report_abs(dev, ABS_X, x);
> > + input_report_abs(dev, ABS_Y, y);
> > + }
> > +
> > + /* quickpoint move */
> > + if ( !pkt_cur_touch && !pkt_lst_touch && (x || y ) ) {
> > + input_report_rel(dev, REL_X, x);
> > + input_report_rel(dev, REL_Y, y);
> > + }
>
> You don't need to check for x and y being nonzero here.
>
> This looks like a stupid workaround for not using the relative/absolute
> bit I refer to above properly.
>
> Also, you can simply merge the reporting and computing of the x/y
> values, making the use of the two variables completely unnecessary.
OK
>
> > + input_sync(dev);
> > +
> > + /* save the state for the currently received packet */
> > + pkt_lst_touch = pkt_cur_touch;
> > +
> > + return PSMOUSE_FULL_PACKET;
> > +}
> > +
> > +static int lifebook_initialize(struct psmouse *psmouse)
> > +{
> > + struct ps2dev *ps2dev = &psmouse->ps2dev;
> > + unsigned char param;
> > +
> > + if (ps2_command(ps2dev, NULL, PSMOUSE_CMD_DISABLE))
> > + return -1;
> > +
> > + if (ps2_command(ps2dev, NULL, PSMOUSE_CMD_RESET_BAT))
> > + return -1;
>
> Do we need two resets here? I'd expect RESET_BAT to override completely
> everything.
It's quite a while ago when I developed the init-sequence of the
touchscreen. But if I remember correctly it needed the DISABLE and the
RESET_BAT.
>
> > +
> > + /*
> > + Enable absolute output -- ps2_command fails always but if
> > + you leave this call out the touchsreen will never send
> > + absolute coordinates
> > + */
> > + param = 0x07;
> > + ps2_command(ps2dev, ¶m, PSMOUSE_CMD_SETRES);
>
> Have you checked whether really the touchscreen sends a 0xfe error back,
> or some other value, or timeout? i8042.debug=1 is your friend here.
OK -- that's a good hint
[...]
-
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/
