lkml.org 
[lkml]   [2007]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/3] Input/Touchscreen Driver: add support AD7877 touchscreen driver
    Hi Bryan, Michael,

    On 10/11/07, Bryan Wu <bryan.wu@analog.com> wrote:
    > +
    > +static int gpio3 = 0;

    No need to initialize.

    > +
    > +static int ad7877_read(struct device *dev, u16 reg)
    > +{
    > + struct spi_device *spi = to_spi_device(dev);
    > + struct ser_req *req = kzalloc(sizeof *req, GFP_KERNEL);

    How many reads can happen at once? Maybe allocate 1 ser_req per
    touchcsreen when creating it?

    > +
    > + if (likely(x && z1 && !device_suspended(&ts->spi->dev))) {
    > + /* compute touch pressure resistance using equation #2 */
    > + Rt = z2;
    > + Rt -= z1;
    > + Rt *= x;
    > + Rt *= ts->x_plate_ohms;
    > + Rt /= z1;
    > + Rt = (Rt + 2047) >> 12;
    > + } else
    > + Rt = 0;
    > +
    > + if (Rt) {
    > + input_report_abs(input_dev, ABS_X, x);
    > + input_report_abs(input_dev, ABS_Y, y);
    > + sync = 1;
    > + }
    > +
    > + if (sync) {
    > + input_report_abs(input_dev, ABS_PRESSURE, Rt);
    > + input_sync(input_dev);
    > + }

    Confused about the logic - you set sync only if Rt != 0 so why don't
    fold second "if" into the first one?

    > +/* Must be called with ts->lock held */
    > +static void ad7877_disable(struct ad7877 *ts)
    > +{
    > + if (ts->disabled)
    > + return;
    > +
    > + ts->disabled = 1;
    > +
    > + if (!ts->pending) {
    > + ts->irq_disabled = 1;
    > + disable_irq(ts->spi->irq);
    > + } else {
    > + /* the kthread will run at least once more, and
    > + * leave everything in a clean state, IRQ disabled
    > + */
    > + while (ts->pending) {
    > + spin_unlock_irq(&ts->lock);
    > + msleep(1);
    > + spin_lock_irq(&ts->lock);
    > + }
    > + }
    > +
    > + /* we know the chip's in lowpower mode since we always
    > + * leave it that way after every request
    > + */
    > +
    > +}

    This looks scary. Can you try moving locking inside ad7877_enable and
    ad7877_disable?

    > +
    > +static int __devinit ad7877_probe(struct spi_device *spi)
    > +{
    > + struct ad7877 *ts;
    > + struct input_dev *input_dev;
    > + struct ad7877_platform_data *pdata = spi->dev.platform_data;
    > + struct spi_message *m;
    > + int err;
    > + u16 verify;
    > +
    > +
    > + if (!spi->irq) {
    > + dev_dbg(&spi->dev, "no IRQ?\n");
    > + return -ENODEV;
    > + }
    > +
    > +
    > + if (!pdata) {
    > + dev_dbg(&spi->dev, "no platform data?\n");
    > + return -ENODEV;
    > + }
    > +
    > +
    > + /* don't exceed max specified SPI CLK frequency */
    > + if (spi->max_speed_hz > MAX_SPI_FREQ_HZ) {
    > + dev_dbg(&spi->dev, "SPI CLK %d Hz?\n",spi->max_speed_hz);
    > + return -EINVAL;
    > + }
    > +
    > + ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
    > + input_dev = input_allocate_device();
    > + if (!ts || !input_dev) {
    > + err = -ENOMEM;
    > + goto err_free_mem;
    > + }
    > +
    > +
    > + dev_set_drvdata(&spi->dev, ts);
    > + spi->dev.power.power_state = PMSG_ON;
    > +
    > + ts->spi = spi;
    > + ts->input = input_dev;
    > + ts->intr_flag = 0;
    > + init_timer(&ts->timer);
    > + ts->timer.data = (unsigned long) ts;
    > + ts->timer.function = ad7877_timer;
    > +
    > + spin_lock_init(&ts->lock);
    > +
    > + ts->model = pdata->model ? : 7877;
    > + ts->vref_delay_usecs = pdata->vref_delay_usecs ? : 100;
    > + ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
    > + ts->pressure_max = pdata->pressure_max ? : ~0;
    > +
    > +
    > + ts->stopacq_polarity = pdata->stopacq_polarity;
    > + ts->first_conversion_delay = pdata->first_conversion_delay;
    > + ts->acquisition_time = pdata->acquisition_time;
    > + ts->averaging = pdata->averaging;
    > + ts->pen_down_acc_interval = pdata->pen_down_acc_interval;
    > +
    > + snprintf(ts->phys, sizeof(ts->phys), "%s/input0", spi->dev.bus_id);
    > +
    > + input_dev->name = "AD7877 Touchscreen";
    > + input_dev->phys = ts->phys;
    > + input_dev->cdev.dev = &spi->dev;

    Please use input_dev->dev.parent, i will kill 'cdev' soon.

    > +
    > + err = input_register_device(input_dev);
    > + if (err)
    > + goto err_remove_attr;
    > +
    > + ts->intr_flag = 0;
    > +
    > + ad7877_task = kthread_run(ad7877_thread, ts, "ad7877_ktsd");
    > +
    > + if (IS_ERR(ad7877_task)) {
    > + printk(KERN_ERR "ts: Failed to start ad7877_task\n");
    > + goto err_remove_attr;

    You shoudl use input_unregister_device() once it was registered. So
    you need something like
    goto err_unregister_idev;
    ...
    err_unregister_idev:
    input_unregister_device(input_dev);
    input-dve = NULL; /* so we don't try to free it later */
    err_remove_attr:
    ...

    > + }
    > +
    > + return 0;
    > +
    > + err_remove_attr:
    > +
    > + sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
    > +
    > + if(gpio3)
    > + device_remove_file(&spi->dev, &dev_attr_gpio3);
    > + else
    > + device_remove_file(&spi->dev, &dev_attr_aux3);
    > +
    > +
    > + free_irq(spi->irq, ts);
    > + err_free_mem:
    > + input_free_device(input_dev);
    > + kfree(ts);
    > + return err;
    > +}
    > +
    > +static int __devexit ad7877_remove(struct spi_device *spi)
    > +{
    > + struct ad7877 *ts = dev_get_drvdata(&spi->dev);
    > +
    > + input_unregister_device(ts->input);
    > +
    > + ad7877_suspend(spi, PMSG_SUSPEND);
    > +
    > + kthread_stop(ad7877_task);

    You don't want to unregister device before stopping
    interrupts/kthread. Otherwise there is a chance you will try to use
    just freed device to send event through.

    The driver also contains some indenting damage, please re-check.

    Thanks!

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

    \
     
     \ /
      Last update: 2007-10-11 15:31    [W:0.060 / U:2.252 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site