On Thu, 2019-11-28 at 15:19 +0000, Eugen.Hristev@xxxxxxxxxxxxx wrote:
>
Hey,
Sorry for the late reply.
I'm also juggling a few things.
>
> On 28.11.2019 10:36, Eugen.Hristev@xxxxxxxxxxxxx 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.
> 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.
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.
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.
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@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxxxxxxx
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > >
