Re: [PATCH 12/14] iio: adc: at91-sama5d2_adc: support for position and pressure channels

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

 




On Fri, 22 Dec 2017 17:07:19 +0200
Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx> wrote:

> The ADC IP supports position and pressure measurements for a touchpad
> connected on channels 0,1,2,3 for a 4-wire touchscreen with pressure
> measurement support.
> Using the inkern API, a driver can request a trigger and read the
> channel values from the ADC.
> The implementation provides a trigger named "touch" which can be
> connected to a consumer driver.
> Once a driver connects and attaches a pollfunc to this trigger, the
> configure trigger callback is called, and then the ADC driver will
> initialize pad measurement.
> First step is to enable touchscreen 4wire support and enable
> pen detect IRQ.
> Once a pen is detected, a periodic trigger is setup to trigger every
> 2 ms (e.g.) and sample the resistive touchscreen values. The trigger poll
> is called, and the consumer driver is then woke up, and it can read the
> respective channels for the values : X, and Y for position and pressure
> channel.
> Because only one trigger can be active in hardware in the same time,
> while touching the pad, the ADC will block any attempt to use the
> triggered buffer. Same, conversions using the software trigger are also
> impossible (since the periodic trigger is setup).
> If some driver wants to attach while the trigger is in use, it will
> also fail.
> Once the pen is not detected anymore, the trigger is free for use (hardware
> or software trigger, with or without DMA).
> Channels 0,1,2 and 3 are unavailable if a touchscreen is enabled.
> 
> Some parts of this patch are based on initial original work by
> Mohamed Jamsheeth Hajanajubudeen and Bandaru Venkateswara Swamy
> 
OK, so comments inline.

What I'm missing currently though is an explanation of why the slightly
more standard arrangement of using a callback buffer doesn't work here.
The only addition I think you need to do that is to allow a consumer to
request a particular trigger.  I also think some of the other provisions
could be handled using standard features and slightly reducing the flexibility.
I don't know for example if it's useful to allow other channels to be
read when touch is not in progress or not.

So restrictions:

1. Touch screen channels can only be read when touch is enabled.
 - use the available_scan_masks to control this. Or the callback that lets
   you do the same dynamically.
2. You need to push these channels to your consumer driver.
 - register a callback buffer rather than jumping through the hoops to
   insert your own pollfunc.  That will call a function in your
   consumer, providing the data from the 3 channels directly.
3. You need to make sure it is using the right driver.  For that you
   will I think need a new interface.

Various other comments inline. I may well be missing something as this is
a fair bit of complex code to read - if so then next version should have
a clear cover letter describing why this more standard approach can't be
used.

> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
> ---
>  drivers/iio/adc/at91-sama5d2_adc.c | 455 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 446 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 9610393..79eb197 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -102,14 +102,26 @@
>  #define AT91_SAMA5D2_LCDR	0x20
>  /* Interrupt Enable Register */
>  #define AT91_SAMA5D2_IER	0x24
> +/* Interrupt Enable Register - TS X measurement ready */
> +#define AT91_SAMA5D2_IER_XRDY   BIT(20)
> +/* Interrupt Enable Register - TS Y measurement ready */
> +#define AT91_SAMA5D2_IER_YRDY   BIT(21)
> +/* Interrupt Enable Register - TS pressure measurement ready */
> +#define AT91_SAMA5D2_IER_PRDY   BIT(22)
>  /* Interrupt Enable Register - general overrun error */
>  #define AT91_SAMA5D2_IER_GOVRE BIT(25)
> +/* Interrupt Enable Register - Pen detect */
> +#define AT91_SAMA5D2_IER_PEN    BIT(29)
> +/* Interrupt Enable Register - No pen detect */
> +#define AT91_SAMA5D2_IER_NOPEN  BIT(30)
>  /* Interrupt Disable Register */
>  #define AT91_SAMA5D2_IDR	0x28
>  /* Interrupt Mask Register */
>  #define AT91_SAMA5D2_IMR	0x2c
>  /* Interrupt Status Register */
>  #define AT91_SAMA5D2_ISR	0x30
> +/* Interrupt Status Register - Pen touching sense status */
> +#define AT91_SAMA5D2_ISR_PENS   BIT(31)
>  /* Last Channel Trigger Mode Register */
>  #define AT91_SAMA5D2_LCTMR	0x34
>  /* Last Channel Compare Window Register */
> @@ -131,8 +143,37 @@
>  #define AT91_SAMA5D2_CDR0	0x50
>  /* Analog Control Register */
>  #define AT91_SAMA5D2_ACR	0x94
> +/* Analog Control Register - Pen detect sensitivity mask */
> +#define AT91_SAMA5D2_ACR_PENDETSENS_MASK        GENMASK(0, 1)
>  /* Touchscreen Mode Register */
>  #define AT91_SAMA5D2_TSMR	0xb0
> +/* Touchscreen Mode Register - No touch mode */
> +#define AT91_SAMA5D2_TSMR_TSMODE_NONE           0
> +/* Touchscreen Mode Register - 4 wire screen, no pressure measurement */
> +#define AT91_SAMA5D2_TSMR_TSMODE_4WIRE_NO_PRESS 1
> +/* Touchscreen Mode Register - 4 wire screen, pressure measurement */
> +#define AT91_SAMA5D2_TSMR_TSMODE_4WIRE_PRESS    2
> +/* Touchscreen Mode Register - 5 wire screen */
> +#define AT91_SAMA5D2_TSMR_TSMODE_5WIRE          3
> +/* Touchscreen Mode Register - Average samples mask */
> +#define AT91_SAMA5D2_TSMR_TSAV_MASK		(3 << 4)
> +/* Touchscreen Mode Register - Average samples */
> +#define AT91_SAMA5D2_TSMR_TSAV(x)		((x) << 4)
> +/* Touchscreen Mode Register - Touch/trigger frequency ratio mask */
> +#define AT91_SAMA5D2_TSMR_TSFREQ_MASK		(0xf << 8)
> +/* Touchscreen Mode Register - Touch/trigger freqency ratio */
> +#define AT91_SAMA5D2_TSMR_TSFREQ(x)		((x) << 8)
> +/* Touchscreen Mode Register - Pen Debounce Time mask */
> +#define AT91_SAMA5D2_TSMR_PENDBC_MASK		(0xf << 28)
> +/* Touchscreen Mode Register - Pen Debounce Time */
> +#define AT91_SAMA5D2_TSMR_PENDBC(x)            ((x) << 28)
> +/* Touchscreen Mode Register - No DMA for touch measurements */
> +#define AT91_SAMA5D2_TSMR_NOTSDMA               BIT(22)
> +/* Touchscreen Mode Register - Disable pen detection */
> +#define AT91_SAMA5D2_TSMR_PENDET_DIS            (0 << 24)
> +/* Touchscreen Mode Register - Enable pen detection */
> +#define AT91_SAMA5D2_TSMR_PENDET_ENA            BIT(24)
> +
>  /* Touchscreen X Position Register */
>  #define AT91_SAMA5D2_XPOSR	0xb4
>  /* Touchscreen Y Position Register */
> @@ -151,7 +192,12 @@
>  #define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_FALL 2
>  /* Trigger Mode external trigger any edge */
>  #define AT91_SAMA5D2_TRGR_TRGMOD_EXT_TRIG_ANY 3
> -
> +/* Trigger Mode internal periodic */
> +#define AT91_SAMA5D2_TRGR_TRGMOD_PERIODIC 5
> +/* Trigger Mode - trigger period mask */
> +#define AT91_SAMA5D2_TRGR_TRGPER_MASK		(0xffff << 16)
> +/* Trigger Mode - trigger period */
> +#define AT91_SAMA5D2_TRGR_TRGPER(x)		((x) << 16)
>  /* Correction Select Register */
>  #define AT91_SAMA5D2_COSR	0xd0
>  /* Correction Value Register */
> @@ -169,6 +215,21 @@
>  #define AT91_SAMA5D2_SINGLE_CHAN_CNT 12
>  #define AT91_SAMA5D2_DIFF_CHAN_CNT 6
>  
> +#define AT91_SAMA5D2_TIMESTAMP_CHAN_IDX	(AT91_SAMA5D2_SINGLE_CHAN_CNT + \
> +					AT91_SAMA5D2_DIFF_CHAN_CNT + 1)
> +
> +#define AT91_SAMA5D2_TOUCH_X_CHAN_IDX	(AT91_SAMA5D2_TIMESTAMP_CHAN_IDX + 1)
> +#define AT91_SAMA5D2_TOUCH_Y_CHAN_IDX	(AT91_SAMA5D2_TOUCH_X_CHAN_IDX + 1)
> +#define AT91_SAMA5D2_TOUCH_P_CHAN_IDX	(AT91_SAMA5D2_TOUCH_Y_CHAN_IDX + 1)
> +
> +#define TOUCH_SAMPLE_PERIOD_US          2000    /* 2ms */

