Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Jonathan Cameron]

On 09/11/13 09:18, Juergen Beisert wrote:
> For battery driven systems it is a very bad idea to collect the touchscreen
> data within a kernel busy loop.
> 
> This change uses the features of the hardware to delay and accumulate samples in
> hardware to avoid a high interrupt and CPU load.
> 
> Note: this is only tested on an i.MX23 SoC yet.
> 
> Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
While this driver is placed in IIO within staging at the moment, these changes are definitely
input related.  Hence I have cc'd Dmitry and the input list.

I am personaly a little uncomfortable that we have such a complex bit of input code sat
within an IIO driver but such is life.

> ---
>  drivers/staging/iio/adc/mxs-lradc.c | 530 ++++++++++++++++++++++++++++++++----
>  1 file changed, 476 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index c81b186..185f068 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -129,6 +129,17 @@ enum mxs_lradc_ts {
>  	MXS_LRADC_TOUCHSCREEN_5WIRE,
>  };
>  
> +/*
> + * Touchscreen handling
> + */
> +enum lradc_ts_plate {
> +	LRADC_TOUCH = 0,
> +	LRADC_SAMPLE_X,
> +	LRADC_SAMPLE_Y,
> +	LRADC_SAMPLE_PRESSURE,
> +	LRADC_SAMPLE_VALID,
> +};
> +
>  struct mxs_lradc {
>  	struct device		*dev;
>  	void __iomem		*base;
> @@ -169,13 +180,25 @@ struct mxs_lradc {
>  #define CHAN_MASK_TOUCHSCREEN_4WIRE	(0xf << 2)
>  #define CHAN_MASK_TOUCHSCREEN_5WIRE	(0x1f << 2)
>  	enum mxs_lradc_ts	use_touchscreen;
> -	bool			stop_touchscreen;
>  	bool			use_touchbutton;
>  
>  	struct input_dev	*ts_input;
>  	struct work_struct	ts_work;
>  
>  	enum mxs_lradc_id	soc;
> +	enum lradc_ts_plate	cur_plate; /* statemachine */
> +	bool			ts_valid;
> +	unsigned		ts_x_pos;
> +	unsigned		ts_y_pos;
> +	unsigned		ts_pressure;
> +
> +	/* handle touchscreen's physical behaviour */
> +	/* samples per coordinate */
> +	unsigned		over_sample_cnt;
> +	/* time clocks between samples */
> +	unsigned		over_sample_delay;
> +	/* time in clocks to wait after the plates where switched */
> +	unsigned		settling_delay;
>  };
>  
>  #define	LRADC_CTRL0				0x00
> @@ -227,19 +250,33 @@ struct mxs_lradc {
>  #define	LRADC_CH_ACCUMULATE			(1 << 29)
>  #define	LRADC_CH_NUM_SAMPLES_MASK		(0x1f << 24)
>  #define	LRADC_CH_NUM_SAMPLES_OFFSET		24
> +#define	LRADC_CH_NUM_SAMPLES(x) \
> +				((x) << LRADC_CH_NUM_SAMPLES_OFFSET)
>  #define	LRADC_CH_VALUE_MASK			0x3ffff
>  #define	LRADC_CH_VALUE_OFFSET			0
>  
>  #define	LRADC_DELAY(n)				(0xd0 + (0x10 * (n)))
>  #define	LRADC_DELAY_TRIGGER_LRADCS_MASK		(0xff << 24)
>  #define	LRADC_DELAY_TRIGGER_LRADCS_OFFSET	24
> +#define	LRADC_DELAY_TRIGGER(x) \
> +				(((x) << LRADC_DELAY_TRIGGER_LRADCS_OFFSET) & \
> +				LRADC_DELAY_TRIGGER_LRADCS_MASK)
>  #define	LRADC_DELAY_KICK			(1 << 20)
>  #define	LRADC_DELAY_TRIGGER_DELAYS_MASK		(0xf << 16)
>  #define	LRADC_DELAY_TRIGGER_DELAYS_OFFSET	16
> +#define	LRADC_DELAY_TRIGGER_DELAYS(x) \
> +				(((x) << LRADC_DELAY_TRIGGER_DELAYS_OFFSET) & \
> +				LRADC_DELAY_TRIGGER_DELAYS_MASK)
>  #define	LRADC_DELAY_LOOP_COUNT_MASK		(0x1f << 11)
>  #define	LRADC_DELAY_LOOP_COUNT_OFFSET		11
> +#define	LRADC_DELAY_LOOP(x) \
> +				(((x) << LRADC_DELAY_LOOP_COUNT_OFFSET) & \
> +				LRADC_DELAY_LOOP_COUNT_MASK)
>  #define	LRADC_DELAY_DELAY_MASK			0x7ff
>  #define	LRADC_DELAY_DELAY_OFFSET		0
> +#define	LRADC_DELAY_DELAY(x) \
> +				(((x) << LRADC_DELAY_DELAY_OFFSET) & \
> +				LRADC_DELAY_DELAY_MASK)
>  
>  #define	LRADC_CTRL4				0x140
>  #define	LRADC_CTRL4_LRADCSELECT_MASK(n)		(0xf << ((n) * 4))
> @@ -312,6 +349,404 @@ static u32 mxs_lradc_drive_pressure(struct mxs_lradc *lradc)
>  	return LRADC_CTRL0_MX28_YPPSW | LRADC_CTRL0_MX28_XNNSW;
>  }
>  
> +static bool mxs_lradc_check_touch_event(struct mxs_lradc *lradc)
> +{
> +	return !!(readl(lradc->base + LRADC_STATUS) &
> +					LRADC_STATUS_TOUCH_DETECT_RAW);
> +}
> +
> +static void mxs_lradc_setup_ts_channel(struct mxs_lradc *lradc, unsigned ch)
> +{
> +	/*
> +	 * prepare for oversampling conversion
> +	 *
> +	 * from the datasheet:
> +	 * "The ACCUMULATE bit in the appropriate channel register
> +	 * HW_LRADC_CHn must be set to 1 if NUM_SAMPLES is greater then 0;
> +	 * otherwise, the IRQs will not fire."
> +	 */
> +	mxs_lradc_reg(lradc, LRADC_CH_ACCUMULATE |
> +			LRADC_CH_NUM_SAMPLES(lradc->over_sample_cnt - 1),
> +			LRADC_CH(ch));
> +
> +	/* from the datasheet:
> +	 * "Software must clear this register in preparation for a
> +	 * multi-cycle accumulation.
> +	 */
> +	mxs_lradc_reg_clear(lradc, LRADC_CH_VALUE_MASK, LRADC_CH(ch));
> +
> +	/* prepare the delay/loop unit according to the oversampling count */
> +	mxs_lradc_reg(lradc, LRADC_DELAY_TRIGGER(1 << ch) |
> +		LRADC_DELAY_TRIGGER_DELAYS(0) |
> +		LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
> +		LRADC_DELAY_DELAY(lradc->over_sample_delay - 1),
> +			LRADC_DELAY(3));
> +
> +	mxs_lradc_reg_clear(lradc, LRADC_CTRL1_LRADC_IRQ(2) |
> +			LRADC_CTRL1_LRADC_IRQ(3) | LRADC_CTRL1_LRADC_IRQ(4) |
> +			LRADC_CTRL1_LRADC_IRQ(5), LRADC_CTRL1);
> +
> +	/* wake us again, when the complete conversion is done */
> +	mxs_lradc_reg_set(lradc, LRADC_CTRL1_LRADC_IRQ_EN(ch), LRADC_CTRL1);
> +	/*
> +	 * after changing the touchscreen plates setting
> +	 * the signals need some initial time to settle. Start the
> +	 * SoC's delay unit and start the conversion later
> +	 * and automatically.
> +	 */
> +	mxs_lradc_reg(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
> +		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
> +		LRADC_DELAY_KICK |
> +		LRADC_DELAY_DELAY(lradc->settling_delay),
> +			LRADC_DELAY(2));
> +}
> +
> +/*
> + * Pressure detection is special:
> + * We want to do both required measurements for the pressure detection in
> + * one turn. Use the hardware features to chain both conversions and let the
> + * hardware report one interrupt if both conversions are done
> + */
> +static void mxs_lradc_setup_ts_pressure(struct mxs_lradc *lradc, unsigned ch1,
> +							unsigned ch2)
> +{
> +	u32 reg;
> +
> +	/*
> +	 * prepare for oversampling conversion
> +	 *
> +	 * from the datasheet:
> +	 * "The ACCUMULATE bit in the appropriate channel register
> +	 * HW_LRADC_CHn must be set to 1 if NUM_SAMPLES is greater then 0;
> +	 * otherwise, the IRQs will not fire."
> +	 */
> +	reg = LRADC_CH_ACCUMULATE |
> +		LRADC_CH_NUM_SAMPLES(lradc->over_sample_cnt - 1);
> +	mxs_lradc_reg(lradc, reg, LRADC_CH(ch1));
> +	mxs_lradc_reg(lradc, reg, LRADC_CH(ch2));
> +
> +	/* from the datasheet:
> +	 * "Software must clear this register in preparation for a
> +	 * multi-cycle accumulation.
> +	 */
> +	mxs_lradc_reg_clear(lradc, LRADC_CH_VALUE_MASK, LRADC_CH(ch1));
> +	mxs_lradc_reg_clear(lradc, LRADC_CH_VALUE_MASK, LRADC_CH(ch2));
> +
> +	/* prepare the delay/loop unit according to the oversampling count */
> +	mxs_lradc_reg(lradc, LRADC_DELAY_TRIGGER(1 << ch1) |
> +		LRADC_DELAY_TRIGGER(1 << ch2) | /* start both channels */
> +		LRADC_DELAY_TRIGGER_DELAYS(0) |
> +		LRADC_DELAY_LOOP(lradc->over_sample_cnt - 1) |
> +		LRADC_DELAY_DELAY(lradc->over_sample_delay - 1),
> +					LRADC_DELAY(3));
> +
> +	mxs_lradc_reg_clear(lradc, LRADC_CTRL1_LRADC_IRQ(2) |
> +			LRADC_CTRL1_LRADC_IRQ(3) | LRADC_CTRL1_LRADC_IRQ(4) |
> +			LRADC_CTRL1_LRADC_IRQ(5), LRADC_CTRL1);
> +
> +	/* wake us again, when the conversions are done */
> +	mxs_lradc_reg_set(lradc, LRADC_CTRL1_LRADC_IRQ_EN(ch2), LRADC_CTRL1);
> +	/*
> +	 * after changing the touchscreen plates setting
> +	 * the signals need some initial time to settle. Start the
> +	 * SoC's delay unit and start the conversion later
> +	 * and automatically.
> +	 */
> +	mxs_lradc_reg(lradc, LRADC_DELAY_TRIGGER(0) | /* don't trigger ADC */
> +		LRADC_DELAY_TRIGGER_DELAYS(1 << 3) | /* trigger DELAY unit#3 */
> +		LRADC_DELAY_KICK |
> +		LRADC_DELAY_DELAY(lradc->settling_delay), LRADC_DELAY(2));
> +}
> +
> +static unsigned mxs_lradc_read_raw_channel(struct mxs_lradc *lradc,
> +							unsigned channel)
> +{
> +	u32 reg;
> +	unsigned num_samples, val;
> +
> +	reg = readl(lradc->base + LRADC_CH(channel));
> +	if (reg & LRADC_CH_ACCUMULATE)
> +		num_samples = lradc->over_sample_cnt;
> +	else
> +		num_samples = 1;
> +
> +	val = (reg & LRADC_CH_VALUE_MASK) >> LRADC_CH_VALUE_OFFSET;
> +	return val / num_samples;
> +}
> +
> +static unsigned mxs_lradc_read_ts_pressure(struct mxs_lradc *lradc,
> +						unsigned ch1, unsigned ch2)
> +{
> +	u32 reg, mask;
> +	unsigned pressure, m1, m2;
> +
> +	mask = LRADC_CTRL1_LRADC_IRQ(ch1) | LRADC_CTRL1_LRADC_IRQ(ch2);
> +	reg = readl(lradc->base + LRADC_CTRL1) & mask;
> +
> +	while (reg != mask) {
> +		reg = readl(lradc->base + LRADC_CTRL1) & mask;
> +		dev_dbg(lradc->dev, "One channel is still busy: %X\n", reg);
> +	}
> +
> +	m1 = mxs_lradc_read_raw_channel(lradc, ch1);
> +	m2 = mxs_lradc_read_raw_channel(lradc, ch2);
> +
> +	if (m2 == 0) {
> +		dev_warn(lradc->dev, "Cannot calculate pressure\n");
> +		return 1 << (LRADC_RESOLUTION - 1);
> +	}
> +
> +	/* simply scale the value from 0 ... max ADC resolution */
> +	pressure = m1;
> +	pressure *= (1 << LRADC_RESOLUTION);
> +	pressure /= m2;
> +
> +	dev_dbg(lradc->dev, "Pressure = %u\n", pressure);
> +	return pressure;
> +}
> +
> +#define TS_CH_XP 2
> +#define TS_CH_YP 3
> +#define TS_CH_XM 4
> +#define TS_CH_YM 5
> +
> +static int mxs_lradc_read_ts_channel(struct mxs_lradc *lradc)
> +{
> +	u32 reg;
> +	int val;
> +
> +	reg = readl(lradc->base + LRADC_CTRL1);
> +
> +	/* only channels 3 to 5 are of interest here */
> +	if (reg & LRADC_CTRL1_LRADC_IRQ(TS_CH_YP)) {
> +		mxs_lradc_reg_clear(lradc, LRADC_CTRL1_LRADC_IRQ_EN(TS_CH_YP) |
> +			LRADC_CTRL1_LRADC_IRQ(TS_CH_YP), LRADC_CTRL1);
> +		val = mxs_lradc_read_raw_channel(lradc, TS_CH_YP);
> +	} else if (reg & LRADC_CTRL1_LRADC_IRQ(TS_CH_XM)) {
> +		mxs_lradc_reg_clear(lradc, LRADC_CTRL1_LRADC_IRQ_EN(TS_CH_XM) |
> +			LRADC_CTRL1_LRADC_IRQ(TS_CH_XM), LRADC_CTRL1);
> +		val = mxs_lradc_read_raw_channel(lradc, TS_CH_XM);
> +	} else if (reg & LRADC_CTRL1_LRADC_IRQ(TS_CH_YM)) {
> +		mxs_lradc_reg_clear(lradc, LRADC_CTRL1_LRADC_IRQ_EN(TS_CH_YM) |
> +			LRADC_CTRL1_LRADC_IRQ(TS_CH_YM), LRADC_CTRL1);
> +		val = mxs_lradc_read_raw_channel(lradc, TS_CH_YM);
> +	} else {
> +		return -EIO;
> +	}
> +
> +	mxs_lradc_reg(lradc, 0, LRADC_DELAY(2));
> +	mxs_lradc_reg(lradc, 0, LRADC_DELAY(3));
> +
> +	return val;
> +}
> +
> +/*
> + * YP(open)--+-------------+
> + *           |             |--+
> + *           |             |  |
> + *    YM(-)--+-------------+  |
> + *             +--------------+
> + *             |              |
> + *         XP(weak+)        XM(open)
> + *
> + * "weak+" means 200k Ohm VDDIO
> + * (-) means GND
> + */
> +static void mxs_lradc_setup_touch_detection(struct mxs_lradc *lradc)
> +{
> +	/*
> +	 * In order to detect a touch event the 'touch detect enable' bit
> +	 * enables:
> +	 *  - a weak pullup to the X+ connector
> +	 *  - a strong ground at the Y- connector
> +	 */
> +	mxs_lradc_reg_clear(lradc, mxs_lradc_plate_mask(lradc), LRADC_CTRL0);
> +	mxs_lradc_reg_set(lradc, mxs_lradc_touch_detect_bit(lradc),
> +				LRADC_CTRL0);
> +}
> +
> +/*
> + * YP(meas)--+-------------+
> + *           |             |--+
> + *           |             |  |
> + * YM(open)--+-------------+  |
> + *             +--------------+
> + *             |              |
> + *           XP(+)          XM(-)
> + *
> + * (+) means here 1.85 V
> + * (-) means here GND
> + */
> +static void mxs_lradc_prepare_x_pos(struct mxs_lradc *lradc)
> +{
> +	mxs_lradc_reg_clear(lradc, mxs_lradc_plate_mask(lradc), LRADC_CTRL0);
> +	mxs_lradc_reg_set(lradc, mxs_lradc_drive_x_plate(lradc), LRADC_CTRL0);
> +
> +	lradc->cur_plate = LRADC_SAMPLE_X;
> +	mxs_lradc_setup_ts_channel(lradc, TS_CH_YP);
> +}
> +
> +/*
> + *   YP(+)--+-------------+
> + *          |             |--+
> + *          |             |  |
> + *   YM(-)--+-------------+  |
> + *            +--------------+
> + *            |              |
> + *         XP(open)        XM(meas)
> + *
> + * (+) means here 1.85 V
> + * (-) means here GND
> + */
> +static void mxs_lradc_prepare_y_pos(struct mxs_lradc *lradc)
> +{
> +	mxs_lradc_reg_clear(lradc, mxs_lradc_plate_mask(lradc), LRADC_CTRL0);
> +	mxs_lradc_reg_set(lradc, mxs_lradc_drive_y_plate(lradc), LRADC_CTRL0);
> +
> +	lradc->cur_plate = LRADC_SAMPLE_Y;
> +	mxs_lradc_setup_ts_channel(lradc, TS_CH_XM);
> +}
> +
> +/*
> + *    YP(+)--+-------------+
> + *           |             |--+
> + *           |             |  |
> + * YM(meas)--+-------------+  |
> + *             +--------------+
> + *             |              |
> + *          XP(meas)        XM(-)
> + *
> + * (+) means here 1.85 V
> + * (-) means here GND
> + */
> +static void mxs_lradc_prepare_pressure(struct mxs_lradc *lradc)
> +{
> +	mxs_lradc_reg_clear(lradc, mxs_lradc_plate_mask(lradc), LRADC_CTRL0);
> +	mxs_lradc_reg_set(lradc, mxs_lradc_drive_pressure(lradc), LRADC_CTRL0);
> +
> +	lradc->cur_plate = LRADC_SAMPLE_PRESSURE;
> +	mxs_lradc_setup_ts_pressure(lradc, TS_CH_XP, TS_CH_YM);
> +}
> +
> +static void mxs_lradc_enable_touch_detection(struct mxs_lradc *lradc)
> +{
> +	mxs_lradc_setup_touch_detection(lradc);
> +
> +	lradc->cur_plate = LRADC_TOUCH;
> +	mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ |
> +				LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
> +	mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
> +}
> +
> +static void mxs_lradc_report_ts_event(struct mxs_lradc *lradc)
> +{
> +	input_report_abs(lradc->ts_input, ABS_X, lradc->ts_x_pos);
> +	input_report_abs(lradc->ts_input, ABS_Y, lradc->ts_y_pos);
> +	input_report_abs(lradc->ts_input, ABS_PRESSURE, lradc->ts_pressure);
> +	input_report_key(lradc->ts_input, BTN_TOUCH, 1);
> +	input_sync(lradc->ts_input);
> +}
> +
> +static void mxs_lradc_complete_touch_event(struct mxs_lradc *lradc)
> +{
> +	mxs_lradc_setup_touch_detection(lradc);
> +	lradc->cur_plate = LRADC_SAMPLE_VALID;
> +	/*
> +	 * start a dummy conversion to burn time to settle the signals
> +	 * note: we are not interested in the conversion's value
> +	 */
> +	mxs_lradc_reg(lradc, 0, LRADC_CH(5));
> +	mxs_lradc_reg_clear(lradc, LRADC_CTRL1_LRADC_IRQ(5), LRADC_CTRL1);
> +	mxs_lradc_reg_set(lradc, LRADC_CTRL1_LRADC_IRQ_EN(5), LRADC_CTRL1);
> +	mxs_lradc_reg(lradc, LRADC_DELAY_TRIGGER(1 << 5) |
> +		LRADC_DELAY_KICK | LRADC_DELAY_DELAY(10), /* waste 5 ms */
> +			LRADC_DELAY(2));
> +}
> +
> +/*
> + * in order to avoid false measurements, report only samples where
> + * the surface is still touched after the position measurement
> + */
> +static void mxs_lradc_finish_touch_event(struct mxs_lradc *lradc, bool valid)
> +{
> +	/* if it is still touched, report the sample */
> +	if (valid && mxs_lradc_check_touch_event(lradc)) {
> +		lradc->ts_valid = true;
> +		mxs_lradc_report_ts_event(lradc);
> +	}
> +
> +	/* if it is even still touched, continue with the next measurement */
> +	if (mxs_lradc_check_touch_event(lradc)) {
> +		mxs_lradc_prepare_y_pos(lradc);
> +		return;
> +	}
> +
> +	if (lradc->ts_valid) {
> +		/* signal the release */
> +		lradc->ts_valid = false;
> +		input_report_key(lradc->ts_input, BTN_TOUCH, 0);
> +		input_sync(lradc->ts_input);
> +	}
> +
> +	/* if it is released, wait for the next touch via IRQ */
> +	mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ, LRADC_CTRL1);
> +	mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
> +}
> +
> +/* touchscreen's state machine */
> +static void mxs_lradc_handle_touch(struct mxs_lradc *lradc)
> +{
> +	int val;
> +
> +	switch (lradc->cur_plate) {
> +	case LRADC_TOUCH:
> +		/*
> +		 * start with the Y-pos, because it uses nearly the same plate
> +		 * settings like the touch detection
> +		 */
> +		if (mxs_lradc_check_touch_event(lradc)) {
> +			mxs_lradc_reg_clear(lradc,
> +					LRADC_CTRL1_TOUCH_DETECT_IRQ_EN,
> +					LRADC_CTRL1);
> +			mxs_lradc_prepare_y_pos(lradc);
> +		}
> +		mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ,
> +					LRADC_CTRL1);
> +		return;
> +
> +	case LRADC_SAMPLE_Y:
> +		val = mxs_lradc_read_ts_channel(lradc);
> +		if (val < 0) {
> +			mxs_lradc_enable_touch_detection(lradc); /* re-start */
> +			return;
> +		}
> +		lradc->ts_y_pos = val;
> +		mxs_lradc_prepare_x_pos(lradc);
> +		return;
> +
> +	case LRADC_SAMPLE_X:
> +		val = mxs_lradc_read_ts_channel(lradc);
> +		if (val < 0) {
> +			mxs_lradc_enable_touch_detection(lradc); /* re-start */
> +			return;
> +		}
> +		lradc->ts_x_pos = val;
> +		mxs_lradc_prepare_pressure(lradc);
> +		return;
> +
> +	case LRADC_SAMPLE_PRESSURE:
> +		lradc->ts_pressure =
> +			mxs_lradc_read_ts_pressure(lradc, TS_CH_XP, TS_CH_YM);
> +		mxs_lradc_complete_touch_event(lradc);
> +		return;
> +
> +	case LRADC_SAMPLE_VALID:
> +		val = mxs_lradc_read_ts_channel(lradc); /* ignore the value */
> +		mxs_lradc_finish_touch_event(lradc, 1);
> +		break;
> +	}
> +}
> +
>  /*
>   * Raw I/O operations
>   */
> @@ -385,15 +820,6 @@ static const struct iio_info mxs_lradc_iio_info = {
>  	.read_raw		= mxs_lradc_read_raw,
>  };
>  
> -/*
> - * Touchscreen handling
> - */
> -enum lradc_ts_plate {
> -	LRADC_SAMPLE_X,
> -	LRADC_SAMPLE_Y,
> -	LRADC_SAMPLE_PRESSURE,
> -};
> -
>  static int mxs_lradc_ts_touched(struct mxs_lradc *lradc)
>  {
>  	uint32_t reg;
> @@ -551,10 +977,6 @@ static void mxs_lradc_ts_work(struct work_struct *ts_work)
>  	input_report_key(lradc->ts_input, BTN_TOUCH, 0);
>  	input_sync(lradc->ts_input);
>  
> -	/* Do not restart the TS IRQ if the driver is shutting down. */
> -	if (lradc->stop_touchscreen)
> -		return;
> -
>  	/* Restart the touchscreen interrupts. */
>  	mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ, LRADC_CTRL1);
>  	mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
> @@ -564,36 +986,29 @@ static int mxs_lradc_ts_open(struct input_dev *dev)
>  {
>  	struct mxs_lradc *lradc = input_get_drvdata(dev);
>  
> -	/* The touchscreen is starting. */
> -	lradc->stop_touchscreen = false;
> -
>  	/* Enable the touch-detect circuitry. */
> -	mxs_lradc_reg_set(lradc, mxs_lradc_touch_detect_bit(lradc),
> -						LRADC_CTRL0);
> -
> -	/* Enable the touch-detect IRQ. */
> -	mxs_lradc_reg_set(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN, LRADC_CTRL1);
> +	mxs_lradc_enable_touch_detection(lradc);
>  
>  	return 0;
>  }
>  
> -static void mxs_lradc_ts_close(struct input_dev *dev)
> +static void mxs_lradc_disable_ts(struct mxs_lradc *lradc)
>  {
> -	struct mxs_lradc *lradc = input_get_drvdata(dev);
> -
> -	/* Indicate the touchscreen is stopping. */
> -	lradc->stop_touchscreen = true;
> -	mb();
> +	/* stop all interrupts from firing */
> +	mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN |
> +		LRADC_CTRL1_LRADC_IRQ_EN(2) | LRADC_CTRL1_LRADC_IRQ_EN(3) |
> +		LRADC_CTRL1_LRADC_IRQ_EN(4) | LRADC_CTRL1_LRADC_IRQ_EN(5),
> +		LRADC_CTRL1);
>  
> -	/* Wait until touchscreen thread finishes any possible remnants. */
> -	cancel_work_sync(&lradc->ts_work);
> +	/* Power-down touchscreen touch-detect circuitry. */
> +	mxs_lradc_reg_clear(lradc, mxs_lradc_plate_mask(lradc), LRADC_CTRL0);
> +}
>  
> -	/* Disable touchscreen touch-detect IRQ. */
> -	mxs_lradc_reg_clear(lradc, LRADC_CTRL1_TOUCH_DETECT_IRQ_EN,
> -						LRADC_CTRL1);
> +static void mxs_lradc_ts_close(struct input_dev *dev)
> +{
> +	struct mxs_lradc *lradc = input_get_drvdata(dev);
>  
> -	/* Power-down touchscreen touch-detect circuitry. */
> -	mxs_lradc_reg_clear(lradc, mxs_lradc_touch_detect_bit(lradc), LRADC_CTRL0);
> +	mxs_lradc_disable_ts(lradc);
>  }
>  
>  static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
> @@ -641,6 +1056,7 @@ static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
>  
>  	cancel_work_sync(&lradc->ts_work);
>  
> +	mxs_lradc_disable_ts(lradc);
>  	input_unregister_device(lradc->ts_input);
>  }
>  
> @@ -653,30 +1069,23 @@ static irqreturn_t mxs_lradc_handle_irq(int irq, void *data)
>  	struct mxs_lradc *lradc = iio_priv(iio);
>  	unsigned long reg = readl(lradc->base + LRADC_CTRL1);
>  	const uint32_t ts_irq_mask =
> -		LRADC_CTRL1_TOUCH_DETECT_IRQ_EN |
> -		LRADC_CTRL1_TOUCH_DETECT_IRQ;
> +		LRADC_CTRL1_TOUCH_DETECT_IRQ |
> +		LRADC_CTRL1_LRADC_IRQ(2) |
> +		LRADC_CTRL1_LRADC_IRQ(3) |
> +		LRADC_CTRL1_LRADC_IRQ(4) |
> +		LRADC_CTRL1_LRADC_IRQ(5);
>  
>  	if (!(reg & mxs_lradc_irq_mask(lradc)))
>  		return IRQ_NONE;
>  
> -	/*
> -	 * Touchscreen IRQ handling code has priority and therefore
> -	 * is placed here. In case touchscreen IRQ arrives, disable
> -	 * it ASAP
> -	 */
> -	if (reg & LRADC_CTRL1_TOUCH_DETECT_IRQ) {
> -		mxs_lradc_reg_clear(lradc, ts_irq_mask, LRADC_CTRL1);
> -		if (!lradc->stop_touchscreen)
> -			schedule_work(&lradc->ts_work);
> -	}
> +	if (lradc->use_touchscreen && (reg & ts_irq_mask))
> +		mxs_lradc_handle_touch(lradc);
>  
>  	if (iio_buffer_enabled(iio))
>  		iio_trigger_poll(iio->trig, iio_get_time_ns());
>  	else if (reg & LRADC_CTRL1_LRADC_IRQ(0))
>  		complete(&lradc->completion);
>  
> -	mxs_lradc_reg_clear(lradc, reg & mxs_lradc_irq_mask(lradc), LRADC_CTRL1);
> -
>  	return IRQ_HANDLED;
>  }
>  
> @@ -971,6 +1380,17 @@ static const struct of_device_id mxs_lradc_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, mxs_lradc_dt_ids);
>  
> +static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
> +						struct device_node *lradc_node)
> +{
> +	/* TODO retrieve from device tree */
> +	lradc->over_sample_cnt = 4;
> +	lradc->over_sample_delay = 2;
> +	lradc->settling_delay = 10;
> +
> +	return 0;
> +}
> +
>  static int mxs_lradc_probe(struct platform_device *pdev)
>  {
>  	const struct of_device_id *of_id =
> @@ -983,7 +1403,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
>  	struct iio_dev *iio;
>  	struct resource *iores;
>  	uint32_t ts_wires = 0;
> -	int ret = 0;
> +	int ret = 0, touch_ret;
>  	int i;
>  
>  	/* Allocate the IIO device. */
> @@ -1003,7 +1423,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
>  	if (IS_ERR(lradc->base))
>  		return PTR_ERR(lradc->base);
>  
> -	INIT_WORK(&lradc->ts_work, mxs_lradc_ts_work);
> +	touch_ret = mxs_lradc_probe_touchscreen(lradc, node);
>  
>  	/* Check if touchscreen is enabled in DT. */
>  	ret = of_property_read_u32(node, "fsl,lradc-touchscreen-wires",
> @@ -1066,9 +1486,11 @@ static int mxs_lradc_probe(struct platform_device *pdev)
>  		goto err_dev;
>  
>  	/* Register the touchscreen input device. */
> -	ret = mxs_lradc_ts_register(lradc);
> -	if (ret)
> -		goto err_dev;
> +	if (touch_ret == 0) {
> +		ret = mxs_lradc_ts_register(lradc);
> +		if (ret)
> +			goto err_dev;
> +	}
>  
>  	/* Register IIO device. */
>  	ret = iio_device_register(iio);
> 

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Jonathan Cameron]

On 09/15/13 11:56, Jonathan Cameron wrote:
> On 09/11/13 09:18, Juergen Beisert wrote:
>> For battery driven systems it is a very bad idea to collect the touchscreen
>> data within a kernel busy loop.
>>
>> This change uses the features of the hardware to delay and accumulate samples in
>> hardware to avoid a high interrupt and CPU load.
>>
>> Note: this is only tested on an i.MX23 SoC yet.
>>
>> Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
>> CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
>> CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> While this driver is placed in IIO within staging at the moment, these changes are definitely
> input related.  Hence I have cc'd Dmitry and the input list.
>
> I am personaly a little uncomfortable that we have such a complex bit of input code sat
> within an IIO driver but such is life.

The logic in here looks reasonable to me. I am far from a specialist in how these touch
screens are normally handled though.

One thing to note is that you really want to get a proposed device tree spec out asap
as that can take longer to review than the driver.  If you are proposing to do that as a future
patch, then take into account that you'll need to ensure these are the defaults if
it is not specified in the device tree for ever more (which is more painful than
hammering out he device tree stuff now!)
...
>> +static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
>> +						struct device_node *lradc_node)
>> +{
>> +	/* TODO retrieve from device tree */
>> +	lradc->over_sample_cnt = 4;
>> +	lradc->over_sample_delay = 2;
>> +	lradc->settling_delay = 10;
>> +
>> +	return 0;
>> +}
>> +
...

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Jürgen Beisert]

