On Sun, Jan 28, 2024 at 9:06 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
> Given we now have iio_device_claim_direct_scoped() to perform automatic
> releasing of direct mode at exit from the scope that follows it, this can
> be used in conjunction with guard(mutex) etc remove a lot of special case
> handling.
>
> Note that in this particular example code, there is no real reason you can't
> read channels via sysfs at the same time as filling the software buffer.
> To make it look more like a real driver constrain raw and processed
> channel reads from occurring whilst the buffer is in use.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> ---
> Since RFC:
> - Dropped a stale comment about a local variable that existed
> in an earlier version of this patch before the new scoped_guard_cond()
> infrastructure was added.
> - Use unreachable() to convince the compiler we can't get to code
> at end of the pattern.
>
> iio_device_claim_direct_scoped(return -EBUSY, iio_dev) {
> return 0;
> }
> unreacahable();
> ---
> drivers/iio/dummy/iio_simple_dummy.c | 182 +++++++++++++--------------
> 1 file changed, 88 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/iio/dummy/iio_simple_dummy.c b/drivers/iio/dummy/iio_simple_dummy.c
> index c24f609c2ade..d6ef556698fb 100644
> --- a/drivers/iio/dummy/iio_simple_dummy.c
> +++ b/drivers/iio/dummy/iio_simple_dummy.c
> @@ -283,65 +283,63 @@ static int iio_dummy_read_raw(struct iio_dev *indio_dev,
> long mask)
> {
> struct iio_dummy_state *st = iio_priv(indio_dev);
> - int ret = -EINVAL;
>
> - mutex_lock(&st->lock);
> switch (mask) {
> case IIO_CHAN_INFO_RAW: /* magic value - channel value read */
> - switch (chan->type) {
> - case IIO_VOLTAGE:
> - if (chan->output) {
> - /* Set integer part to cached value */
> - *val = st->dac_val;
> - ret = IIO_VAL_INT;
> - } else if (chan->differential) {
> - if (chan->channel == 1)
> - *val = st->differential_adc_val[0];
> - else
> - *val = st->differential_adc_val[1];
> - ret = IIO_VAL_INT;
> - } else {
> - *val = st->single_ended_adc_val;
> - ret = IIO_VAL_INT;
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + guard(mutex)(&st->lock);
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + if (chan->output) {
> + /* Set integer part to cached value */
> + *val = st->dac_val;
> + return IIO_VAL_INT;
> + } else if (chan->differential) {
> + if (chan->channel == 1)
> + *val = st->differential_adc_val[0];
> + else
> + *val = st->differential_adc_val[1];
> + return IIO_VAL_INT;
> + } else {
> + *val = st->single_ended_adc_val;
> + return IIO_VAL_INT;
> + }
> +
> + case IIO_ACCEL:
> + *val = st->accel_val;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> }
> - break;
> - case IIO_ACCEL:
> - *val = st->accel_val;
> - ret = IIO_VAL_INT;
> - break;
> - default:
> - break;
> }
> - break;
> + unreachable();
> case IIO_CHAN_INFO_PROCESSED:
> - switch (chan->type) {
> - case IIO_STEPS:
> - *val = st->steps;
> - ret = IIO_VAL_INT;
> - break;
> - case IIO_ACTIVITY:
> - switch (chan->channel2) {
> - case IIO_MOD_RUNNING:
> - *val = st->activity_running;
> - ret = IIO_VAL_INT;
> - break;
> - case IIO_MOD_WALKING:
> - *val = st->activity_walking;
> - ret = IIO_VAL_INT;
> - break;
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + guard(mutex)(&st->lock);
> + switch (chan->type) {
> + case IIO_STEPS:
> + *val = st->steps;
> + return IIO_VAL_INT;
> + case IIO_ACTIVITY:
> + switch (chan->channel2) {
> + case IIO_MOD_RUNNING:
> + *val = st->activity_running;
> + return IIO_VAL_INT;
> + case IIO_MOD_WALKING:
> + *val = st->activity_walking;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> default:
> - break;
> + return -EINVAL;
> }
> - break;
> - default:
> - break;
> }
> - break;
> + unreachable();
> case IIO_CHAN_INFO_OFFSET:
> /* only single ended adc -> 7 */
> *val = 7;
> - ret = IIO_VAL_INT;
> - break;
> + return IIO_VAL_INT;
> case IIO_CHAN_INFO_SCALE:
> switch (chan->type) {
> case IIO_VOLTAGE:
> @@ -350,60 +348,57 @@ static int iio_dummy_read_raw(struct iio_dev *indio_dev,
> /* only single ended adc -> 0.001333 */
> *val = 0;
> *val2 = 1333;
> - ret = IIO_VAL_INT_PLUS_MICRO;
> - break;
> + return IIO_VAL_INT_PLUS_MICRO;
> case 1:
> /* all differential adc -> 0.000001344 */
> *val = 0;
> *val2 = 1344;
> - ret = IIO_VAL_INT_PLUS_NANO;
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return -EINVAL;
> }
> - break;
> default:
> - break;
> + return -EINVAL;
> }
> - break;
> - case IIO_CHAN_INFO_CALIBBIAS:
> + case IIO_CHAN_INFO_CALIBBIAS: {
> + guard(mutex)(&st->lock);
> /* only the acceleration axis - read from cache */
> *val = st->accel_calibbias;
> - ret = IIO_VAL_INT;
> - break;
> - case IIO_CHAN_INFO_CALIBSCALE:
> + return IIO_VAL_INT;
> + }
> + case IIO_CHAN_INFO_CALIBSCALE: {
> + guard(mutex)(&st->lock);
> *val = st->accel_calibscale->val;
> *val2 = st->accel_calibscale->val2;
> - ret = IIO_VAL_INT_PLUS_MICRO;
> - break;
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> case IIO_CHAN_INFO_SAMP_FREQ:
> *val = 3;
> *val2 = 33;
> - ret = IIO_VAL_INT_PLUS_NANO;
> - break;
> - case IIO_CHAN_INFO_ENABLE:
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_ENABLE: {
> + guard(mutex)(&st->lock);
> switch (chan->type) {
> case IIO_STEPS:
> *val = st->steps_enabled;
> - ret = IIO_VAL_INT;
> - break;
> + return IIO_VAL_INT;
> default:
> - break;
> + return -EINVAL;
> }
> - break;
> - case IIO_CHAN_INFO_CALIBHEIGHT:
> + }
> + case IIO_CHAN_INFO_CALIBHEIGHT: {
> + guard(mutex)(&st->lock);
> switch (chan->type) {
> case IIO_STEPS:
> *val = st->height;
> - ret = IIO_VAL_INT;
> - break;
> + return IIO_VAL_INT;
> default:
> - break;
> + return -EINVAL;
> }
> - break;
> -
> + }
> default:
> - break;
> + return -EINVAL;
> }
> - mutex_unlock(&st->lock);
> - return ret;
> }
>
> /**
> @@ -436,10 +431,10 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> if (chan->output == 0)
> return -EINVAL;
>
> - /* Locking not required as writing single value */
> - mutex_lock(&st->lock);
> - st->dac_val = val;
> - mutex_unlock(&st->lock);
> + scoped_guard(mutex, &st->lock) {
> + /* Locking not required as writing single value */
> + st->dac_val = val;
> + }
> return 0;
> default:
> return -EINVAL;
> @@ -447,9 +442,9 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_PROCESSED:
> switch (chan->type) {
> case IIO_STEPS:
> - mutex_lock(&st->lock);
> - st->steps = val;
> - mutex_unlock(&st->lock);
> + scoped_guard(mutex, &st->lock) {
> + st->steps = val;
> + }
> return 0;
> case IIO_ACTIVITY:
> if (val < 0)
> @@ -470,30 +465,29 @@ static int iio_dummy_write_raw(struct iio_dev *indio_dev,
> default:
> return -EINVAL;
> }
> - case IIO_CHAN_INFO_CALIBSCALE:
> - mutex_lock(&st->lock);
> + case IIO_CHAN_INFO_CALIBSCALE: {
> + guard(mutex)(&st->lock);
> /* Compare against table - hard matching here */
> for (i = 0; i < ARRAY_SIZE(dummy_scales); i++)
> if (val == dummy_scales[i].val &&
> val2 == dummy_scales[i].val2)
> break;
> if (i == ARRAY_SIZE(dummy_scales))
> - ret = -EINVAL;
> - else
> - st->accel_calibscale = &dummy_scales[i];
> - mutex_unlock(&st->lock);
> + return -EINVAL;
> + st->accel_calibscale = &dummy_scales[i];
> return ret;
Can we change this to `return 0;` and get rid of the `ret = 0`
initialization at the beginning of the function?
> + }
> case IIO_CHAN_INFO_CALIBBIAS:
> - mutex_lock(&st->lock);
> - st->accel_calibbias = val;
> - mutex_unlock(&st->lock);
> + scoped_guard(mutex, &st->lock) {
> + st->accel_calibbias = val;
> + }
> return 0;
> case IIO_CHAN_INFO_ENABLE:
> switch (chan->type) {
> case IIO_STEPS:
> - mutex_lock(&st->lock);
> - st->steps_enabled = val;
> - mutex_unlock(&st->lock);
> + scoped_guard(mutex, &st->lock) {
> + st->steps_enabled = val;
> + }
> return 0;
> default:
> return -EINVAL;
> --
> 2.43.0
>
