Re: [PATCH v2 3/3] iio: adc: at91-sama5d2_adc: update for other trigger usage

[Jonathan Cameron]

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,  
> > 
> > 
> >