Re: [PATCH] iio: at91-sama5d2_adc: fix iio_triggered_buffer_{predisable,postenable} positions

[<Eugen.Hristev@microchip.com>]

On 25.11.2019 17:03, Ardelean, Alexandru wrote:
> On Wed, 2019-10-23 at 11:25 +0300, Alexandru Ardelean wrote:
>> The iio_triggered_buffer_{predisable,postenable} functions attach/detach
>> poll functions.
>>
>> The iio_triggered_buffer_postenable() should be called first to attach
>> the
>> poll function, and then the driver can init the data to be triggered.
>>
>> Similarly, iio_triggered_buffer_predisable() should be called last to
>> first
>> disable the data (to be triggered) and then the poll function should be
>> detached.

Hi Alexandru,

Sorry for this late reply,

I remember that by adding specific at91_adc code for 
predisable/postenable , I was replacing the existing standard callback 
with my own, and have my specific at91 code before postenable and then 
calling the subsystem postenable,
and in similar way, for predisable, first call the subsystem predisable 
then doing my predisable code (in reverse order as in postenable)

If you say the order should be reversed (basically have the pollfunction 
first), how is current code working ?
Should current code fail if the poll function is not attached in time ? 
Or there is a race between triggered data and the attachment of the 
pollfunc ?

I am thinking that attaching the pollfunc later makes it work because 
the DMA is not started yet. What happens if we have the pollfunc 
attached but DMA is not started (basically the trigger is not started) , 
can this lead to unexpected behavior ? Like the pollfunc polling but no 
trigger started/no DMA started.

>>
>> For this driver, the predisable & postenable hooks are also need to take
>> into consideration the touchscreen, so the hooks need to be put in places
>> that avoid the code for that cares about it.
>>
> 
> ping here
> 
>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>> ---
>>   drivers/iio/adc/at91-sama5d2_adc.c | 19 ++++++++++---------
>>   1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-
>> sama5d2_adc.c
>> index e1850f3d5cf3..ac3e5c4c9840 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -889,20 +889,24 @@ static int at91_adc_buffer_postenable(struct
>> iio_dev *indio_dev)
>>        if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES))
>>                return -EINVAL;
>>
>> +     ret = iio_triggered_buffer_postenable(indio_dev);
>> +     if (ret)
>> +             return ret;
>> +
>>        /* we continue with the triggered buffer */
>>        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;
>>        }
>>
>> -     return iio_triggered_buffer_postenable(indio_dev);
>> +     return 0;
>>   }
>>
>>   static int at91_adc_buffer_predisable(struct iio_dev *indio_dev)
>>   {
>>        struct at91_adc_state *st = iio_priv(indio_dev);
>> -     int ret;
>>        u8 bit;
>>
>>        /* check if we are disabling triggered buffer or the touchscreen */
>> @@ -916,13 +920,8 @@ 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;
>> +             goto out;
>>
>>        /* if we are using DMA we must clear registers and end DMA */
>>        dmaengine_terminate_sync(st->dma_st.dma_chan);
>> @@ -949,7 +948,9 @@ static int at91_adc_buffer_predisable(struct iio_dev
>> *indio_dev)
>>
>>        /* read overflow register to clear possible overflow status */
>>        at91_adc_readl(st, AT91_SAMA5D2_OVER);
>> -     return ret;
>> +
>> +out:


I would prefer if this label is named with a function name prefix, 
otherwise 'out' is pretty generic and can collide with other things in 
the file... I want to avoid having an out2 , out3 later if code changes.

Thanks for the patch,
Eugen

>> +     return iio_triggered_buffer_predisable(indio_dev);
>>   }
>>
>>   static const struct iio_buffer_setup_ops at91_buffer_setup_ops = {
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel