From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Luotao Fu <l.fu@pengutronix.de>
Cc: Rabin VINCENT <rabin.vincent@stericsson.com>,
Samuel Ortiz <sameo@linux.intel.com>,
Linus WALLEIJ <linus.walleij@stericsson.com>,
linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
STEricsson_nomadik_linux <STEricsson_nomadik_linux@list.st.com>
Subject: Re: [PATCH 5/5] input: STMPE touch controller support
Date: Thu, 24 Jun 2010 09:24:07 -0700 [thread overview]
Message-ID: <20100624162407.GA18899@core.coreip.homeip.net> (raw)
In-Reply-To: <1277389575-574-1-git-send-email-l.fu@pengutronix.de>
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.
> +
> +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.
> +{
> + 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.
*/
> + 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?
> +}
> +
> +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?
> +
> + 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.
> +
> + 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().
> +
> + 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().
> + 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?
> +};
> +
> +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@pengutronix.de>");
> +MODULE_DESCRIPTION("STMPEXXX touchscreen driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" STMPE_TS_NAME);
> --
> 1.7.1
>
Thanks.
--
Dmitry
next prev parent reply other threads:[~2010-06-24 16:32 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-05-31 12:17 [PATCH 1/3] mfd: add STMPExxxx I/O Expander support Rabin Vincent
2010-05-31 12:17 ` [PATCH 2/3] gpio: add STMPExxxx GPIO driver Rabin Vincent
2010-05-31 12:17 ` [PATCH 3/3] input: add STMPExxxx keypad driver Rabin Vincent
2010-06-01 22:16 ` Dmitry Torokhov
2010-06-02 13:56 ` Rabin VINCENT
2010-06-02 16:05 ` Dmitry Torokhov
2010-06-18 23:42 ` [PATCH 1/3] mfd: add STMPExxxx I/O Expander support Samuel Ortiz
2010-06-19 13:50 ` Luotao Fu
2010-06-21 13:33 ` Rabin VINCENT
2010-06-21 15:45 ` Luotao Fu
2010-06-22 13:55 ` [PATCHv2 1/3] mfd: add STMPE " Rabin Vincent
2010-06-27 23:55 ` Samuel Ortiz
2010-06-29 3:13 ` Rabin VINCENT
2010-06-29 15:33 ` Samuel Ortiz
2010-07-01 12:00 ` [PATCHv3 " Rabin Vincent
2010-07-01 12:34 ` Luotao Fu
2010-07-02 11:22 ` [PATCHv4 " Rabin Vincent
2010-07-02 15:31 ` Samuel Ortiz
2010-07-02 11:22 ` [PATCHv4 2/3] gpio: add STMPE GPIO driver Rabin Vincent
2010-07-02 11:22 ` [PATCHv4 3/3] input: add STMPE keypad driver Rabin Vincent
2010-07-02 12:10 ` [RESEND] [PATCH V8] input: STMPE touch controller support Luotao Fu
2010-07-05 14:53 ` Samuel Ortiz
2010-07-01 12:00 ` [PATCHv3 2/3] gpio: add STMPE GPIO driver Rabin Vincent
2010-07-01 12:29 ` Luotao Fu
2010-07-01 12:00 ` [PATCHv3 3/3] input: add STMPE keypad driver Rabin Vincent
2010-06-22 13:55 ` [PATCHv2 2/3] gpio: add STMPE GPIO driver Rabin Vincent
2010-06-22 13:55 ` [PATCHv2 3/3] input: add STMPE keypad driver Rabin Vincent
2010-06-22 13:56 ` [PATCH 1/3] mfd: add STMPExxxx I/O Expander support Rabin VINCENT
2010-06-24 11:13 ` mfd: STMPExxxx fixes and touch screen support Luotao Fu
2010-06-24 11:13 ` [PATCH 1/6] gpio/stmpe-gpio: set GPIO alternate function while requesting Luotao Fu
2010-06-24 12:43 ` Rabin VINCENT
2010-06-24 11:13 ` [PATCH 2/6] gpio/stmpe-gpio: fix set direction input Luotao Fu
2010-06-24 12:03 ` Rabin VINCENT
2010-06-24 11:13 ` [PATCH 3/6] mfd/stmpexxx: add touchscreen platform data Luotao Fu
2010-06-24 11:13 ` [PATCH 4/6] mfd/stmpexxx: change touchscreen irq Luotao Fu
2010-06-24 13:09 ` Rabin VINCENT
2010-06-24 13:17 ` Luotao Fu
2010-06-24 11:13 ` [PATCH 5/6] mfd/stmpexxx: fix stmpe811 enable hook Luotao Fu
2010-06-24 12:11 ` Rabin VINCENT
2010-06-24 12:32 ` Luotao Fu
2010-06-24 12:47 ` [PATCH 5/6 V3] " Luotao Fu
2010-06-24 13:05 ` Rabin VINCENT
2010-06-24 11:13 ` [PATCH 6/6 V4] input: STMPE touch controller support Luotao Fu
2010-06-24 12:27 ` [PATCH 5/6 V2] mfd/stmpexxx: fix stmpe811 enable hook Luotao Fu
2010-06-24 12:35 ` Rabin VINCENT
2010-06-24 12:46 ` Luotao Fu
2010-06-24 12:28 ` [PATCH 6/6 V5] input: STMPE touch controller support Luotao Fu
2010-06-24 14:26 ` [PATCH 5/5] " Luotao Fu
2010-06-24 16:24 ` Dmitry Torokhov [this message]
2010-06-24 16:57 ` Luotao Fu
2010-06-25 8:37 ` [PATCH 5/5 V7] " Luotao Fu
2010-06-25 9:11 ` Dmitry Torokhov
2010-06-25 9:32 ` Luotao Fu
2010-06-27 21:24 ` Samuel Ortiz
2010-06-25 9:34 ` [PATCH 5/5 V8] " Luotao Fu
2010-06-24 12:31 ` [PATCH 6/6 V4] " Rabin VINCENT
2010-06-24 12:42 ` Luotao Fu
2010-06-24 13:01 ` Rabin VINCENT
2010-06-24 13:11 ` Luotao Fu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100624162407.GA18899@core.coreip.homeip.net \
--to=dmitry.torokhov@gmail.com \
--cc=STEricsson_nomadik_linux@list.st.com \
--cc=l.fu@pengutronix.de \
--cc=linus.walleij@stericsson.com \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rabin.vincent@stericsson.com \
--cc=sameo@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).