Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Jonathan Cameron]

On 20/07/14 14:49, Jonathan Cameron wrote:
> On 18/07/14 20:29, Arnd Bergmann wrote:
>> This adds support for the touchscreen on Samsung s3c64xx.
>> The driver is completely untested but shows roughly how
>> it could be done, following the example of the at91 driver.
>>
> Hi Arnd,
Cc'd linux-input and Dmitry.
>
>> Open questions include:
>>
>> - compared to the old plat-samsung/adc driver, there is
>>    no support for prioritizing ts over other clients, nor
>>    for oversampling. From my reading of the code, the
>>    priorities didn't actually have any effect at all, but
>>    the oversampling might be needed. Maybe the original
>>    authors have some insight.
>>
>> - I simply register the input device from the adc driver
>>    itself, as the at91 code does. The driver also supports
>>    sub-nodes, but I don't understand how they are meant
>>    to be used, so using those might be better.
> So, the alternative you are (I think) referring to is to use
> the buffered in kernel client interface.  That way a separate
> touch screen driver can use the output channels provided by IIO
> in much the same way you might use a regulator or similar.
> Note that whilst this is similar to the simple polled interface
> used for things like the iio_hwmon driver, the data flow is
> quite different (clearly the polled interfce would be
> inappropriate here).
>
> Whilst we've discussed it in the past for touch screen drivers
> like this, usually the hardware isn't generic enough to be
> of any real use if not being used as a touch screen.  As such
> it's often simpler to just have the support directly in the
> driver (as you've observed the at91 driver does this).
>
> Whilst the interface has been there a while, it's not really had
> all that much use.  The original target was the simpler case
> of 3D accelerometer where we have a generic iio to input
> bridge driver. Time constraints meant that I haven't yet actually
> formally submitted the input side of this. Whilst there are lots
> of other things that can use this interface, right now nothing
> actually does so.
>
>>
>> - The new exynos_read_s3c64xx_ts() function is intentionally
>>    very similar to the existing exynos_read_raw() functions.
>>    It should probably be changed, either by merging the two
>>    into one, or by simplifying the exynos_read_s3c64xx_ts()
>>    function. This depends a bit on the answers to the questions
>>    above.
> I'd be tempted to not bother keeping them that similar.  It's
> not a generic IIO channel so simplify it where possible.
>>
>> - We probably need to add support for platform_data as well,
>>    I've skipped this so far.
>>
>> - Is anybody able to debug this driver on real hardware?
>>    While it's possible that it actually works, it's more
>>    likely that I made a few subtle mistakes.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Looks pretty good to me.  A few symantic bits and pieces and
> one bug spotted.  Short and sweet.
>>
>> diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> index e1b74828f413..4329bf3c3326 100644
>> --- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> +++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
>> @@ -41,6 +41,10 @@ Required properties:
>>                          and compatible ADC block)
>>   - vdd-supply        VDD input supply.
>>
>> +Optional properties:
>> +- has-touchscreen:    If present, indicates that a touchscreen is
>> +            connected an usable.
>> +
>>   Note: child nodes can be added for auto probing from device tree.
>>
>>   Example: adding device info in dtsi file
>> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
>> index 5f95638513d2..cf1d9f3e2492 100644
>> --- a/drivers/iio/adc/exynos_adc.c
>> +++ b/drivers/iio/adc/exynos_adc.c
>> @@ -34,6 +34,7 @@
>>   #include <linux/regulator/consumer.h>
>>   #include <linux/of_platform.h>
>>   #include <linux/err.h>
>> +#include <linux/input.h>
> Might want to make the input side optional at compile time...
> I supose the existing parts are unlikely to be used much in headless
> devices, but you never know.  Maybe we just leave this until someone
> shouts they want to be able to avoid compiling it in.
>>
>>   #include <linux/iio/iio.h>
>>   #include <linux/iio/machine.h>
>> @@ -103,6 +104,7 @@
>>
>>   /* Bit definitions common for ADC_V1 and ADC_V2 */
>>   #define ADC_CON_EN_START    (1u << 0)
>> +#define ADC_DATX_PRESSED    (1u << 15)
>>   #define ADC_DATX_MASK        0xFFF
>>
>>   #define EXYNOS_ADC_TIMEOUT    (msecs_to_jiffies(100))
>> @@ -110,16 +112,20 @@
>>   struct exynos_adc {
>>       struct exynos_adc_data    *data;
>>       struct device        *dev;
>> +    struct input_dev    *input;
>>       void __iomem        *regs;
>>       void __iomem        *enable_reg;
>>       struct clk        *clk;
>>       struct clk        *sclk;
>>       unsigned int        irq;
>> +    unsigned int        tsirq;
>>       struct regulator    *vdd;
>>
>>       struct completion    completion;
>>
>> +    bool            read_ts;
>>       u32            value;
>> +    u32            value2;
> As I state further down, I'd rather keep a little
> clear of the naming used in IIO for bits that aren't
> going through IIO (less confusing!). Maybe just
> have
>      u32 x, y;
>>       unsigned int            version;
>>   };
>>
>> @@ -390,12 +396,61 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
>>       return ret;
>>   }
>>
>> +static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
>> +                struct iio_chan_spec const *chan,
>> +                int *val,
>> +                int *val2,
>> +                long mask)
>> +{
>> +    struct exynos_adc *info = iio_priv(indio_dev);
>> +    unsigned long timeout;
>> +    int ret;
>> +
>> +    if (mask != IIO_CHAN_INFO_RAW)
>> +        return -EINVAL;
>> +
>> +    mutex_lock(&indio_dev->mlock);
>> +    info->read_ts = 1;
>> +
>> +    reinit_completion(&info->completion);
>> +
>> +    writel(ADC_S3C2410_TSC_PULL_UP_DISABLE | ADC_TSC_AUTOPST,
>> +           ADC_V1_TSC(info->regs));
>> +
>> +    /* Select the ts channel to be used and Trigger conversion */
>> +    info->data->start_conv(info, 0);
> 0 is a rather magic value.  A define perhaps?
>> +
>> +    timeout = wait_for_completion_timeout
>> +            (&info->completion, EXYNOS_ADC_TIMEOUT);
>> +    if (timeout == 0) {
>> +        dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
>> +        if (info->data->init_hw)
>> +            info->data->init_hw(info);
>> +        ret = -ETIMEDOUT;
>> +    } else {
>> +        *val = info->value;
>> +        *val2 = info->value2;
> This is definitely abuse as those two values are not intended for
> different values.  If you want to do this please use different naming
> and don't try to fiddle it into the IIO read raw framework.
> As you've suggested above, better to simplify this code and drop the
> bits cloned from the other handler.
>> +        ret = IIO_VAL_INT;
>> +    }
>> +
>> +    info->read_ts = 0;
>> +    mutex_unlock(&indio_dev->mlock);
>> +
>> +    return ret;
>> +}
>> +
>>   static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>>   {
>>       struct exynos_adc *info = (struct exynos_adc *)dev_id;
>>
>>       /* Read value */
>> -    info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
>> +    if (info->read_ts) {
>> +        info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
>> +        info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
> ADC_DATY_MASK would be more obviously correct.
>
>> +        writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
> Perhaps the above is cryptic enough to warrant a comment?
>> +    } else {
>> +        info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
>> +    }
>>
>>       /* clear irq */
>>       if (info->data->clear_irq)
>> @@ -406,6 +461,46 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>>       return IRQ_HANDLED;
>>   }
>>
>> +/*
>> + * Here we (ab)use a threaded interrupt handler to stay running
>> + * for as long as the touchscreen remains pressed, we report
>> + * a new event with the latest data and then sleep until the
>> + * next timer tick. This mirrors the behavior of the old
>> + * driver, with much less code.
>> + */
>> +static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
>> +{
>> +    struct exynos_adc *info = dev_id;
>> +    struct iio_dev *dev = dev_get_drvdata(info->dev);
>> +    u32 x, y;
>> +    bool pressed;
>> +    int ret;
>> +
>> +    do {
>> +        ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
> = exynos
>> +        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);
>> +
>> +    input_report_key(info->input, BTN_TOUCH, 0);
>> +    input_sync(info->input);
>> +
>> +    writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
>> +
>> +    return IRQ_HANDLED;
>> +}
>> +
>>   static int exynos_adc_reg_access(struct iio_dev *indio_dev,
>>                     unsigned reg, unsigned writeval,
>>                     unsigned *readval)
>> @@ -457,12 +552,57 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
>>       return 0;
>>   }
>>
>> +static int exynos_adc_ts_init(struct exynos_adc *info)
>> +{
>> +    int ret;
>> +
>> +    info->input = input_allocate_device();
>> +    if (!info->input)
>> +        return -ENOMEM;
>> +
>> +    info->input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
>> +    info->input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
>> +
>> +    input_set_abs_params(info->input, ABS_X, 0, 0x3FF, 0, 0);
>> +    input_set_abs_params(info->input, ABS_Y, 0, 0x3FF, 0, 0);
>> +
>> +    /* 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;
>> +    info->input->id.version = 0x0200;
>> +
>> +    ret = input_register_device(info->input);
>> +    if (ret) {
>> +        input_free_device(info->input);
>> +        goto err;
>> +    }
>> +
>> +    if (info->tsirq > 0)
>> +        ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
>> +                    0, "touchscreen", info);
> info->tsirq
> (that had me really confused for a moment ;)
> Also, perhaps a more specific name.  touchscreen_updown or similar as the
> main interrupt is also used during touchscreen operation.
>> +    if (ret < 0) {
>> +        dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
>> +                            info->irq);
>> +        goto err_input;
>> +    }
>> +
>> +    return 0;
>> +
>> +err_input:
>> +    input_unregister_device(info->input);
>> +err:
>> +    return ret;
>> +}
>> +
>>   static int exynos_adc_probe(struct platform_device *pdev)
>>   {
>>       struct exynos_adc *info = NULL;
>>       struct device_node *np = pdev->dev.of_node;
>>       struct iio_dev *indio_dev = NULL;
>>       struct resource    *mem;
>> +    bool has_ts;
>>       int ret = -ENODEV;
>>       int irq;
>>
>> @@ -498,8 +638,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>           dev_err(&pdev->dev, "no irq resource?\n");
>>           return irq;
>>       }
>> -
>>       info->irq = irq;
>> +
>> +    irq = platform_get_irq(pdev, 1);
>> +    if (irq == -EPROBE_DEFER)
>> +        return irq;
>> +
>> +    info->tsirq = irq;
>> +
>>       info->dev = &pdev->dev;
>>
>>       init_completion(&info->completion);
>> @@ -565,6 +711,12 @@ static int exynos_adc_probe(struct platform_device *pdev)
>>       if (info->data->init_hw)
>>           info->data->init_hw(info);
>>
>> +    has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
>> +    if (has_ts)
>> +        ret = exynos_adc_ts_init(info);
>> +    if (ret)
>> +        goto err_iio;
>> +
>>       ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
>>       if (ret < 0) {
>>           dev_err(&pdev->dev, "failed adding child nodes\n");
>> @@ -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);
>> +    }
>> +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);
>>       device_for_each_child(&indio_dev->dev, NULL,
>>                   exynos_adc_remove_devices);
>>       iio_device_unregister(indio_dev);
>> +    if (info->tsirq > 0)
>> +        free_irq(info->tsirq, info);
>>       free_irq(info->irq, info);
>>       if (info->data->exit_hw)
>>           info->data->exit_hw(info);
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Dmitry Torokhov]

On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:
> On 20/07/14 14:49, Jonathan Cameron wrote:
> >On 18/07/14 20:29, Arnd Bergmann wrote:
> >>This adds support for the touchscreen on Samsung s3c64xx.
> >>The driver is completely untested but shows roughly how
> >>it could be done, following the example of the at91 driver.
> >>
> >Hi Arnd,
> Cc'd linux-input and Dmitry.
> >
> >>Open questions include:
> >>
> >>- compared to the old plat-samsung/adc driver, there is
> >>   no support for prioritizing ts over other clients, nor
> >>   for oversampling. From my reading of the code, the
> >>   priorities didn't actually have any effect at all, but
> >>   the oversampling might be needed. Maybe the original
> >>   authors have some insight.
> >>
> >>- I simply register the input device from the adc driver
> >>   itself, as the at91 code does. The driver also supports
> >>   sub-nodes, but I don't understand how they are meant
> >>   to be used, so using those might be better.
> >So, the alternative you are (I think) referring to is to use
> >the buffered in kernel client interface.  That way a separate
> >touch screen driver can use the output channels provided by IIO
> >in much the same way you might use a regulator or similar.
> >Note that whilst this is similar to the simple polled interface
> >used for things like the iio_hwmon driver, the data flow is
> >quite different (clearly the polled interfce would be
> >inappropriate here).
> >
> >Whilst we've discussed it in the past for touch screen drivers
> >like this, usually the hardware isn't generic enough to be
> >of any real use if not being used as a touch screen.  As such
> >it's often simpler to just have the support directly in the
> >driver (as you've observed the at91 driver does this).
> >
> >Whilst the interface has been there a while, it's not really had
> >all that much use.  The original target was the simpler case
> >of 3D accelerometer where we have a generic iio to input
> >bridge driver. Time constraints meant that I haven't yet actually
> >formally submitted the input side of this. Whilst there are lots
> >of other things that can use this interface, right now nothing
> >actually does so.
> >
> >>
> >>- The new exynos_read_s3c64xx_ts() function is intentionally
> >>   very similar to the existing exynos_read_raw() functions.
> >>   It should probably be changed, either by merging the two
> >>   into one, or by simplifying the exynos_read_s3c64xx_ts()
> >>   function. This depends a bit on the answers to the questions
> >>   above.
> >I'd be tempted to not bother keeping them that similar.  It's
> >not a generic IIO channel so simplify it where possible.
> >>
> >>- We probably need to add support for platform_data as well,
> >>   I've skipped this so far.
> >>
> >>- Is anybody able to debug this driver on real hardware?
> >>   While it's possible that it actually works, it's more
> >>   likely that I made a few subtle mistakes.
> >>
> >>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >Looks pretty good to me.  A few symantic bits and pieces and
> >one bug spotted.  Short and sweet.
> >>
> >>diff --git a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> >>index e1b74828f413..4329bf3c3326 100644
> >>--- a/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> >>+++ b/Documentation/devicetree/bindings/arm/samsung/exynos-adc.txt
> >>@@ -41,6 +41,10 @@ Required properties:
> >>                         and compatible ADC block)
> >>  - vdd-supply        VDD input supply.
> >>
> >>+Optional properties:
> >>+- has-touchscreen:    If present, indicates that a touchscreen is
> >>+            connected an usable.
> >>+
> >>  Note: child nodes can be added for auto probing from device tree.
> >>
> >>  Example: adding device info in dtsi file
> >>diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> >>index 5f95638513d2..cf1d9f3e2492 100644
> >>--- a/drivers/iio/adc/exynos_adc.c
> >>+++ b/drivers/iio/adc/exynos_adc.c
> >>@@ -34,6 +34,7 @@
> >>  #include <linux/regulator/consumer.h>
> >>  #include <linux/of_platform.h>
> >>  #include <linux/err.h>
> >>+#include <linux/input.h>
> >Might want to make the input side optional at compile time...
> >I supose the existing parts are unlikely to be used much in headless
> >devices, but you never know.  Maybe we just leave this until someone
> >shouts they want to be able to avoid compiling it in.
> >>
> >>  #include <linux/iio/iio.h>
> >>  #include <linux/iio/machine.h>
> >>@@ -103,6 +104,7 @@
> >>
> >>  /* Bit definitions common for ADC_V1 and ADC_V2 */
> >>  #define ADC_CON_EN_START    (1u << 0)
> >>+#define ADC_DATX_PRESSED    (1u << 15)
> >>  #define ADC_DATX_MASK        0xFFF
> >>
> >>  #define EXYNOS_ADC_TIMEOUT    (msecs_to_jiffies(100))
> >>@@ -110,16 +112,20 @@
> >>  struct exynos_adc {
> >>      struct exynos_adc_data    *data;
> >>      struct device        *dev;
> >>+    struct input_dev    *input;
> >>      void __iomem        *regs;
> >>      void __iomem        *enable_reg;
> >>      struct clk        *clk;
> >>      struct clk        *sclk;
> >>      unsigned int        irq;
> >>+    unsigned int        tsirq;
> >>      struct regulator    *vdd;
> >>
> >>      struct completion    completion;
> >>
> >>+    bool            read_ts;
> >>      u32            value;
> >>+    u32            value2;
> >As I state further down, I'd rather keep a little
> >clear of the naming used in IIO for bits that aren't
> >going through IIO (less confusing!). Maybe just
> >have
> >     u32 x, y;
> >>      unsigned int            version;
> >>  };
> >>
> >>@@ -390,12 +396,61 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> >>      return ret;
> >>  }
> >>
> >>+static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
> >>+                struct iio_chan_spec const *chan,
> >>+                int *val,
> >>+                int *val2,
> >>+                long mask)
> >>+{
> >>+    struct exynos_adc *info = iio_priv(indio_dev);
> >>+    unsigned long timeout;
> >>+    int ret;
> >>+
> >>+    if (mask != IIO_CHAN_INFO_RAW)
> >>+        return -EINVAL;
> >>+
> >>+    mutex_lock(&indio_dev->mlock);
> >>+    info->read_ts = 1;
> >>+
> >>+    reinit_completion(&info->completion);
> >>+
> >>+    writel(ADC_S3C2410_TSC_PULL_UP_DISABLE | ADC_TSC_AUTOPST,
> >>+           ADC_V1_TSC(info->regs));
> >>+
> >>+    /* Select the ts channel to be used and Trigger conversion */
> >>+    info->data->start_conv(info, 0);
> >0 is a rather magic value.  A define perhaps?
> >>+
> >>+    timeout = wait_for_completion_timeout
> >>+            (&info->completion, EXYNOS_ADC_TIMEOUT);
> >>+    if (timeout == 0) {
> >>+        dev_warn(&indio_dev->dev, "Conversion timed out! Resetting\n");
> >>+        if (info->data->init_hw)
> >>+            info->data->init_hw(info);
> >>+        ret = -ETIMEDOUT;
> >>+    } else {
> >>+        *val = info->value;
> >>+        *val2 = info->value2;
> >This is definitely abuse as those two values are not intended for
> >different values.  If you want to do this please use different naming
> >and don't try to fiddle it into the IIO read raw framework.
> >As you've suggested above, better to simplify this code and drop the
> >bits cloned from the other handler.
> >>+        ret = IIO_VAL_INT;
> >>+    }
> >>+
> >>+    info->read_ts = 0;
> >>+    mutex_unlock(&indio_dev->mlock);
> >>+
> >>+    return ret;
> >>+}
> >>+
> >>  static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> >>  {
> >>      struct exynos_adc *info = (struct exynos_adc *)dev_id;
> >>
> >>      /* Read value */
> >>-    info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >>+    if (info->read_ts) {
> >>+        info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >>+        info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
> >ADC_DATY_MASK would be more obviously correct.
> >
> >>+        writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
> >Perhaps the above is cryptic enough to warrant a comment?
> >>+    } else {
> >>+        info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
> >>+    }
> >>
> >>      /* clear irq */
> >>      if (info->data->clear_irq)
> >>@@ -406,6 +461,46 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> >>      return IRQ_HANDLED;
> >>  }
> >>
> >>+/*
> >>+ * Here we (ab)use a threaded interrupt handler to stay running
> >>+ * for as long as the touchscreen remains pressed, we report
> >>+ * a new event with the latest data and then sleep until the
> >>+ * next timer tick. This mirrors the behavior of the old
> >>+ * driver, with much less code.
> >>+ */
> >>+static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
> >>+{
> >>+    struct exynos_adc *info = dev_id;
> >>+    struct iio_dev *dev = dev_get_drvdata(info->dev);
> >>+    u32 x, y;
> >>+    bool pressed;
> >>+    int ret;
> >>+
> >>+    do {
> >>+        ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
> >= exynos
> >>+        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.


> >>+
> >>+    input_report_key(info->input, BTN_TOUCH, 0);
> >>+    input_sync(info->input);
> >>+
> >>+    writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
> >>+
> >>+    return IRQ_HANDLED;
> >>+}
> >>+
> >>  static int exynos_adc_reg_access(struct iio_dev *indio_dev,
> >>                    unsigned reg, unsigned writeval,
> >>                    unsigned *readval)
> >>@@ -457,12 +552,57 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
> >>      return 0;
> >>  }
> >>
> >>+static int exynos_adc_ts_init(struct exynos_adc *info)
> >>+{
> >>+    int ret;
> >>+
> >>+    info->input = input_allocate_device();
> >>+    if (!info->input)
> >>+        return -ENOMEM;
> >>+
> >>+    info->input->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> >>+    info->input->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> >>+
> >>+    input_set_abs_params(info->input, ABS_X, 0, 0x3FF, 0, 0);
> >>+    input_set_abs_params(info->input, ABS_Y, 0, 0x3FF, 0, 0);
> >>+
> >>+    /* 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.

> >>+    info->input->id.version = 0x0200;
> >>+
> >>+    ret = input_register_device(info->input);
> >>+    if (ret) {
> >>+        input_free_device(info->input);
> >>+        goto err;
> >>+    }
> >>+
> >>+    if (info->tsirq > 0)
> >>+        ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
> >>+                    0, "touchscreen", info);
> >info->tsirq
> >(that had me really confused for a moment ;)
> >Also, perhaps a more specific name.  touchscreen_updown or similar as the
> >main interrupt is also used during touchscreen operation.
> >>+    if (ret < 0) {
> >>+        dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
> >>+                            info->irq);
> >>+        goto err_input;
> >>+    }
> >>+
> >>+    return 0;
> >>+
> >>+err_input:
> >>+    input_unregister_device(info->input);
> >>+err:
> >>+    return ret;
> >>+}
> >>+
> >>  static int exynos_adc_probe(struct platform_device *pdev)
> >>  {
> >>      struct exynos_adc *info = NULL;
> >>      struct device_node *np = pdev->dev.of_node;
> >>      struct iio_dev *indio_dev = NULL;
> >>      struct resource    *mem;
> >>+    bool has_ts;
> >>      int ret = -ENODEV;
> >>      int irq;
> >>
> >>@@ -498,8 +638,14 @@ static int exynos_adc_probe(struct platform_device *pdev)
> >>          dev_err(&pdev->dev, "no irq resource?\n");
> >>          return irq;
> >>      }
> >>-
> >>      info->irq = irq;
> >>+
> >>+    irq = platform_get_irq(pdev, 1);
> >>+    if (irq == -EPROBE_DEFER)
> >>+        return irq;
> >>+
> >>+    info->tsirq = irq;
> >>+
> >>      info->dev = &pdev->dev;
> >>
> >>      init_completion(&info->completion);
> >>@@ -565,6 +711,12 @@ static int exynos_adc_probe(struct platform_device *pdev)
> >>      if (info->data->init_hw)
> >>          info->data->init_hw(info);
> >>
> >>+    has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
> >>+    if (has_ts)
> >>+        ret = exynos_adc_ts_init(info);
> >>+    if (ret)
> >>+        goto err_iio;
> >>+
> >>      ret = of_platform_populate(np, exynos_adc_match, NULL, &indio_dev->dev);
> >>      if (ret < 0) {
> >>          dev_err(&pdev->dev, "failed adding child nodes\n");
> >>@@ -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.

> >>+    }
> >>+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.

> >>      device_for_each_child(&indio_dev->dev, NULL,
> >>                  exynos_adc_remove_devices);
> >>      iio_device_unregister(indio_dev);
> >>+    if (info->tsirq > 0)
> >>+        free_irq(info->tsirq, info);
> >>      free_irq(info->irq, info);
> >>      if (info->data->exit_hw)
> >>          info->data->exit_hw(info);
> >>

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Arnd Bergmann]

On Sunday 20 July 2014 13:28:42 Dmitry Torokhov wrote:
> On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:
> > >>+
> > >>+    do {
> > >>+        ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
> > >= exynos
> > >>+        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.

> > >>+    /* 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).

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

Do I need this?

> > >>@@ -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.

> > >>+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

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Arnd Bergmann]

On Monday 21 July 2014 12:23:58 Arnd Bergmann wrote:
> 
> Thanks a lot for the review! I'll send out the new version after some build testing.

Here are the changes I did to the adc driver based on the review comments.
I'll start a new thread for the updated version that includes the changes.

diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index 3d2caea05977..823d7ebc7f52 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -67,6 +67,9 @@
 
 #define ADC_S3C2410_CON_SELMUX(x) (((x)&0x7)<<3)
 
+/* touch screen always uses channel 0 */
+#define ADC_S3C2410_MUX_TS	0
+
 /* ADCTSC Register Bits */
 #define ADC_S3C2443_TSC_UD_SEN		(1u << 8)
 #define ADC_S3C2410_TSC_YM_SEN		(1u << 7)
@@ -106,6 +109,7 @@
 #define ADC_CON_EN_START	(1u << 0)
 #define ADC_DATX_PRESSED	(1u << 15)
 #define ADC_DATX_MASK		0xFFF
+#define ADC_DATY_MASK		0xFFF
 
 #define EXYNOS_ADC_TIMEOUT	(msecs_to_jiffies(100))
 
@@ -123,10 +127,12 @@ struct exynos_adc {
 
 	struct completion	completion;
 
-	bool			read_ts;
 	u32			value;
-	u32			value2;
 	unsigned int            version;
+
+	bool			read_ts;
+	u32			ts_x;
+	u32			ts_y;
 };
 
 struct exynos_adc_data {
@@ -396,21 +402,14 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
-				struct iio_chan_spec const *chan,
-				int *val,
-				int *val2,
-				long mask)
+static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev, int *x, int *y)
 {
 	struct exynos_adc *info = iio_priv(indio_dev);
 	unsigned long timeout;
 	int ret;
 
-	if (mask != IIO_CHAN_INFO_RAW)
-		return -EINVAL;
-
 	mutex_lock(&indio_dev->mlock);
-	info->read_ts = 1;
+	info->read_ts = true;
 
 	reinit_completion(&info->completion);
 
@@ -418,7 +417,7 @@ static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
 	       ADC_V1_TSC(info->regs));
 
 	/* Select the ts channel to be used and Trigger conversion */
-	info->data->start_conv(info, 0);
+	info->data->start_conv(info, ADC_S3C2410_MUX_TS);
 
 	timeout = wait_for_completion_timeout
 			(&info->completion, EXYNOS_ADC_TIMEOUT);
@@ -428,12 +427,12 @@ static int exynos_read_s3c64xx_ts(struct iio_dev *indio_dev,
 			info->data->init_hw(info);
 		ret = -ETIMEDOUT;
 	} else {
-		*val = info->value;
-		*val2 = info->value2;
-		ret = IIO_VAL_INT;
+		*x = info->ts_x;
+		*y = info->ts_y;
+		ret = 0;
 	}
 
-	info->read_ts = 0;
+	info->read_ts = false;
 	mutex_unlock(&indio_dev->mlock);
 
 	return ret;
@@ -445,8 +444,8 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
 
 	/* Read value */
 	if (info->read_ts) {
-		info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
-		info->value2 = readl(ADC_V1_DATY(info->regs)) & ADC_DATX_MASK;
+		info->ts_x = readl(ADC_V1_DATX(info->regs));
+		info->ts_y = readl(ADC_V1_DATY(info->regs));
 		writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs));
 	} else {
 		info->value = readl(ADC_V1_DATX(info->regs)) & ADC_DATX_MASK;
@@ -477,7 +476,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
 	int ret;
 
 	do {
-		ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
+		ret = exynos_read_s3c64xx_ts(dev, &x, &y);
 		if (ret == -ETIMEDOUT)
 			break;
 
@@ -486,11 +485,15 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
 			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_abs(info->input, ABS_Y, y & ADC_DATY_MASK);
 		input_report_key(info->input, BTN_TOUCH, 1);
 		input_sync(info->input);
 
 		msleep(1);
+
+		/* device may have been closed while touched */
+		if (!info->input->users)
+			return IRQ_HANDLED;
 	} while (1);
 
 	input_report_key(info->input, BTN_TOUCH, 0);
@@ -552,10 +555,28 @@ static int exynos_adc_remove_devices(struct device *dev, void *c)
 	return 0;
 }
 
+static int exynos_adc_ts_open(struct input_dev *dev)
+{
+	struct exynos_adc *info = input_get_drvdata(dev);
+
+	return request_threaded_irq(info->tsirq, NULL, exynos_ts_isr,
+				    0, "touchscreen", info);
+}
+
+static void exynos_adc_ts_close(struct input_dev *dev)
+{
+	struct exynos_adc *info = input_get_drvdata(dev);
+
+	free_irq(info->tsirq, info);
+}
+
 static int exynos_adc_ts_init(struct exynos_adc *info)
 {
 	int ret;
 
+	if (info->tsirq <= 0)
+		return -ENODEV;
+
 	info->input = input_allocate_device();
 	if (!info->input)
 		return -ENOMEM;
@@ -566,33 +587,17 @@ static int exynos_adc_ts_init(struct exynos_adc *info)
 	input_set_abs_params(info->input, ABS_X, 0, 0x3FF, 0, 0);
 	input_set_abs_params(info->input, ABS_Y, 0, 0x3FF, 0, 0);
 
-	/* 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;
-	info->input->id.version = 0x0200;
+	info->input->open = exynos_adc_ts_open;
+	info->input->close = exynos_adc_ts_close;
+	
+	input_set_drvdata(info->input, info);
 
 	ret = input_register_device(info->input);
-	if (ret) {
+	if (ret)
 		input_free_device(info->input);
-		goto err;
-	}
-
-	if (info->tsirq > 0)
-		ret = request_threaded_irq(info->irq, NULL, exynos_ts_isr,
-					0, "touchscreen", info);
-	if (ret < 0) {
-		dev_err(info->dev, "failed requesting touchsccreen irq, irq = %d\n",
-							info->irq);
-		goto err_input;
-	}
-
-	return 0;
 
-err_input:
-	input_unregister_device(info->input);
-err:
 	return ret;
 }
 
@@ -602,7 +607,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct iio_dev *indio_dev = NULL;
 	struct resource	*mem;
-	bool has_ts;
+	bool has_ts = false;
 	int ret = -ENODEV;
 	int irq;
 
@@ -711,7 +716,10 @@ static int exynos_adc_probe(struct platform_device *pdev)
 	if (info->data->init_hw)
 		info->data->init_hw(info);
 
-	has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
+	/* leave out any TS related code if unreachable */
+	if (IS_BUILTIN(CONFIG_INPUT) ||
+	    (IS_MODULE(CONFIG_INPUT) && config_enabled(MODULE)))
+		has_ts = of_property_read_bool(pdev->dev.of_node, "has-touchscreen");
 	if (has_ts)
 		ret = exynos_adc_ts_init(info);
 	if (ret)
@@ -752,7 +760,7 @@ 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);	
+	input_unregister_device(info->input);	
 	device_for_each_child(&indio_dev->dev, NULL,
 				exynos_adc_remove_devices);
 	iio_device_unregister(indio_dev);

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Dmitry Torokhov]

