lkml.org 
[lkml]   [2008]   [May]   [19]   [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,

    On Mon, May 19, 2008 at 05:12:56PM +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>
    >

    Thank you for the patch, overall looks very good, some comments below:

    > +
    > +#define DRIVER_VERSION "0.8"
    > +#define DRIVER_DESC "HTC Shift touchscreen driver v" DRIVER_VERSION
    > +

    I'd lose the driver version, kernel version should be enough for
    tracking.

    > +MODULE_AUTHOR("Pau Oliva Fora <pau@eslack.org>");
    > +MODULE_DESCRIPTION(DRIVER_DESC);
    > +MODULE_VERSION(DRIVER_VERSION);
    > +MODULE_LICENSE("GPL");
    > +
    > +#define HTCPEN_PORT_IRQ_CLEAR 0x068
    > +#define HTCPEN_PORT_INIT 0x06c
    > +#define HTCPEN_PORT_INDEX 0x0250
    > +#define HTCPEN_PORT_DATA 0x0251
    > +#define HTCPEN_IRQ 3
    > +
    > +#define X_INDEX 3
    > +#define Y_INDEX 5
    > +#define LSB_XY_INDEX 0xc
    > +#define X_AXIS_MAX 2040
    > +#define Y_AXIS_MAX 2040
    > +#define DEVICE_ENABLE 0xa2
    > +
    > +static int inverse_x;
    > +module_param(inverse_x, bool, 0644);
    > +MODULE_PARM_DESC(inverse_x, "If set X axis is inversed");
    > +static int inverse_y;
    > +module_param(inverse_y, bool, 0644);
    > +MODULE_PARM_DESC(inverse_y, "If set Y axis is inversed");

    For some reason 'inverted' sounds better to my ear. Anyone with
    native English has any opinion here?

    > +
    > +static void poll_htcpen(struct input_dev *htcpen_dev)
    > +{
    > + unsigned short x, y, xy;
    > + unsigned short touch;
    > +
    > + /* 0=press ; 1=release */
    > + outb_p(0xb, HTCPEN_PORT_INDEX);
    > + touch = inb_p(HTCPEN_PORT_DATA);
    > + 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(X_INDEX, HTCPEN_PORT_INDEX);
    > + x = inb_p(HTCPEN_PORT_DATA);
    > +
    > + outb_p(Y_INDEX, HTCPEN_PORT_INDEX);
    > + y = inb_p(HTCPEN_PORT_DATA);
    > +
    > + outb_p(LSB_XY_INDEX, HTCPEN_PORT_INDEX);
    > + xy = inb_p(HTCPEN_PORT_DATA);
    > +
    > + /* get high resolution value of X and Y using LSB */
    > + x = X_AXIS_MAX - ((x * 8) + ((xy >> 4) & 0xf));
    > + y = (y * 8) + (xy & 0xf);
    > + if (inverse_x)
    > + x = X_AXIS_MAX - x;
    > + if (inverse_y)
    > + y = Y_AXIS_MAX - y;
    > +
    > + input_report_abs(htcpen_dev, ABS_X, x);
    > + input_report_abs(htcpen_dev, ABS_Y, y);
    > + }
    > +

    Maybe do:
    if (inb_p(HTCPEN_PORT_DATA)) {
    input_report_key(htcpen_dev, BTN_TOUCH, 0);
    } else {
    ...

    input_report_key(htcpen_dev, BTN_TOUCH, 1);
    input_report_abs(htcpen_dev, ABS_X, x);
    input_report_abs(htcpen_dev, ABS_Y, y);
    }

    > + input_sync(htcpen_dev);
    > +
    > + inb_p(HTCPEN_PORT_IRQ_CLEAR);
    > +}
    > +
    > +static irqreturn_t htcpen_interrupt(int irq, void *handle)
    > +{
    > + struct input_dev *htcpen_dev = handle;
    > +
    > + poll_htcpen(htcpen_dev);
    > + return IRQ_HANDLED;
    > +}

    Why don't you put the poll straight into IRQ handler body?

    > +
    > +static int __devinit htcpen_isa_probe(struct device *dev, unsigned int id)
    > +{
    > + int err;
    > + struct input_dev *htcpen_dev;
    > +
    > + printk(KERN_INFO "htcpen: %s\n", DRIVER_DESC);

    If you want the driver to print identification string it should go
    into the module init function, but I'd rather just keep it silent. If
    it finds the device input core will print nice message about the
    device.

    > +
    > + inb_p(HTCPEN_PORT_IRQ_CLEAR);
    > +
    > + htcpen_dev = input_allocate_device();
    > + if (!htcpen_dev) {
    > + printk(KERN_ERR "htcpen: can't allocate device\n");
    > + err = -ENOMEM;
    > + goto input_alloc_failed;
    > + }
    > +
    > + 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_allocate_device() zeroes the memory so initialize just what is
    needed.

    > +
    > + input_set_abs_params(htcpen_dev, ABS_X, 0, X_AXIS_MAX, 0, 0);
    > + input_set_abs_params(htcpen_dev, ABS_Y, 0, Y_AXIS_MAX, 0, 0);
    > +
    > + htcpen_dev->evbit[0] = BIT_MASK(EV_ABS) | BIT_MASK(EV_KEY);

    > + htcpen_dev->absbit[0] = BIT_MASK(ABS_X) | BIT_MASK(ABS_Y);
    Not needed, input_set_abs_params does this for you.

    > + htcpen_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
    > +
    > + err = request_irq(HTCPEN_IRQ, htcpen_interrupt, 0, "htcpen",
    > + htcpen_dev);
    > + if (err) {
    > + printk(KERN_ERR "htcpen: irq busy\n");
    > + err = -EBUSY;

    Just report the error returned by request_irq, no need to override it.

    > + goto request_irq_failed;
    > + }
    > +
    > + err = input_register_device(htcpen_dev);
    > + if (err)
    > + goto input_register_failed;
    > +
    > + outb_p(DEVICE_ENABLE, HTCPEN_PORT_INIT);
    > +
    > + dev_set_drvdata(dev, htcpen_dev);
    > + return 0;
    > +
    > + input_register_failed:
    > + free_irq(HTCPEN_IRQ, htcpen_dev);
    > + request_irq_failed:
    > + input_free_device(htcpen_dev);
    > + input_alloc_failed:
    > + return err;
    > +}
    > +
    > +static int __devinit htcpen_isa_match(struct device *dev, unsigned int id)
    > +{
    > + unsigned short val1, val2;
    > +
    > + /* device detection */
    > + outb_p(0x8, HTCPEN_PORT_INIT);
    > + val1 = inb_p(HTCPEN_PORT_INIT);
    > + val2 = inb_p(HTCPEN_PORT_IRQ_CLEAR);
    > + if ((val1 > 0xa) || (val2 != 0x55)) {
    > + printk(KERN_ERR "htcpen: no such device (%x|%x)\n", val1, val2);

    KERN_DEBUG at most, I'd simply remove. Not having a device is not an
    error.

    > + return 0;
    > + }
    > + return 1;
    > +}
    > +
    > +static int __devexit htcpen_isa_remove(struct device *dev, unsigned int id)
    > +{
    > + input_unregister_device(dev_get_drvdata(dev));

    At this point the structure may be freed, but interrupts are still
    running and you haven't stopped the device. I would recommend
    implementing ->open() and ->close methonds for the input_dev and start
    and stop the device from there (I thought one of your patches did just
    that).

    > + free_irq(HTCPEN_IRQ, dev_get_drvdata(dev));
    > + input_free_device(dev_get_drvdata(dev));

    Never call input_free_device after input_unregister_device. The
    input_dev structure is refcounted and will free itself one last user
    drops its reference. input_free_device may cause attempt to free
    already freed memory.

    > + dev_set_drvdata(dev, NULL);
    > + printk(KERN_INFO "htcpen: module removed\n");

    Kill this too.

    > + return 0;
    > +}
    > +
    > +static struct isa_driver htcpen_isa_driver = {
    > + .match = htcpen_isa_match,
    > + .probe = htcpen_isa_probe,
    > + .remove = __devexit_p(htcpen_isa_remove),
    > + .driver = {
    > + .owner = THIS_MODULE,
    > + .name = "htcpen",
    > + }
    > +};
    > +
    > +static int __init htcpen_isa_init(void)
    > +{
    > + return isa_register_driver(&htcpen_isa_driver, 1);
    > +}
    > +
    > +static void __exit htcpen_isa_exit(void)
    > +{
    > + isa_unregister_driver(&htcpen_isa_driver);
    > +}
    > +
    --
    Dmitry


    \
     
     \ /
      Last update: 2008-05-19 17:39    [W:0.041 / U:0.240 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site