Hi Jonathan,

On Sunday 15 September 2013 18:10:18 Jonathan Cameron wrote:
> On 09/15/13 11:56, Jonathan Cameron wrote:
> > On 09/11/13 09:18, Juergen Beisert wrote:
> >> For battery driven systems it is a very bad idea to collect the
> >> touchscreen data within a kernel busy loop.
> >>
> >> This change uses the features of the hardware to delay and accumulate
> >> samples in hardware to avoid a high interrupt and CPU load.
> >>
> >> Note: this is only tested on an i.MX23 SoC yet.
> >>
> >> Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> >> CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> >> CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> >> CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >> CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> >
> > While this driver is placed in IIO within staging at the moment, these
> > changes are definitely input related.  Hence I have cc'd Dmitry and the
> > input list.
> >
> > I am personaly a little uncomfortable that we have such a complex bit of
> > input code sat within an IIO driver but such is life.
>
> The logic in here looks reasonable to me. I am far from a specialist in how
> these touch screens are normally handled though.
>
> One thing to note is that you really want to get a proposed device tree
> spec out asap as that can take longer to review than the driver.  If you
> are proposing to do that as a future patch, then take into account that
> you'll need to ensure these are the defaults if it is not specified in the
> device tree for ever more (which is more painful than hammering out he
> device tree stuff now!)
> ...

