On Mon, 20 Jan 2020 07:24:50 +0000
<Eugen.Hristev@xxxxxxxxxxxxx> wrote:
> On 17.01.2020 19:42, Jonathan Cameron wrote:
>
> > On Mon, 13 Jan 2020 12:07:10 +0000
> > <Eugen.Hristev@xxxxxxxxxxxxx> wrote:
> >
> >> From: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
> >>
> >> This change will allow the at91-sama5d2_adc driver to use other triggers
> >> than it's own.
> >> In particular, tested with the sysfs trigger.
> >> To be able to achieve this functionality, some changes were required:
> >> 1) Do not enable/disable channels when enabling/disabling the trigger.
> >> This is because the trigger is enabled/disabled only for our trigger
> >> (obviously). We need channels enabled/disabled regardless of what trigger is
> >> being used.
> >> 2) Cope with DMA : DMA cannot be used when using another type of trigger.
> >> Other triggers work through pollfunc, so we get polled anyway on every trigger.
> >> Thus we have to obtain data at every trigger.
> >> 3) When to start conversion? The usual pollfunc (store time from subsystem)
> >> would be in hard irq and this would be a good way, but current iio subsystem
> >> recommends to have it in the threaded irq. Thus adding software start
> >> code in this handler.
> >> 4) Buffer config: we need to setup buffer regardless of our own device's
> >> trigger. We may get one attached later.
> >> 5) IRQ handling: we use our own device IRQ only if it's our own trigger
> >> and we do not use DMA . If we use DMA, we use the DMA controller's IRQ.
> >>
> >> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
> >
> > +CC Alexandru as he's doing a lot of cleanup around the buffer functions.
> > I'd like Alex to take a look at this.
> >
> > A few comments inline from me.
> >
> > Thanks,
> >
> > Jonathan
> >
> >
> >
> >> ---
> >> Changes in v2:
> >> - adapt to the situation of having the previous two patches ahead in the series
> >>
> >> drivers/iio/adc/at91-sama5d2_adc.c | 140 +++++++++++++++++++------------------
> >> 1 file changed, 73 insertions(+), 67 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> >> index 454a493..34df043 100644
> >> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> >> @@ -728,7 +728,6 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> >> struct iio_dev *indio = iio_trigger_get_drvdata(trig);
> >> struct at91_adc_state *st = iio_priv(indio);
> >> u32 status = at91_adc_readl(st, AT91_SAMA5D2_TRGR);
> >> - u8 bit;
> >>
> >> /* clear TRGMOD */
> >> status &= ~AT91_SAMA5D2_TRGR_TRGMOD_MASK;
> >> @@ -739,45 +738,6 @@ static int at91_adc_configure_trigger(struct iio_trigger *trig, bool state)
> >> /* set/unset hw trigger */
> >> at91_adc_writel(st, AT91_SAMA5D2_TRGR, status);
> >>
> >> - for_each_set_bit(bit, indio->active_scan_mask, indio->num_channels) {
> >> - struct iio_chan_spec const *chan = at91_adc_chan_get(indio, bit);
> >> - u32 cor;
> >> -
> >> - if (!chan)
> >> - continue;
> >> - /* these channel types cannot be handled by this trigger */
> >> - if (chan->type == IIO_POSITIONRELATIVE ||
> >> - chan->type == IIO_PRESSURE)
> >> - continue;
> >> -
> >> - if (state) {
> >> - cor = at91_adc_readl(st, AT91_SAMA5D2_COR);
> >> -
> >> - if (chan->differential)
> >> - cor |= (BIT(chan->channel) |
> >> - BIT(chan->channel2)) <<
> >> - AT91_SAMA5D2_COR_DIFF_OFFSET;
> >> - else
> >> - cor &= ~(BIT(chan->channel) <<
> >> - AT91_SAMA5D2_COR_DIFF_OFFSET);
> >> -
> >> - at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
> >> - }
> >> -
> >> - if (state)
> >> - at91_adc_writel(st, AT91_SAMA5D2_CHER,
> >> - BIT(chan->channel));
> >> - else
> >> - at91_adc_writel(st, AT91_SAMA5D2_CHDR,
> >> - BIT(chan->channel));
> >> - }
> >> - /* enable irq only if not using DMA */
> >> - if (state && !st->dma_st.dma_chan)
> >> - at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
> >> - /* disable irq only if not using DMA */
> >> - if (!state && !st->dma_st.dma_chan)
> >> - at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
> >> -
> >> return 0;
> >> }
> >>
> >> @@ -901,9 +861,22 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev)
> >> return 0;
> >> }
> >>
> >> +static bool at91_adc_buffer_check_use_irq(struct iio_dev *indio,
> >> + struct at91_adc_state *st)
> >> +{
> >> + /* if using DMA, we do not use our own IRQ (we use DMA-controller) */
> >> + if (st->dma_st.dma_chan)
> >> + return false;
> >> + /* if the trigger is not ours, then it has its own IRQ */
> >> + if (iio_trigger_validate_own_device(indio->trig, indio))
> >> + return false;
> >> + return true;
> >> +}
> >> +
> >> static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
> >> {
> >> int ret;
> >> + u8 bit;
> >> struct at91_adc_state *st = iio_priv(indio_dev);
> >>
> >> /* check if we are enabling triggered buffer or the touchscreen */
> >> @@ -921,9 +894,40 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio_dev)
> >> ret = at91_adc_dma_start(indio_dev);
> >> if (ret) {
> >> dev_err(&indio_dev->dev, "buffer postenable failed\n");
> >> + iio_triggered_buffer_predisable(indio_dev);
> >
> > This seems odd given you have called the iio_triggered_buffer_postenable yet..
> > That is below.
>
> Hi Jonathan,
>
> You are right, I will remove this.
>
> >
> >> return ret;
> >> }
> >>
> >> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> >> + indio_dev->num_channels) {
> >> + struct iio_chan_spec const *chan =
> >> + at91_adc_chan_get(indio_dev, bit);
> >> + u32 cor;
> >> +
> >> + if (!chan)
> >> + continue;
> >> + /* these channel types cannot be handled by this trigger */
> >> + if (chan->type == IIO_POSITIONRELATIVE ||
> >> + chan->type == IIO_PRESSURE)
> >> + continue;
> >> +
> >> + cor = at91_adc_readl(st, AT91_SAMA5D2_COR);
> >> +
> >> + if (chan->differential)
> >> + cor |= (BIT(chan->channel) | BIT(chan->channel2)) <<
> >> + AT91_SAMA5D2_COR_DIFF_OFFSET;
> >> + else
> >> + cor &= ~(BIT(chan->channel) <<
> >> + AT91_SAMA5D2_COR_DIFF_OFFSET);
> >> +
> >> + at91_adc_writel(st, AT91_SAMA5D2_COR, cor);
> >> +
> >> + at91_adc_writel(st, AT91_SAMA5D2_CHER, BIT(chan->channel));
> >> + }
> >> +
> >> + if (at91_adc_buffer_check_use_irq(indio_dev, st))
> >> + at91_adc_writel(st, AT91_SAMA5D2_IER, AT91_SAMA5D2_IER_DRDY);
> >> +
> >> return iio_triggered_buffer_postenable(indio_dev);
> >> }
> >>
> >> @@ -944,21 +948,11 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
> >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
> >> return -EINVAL;
> >>
> >> - /* continue with the triggered buffer */
> >> - ret = iio_triggered_buffer_predisable(indio_dev);
> >> - if (ret < 0)
> >> - dev_err(&indio_dev->dev, "buffer predisable failed\n");
> >> -
> >> - if (!st->dma_st.dma_chan)
> >> - return ret;
> >> -
> >> - /* if we are using DMA we must clear registers and end DMA */
> >> - dmaengine_terminate_sync(st->dma_st.dma_chan);
> >> -
> >> /*
> >> - * For each enabled channel we must read the last converted value
> >> + * For each enable channel we must disable it in hardware.
> >> + * In the case of DMA, we must read the last converted value
> >> * to clear EOC status and not get a possible interrupt later.
> >> - * This value is being read by DMA from LCDR anyway
> >> + * This value is being read by DMA from LCDR anyway, so it's not lost.
> >> */
> >> for_each_set_bit(bit, indio_dev->active_scan_mask,
> >> indio_dev->num_channels) {
> >> @@ -971,12 +965,28 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
> >> if (chan->type == IIO_POSITIONRELATIVE ||
> >> chan->type == IIO_PRESSURE)
> >> continue;
> >> +
> >> + at91_adc_writel(st, AT91_SAMA5D2_CHDR, BIT(chan->channel));
> >> +
> >> if (st->dma_st.dma_chan)
> >> at91_adc_readl(st, chan->address);
> >> }
> >>
> >> + if (at91_adc_buffer_check_use_irq(indio_dev, st))
> >> + at91_adc_writel(st, AT91_SAMA5D2_IDR, AT91_SAMA5D2_IER_DRDY);
> >> +
> >> /* read overflow register to clear possible overflow status */
> >> at91_adc_readl(st, AT91_SAMA5D2_OVER);
> >> +
> >> + /* continue with the triggered buffer */
> >> + ret = iio_triggered_buffer_predisable(indio_dev);
> >> + if (ret < 0)
> >> + dev_err(&indio_dev->dev, "buffer predisable failed\n");
> >> +
> >> + /* if we are using DMA we must clear registers and end DMA */
> >> + if (st->dma_st.dma_chan)
> >> + dmaengine_terminate_sync(st->dma_st.dma_chan);
> >
> > This ordering is going to stop Alex doing his rework to remove the need
> > to manually call iio_triggered_buffer_predisable. Why does it make
> > sense to do the dma stuff after that?
> >
> > Ah I see it always did and the postenable is the opposite of what Alex
> > has been moving to as well.
>
> Ok, so keep it like this ?
For this series yes. But I'd like input from Alex on this more generally!
Thanks,
Jonathan
>
> >
> >> +
> >> return ret;
> >> }
> >>
> >> @@ -1131,6 +1141,13 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> >> struct iio_dev *indio_dev = pf->indio_dev;
> >> struct at91_adc_state *st = iio_priv(indio_dev);
> >>
> >> + /*
> >> + * If it's not our trigger, start a conversion now, as we are
> >> + * actually polling the trigger now.
> >> + */
> >> + if (iio_trigger_validate_own_device(indio_dev->trig, indio_dev))
> >> + at91_adc_writel(st, AT91_SAMA5D2_CR, AT91_SAMA5D2_CR_START);
> >> +
> >> if (st->dma_st.dma_chan)
> >> at91_adc_trigger_handler_dma(indio_dev);
> >> else
> >> @@ -1143,20 +1160,9 @@ static irqreturn_t at91_adc_trigger_handler(int irq, void *p)
> >>
> >> static int at91_adc_buffer_init(struct iio_dev *indio)
> >> {
> >> - struct at91_adc_state *st = iio_priv(indio);
> >> -
> >> - if (st->selected_trig->hw_trig) {
> >> - return devm_iio_triggered_buffer_setup(&indio->dev, indio,
> >> - &iio_pollfunc_store_time,
> >> - &at91_adc_trigger_handler, &at91_buffer_setup_ops);
> >> - }
> >> - /*
> >> - * we need to prepare the buffer ops in case we will get
> >> - * another buffer attached (like a callback buffer for the touchscreen)
> >> - */
> >> - indio->setup_ops = &at91_buffer_setup_ops;
> >> -
> >> - return 0;
> >> + return devm_iio_triggered_buffer_setup(&indio->dev, indio,
> >> + &iio_pollfunc_store_time,
> >> + &at91_adc_trigger_handler, &at91_buffer_setup_ops);
> >> }
> >>
> >> static unsigned at91_adc_startup_time(unsigned startup_time_min,
> >
> >
> >
