| Message ID | 5456679.XxlLIvc3Q6@wuerfel |
|---|---|
| State | Superseded, archived |
| Headers | show |
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, > 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
Jonathan Cameron schrieb: > 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, > >> 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; Since info->read_ts is of type bool, use true/false. >> + >> + 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; Just return ret, without goto (and get rid of label 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; >> + Probably better to get rid of the labels and move the code up, as it is only used once. >> +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; What about other possible error codes? >> + >> + 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 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday 20 July 2014 23:37:18 Hartmut Knaack wrote: > Jonathan Cameron schrieb: > > On 18/07/14 20:29, Arnd Bergmann wrote: > >> - 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). Ok, I see. That's exactly the information I was looking for. > > > > 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. Ok. > >> - 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. Ok. > >> 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. I expected the input stuff to just be left out by the compiler if CONFIG_INPUT is not set. I'll try it out and change it if necessary. > >> 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; Ok. > >> 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; > Since info->read_ts is of type bool, use true/false. Ok > >> + > >> + 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? I'm not entirely sure about why we pass 0 here, it's either channel zero being used for touchscreen, or the channel number being ignore after we write to the TSC register above. I copied it from the original driver, but it might be helpful if someone with access to the specs could comment here. I'll add a macro for now. > >> + > >> + 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. Ok, adding ts_x and ts_y members. > >> + 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. Agreed. I thought about it, but then kept it as it was in the original driver. Will change now. > >> + writel(ADC_TSC_WAIT4INT | ADC_S3C2443_TSC_UD_SEN, ADC_V1_TSC(info->regs)); > > Perhaps the above is cryptic enough to warrant a comment? This is also taken directly from the old driver, I have no idea what it really does... > >> + /* 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; > Just return ret, without goto (and get rid of label err). ok > >> + } > >> + > >> + 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; > >> + > Probably better to get rid of the labels and move the code up, as it is only used once. Ok. I try not to mix goto and early return, so I've removed all the labels here. > >> 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; > What about other possible error codes? We handle them later, in the request_threaded_irq call. In particular, if the touchscreen is not used because either the "has-touchscreen" flag is not set or because the input layer is not available, failing to get the irq line should not be treated as an error. Checking -EPROBE_DEFER however makes sense, so we get out of the probe function early and don't have to undo and later retry everything. Thanks everybody for the review! Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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> #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; 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); + + 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; + 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; + 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; + } /* 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); + 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); + 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);
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. 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. - 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. - 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> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html