Hi Bryan, Michael, On 10/11/07, Bryan Wu <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/