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

[Jonathan Cameron]

On Thu, 14 May 2020 10:56:16 +0000
<Eugen.Hristev@xxxxxxxxxxxxx> wrote:

> On 04.05.2020 15:27, Eugen Hristev - M18282 wrote:
> > 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  
> 
> Hello Jonathan,
> 
> Without this patch, your current testing branch is broken for hardware 
> trigger for at91-sama5d2_adc (including for-5.8b tag)
> 
> You want me to create a patch that fixes that in the driver, or you will 
> take this patch maybe ?

Hmmm. Semantically I wasn't overly keen on this patch, but it does appear
harmless and will kind of make more sense once the big rework Alex
is doing is finished..  My concern was that preenable should see the
currentmode as the one before enabling - so not a buffered mode, 
and with this it doesn't.

I sanity checked the cases identified and they all seem fine so
let's go with it.

We are a bit near the merge window for me to be absolutely sure I'll get
another pull request out, so this might get shifted to the fixes branch
if I miss that.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> 
> Thanks !
> 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
> >>>  
> >>  
> >   
>