Will do.

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Jürgen Beisert]

Hi Jonathan,

On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
> On 09/11/13 09:18, Juergen Beisert wrote:
> > For battery driven systems it is a very bad idea to collect the
> > touchscreen data within a kernel busy loop.
> >
> > This change uses the features of the hardware to delay and accumulate
> > samples in hardware to avoid a high interrupt and CPU load.
> >
> > Note: this is only tested on an i.MX23 SoC yet.
> >
> > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
>
> While this driver is placed in IIO within staging at the moment, these
> changes are definitely input related.  Hence I have cc'd Dmitry and the
> input list.
>
> I am personaly a little uncomfortable that we have such a complex bit of
> input code sat within an IIO driver but such is life.

Maybe an MFD for this ADC unit would be a better way to go? Currently I have a 
different problem with this driver, because the ADC unit monitors the battery 
as well. And the charging driver from the power subsystem needs these values 
to charge the battery in a correct manner.

> [...]

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Jürgen Beisert]

Hi Jonathan,

On Monday 16 September 2013 10:10:22 Jürgen Beisert wrote:
> [...]
> > While this driver is placed in IIO within staging at the moment, these
> > changes are definitely input related.  Hence I have cc'd Dmitry and the
> > input list.
> >
> > I am personaly a little uncomfortable that we have such a complex bit of
> > input code sat within an IIO driver but such is life.
>
> Maybe an MFD for this ADC unit would be a better way to go? Currently I
> have a different problem with this driver, because the ADC unit monitors
> the battery as well. And the charging driver from the power subsystem needs
> these values to charge the battery in a correct manner.

