Re: [RFC PATCH] iio: __iio_update_buffers: Update mode before preenable/after postdisable

[Eugen.Hristev]

On 30.04.2020 11:42, Eugen Hristev - M18282 wrote:
> On 30.04.2020 11:24, Alexandru Ardelean wrote:
>> From: Lars-Peter Clausen <lars@xxxxxxxxxx>
>>
>> It is clear that we transition to INDIO_DIRECT_MODE when disabling the
>> buffer(s) and it is also clear that we transition from INDIO_DIRECT_MODE
>> when enabling the buffer(s). So leaving the currentmode field
>> INDIO_DIRECT_MODE until after the preenable() callback and updating it to
>> INDIO_DIRECT_MODE before the postdisable() callback doesn't add additional
>> value. On the other hand some drivers will need to perform different
>> actions depending on which mode the device is going to operate in/was
>> operating in.
>>
>> Moving the update of currentmode before preenable() and after postdisable()
>> enables us to have drivers which perform mode dependent actions in those
>> callbacks.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
>> ---
>>
>> This patch is also a V2 of this older patch from a while ago:
>>    https://lore.kernel.org/linux-iio/1431525891-19285-5-git-send-email-lars@xxxxxxxxxx/
>>
>> However, in this recent context, it comes to fix this:
>>    https://lore.kernel.org/linux-iio/b9ab676489de3575984dac5610fcf05fd8742a38.camel@xxxxxxxxxx/T/#mc09284c8f79250b92a52fd5b8d1f541d1c02c0c0
>>
>> At this point, I don't have a clear idea if this approach is good or
>> not; since the motivation is to fix the at91 adc.
>> Hence the RFC.
>>
>> Some excerpt from the AT91 discussion:
>> -------------------------------------------------------------------
>> I decided to do a bit of shell magic for this:
>>
>> get_files() {
>> git grep -w iio_buffer_setup_ops  | grep drivers | cut -d: -f1 | sort | uniq
>> }
>>
>> for file in $(get_files) ; do
>>       if grep -q currentmode $file ; then
>>           echo $file
>>       fi
>> done
>>
>> It finds 4 drivers.
>> Though, `get_files()` will return 56 files.
>>
>> drivers/iio/accel/bmc150-accel-core.c
>> drivers/iio/adc/at91-sama5d2_adc.c
>> drivers/iio/adc/stm32-dfsdm-adc.c
>> drivers/iio/magnetometer/rm3100-core.c
>>
>> The rm3100 driver doesn't do any checks in the setup_ops for 'currentmode' as
>> far as I could see.
>>
>> So, Lars' patch could work nicely to fix this current case and not break others.
>>
>> Semantically though, it would sound nicer to have a 'nextmode' parameter
>> somewhere; maybe on the setup_ops(indio_dev, nextmode)?
>> Though, only those 3 drivers would really ever use it; so doing it like that
>> sounds like overkill.
>>
>> So, we're left with Lars' patch or we could add an 'indio_dev->nextmode' field,
>> that may be used in just these 3 drivers [which again: sounds overkill at this
>> point in time].
>>
>> Alternatively, this 'indio_dev->currentmode' could be removed from all these 3
>> drivers somehow. But that needs testing and a thorough understanding of all 3
>> drivers and what they're doing, to do properly.
>> -------------------------------------------------------------------
> 
> Hi Alex,
> 
> Thanks for finding this. I will test this with the at91-sama5d2_adc
> driver on Jonathan's testing branch. I will let you know of the results.
> 
> Eugen

Hi,

For sama5d2-xplained, hw trigger testing, at91-sama5d2_adc driver,
Tested-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>

I did not get my hands on the touchscreen yet, but hopefully soon.

Thanks again,
Eugen
> 
>>
>>    drivers/iio/industrialio-buffer.c | 8 +++-----
>>    1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
>> index 30af8af8f312..efcc44b62946 100644
>> --- a/drivers/iio/industrialio-buffer.c
>> +++ b/drivers/iio/industrialio-buffer.c
>> @@ -989,6 +989,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>>           indio_dev->active_scan_mask = config->scan_mask;
>>           indio_dev->scan_timestamp = config->scan_timestamp;
>>           indio_dev->scan_bytes = config->scan_bytes;
>> +       indio_dev->currentmode = config->mode;
>>
>>           iio_update_demux(indio_dev);
>>
>> @@ -1024,8 +1025,6 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>>                           goto err_disable_buffers;
>>           }
>>
>> -       indio_dev->currentmode = config->mode;
>> -
>>           if (indio_dev->setup_ops->postenable) {
>>                   ret = indio_dev->setup_ops->postenable(indio_dev);
>>                   if (ret) {
>> @@ -1042,10 +1041,10 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>>                                                buffer_list)
>>                   iio_buffer_disable(buffer, indio_dev);
>>    err_run_postdisable:
>> -       indio_dev->currentmode = INDIO_DIRECT_MODE;
>>           if (indio_dev->setup_ops->postdisable)
>>                   indio_dev->setup_ops->postdisable(indio_dev);
>>    err_undo_config:
>> +       indio_dev->currentmode = INDIO_DIRECT_MODE;
>>           indio_dev->active_scan_mask = NULL;
>>
>>           return ret;
>> @@ -1080,8 +1079,6 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
>>                           ret = ret2;
>>           }
>>
>> -       indio_dev->currentmode = INDIO_DIRECT_MODE;
>> -
>>           if (indio_dev->setup_ops->postdisable) {
>>                   ret2 = indio_dev->setup_ops->postdisable(indio_dev);
>>                   if (ret2 && !ret)
>> @@ -1090,6 +1087,7 @@ static int iio_disable_buffers(struct iio_dev *indio_dev)
>>
>>           iio_free_scan_mask(indio_dev, indio_dev->active_scan_mask);
>>           indio_dev->active_scan_mask = NULL;
>> +       indio_dev->currentmode = INDIO_DIRECT_MODE;
>>
>>           return ret;
>>    }
>> --
>> 2.17.1
>>
>