These all need the AT91_SAMA5D2 prefix.

> +#define TOUCH_PEN_DETECT_DEBOUNCE_US    200
> +
> +#define XYZ_MASK			GENMASK(11, 0)
> +
> +#define MAX_POS_BITS			12
> +
> +#define AT91_ADC_TOUCH_TRIG_SHORTNAME	"touch"
>  /*
>   * Maximum number of bytes to hold conversion from all channels
>   * without the timestamp.
> @@ -222,6 +283,37 @@
>  		.indexed = 1,						\
>  	}
>  
> +#define AT91_SAMA5D2_CHAN_TOUCH(num, name, mod)				\
> +	{								\
> +		.type = IIO_POSITION,					\
> +		.modified = 1,						\
> +		.channel = num,						\
> +		.channel2 = mod,					\
> +		.scan_index = num,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 12,					\
> +			.storagebits = 16,				\
> +		},							\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> +		.datasheet_name = name,					\
> +	}
> +#define AT91_SAMA5D2_CHAN_PRESSURE(num, name)				\
> +	{								\
> +		.type = IIO_PRESSURE,					\
> +		.channel = num,						\
> +		.scan_index = num,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 12,					\
> +			.storagebits = 16,				\
> +		},							\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),\
> +		.datasheet_name = name,					\
> +	}
> +
>  #define at91_adc_readl(st, reg)		readl_relaxed(st->base + reg)
>  #define at91_adc_writel(st, reg, val)	writel_relaxed(val, st->base + reg)
>  
> @@ -239,6 +331,20 @@ struct at91_adc_trigger {
>  };
>  
>  /**
> + * at91_adc_touch - at91-sama5d2 touchscreen information struct
> + * @trig:			hold the start timestamp of dma operation
> + * @sample_period_val:		the value for periodic trigger interval
> + * @touching:			is the pen touching the screen or not
> + * @x_pos:			temporary placeholder for pressure computation
> + */
> +struct at91_adc_touch {
> +	struct iio_trigger		*trig;
> +	u16				sample_period_val;
> +	bool				touching;
> +	u32				x_pos;
> +};
> +
> +/**
>   * at91_adc_dma - at91-sama5d2 dma information struct
>   * @dma_chan:		the dma channel acquired
>   * @rx_buf:		dma coherent allocated area
> @@ -267,18 +373,22 @@ struct at91_adc_state {
>  	struct regulator		*reg;
>  	struct regulator		*vref;
>  	int				vref_uv;
> +	unsigned int			current_sample_rate;
>  	struct iio_trigger		*trig;
>  	const struct at91_adc_trigger	*selected_trig;
>  	const struct iio_chan_spec	*chan;
>  	bool				conversion_done;
>  	u32				conversion_value;
> +	bool				touch_requested;
>  	struct at91_adc_soc_info	soc_info;
>  	wait_queue_head_t		wq_data_available;
>  	struct at91_adc_dma		dma_st;
> +	struct at91_adc_touch		touch_st;
>  	u16				buffer[AT91_BUFFER_MAX_HWORDS];
>  	/*
>  	 * lock to prevent concurrent 'single conversion' requests through
> -	 * sysfs.
> +	 * sysfs. Also protects when enabling or disabling touchscreen
> +	 * producer mode and checking if this mode is enabled or not.
>  	 */
>  	struct mutex			lock;
>  };
> @@ -310,6 +420,7 @@ static const struct at91_adc_trigger at91_adc_trigger_list[] = {
>  	},
>  };
>  
> +/* channel order is not subject to change. inkern consumers rely on this */
>  static const struct iio_chan_spec at91_adc_channels[] = {
>  	AT91_SAMA5D2_CHAN_SINGLE(0, 0x50),
>  	AT91_SAMA5D2_CHAN_SINGLE(1, 0x54),
> @@ -329,10 +440,103 @@ static const struct iio_chan_spec at91_adc_channels[] = {
>  	AT91_SAMA5D2_CHAN_DIFF(6, 7, 0x68),
>  	AT91_SAMA5D2_CHAN_DIFF(8, 9, 0x70),
>  	AT91_SAMA5D2_CHAN_DIFF(10, 11, 0x78),
> -	IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_SINGLE_CHAN_CNT
> -				+ AT91_SAMA5D2_DIFF_CHAN_CNT + 1),
> +	IIO_CHAN_SOFT_TIMESTAMP(AT91_SAMA5D2_TIMESTAMP_CHAN_IDX),
> +	AT91_SAMA5D2_CHAN_TOUCH(AT91_SAMA5D2_TOUCH_X_CHAN_IDX, "x", IIO_MOD_X),
> +	AT91_SAMA5D2_CHAN_TOUCH(AT91_SAMA5D2_TOUCH_Y_CHAN_IDX, "y", IIO_MOD_Y),
> +	AT91_SAMA5D2_CHAN_PRESSURE(AT91_SAMA5D2_TOUCH_P_CHAN_IDX, "pressure"),
>  };
>  
> +static int at91_adc_configure_touch(struct at91_adc_state *st, bool state)
> +{
> +	u32 clk_khz = st->current_sample_rate / 1000;
> +	int i = 0;
> +	u16 pendbc;
> +	u32 tsmr, acr;
> +
> +	if (!state) {
> +		/* disabling touch IRQs and setting mode to no touch enabled */
> +		at91_adc_writel(st, AT91_SAMA5D2_IDR,
> +				AT91_SAMA5D2_IER_PEN | AT91_SAMA5D2_IER_NOPEN);
> +		at91_adc_writel(st, AT91_SAMA5D2_TSMR, 0);
> +		return 0;
> +	}
> +	/*
> +	 * debounce time is in microseconds, we need it in milliseconds to
> +	 * multiply with kilohertz, so, divide by 1000, but after the multiply.
> +	 * round up to make sure pendbc is at least 1
> +	 */
> +	pendbc = round_up(TOUCH_PEN_DETECT_DEBOUNCE_US * clk_khz / 1000, 1);
> +
> +	/* get the required exponent */
> +	while (pendbc >> i++)
> +		;
This is related to the first 0?  There are cleaner ways of doing this
with ffs and friends.
> +
> +	pendbc = i;
> +
> +	tsmr = AT91_SAMA5D2_TSMR_TSMODE_4WIRE_PRESS;
> +
> +	tsmr |= AT91_SAMA5D2_TSMR_TSAV(1) & AT91_SAMA5D2_TSMR_TSAV_MASK;
> +	tsmr |= AT91_SAMA5D2_TSMR_PENDBC(pendbc) &
> +		AT91_SAMA5D2_TSMR_PENDBC_MASK;
> +	tsmr |= AT91_SAMA5D2_TSMR_NOTSDMA;
> +	tsmr |= AT91_SAMA5D2_TSMR_PENDET_ENA;
> +	tsmr |= AT91_SAMA5D2_TSMR_TSFREQ(1) & AT91_SAMA5D2_TSMR_TSFREQ_MASK;
> +
> +	at91_adc_writel(st, AT91_SAMA5D2_TSMR, tsmr);
> +
> +	acr =  at91_adc_readl(st, AT91_SAMA5D2_ACR);
> +	acr &= ~AT91_SAMA5D2_ACR_PENDETSENS_MASK;
> +	acr |= 0x02 & AT91_SAMA5D2_ACR_PENDETSENS_MASK;
> +	at91_adc_writel(st, AT91_SAMA5D2_ACR, acr);
> +
> +	/* Sample Period Time = (TRGPER + 1) / ADCClock */
> +	st->touch_st.sample_period_val = round_up((TOUCH_SAMPLE_PERIOD_US *
> +					 clk_khz / 1000) - 1, 1);
> +	/* enable pen detect IRQ */
> +	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_PEN);
> +
> +	return 0;
> +}
> +
> +static int at91_adc_touch_trigger_validate_device(struct iio_trigger *trig,
> +						  struct iio_dev *indio_dev)
> +{
> +	/* the touch trigger cannot be used with a buffer */
> +	return -EBUSY;
> +}
> +
> +static int at91_adc_configure_touch_trigger(struct iio_trigger *trig,
> +					    bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	/*
> +	 * If we configure this with the IRQ enabled, the pen detected IRQ
> +	 * might fire before we finish setting all up, and the IRQ handler
> +	 * might misbehave. Better to reenable the IRQ after we are done
> +	 */
> +	disable_irq_nosync(st->irq);
> +
> +	mutex_lock(&st->lock);
> +	if (state) {
> +		ret = iio_buffer_enabled(indio_dev);
> +		if (ret) {
> +			dev_dbg(&indio_dev->dev, "trigger is currently in use\n");
> +			ret = -EBUSY;
> +			goto configure_touch_unlock_exit;
> +		}
> +	}
> +	at91_adc_configure_touch(st, state);
> +	st->touch_requested = state;
> +
> +configure_touch_unlock_exit:
> +	enable_irq(st->irq);
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}
> +
>  static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
>  {
>  	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> @@ -390,12 +594,27 @@ static int at91_adc_reenable_trigger(struct iio_trigger *trig)
>  	return 0;
>  }
>  
> +static int at91_adc_reenable_touch_trigger(struct iio_trigger *trig)
> +{
> +	struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> +	struct at91_adc_state *st = iio_priv(indio);
> +
> +	enable_irq(st->irq);
> +
> +	return 0;
> +}
>  static const struct iio_trigger_ops at91_adc_trigger_ops = {
>  	.set_trigger_state = &at91_adc_configure_trigger,
>  	.try_reenable = &at91_adc_reenable_trigger,
>  	.validate_device = iio_trigger_validate_own_device,
>  };
>  
> +static const struct iio_trigger_ops at91_adc_touch_trigger_ops = {
> +	.set_trigger_state = &at91_adc_configure_touch_trigger,
> +	.try_reenable = &at91_adc_reenable_touch_trigger,
> +	.validate_device = &at91_adc_touch_trigger_validate_device,
> +};
> +
>  static int at91_adc_dma_size_done(struct at91_adc_state *st)
>  {
>  	struct dma_tx_state state;
> @@ -490,6 +709,23 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
>  	return 0;
>  }
>  
> +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct at91_adc_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	/* have to make sure nobody is requesting the trigger right now */

This needs some more explanation as I don't totally follow what this
is designed to protect against.

Realistically a device is only useful if it has one trigger at a time
feeding a valid set of channels to however many consumers (whether
in the driver or not).

> +	mutex_lock(&st->lock);
> +	ret = st->touch_requested;
> +	mutex_unlock(&st->lock);
> +
> +	/*
> +	 * if the trigger is used by the touchscreen,
> +	 * we must return an error
> +	 */
> +	return ret ? -EBUSY : 0;
> +}
> +
>  static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
>  {
>  	int ret;
> @@ -538,6 +774,7 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
>  }
>  
>  static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
> +	.preenable = &at91_adc_buffer_preenable,
>  	.postenable = &at91_adc_buffer_postenable,
>  	.predisable = &at91_adc_buffer_predisable,
>  };
> @@ -555,7 +792,11 @@ static struct iio_trigger *at91_adc_allocate_trigger(struct iio_dev *indio,
>  
>  	trig->dev.parent = indio->dev.parent;
>  	iio_trigger_set_drvdata(trig, indio);
> -	trig->ops = &at91_adc_trigger_ops;
> +
> +	if (strcmp(trigger_name, AT91_ADC_TOUCH_TRIG_SHORTNAME))

Pass this is as a parameter to the function and avoid the strcmp nastiness.

> +		trig->ops = &at91_adc_trigger_ops;
> +	else
> +		trig->ops = &at91_adc_touch_trigger_ops;
>  
>  	ret = devm_iio_trigger_register(&indio->dev, trig);
>  	if (ret)
> @@ -571,7 +812,16 @@ static int at91_adc_trigger_init(struct iio_dev *indio)
>  	st->trig = at91_adc_allocate_trigger(indio, st->selected_trig->name);
>  	if (IS_ERR(st->trig)) {
>  		dev_err(&indio->dev,
> -			"could not allocate trigger\n");
> +			"could not allocate trigger %s\n",
> +			 st->selected_trig->name);
> +		return PTR_ERR(st->trig);
> +	}
> +
> +	st->touch_st.trig = at91_adc_allocate_trigger(indio,
> +						AT91_ADC_TOUCH_TRIG_SHORTNAME);
> +	if (IS_ERR(st->trig)) {
> +		dev_err(&indio->dev, "could not allocate trigger"
> +			AT91_ADC_TOUCH_TRIG_SHORTNAME "\n");
>  		return PTR_ERR(st->trig);
>  	}
>  
> @@ -703,6 +953,8 @@ static void at91_adc_setup_samp_freq(struct at91_adc_state *st, unsigned freq)
>  
>  	dev_dbg(&indio_dev->dev, "freq: %u, startup: %u, prescal: %u\n",
>  		freq, startup, prescal);
> +
> +	st->current_sample_rate = freq;
>  }
>  
>  static unsigned at91_adc_get_sample_freq(struct at91_adc_state *st)
> @@ -718,23 +970,77 @@ static unsigned at91_adc_get_sample_freq(struct at91_adc_state *st)
>  	return f_adc;
>  }
>  
> +static irqreturn_t at91_adc_pen_detect_interrupt(struct at91_adc_state *st)
> +{
> +	at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_PEN);
> +	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_NOPEN |
> +			AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
> +			AT91_SAMA5D2_IER_PRDY);
> +	at91_adc_writel(st, AT91_SAMA5D2_TRGR,
> +			AT91_SAMA5D2_TRGR_TRGMOD_PERIODIC |
> +			AT91_SAMA5D2_TRGR_TRGPER(st->touch_st.sample_period_val));
> +	st->touch_st.touching = true;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t at91_adc_no_pen_detect_interrupt(struct at91_adc_state *st)
> +{
> +	at91_adc_writel(st, AT91_SAMA5D2_TRGR, 0);
> +	at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_NOPEN |
> +			AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
> +			AT91_SAMA5D2_IER_PRDY);
> +	st->touch_st.touching = false;
Hmm. I think we are unfortunately racing here.  There is nothing preventing
this running concurrently with the read_raw calls that check the same variable.

