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. ------------------------------------------------------------------- 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