I would suggest:

diff --git a/drivers/staging/iio/TODO b/drivers/staging/iio/TODO
index 04c2326..c22a0ed 100644
--- a/drivers/staging/iio/TODO
+++ b/drivers/staging/iio/TODO
@@ -13,6 +13,17 @@ Would be nice
 3) Expand device set. Lots of other maxim adc's have very
    similar interfaces.
 
+MXS LRADC driver:
+This is a classic MFD device as it combines the following subdevices
+ - touchscreen controller (input subsystem related device)
+ - general purpose ADC channels
+ - battery voltage monitor (power subsystem related device)
+ - die temperature monitor (thermal management)
+
+At least the battery voltage and die temperature feature is required in-kernel
+by a driver of the SoC's battery charging unit to avoid any damage to the
+silicon and the battery.
+
 TSL2561
 Would be nice
 1) Open question of userspace vs kernel space balance when

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Marek Vasut]

Dear Jürgen Beisert,

> Hi Jonathan,
> 
> On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
> > On 09/11/13 09:18, Juergen Beisert wrote:
> > > For battery driven systems it is a very bad idea to collect the
> > > touchscreen data within a kernel busy loop.
> > > 
> > > This change uses the features of the hardware to delay and accumulate
> > > samples in hardware to avoid a high interrupt and CPU load.
> > > 
> > > Note: this is only tested on an i.MX23 SoC yet.
> > > 
> > > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> > > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> > 
> > While this driver is placed in IIO within staging at the moment, these
> > changes are definitely input related.  Hence I have cc'd Dmitry and the
> > input list.
> > 
> > I am personaly a little uncomfortable that we have such a complex bit of
> > input code sat within an IIO driver but such is life.
> 
> Maybe an MFD for this ADC unit would be a better way to go? Currently I
> have a different problem with this driver, because the ADC unit monitors
> the battery as well. And the charging driver from the power subsystem
> needs these values to charge the battery in a correct manner.

