Re: [PATCH v2 1/2] input: touchscreen: introduce tsc2005 driver

[Phil Carmody]

Many thanks for your reply, Dmitry. Some comments follow yours.

On 14/11/09 07:03 +0100, ext Dmitry Torokhov wrote:
> Hi Aaro,
> 
> On Wed, Nov 04, 2009 at 03:23:01PM +0200, Aaro Koskinen wrote:
> > +
> > +static void tsc2005_ts_update_pen_state(struct tsc2005 *ts,
> > +                                     int x, int y, int pressure)
> > +{
> > +     if (pressure) {
> > +             input_report_abs(ts->idev, ABS_X, x);
> > +             input_report_abs(ts->idev, ABS_Y, y);
> > +             input_report_abs(ts->idev, ABS_PRESSURE, pressure);
> > +             if (!ts->pen_down) {
> > +                     input_report_key(ts->idev, BTN_TOUCH, 1);
> > +                     ts->pen_down = 1;
> > +             }
> 
> Just report BTN_TOUCH always, input core will filter duplicates.

Because there are several places where a check will be made whether
we need to do anything or not. We'll grab a spinlock, decide to add 
no entropy, decide to toggle no status bits, and decide to not do 
anything more after that. Sounds like waste.

> > +     } else {
> > +             input_report_abs(ts->idev, ABS_PRESSURE, 0);
> > +             if (ts->pen_down) {
> > +                     input_report_key(ts->idev, BTN_TOUCH, 0);
> > +                     ts->pen_down = 0;
> > +             }
> > +     }
> > +
> > +     input_sync(ts->idev);
> > +}
> > +
> > +/*
> > + * This function is called by the SPI framework after the coordinates
> > + * have been read from TSC2005
> > + */
> > +static void tsc2005_ts_rx(void *arg)
> > +{
> > +     struct tsc2005 *ts = arg;
> > +     unsigned long flags;
> > +     int inside_rect, pressure_limit;
> > +     int x, y, z1, z2, pressure;
> > +
> > +     spin_lock_irqsave(&ts->lock, flags);
> > +
> > +     if (ts->disable_depth) {
> > +             ts->spi_pending = 0;
> > +             goto out;
> > +     }
> > +
> > +     x = ts->data[0];
> > +     y = ts->data[1];
> > +     z1 = ts->data[2];
> > +     z2 = ts->data[3];
> > +
> > +     /* validate pressure and position */
> > +     if (x > MAX_12BIT || y > MAX_12BIT)
> > +             goto out;
> > +
> > +     /* skip coords if the pressure-components are out of range */
> > +     if (z1 < 100 || z2 > MAX_12BIT || z1 >= z2)
> > +             goto out;
> > +
> > +     /* skip point if this is a pen down with the exact same values as
> > +      * the value before pen-up - that implies SPI fed us stale data
> > +      */
> > +     if (!ts->pen_down &&
> > +         ts->in_x == x &&
> > +         ts->in_y == y &&
> > +         ts->in_z1 == z1 &&
> > +         ts->in_z2 == z2)
> > +             goto out;
> > +
> > +     /* At this point we are happy we have a valid and useful reading.
> > +      * Remember it for later comparisons. We may now begin downsampling
> > +      */
> > +     ts->in_x = x;
> > +     ts->in_y = y;
> > +     ts->in_z1 = z1;
> > +     ts->in_z2 = z2;
> > +
> > +     /* don't run average on the "pen down" event */
> > +     if (ts->sample_sent) {
> > +             ts->avg_x += x;
> > +             ts->avg_y += y;
> > +             ts->avg_z1 += z1;
> > +             ts->avg_z2 += z2;
> > +
> > +             if (++ts->sample_cnt < TS_SAMPLES)
> > +                     goto out;
> > +
> > +             x = ts->avg_x / TS_SAMPLES;
> > +             y = ts->avg_y / TS_SAMPLES;
> > +             z1 = ts->avg_z1 / TS_SAMPLES;
> > +             z2 = ts->avg_z2 / TS_SAMPLES;
> > +     }
> >
> 
> Do we really need to do filtering and averaging in kernel? What about
> tslib?

Not everyone uses tslib. There's already some filtering in the tsc2005 
already, but it's not really enough. This averaging also has the
pleasant side-effect of reducing the number of input events. The
fudging that takes place later has slightly different smoothing
behaviour, and isn't a substitute.

> > +     ts->sample_cnt = 0;
> > +     ts->avg_x = 0;
> > +     ts->avg_y = 0;
> > +     ts->avg_z1 = 0;
> > +     ts->avg_z2 = 0;
> > +
> > +     pressure = x * (z2 - z1) / z1;
> > +     pressure = pressure * ts->x_plate_ohm / 4096;
> > +
> > +     pressure_limit = ts->sample_sent ? ts->p_max : ts->touch_pressure;
> > +     if (pressure > pressure_limit)
> > +             goto out;
> > +
> > +     /* Discard the event if it still is within the previous rect -
> > +      * unless the pressure is clearly harder, but then use previous
> > +      * x,y position. If any coordinate deviates enough, fudging
> > +      * of all three will still take place in the input layer.
> > +      */
> > +     inside_rect = (ts->sample_sent &&
> > +             x > (int)ts->out_x - ts->fudge_x &&
> > +             x < (int)ts->out_x + ts->fudge_x &&
> > +             y > (int)ts->out_y - ts->fudge_y &&
> > +             y < (int)ts->out_y + ts->fudge_y);
> > +     if (inside_rect)
> > +             x = ts->out_x, y = ts->out_y;
> > +
> > +     if (!inside_rect || pressure < (ts->out_p - ts->fudge_p)) {
> > +             tsc2005_ts_update_pen_state(ts, x, y, pressure);
> > +             ts->sample_sent = 1;
> > +             ts->out_x = x;
> > +             ts->out_y = y;
> > +             ts->out_p = pressure;
> > +     }
> > +out:
> > +     if (ts->spi_pending > 1) {
> > +             /* One or more interrupts (sometimes several dozens)
> > +              * occured while waiting for the SPI read - get
> > +              * another read going.
> > +              */
> > +             ts->spi_pending = 1;
> > +             if (spi_async(ts->spi, &ts->read_msg)) {
> > +                     dev_err(&ts->spi->dev, "ts: spi_async() failed");
> > +                     ts->spi_pending = 0;
> > +             }
> > +     } else
> > +             ts->spi_pending = 0;
> > +
> > +     /* kick pen up timer - to make sure it expires again(!) */
> > +     if (ts->sample_sent) {
> > +             mod_timer(&ts->penup_timer,
> > +                       jiffies + msecs_to_jiffies(TSC2005_TS_PENUP_TIME));
> > +             /* Also kick the watchdog, as we still think we're alive */
> > +             if (ts->esd_timeout && ts->disable_depth == 0) {
> > +                     unsigned long wdj = msecs_to_jiffies(ts->esd_timeout);
> > +                     mod_timer(&ts->esd_timer, round_jiffies(jiffies+wdj));
> > +             }
> > +     }
> > +     spin_unlock_irqrestore(&ts->lock, flags);
> > +}
> > +
> > +/* This penup timer is very forgiving of delayed SPI reads. The
> > + * (ESD) watchdog will rescue us if spi_pending remains set, unless
> > + * we are enterring the disabled state. In that case we must just
> > + * handle the pen up, and let disabling complete.
> > + */
> > +static void tsc2005_ts_penup_timer_handler(unsigned long data)
> > +{
> > +     struct tsc2005 *ts = (struct tsc2005 *)data;
> > +     if ((!ts->spi_pending || ts->disable_depth) &&
> > +         ts->sample_sent) {
> > +             tsc2005_ts_update_pen_state(ts, 0, 0, 0);
> > +             ts->sample_sent = 0;
> > +     }
> > +}
> > +
> > +/*
> > + * This interrupt is called when pen is down and coordinates are
> > + * available. That is indicated by a either:
> > + * a) a rising edge on PINTDAV or (PENDAV mode)
> > + * b) a falling edge on DAV line (DAV mode)
> > + * depending on the setting of the IRQ bits in the CFR2 setting above.
> > + */
> > +static irqreturn_t tsc2005_ts_irq_handler(int irq, void *dev_id)
> > +{
> > +     struct tsc2005 *ts = dev_id;
> > +     if (ts->disable_depth)
> > +             goto out;
> > +
> > +     if (!ts->spi_pending) {
> > +             if (spi_async(ts->spi, &ts->read_msg)) {
> > +                     dev_err(&ts->spi->dev, "ts: spi_async() failed");
> > +                     goto out;
> > +             }
> > +     }
> > +     /* By shifting in 1s we can never wrap */
> > +     ts->spi_pending = (ts->spi_pending<<1)+1;
> > +
> > +     /* Kick pen up timer only if it's not been started yet. Strictly,
> > +      * it isn't even necessary to start it at all here,  but doing so
> > +      * keeps an equivalence between pen state and timer state.
> > +      * The SPI read loop will keep pushing it into the future.
> > +      * If it times out with an SPI pending, it's ignored anyway.
> > +      */
> > +     if (!timer_pending(&ts->penup_timer)) {
> > +             unsigned long pu = msecs_to_jiffies(TSC2005_TS_PENUP_TIME);
> > +             ts->penup_timer.expires = jiffies + pu;
> > +             add_timer(&ts->penup_timer);
> > +     }
> > +out:
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static void tsc2005_ts_setup_spi_xfer(struct tsc2005 *ts)
> > +{
> > +     struct spi_message *m = &ts->read_msg;
> > +     struct spi_transfer *x = &ts->read_xfer[0];
> > +     int i;
> > +
> > +     spi_message_init(m);
> > +
> > +     for (i = 0; i < NUM_READ_REGS; i++, x++) {
> > +             x->tx_buf = &tsc2005_read_reg[i];
> > +             x->rx_buf = &ts->data[i];
> > +             x->len = 4;
> > +             x->bits_per_word = 24;
> > +             x->cs_change = i < (NUM_READ_REGS - 1);
> > +             spi_message_add_tail(x, m);
> > +     }
> > +
> > +     m->complete = tsc2005_ts_rx;
> > +     m->context = ts;
> > +}
> > +
> > +static ssize_t tsc2005_ts_pen_down_show(struct device *dev,
> > +                                     struct device_attribute *attr,
> > +                                     char *buf)
> > +{
> > +     struct tsc2005 *ts = dev_get_drvdata(dev);
> > +
> > +     return sprintf(buf, "%u\n", ts->pen_down);
> > +}
> > +
> > +static DEVICE_ATTR(pen_down, S_IRUGO, tsc2005_ts_pen_down_show, NULL);
> 
> What is the point of exporting this through sysfs?

It's a quick and simple way for any user-space app to know if the
screen is currently being touched without having to subscribe to the
stream of input events. OK, that wasn't a great reason, I admit.

> > +static int tsc2005_configure(struct tsc2005 *ts, int flags)
> > +{
> > +     tsc2005_write(ts, TSC2005_REG_CFR0, TSC2005_CFR0_INITVALUE);
> > +     tsc2005_write(ts, TSC2005_REG_CFR1, TSC2005_CFR1_INITVALUE);
> > +     tsc2005_write(ts, TSC2005_REG_CFR2, TSC2005_CFR2_INITVALUE);
> > +     tsc2005_cmd(ts, flags);
> > +
> > +     return 0;
> > +}
> > +
> > +static void tsc2005_start_scan(struct tsc2005 *ts)
> > +{
> > +     tsc2005_configure(ts, TSC2005_CMD_SCAN_XYZZ);
> > +}
> > +
> > +static void tsc2005_stop_scan(struct tsc2005 *ts)
> > +{
> > +     tsc2005_cmd(ts, TSC2005_CMD_STOP);
> > +}
> > +
> > +/* Must be called with mutex held */
> > +static void tsc2005_disable(struct tsc2005 *ts)
> > +{
> > +     if (ts->disable_depth++ != 0)
> > +             return;
> > +
> > +     disable_irq(ts->spi->irq);
> > +     if (ts->esd_timeout)
> > +             del_timer(&ts->esd_timer);
> 
> del_timer_sync()?

Yup.

> > +
> > +     /* wait until penup timer expire normally */
> > +     do {
> > +             msleep(4);
> > +     } while (ts->sample_sent);
> > +
> > +     tsc2005_stop_scan(ts);
> > +}
> > +
> > +static void tsc2005_enable(struct tsc2005 *ts)
> > +{
> > +     if (ts->disable_depth != 1)
> > +             goto out;
> > +
> > +     if (ts->esd_timeout) {
> > +             unsigned long wdj = msecs_to_jiffies(ts->esd_timeout);
> > +             ts->esd_timer.expires = round_jiffies(jiffies+wdj);
> > +             add_timer(&ts->esd_timer);
> 
> just use mod_timer().

There are, or at least should be, no situations where the timer can be 
active at that point. So mod_timer()'s not necessary. My preference is
for add_timer(), but I won't cling to that religiously.

> > +     }
> > +     tsc2005_start_scan(ts);
> > +     enable_irq(ts->spi->irq);
> > +out:
> > +     --ts->disable_depth;
> > +}
> > +
> > +static ssize_t tsc2005_disable_show(struct device *dev,
> > +                                 struct device_attribute *attr, char *buf)
> > +{
> > +     struct tsc2005 *ts = dev_get_drvdata(dev);
> > +
> > +     return sprintf(buf, "%u\n", ts->disabled);
> > +}
> > +
> > +static ssize_t tsc2005_disable_store(struct device *dev,
> > +                                  struct device_attribute *attr,
> > +                                  const char *buf, size_t count)
> > +{
> > +     struct tsc2005          *ts = dev_get_drvdata(dev);
> > +     unsigned long res;
> > +     int i;
> > +
> > +     if (strict_strtoul(buf, 10, &res) < 0)
> > +             return -EINVAL;
> > +     i = res ? 1 : 0;
> > +
> > +     mutex_lock(&ts->mutex);
> > +     if (i == ts->disabled)
> > +             goto out;
> > +     ts->disabled = i;
> > +
> > +     if (i)
> > +             tsc2005_disable(ts);
> > +     else
> > +             tsc2005_enable(ts);
> > +out:
> > +     mutex_unlock(&ts->mutex);
> > +     return count;
> > +}
> > +
> > +static DEVICE_ATTR(disable_ts, 0664, tsc2005_disable_show,
> > +                tsc2005_disable_store);
> > +
> > +static ssize_t tsc2005_ctrl_selftest_show(struct device *dev,
> > +                                       struct device_attribute *attr,
> > +                                       char *buf)
> > +{
> > +     u16 temp_high_orig, temp_high_test, temp_high;
> > +     unsigned int result = 1;
> > +     struct tsc2005 *ts = dev_get_drvdata(dev);
> > +
> > +     if (!ts->set_reset) {
> > +             dev_warn(&ts->spi->dev,
> > +                      "unable to selftest: reset not configured\n");
> > +             result = 0;
> > +             goto out;
> > +     }
> > +
> > +     mutex_lock(&ts->mutex);
> > +     tsc2005_disable(ts);
> > +
> > +     /* Test ctrl communications via temp high / low registers */
> > +     tsc2005_read(ts, TSC2005_REG_TEMP_HIGH, &temp_high_orig);
> > +
> > +     temp_high_test = (temp_high_orig - 1) & 0x0FFF;
> > +
> > +     tsc2005_write(ts, TSC2005_REG_TEMP_HIGH, temp_high_test);
> > +
> > +     tsc2005_read(ts, TSC2005_REG_TEMP_HIGH, &temp_high);
> > +
> > +     if (temp_high != temp_high_test) {
> > +             result = 0;
> > +             dev_warn(dev, "selftest failed: %d != %d\n",
> > +                      temp_high, temp_high_test);
> > +     }
> > +
> > +     /* HW Reset */
> > +     ts->set_reset(0);
> > +     msleep(1); /* only 10us required */
> > +     ts->set_reset(1);
> > +
> > +     tsc2005_enable(ts);
> > +
> > +     /* Test that reset really happened */
> > +     tsc2005_read(ts, TSC2005_REG_TEMP_HIGH, &temp_high);
> > +
> > +     if (temp_high != temp_high_orig) {
> > +             result = 0;
> > +             dev_warn(dev, "selftest failed after reset: "
> > +                      "%d != %d\n",
> > +                      temp_high, temp_high_orig);
> > +     }
> > +
> > +     mutex_unlock(&ts->mutex);
> > +
> > +out:
> > +     return sprintf(buf, "%u\n", result);
> > +}
> > +
> > +static DEVICE_ATTR(ts_ctrl_selftest, S_IRUGO, tsc2005_ctrl_selftest_show, NULL);
> > +
> > +static void tsc2005_esd_timer_handler(unsigned long data)
> > +{
> > +     struct tsc2005 *ts = (struct tsc2005 *)data;
> > +     if (!ts->disable_depth)
> > +             schedule_work(&ts->esd_work);
> > +}
> > +
> > +static void tsc2005_rst_handler(struct work_struct *work)
> > +{
> > +     u16 reg_val;
> > +     struct tsc2005 *ts = container_of(work, struct tsc2005, esd_work);
> > +     unsigned long wdj;
> > +
> > +     mutex_lock(&ts->mutex);
> > +
> > +     /* If we are disabled, or the a touch has been detected,
> > +      * then ignore this timeout. The enable will restart the
> > +      * watchdog, as it restarts scanning
> > +      */
> > +     if (ts->disable_depth)
> > +             goto out;
> > +
> > +     /* If we cannot read our known value from configuration register 0
> > +      * then reset the controller as if from power-up and start
> > +      * scanning again. Always re-arm the watchdog.
> > +      */
> > +     tsc2005_read(ts, TSC2005_REG_CFR0, &reg_val);
> > +     if ((reg_val ^ TSC2005_CFR0_INITVALUE) & TSC2005_CFR0_RW_MASK) {
> > +             dev_info(&ts->spi->dev, "TSC not responding, resetting.\n");
> > +             /* If this timer kicked in, the penup timer, if ever active
> > +              * at all, must have expired ages ago, so no need to del it.
> > +              */
> > +             ts->set_reset(0);
> > +             if (ts->sample_sent) {
> > +                     tsc2005_ts_update_pen_state(ts, 0, 0, 0);
> > +                     ts->sample_sent = 0;
> > +             }
> > +             ts->spi_pending = 0;
> > +             msleep(1); /* only 10us required */
> > +             ts->set_reset(1);
> > +             tsc2005_start_scan(ts);
> > +     }
> > +     wdj = msecs_to_jiffies(ts->esd_timeout);
> > +     mod_timer(&ts->esd_timer, round_jiffies(jiffies+wdj));
> > +
> > +out:
> > +     mutex_unlock(&ts->mutex);
> > +}
> > +
> > +static int __devinit tsc2005_ts_init(struct tsc2005 *ts,
> > +                                  struct tsc2005_platform_data *pdata)
> > +{
> > +     struct input_dev *idev;
> > +     int r;
> > +     int x_max, y_max;
> > +
> > +     init_timer(&ts->penup_timer);
> > +     setup_timer(&ts->penup_timer, tsc2005_ts_penup_timer_handler,
> > +                     (unsigned long)ts);
> > +
> > +     spin_lock_init(&ts->lock);
> > +     mutex_init(&ts->mutex);
> > +
> > +     ts->x_plate_ohm         = pdata->ts_x_plate_ohm ? : 280;
> > +     ts->hw_avg_max          = pdata->ts_hw_avg;
> > +     ts->stab_time           = pdata->ts_stab_time;
> > +     x_max                   = pdata->ts_x_max ? : 4096;
> > +     ts->fudge_x             = pdata->ts_x_fudge ? : 4;
> > +     y_max                   = pdata->ts_y_max ? : 4096;
> > +     ts->fudge_y             = pdata->ts_y_fudge ? : 8;
> > +     ts->p_max               = pdata->ts_pressure_max ? : MAX_12BIT;
> > +     ts->touch_pressure      = pdata->ts_touch_pressure ? : ts->p_max;
> > +     ts->fudge_p             = pdata->ts_pressure_fudge ? : 2;
> > +
> > +     ts->set_reset           = pdata->set_reset;
> > +
> > +     idev = input_allocate_device();
> > +     if (idev == NULL) {
> > +             r = -ENOMEM;
> > +             goto err1;
> > +     }
> > +
> > +     idev->name = "TSC2005 touchscreen";
> > +     snprintf(ts->phys, sizeof(ts->phys), "%s/input-ts",
> > +              dev_name(&ts->spi->dev));
> > +     idev->phys = ts->phys;
> > +
> > +     idev->evbit[0] = BIT(EV_ABS) | BIT(EV_KEY);
> > +     idev->absbit[0] = BIT(ABS_X) | BIT(ABS_Y) | BIT(ABS_PRESSURE);
> > +     idev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> > +     ts->idev = idev;
> > +
> > +     tsc2005_ts_setup_spi_xfer(ts);
> > +
> > +     input_set_abs_params(idev, ABS_X, 0, x_max, ts->fudge_x, 0);
> > +     input_set_abs_params(idev, ABS_Y, 0, y_max, ts->fudge_y, 0);
> > +     input_set_abs_params(idev, ABS_PRESSURE, 0, ts->p_max, ts->fudge_p, 0);
> > +
> > +     tsc2005_start_scan(ts);
> > +
> > +     r = request_irq(ts->spi->irq, tsc2005_ts_irq_handler,
> > +                     (((TSC2005_CFR2_INITVALUE & TSC2005_CFR2_IRQ_MASK) ==
> > +                       TSC2005_CFR2_IRQ_PENDAV)
> > +                      ? IRQF_TRIGGER_RISING
> > +                      : IRQF_TRIGGER_FALLING) |
> > +                     IRQF_DISABLED, "tsc2005", ts);
> > +     if (r < 0) {
> > +             dev_err(&ts->spi->dev, "unable to get DAV IRQ");
> > +             goto err2;
> > +     }
> > +
> > +     set_irq_wake(ts->spi->irq, 1);
> > +
> > +     r = input_register_device(idev);
> > +     if (r < 0) {
> > +             dev_err(&ts->spi->dev, "can't register touchscreen device\n");
> > +             goto err3;
> > +     }
> > +
> > +     /* We can tolerate these failing */
> 
> Should we though?

For graceful degradation of functionality, yes. The core interface is
the touchscreen event stream, without that we make no sense at all. 
We can function as such without the following.

> > +     r = device_create_file(&ts->spi->dev, &dev_attr_ts_ctrl_selftest);
> > +     if (r < 0)
> > +             dev_warn(&ts->spi->dev, "can't create sysfs file for %s: %d\n",
> > +                      dev_attr_ts_ctrl_selftest.attr.name, r);
> > +
> > +     r = device_create_file(&ts->spi->dev, &dev_attr_pen_down);
> > +     if (r < 0)
> > +             dev_warn(&ts->spi->dev, "can't create sysfs file for %s: %d\n",
> > +                      dev_attr_pen_down.attr.name, r);
> > +
> > +     r = device_create_file(&ts->spi->dev, &dev_attr_disable_ts);
> > +     if (r < 0)
> > +             dev_warn(&ts->spi->dev, "can't create sysfs file for %s: %d\n",
> > +                      dev_attr_disable_ts.attr.name, r);
> > +
> 
> attribute_group is more compact by the way.

I promise I won't add to that list in the future, and will instead
move it over to an attribute_group.

> > +     /* Finally, configure and start the optional EDD watchdog. */
> > +     ts->esd_timeout = pdata->esd_timeout;
> > +     if (ts->esd_timeout && ts->set_reset) {
> > +             unsigned long wdj;
> > +             setup_timer(&ts->esd_timer, tsc2005_esd_timer_handler,
> > +                         (unsigned long)ts);
> > +             INIT_WORK(&ts->esd_work, tsc2005_rst_handler);
> > +             wdj = msecs_to_jiffies(ts->esd_timeout);
> > +             ts->esd_timer.expires = round_jiffies(jiffies+wdj);
> > +             add_timer(&ts->esd_timer);
> > +     }
> > +
> > +     return 0;
> > +err3:
> > +     free_irq(ts->spi->irq, ts);
> > +err2:
> > +     tsc2005_stop_scan(ts);
> > +     input_free_device(idev);
> > +err1:
> > +     return r;
> > +}
> > +
> > +static int __devinit tsc2005_probe(struct spi_device *spi)
> > +{
> > +     struct tsc2005                  *ts;
> > +     struct tsc2005_platform_data    *pdata = spi->dev.platform_data;
> > +     int r;
> > +
> > +     if (spi->irq < 0) {
> > +             dev_dbg(&spi->dev, "no irq?\n");
> > +             return -ENODEV;
> > +     }
> > +     if (!pdata) {
> > +             dev_dbg(&spi->dev, "no platform data?\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     ts = kzalloc(sizeof(*ts), GFP_KERNEL);
> > +     if (ts == NULL)
> > +             return -ENOMEM;
> > +
> > +     dev_set_drvdata(&spi->dev, ts);
> > +     ts->spi = spi;
> > +     spi->dev.power.power_state = PMSG_ON;
> > +
> > +     spi->mode = SPI_MODE_0;
> > +     spi->bits_per_word = 8;
> > +     /* The max speed might've been defined by the board-specific
> > +      * struct */
> > +     if (!spi->max_speed_hz)
> > +             spi->max_speed_hz = TSC2005_HZ;
> > +
> > +     spi_setup(spi);
> > +
> > +     r = tsc2005_ts_init(ts, pdata);
> > +     if (r)
> > +             goto err1;
> > +
> > +     return 0;
> > +
> > +err1:
> > +     kfree(ts);
> > +     return r;
> > +}
> > +
> > +static int __devexit tsc2005_remove(struct spi_device *spi)
> > +{
> > +     struct tsc2005 *ts = dev_get_drvdata(&spi->dev);
> > +
> > +     mutex_lock(&ts->mutex);
> > +     tsc2005_disable(ts);
> > +     mutex_unlock(&ts->mutex);
> > +
> > +     device_remove_file(&ts->spi->dev, &dev_attr_disable_ts);
> > +     device_remove_file(&ts->spi->dev, &dev_attr_pen_down);
> > +     device_remove_file(&ts->spi->dev, &dev_attr_ts_ctrl_selftest);
> > +
> > +     free_irq(ts->spi->irq, ts);
> > +     input_unregister_device(ts->idev);
> > +
> > +     if (ts->esd_timeout)
> > +             del_timer(&ts->esd_timer);
> 
> del_timer_sync().

Yup.

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