Re: [PATCH] input/touch: Introduce the LPC32xx touchscreen controller driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Kevin,

On Wed, Aug 11, 2010 at 04:09:02PM -0700, wellsk40@xxxxxxxxx wrote:
> From: Kevin Wells <wellsk40@xxxxxxxxx>
> 
> This patch set introduces support for the LPC32xx touchscreen
> controller driver. The LPC32xx touchscreen controller supports
> automated event detection and X/Y data conversion for resistive
> touchscreens.
> 

Overall looks very nice, a few comments below.

> Signed-off-by: Kevin Wells <wellsk40@xxxxxxxxx>
> Signed-off-by: Durgesh Pattamatta <durgesh.pattamatta@xxxxxxx>
> ---
>  drivers/input/touchscreen/Kconfig      |   10 +
>  drivers/input/touchscreen/Makefile     |    1 +
>  drivers/input/touchscreen/lpc32xx_ts.c |  365 ++++++++++++++++++++++++++++++++
>  3 files changed, 376 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/input/touchscreen/lpc32xx_ts.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 61f3518..f74b6eb 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -214,6 +214,16 @@ config TOUCHSCREEN_WACOM_W8001
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called wacom_w8001.
>  
> +config TOUCHSCREEN_LPC32XX
> +	tristate "LPC32XX touchscreen controller"
> +	depends on ARCH_LPC32XX
> +	help
> +	  Say Y here if you have a LPC32XX device and want
> +	  to support the built-in touchscreen.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called lpc32xx_ts.
> +
>  config TOUCHSCREEN_MCS5000
>  	tristate "MELFAS MCS-5000 touchscreen"
>  	depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index bd6f30b..595275d 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_TOUCHSCREEN_EETI)		+= eeti_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_ELO)		+= elo.o
>  obj-$(CONFIG_TOUCHSCREEN_FUJITSU)	+= fujitsu_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_INEXIO)	+= inexio.o
> +obj-$(CONFIG_TOUCHSCREEN_LPC32XX)	+= lpc32xx_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_MC13783)	+= mc13783_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_MCS5000)	+= mcs5000_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_MIGOR)		+= migor_ts.o
> diff --git a/drivers/input/touchscreen/lpc32xx_ts.c b/drivers/input/touchscreen/lpc32xx_ts.c
> new file mode 100644
> index 0000000..ebb8f28
> --- /dev/null
> +++ b/drivers/input/touchscreen/lpc32xx_ts.c
> @@ -0,0 +1,365 @@
> +/*
> + * LPC32xx built-in touchscreen driver
> + *
> + * Copyright (C) 2010 NXP Semiconductors
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/init.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +
> +/*
> + * Touchscreen controller register offsets
> + */
> +#define TSC_STAT			0x00
> +#define TSC_SEL				0x04
> +#define TSC_CON				0x08
> +#define TSC_FIFO			0x0C
> +#define TSC_DTR				0x10
> +#define TSC_RTR				0x14
> +#define TSC_UTR				0x18
> +#define TSC_TTR				0x1C
> +#define TSC_DXP				0x20
> +#define TSC_MIN_X			0x24
> +#define TSC_MAX_X			0x28
> +#define TSC_MIN_Y			0x2C
> +#define TSC_MAX_Y			0x30
> +#define TSC_AUX_UTR			0x34
> +#define TSC_AUX_MIN			0x38
> +#define TSC_AUX_MAX			0x3C
> +
> +#define TSC_STAT_FIFO_OVRRN		(1 << 8)
> +#define TSC_STAT_FIFO_EMPTY		(1 << 7)
> +
> +#define TSC_SEL_DEFVAL			0x0284
> +
> +#define TSC_ADCCON_IRQ_TO_FIFO_4	(0x1 << 11)
> +#define TSC_ADCCON_X_SAMPLE_SIZE(s)	((10 - (s)) << 7)
> +#define TSC_ADCCON_Y_SAMPLE_SIZE(s)	((10 - (s)) << 4)
> +#define TSC_ADCCON_POWER_UP		(1 << 2)
> +#define TSC_ADCCON_AUTO_EN		(1 << 0)
> +
> +#define TSC_FIFO_TS_P_LEVEL		(1 << 31)
> +#define TSC_FIFO_NORMALIZE_X_VAL(x)	(((x) & 0x03FF0000) >> 16)
> +#define TSC_FIFO_NORMALIZE_Y_VAL(y)	((y) & 0x000003FF)
> +
> +#define TSC_ADCDAT_VALUE_MASK		0x000003FF
> +
> +#define TSC_MIN_XY_VAL			0x0
> +#define TSC_MAX_XY_VAL			0x3FF
> +
> +#define MOD_NAME "ts-lpc32xx"
> +
> +#define tsc_readl(dev, reg) \
> +	__raw_readl((dev)->tsc_base + (reg))
> +#define tsc_writel(dev, reg, val) \
> +	__raw_writel((val), (dev)->tsc_base + (reg))
> +
> +struct lpc32xx_tsc {
> +	struct input_dev *dev;
> +	void __iomem *tsc_base;
> +	int irq;
> +	struct clk *clk;
> +};
> +
> +static void lpc32xx_fifo_clear(struct lpc32xx_tsc *tsc)
> +{
> +	while (!(tsc_readl(tsc, TSC_STAT) & TSC_STAT_FIFO_EMPTY))
> +		tsc_readl(tsc, TSC_FIFO);
> +}
> +
> +static irqreturn_t lpc32xx_ts_interrupt(int irq, void *dev_id)
> +{
> +	u32 tmp, rv[4], xs[4], ys[4];
> +	int idx;
> +	struct lpc32xx_tsc *tsc = dev_id;
> +	struct input_dev *input = tsc->dev;
> +
> +	tmp = tsc_readl(tsc, TSC_STAT);
> +
> +	if (tmp & TSC_STAT_FIFO_OVRRN) {
> +		/* FIFO overflow - throw away samples */
> +		lpc32xx_fifo_clear(tsc);
> +		return IRQ_HANDLED;
> +	}
> +
> +	/*
> +	 * Gather and normalize 4 samples. Pen-up events may have less
> +	 * than 4 samples, but its ok to pop 4 and let the last sample
> +	 * pen status check drop the samples.
> +	 */
> +	idx = 0;
> +	while ((idx < 4) &&
> +		(!(tsc_readl(tsc, TSC_STAT) & TSC_STAT_FIFO_EMPTY))) {
> +		tmp = tsc_readl(tsc, TSC_FIFO);
> +		xs[idx] = TSC_ADCDAT_VALUE_MASK -
> +			TSC_FIFO_NORMALIZE_X_VAL(tmp);
> +		ys[idx] = TSC_ADCDAT_VALUE_MASK -
> +			TSC_FIFO_NORMALIZE_Y_VAL(tmp);
> +		rv[idx] = tmp;
> +		idx++;
> +	}
> +
> +	/* Data is only valid if pen is still down in last sample */
> +	if ((!(rv[3] & TSC_FIFO_TS_P_LEVEL)) && (idx == 4)) {
> +		/* Use average of 2nd and 3rd sample for position */
> +		input_report_abs(input, ABS_X, ((xs[1] + xs[2]) / 2));
> +		input_report_abs(input, ABS_Y, ((ys[1] + ys[2]) / 2));
> +		input_report_key(input, BTN_TOUCH, 1);
> +	} else
> +		input_report_key(input, BTN_TOUCH, 0);
> +
> +	input_sync(input);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void stop_tsc(struct lpc32xx_tsc *tsc)
> +{
> +	/* Disable auto mode */
> +	tsc_writel(tsc, TSC_CON,
> +		   tsc_readl(tsc, TSC_CON) & ~TSC_ADCCON_AUTO_EN);
> +}
> +

Hmm, that looks like good candidate for input_dev->close() method. And
setup_tsc() should be ->open(). Btw, maybe move clk_disable() here as
well?

> +static void setup_tsc(struct lpc32xx_tsc *tsc)
> +{

Keep the common prefix (lpc32xx_) perhaps?

> +	u32 tmp;
> +
> +	tmp = tsc_readl(tsc, TSC_CON) & ~TSC_ADCCON_POWER_UP;
> +
> +	/* Set the TSC FIFO depth to 4 samples @ 10-bits per sample (max) */
> +	tmp = (TSC_ADCCON_IRQ_TO_FIFO_4 |
> +		TSC_ADCCON_X_SAMPLE_SIZE(10) |
> +		TSC_ADCCON_Y_SAMPLE_SIZE(10));
> +	tsc_writel(tsc, TSC_CON, tmp);
> +
> +	/* These values are all preset */
> +	tsc_writel(tsc, TSC_SEL, TSC_SEL_DEFVAL);
> +	tsc_writel(tsc, TSC_MIN_X, TSC_MIN_XY_VAL);
> +	tsc_writel(tsc, TSC_MAX_X, TSC_MAX_XY_VAL);
> +	tsc_writel(tsc, TSC_MIN_Y, TSC_MIN_XY_VAL);
> +	tsc_writel(tsc, TSC_MAX_Y, TSC_MAX_XY_VAL);
> +
> +	/* Aux support is not used */
> +	tsc_writel(tsc, TSC_AUX_UTR, 0);
> +	tsc_writel(tsc, TSC_AUX_MIN, 0);
> +	tsc_writel(tsc, TSC_AUX_MAX, 0);
> +
> +	/*
> +	 * Set sample rate to about 240Hz per X/Y pair. A single measurement
> +	 * consists of 4 pairs which gives about a 60Hz sample rate based on
> +	 * a stable 32768Hz clock source. Values are in clocks.
> +	 * Rate is (32768 / (RTR + XCONV + RTR + YCONV + DXP + TTR + UTR) / 4
> +	 */
> +	tsc_writel(tsc, TSC_RTR, 0x2);
> +	tsc_writel(tsc, TSC_DTR, 0x2);
> +	tsc_writel(tsc, TSC_TTR, 0x10);
> +	tsc_writel(tsc, TSC_DXP, 0x4);
> +	tsc_writel(tsc, TSC_UTR, 88);
> +
> +	lpc32xx_fifo_clear(tsc);
> +
> +	/* Enable automatic ts event capture */
> +	tsc_writel(tsc, TSC_CON, tmp | TSC_ADCCON_AUTO_EN);
> +}
> +
> +static int __devinit lpc32xx_ts_probe(struct platform_device *pdev)
> +{
> +	struct lpc32xx_tsc *tsc;
> +	struct resource *res;
> +	resource_size_t size;
> +	int retval;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Can't get memory resource\n");
> +		return -ENOENT;
> +	}
> +
> +	retval = platform_get_irq(pdev, 0);
> +	if (retval < 0) {
> +		dev_err(&pdev->dev, "Can't get interrupt resource\n");
> +		return retval;
> +	}
> +
> +	tsc = devm_kzalloc(&pdev->dev, sizeof(*tsc), GFP_KERNEL);
> +	if (unlikely(!tsc)) {
> +		dev_err(&pdev->dev, "failed allocating memory\n");
> +		return -ENOMEM;
> +	}
> +	tsc->irq = retval;
> +
> +	size = resource_size(res);
> +
> +	if (!devm_request_mem_region(&pdev->dev, res->start, size,
> +				     pdev->name)) {
> +		dev_err(&pdev->dev, "TSC registers are not free\n");
> +		return -EBUSY;
> +	}
> +
> +	tsc->tsc_base = devm_ioremap(&pdev->dev, res->start, size);
> +	if (!tsc->tsc_base) {
> +		dev_err(&pdev->dev, "Can't map memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	tsc->dev = input_allocate_device();
> +	if (!tsc->dev) {
> +		dev_err(&pdev->dev, "failed allocating input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	tsc->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(tsc->clk)) {
> +		dev_err(&pdev->dev, "failed getting clock\n");
> +		retval = -ENOENT;
> +		goto err_no_clk;
> +	}
> +
> +	clk_enable(tsc->clk);
> +	setup_tsc(tsc);
> +
> +	platform_set_drvdata(pdev, tsc);
> +
> +	tsc->dev->name = MOD_NAME;
> +	tsc->dev->phys = "lpc32xx/input0";
> +	tsc->dev->id.bustype = BUS_HOST;
> +	tsc->dev->id.vendor = 0x0001;
> +	tsc->dev->id.product = 0x0002;
> +	tsc->dev->id.version = 0x0100;
> +	tsc->dev->dev.parent = &pdev->dev;
> +
> +	tsc->dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> +	tsc->dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> +	input_set_abs_params(tsc->dev, ABS_X, TSC_MIN_XY_VAL,
> +			     TSC_MAX_XY_VAL, 0, 0);
> +	input_set_abs_params(tsc->dev, ABS_Y, TSC_MIN_XY_VAL,
> +			     TSC_MAX_XY_VAL, 0, 0);
> +
> +	if (input_register_device(tsc->dev)) {
> +		dev_err(&pdev->dev, "failed registering input device\n");
> +		goto err_stop_clk;

retval is garbage here, you need to do:

	error = input_register_device();
	if (error) {
		...
		goto err_stop_clk;
	}

I must say that I do not like mixing devm_* with the standard error path
unwinding, if we can rely on devm_xxx() calls to take care of everything
we should revert to standard error unsinding practice for everythng.

> +	}
> +
> +	retval = devm_request_irq(&pdev->dev, tsc->irq, lpc32xx_ts_interrupt,
> +		IRQF_DISABLED, pdev->name, tsc);
> +	if (retval < 0) {
> +		dev_err(&pdev->dev, "failed requesting interrupt\n");
> +		goto err_no_irq;
> +	}
> +
> +	device_init_wakeup(&pdev->dev, 1);
> +
> +	return 0;
> +
> +err_no_irq:
> +	input_unregister_device(tsc->dev);

You'll fall through to input_free_device() which is forbidden after
calling input_unregister_device(). Add tsc->dev = NULL to avoid this
issue.
 
> +
> +err_stop_clk:
> +	stop_tsc(tsc);
> +	clk_disable(tsc->clk);
> +	clk_put(tsc->clk);
> +
> +err_no_clk:
> +	input_free_device(tsc->dev);
> +
> +	return retval;
> +}
> +
> +static int __devexit lpc32xx_ts_remove(struct platform_device *pdev)
> +{
> +	struct lpc32xx_tsc *tsc = platform_get_drvdata(pdev);
> +
> +	device_init_wakeup(&pdev->dev, 0);
> +	disable_irq(tsc->irq);
> +
> +	input_unregister_device(tsc->dev);
> +
> +	stop_tsc(tsc);
> +	clk_disable(tsc->clk);
> +	clk_put(tsc->clk);
> +
> +	input_free_device(tsc->dev);

Again, do not call input_free_device() here, input_unregister_device()
will free the structure (if it holds the last reference).


> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int lpc32xx_ts_suspend(struct device *dev)
> +{
> +	struct lpc32xx_tsc *tsc = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		enable_irq_wake(tsc->irq);
> +	else {
> +		stop_tsc(tsc);
> +		clk_disable(tsc->clk);
> +	}
> +
> +	return 0;
> +}
> +
> +static int lpc32xx_ts_resume(struct device *dev)
> +{
> +	struct lpc32xx_tsc *tsc = dev_get_drvdata(dev);
> +
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(tsc->irq);
> +	else {
> +		clk_enable(tsc->clk);
> +		setup_tsc(tsc);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops lpc32xx_ts_pm_ops = {
> +	.suspend	= lpc32xx_ts_suspend,
> +	.resume		= lpc32xx_ts_resume,
> +};
> +#define LPC32XX_TS_PM_OPS (&lpc32xx_ts_pm_ops)
> +#else
> +#define LPC32XX_TS_PM_OPS NULL
> +#endif
> +
> +static struct platform_driver lpc32xx_ts_driver = {
> +	.probe		= lpc32xx_ts_probe,
> +	.remove		= __devexit_p(lpc32xx_ts_remove),
> +	.driver		= {
> +		.name	= MOD_NAME,
> +		.owner	= THIS_MODULE,
> +		.pm	= LPC32XX_TS_PM_OPS,
> +	},
> +};
> +
> +static int __init lpc32xx_ts_init(void)
> +{
> +	return platform_driver_register(&lpc32xx_ts_driver);
> +}
> +module_init(lpc32xx_ts_init);
> +
> +static void __exit lpc32xx_ts_exit(void)
> +{
> +	platform_driver_unregister(&lpc32xx_ts_driver);
> +}
> +module_exit(lpc32xx_ts_exit);
> +
> +MODULE_AUTHOR("Kevin Wells <kevin.wells@xxxxxxx");
> +MODULE_DESCRIPTION("LPC32XX TSC Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:lpc32xx_ts");
> -- 
> 1.7.1.1
> 

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux