Message-ID: <20200602095406.00005add@Huawei.com>
Date:   Tue, 2 Jun 2020 09:54:06 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
CC:     "jic23@...nel.org" <jic23@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "alexandre.torgue@...com" <alexandre.torgue@...com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "songqiang1304521@...il.com" <songqiang1304521@...il.com>,
        "mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
        "lorenzo.bianconi83@...il.com" <lorenzo.bianconi83@...il.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "linux-stm32@...md-mailman.stormreply.com" 
        <linux-stm32@...md-mailman.stormreply.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 3/3] iio: remove
 iio_triggered_buffer_postenable()/iio_triggered_buffer_predisable()

On Tue, 2 Jun 2020 07:50:23 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@...log.com> wrote:

> On Sun, 2020-05-31 at 16:40 +0100, Jonathan Cameron wrote:
> > On Mon, 25 May 2020 14:38:55 +0300
> > Alexandru Ardelean <alexandru.ardelean@...log.com> wrote:
> >   
> > > From: Lars-Peter Clausen <lars@...afoo.de>
> > > 
> > > This patch should be squashed into the first one, as the first one is
> > > breaking the build (intentionally) to make the IIO core files easier to
> > > review.
> > > 
> > > Signed-off-by: Lars-Peter Clausen <lars@...afoo.de>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.com>
> > > ---  
> > 
> > Friend poke.  Version log?  
> 
> Version log is in the first patch.
> I was wondering if I omitted it.
> Seems, this time I didn't. But I admit, it probably would have been better
> here.
Ah fair enough.  That works fine if there is a cover letter but not
so much just putting things in the first patch!
> 
> > 
> > Other than the wistful comment below (which I'm not expecting you to
> > do anything about btw!) whole series looks good to me.
> > 
> > These are obviously no functional changes (I think) so it's only really
> > patch 2 that
> > could do with more eyes and acks.
> > 
> > Far as I can tell that case is fine as well because of the protections
> > on being in the right mode, but more eyes on that would be great.
> > 
> > So assuming that's fine, what commit message do you want me to use for
> > the fused single patch?  
> 
> Commit message-wise: I think the message in the first commit would be
> mostly sufficient.
> No idea what other description would be needed.
> 
> So, maybe something like:
> 
> ----------------------------------------------------------------------
> All devices using a triggered buffer need to attach and detach the trigger
> to the device in order to properly work. Instead of doing this in each and
> every driver by hand move this into the core.
> 
> At this point in time, all drivers should have been resolved to
> attach/detach the poll-function in the same order.
> 
> This patch removes all explicit calls of iio_triggered_buffer_postenable()
> & iio_triggered_buffer_predisable() in all drivers, since the core handles
> now the pollfunc attach/detach.
> 
> The more peculiar change is for the 'at91-sama5d2_adc' driver, since it's
> not obvious that removing the hooks doesn't break anything**
> ----------------------------------------------------------------------
> 

Looks good.

> ** for the comment about 'at91-sama5d2_adc', we really do need to get some
> testing; otherwise this risks breaking it.

Agreed.  

> 
> 
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> > >  static const struct iio_trigger_ops atlas_interrupt_trigger_ops = {
> > > diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c
> > > b/drivers/iio/dummy/iio_simple_dummy_buffer.c
> > > index 17606eca42b4..8e13c53d4360 100644
> > > --- a/drivers/iio/dummy/iio_simple_dummy_buffer.c
> > > +++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c
> > > @@ -99,20 +99,6 @@ static irqreturn_t iio_simple_dummy_trigger_h(int
> > > irq, void *p)
> > >  }
> > >  
> > >  static const struct iio_buffer_setup_ops
> > > iio_simple_dummy_buffer_setup_ops = {
> > > -	/*
> > > -	 * iio_triggered_buffer_postenable:
> > > -	 * Generic function that simply attaches the pollfunc to the
> > > trigger.
> > > -	 * Replace this to mess with hardware state before we attach the
> > > -	 * trigger.
> > > -	 */
> > > -	.postenable = &iio_triggered_buffer_postenable,
> > > -	/*
> > > -	 * iio_triggered_buffer_predisable:
> > > -	 * Generic function that simple detaches the pollfunc from the
> > > trigger.
> > > -	 * Replace this to put hardware state back again after the trigger
> > > is
> > > -	 * detached but before userspace knows we have disabled the ring.
> > > -	 */
> > > -	.predisable = &iio_triggered_buffer_predisable,
> > >  };
> > >    
> > Hmm. Guess we should probably 'invent' a reason to illustrate the bufer
> > ops in the dummy example.  Anyone feeling creative?  
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Powered by blists - more mailing lists