Are you planning to post the power block patches too ?

Best regards,
Marek Vasut

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Jürgen Beisert]

Hi Marek,

On Monday 16 September 2013 16:23:48 Marek Vasut wrote:
> > On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
> > > On 09/11/13 09:18, Juergen Beisert wrote:
> > > > For battery driven systems it is a very bad idea to collect the
> > > > touchscreen data within a kernel busy loop.
> > > >
> > > > This change uses the features of the hardware to delay and accumulate
> > > > samples in hardware to avoid a high interrupt and CPU load.
> > > >
> > > > Note: this is only tested on an i.MX23 SoC yet.
> > > >
> > > > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > > > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > > > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> > > > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > > > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > > > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> > >
> > > While this driver is placed in IIO within staging at the moment, these
> > > changes are definitely input related.  Hence I have cc'd Dmitry and the
> > > input list.
> > >
> > > I am personaly a little uncomfortable that we have such a complex bit
> > > of input code sat within an IIO driver but such is life.
> >
> > Maybe an MFD for this ADC unit would be a better way to go? Currently I
> > have a different problem with this driver, because the ADC unit monitors
> > the battery as well. And the charging driver from the power subsystem
> > needs these values to charge the battery in a correct manner.
>
> Are you planning to post the power block patches too ?

When they will work: yes. Currently it crashes all the time. The PMIC is a 
beast...

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Marek Vasut]

Dear Jürgen Beisert,

> Hi Marek,
> 
> On Monday 16 September 2013 16:23:48 Marek Vasut wrote:
> > > On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
> > > > On 09/11/13 09:18, Juergen Beisert wrote:
> > > > > For battery driven systems it is a very bad idea to collect the
> > > > > touchscreen data within a kernel busy loop.
> > > > > 
> > > > > This change uses the features of the hardware to delay and
> > > > > accumulate samples in hardware to avoid a high interrupt and CPU
> > > > > load.
> > > > > 
> > > > > Note: this is only tested on an i.MX23 SoC yet.
> > > > > 
> > > > > Signed-off-by: Juergen Beisert <jbe@pengutronix.de>
> > > > > CC: linux-arm-kernel@lists.infradead.org
> > > > > CC: devel@driverdev.osuosl.org
> > > > > CC: Marek Vasut <marex@denx.de>
> > > > > CC: Fabio Estevam <fabio.estevam@freescale.com>
> > > > > CC: Jonathan Cameron <jic23@cam.ac.uk>
> > > > 
> > > > While this driver is placed in IIO within staging at the moment,
> > > > these changes are definitely input related.  Hence I have cc'd
> > > > Dmitry and the input list.
> > > > 
> > > > I am personaly a little uncomfortable that we have such a complex bit
> > > > of input code sat within an IIO driver but such is life.
> > > 
> > > Maybe an MFD for this ADC unit would be a better way to go? Currently I
> > > have a different problem with this driver, because the ADC unit
> > > monitors the battery as well. And the charging driver from the power
> > > subsystem needs these values to charge the battery in a correct
> > > manner.
> > 
> > Are you planning to post the power block patches too ?
> 
> When they will work: yes. Currently it crashes all the time. The PMIC is a
> beast...

Full ACK

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

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Jonathan Cameron]

"Jürgen Beisert" <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>Hi Jonathan,
>
>On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
>> On 09/11/13 09:18, Juergen Beisert wrote:
>> > For battery driven systems it is a very bad idea to collect the
>> > touchscreen data within a kernel busy loop.
>> >
>> > This change uses the features of the hardware to delay and
>accumulate
>> > samples in hardware to avoid a high interrupt and CPU load.
>> >
>> > Note: this is only tested on an i.MX23 SoC yet.
>> >
>> > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
>> > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
>> > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
>>
>> While this driver is placed in IIO within staging at the moment,
>these
>> changes are definitely input related.  Hence I have cc'd Dmitry and
>the
>> input list.
>>
>> I am personaly a little uncomfortable that we have such a complex bit
>of
>> input code sat within an IIO driver but such is life.
>
>Maybe an MFD for this ADC unit would be a better way to go?
That would be great and is definitely the preferred method.


 Currently I
>have a 
>different problem with this driver, because the ADC unit monitors the
>battery 
>as well. And the charging driver from the power subsystem needs these
>values 
>to charge the battery in a correct manner.

There is an iio client battery driver as generic_ADC_battery.c but it is currently only doing polled access. Not too hard to add buffered access though ideally we would want a generic way of combining consumers doing polled and interrupt driven accesses.
>
>> [...]
>
>Regards,
>Juergen

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Jürgen Beisert]

