diff mbox series

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

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

Commit Message

Eugen Hristev Jan. 13, 2020, 12:07 p.m. UTC
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>
---
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(-)

Comments

Jonathan Cameron Jan. 17, 2020, 5:42 p.m. UTC | #1
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,
Eugen Hristev Jan. 20, 2020, 7:24 a.m. UTC | #2
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,
> 
> 
>
Jonathan Cameron Jan. 27, 2020, 12:13 p.m. UTC | #3
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 mbox series

Patch

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,