Message-ID: <20100624165751.GA17112@pengutronix.de>
Date:	Thu, 24 Jun 2010 18:57:51 +0200
From:	Luotao Fu <l.fu@...gutronix.de>
To:	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	Luotao Fu <l.fu@...gutronix.de>,
	Rabin VINCENT <rabin.vincent@...ricsson.com>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Linus WALLEIJ <linus.walleij@...ricsson.com>,
	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
	STEricsson_nomadik_linux <STEricsson_nomadik_linux@...t.st.com>
Subject: Re: [PATCH 5/5] input: STMPE touch controller support

Hi Dmitry,

thanks for reviewing the patch.

On Thu, Jun 24, 2010 at 09:24:07AM -0700, Dmitry Torokhov wrote:
> Hi Luotao,
> 
> On Thu, Jun 24, 2010 at 04:26:15PM +0200, Luotao Fu wrote:
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 497964a..2691c62 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -47,3 +47,4 @@ obj-$(CONFIG_TOUCHSCREEN_WM97XX_MAINSTONE)	+= mainstone-wm97xx.o
> >  obj-$(CONFIG_TOUCHSCREEN_WM97XX_ZYLONITE)	+= zylonite-wm97xx.o
> >  obj-$(CONFIG_TOUCHSCREEN_W90X900)	+= w90p910_ts.o
> >  obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
> > +obj-$(CONFIG_TOUCHSCREEN_STMPE)		+= stmpe-ts.o
> 
> Please keep Makefile ordered alphabetically, it makes it easier to merge
> stuff. Might try to do the same for Kconfig as well.
> 

