lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 3 Dec 2008 14:01:44 +0200
From:	Felipe Balbi <felipe.balbi@...ia.com>
To:	ext Trilok Soni <soni.trilok@...il.com>
Cc:	Kwangwoo Lee <kwangwoo.lee@...il.com>, dmitry.torokhov@...il.com,
	linux-kernel@...r.kernel.org, David Brownell <david-b@...bell.net>,
	"linux-omap@...r.kernel.org Mailing List" 
	<linux-omap@...r.kernel.org>
Subject: Re: [PATCH] Add tsc2007 based touchscreen driver.

On Wed, Dec 03, 2008 at 04:51:12PM +0530, ext Trilok Soni wrote:
> Hi Lee
> 
> Adding linux-omap mailing list. Sometime I am thinking that same chip
> might have interface to be connected on another board/cpu through SPI
> too, so in this case we will have two drivers doing the same thing and
> difference is  just a change of the bus interface.
> 
> We should do something v4l2_subdev framework right now going on on
> v4l2 mailing list to abstract the sensor drivers from bus.

I've seen spi only for tsc2005.

> > /* TSC2007 Touchscreen */
> > #ifdef CONFIG_LCD_CT024TN02
> > /* GPIO13:GINT3 = PENIRQ */
> > #define TS_PENIRQ_GPIO          GPIO13
> > #define TS_CHAINED_GPIO         3
> > #else
> > /* GPIO5:GINT1 = PENIRQ */
> > #define TS_PENIRQ_GPIO          GPIO5
> > #define TS_CHAINED_GPIO         1
> > #endif

this should be using the platform_data.

> > static int mv_get_pendown_state(void)
> > {
> >        int val = 0;
> >
> >        mv_gpio_set_direction(TS_PENIRQ_GPIO, 0);
> >        mv_gpio_mux_ctrl(TS_PENIRQ_GPIO, GPIO_PIN_MODE_GPIO);

should be using gpiolib

> >        val =  mv_gpio_get_value(TS_PENIRQ_GPIO);
> >        mv_gpio_mux_ctrl(TS_PENIRQ_GPIO, GPIO_PIN_MODE_INT);
> >
> >        val = val ? 0 : 1;
> >        return val;
> > }
> >
> > static void mv_clear_penirq(void)
> > {
> >        mv_gpio_int_clear(TS_CHAINED_GPIO);
> > }
> >
> > static int mv_init_ts(void)
> > {
> >        mv_gpio_set_direction(TS_PENIRQ_GPIO, 0);       /* input */
> >        mv_gpio_mux_ctrl(TS_PENIRQ_GPIO, GPIO_PIN_MODE_INT);
> >
> >        mv_gpio_int_mode(TS_CHAINED_GPIO, GPIO_INT_FALLING_EDGE);
> >        mv_gpio_int_clear(TS_CHAINED_GPIO);
> >        mv_gpio_int_unmask(TS_CHAINED_GPIO);
> >        return 0;
> > }
> >
> > static void mv_exit_ts(void)
> > {
> >        mv_gpio_mux_ctrl(TS_PENIRQ_GPIO, GPIO_PIN_MODE_GPIO);
> >        mv_gpio_int_mask(TS_CHAINED_GPIO);
> > }
> >
> > struct tsc2007_platform_data mv_tsc2007_data = {

should be static.

> >        .model = 2007,
> >        .x_plate_ohms = 180,
> >        .get_pendown_state = mv_get_pendown_state,
> >        .clear_penirq      = mv_clear_penirq,
> >        .init_platform_hw  = mv_init_ts,
> >        .exit_platform_hw  = mv_exit_ts,
> > };
> >
> > /* I2C clients */
> > static struct i2c_board_info __initdata mv_i2c_clients[] = {
> >        [0] = {
> >                I2C_BOARD_INFO("tsc2007", 0x90),
> >                .type           = "tsc2007",
> >                .platform_data  = &mv_tsc2007_data,
> >                .irq            = IRQ_GPIO,
> >        },
> > };
> >
> > i2c_register_board_info() should be used.
> >
> > Signed-off-by: Kwangwoo Lee <kwangwoo.lee@...il.com>
> > ---
> >  drivers/input/touchscreen/Kconfig   |    9 +
> >  drivers/input/touchscreen/Makefile  |    1 +
> >  drivers/input/touchscreen/tsc2007.c |  398 +++++++++++++++++++++++++++++++++++
> >  include/linux/i2c/tsc2007.h         |   17 ++
> >  4 files changed, 425 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/input/touchscreen/tsc2007.c
> >  create mode 100644 include/linux/i2c/tsc2007.h
> >
> > diff --git a/drivers/input/touchscreen/Kconfig
> > b/drivers/input/touchscreen/Kconfig
> > index 3d1ab8f..78abdfb 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -376,4 +376,13 @@ config TOUCHSCREEN_TOUCHIT213
> >          To compile this driver as a module, choose M here: the
> >          module will be called touchit213.
> >
> > +config TOUCHSCREEN_TSC2007
> > +       tristate "TSC2007 based touchscreens"
> > +       depends on I2C
> > +       help
> > +         Say Y here if you have a TSC2007 based touchscreens.
> > +
> > +         To compile this driver as a module, choose M here: the
> > +         module will be called tsc2007.
> > +
> >  endif
> > diff --git a/drivers/input/touchscreen/Makefile
> > b/drivers/input/touchscreen/Makefile
> > index 15cf290..824999c 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -31,3 +31,4 @@ wm97xx-ts-$(CONFIG_TOUCHSCREEN_WM9705)        += wm9705.o
> >  wm97xx-ts-$(CONFIG_TOUCHSCREEN_WM9712) += wm9712.o
> >  wm97xx-ts-$(CONFIG_TOUCHSCREEN_WM9713) += wm9713.o
> >  obj-$(CONFIG_TOUCHSCREEN_WM97XX_MAINSTONE)     += mainstone-wm97xx.o
> > +obj-$(CONFIG_TOUCHSCREEN_TSC2007)      += tsc2007.o
> > diff --git a/drivers/input/touchscreen/tsc2007.c
> > b/drivers/input/touchscreen/tsc2007.c
> > new file mode 100644
> > index 0000000..85fb520
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/tsc2007.c
> > @@ -0,0 +1,398 @@
> > +/*
> > + * drivers/input/touchscreen/tsc2007.c
> > + *
> > + * Copyright (c) 2008 MtekVision Co., Ltd.
> > + *     Kwangwoo Lee <kwlee@...kvision.com>
> > + *
> > + * Using code from:
> > + *  - ads7846.c
> > + *     Copyright (c) 2005 David Brownell
> > + *     Copyright (c) 2006 Nokia Corporation
> > + *  - corgi_ts.c
> > + *     Copyright (C) 2004-2005 Richard Purdie
> > + *  - omap_ts.[hc], ads7846.h, ts_osk.c
> > + *     Copyright (C) 2002 MontaVista Software
> > + *     Copyright (C) 2004 Texas Instruments
> > + *     Copyright (C) 2005 Dirk Behme
> > + *
> > + *  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/hwmon.h>
> > +#include <linux/init.h>
> > +#include <linux/err.h>
> > +#include <linux/delay.h>
> > +#include <linux/input.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/i2c/tsc2007.h>
> > +#include <linux/io.h>
> > +
> > +#include <asm/irq.h>
> > +
> > +#define TS_POLL_DELAY  (10 * 1000)     /* ns delay before the first sample */
> > +#define TS_POLL_PERIOD (5 * 1000)      /* ns delay between samples */
> > +
> > +#define TSC2007_MEASURE_TEMP0          (0x0 << 4)
> > +#define TSC2007_MEASURE_AUX            (0x2 << 4)
> > +#define TSC2007_MEASURE_TEMP1          (0x4 << 4)
> > +#define TSC2007_ACTIVATE_XN            (0x8 << 4)
> > +#define TSC2007_ACTIVATE_YN            (0x9 << 4)
> > +#define TSC2007_ACTIVATE_YP_XN         (0xa << 4)
> > +#define TSC2007_SETUP                  (0xb << 4)
> > +#define TSC2007_MEASURE_X              (0xc << 4)
> > +#define TSC2007_MEASURE_Y              (0xd << 4)
> > +#define TSC2007_MEASURE_Z1             (0xe << 4)
> > +#define TSC2007_MEASURE_Z2             (0xf << 4)
> > +
> > +#define TSC2007_POWER_OFF_IRQ_EN       (0x0 << 2)
> > +#define TSC2007_ADC_ON_IRQ_DIS0                (0x1 << 2)
> > +#define TSC2007_ADC_OFF_IRQ_EN         (0x2 << 2)
> > +#define TSC2007_ADC_ON_IRQ_DIS1                (0x3 << 2)
> > +
> > +#define TSC2007_12BIT                  (0x0 << 1)
> > +#define TSC2007_8BIT                   (0x1 << 1)
> > +
> > +#define        MAX_12BIT                       ((1 << 12) - 1)
> > +
> > +#define ADC_ON_12BIT   (TSC2007_12BIT | TSC2007_ADC_ON_IRQ_DIS0)
> > +
> > +#define READ_Y         (ADC_ON_12BIT | TSC2007_MEASURE_Y)
> > +#define READ_Z1                (ADC_ON_12BIT | TSC2007_MEASURE_Z1)
> > +#define READ_Z2                (ADC_ON_12BIT | TSC2007_MEASURE_Z2)
> > +#define READ_X         (ADC_ON_12BIT | TSC2007_MEASURE_X)
> > +#define PWRDOWN                (TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
> > +
> > +struct ts_event {
> > +       u16     x;
> > +       u16     y;
> > +       u16     z1, z2;
> > +};
> > +
> > +struct tsc2007 {
> > +       struct input_dev        *input;
> > +       char                    phys[32];
> > +       struct hrtimer          timer;
> > +       struct ts_event         tc;
> > +
> > +       struct i2c_client       *client;
> > +
> > +       spinlock_t              lock;
> > +
> > +       u16                     model;
> > +       u16                     x_plate_ohms;
> > +
> > +       unsigned                pendown;
> > +       int                     irq;
> > +
> > +       int                     (*get_pendown_state)(void);
> > +       void                    (*clear_penirq)(void);
> > +};
> > +
> > +static int tsc2007_xfer(void *tsc, unsigned char cmd)
> > +{
> > +       struct tsc2007    *ts     = tsc;
> > +       struct i2c_client *client = ts->client;
> > +
> > +       unsigned char  rbuf[2];
> > +       unsigned short val;
> > +       int result;
> > +
> > +       result = i2c_master_send(client, &cmd, 1);
> > +       if (result != 1) {
> > +               dev_err(&client->dev, "send failed, cmd 0x%x\n", cmd);
> > +               goto cmd_fail;
> > +       }
> > +
> > +       result = i2c_master_recv(client, rbuf, 2);
> > +       if (result != 2) {
> > +               dev_err(&client->dev, "recv failed, cmd 0x%x\n", cmd);
> > +               goto cmd_fail;
> > +       }
> > +
> > +       rbuf[1] = (rbuf[1] >> 4) | ((rbuf[0] & 0x0f) << 4);
> > +       rbuf[0] = rbuf[0] >> 4;
> > +
> > +       val = *((unsigned short *) rbuf);
> > +       val = be16_to_cpu(val);
> > +
> > +       dev_dbg(&client->dev, "cmd [0x%x], rbuf [0x%x, 0x%x] => val [%u]\n",
> > +               cmd, rbuf[0], rbuf[1], val);
> > +
> > +       return (int) val;
> > +
> > +cmd_fail:
> > +       return -EIO;
> > +}

