Message-ID: <37ef45f4-9330-86ea-77c1-3138bb88601b@microchip.com>
Date:   Wed, 17 Jun 2020 13:37:20 +0000
From:   <Eugen.Hristev@...rochip.com>
To:     <Jonathan.Cameron@...wei.com>, <alexandru.Ardelean@...log.com>
CC:     <jic23@...nel.org>, <lars@...afoo.de>, <s.hauer@...gutronix.de>,
        <alexandre.torgue@...com>, <linux-iio@...r.kernel.org>,
        <linus.walleij@...aro.org>, <linux-kernel@...r.kernel.org>,
        <songqiang1304521@...il.com>, <mcoquelin.stm32@...il.com>,
        <lorenzo.bianconi83@...il.com>, <shawnguo@...nel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 3/3] iio: remove
 iio_triggered_buffer_postenable()/iio_triggered_buffer_predisable()

On 02.06.2020 11:54, Jonathan Cameron wrote:
> 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.
> 

Hi,

I can test it, do we have any patchwork so I can easily download the 
patches ?
I have issues when applying them.

Thanks !

> 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