Hi Jonathan,

On Monday 16 September 2013 17:30:32 Jonathan Cameron wrote:
> >On Sunday 15 September 2013 12:56:25 Jonathan Cameron wrote:
> >> On 09/11/13 09:18, Juergen Beisert wrote:
> >> > For battery driven systems it is a very bad idea to collect the
> >> > touchscreen data within a kernel busy loop.
> >> >
> >> > This change uses the features of the hardware to delay and
> >> > accumulate samples in hardware to avoid a high interrupt and CPU load.
> >> >
> >> > Note: this is only tested on an i.MX23 SoC yet.
> >> >
> >> > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> >> > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> >> > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> >> > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >> > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> >>
> >> While this driver is placed in IIO within staging at the moment,
> >> these changes are definitely input related.  Hence I have cc'd Dmitry and
> >> the input list.
> >>
> >> I am personaly a little uncomfortable that we have such a complex bit
> >> of input code sat within an IIO driver but such is life.
> >
> > Maybe an MFD for this ADC unit would be a better way to go?
>
> That would be great and is definitely the preferred method.

Cannot continue to convert the driver into an MFD device. The project does not 
give me the time to do so.

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Jonathan Cameron]

.

"Jürgen Beisert" <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>Hi Jonathan,
>
>On Monday 16 September 2013 17:30:32 Jonathan Cameron wrote:
>> >On Sunday 15 September 2013 12:56:25 Jonathan ,,, wrote:
>> >> On 09/11/13 09:18, Juergen Beisert wrote:
>> >> > For battery driven systems it is a very bad idea to collect the
>> >> > touchscreen data within a kernel busy loop.
>> >> >
>> >> > This change uses the features of the hardware to delay and
>> >> > accumulate samples in hardware to avoid a high interrupt and CPU
>load.
>> >> >
>> >> > Note: this is only tested on an i.MX23 SoC yet.
>> >> >
>> >> > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> >> > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> >> > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
>> >> > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
>> >> > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> >> > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
>> >>
>> >> While this driver is placed in IIO within staging at the moment,
>> >> these changes are definitely input related.  Hence I have cc'd
>Dmitry and
>> >> the input list.
>> >>
>> >> I am personaly a little uncomfortable that we have such a complex
>bit
>> >> of input code sat within an IIO driver but such is life.
>> >
>> > Maybe an MFD for this ADC unit would be a better way to go?
>>
>> That would be great and is definitely the preferred method.
>
>Cannot continue to convert the driver into an MFD device. The project
>does not 
>give me the time to do so.
>
>Regards,
>Juergen

Fair enough. Perhaps someone else will pick up the gauntlet :) 
The old call of why there isn't enough time in the day!

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Dmitry Torokhov]

On Sun, Sep 15, 2013 at 11:56:25AM +0100, Jonathan Cameron wrote:
> On 09/11/13 09:18, Juergen Beisert wrote:
> > For battery driven systems it is a very bad idea to collect the touchscreen
> > data within a kernel busy loop.
> > 
> > This change uses the features of the hardware to delay and accumulate samples in
> > hardware to avoid a high interrupt and CPU load.
> > 
> > Note: this is only tested on an i.MX23 SoC yet.
> > 
> > Signed-off-by: Juergen Beisert <jbe-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > CC: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> > CC: devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b@public.gmane.org
> > CC: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > CC: Fabio Estevam <fabio.estevam-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > CC: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> While this driver is placed in IIO within staging at the moment, these changes are definitely
> input related.  Hence I have cc'd Dmitry and the input list.
> 
> I am personaly a little uncomfortable that we have such a complex bit of input code sat
> within an IIO driver but such is life.
> 

...

> >  
> >  static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
> > @@ -641,6 +1056,7 @@ static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
> >  
> >  	cancel_work_sync(&lradc->ts_work);
> >  
> > +	mxs_lradc_disable_ts(lradc);
> >  	input_unregister_device(lradc->ts_input);
> >  }

This looks iffy... Normally you disable the device so that it does not
generate more interrupts, and then cancel outstanding work(s), otherwise
newly generated interrupts may cause more work to be scheduled. Or I
missed some of the context and this is not a concern here?

Thanks.

-- 
Dmitry

Re: [PATCH 5/6] Staging/iio/adc/touchscreen/MXS: add interrupt driven touch detection

[Jürgen Beisert]

Hi Dmitry,

On Monday 16 September 2013 17:28:46 Dmitry Torokhov wrote:
> [...]
> > >  static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
> > > @@ -641,6 +1056,7 @@ static void mxs_lradc_ts_unregister(struct
> > > mxs_lradc *lradc)
> > >
> > >  	cancel_work_sync(&lradc->ts_work);
> > >
> > > +	mxs_lradc_disable_ts(lradc);
> > >  	input_unregister_device(lradc->ts_input);
> > >  }
>
> This looks iffy... Normally you disable the device so that it does not
> generate more interrupts, and then cancel outstanding work(s), otherwise
> newly generated interrupts may cause more work to be scheduled. Or I
> missed some of the context and this is not a concern here?

This part gets removed in patch 6/6:

@@ -1054,8 +891,6 @@ static void mxs_lradc_ts_unregister(struct mxs_lradc *lradc)
        if (!lradc->use_touchscreen)
                return;
 
-       cancel_work_sync(&lradc->ts_work);
-
        mxs_lradc_disable_ts(lradc);
        input_unregister_device(lradc->ts_input);
 }

But you are right, I should move this into patch 5/6.

Thanks

Regards,
Juergen

-- 
Pengutronix e.K.                              | Juergen Beisert             |
Linux Solutions for Science and Industry      | http://www.pengutronix.de/  |