On Sunday 20 July 2014 13:28:42 Dmitry Torokhov wrote:

On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:

quoted

quoted

quoted

+
+    do {
+        ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);

= exynos

quoted

+        if (ret == -ETIMEDOUT)
+            break;
+
+        pressed = x & y & ADC_DATX_PRESSED;
+        if (!pressed)
+            break;
+
+        input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
+        input_report_abs(info->input, ABS_Y, y & ADC_DATX_MASK);
+        input_report_key(info->input, BTN_TOUCH, 1);
+        input_sync(info->input);
+
+        msleep(1);
+    } while (1);

It would be nice to actually close the device even if someone is
touching screen. Please implement open/close methods and have them set a
flag that you would check here.

Ok. I think it's even better to move the request_irq() into the open function,
which will avoid the flag and defer the error handling into the actual opening,
as well as syncing the running irq with the close function.

quoted

quoted

quoted

+    /* data from s3c2410_ts driver */
+    info->input->name = "S3C24xx TouchScreen";
+    info->input->id.bustype = BUS_HOST;
+    info->input->id.vendor = 0xDEAD;
+    info->input->id.product = 0xBEEF;

You do not need to fill these entries with fake data.

Ok, I wondered about this, but didn't want to change too much from
the old driver (I changed the version number).

quoted

quoted

quoted

+    info->input->id.version = 0x0200;

Do I need this?

quoted

quoted

quoted

@@ -576,6 +728,11 @@ static int exynos_adc_probe(struct platform_device *pdev)
 err_of_populate:
     device_for_each_child(&indio_dev->dev, NULL,
                 exynos_adc_remove_devices);
+    if (has_ts) {
+        input_unregister_device(info->input);
+        free_irq(info->tsirq, info);

Are we sure that device is quiesced here and interrupt won't arrive
between input_unregister_device() and free_irq()? I guess if you
properly implement open/close it won't matter.

 
Should be fixed now.

quoted

quoted

quoted

+err_iio:
     iio_device_unregister(indio_dev);
 err_irq:
     free_irq(info->irq, info);

@@ -595,9 +752,12 @@ static int exynos_adc_remove(struct platform_device *pdev)
     struct iio_dev *indio_dev = platform_get_drvdata(pdev);
     struct exynos_adc *info = iio_priv(indio_dev);

+    input_free_device(info->input);

Should be unregister, not free.

Ok.

Thanks a lot for the review! I'll send out the new version after some build testing.

	Arnd