| Message ID | 1513955241-10985-13-git-send-email-eugen.hristev@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 22 Dec 2017 17:07:19 +0200 Eugen Hristev <eugen.hristev@microchip.com> 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@microchip.com> > --- > 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 linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29.12.2017 19:02, Jonathan Cameron wrote: > On Fri, 22 Dec 2017 17:07:19 +0200 > Eugen Hristev <eugen.hristev@microchip.com> 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. Hello Jonathan and thanks for the review of my patch series, before starting and working over the required modifications and suggestions that you sent me, I want to be a little more explicit about the design of my implementation. Hope this will clarify some things, and maybe I can as well understand better what you have in mind to support this feature set. Why have I picked a pollfunction: We discussed a while back on the mailing list that you do not have an inkern mechanism to expose the triggers to other drivers, and that it may be a good idea to have it for such kind of actually multi function device, instead of having a MFD driver, an ADC driver, and an Input driver, all sharing the same register map, the same IRQ , etc, with some kind of synchronization to avoid stepping on each other for the hardware resource. So I considered to expose the trigger by attaching and detaching pollfunctions to it. Which is the main thing what we use a trigger for. So, what I had in mind, was to create a consumer driver that will request triggers from the IIO device just like other drivers request channels (part which is already done in IIO). In order to do this I had to somehow wake up the consumer driver when new data was available from the touchscreen. So, having the IRQ only in the ADC device, and then on Pen detect and No pen detect just start or stop the periodic trigger, which needs to be polled. The magic part is that the consumer driver has a poll function already attached to this trigger, so the poll function is just called every time we have new data. The poll function is attached as an irq handler, and then we can reuse all the read_raw data by using a scheduled work from the consumer driver, to read the channels. To do this, the ADC registers a special trigger named "touch trigger" which is never enabled by the ADC driver. Instead, when a pollfunc is attached to it, the attach function will also configure it with enabled state. In the ADC, this means to start the touchscreen functionality. If the touch is requested, it will standby and wait for pen detect IRQ. Once we have pen detect, we can use a periodic trigger to sample the touch data, and poll the "touch" trigger. The consumer driver will wake up and schedule a work , that will use the standard read raw interface (inkern) that will read three virtual channels (position + pressure). They are not actual hardware channels, as the touch information is being received on channels 0,1,2,3, but reading these virtual channels will read from different registers inside the ADC IP ( x position, y position, pressure), do some computations on the data, and feed the consumer with the values , hiding the behind the scenes hardware specific calculations. After trigger is polled , the ADC will resume normal functionality, and the consumer driver will continue to sleep. We need to have a periodic trigger to sample the data because the actual analog to digital conversion inside the IP block needs to be triggered. The touchscreen data measurements cannot happen in hardware without being triggered. If I try with a hrtimer to get a periodic IRQ to just read the data, it will never be ready. The datasheet states that the touchscreen measurements "will be attached to the conversion sequence". So the periodic trigger is forcing a conversion sequence. This could be done with a software trigger as well, but why the hassle to start it every 2 milliseconds (or other time interval), if we can do it by periodic trigger ? Once we get the No pen IRQ, we stop the periodic trigger and it can be used in another purpose (software or external as of now in the driver, in the future we can add PWM trigger and Timer trigger) In short, the ADC in Sama5D2 also supports touchscreen, and in touchscreen mode , 4 of the channels are being used for this purpose. This however, doesn't stop the ADC to use the other channels . The hardware has 12 total single channels and they can be paired to have 6 more differential channels. The only thing that is blocked is the trigger, but only if the pen is touching (when we start the periodic trigger to sample the touchscreen). If the pen is not touching, an external trigger or software trigger can be used without any issues (so why limit the functionality, if this is available from hardware ?). Because of the reason I discussed above (touchscreen sequence must be triggered), we cannot use another trigger in the same time. I see your idea with the callback buffer and it's worth exploring. Mainly this series was to actually show you what I had in mind about supporting the resistive touchscreen, and to give you some actually working code/patch, so we can discuss based on real implementation, not just suppositions. You are right in many of the other comments that you said, and I will come up with a v2 to this series. For now, I need to know if this is a good or right direction in which I am going, or I should try to change all the mechanism to callback buffer ? Or maybe I am totally in a bad direction ? The requirements are that the consumer driver needs to be somehow woke up for every new touch data available, and report to the input subsystem. As it was done before, the at91 old driver, just creates and registers an input device by itself, and then reports the position and touches. I was thinking that with this trigger consumer implementation, things can be better in terms of subsystem separation and support. Thanks again and let me know of your thoughts, Eugen [...] -- 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
On Thu, 4 Jan 2018 17:17:54 +0200 Eugen Hristev <eugen.hristev@microchip.com> wrote: > On 29.12.2017 19:02, Jonathan Cameron wrote: > > On Fri, 22 Dec 2017 17:07:19 +0200 > > Eugen Hristev <eugen.hristev@microchip.com> 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. > > Hello Jonathan and thanks for the review of my patch series, > > before starting and working over the required modifications and > suggestions that you sent me, I want to be a little more explicit about > the design of my implementation. > Hope this will clarify some things, and maybe I can as well understand > better what you have in mind to support this feature set. > > Why have I picked a pollfunction: We discussed a while back on the > mailing list that you do not have an inkern mechanism to expose the > triggers to other drivers, and that it may be a good idea to have it for > such kind of actually multi function device, instead of having a MFD > driver, an ADC driver, and an Input driver, all sharing the same > register map, the same IRQ , etc, with some kind of synchronization to > avoid stepping on each other for the hardware resource. No disagreement with that principle. > So I considered to expose the trigger by attaching and detaching > pollfunctions to it. Which is the main thing what we use a trigger for. Hmm. It's definitely one approach. But we do already have other drivers where the trigger is controlled by a consumer and to my mind that is a cleaner approach as it doesn't short cut the equivalent of doing it from userspace. drivers/iio/potentiostat/lmp91000.c does something similar though for a rather different use. You need your consumer interface to get the handle to the trigger in this case (the lmp91000 is actually providing the trigger rather than consuming it). > > So, what I had in mind, was to create a consumer driver that will > request triggers from the IIO device just like other drivers request > channels (part which is already done in IIO). > In order to do this I had to somehow wake up the consumer driver when > new data was available from the touchscreen. So, having the IRQ only in > the ADC device, and then on Pen detect and No pen detect just start or > stop the periodic trigger, which needs to be polled. The magic part is > that the consumer driver has a poll function already attached to this > trigger, so the poll function is just called every time we have new > data. The poll function is attached as an irq handler, and then we can > reuse all the read_raw data by using a scheduled work from the consumer > driver, to read the channels. If you had done this via a callback buffer the only difference is that the pollfunc would have been a standard one pulling the relevant channels and passing them on down to the buffer interface which could then decide what to do with them. > To do this, the ADC registers a special trigger named "touch trigger" > which is never enabled by the ADC driver. Instead, when a pollfunc is > attached to it, the attach function will also configure it with enabled > state. Whilst it might not make sense to enable it in the touch screen driver I'm not sure there is strictly any reason to prevent it being so used. > In the ADC, this means to start the touchscreen functionality. If > the touch is requested, it will standby and wait for pen detect IRQ. > Once we have pen detect, we can use a periodic trigger to sample the > touch data, and poll the "touch" trigger. The consumer driver will wake > up and schedule a work , that will use the standard read raw interface > (inkern) that will read three virtual channels (position + pressure). > They are not actual hardware channels, as the touch information is being > received on channels 0,1,2,3, but reading these virtual channels will > read from different registers inside the ADC IP ( x position, y > position, pressure), do some computations on the data, and feed the > consumer with the values , hiding the behind the scenes hardware > specific calculations. I wouldn't worry about whether they are real channels or not. This is really similar to a differential ADC (some of those do the differential digitally). Light sensors often have a number of 'real' channels used to derive (via hideous non linear calculations) the illuminance as it's hard to build a light sensor with the same sensitivity as the human eye. We have worse 'non real' channels as well such as activity channels on some the accelerometers that report if it thinks you are walking / running etc. > After trigger is polled , the ADC will resume normal functionality, and > the consumer driver will continue to sleep. So this is where I'm unsure. Do you actually have a usecase where it makes the sense to read from the ADC only when there is no touch? Any system doing that has an obvious denial of service attack - touch the screen. > We need to have a periodic trigger to sample the data because the actual > analog to digital conversion inside the IP block needs to be triggered. > The touchscreen data measurements cannot happen in hardware without > being triggered. If I try with a hrtimer to get a periodic IRQ to just > read the data, it will never be ready. The datasheet states that the > touchscreen measurements "will be attached to the conversion sequence". > So the periodic trigger is forcing a conversion sequence. This could be > done with a software trigger as well, but why the hassle to start it > every 2 milliseconds (or other time interval), if we can do it by > periodic trigger ? Ah, one reason here would be to allow separate consumers to use the device. In that case you'd run with a periodic trigger all the time and have two buffers attached, the buffer_cb that is feeding your touchscreen and another buffer to deal with the other channels (presumably the standard one an IIO device has when using buffered interfaces). The buffer demux would ensure the data from the right channels ends up in the right place. It makes it look to the buffer consumer like it is the only thing using / controlling the data flow. > Once we get the No pen IRQ, we stop the periodic trigger and it can be > used in another purpose (software or external as of now in the driver, > in the future we can add PWM trigger and Timer trigger) This case isn't really useful though as any other use is denied access when touch occurs. I'll summarise what I think would work for this below. > > In short, the ADC in Sama5D2 also supports touchscreen, and in > touchscreen mode , 4 of the channels are being used for this purpose. > This however, doesn't stop the ADC to use the other channels . The > hardware has 12 total single channels and they can be paired to have 6 > more differential channels. The only thing that is blocked is the > trigger, but only if the pen is touching (when we start the periodic > trigger to sample the touchscreen). If the pen is not touching, an > external trigger or software trigger can be used without any issues (so > why limit the functionality, if this is available from hardware ?). > Because of the reason I discussed above (touchscreen sequence must be > triggered), we cannot use another trigger in the same time. > > > I see your idea with the callback buffer and it's worth exploring. > Mainly this series was to actually show you what I had in mind about > supporting the resistive touchscreen, and to give you some actually > working code/patch, so we can discuss based on real implementation, not > just suppositions. That side of things is fine. > > You are right in many of the other comments that you said, and I will > come up with a v2 to this series. For now, I need to know if this is a > good or right direction in which I am going, or I should try to change > all the mechanism to callback buffer ? Or maybe I am totally in a bad > direction ? > The requirements are that the consumer driver needs to be somehow woke > up for every new touch data available, and report to the input > subsystem. As it was done before, the at91 old driver, just creates and > registers an input device by itself, and then reports the position and > touches. I was thinking that with this trigger consumer implementation, > things can be better in terms of subsystem separation and support. > > Thanks again and let me know of your thoughts, > > Eugen So a couple of things come to mind on how I'd structure this. So what we have (very briefly) No touch screen case: * Generic ADC using all sorts of different triggers Touch screen only case: * Interrupt to indicate pen on / off * A need to do a periodic trigger of the device but only useful when touch is in progress. Touch screen and other users: * Interrupt to indicate pen on / off * Periodic trigger needed for touchscreen when touch in progress. * Do not have denial of service on other channels. First two cases are easy enough by having a magic trigger, third case is harder. If we have the touchscreen then I would drop support for direct access to to ADC channels whilst it's in use (so no sysfs - or emulate it if you really want it by stashing results from scans done when touch is in progress). Have your touch screen channels just as normal additional channels, but only via the buffered interface (no _RAW attribute). If someone sets up to read them via buffered interface with a different trigger I think they'll get values - whether they are right is dependent (if I understand correctly) on whether there is a touch in progress. So no harm done and it'll make the logic simpler. The moment touch is opened and acquires the IIO channels fix the trigger (may need new interface) to the periodic one that you were enabling and disabling on touch. Things get dicey if there is an existing user so you may have to do it on driver probe rather than open of the input device if we effectively want touch to have the highest priority use of the ADC. If other channels are enabled for buffered mode then note this in the driver and have the periodic trigger on all the time (to ensure they keep getting read) This will pass garbage to your touch screen driver, but it'll remove it due to the pressure value being too low so no harm there. Normal path will work for non touch channels (and in theory the touch ones if they are turned on) via IIO buffer interface. It'll be restricted in form due to the needs of the touch driver, but better than nothing and should cover most usecases. Now the interrupt on / off on touch bit becomes an optimization in the case of only the buffer_cb being attached. I think that fits cleanly in the current IIO framework and looks more similar to our existing provider consumer approaches. Still needs the hooks to get hold of the trigger though so as to be able to tell the ADC which one to use. So rather than being a trigger consumer interface, it's more of a trigger configuration interface.. Exact term doesn't matter though. Jonathan > > > > [...] > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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
Hi Jonathan, Eugen, On Sat, Jan 06, 2018 at 03:05:37PM +0000, Jonathan Cameron wrote: > On Thu, 4 Jan 2018 17:17:54 +0200 > Eugen Hristev <eugen.hristev@microchip.com> wrote: > > > On 29.12.2017 19:02, Jonathan Cameron wrote: > > > On Fri, 22 Dec 2017 17:07:19 +0200 > > > Eugen Hristev <eugen.hristev@microchip.com> 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. > > > > Hello Jonathan and thanks for the review of my patch series, > > > > before starting and working over the required modifications and > > suggestions that you sent me, I want to be a little more explicit about > > the design of my implementation. > > Hope this will clarify some things, and maybe I can as well understand > > better what you have in mind to support this feature set. > > > > Why have I picked a pollfunction: We discussed a while back on the > > mailing list that you do not have an inkern mechanism to expose the > > triggers to other drivers, and that it may be a good idea to have it for > > such kind of actually multi function device, instead of having a MFD > > driver, an ADC driver, and an Input driver, all sharing the same > > register map, the same IRQ , etc, with some kind of synchronization to > > avoid stepping on each other for the hardware resource. > > No disagreement with that principle. > > > So I considered to expose the trigger by attaching and detaching > > pollfunctions to it. Which is the main thing what we use a trigger for. > > Hmm. It's definitely one approach. But we do already have other drivers > where the trigger is controlled by a consumer and to my mind that > is a cleaner approach as it doesn't short cut the equivalent of > doing it from userspace. > > drivers/iio/potentiostat/lmp91000.c does something similar though > for a rather different use. You need your consumer interface > to get the handle to the trigger in this case > (the lmp91000 is actually providing the trigger rather than > consuming it). > > > > > > So, what I had in mind, was to create a consumer driver that will > > request triggers from the IIO device just like other drivers request > > channels (part which is already done in IIO). > > In order to do this I had to somehow wake up the consumer driver when > > new data was available from the touchscreen. So, having the IRQ only in > > the ADC device, and then on Pen detect and No pen detect just start or > > stop the periodic trigger, which needs to be polled. The magic part is > > that the consumer driver has a poll function already attached to this > > trigger, so the poll function is just called every time we have new > > data. The poll function is attached as an irq handler, and then we can > > reuse all the read_raw data by using a scheduled work from the consumer > > driver, to read the channels. > > If you had done this via a callback buffer the only difference is that > the pollfunc would have been a standard one pulling the relevant channels > and passing them on down to the buffer interface which could then decide > what to do with them. > > > To do this, the ADC registers a special trigger named "touch trigger" > > which is never enabled by the ADC driver. Instead, when a pollfunc is > > attached to it, the attach function will also configure it with enabled > > state. > > Whilst it might not make sense to enable it in the touch screen driver > I'm not sure there is strictly any reason to prevent it being so used. > > > In the ADC, this means to start the touchscreen functionality. If > > the touch is requested, it will standby and wait for pen detect IRQ. > > Once we have pen detect, we can use a periodic trigger to sample the > > touch data, and poll the "touch" trigger. The consumer driver will wake > > up and schedule a work , that will use the standard read raw interface > > (inkern) that will read three virtual channels (position + pressure). > > They are not actual hardware channels, as the touch information is being > > received on channels 0,1,2,3, but reading these virtual channels will > > read from different registers inside the ADC IP ( x position, y > > position, pressure), do some computations on the data, and feed the > > consumer with the values , hiding the behind the scenes hardware > > specific calculations. > > I wouldn't worry about whether they are real channels or not. This > is really similar to a differential ADC (some of those do the differential > digitally). Light sensors often have a number of 'real' channels used > to derive (via hideous non linear calculations) the illuminance as > it's hard to build a light sensor with the same sensitivity as the human > eye. We have worse 'non real' channels as well such as activity channels > on some the accelerometers that report if it thinks you are walking / > running etc. > > > After trigger is polled , the ADC will resume normal functionality, and > > the consumer driver will continue to sleep. > > So this is where I'm unsure. Do you actually have a usecase where it > makes the sense to read from the ADC only when there is no touch? Any > system doing that has an obvious denial of service attack - touch the > screen. > You're right. We have an issue in this case due to the hardware. Using touchscreen has side effects on other channels. We can use only one trigger for all the channels. The situation would have been better with a trigger dedicated to the touchscreen. At the moment, we have not really stated about the exclusive use or not of the touchscreen. We suppose we can get some customers wanting to use both touchscreen and ADC. Eugen tried to deal with this case but, as you noticed, it can lead to DoS. > > We need to have a periodic trigger to sample the data because the actual > > analog to digital conversion inside the IP block needs to be triggered. > > The touchscreen data measurements cannot happen in hardware without > > being triggered. If I try with a hrtimer to get a periodic IRQ to just > > read the data, it will never be ready. The datasheet states that the > > touchscreen measurements "will be attached to the conversion sequence". > > So the periodic trigger is forcing a conversion sequence. This could be > > done with a software trigger as well, but why the hassle to start it > > every 2 milliseconds (or other time interval), if we can do it by > > periodic trigger ? > > Ah, one reason here would be to allow separate consumers to use the > device. In that case you'd run with a periodic trigger all the time > and have two buffers attached, the buffer_cb that is feeding your > touchscreen and another buffer to deal with the other channels > (presumably the standard one an IIO device has when using buffered > interfaces). The issue is that we are sharing the periodic trigger so we have to use the same period for both usage. Regards Ludovic > > The buffer demux would ensure the data from the right channels > ends up in the right place. It makes it look to the buffer > consumer like it is the only thing using / controlling the data > flow. > > > Once we get the No pen IRQ, we stop the periodic trigger and it can be > > used in another purpose (software or external as of now in the driver, > > in the future we can add PWM trigger and Timer trigger) > > This case isn't really useful though as any other use is denied > access when touch occurs. > > I'll summarise what I think would work for this below. > > > > > In short, the ADC in Sama5D2 also supports touchscreen, and in > > touchscreen mode , 4 of the channels are being used for this purpose. > > This however, doesn't stop the ADC to use the other channels . The > > hardware has 12 total single channels and they can be paired to have 6 > > more differential channels. The only thing that is blocked is the > > trigger, but only if the pen is touching (when we start the periodic > > trigger to sample the touchscreen). If the pen is not touching, an > > external trigger or software trigger can be used without any issues (so > > why limit the functionality, if this is available from hardware ?). > > Because of the reason I discussed above (touchscreen sequence must be > > triggered), we cannot use another trigger in the same time. > > > > > > I see your idea with the callback buffer and it's worth exploring. > > Mainly this series was to actually show you what I had in mind about > > supporting the resistive touchscreen, and to give you some actually > > working code/patch, so we can discuss based on real implementation, not > > just suppositions. > > That side of things is fine. > > > > > You are right in many of the other comments that you said, and I will > > come up with a v2 to this series. For now, I need to know if this is a > > good or right direction in which I am going, or I should try to change > > all the mechanism to callback buffer ? Or maybe I am totally in a bad > > direction ? > > The requirements are that the consumer driver needs to be somehow woke > > up for every new touch data available, and report to the input > > subsystem. As it was done before, the at91 old driver, just creates and > > registers an input device by itself, and then reports the position and > > touches. I was thinking that with this trigger consumer implementation, > > things can be better in terms of subsystem separation and support. > > > > Thanks again and let me know of your thoughts, > > > > Eugen > > So a couple of things come to mind on how I'd structure this. > So what we have (very briefly) > > No touch screen case: > * Generic ADC using all sorts of different triggers > > Touch screen only case: > * Interrupt to indicate pen on / off > * A need to do a periodic trigger of the device but only > useful when touch is in progress. > > Touch screen and other users: > * Interrupt to indicate pen on / off > * Periodic trigger needed for touchscreen when touch in progress. > * Do not have denial of service on other channels. > > First two cases are easy enough by having a magic trigger, third > case is harder. > If we have the touchscreen then I would drop support for direct access to > to ADC channels whilst it's in use (so no sysfs - or emulate it if you > really want it by stashing results from scans done when touch is in > progress). > > Have your touch screen channels just as normal additional channels, > but only via the buffered interface (no _RAW attribute). > If someone sets up to read them via buffered interface with > a different trigger I think they'll get values - whether they > are right is dependent (if I understand correctly) on whether > there is a touch in progress. So no harm done and it'll make > the logic simpler. > > The moment touch is opened and acquires the IIO channels > fix the trigger (may need new interface) to the periodic one > that you were enabling and disabling on touch. > Things get dicey if there is an existing user so you may > have to do it on driver probe rather than open of the input > device if we effectively want touch to have the highest > priority use of the ADC. > > If other channels are enabled for buffered mode then note > this in the driver and have the periodic trigger on all the > time (to ensure they keep getting read) This will pass > garbage to your touch screen driver, but it'll remove it due > to the pressure value being too low so no harm there. > > Normal path will work for non touch channels (and in theory > the touch ones if they are turned on) via IIO buffer > interface. It'll be restricted in form due to the needs of > the touch driver, but better than nothing and should cover > most usecases. > > Now the interrupt on / off on touch bit becomes an optimization > in the case of only the buffer_cb being attached. > > I think that fits cleanly in the current IIO framework and > looks more similar to our existing provider consumer approaches. > > Still needs the hooks to get hold of the trigger though so > as to be able to tell the ADC which one to use. So rather > than being a trigger consumer interface, it's more of a trigger > configuration interface.. Exact term doesn't matter though. > > Jonathan > > > > > > > > > [...] > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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
On Mon, 8 Jan 2018 15:12:52 +0100 Ludovic Desroches <ludovic.desroches@microchip.com> wrote: > Hi Jonathan, Eugen, > > On Sat, Jan 06, 2018 at 03:05:37PM +0000, Jonathan Cameron wrote: > > On Thu, 4 Jan 2018 17:17:54 +0200 > > Eugen Hristev <eugen.hristev@microchip.com> wrote: > > > > > On 29.12.2017 19:02, Jonathan Cameron wrote: > > > > On Fri, 22 Dec 2017 17:07:19 +0200 > > > > Eugen Hristev <eugen.hristev@microchip.com> 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. > > > > > > Hello Jonathan and thanks for the review of my patch series, > > > > > > before starting and working over the required modifications and > > > suggestions that you sent me, I want to be a little more explicit about > > > the design of my implementation. > > > Hope this will clarify some things, and maybe I can as well understand > > > better what you have in mind to support this feature set. > > > > > > Why have I picked a pollfunction: We discussed a while back on the > > > mailing list that you do not have an inkern mechanism to expose the > > > triggers to other drivers, and that it may be a good idea to have it for > > > such kind of actually multi function device, instead of having a MFD > > > driver, an ADC driver, and an Input driver, all sharing the same > > > register map, the same IRQ , etc, with some kind of synchronization to > > > avoid stepping on each other for the hardware resource. > > > > No disagreement with that principle. > > > > > So I considered to expose the trigger by attaching and detaching > > > pollfunctions to it. Which is the main thing what we use a trigger for. > > > > Hmm. It's definitely one approach. But we do already have other drivers > > where the trigger is controlled by a consumer and to my mind that > > is a cleaner approach as it doesn't short cut the equivalent of > > doing it from userspace. > > > > drivers/iio/potentiostat/lmp91000.c does something similar though > > for a rather different use. You need your consumer interface > > to get the handle to the trigger in this case > > (the lmp91000 is actually providing the trigger rather than > > consuming it). > > > > > > > > > > So, what I had in mind, was to create a consumer driver that will > > > request triggers from the IIO device just like other drivers request > > > channels (part which is already done in IIO). > > > In order to do this I had to somehow wake up the consumer driver when > > > new data was available from the touchscreen. So, having the IRQ only in > > > the ADC device, and then on Pen detect and No pen detect just start or > > > stop the periodic trigger, which needs to be polled. The magic part is > > > that the consumer driver has a poll function already attached to this > > > trigger, so the poll function is just called every time we have new > > > data. The poll function is attached as an irq handler, and then we can > > > reuse all the read_raw data by using a scheduled work from the consumer > > > driver, to read the channels. > > > > If you had done this via a callback buffer the only difference is that > > the pollfunc would have been a standard one pulling the relevant channels > > and passing them on down to the buffer interface which could then decide > > what to do with them. > > > > > To do this, the ADC registers a special trigger named "touch trigger" > > > which is never enabled by the ADC driver. Instead, when a pollfunc is > > > attached to it, the attach function will also configure it with enabled > > > state. > > > > Whilst it might not make sense to enable it in the touch screen driver > > I'm not sure there is strictly any reason to prevent it being so used. > > > > > In the ADC, this means to start the touchscreen functionality. If > > > the touch is requested, it will standby and wait for pen detect IRQ. > > > Once we have pen detect, we can use a periodic trigger to sample the > > > touch data, and poll the "touch" trigger. The consumer driver will wake > > > up and schedule a work , that will use the standard read raw interface > > > (inkern) that will read three virtual channels (position + pressure). > > > They are not actual hardware channels, as the touch information is being > > > received on channels 0,1,2,3, but reading these virtual channels will > > > read from different registers inside the ADC IP ( x position, y > > > position, pressure), do some computations on the data, and feed the > > > consumer with the values , hiding the behind the scenes hardware > > > specific calculations. > > > > I wouldn't worry about whether they are real channels or not. This > > is really similar to a differential ADC (some of those do the differential > > digitally). Light sensors often have a number of 'real' channels used > > to derive (via hideous non linear calculations) the illuminance as > > it's hard to build a light sensor with the same sensitivity as the human > > eye. We have worse 'non real' channels as well such as activity channels > > on some the accelerometers that report if it thinks you are walking / > > running etc. > > > > > After trigger is polled , the ADC will resume normal functionality, and > > > the consumer driver will continue to sleep. > > > > So this is where I'm unsure. Do you actually have a usecase where it > > makes the sense to read from the ADC only when there is no touch? Any > > system doing that has an obvious denial of service attack - touch the > > screen. > > > > You're right. We have an issue in this case due to the hardware. Using > touchscreen has side effects on other channels. We can use only one > trigger for all the channels. The situation would have been better with > a trigger dedicated to the touchscreen. > > At the moment, we have not really stated about the exclusive use or not > of the touchscreen. We suppose we can get some customers wanting to use > both touchscreen and ADC. Eugen tried to deal with this case but, as you > noticed, it can lead to DoS. It's a restriction people aren't going to expect unfortunately... > > > > We need to have a periodic trigger to sample the data because the actual > > > analog to digital conversion inside the IP block needs to be triggered. > > > The touchscreen data measurements cannot happen in hardware without > > > being triggered. If I try with a hrtimer to get a periodic IRQ to just > > > read the data, it will never be ready. The datasheet states that the > > > touchscreen measurements "will be attached to the conversion sequence". > > > So the periodic trigger is forcing a conversion sequence. This could be > > > done with a software trigger as well, but why the hassle to start it > > > every 2 milliseconds (or other time interval), if we can do it by > > > periodic trigger ? > > > > Ah, one reason here would be to allow separate consumers to use the > > device. In that case you'd run with a periodic trigger all the time > > and have two buffers attached, the buffer_cb that is feeding your > > touchscreen and another buffer to deal with the other channels > > (presumably the standard one an IIO device has when using buffered > > interfaces). > > The issue is that we are sharing the periodic trigger so we have to use > the same period for both usage. Whilst a somewhat irritating restriction, it's probably not disastrous for most ADC uses. Jonathan > > Regards > > Ludovic > > > > > The buffer demux would ensure the data from the right channels > > ends up in the right place. It makes it look to the buffer > > consumer like it is the only thing using / controlling the data > > flow. > > > > > Once we get the No pen IRQ, we stop the periodic trigger and it can be > > > used in another purpose (software or external as of now in the driver, > > > in the future we can add PWM trigger and Timer trigger) > > > > This case isn't really useful though as any other use is denied > > access when touch occurs. > > > > I'll summarise what I think would work for this below. > > > > > > > > In short, the ADC in Sama5D2 also supports touchscreen, and in > > > touchscreen mode , 4 of the channels are being used for this purpose. > > > This however, doesn't stop the ADC to use the other channels . The > > > hardware has 12 total single channels and they can be paired to have 6 > > > more differential channels. The only thing that is blocked is the > > > trigger, but only if the pen is touching (when we start the periodic > > > trigger to sample the touchscreen). If the pen is not touching, an > > > external trigger or software trigger can be used without any issues (so > > > why limit the functionality, if this is available from hardware ?). > > > Because of the reason I discussed above (touchscreen sequence must be > > > triggered), we cannot use another trigger in the same time. > > > > > > > > > I see your idea with the callback buffer and it's worth exploring. > > > Mainly this series was to actually show you what I had in mind about > > > supporting the resistive touchscreen, and to give you some actually > > > working code/patch, so we can discuss based on real implementation, not > > > just suppositions. > > > > That side of things is fine. > > > > > > > > You are right in many of the other comments that you said, and I will > > > come up with a v2 to this series. For now, I need to know if this is a > > > good or right direction in which I am going, or I should try to change > > > all the mechanism to callback buffer ? Or maybe I am totally in a bad > > > direction ? > > > The requirements are that the consumer driver needs to be somehow woke > > > up for every new touch data available, and report to the input > > > subsystem. As it was done before, the at91 old driver, just creates and > > > registers an input device by itself, and then reports the position and > > > touches. I was thinking that with this trigger consumer implementation, > > > things can be better in terms of subsystem separation and support. > > > > > > Thanks again and let me know of your thoughts, > > > > > > Eugen > > > > So a couple of things come to mind on how I'd structure this. > > So what we have (very briefly) > > > > No touch screen case: > > * Generic ADC using all sorts of different triggers > > > > Touch screen only case: > > * Interrupt to indicate pen on / off > > * A need to do a periodic trigger of the device but only > > useful when touch is in progress. > > > > Touch screen and other users: > > * Interrupt to indicate pen on / off > > * Periodic trigger needed for touchscreen when touch in progress. > > * Do not have denial of service on other channels. > > > > First two cases are easy enough by having a magic trigger, third > > case is harder. > > If we have the touchscreen then I would drop support for direct access to > > to ADC channels whilst it's in use (so no sysfs - or emulate it if you > > really want it by stashing results from scans done when touch is in > > progress). > > > > Have your touch screen channels just as normal additional channels, > > but only via the buffered interface (no _RAW attribute). > > If someone sets up to read them via buffered interface with > > a different trigger I think they'll get values - whether they > > are right is dependent (if I understand correctly) on whether > > there is a touch in progress. So no harm done and it'll make > > the logic simpler. > > > > The moment touch is opened and acquires the IIO channels > > fix the trigger (may need new interface) to the periodic one > > that you were enabling and disabling on touch. > > Things get dicey if there is an existing user so you may > > have to do it on driver probe rather than open of the input > > device if we effectively want touch to have the highest > > priority use of the ADC. > > > > If other channels are enabled for buffered mode then note > > this in the driver and have the periodic trigger on all the > > time (to ensure they keep getting read) This will pass > > garbage to your touch screen driver, but it'll remove it due > > to the pressure value being too low so no harm there. > > > > Normal path will work for non touch channels (and in theory > > the touch ones if they are turned on) via IIO buffer > > interface. It'll be restricted in form due to the needs of > > the touch driver, but better than nothing and should cover > > most usecases. > > > > Now the interrupt on / off on touch bit becomes an optimization > > in the case of only the buffer_cb being attached. > > > > I think that fits cleanly in the current IIO framework and > > looks more similar to our existing provider consumer approaches. > > > > Still needs the hooks to get hold of the trigger though so > > as to be able to tell the ADC which one to use. So rather > > than being a trigger consumer interface, it's more of a trigger > > configuration interface.. Exact term doesn't matter though. > > > > Jonathan > > > > > > > > > > > > > > [...] > > > -- > > > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > > > the body of a message to majordomo@vger.kernel.org > > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- 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
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 */ +#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++) + ; + + 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 */ + 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)) + 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; + + disable_irq_nosync(st->irq); + iio_trigger_poll(st->touch_st.trig); + + 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; + 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) + *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) { + 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); + 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:
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 Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com> --- drivers/iio/adc/at91-sama5d2_adc.c | 455 ++++++++++++++++++++++++++++++++++++- 1 file changed, 446 insertions(+), 9 deletions(-)