On Saturday 21 November 2015 17:48:10 Jonathan Cameron wrote:
> On 16/11/15 12:01, Markus Pargmann wrote:
> > This is a driver for the imx25 ADC/TSC module. It controls the
> > touchscreen conversion queue and creates a touchscreen input device.
> > The driver currently only supports 4 wire touchscreens. The driver uses
> > a simple conversion queue of precharge, touch detection, X measurement,
> > Y measurement, precharge and another touch detection.
> >
> > This driver uses the regmap from the parent to setup some touch specific
> > settings in the core driver and setup a idle configuration with touch
> > detection.
> >
> > Signed-off-by: Markus Pargmann <mpa@xxxxxxxxxxxxxx>
> > Signed-off-by: Denis Carikli <denis@xxxxxxxxxx>
> >
> > [fix clock's period calculation]
> > [fix calculation of the 'settling' value]
> > Signed-off-by: Juergen Borleis <jbe@xxxxxxxxxxxxxx>
> I read this out of curiousity to see how you had handled the touchscreen
> usecase of this hardware differently from the generic version.
>
> Anyhow, a few little bits and pieces inline from me as a result
>
> Jonathan
> > ---
> >
> > Notes:
> > Changes in v7:
> > - Moved clk_prepare_enable() and mx25_tcq_init() into mx25_tcq_open(). This
> > was done to be able to use devm_request_threaded_irq().
> > - Cleanup of the probe function through above change
> > - Removed mx25_tcq_remove(), not necessary now
> >
> > drivers/input/touchscreen/Kconfig | 6 +
> > drivers/input/touchscreen/Makefile | 1 +
> > drivers/input/touchscreen/fsl-imx25-tcq.c | 600 ++++++++++++++++++++++++++++++
> > 3 files changed, 607 insertions(+)
> > create mode 100644 drivers/input/touchscreen/fsl-imx25-tcq.c
> >
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index ae33da7ab51f..b44651d33080 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -811,6 +811,12 @@ config TOUCHSCREEN_USB_COMPOSITE
> > To compile this driver as a module, choose M here: the
> > module will be called usbtouchscreen.
> >
> > +config TOUCHSCREEN_MX25
> > + tristate "Freescale i.MX25 touchscreen input driver"
> > + depends on MFD_MX25_TSADC
> > + help
> > + Enable support for touchscreen connected to your i.MX25.
> > +
> > config TOUCHSCREEN_MC13783
> > tristate "Freescale MC13783 touchscreen input driver"
> > depends on MFD_MC13XXX
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index cbaa6abb08da..77a2ac54101a 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_INTEL_MID) += intel-mid-touch.o
> > obj-$(CONFIG_TOUCHSCREEN_IPROC) += bcm_iproc_tsc.o
> > obj-$(CONFIG_TOUCHSCREEN_LPC32XX) += lpc32xx_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_MAX11801) += max11801_ts.o
> > +obj-$(CONFIG_TOUCHSCREEN_MX25) += fsl-imx25-tcq.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/fsl-imx25-tcq.c b/drivers/input/touchscreen/fsl-imx25-tcq.c
> > new file mode 100644
> > index 000000000000..c833cd814972
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/fsl-imx25-tcq.c
> > @@ -0,0 +1,600 @@
> > +/*
> > + * Copyright (C) 2014-2015 Pengutronix, Markus Pargmann <mpa@xxxxxxxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it under
> > + * the terms of the GNU General Public License version 2 as published by the
> > + * Free Software Foundation.
> > + *
> > + * Based on driver from 2011:
> > + * Juergen Beisert, Pengutronix <kernel@xxxxxxxxxxxxxx>
> > + *
> > + * This is the driver for the imx25 TCQ (Touchscreen Conversion Queue)
> > + * connected to the imx25 ADC.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/imx25-tsadc.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +static const char mx25_tcq_name[] = "mx25-tcq";
> > +
> > +enum mx25_tcq_mode {
> > + MX25_TS_4WIRE,
> > +};
> > +
> > +struct mx25_tcq_priv {
> > + struct regmap *regs;
> > + struct regmap *core_regs;
> > + struct input_dev *idev;
> > + enum mx25_tcq_mode mode;
> > + unsigned int pen_threshold;
> > + unsigned int sample_count;
> > + unsigned int expected_samples;
> > + unsigned int pen_debounce;
> > + unsigned int settling_time;
> > + struct clk *clk;
> > + int irq;
> > +};
> > +
> > +static struct regmap_config mx25_tcq_regconfig = {
> > + .fast_io = true,
> > + .max_register = 0x5c,
> > + .reg_bits = 32,
> > + .val_bits = 32,
> > + .reg_stride = 4,
> > +};
> > +
> > +static const struct of_device_id mx25_tcq_ids[] = {
> > + { .compatible = "fsl,imx25-tcq", },
> > + { /* Sentinel */ }
> > +};
> > +
> > +#define TSC_4WIRE_PRE_INDEX 0
> > +#define TSC_4WIRE_X_INDEX 1
> > +#define TSC_4WIRE_Y_INDEX 2
> > +#define TSC_4WIRE_POST_INDEX 3
> > +#define TSC_4WIRE_LEAVE 4
> > +
> > +#define MX25_TSC_DEF_THRESHOLD 80
> > +#define TSC_MAX_SAMPLES 16
> > +
> > +#define MX25_TSC_REPEAT_WAIT 14
> > +
> > +enum mx25_adc_configurations {
> > + MX25_CFG_PRECHARGE = 0,
> > + MX25_CFG_TOUCH_DETECT,
> > + MX25_CFG_X_MEASUREMENT,
> > + MX25_CFG_Y_MEASUREMENT,
> > +};
> > +
> > +#define MX25_PRECHARGE_VALUE (\
> > + MX25_ADCQ_CFG_YPLL_OFF | \
> > + MX25_ADCQ_CFG_XNUR_OFF | \
> > + MX25_ADCQ_CFG_XPUL_HIGH | \
> > + MX25_ADCQ_CFG_REFP_INT | \
> > + MX25_ADCQ_CFG_IN_XP | \
> > + MX25_ADCQ_CFG_REFN_NGND2 | \
> > + MX25_ADCQ_CFG_IGS)
> > +
> > +#define MX25_TOUCH_DETECT_VALUE (\
> > + MX25_ADCQ_CFG_YNLR | \
> > + MX25_ADCQ_CFG_YPLL_OFF | \
> > + MX25_ADCQ_CFG_XNUR_OFF | \
> > + MX25_ADCQ_CFG_XPUL_OFF | \
> > + MX25_ADCQ_CFG_REFP_INT | \
> > + MX25_ADCQ_CFG_IN_XP | \
> > + MX25_ADCQ_CFG_REFN_NGND2 | \
> > + MX25_ADCQ_CFG_PENIACK)
> > +
> > +static void imx25_setup_queue_cfgs(struct mx25_tcq_priv *priv,
> > + unsigned int settling_cnt)
> > +{
> > + u32 precharge_cfg =
> > + MX25_PRECHARGE_VALUE |
> > + MX25_ADCQ_CFG_SETTLING_TIME(settling_cnt);
> > + u32 touch_detect_cfg =
> > + MX25_TOUCH_DETECT_VALUE |
> > + MX25_ADCQ_CFG_NOS(1) |
> > + MX25_ADCQ_CFG_SETTLING_TIME(settling_cnt);
> > +
> > + regmap_write(priv->core_regs, MX25_TSC_TICR, precharge_cfg);
> > +
> > + /* PRECHARGE */
> > + regmap_write(priv->regs, MX25_ADCQ_CFG(MX25_CFG_PRECHARGE),
> > + precharge_cfg);
> > +
> > + /* TOUCH_DETECT */
> > + regmap_write(priv->regs, MX25_ADCQ_CFG(MX25_CFG_TOUCH_DETECT),
> > + touch_detect_cfg);
> > +
> > + /* X Measurement */
> > + regmap_write(priv->regs, MX25_ADCQ_CFG(MX25_CFG_X_MEASUREMENT),
> > + MX25_ADCQ_CFG_YPLL_OFF |
> > + MX25_ADCQ_CFG_XNUR_LOW |
> > + MX25_ADCQ_CFG_XPUL_HIGH |
> > + MX25_ADCQ_CFG_REFP_XP |
> > + MX25_ADCQ_CFG_IN_YP |
> > + MX25_ADCQ_CFG_REFN_XN |
> > + MX25_ADCQ_CFG_NOS(priv->sample_count) |
> > + MX25_ADCQ_CFG_SETTLING_TIME(settling_cnt));
> > +
> > + /* Y Measurement */
> > + regmap_write(priv->regs, MX25_ADCQ_CFG(MX25_CFG_Y_MEASUREMENT),
> > + MX25_ADCQ_CFG_YNLR |
> > + MX25_ADCQ_CFG_YPLL_HIGH |
> > + MX25_ADCQ_CFG_XNUR_OFF |
> > + MX25_ADCQ_CFG_XPUL_OFF |
> > + MX25_ADCQ_CFG_REFP_YP |
> > + MX25_ADCQ_CFG_IN_XP |
> > + MX25_ADCQ_CFG_REFN_YN |
> > + MX25_ADCQ_CFG_NOS(priv->sample_count) |
> > + MX25_ADCQ_CFG_SETTLING_TIME(settling_cnt));
> > +
> > + /* Enable the touch detection right now */
> > + regmap_write(priv->core_regs, MX25_TSC_TICR, touch_detect_cfg |
> > + MX25_ADCQ_CFG_IGS);
> > +}
> > +
> > +static int imx25_setup_queue_4wire(struct mx25_tcq_priv *priv,
> > + unsigned settling_cnt, int *items)
> > +{
> > + imx25_setup_queue_cfgs(priv, settling_cnt);
> > +
> > + /* Setup the conversion queue */
> > + regmap_write(priv->regs, MX25_ADCQ_ITEM_7_0,
> > + MX25_ADCQ_ITEM(0, MX25_CFG_PRECHARGE) |
> > + MX25_ADCQ_ITEM(1, MX25_CFG_TOUCH_DETECT) |
> > + MX25_ADCQ_ITEM(2, MX25_CFG_X_MEASUREMENT) |
> > + MX25_ADCQ_ITEM(3, MX25_CFG_Y_MEASUREMENT) |
> > + MX25_ADCQ_ITEM(4, MX25_CFG_PRECHARGE) |
> > + MX25_ADCQ_ITEM(5, MX25_CFG_TOUCH_DETECT));
> > +
> > + /*
> > + * We measure X/Y with 'sample_count' number of samples and execute a
> > + * touch detection twice, with 1 sample each
> > + */
> > + priv->expected_samples = priv->sample_count * 2 + 2;
> > + *items = 6;
> > +
> > + return 0;
> > +}
> > +
> Another personal preference. I'd not bother wrapping these single line
> calls up but rather just make them inline. They don't in of
> themselves add much to my mind. Still this one is very much up to you
> as far as I'm concerned.
> > +static void mx25_tcq_disable_touch_irq(struct mx25_tcq_priv *priv)
> > +{
> > + regmap_update_bits(priv->regs, MX25_ADCQ_CR, MX25_ADCQ_CR_PDMSK,
> > + MX25_ADCQ_CR_PDMSK);
> > +}
> > +
> > +static void mx25_tcq_enable_touch_irq(struct mx25_tcq_priv *priv)
> > +{
> > + regmap_update_bits(priv->regs, MX25_ADCQ_CR, MX25_ADCQ_CR_PDMSK, 0);
> > +}
> > +
> > +static void mx25_tcq_disable_fifo_irq(struct mx25_tcq_priv *priv)
> > +{
> > + regmap_update_bits(priv->regs, MX25_ADCQ_MR, MX25_ADCQ_MR_FDRY_IRQ,
> > + MX25_ADCQ_MR_FDRY_IRQ);
> > +}
> > +
> > +static void mx25_tcq_enable_fifo_irq(struct mx25_tcq_priv *priv)
> > +{
> > + regmap_update_bits(priv->regs, MX25_ADCQ_MR, MX25_ADCQ_MR_FDRY_IRQ, 0);
> > +}
> > +
> > +static void mx25_tcq_force_queue_start(struct mx25_tcq_priv *priv)
> > +{
> > + regmap_update_bits(priv->regs, MX25_ADCQ_CR,
> > + MX25_ADCQ_CR_FQS,
> > + MX25_ADCQ_CR_FQS);
> > +}
> > +
> > +static void mx25_tcq_force_queue_stop(struct mx25_tcq_priv *priv)
> > +{
> > + regmap_update_bits(priv->regs, MX25_ADCQ_CR,
> > + MX25_ADCQ_CR_FQS, 0);
> > +}
> > +
> > +static void mx25_tcq_fifo_reset(struct mx25_tcq_priv *priv)
> > +{
> > + u32 tcqcr;
> > +
> > + regmap_read(priv->regs, MX25_ADCQ_CR, &tcqcr);
> > + regmap_update_bits(priv->regs, MX25_ADCQ_CR, MX25_ADCQ_CR_FRST,
> > + MX25_ADCQ_CR_FRST);
> > + regmap_update_bits(priv->regs, MX25_ADCQ_CR, MX25_ADCQ_CR_FRST, 0);
> > + regmap_write(priv->regs, MX25_ADCQ_CR, tcqcr);
> > +}
> > +
> > +static void mx25_tcq_re_enable_touch_detection(struct mx25_tcq_priv *priv)
> > +{
> > + /* stop the queue from looping */
> > + mx25_tcq_force_queue_stop(priv);
> > +
> > + /* for a clean touch detection, preload the X plane */
> > + regmap_write(priv->core_regs, MX25_TSC_TICR, MX25_PRECHARGE_VALUE);
> > +
> > + /* waste some time now to pre-load the X plate to high voltage */
> > + mx25_tcq_fifo_reset(priv);
> > +
> > + /* re-enable the detection right now */
> > + regmap_write(priv->core_regs, MX25_TSC_TICR,
> > + MX25_TOUCH_DETECT_VALUE | MX25_ADCQ_CFG_IGS);
> > +
> > + regmap_update_bits(priv->regs, MX25_ADCQ_SR, MX25_ADCQ_SR_PD,
> > + MX25_ADCQ_SR_PD);
> > +
> > + /* enable the pen down event to be a source for the interrupt */
> > + regmap_update_bits(priv->regs, MX25_ADCQ_MR, MX25_ADCQ_MR_PD_IRQ, 0);
> > +
> > + /* lets fire the next IRQ if someone touches the touchscreen */
> > + mx25_tcq_enable_touch_irq(priv);
> > +}
> > +
> > +static int mx25_tcq_create_event_for_4wire(struct mx25_tcq_priv *priv,
> > + u32 *sample_buf,
> > + unsigned int samples)
> > +{
> > + unsigned int x_pos = 0;
> > + unsigned int y_pos = 0;
> > + unsigned int touch_pre = 0;
> > + unsigned int touch_post = 0;
> > + unsigned int i;
> > + int ret = 0;
> > +
> > + for (i = 0; i < samples; i++) {
> > + unsigned int index = MX25_ADCQ_FIFO_ID(sample_buf[i]);
> > + unsigned int val = MX25_ADCQ_FIFO_DATA(sample_buf[i]);
> > +
> > + switch (index) {
> > + case 1:
> > + touch_pre = val;
> > + break;
> > + case 2:
> > + x_pos = val;
> > + break;
> > + case 3:
> > + y_pos = val;
> > + break;
> > + case 5:
> > + touch_post = val;
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > + }
> > +
> > + if (ret == 0 && samples != 0) {
> > + /*
> > + * only if both touch measures are below a threshold,
> > + * the position is valid
> > + */
> > + if (touch_pre < priv->pen_threshold &&
> > + touch_post < priv->pen_threshold) {
> > + /* valid samples, generate a report */
> > + x_pos /= priv->sample_count;
> > + y_pos /= priv->sample_count;
> > + input_report_abs(priv->idev, ABS_X, x_pos);
> > + input_report_abs(priv->idev, ABS_Y, y_pos);
> > + input_report_key(priv->idev, BTN_TOUCH, 1);
> > + input_sync(priv->idev);
> > +
> > + /* get next sample */
> > + mx25_tcq_enable_fifo_irq(priv);
> > + } else if (touch_pre >= priv->pen_threshold &&
> > + touch_post >= priv->pen_threshold) {
> > + /*
> > + * if both samples are invalid,
> > + * generate a release report
> > + */
> > + input_report_key(priv->idev, BTN_TOUCH, 0);
> > + input_sync(priv->idev);
> > + mx25_tcq_re_enable_touch_detection(priv);
> > + } else {
> > + /*
> > + * if only one of both touch measurements are
> > + * below the threshold, still some bouncing
> > + * happens. Take additional samples in this
> > + * case to be sure
> > + */
> > + mx25_tcq_enable_fifo_irq(priv);
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static irqreturn_t mx25_tcq_irq_thread(int irq, void *dev_id)
> > +{
> > + struct mx25_tcq_priv *priv = dev_id;
> > + u32 sample_buf[TSC_MAX_SAMPLES];
> > + unsigned int samples = 0;
> > +
> > + /* read all samples */
> It's not a terribly big fifo, so I'm not convinced personally
> of the merits of having the separate thread for this interrupt...
Yes that is correct, but it is calling into the input framework as well.
I am not sure how much work is done there. Also the thread shouldn't be
extremely costy as it is mostly sleeping?
>
> > + while (1) {
> > + u32 stats;
> > +
> > + regmap_read(priv->regs, MX25_ADCQ_SR, &stats);
> > + if (stats & MX25_ADCQ_SR_EMPT)
> > + break;
> > +
> > + if (samples < TSC_MAX_SAMPLES) {
> > + regmap_read(priv->regs, MX25_ADCQ_FIFO,
> > + &sample_buf[samples]);
> > + ++samples;
> > + } else {
> > + u32 discarded;
> > + /* discard samples */
> > + regmap_read(priv->regs, MX25_ADCQ_FIFO, &discarded);
> Comment on how this could happen would be good.
This could happen if there are new samples while we read all samples
from the fifo.
Thanks to your comment I had a closer look on this handler again. The
problem here is that we read x samples while x may be a different number
than the number of samples generated by the conversion queue. But only
want to read samples that were produced by exactly one conversion queue
run.
So I redesigned this handler to first read the number of samples in the
fifo and then read as many as we want.
>
> > + }
> > + }
> > +
> > + mx25_tcq_create_event_for_4wire(priv, sample_buf, samples);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t mx25_tcq_irq(int irq, void *dev_id)
> > +{
> > + struct mx25_tcq_priv *priv = dev_id;
> > + u32 stat;
> > + int ret = IRQ_HANDLED;
> > +
> > + regmap_read(priv->regs, MX25_ADCQ_SR, &stat);
> > +
> > + if (stat & (MX25_ADCQ_SR_FRR | MX25_ADCQ_SR_FUR | MX25_ADCQ_SR_FOR))
> > + mx25_tcq_fifo_reset(priv);
> Again, currious cross comparison with the adc driver (hardware is pretty
> much the same ;) In there you don't reset the fifo if you get one
> of these. Why not? Now you should never get one as you only ever schedule
> a single reading, but best to be consistent.
Yes, the conversion queue in the touch driver is running continously so
that this may happen. Actually a fifo reset is not enough here. For the
touch we need to reset the conversion queue as well, thanks.
This could be added to the gcq driver but this should never happen.
>
> > +
> > + if (stat & MX25_ADCQ_SR_PD) {
> > + mx25_tcq_disable_touch_irq(priv);
> > + mx25_tcq_force_queue_start(priv);
> > + mx25_tcq_enable_fifo_irq(priv);
> > + }
> > +
> > + if (stat & MX25_ADCQ_SR_FDRY) {
> > + mx25_tcq_disable_fifo_irq(priv);
> I'm missing something I think.. In a oneshot irq the irq will be masked
> anyway till the thread is done. Why the explicit disable as well?
The driver which receives the interrupts for both units GCQ and TCQ has
a proper interrupt status register. It uses this register to distribute
them to the different units. The problem is that it doesn't have a mask
register. The interrupts can only be masked in the TCQ or GCQ unit. So
by default the interrupt for both units will be masked.
GCQ currently doesn't use any time-critical interrupts. But waiting for
the thread shouldn't block the other unit. So I think it's better to
mask the interrupt here explicitly. This of course should not be with
IRQF_ONESHOT. That's a mistake, will fix that.
>
> > + ret = IRQ_WAKE_THREAD;
> > + }
> > +
> > + regmap_update_bits(priv->regs, MX25_ADCQ_SR, MX25_ADCQ_SR_FRR |
> > + MX25_ADCQ_SR_FUR | MX25_ADCQ_SR_FOR |
> > + MX25_ADCQ_SR_PD | MX25_ADCQ_SR_EOQ,
> > + MX25_ADCQ_SR_FRR | MX25_ADCQ_SR_FUR |
> > + MX25_ADCQ_SR_FOR | MX25_ADCQ_SR_PD |
> > + MX25_ADCQ_SR_EOQ);
> As with the adc driver you are clearing at least one bit that should
> not I think ever be set... EOQ. Why?
Indeed, this is always masked. Thanks, removed that.
> > +
> > + return ret;
> > +}
> > +
> > +/* configure the statemachine for a 4-wire touchscreen */
> state machine
> > +static int mx25_tcq_init(struct mx25_tcq_priv *priv)
> > +{
> > + u32 tgcr;
> > + unsigned int ipg_div;
> > + unsigned int adc_period;
> > + unsigned int debounce_cnt;
> > + unsigned int settling_cnt;
> > + int itemct;
> > + int ret;
> > +
> > + regmap_read(priv->core_regs, MX25_TSC_TGCR, &tgcr);
> > + ipg_div = max_t(unsigned int, 4, MX25_TGCR_GET_ADCCLK(tgcr));
> > + adc_period = USEC_PER_SEC * ipg_div * 2 + 2;
> > + adc_period /= clk_get_rate(priv->clk) / 1000 + 1;
> > + debounce_cnt = DIV_ROUND_UP(priv->pen_debounce, adc_period * 8) - 1;
> > + settling_cnt = DIV_ROUND_UP(priv->settling_time, adc_period * 8) - 1;
> > +
> > + /* Reset */
> > + regmap_write(priv->regs, MX25_ADCQ_CR,
> > + MX25_ADCQ_CR_QRST | MX25_ADCQ_CR_FRST);
> > + regmap_update_bits(priv->regs, MX25_ADCQ_CR,
> > + MX25_ADCQ_CR_QRST | MX25_ADCQ_CR_FRST, 0);
> > +
> > + /* up to 128 * 8 ADC clocks are possible */
> > + if (debounce_cnt > 127)
> > + debounce_cnt = 127;
> > +
> > + /* up to 255 * 8 ADC clocks are possible */
> > + if (settling_cnt > 255)
> > + settling_cnt = 255;
> > +
> > + ret = imx25_setup_queue_4wire(priv, settling_cnt, &itemct);
> > + if (ret)
> > + return ret;
> > +
> > + regmap_update_bits(priv->regs, MX25_ADCQ_CR,
> > + MX25_ADCQ_CR_LITEMID_MASK | MX25_ADCQ_CR_WMRK_MASK,
> > + MX25_ADCQ_CR_LITEMID(itemct - 1) |
> > + MX25_ADCQ_CR_WMRK(priv->expected_samples - 1));
> > +
> > + /* setup debounce count */
> > + regmap_update_bits(priv->core_regs, MX25_TSC_TGCR,
> > + MX25_TGCR_PDBTIME_MASK,
> > + MX25_TGCR_PDBTIME(debounce_cnt));
> > +
> > + /* enable debounce */
> > + regmap_update_bits(priv->core_regs, MX25_TSC_TGCR, MX25_TGCR_PDBEN,
> > + MX25_TGCR_PDBEN);
> > + regmap_update_bits(priv->core_regs, MX25_TSC_TGCR, MX25_TGCR_PDEN,
> > + MX25_TGCR_PDEN);
> > +
> > + /* enable the engine on demand */
> > + regmap_update_bits(priv->regs, MX25_ADCQ_CR, MX25_ADCQ_CR_QSM_MASK,
> > + MX25_ADCQ_CR_QSM_FQS);
> > +
> > + /* Enable repeat and repeat wait */
> > + regmap_update_bits(priv->regs, MX25_ADCQ_CR,
> > + MX25_ADCQ_CR_RPT | MX25_ADCQ_CR_RWAIT_MASK,
> > + MX25_ADCQ_CR_RPT |
> > + MX25_ADCQ_CR_RWAIT(MX25_TSC_REPEAT_WAIT));
> > +
> > + mx25_tcq_re_enable_touch_detection(priv);
> > +
> > + return 0;
> > +}
> > +
> > +static int mx25_tcq_parse_dt(struct platform_device *pdev,
> > + struct mx25_tcq_priv *priv)
> > +{
> > + struct device_node *np = pdev->dev.of_node;
> > + u32 wires;
> > + int ret;
> > +
> > + /* Setup defaults */
> > + priv->pen_threshold = 500;
> > + priv->sample_count = 3;
> > + priv->pen_debounce = 1000000;
> > + priv->settling_time = 250000;
> > +
> > + ret = of_property_read_u32(np, "fsl,wires", &wires);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Failed to find fsl,wires properties\n");
> > + return ret;
> > + }
> > +
> > + if (wires == 4) {
> > + priv->mode = MX25_TS_4WIRE;
> > + } else {
> > + dev_err(&pdev->dev, "%u-wire mode not supported\n", wires);
> > + return -EINVAL;
> > + }
> > +
> > + /* These are optional, we don't care about the return values */
> > + of_property_read_u32(np, "fsl,pen-threshold", &priv->pen_threshold);
> > + of_property_read_u32(np, "fsl,settling-time", &priv->settling_time);
> > + of_property_read_u32(np, "fsl,pen-debounce", &priv->pen_debounce);
> > +
> > + return 0;
> > +}
> > +
> > +static int mx25_tcq_open(struct input_dev *idev)
> > +{
> > + struct device *dev = &idev->dev;
> > + struct mx25_tcq_priv *priv = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + ret = clk_prepare_enable(priv->clk);
> > + if (ret) {
> > + dev_err(dev, "Failed to enable ipg clock\n");
> > + return ret;
> > + }
> > +
> > + ret = mx25_tcq_init(priv);
> > + if (ret) {
> There is a certain missbalance in naming at least between the open
> and the close. I'd be tempted to reorganise the two so that
> they obviously balance.. Either split up the init into the various
> things such as enabling the interrupts and bringing the queue up
> (so the oposite of the remove) or wrap the remove calls up in an
> _exit which can easily be compared to the init)
>
> This definitely comes in the category of writing the code so it
> is 'obviously correct', rather than making review harder!
>
> There is also some stuff in that init - like enabling debounce that
> isn't undone in the close - to my mind that suggests it doesn't
> belong in this function, but rather in device startup, but I may
> well be missing some interactions in the hardware that prevent this.
Yes, init() is more of a reset_configure_and_init() function. I will try
to make this more obvious.
>
> > + dev_err(dev, "Failed to init tcq\n");
> > + clk_disable_unprepare(priv->clk);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void mx25_tcq_close(struct input_dev *idev)
> > +{
> > + struct mx25_tcq_priv *priv = input_get_drvdata(idev);
> > +
> > + mx25_tcq_force_queue_stop(priv);
> > + mx25_tcq_disable_touch_irq(priv);
> > + mx25_tcq_disable_fifo_irq(priv);
> > + clk_disable_unprepare(priv->clk);
> > +}
> > +
> > +static int mx25_tcq_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct input_dev *idev;
> > + struct mx25_tcq_priv *priv;
> > + struct mx25_tsadc *tsadc = dev_get_drvdata(pdev->dev.parent);
> > + struct resource *res;
> > + void __iomem *mem;
> > + int ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + mem = devm_ioremap_resource(dev, res);
> > + if (!mem)
> > + return -ENOMEM;
> > +
> > + ret = mx25_tcq_parse_dt(pdev, priv);
> > + if (ret)
> > + return ret;
> > +
> > + priv->regs = devm_regmap_init_mmio(dev, mem, &mx25_tcq_regconfig);
> > + if (IS_ERR(priv->regs)) {
> > + dev_err(dev, "Failed to initialize regmap\n");
> > + return PTR_ERR(priv->regs);
> > + }
> > +
> > + priv->irq = platform_get_irq(pdev, 0);
> > + if (priv->irq <= 0) {
> > + dev_err(dev, "Failed to get IRQ\n");
> > + return priv->irq;
> > + }
> > +
> > + idev = devm_input_allocate_device(dev);
> > + if (!idev) {
> > + dev_err(dev, "Failed to allocate input device\n");
> > + return -ENOMEM;
> > + }
> > +
> > + idev->name = mx25_tcq_name;
> > + idev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
> > + idev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
> > + input_set_abs_params(idev, ABS_X, 0, 0xfff, 0, 0);
> > + input_set_abs_params(idev, ABS_Y, 0, 0xfff, 0, 0);
> > +
> > + idev->id.bustype = BUS_HOST;
> > + idev->open = mx25_tcq_open;
> > + idev->close = mx25_tcq_close;
> > +
> > + priv->idev = idev;
> > + input_set_drvdata(idev, priv);
> > +
> > + priv->core_regs = tsadc->regs;
> > + if (!priv->core_regs)
> > + return -EINVAL;
> > +
> > + priv->clk = tsadc->clk;
> > + if (!priv->clk)
> > + return -EINVAL;
> > +
> > + platform_set_drvdata(pdev, priv);
> > +
> > + ret = input_register_device(idev);
> I'm not 100% sure of whether input works the way I think it does ;)
> but I'd imagine this just exposed the userspace interface. Hence from
> here on you can open the device and cause the interrupt to be enabled.
> It doesn't have a handler until after the request_threaded_irq device
> below... So nasty if unlikely race condition here.
Yes, fixed as Dmitry suggested.
>
> > + if (ret) {
> > + dev_err(dev, "Failed to register input device\n");
> > + return ret;
> > + }
> > +
> > + ret = devm_request_threaded_irq(dev, priv->irq, mx25_tcq_irq,
> > + mx25_tcq_irq_thread, IRQF_ONESHOT,
> > + pdev->name, priv);
> currious difference in approach here. Why a devm call in this driver
> and not in the adc one? I assumed general paranoia with devm irq requests
> and the various obscure ways they can go wrong so didn't mention it there...
For the gcq driver we have things are a bit different. There is no real
open()/close() function. We configure the unit once (including
interrupts) and want to be careful when shutting down the driver to do
it in the right order of disabling clocks, interrupts and regulators.
The interrupts there are actually never masked there.
In this tcq driver the interrupts are masked and clocks are off when the
input was closed.
Thanks,
Markus
> > + if (ret) {
> > + dev_err(dev, "Failed requesting IRQ\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver mx25_tcq_driver = {
> > + .driver = {
> > + .name = "mx25-tcq",
> > + .of_match_table = mx25_tcq_ids,
> > + },
> > + .probe = mx25_tcq_probe,
> > +};
> > +module_platform_driver(mx25_tcq_driver);
> > +
> > +MODULE_DESCRIPTION("TS input driver for Freescale mx25");
> > +MODULE_AUTHOR("Markus Pargmann <mpa@xxxxxxxxxxxxxx>");
> > +MODULE_LICENSE("GPL v2");
> >
>
>
--
Pengutronix e.K. | |
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 |
Attachment:
signature.asc
Description: This is a digitally signed message part.