OK, will fix.
> > +
> > +static int inline __stmpe_reset_fifo(struct stmpe *stmpe)
> 
> Do not mark private function inline unless absolutely necessary - let
> compiler figure out whether it makes sense to inline and what.
> 
OK, will fix.
> > +{
> > +	int ret;
> > +
> > +	ret = stmpe_set_bits(stmpe, STMPE_REG_FIFO_STA,
> > +			STMPE_FIFO_STA_RESET, STMPE_FIFO_STA_RESET);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return stmpe_set_bits(stmpe, STMPE_REG_FIFO_STA,
> > +			STMPE_FIFO_STA_RESET, 0);
> > +}
> > +
> > +static void stmpe_work(struct work_struct *work)
> > +{
> > +	int int_sta;
> > +	u32 timeout = 40;
> > +
> > +	struct stmpe_touch *ts =
> > +	    container_of(work, struct stmpe_touch, work.work);
> > +
> > +	int_sta = stmpe_reg_read(ts->stmpe, STMPE_REG_INT_STA);
> > +
> > +	/* touch_det sometimes get desasserted or just get stuck. This appears
> > +	 * to be a silicon bug, We still have to clearify this with the
> > +	 * manufacture. As a workaround We release the key anyway if the
> > +	 * touch_det keeps coming in after 4ms, while the FIFO contains no value
> > +	 * during the whole time. */
> 
> I would appreciate if we could maintain the following format for
> multiline comments (within input at least):
> 
> /*
>  * This is a long long
>  * long comment.
>  */
> 

OK, will fix.
> > +	while ((int_sta & (1 << STMPE_IRQ_TOUCH_DET)) && (timeout > 0)) {
> > +		timeout--;
> > +		int_sta = stmpe_reg_read(ts->stmpe, STMPE_REG_INT_STA);
> > +		udelay(100);
> > +	}
> > +
> > +	/* reset the FIFO before we report release event */
> > +	__stmpe_reset_fifo(ts->stmpe);
> > +
> > +	input_report_abs(ts->idev, ABS_PRESSURE, 0);
> > +	input_sync(ts->idev);
> 
> Hmm, this function appears to be always reporting release, no matter
> what the state of hardware is. I do not think this was the intention...
> or was it?
> 

It unfortunately is. The original idea was polling for touch_det
and only report a release if the timeout was not reached. Unfortunately
the chip just sometime runs amok and never clears the touch_det
carelessly what happend. The only way here is report release any way
both after successed poll or timeout. This works fine, though the
polling sure looks unneccessary. I, however, would like to keep this
since I suppose a silicon bug here. Until the issue is cleared with the
vendor. Hence I'd like to have the polling for wait_det here though it
is sometimes useless.

> > +}
> > +
> > +static irqreturn_t stmpe_ts_handler(int irq, void *data)
> > +{
> > +	u8 data_set[4];
> > +	int x, y, z;
> > +	struct stmpe_touch *ts = data;
> > +
> > +	/* Cancel scheduled polling for release if we have new value
> > +	 * available. Wait if the polling is already running. */
> > +	cancel_delayed_work_sync(&ts->work);
> > +
> > +	/*
> > +	 * The FIFO sometimes just crashes and stops generating interrupts. This
> > +	 * appears to be a silicon bug. We still have to clearify this with
> > +	 * the manufacture. As a workaround we disable the TSC while we are
> > +	 * collecting data and flush the FIFO after reading
> > +	 */
> > +	stmpe_set_bits(ts->stmpe, STMPE_REG_TSC_CTRL,
> > +				STMPE_TSC_CTRL_TSC_EN, 0);
> > +
> > +	stmpe_block_read(ts->stmpe, STMPE_REG_TSC_DATA_XYZ, 4, data_set);
> > +
> > +	x = (data_set[0] << 4) | (data_set[1] >> 4);
> > +	y = ((data_set[1] & 0xf) << 8) | data_set[2];
> > +	z = data_set[3];
> > +
> > +	input_report_abs(ts->idev, ABS_X, x);
> > +	input_report_abs(ts->idev, ABS_Y, y);
> > +	input_report_abs(ts->idev, ABS_PRESSURE, z);
> > +	input_sync(ts->idev);
> > +
> > +       /* flush the FIFO after we have read out our values. */
> > +	__stmpe_reset_fifo(ts->stmpe);
> > +
> > +	/* reenable the tsc */
> > +	stmpe_set_bits(ts->stmpe, STMPE_REG_TSC_CTRL,
> > +			STMPE_TSC_CTRL_TSC_EN, STMPE_TSC_CTRL_TSC_EN);
> > +
> > +	/* start polling for touch_det to detect release */
> > +	schedule_delayed_work(&ts->work, HZ / 50);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int stmpe_ts_open(struct input_dev *dev)
> > +{
> > +	struct stmpe_touch *ts = input_get_drvdata(dev);
> > +	int ret = 0;
> > +
> > +	ret = __stmpe_reset_fifo(ts->stmpe);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return stmpe_set_bits(ts->stmpe, STMPE_REG_TSC_CTRL,
> > +			STMPE_TSC_CTRL_TSC_EN, STMPE_TSC_CTRL_TSC_EN);
> > +}
> > +
> > +static void stmpe_ts_close(struct input_dev *dev)
> > +{
> > +	struct stmpe_touch *ts = input_get_drvdata(dev);
> > +
> > +	stmpe_set_bits(ts->stmpe, STMPE_REG_TSC_CTRL,
> > +			STMPE_TSC_CTRL_TSC_EN, 0);
> > +}
> > +
> > +static int __devinit stmpe_input_probe(struct platform_device *pdev)
> > +{
> > +	struct stmpe *stmpe = dev_get_drvdata(pdev->dev.parent);
> > +	struct stmpe_platform_data *pdata = stmpe->pdata;
> > +	struct stmpe_touch *ts;
> > +	struct input_dev *idev;
> > +	struct stmpe_ts_platform_data *ts_pdata = NULL;
> > +	int ret = 0;
> > +	unsigned int ts_irq;
> > +	u8 adc_ctrl1, adc_ctrl1_mask, tsc_cfg, tsc_cfg_mask;
> > +
> > +	ts_irq = platform_get_irq_byname(pdev, "FIFO_TH");
> > +	if (ts_irq < 0)
> > +		return ts_irq;
> > +
> > +	ts = kzalloc(sizeof(*ts), GFP_KERNEL);
> > +	if (!ts)
> > +		goto err_out;
> > +
> > +	idev = input_allocate_device();
> > +	if (!idev)
> > +		goto err_free_ts;
> > +
> > +	platform_set_drvdata(pdev, ts);
> > +	ts->stmpe = stmpe;
> > +	ts->idev = idev;
> > +
> > +	if (pdata)
> > +		ts_pdata = pdata->ts;
> > +
> > +	if (ts_pdata) {
> > +		ts->sample_time = ts_pdata->sample_time;
> > +		ts->mod_12b = ts_pdata->mod_12b;
> > +		ts->ref_sel = ts_pdata->ref_sel;
> > +		ts->adc_freq = ts_pdata->adc_freq;
> > +		ts->ave_ctrl = ts_pdata->ave_ctrl;
> > +		ts->touch_det_delay = ts_pdata->touch_det_delay;
> > +		ts->settling = ts_pdata->settling;
> > +		ts->fraction_z = ts_pdata->fraction_z;
> > +		ts->i_drive = ts_pdata->i_drive;
> > +	}
> > +
> > +	INIT_DELAYED_WORK(&ts->work, stmpe_work);
> > +
> > +	ret = request_threaded_irq(ts_irq, NULL, stmpe_ts_handler,
> > +			IRQF_ONESHOT, STMPE_TS_NAME, ts);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to request IRQ %d\n", ts_irq);
> > +		goto err_free_input;
> > +	}
> > +
> > +	ret = stmpe_enable(stmpe, STMPE_BLOCK_TOUCHSCREEN | STMPE_BLOCK_ADC);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Could not enable clock for ADC and TS\n");
> > +		goto err_free_irq;
> > +	}
> > +
> > +	adc_ctrl1 = SAMPLE_TIME(ts->sample_time) | MOD_12B(ts->mod_12b) |
> > +		REF_SEL(ts->ref_sel);
> > +	adc_ctrl1_mask = SAMPLE_TIME(0xff) | MOD_12B(0xff) | REF_SEL(0xff);
> > +	ret = stmpe_set_bits(stmpe, STMPE_REG_ADC_CTRL1,
> > +			adc_ctrl1_mask, adc_ctrl1);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Could not setup ADC\n");
> > +		goto err_free_irq;
> > +	}
> > +
> > +	ret = stmpe_set_bits(stmpe, STMPE_REG_ADC_CTRL2,
> > +			ADC_FREQ(0xff), ADC_FREQ(ts->adc_freq));
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Could not setup ADC\n");
> > +		goto err_free_irq;
> > +	}
> > +
> > +	tsc_cfg = AVE_CTRL(ts->ave_ctrl) | DET_DELAY(ts->touch_det_delay) |
> > +			SETTLING(ts->settling);
> > +	tsc_cfg_mask = AVE_CTRL(0xff) | DET_DELAY(0xff) | SETTLING(0xff);
> > +
> > +	ret = stmpe_set_bits(stmpe, STMPE_REG_TSC_CFG, tsc_cfg_mask, tsc_cfg);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Could not config touch\n");
> > +		goto err_free_irq;
> > +	}
> > +
> > +	ret = stmpe_set_bits(stmpe, STMPE_REG_TSC_FRACTION_Z,
> > +			FRACTION_Z(0xff), FRACTION_Z(ts->fraction_z));
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Could not config touch\n");
> > +		goto err_free_irq;
> > +	}
> > +
> > +	ret = stmpe_set_bits(stmpe, STMPE_REG_TSC_I_DRIVE,
> > +			I_DRIVE(0xff), I_DRIVE(ts->i_drive));
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Could not config touch\n");
> > +		goto err_free_irq;
> > +	}
> > +
> > +	/* set FIFO to 1 for single point reading */
> > +	ret = stmpe_reg_write(stmpe, STMPE_REG_FIFO_TH, 1);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Could not set FIFO\n");
> > +		goto err_free_irq;
> > +	}
> > +
> > +	ret = stmpe_set_bits(stmpe, STMPE_REG_TSC_CTRL,
> > +			OP_MODE(0xff), OP_MODE(OP_MOD_XYZ));
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Could not set mode\n");
> > +		goto err_free_irq;
> > +	}
> 
> 
> Could we pull all these stmpe_set_bits(...) settig up the hardware into
> a separate function?
> 

OK, will add a call ala stmpe_init_hw().

> > +
> > +	idev->name = STMPE_TS_NAME;
> > +	idev->id.bustype = BUS_I2C;
> > +	idev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> > +	idev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> > +
> > +	idev->open = stmpe_ts_open;
> > +	idev->close = stmpe_ts_close;
> > +
> > +	input_set_drvdata(idev, ts);
> > +
> > +	ret = input_register_device(idev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Could not register input device\n");
> > +		goto err_free_irq;
> > +	}
> > +
> > +	input_set_abs_params(idev, ABS_X, 0, XY_MASK, 0, 0);
> > +	input_set_abs_params(idev, ABS_Y, 0, XY_MASK, 0, 0);
> > +	input_set_abs_params(idev, ABS_PRESSURE, 0x0, 0xff, 0, 0);
> 
> These calls better happen before registering the device.
> 

OK. will fix.

> > +
> > +	return ret;
> > +
> > +err_free_irq:
> > +	free_irq(ts_irq, ts);
> > +err_free_input:
> > +	input_free_device(idev);
> > +	platform_set_drvdata(pdev, NULL);
> > +err_free_ts:
> > +	kfree(ts);
> > +err_out:
> > +	return ret;
> > +}
> > +
> > +static int __devexit stmpe_ts_remove(struct platform_device *pdev)
> > +{
> > +	struct stmpe_touch *ts = platform_get_drvdata(pdev);
> > +	unsigned int ts_irq = platform_get_irq_byname(pdev, "FIFO_TH");
> > +
> > +	cancel_delayed_work(&ts->work);
> 
> cancel_delayed_work_sync().
> 

Ah, forgot this, will fix.

> > +
> > +	stmpe_reg_write(ts->stmpe, STMPE_REG_FIFO_TH, 0);
> > +
> > +	stmpe_disable(ts->stmpe, STMPE_BLOCK_TOUCHSCREEN);
> > +
> 
> I'd expect the 3 calls above being part of stmpe_ts_close().
> 

OK, will fix.

> > +	free_irq(ts_irq, ts);
> > +
> > +	platform_set_drvdata(pdev, NULL);
> > +
> > +	input_unregister_device(ts->idev);
> > +	input_free_device(ts->idev);
> > +
> > +	kfree(ts);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver stmpe_ts_driver = {
> > +	.driver = {
> > +		   .name = STMPE_TS_NAME,
> 
> 		.owner = THIS_MODULE,
> 
> > +		   },
> > +	.probe = stmpe_input_probe,
> > +	.remove = __devexit_p(stmpe_ts_remove),
> 
> No PM? Or simply not yet?
> 

not yet. ;-) will add this in a later patch.

> > +};
> > +
> > +static int __init stmpe_ts_init(void)
> > +{
> > +	return platform_driver_register(&stmpe_ts_driver);
> > +}
> > +
> > +module_init(stmpe_ts_init);
> > +
> > +static void __exit stmpe_ts_exit(void)
> > +{
> > +	platform_driver_unregister(&stmpe_ts_driver);
> > +}
> > +
> > +module_exit(stmpe_ts_exit);
> > +
> > +MODULE_AUTHOR("Luotao Fu <l.fu@...gutronix.de>");
> > +MODULE_DESCRIPTION("STMPEXXX touchscreen driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:" STMPE_TS_NAME);
> > -- 
> > 1.7.1
> > 
> 
> Thanks.

thanks

> 
> -- 
> Dmitry

cheers
Luotao Fu
-- 
Pengutronix e.K.                           | Dipl.-Ing. Luotao Fu        |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Download attachment "signature.asc" of type "application/pgp-signature" (198 bytes)

Powered by blists - more mailing lists