lkml.org 
[lkml]   [2019]   [Dec]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH] iio: at91-sama5d2_adc: fix iio_triggered_buffer_{predisable,postenable} positions
    Date
    On Tue, 2019-12-03 at 12:17 +0000, Eugen.Hristev@microchip.com wrote:
    >
    > 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.

    Because I am preparing a framework change to IIO core and all IIO drivers
    in mainline need to be resolved when that change happens.
    I am not sure if the change will break any driver, but at least we can
    minimalize breakage.

    >
    > > 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
    > > > >
    \
     
     \ /
      Last update: 2019-12-03 14:41    [W:4.228 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site