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

[<Eugen.Hristev@microchip.com>]

On 03.12.2019 14:04, Ardelean, Alexandru wrote:

> On Tue, 2019-12-03 at 09:49 +0000, Eugen.Hristev@microchip.com wrote:
>> [External]
>>
>>
>>
>> On 29.11.2019 09:02, Ardelean, Alexandru wrote:
>>
>>> On Thu, 2019-11-28 at 15:19 +0000, Eugen.Hristev@microchip.com wrote:
>>>
>>> Hey,
>>>
>>> Sorry for the late reply.
>>> I'm also juggling a few things.
>>>
>>>> On 28.11.2019 10:36, Eugen.Hristev@microchip.com wrote:
>>>>
>>>>> 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.
>>>>
>>>> I looked a bit more into the code and in DMA case, using postenable
>>>> first will lead to calling attach pollfunc, which will also enable
>>>> the
>>>> trigger, but the DMA is not yet started.
>>>> Is this the desired effect ?
>>>
>>> Yes.
>>
>> How is this correct ? We start the trigger but have no buffer to carry
>> to... what happens with the data ? -> I think we both have an answer to
>> that, as you state below
>>
>>>> Normally when using DMA I would say we
>>>> would need to enable DMA first to be ready to carry data (and
>>>> coherent
>>>> area etc.) and then enable the trigger.
>>>
>>> So, there is a change in our tree [from some time ago].
>>> See here:
>>> https://github.com/analogdevicesinc/linux/commit/eee97d12665fef8cf429a1e5035b23ae969705b8
>>>
>>> Particularly, what's interesting is around line:
>>> https://github.com/analogdevicesinc/linux/commit/eee97d12665fef8cf429a1e5035b23ae969705b8#diff-0a87744ce945d2c1c89ea19f21fb35bbR722
>>> And you may need to expand some stuff to see more of the function-body.
>>> And some things may have changed in upstream IIO since that change.
>>>
>>> The change is to make the pollfunc attach/detach become part of the IIO
>>> framework, because plenty of drivers just call
>>> iio_triggered_buffer_postenable() & iio_triggered_buffer_predisable()
>>> to
>>> manually attach/detach the pollfunc for triggered buffers.
>>
>> Okay, I understand this. at91-sama5d2_adc does not manually
>> attach/detach the pollfunc. So why do we need to change anything here ?
>>
>>
>>> That change is from 2015, and since then, some drivers were added that
>>> just
>>> manually attach/detach the pollfunc [and do nothing more with the
>>> postenable/predisable hooks].
>>>
>>> I tried to upstream a more complete version of that patch a while ago
>>> [u1].
>>> https://patchwork.kernel.org/patch/10482167/
>>> https://patchwork.kernel.org/patch/10737291/
>>>
>>> The conclusion was to first fix the attach/detach pollfunc order in all
>>> IIO
>>> drivers, so that when patch [u1] is applied, there is no more
>>> discussion
>>> about the correct order for attach/detach pollfunc.
>>
>> Allright, what is required to be fixed regarding the order, in this
>> specific case? We enable the DMA, and then we do the normal 'postenable'
>> that was called anyway if we did not override the 'postenable' in the
>> ops. Do you want to move this code to 'preenable' and keep 'postenable'
>> to the standard subsystem one ?
>>
>> The same applies to the predisable, we first call the subsystem
>> 'predisable' then do the specific at91 stuff. You want to move this to
>> the 'postdisable' ?
>>
>> I think reverting the order inside the functions themselves is not good
>> as we replace the order of starting trigger/DMA setup.
>> So, coming to your question below...
>>
>>> Coming back here [and to your question], my answer is: I don't know if
>>> the
>>> at91 DMA needs to be enabled/disabled before/after the pollfunc
>>> attach/detach.
>>> This sounds like specific stuff for at91 [which is fine].
>>>
>>> It could be that some other hooks may need to used to enable DMA
>>> before/after the attach/detach pollfunc. Maybe
>>> preenable()/postdisable() ?
>>>
>>> In any case, what I would like [with this discussion], is to resolve a
>>> situation where we can get closer to moving the attach/pollfunc code to
>>> IIO
>>> core. So, if AT91 requires a different ordering, I think you would be
>>> more
>>> appropriate to tell me, and propose an alternative to this patch.
>>
>> ... yes, this looks more appropriate, to move things to
>> 'preenable/postdisable', if you feel like 'postenable/predisable' is not
>> the proper place to put them.
>> But the order itself, first enable DMA then trigger, and disable in
>> reverse order, I do not think there is anything wrong with that? Am I
>> misunderstanding ?
> 
> Should be good.
> 
>>
>> If Jonathan or Ludovic have a different idea, please let me know.
> 
> There is an alternative here [to this].
> Maybe using the IIO Buffer DMA[Engine] integration that Lars wrote [1].
> This would avoid calling dmaengine_terminate_sync() and similar hooks in
> the AT91 driver. That also preserves the correct order (start DMA first,
> then attach pollfunc ; and reverse on disable).
> But that is more work; not on the patch itself, but more on the testing.

Initially, when I implemented the DMA part for this driver, this was the 
idea. However the DMA engine was not used at that time by anyone , and I 
could not make it work properly. Jonathan advised at that moment to use 
this current framework.

> 
> [1] Upstreaming more parts for the IIO Buffer DMA[Engine] integration is on
> my to-do-list as well. I think there are still some patches that we use,
> but are not upstreamed yet.
> 
> I'll come-up a with a V2 for this with preenable()/postdisable()
> alternative here.

Ok, I will test it .

What I do not understand completely is why it bothers you to have at91 
specific code in postenable / predisable.
The same thing will happen will happen with preenable/postdisable: 
specific at91 code will be called after subsystem preenable and before 
subsystem postdisable.

> 
> Thanks
> Alex
> 
>>
>> Also, I can test your patch to see if everything is fine.
>>
>> Thanks,
>> Eugen
>>
>>> Thanks :)
>>> Alex
>>>
>>>>>>> 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.
>>>>>
>>>
>>> Sure.
>>> Will do that.
>>>
>>> I did not bother much with these labels, because after applying [u1],
>>> some
>>> of them [maybe all] should go away.
>>>
>>>
>>>>> 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
>>>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel