| Message ID | 1578917098-9674-4-git-send-email-eugen.hristev@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enhancements to at91-sama5d2_adc driver | expand |
On Mon, 13 Jan 2020 12:07:10 +0000 <Eugen.Hristev@microchip.com> wrote: > From: Eugen Hristev <eugen.hristev@microchip.com> > > 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@microchip.com> +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. > 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. > + > 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,
On 17.01.2020 19:42, Jonathan Cameron wrote: > On Mon, 13 Jan 2020 12:07:10 +0000 > <Eugen.Hristev@microchip.com> wrote: > >> From: Eugen Hristev <eugen.hristev@microchip.com> >> >> 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@microchip.com> > > +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 ? > >> + >> 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, > > >
On Mon, 20 Jan 2020 07:24:50 +0000 <Eugen.Hristev@microchip.com> wrote: > On 17.01.2020 19:42, Jonathan Cameron wrote: > > > On Mon, 13 Jan 2020 12:07:10 +0000 > > <Eugen.Hristev@microchip.com> wrote: > > > >> From: Eugen Hristev <eugen.hristev@microchip.com> > >> > >> 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@microchip.com> > > > > +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, > > > > > >
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); 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); + 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,