Hi Dmitry,
>From: Dmitry Torokhov [mailto:[email protected]]
>Sent: Donnerstag, 11. Oktober 2007 15:28
>To: Bryan Wu; Michael Hennerich
>Cc: [email protected]; linux-
>[email protected]; [email protected]; linux-
>[email protected]; [email protected]
>Subject: Re: [PATCH 2/3] Input/Touchscreen Driver: add support AD7877
>touchscreen driver
>
>Hi Bryan, Michael,
>
>On 10/11/07, Bryan Wu <[email protected]> wrote:
>> +
>> +static int gpio3 = 0;
>
>No need to initialize.
Sure - It's ZERO by default.
>
>> +
>> +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?
ad7877_read_adc, ad7877_read and ad7877_write are just used by the sysfs
hooks. Touchscreen samples are read by the kthread using a different
message struct. So far each sysfs invocation got its own storage for the
spi message, which then is handed over to the SPI bus driver.
The SPI bus driver serializes transfers in a kthread.
Two different processes could access the drivers sysfs hooks.
Using one ser_req per touch screen could require additional locking?
Things at is, looks pretty safe to me.
>
>> +
>> + 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?
Sure - I guess this was just a leftover form the original driver, this
driver was derived from.
>
>> +/* 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?
This is also something imported from the ads7846.c driver.
Since it worked pretty well I never touched this function.
However I think the spin_locks here are not necessary.
>
>> +
>> +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.
Will do.
>
>> +
>> + 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.
Ok.
>
>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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
