lkml.org 
[lkml]   [2008]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] Add support for HTC Shift Touchscreen
    Hi Pau.

    Some nitpick comments - looks good overall.


    On Sat, May 17, 2008 at 02:44:14PM +0200, Pau Oliva Fora wrote:
    > From: Pau Oliva Fora <pau@eslack.org>
    >
    > The patch below adds support for HTC Shift UMPC touchscreen.
    >
    > Signed-off-by: Pau Oliva Fora <pau@eslack.org>
    >
    > ---
    >
    > Patch against linux-2.6.25.4, should apply clean on other versions too.
    >
    >
    > diff -uprN linux-2.6.25.4/drivers/input/touchscreen/htcpen.c
    > linux/drivers/input/touchscreen/htcpen.c
    > --- linux-2.6.25.4/drivers/input/touchscreen/htcpen.c 1970-01-01
    > 01:00:00.000000000 +0100
    > +++ linux/drivers/input/touchscreen/htcpen.c 2008-05-17
    > 13:48:57.000000000 +0200
    > @@ -0,0 +1,148 @@
    > +/*
    > + * HTC Shift touchscreen driver
    > + *
    > + * Copyright (C) 2008 Pau Oliva Fora <pof@eslack.org>
    > + *
    > + * Thanks to:
    > + * Heikki Linnakangas - Penmount LPC touchscreen driver
    > + * Wacom / Ping Cheng - Help on linuxwacom-devel
    > + * Esteve Espuna - Ideas, tips, moral support :)
    > + *
    > + * Changelog:
    > + * 29980517 v0.5 - code cleanup
    > + * 29980517 v0.4 - initialization works (thanks esteve!)
    > + * 20080514 v0.3 - works best with TouchKit egalax xorg driver
    > + * 20080511 v0.2 - little hacks to make it more usable
    > + * 20080510 v0.1 - works with evtouch driver
    > + * 20080428 first non-working attempt
    Drop changelog - we do not keep them in-kernel.
    Here we rely on git.
    You can add it to your commit message if your prefer to
    have it saved.

    > + */
    > +
    > +/*
    > + * 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/errno.h>
    > +#include <linux/kernel.h>
    > +#include <linux/module.h>
    > +#include <linux/input.h>
    > +#include <linux/interrupt.h>
    > +#include <linux/io.h>
    > +#include <linux/init.h>
    > +#include <linux/irq.h>
    > +
    > +#define DRIVER_VERSION "v0.5"
    > +#define DRIVER_DESC "HTC Shift touchscreen driver"
    > +
    > +MODULE_AUTHOR("Pau Oliva Fora <pau@eslack.org>");
    > +MODULE_DESCRIPTION(DRIVER_DESC);
    > +MODULE_LICENSE("GPL");
    > +
    > +#define HTCPEN_PORT0 0x068
    > +#define HTCPEN_PORT1 0x06c
    > +#define HTCPEN_PORT2 0x0250
    > +#define HTCPEN_PORT3 0x0251
    The use of the four ports would be nice to be documented

    > +#define HTCPEN_IRQ 3
    > +
    > +static int inverse_y;
    > +module_param(inverse_y, bool, 0644);
    > +MODULE_PARM_DESC(inverse_y, "If set Y axe is inversed.");
    > +
    > +struct input_dev *htcpen_dev;
    > +
    > +static void poll_htcpen(void)
    > +{
    > +
    > + unsigned short x, y, xy;
    General comment.
    In kernel we usually do not have an empty line following the
    opening '{'

    > + unsigned short touch;
    > +
    > + /* 0=press ; 1=release */
    > + outb_p(0xb, HTCPEN_PORT2);
    > + touch = inb_p(HTCPEN_PORT3);
    > + if (touch)
    > + touch = 0;
    > + else
    > + touch = 1;
    > +
    > + input_report_key(htcpen_dev, BTN_TOUCH, touch);
    > +
    > + /* only update x/y when screen is being touched */
    > + if (touch) {
    > + outb_p(3, HTCPEN_PORT2);
    What is the semantic of 3 here.
    Add a comment or a DEFINE
    > + x = inb_p(HTCPEN_PORT3);
    > +
    > + outb_p(5, HTCPEN_PORT2);
    Same here.

    > + y = inb_p(HTCPEN_PORT3);
    > +
    > + outb_p(0xc, HTCPEN_PORT2);
    Same here.

    > + xy = inb_p(HTCPEN_PORT3);
    > +
    > + x = 8*255-((x*8)+((xy>>4)&0xf));
    > + y = (y*8)+(xy&0xf);
    > + if (inverse_y)
    > + y = 8*255-y;
    These calculations needs spaces and comments.


    > +
    > + input_report_abs(htcpen_dev, ABS_X, x);
    > + input_report_abs(htcpen_dev, ABS_Y, y);
    > + }
    > +
    > + input_sync(htcpen_dev);
    > +
    > + inb_p(HTCPEN_PORT0);
    > +}
    > +
    > +static irqreturn_t htcpen_interrupt(int irq, void *handle)
    > +{
    > + poll_htcpen();
    > + return IRQ_HANDLED;
    > +}
    > +
    > +
    > +static int __init htcpen_init(void)
    > +{
    > +
    > + printk(KERN_INFO "htcpen: module inserted\n");
    > +
    > + inb_p(HTCPEN_PORT0);
    > +
    > + htcpen_dev = input_allocate_device();
    > + if (!htcpen_dev) {
    > + printk(KERN_ERR "htcpen: can't allocate device\n");
    > + return -ENOMEM;
    > + }
    > +
    > + if (request_irq(HTCPEN_IRQ, htcpen_interrupt, 0, "htcpen", NULL)) {
    > + printk(KERN_ERR "htcpen: irq busy\n");
    > + return -EBUSY;
    > + }
    I would asusme you are missing a: input_unregister_device(htcpen_dev);?

    > +
    > + htcpen_dev->name = "HTC Pen TouchScreen";
    > + htcpen_dev->id.bustype = BUS_ISA;
    > + htcpen_dev->id.vendor = 0;
    > + htcpen_dev->id.product = 0;
    > + htcpen_dev->id.version = 0;
    > +
    > + input_set_abs_params(htcpen_dev, ABS_X, 0, 255*8, 0, 0);
    > + input_set_abs_params(htcpen_dev, ABS_Y, 0, 255*8, 0, 0);
    Again a 'magic' calculation

    > +
    > + htcpen_dev->evbit[0] = BIT_MASK(EV_ABS) | BIT_MASK(EV_KEY);
    > + htcpen_dev->absbit[0] = BIT_MASK(ABS_X) | BIT_MASK(ABS_Y);
    > + htcpen_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
    > +
    > + input_register_device(htcpen_dev);
    > +
    > + outb_p(0xa2, HTCPEN_PORT1);
    > +
    > + return 0;
    > +}
    > +
    > +static void __exit htcpen_exit(void)
    > +{
    > + input_unregister_device(htcpen_dev);
    > + free_irq(HTCPEN_IRQ, NULL);
    > + printk(KERN_INFO "htcpen: module removed\n");
    > +}
    > +
    > +module_init(htcpen_init);
    > +module_exit(htcpen_exit);


    Sam


    \
     
     \ /
      Last update: 2008-05-17 15:05    [W:2.224 / U:2.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site