so you really need to go that low level ?? Did you try
i2c_smbus_write_*() ??

> > +static void tsc2007_send_event(void *tsc)
> > +{
> > +       struct tsc2007  *ts = tsc;
> > +       u32             Rt;
> > +       u16             x, y, z1, z2;
> > +
> > +       x = ts->tc.x;
> > +       y = ts->tc.y;
> > +       z1 = ts->tc.z1;
> > +       z2 = ts->tc.z2;
> > +
> > +       /* range filtering */
> > +       if (x == MAX_12BIT)
> > +               x = 0;
> > +
> > +       if (likely(x && z1)) {
> > +               /* compute touch pressure resistance using equation #1 */
> > +               Rt = z2;
> > +               Rt -= z1;
> > +               Rt *= x;
> > +               Rt *= ts->x_plate_ohms;
> > +               Rt /= z1;
> > +               Rt = (Rt + 2047) >> 12;
> > +       } else
> > +               Rt = 0;
> > +
> > +       /* Sample found inconsistent by debouncing or pressure is beyond
> > +        * the maximum. Don't report it to user space, repeat at least
> > +        * once more the measurement
> > +        */
> > +       if (Rt > MAX_12BIT) {
> > +               dev_dbg(&ts->client->dev, "ignored pressure %d\n", Rt);
> > +
> > +               hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
> > +                             HRTIMER_MODE_REL);
> > +               return;
> > +       }
> > +
> > +       /* NOTE: We can't rely on the pressure to determine the pen down
> > +        * state, even this controller has a pressure sensor.  The pressure
> > +        * value can fluctuate for quite a while after lifting the pen and
> > +        * in some cases may not even settle at the expected value.
> > +        *
> > +        * The only safe way to check for the pen up condition is in the
> > +        * timer by reading the pen signal state (it's a GPIO _and_ IRQ).
> > +        */
> > +       if (Rt) {
> > +               struct input_dev *input = ts->input;
> > +
> > +               if (!ts->pendown) {
> > +                       dev_dbg(&ts->client->dev, "DOWN\n");
> > +
> > +                       input_report_key(input, BTN_TOUCH, 1);
> > +                       ts->pendown = 1;
> > +               }
> > +
> > +               input_report_abs(input, ABS_X, x);
> > +               input_report_abs(input, ABS_Y, y);
> > +               input_report_abs(input, ABS_PRESSURE, Rt);
> > +
> > +               input_sync(input);
> > +
> > +               dev_dbg(&ts->client->dev, "point(%4d,%4d), pressure (%4u)\n",
> > +                       x, y, Rt);
> > +       }
> > +
> > +       hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
> > +                       HRTIMER_MODE_REL);
> > +}
> > +
> > +static int tsc2007_read_values(struct tsc2007 *tsc)
> > +{
> > +       struct tsc2007  *ts = tsc;
> > +
> > +       /* y- still on; turn on only y+ (and ADC) */
> > +       ts->tc.y = tsc2007_xfer(ts, READ_Y);
> > +
> > +       /* turn y- off, x+ on, then leave in lowpower */
> > +       ts->tc.x = tsc2007_xfer(ts, READ_X);
> > +
> > +       /* turn y+ off, x- on; we'll use formula #1 */
> > +       ts->tc.z1 = tsc2007_xfer(ts, READ_Z1);
> > +       ts->tc.z2 = tsc2007_xfer(ts, READ_Z2);
> > +
> > +       /* power down */
> > +       tsc2007_xfer(ts, PWRDOWN);
> > +       return 0;
> > +}
> > +
> > +static enum hrtimer_restart tsc2007_timer(struct hrtimer *handle)
> > +{
> > +       struct tsc2007  *ts = container_of(handle, struct tsc2007, timer);
> > +
> > +       spin_lock_irq(&ts->lock);
> > +
> > +       if (unlikely(!ts->get_pendown_state() && ts->pendown)) {
> > +               struct input_dev *input = ts->input;
> > +
> > +               dev_dbg(&ts->client->dev, "UP\n");
> > +
> > +               input_report_key(input, BTN_TOUCH, 0);
> > +               input_report_abs(input, ABS_PRESSURE, 0);
> > +               input_sync(input);
> > +
> > +               ts->pendown = 0;
> > +               enable_irq(ts->irq);
> > +       } else {
> > +               /* pen is still down, continue with the measurement */
> > +               dev_dbg(&ts->client->dev, "pen is still down\n");
> > +
> > +               tsc2007_read_values(ts);
> > +               tsc2007_send_event(ts);
> > +       }
> > +
> > +       spin_unlock_irq(&ts->lock);
> > +       return HRTIMER_NORESTART;
> > +}
> > +
> > +static irqreturn_t tsc2007_irq(int irq, void *handle)
> > +{
> > +       struct tsc2007 *ts = handle;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&ts->lock, flags);
> > +       if (likely(ts->get_pendown_state())) {
> > +               disable_irq(ts->irq);
> > +               hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY),
> > +                                       HRTIMER_MODE_REL);
> > +       }
> > +
> > +       if (ts->clear_penirq)
> > +               ts->clear_penirq();
> > +
> > +       spin_unlock_irqrestore(&ts->lock, flags);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +static int tsc2007_probe(struct i2c_client *client,
> > +                       const struct i2c_device_id *id)
> > +{
> > +       struct i2c_adapter      *adapter = to_i2c_adapter(client->dev.parent);

why ??

> > +       struct tsc2007          *ts;
> > +       struct tsc2007_platform_data *pdata;
> > +       struct input_dev        *input_dev;
> > +       int                     err;
> > +
> > +       pdata = client->dev.platform_data;
> > +       if (pdata == NULL) {
> > +               dev_err(&client->dev, "platform data is required!\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       if (!i2c_check_functionality(adapter,
> > +                       I2C_FUNC_I2C | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
> > +               return -EIO;

should not be in this driver, I'd say.

> > +
> > +       ts = kzalloc(sizeof(struct tsc2007), GFP_KERNEL);
> > +       input_dev = input_allocate_device();
> > +       if (!ts || !input_dev) {
> > +               err = -ENOMEM;
> > +               goto err_free_mem;
> > +       }
> > +
> > +       ts->client = client;
> > +       i2c_set_clientdata(client, ts);
> > +
> > +       ts->input = input_dev;
> > +
> > +       hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +       ts->timer.function = tsc2007_timer;
> > +
> > +       spin_lock_init(&ts->lock);
> > +
> > +       ts->model             = pdata->model;
> > +       ts->x_plate_ohms      = pdata->x_plate_ohms;
> > +       ts->get_pendown_state = pdata->get_pendown_state;
> > +       ts->clear_penirq      = pdata->clear_penirq;
> > +
> > +       pdata->init_platform_hw();
> > +
> > +       snprintf(ts->phys, sizeof(ts->phys), "%s/input0", client->dev.bus_id);
> > +
> > +       input_dev->name = "TSC2007 Touchscreen";
> > +       input_dev->phys = ts->phys;
> > +
> > +       input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> > +       input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> > +
> > +       input_set_abs_params(input_dev, ABS_X, 0, MAX_12BIT, 0, 0);
> > +       input_set_abs_params(input_dev, ABS_Y, 0, MAX_12BIT, 0, 0);
> > +       input_set_abs_params(input_dev, ABS_PRESSURE, 0, MAX_12BIT, 0, 0);
> > +
> > +       tsc2007_read_values(ts);
> > +
> > +       ts->irq = client->irq;
> > +       if (request_irq(ts->irq, tsc2007_irq, 0,
> > +                       client->dev.driver->name, ts)) {
> > +               dev_err(&client->dev, "irq %d busy?\n", ts->irq);
> > +               err = -EBUSY;
> > +               goto err_free_mem;

would be better to:

err = request_irq(...)
if (err < 0) {
	dev_err(..)
	...
}

Then we see the error request_irq() returned, which is more useful.

> > +       }
> > +
> > +       err = input_register_device(input_dev);
> > +       if (err)
> > +               goto err_free_irq;
> > +
> > +       dev_info(&client->dev, "registered with irq (%d)\n", ts->irq);
> > +
> > +       return 0;
> > +
> > + err_free_irq:
> > +       free_irq(ts->irq, ts);
> > + err_free_mem:
> > +       input_free_device(input_dev);
> > +       kfree(ts);
> > +       return err;
> > +}
> > +
> > +static int tsc2007_remove(struct i2c_client *client)
> > +{
> > +       struct tsc2007  *ts = i2c_get_clientdata(client);
> > +       struct tsc2007_platform_data *pdata;
> > +
> > +       pdata = client->dev.platform_data;
> > +       pdata->exit_platform_hw();
> > +
> > +       input_unregister_device(ts->input);
> > +       free_irq(ts->irq, ts);
> > +       kfree(ts);
> > +
> > +       dev_info(&client->dev, "unregistered\n");
> > +       return 0;
> > +}
> > +
> > +static struct i2c_device_id tsc2007_idtable[] = {
> > +       { "tsc2007", 0 },
> > +       { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, tsc2007_idtable);
> > +
> > +static struct i2c_driver tsc2007_driver = {
> > +       .driver         = {
> > +               .owner          = THIS_MODULE,
> > +               .name           = "tsc2007"
> > +       },
> > +       .id_table       = tsc2007_idtable,
> > +       .probe          = tsc2007_probe,
> > +       .remove         = tsc2007_remove,
> > +};
> > +
> > +static int __init tsc2007_init(void)
> > +{
> > +       return i2c_add_driver(&tsc2007_driver);
> > +}
> > +
> > +static void __exit tsc2007_exit(void)
> > +{
> > +       i2c_del_driver(&tsc2007_driver);
> > +}
> > +
> > +module_init(tsc2007_init);
> > +module_exit(tsc2007_exit);
> > +
> > +MODULE_AUTHOR("Kwangwoo Lee <kwlee@...kvision.com>");
> > +MODULE_DESCRIPTION("TSC2007 TouchScreen Driver");
> > +MODULE_LICENSE("GPL");

Would be nice to get comments from Jean Delvare and Ben Dooks as well.

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