If this is fine (because we will always get valid data anyway (if stale)
then a comment is needed to explain that.

> +
> +	disable_irq_nosync(st->irq);
> +	iio_trigger_poll(st->touch_st.trig);

Comment to explain why a poll here is fine, but not on the pen on would be
good (I can guess but better to state it!)

> +
> +	at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_PEN);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t at91_adc_interrupt(int irq, void *private)
>  {
>  	struct iio_dev *indio = private;
>  	struct at91_adc_state *st = iio_priv(indio);
>  	u32 status = at91_adc_readl(st, AT91_SAMA5D2_ISR);
>  	u32 imr = at91_adc_readl(st, AT91_SAMA5D2_IMR);
> +	u32 rdy_mask = AT91_SAMA5D2_IER_XRDY | AT91_SAMA5D2_IER_YRDY |
> +			AT91_SAMA5D2_IER_PRDY;
>  
>  	if (!(status & imr))
>  		return IRQ_NONE;
>  
> -	if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
> +	if (st->touch_requested && (status & AT91_SAMA5D2_IER_PEN)) {
> +		/* pen detected IRQ */
> +		return at91_adc_pen_detect_interrupt(st);
> +	} else if (st->touch_requested && (status & AT91_SAMA5D2_IER_NOPEN)) {
> +		/* nopen detected IRQ */
> +		return at91_adc_no_pen_detect_interrupt(st);
> +	} else if (st->touch_requested && (status & AT91_SAMA5D2_ISR_PENS) &&
> +		   ((status & rdy_mask) == rdy_mask)) {
> +		/* periodic trigger IRQ - during pen sense */
> +		disable_irq_nosync(irq);
> +		iio_trigger_poll(st->touch_st.trig);
> +	} else if ((st->touch_requested && (status & AT91_SAMA5D2_ISR_PENS))) {
> +		/*
> +		 * touching, but the measurements are not ready yet.
> +		 * read and ignore.
> +		 */
> +		status = at91_adc_readl(st, AT91_SAMA5D2_XPOSR);
> +		status = at91_adc_readl(st, AT91_SAMA5D2_YPOSR);
> +		status = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
> +	} else if (iio_buffer_enabled(indio) && !st->dma_st.dma_chan) {
> +		/* buffered trigger without DMA */
>  		disable_irq_nosync(irq);
>  		iio_trigger_poll(indio->trig);
>  	} else if (iio_buffer_enabled(indio) && st->dma_st.dma_chan) {
> +		/* buffered trigger with DMA - should not happen */
>  		disable_irq_nosync(irq);
>  		WARN(true, "Unexpected irq occurred\n");
>  	} else if (!iio_buffer_enabled(indio)) {
> +		/* software requested conversion */
>  		st->conversion_value = at91_adc_readl(st, st->chan->address);
>  		st->conversion_done = true;
>  		wake_up_interruptible(&st->wq_data_available);
> @@ -742,6 +1048,96 @@ static irqreturn_t at91_adc_interrupt(int irq, void *private)
>  	return IRQ_HANDLED;
>  }
>  
> +static u32 at91_adc_touch_x_pos(struct at91_adc_state *st)
> +{
> +	u32 xscale, val;
> +	u32 x, xpos;
> +
> +	/* x position = (x / xscale) * max, max = 2^MAX_POS_BITS - 1 */
> +	val = at91_adc_readl(st, AT91_SAMA5D2_XPOSR);
> +	if (!val)
> +		dev_dbg(&iio_priv_to_dev(st)->dev, "x_pos is 0\n");
> +
> +	xpos = val & XYZ_MASK;
> +	x = (xpos << MAX_POS_BITS) - xpos;
> +	xscale = (val >> 16) & XYZ_MASK;
> +	if (xscale == 0) {
> +		dev_err(&iio_priv_to_dev(st)->dev, "xscale is 0\n");
> +		return 0;
> +	}
> +	x /= xscale;
> +	st->touch_st.x_pos = x;
> +
> +	return x;
> +}
> +
> +static u32 at91_adc_touch_y_pos(struct at91_adc_state *st)
> +{
> +	u32 yscale, val;
> +	u32 y, ypos;
> +
> +	/* y position = (y / yscale) * max, max = 2^MAX_POS_BITS - 1 */
> +	val = at91_adc_readl(st, AT91_SAMA5D2_YPOSR);
> +	ypos = val & XYZ_MASK;
> +	y = (ypos << MAX_POS_BITS) - ypos;
> +	yscale = (val >> 16) & XYZ_MASK;
> +
> +	if (yscale == 0)
> +		return 0;
> +
> +	y /= yscale;
> +
> +	return y;
> +}
> +
> +static u32 at91_adc_touch_pressure(struct at91_adc_state *st)
> +{
> +	u32 val, z1, z2;
> +	u32 pres;
> +	u32 rxp = 1;
> +	u32 factor = 1000;
> +
> +	/* calculate the pressure */
> +	val = at91_adc_readl(st, AT91_SAMA5D2_PRESSR);
> +	z1 = val & XYZ_MASK;

XYZ_MASK seems oddly named given what this seems to be doing...

> +	z2 = (val >> 16) & XYZ_MASK;
> +
> +	if (z1 != 0)
> +		pres = rxp * (st->touch_st.x_pos * factor / 1024) *
> +			(z2 * factor / z1 - factor) /
> +			factor;
> +	else
> +		pres = 0xFFFFFFFF;       /* no pen contact */
> +
> +	return pres;
> +}
> +
> +static int at91_adc_read_position(struct at91_adc_state *st, int chan, int *val)
> +{
> +	if (!st->touch_st.touching)
> +		return -ENODATA;
> +	if (chan == AT91_SAMA5D2_TOUCH_X_CHAN_IDX)
> +		*val = at91_adc_touch_x_pos(st);
> +	else if (chan == AT91_SAMA5D2_TOUCH_Y_CHAN_IDX)
> +		*val = at91_adc_touch_y_pos(st);
> +	else
> +		return -ENODATA;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int at91_adc_read_pressure(struct at91_adc_state *st, int chan, int *val)
> +{
> +	if (!st->touch_st.touching)
> +		return -ENODATA;
> +	if (chan == AT91_SAMA5D2_TOUCH_P_CHAN_IDX)

General code flow simpler if you check error first
if (chan != AT91_SAMA5D2_TOUCH_P_CHAN_IDX)
	return -ENODATA;

*val =...

> +		*val = at91_adc_touch_pressure(st);
> +	else
> +		return -ENODATA;
> +
> +	return IIO_VAL_INT;
> +}
> +
>  static int at91_adc_read_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int *val, int *val2, long mask)
> @@ -752,11 +1148,38 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&st->lock);
> +
> +		if (chan->type == IIO_POSITION) {

Switch or else if as only one is true at a time.

Hmm. So you allow sysfs reads of these channels if touch in progress.
Do we actually have a use for this?  Seems we could have simpler code
by just not providing direct reads for them if not.

> +			ret = at91_adc_read_position(st, chan->channel, val);
> +			mutex_unlock(&st->lock);
> +			return ret;
> +		}
> +		if (chan->type == IIO_PRESSURE) {
> +			ret = at91_adc_read_pressure(st, chan->channel, val);
> +			mutex_unlock(&st->lock);
> +			return ret;
> +		}
> +		/* if we using touch, channels 0, 1, 2, 3 are unavailable */
> +		if (st->touch_requested && chan->channel <= 3) {
> +			mutex_unlock(&st->lock);
> +			return -EBUSY;
> +		}
> +		/*
> +		 * if we have the periodic trigger set up, we can't use
> +		 * software trigger either.
> +		 */
> +		if (st->touch_st.touching) {

> +			mutex_unlock(&st->lock);
> +			return -ENODATA;
> +		}
> +
>  		/* we cannot use software trigger if hw trigger enabled */
>  		ret = iio_device_claim_direct_mode(indio_dev);
> -		if (ret)
> +		if (ret) {
> +			mutex_unlock(&st->lock);
>  			return ret;
> -		mutex_lock(&st->lock);
> +		}
>  
>  		st->chan = chan;
>  
> @@ -785,6 +1208,11 @@ static int at91_adc_read_raw(struct iio_dev *indio_dev,
>  
>  		at91_adc_writel(st, AT91_SAMA5D2_IDR, BIT(chan->channel));
>  		at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
> +		/*
> +		 * It is possible that after this conversion, we reuse these
> +		 * channels for the touchscreen. So, reset the COR now.
> +		 */
> +		at91_adc_writel(st, AT91_SAMA5D2_COR, 0);
>  
>  		/* Needed to ACK the DRDY interruption */
>  		at91_adc_readl(st, AT91_SAMA5D2_LCDR);
> @@ -1180,6 +1608,10 @@ static int at91_adc_remove(struct platform_device *pdev)
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>  	struct at91_adc_state *st = iio_priv(indio_dev);
>  
> +	mutex_lock(&st->lock);
> +	devm_iio_trigger_unregister(&indio_dev->dev, st->touch_st.trig);

As before this needs detailed explanation. It should not be necessary.

> +	mutex_unlock(&st->lock);
> +
>  	if (st->selected_trig->hw_trig)
>  		devm_iio_trigger_unregister(&indio_dev->dev, st->trig);
>  
> @@ -1245,6 +1677,11 @@ static __maybe_unused int at91_adc_resume(struct device *dev)
>  	if (iio_buffer_enabled(indio_dev))
>  		at91_adc_configure_trigger(st->trig, true);
>  
> +	mutex_lock(&st->lock);
> +	if (st->touch_requested)
> +		at91_adc_configure_touch_trigger(st->touch_st.trig, true);
> +	mutex_unlock(&st->lock);
> +
>  	return 0;
>  
>  vref_disable_resume:

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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux