linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] New drivers from Blackfin Linux Team
@ 2007-10-11 10:23 Bryan Wu
  2007-10-11 10:23 ` [PATCH 1/3] Input/Joystick Driver: add support AD7142 joystick driver Bryan Wu
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Bryan Wu @ 2007-10-11 10:23 UTC (permalink / raw)
  To: dmitry.torokhov, linux-input, linux-joystick, linux-serial
  Cc: linux-kernel, akpm



^ permalink raw reply	[flat|nested] 15+ messages in thread
* RE: [PATCH 2/3] Input/Touchscreen Driver: add support AD7877 touchscreen driver
@ 2007-10-15 17:20 Hennerich, Michael
  2007-10-15 17:33 ` Dmitry Torokhov
  0 siblings, 1 reply; 15+ messages in thread
From: Hennerich, Michael @ 2007-10-15 17:20 UTC (permalink / raw)
  To: Dmitry Torokhov, Bryan Wu, Michael Hennerich
  Cc: linux-input, linux-joystick, linux-serial, linux-kernel, akpm


Hi Dmitry,


>From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
>Sent: Donnerstag, 11. Oktober 2007 15:28
>To: Bryan Wu; Michael Hennerich
>Cc: linux-input@atrey.karlin.mff.cuni.cz; linux-
>joystick@atrey.karlin.mff.cuni.cz; linux-serial@vger.kernel.org; linux-
>kernel@vger.kernel.org; akpm@linux-foundation.org
>Subject: Re: [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.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread
end of thread, other threads:[~2008-02-04  9:36 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-10-11 10:23 [PATCH 0/3] New drivers from Blackfin Linux Team Bryan Wu
2007-10-11 10:23 ` [PATCH 1/3] Input/Joystick Driver: add support AD7142 joystick driver Bryan Wu
2007-10-11 12:44   ` Dmitry Torokhov
2007-10-11 10:23 ` [PATCH 2/3] Input/Touchscreen Driver: add support AD7877 touchscreen driver Bryan Wu
2007-10-11 13:27   ` Dmitry Torokhov
2007-10-11 10:23 ` [PATCH 3/3] Blackfin serial driver: this driver enable SPORTs on Blackfin emulate UART Bryan Wu
2007-10-15 20:33   ` Andrew Morton
2007-10-15 22:03     ` Mike Frysinger
2007-10-15 22:22       ` Andrew Morton
2007-10-15 22:10     ` Robin Getz
2007-10-15 22:04   ` Mike Frysinger
2008-02-04  3:35   ` Andrew Morton
2008-02-04  9:36     ` Bryan Wu
  -- strict thread matches above, loose matches on Subject: below --
2007-10-15 17:20 [PATCH 2/3] Input/Touchscreen Driver: add support AD7877 touchscreen driver Hennerich, Michael
2007-10-15 17:33 ` Dmitry Torokhov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).