On Mon, Jul 21, 2014 at 12:23:58PM +0200, Arnd Bergmann wrote:
> On Sunday 20 July 2014 13:28:42 Dmitry Torokhov wrote:
> > On Sun, Jul 20, 2014 at 02:51:37PM +0100, Jonathan Cameron wrote:
> > > >>+
> > > >>+    do {
> > > >>+        ret =exynos_read_s3c64xx_ts(dev, NULL, &x, &y, IIO_CHAN_INFO_RAW);
> > > >= exynos
> > > >>+        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.

I do not quite like acquiring resources needed in open. I think drivers should
do all resource acquisition in probe() and leave open()/close() to
activate/quiesce devices.

> 
> > > >>+    /* 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).
> 
> > > >>+    info->input->id.version = 0x0200;
> 
> Do I need this?

Not really.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Arnd Bergmann]

On Monday 21 July 2014 07:44:42 Dmitry Torokhov wrote:
> > > 
> > > 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.
> 
> I do not quite like acquiring resources needed in open. I think drivers should
> do all resource acquisition in probe() and leave open()/close() to
> activate/quiesce devices.

Ok, I'll move it back then. I'm not sure what I'm supposed to do
in open/close then. Isn't it enough to check info->input->users
like this?

static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
{
        struct exynos_adc *info = dev_id;
        struct iio_dev *dev = dev_get_drvdata(info->dev);
        u32 x, y;
        bool pressed;
        int ret;

        while (info->input->users) {
                ret = exynos_read_s3c64xx_ts(dev, &x, &y);
                if (ret == -ETIMEDOUT)
                        break;

                pressed = x & y & ADC_DATX_PRESSED;
                if (!pressed) {
                        input_report_key(info->input, BTN_TOUCH, 0);
                        input_sync(info->input);
                        break;
                }

                input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
                input_report_abs(info->input, ABS_Y, y & ADC_DATY_MASK);
                input_report_key(info->input, BTN_TOUCH, 1);
                input_sync(info->input);

                msleep(1);
        };

        writel(0, ADC_V1_CLRINTPNDNUP(info->regs));

        return IRQ_HANDLED;
}

I could do enable_irq()/disable_irq(), but that leaves a small
race at startup where we request the irq line and then immediately
disable it again.

	Arnd

Re: [PATCH 2/2] iio: exynos-adc: add experimental touchscreen support

[Dmitry Torokhov]

On Mon, Jul 21, 2014 at 05:11:27PM +0200, Arnd Bergmann wrote:
> On Monday 21 July 2014 07:44:42 Dmitry Torokhov wrote:
> > > > 
> > > > 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.
> > 
> > I do not quite like acquiring resources needed in open. I think drivers should
> > do all resource acquisition in probe() and leave open()/close() to
> > activate/quiesce devices.
> 
> Ok, I'll move it back then. I'm not sure what I'm supposed to do
> in open/close then. Isn't it enough to check info->input->users
> like this?
> 
> static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
> {
>         struct exynos_adc *info = dev_id;
>         struct iio_dev *dev = dev_get_drvdata(info->dev);
>         u32 x, y;
>         bool pressed;
>         int ret;
> 
>         while (info->input->users) {
>                 ret = exynos_read_s3c64xx_ts(dev, &x, &y);
>                 if (ret == -ETIMEDOUT)
>                         break;
> 
>                 pressed = x & y & ADC_DATX_PRESSED;
>                 if (!pressed) {
>                         input_report_key(info->input, BTN_TOUCH, 0);
>                         input_sync(info->input);
>                         break;
>                 }
> 
>                 input_report_abs(info->input, ABS_X, x & ADC_DATX_MASK);
>                 input_report_abs(info->input, ABS_Y, y & ADC_DATY_MASK);
>                 input_report_key(info->input, BTN_TOUCH, 1);
>                 input_sync(info->input);
> 
>                 msleep(1);
>         };
> 
>         writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
> 
>         return IRQ_HANDLED;
> }
> 
> I could do enable_irq()/disable_irq(), but that leaves a small
> race at startup where we request the irq line and then immediately
> disable it again.

I think the above (or a separate flag in driver structure) coupled with
enable/disable IRQ is fine - input core can deal with calls to input_report_*
on devices that have been properly allocated but have not been registered yet.

drivers/input/keyboard/samsung-keypad.c does similar handling.

Thanks.

-- 
Dmitry