Re: [PATCH 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver

[Chi-Wen Weng <cwweng.linux@gmail.com>]

Hi Jonathan,

Thanks for the review.

 > Look at the ACQUIRE() macros for claim direct stuff (in iio.h)
 > and then use guard() for this.  Be careful to add {} to define scope
 > to being this case block.  May not save much code but it will be easeir
 > to read than this currently is.

Understood. I will switch the direct read path to
IIO_DEV_ACQUIRE_DIRECT_MODE() / IIO_DEV_ACQUIRE_FAILED() and use
guard(mutex) for the driver lock. I will also add a scope around the
IIO_CHAN_INFO_RAW case block.

 > Move assignment down to just above the loop.  Makes it easier for
 > reviewers to associate the initial value with what is going on.

Will do.

 > For new code
 >     ret = devm_mutex_init(&adc->lock);
 >     if (ret)
 >         return ret;

Will fix this in v2.

 > Second part of this is set by the triggered_buffer call later. So 
don't set it
 > here.

Understood. I will keep only INDIO_DIRECT_MODE here and let
devm_iio_triggered_buffer_setup() set the triggered-buffer mode.

 > Failing suspend because a buffer is enabled is unlikely to be popular.
 > Can you not save necessary state and restore so buffered capture 
continues
 > on resume?

Yes, I will rework the PM callbacks so suspend does not fail just because
the buffer is enabled.

For v2, I plan to suspend the IIO triggering path, stop the EADC
conversion/interrupt path, save the state needed for buffered capture,
and then disable the ADC and its clock. On resume, I will re-enable the
clock, reinitialize the ADC, restore the scan configuration, and restart
the buffered conversion path if it was active before suspend.

Thanks,
Chi-Wen