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