On Sun, 9 Feb 2025 18:06:08 +0000
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
> This complex cleanup.h use case of conditional guards has proved
> to be more trouble that it is worth in terms of false positive compiler
> warnings and hard to read code.
>
> Move directly to the new claim/release_direct() that allow sparse
> to check for unbalanced context. In some cases code is factored
> out to utility functions that can do a direct return with the
> claim and release around the call.
>
> Reviewed-by: David Lechner <dlechner@xxxxxxxxxxxx>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> ---
> v2: Typo in commit description (David).
> Note there are several sets current in flight that touch this driver.
> I'll rebase as necessary depending on what order the dependencies resolve.
I've done this rebase and applied on the testing branch of iio.git.
Would appreciate a sanity check if anyone has time though!
New code is as follows. The one corner I was not sure on was
that for calibbias reading the direct mode claim was held for a long
time. That seems to be unnecessary as we have a copy of osr anyway
in that function used for other purposes.
diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 3a1a6f96480f..9dbf326b6273 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -1029,6 +1029,25 @@ static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int address)
return spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
}
+static int __ad4695_read_info_raw(struct ad4695_state *st,
+ struct iio_chan_spec const *chan,
+ int *val)
+{
+ u8 realbits = chan->scan_type.realbits;
+ int ret;
+
+ ret = ad4695_read_one_sample(st, chan->address);
+ if (ret)
+ return ret;
+
+ if (chan->scan_type.sign == 's')
+ *val = sign_extend32(st->raw_data, realbits - 1);
+ else
+ *val = st->raw_data;
+
+ return IIO_VAL_INT;
+}
+
static int ad4695_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -1049,19 +1068,12 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = ad4695_read_one_sample(st, chan->address);
- if (ret)
- return ret;
-
- if (scan_type->sign == 's')
- *val = sign_extend32(st->raw_data, realbits - 1);
- else
- *val = st->raw_data;
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
- return IIO_VAL_INT;
- }
- unreachable();
+ ret = __ad4695_read_info_raw(st, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
case IIO_VOLTAGE:
@@ -1099,63 +1111,62 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_CALIBSCALE:
switch (chan->type) {
case IIO_VOLTAGE:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = regmap_read(st->regmap16,
- AD4695_REG_GAIN_IN(chan->scan_index),
- ®_val);
- if (ret)
- return ret;
-
- *val = reg_val;
- *val2 = 15;
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = regmap_read(st->regmap16,
+ AD4695_REG_GAIN_IN(chan->scan_index),
+ ®_val);
+ iio_device_release_direct(indio_dev);
+ if (ret)
+ return ret;
+ *val = reg_val;
+ *val2 = 15;
- return IIO_VAL_FRACTIONAL_LOG2;
- }
- unreachable();
+ return IIO_VAL_FRACTIONAL_LOG2;
default:
return -EINVAL;
}
case IIO_CHAN_INFO_CALIBBIAS:
- switch (chan->type) {
- case IIO_VOLTAGE:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = regmap_read(st->regmap16,
- AD4695_REG_OFFSET_IN(chan->scan_index),
- ®_val);
- if (ret)
- return ret;
-
- tmp = sign_extend32(reg_val, 15);
-
- switch (cfg->oversampling_ratio) {
- case 1:
- *val = tmp / 4;
- *val2 = abs(tmp) % 4 * MICRO / 4;
- break;
- case 4:
- *val = tmp / 2;
- *val2 = abs(tmp) % 2 * MICRO / 2;
- break;
- case 16:
- *val = tmp;
- *val2 = 0;
- break;
- case 64:
- *val = tmp * 2;
- *val2 = 0;
- break;
- default:
- return -EINVAL;
- }
-
- if (tmp < 0 && *val2) {
- *val *= -1;
- *val2 *= -1;
- }
-
- return IIO_VAL_INT_PLUS_MICRO;
+ switch (chan->type)
+ case IIO_VOLTAGE: {
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = regmap_read(st->regmap16,
+ AD4695_REG_OFFSET_IN(chan->scan_index),
+ ®_val);
+ iio_device_release_direct(indio_dev);
+ if (ret)
+ return ret;
////THIS IS THE BIT I WOuLD LIKE EYES on.
+
+ tmp = sign_extend32(reg_val, 15);
+
+ switch (osr) {
+ case 1:
+ *val = tmp / 4;
+ *val2 = abs(tmp) % 4 * MICRO / 4;
+ break;
+ case 4:
+ *val = tmp / 2;
+ *val2 = abs(tmp) % 2 * MICRO / 2;
+ break;
+ case 16:
+ *val = tmp;
+ *val2 = 0;
+ break;
+ case 64:
+ *val = tmp * 2;
+ *val2 = 0;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (tmp < 0 && *val2) {
+ *val *= -1;
+ *val2 *= -1;
}
- unreachable();
+
+ return IIO_VAL_INT_PLUS_MICRO;
default:
return -EINVAL;
}
@@ -1255,72 +1266,83 @@ static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
return clamp_t(int, val_calc, S16_MIN, S16_MAX);
}
-static int ad4695_write_raw(struct iio_dev *indio_dev,
- struct iio_chan_spec const *chan,
- int val, int val2, long mask)
+static int __ad4695_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
{
struct ad4695_state *st = iio_priv(indio_dev);
unsigned int reg_val;
unsigned int osr = st->channels_cfg[chan->scan_index].oversampling_ratio;
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- switch (mask) {
- case IIO_CHAN_INFO_CALIBSCALE:
- switch (chan->type) {
- case IIO_VOLTAGE:
- if (val < 0 || val2 < 0)
- reg_val = 0;
- else if (val > 1)
- reg_val = U16_MAX;
- else
- reg_val = (val * (1 << 16) +
- mul_u64_u32_div(val2, 1 << 16,
- MICRO)) / 2;
-
- return regmap_write(st->regmap16,
- AD4695_REG_GAIN_IN(chan->scan_index),
- reg_val);
- default:
- return -EINVAL;
- }
- case IIO_CHAN_INFO_CALIBBIAS:
- switch (chan->type) {
- case IIO_VOLTAGE:
- reg_val = ad4695_get_calibbias(val, val2, osr);
- return regmap_write(st->regmap16,
- AD4695_REG_OFFSET_IN(chan->scan_index),
- reg_val);
- default:
- return -EINVAL;
- }
- case IIO_CHAN_INFO_SAMP_FREQ: {
- struct pwm_state state;
- /*
- * Limit the maximum acceptable sample rate according to
- * the channel's oversampling ratio.
- */
- u64 max_osr_rate = DIV_ROUND_UP_ULL(st->chip_info->max_sample_rate,
- osr);
-
- if (val <= 0 || val > max_osr_rate)
- return -EINVAL;
+ switch (mask) {
+ case IIO_CHAN_INFO_CALIBSCALE:
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ if (val < 0 || val2 < 0)
+ reg_val = 0;
+ else if (val > 1)
+ reg_val = U16_MAX;
+ else
+ reg_val = (val * (1 << 16) +
+ mul_u64_u32_div(val2, 1 << 16,
+ MICRO)) / 2;
- guard(mutex)(&st->cnv_pwm_lock);
- pwm_get_state(st->cnv_pwm, &state);
- /*
- * The required sample frequency for a given OSR is the
- * input frequency multiplied by it.
- */
- state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, val * osr);
- return pwm_apply_might_sleep(st->cnv_pwm, &state);
+ return regmap_write(st->regmap16,
+ AD4695_REG_GAIN_IN(chan->scan_index),
+ reg_val);
+ default:
+ return -EINVAL;
}
- case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
- return ad4695_set_osr_val(st, chan, val);
+ case IIO_CHAN_INFO_CALIBBIAS:
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ reg_val = ad4695_get_calibbias(val, val2, osr);
+ return regmap_write(st->regmap16,
+ AD4695_REG_OFFSET_IN(chan->scan_index),
+ reg_val);
default:
return -EINVAL;
}
+ case IIO_CHAN_INFO_SAMP_FREQ: {
+ struct pwm_state state;
+ /*
+ * Limit the maximum acceptable sample rate according to
+ * the channel's oversampling ratio.
+ */
+ u64 max_osr_rate = DIV_ROUND_UP_ULL(st->chip_info->max_sample_rate,
+ osr);
+
+ if (val <= 0 || val > max_osr_rate)
+ return -EINVAL;
+
+ guard(mutex)(&st->cnv_pwm_lock);
+ pwm_get_state(st->cnv_pwm, &state);
+ /*
+ * The required sample frequency for a given OSR is the
+ * input frequency multiplied by it.
+ */
+ state.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, val * osr);
+ return pwm_apply_might_sleep(st->cnv_pwm, &state);
+ }
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ return ad4695_set_osr_val(st, chan, val);
+ default:
+ return -EINVAL;
}
- unreachable();
+}
+
+static int ad4695_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ int ret;
+
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = __ad4695_write_raw(indio_dev, chan, val, val2, mask);
+ iio_device_release_direct(indio_dev);
+
+ return ret;
}
static int ad4695_read_avail(struct iio_dev *indio_dev,
@@ -1417,26 +1439,29 @@ static int ad4695_debugfs_reg_access(struct iio_dev *indio_dev,
unsigned int *readval)
{
struct ad4695_state *st = iio_priv(indio_dev);
-
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- if (readval) {
- if (regmap_check_range_table(st->regmap, reg,
- &ad4695_regmap_rd_table))
- return regmap_read(st->regmap, reg, readval);
- if (regmap_check_range_table(st->regmap16, reg,
- &ad4695_regmap16_rd_table))
- return regmap_read(st->regmap16, reg, readval);
- } else {
- if (regmap_check_range_table(st->regmap, reg,
- &ad4695_regmap_wr_table))
- return regmap_write(st->regmap, reg, writeval);
- if (regmap_check_range_table(st->regmap16, reg,
- &ad4695_regmap16_wr_table))
- return regmap_write(st->regmap16, reg, writeval);
- }
+ int ret = -EINVAL;
+
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ if (readval) {
+ if (regmap_check_range_table(st->regmap, reg,
+ &ad4695_regmap_rd_table))
+ ret = regmap_read(st->regmap, reg, readval);
+ if (regmap_check_range_table(st->regmap16, reg,
+ &ad4695_regmap16_rd_table))
+ ret = regmap_read(st->regmap16, reg, readval);
+ } else {
+ if (regmap_check_range_table(st->regmap, reg,
+ &ad4695_regmap_wr_table))
+ ret = regmap_write(st->regmap, reg, writeval);
+ if (regmap_check_range_table(st->regmap16, reg,
+ &ad4695_regmap16_wr_table))
+ ret = regmap_write(st->regmap16, reg, writeval);
}
+ iio_device_release_direct(indio_dev);
- return -EINVAL;
+ return ret;
}
static int ad4695_get_current_scan_type(const struct iio_dev *indio_dev,
Jonathan
> ---
> drivers/iio/adc/ad4695.c | 240 ++++++++++++++++++++++-----------------
> 1 file changed, 133 insertions(+), 107 deletions(-)
>
> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> index 13cf01d35301..4bb22f4d739b 100644
> --- a/drivers/iio/adc/ad4695.c
> +++ b/drivers/iio/adc/ad4695.c
> @@ -738,6 +738,25 @@ static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int address)
> return spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> }
>
> +static int __ad4695_read_info_raw(struct ad4695_state *st,
> + struct iio_chan_spec const *chan,
> + int *val)
> +{
> + u8 realbits = chan->scan_type.realbits;
> + int ret;
> +
> + ret = ad4695_read_one_sample(st, chan->address);
> + if (ret)
> + return ret;
> +
> + if (chan->scan_type.sign == 's')
> + *val = sign_extend32(st->raw_data, realbits - 1);
> + else
> + *val = st->raw_data;
> +
> + return IIO_VAL_INT;
> +}
> +
> static int ad4695_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val, int *val2, long mask)
> @@ -750,19 +769,12 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> - iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> - ret = ad4695_read_one_sample(st, chan->address);
> - if (ret)
> - return ret;
> -
> - if (chan->scan_type.sign == 's')
> - *val = sign_extend32(st->raw_data, realbits - 1);
> - else
> - *val = st->raw_data;
>
> - return IIO_VAL_INT;
> - }
> - unreachable();
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> + ret = __ad4695_read_info_raw(st, chan, val);
> + iio_device_release_direct(indio_dev);
> + return ret;
> case IIO_CHAN_INFO_SCALE:
> switch (chan->type) {
> case IIO_VOLTAGE:
> @@ -800,45 +812,45 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
> case IIO_CHAN_INFO_CALIBSCALE:
> switch (chan->type) {
> case IIO_VOLTAGE:
> - iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> - ret = regmap_read(st->regmap16,
> - AD4695_REG_GAIN_IN(chan->scan_index),
> - ®_val);
> - if (ret)
> - return ret;
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> + ret = regmap_read(st->regmap16,
> + AD4695_REG_GAIN_IN(chan->scan_index),
> + ®_val);
> + iio_device_release_direct(indio_dev);
> + if (ret)
> + return ret;
>
> - *val = reg_val;
> - *val2 = 15;
> + *val = reg_val;
> + *val2 = 15;
>
> - return IIO_VAL_FRACTIONAL_LOG2;
> - }
> - unreachable();
> + return IIO_VAL_FRACTIONAL_LOG2;
> default:
> return -EINVAL;
> }
> case IIO_CHAN_INFO_CALIBBIAS:
> switch (chan->type) {
> case IIO_VOLTAGE:
> - iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> - ret = regmap_read(st->regmap16,
> - AD4695_REG_OFFSET_IN(chan->scan_index),
> - ®_val);
> - if (ret)
> - return ret;
> -
> - tmp = sign_extend32(reg_val, 15);
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> + ret = regmap_read(st->regmap16,
> + AD4695_REG_OFFSET_IN(chan->scan_index),
> + ®_val);
> + iio_device_release_direct(indio_dev);
> + if (ret)
> + return ret;
>
> - *val = tmp / 4;
> - *val2 = abs(tmp) % 4 * MICRO / 4;
> + tmp = sign_extend32(reg_val, 15);
>
> - if (tmp < 0 && *val2) {
> - *val *= -1;
> - *val2 *= -1;
> - }
> + *val = tmp / 4;
> + *val2 = abs(tmp) % 4 * MICRO / 4;
>
> - return IIO_VAL_INT_PLUS_MICRO;
> + if (tmp < 0 && *val2) {
> + *val *= -1;
> + *val2 *= -1;
> }
> - unreachable();
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> default:
> return -EINVAL;
> }
> @@ -847,64 +859,75 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
> }
> }
>
> -static int ad4695_write_raw(struct iio_dev *indio_dev,
> - struct iio_chan_spec const *chan,
> - int val, int val2, long mask)
> +static int __ad4695_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> {
> struct ad4695_state *st = iio_priv(indio_dev);
> unsigned int reg_val;
>
> - iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> - switch (mask) {
> - case IIO_CHAN_INFO_CALIBSCALE:
> - switch (chan->type) {
> - case IIO_VOLTAGE:
> - if (val < 0 || val2 < 0)
> - reg_val = 0;
> - else if (val > 1)
> - reg_val = U16_MAX;
> - else
> - reg_val = (val * (1 << 16) +
> - mul_u64_u32_div(val2, 1 << 16,
> - MICRO)) / 2;
> -
> - return regmap_write(st->regmap16,
> - AD4695_REG_GAIN_IN(chan->scan_index),
> - reg_val);
> - default:
> - return -EINVAL;
> - }
> - case IIO_CHAN_INFO_CALIBBIAS:
> - switch (chan->type) {
> - case IIO_VOLTAGE:
> - if (val2 >= 0 && val > S16_MAX / 4)
> - reg_val = S16_MAX;
> - else if ((val2 < 0 ? -val : val) < S16_MIN / 4)
> - reg_val = S16_MIN;
> - else if (val2 < 0)
> - reg_val = clamp_t(int,
> - -(val * 4 + -val2 * 4 / MICRO),
> - S16_MIN, S16_MAX);
> - else if (val < 0)
> - reg_val = clamp_t(int,
> - val * 4 - val2 * 4 / MICRO,
> - S16_MIN, S16_MAX);
> - else
> - reg_val = clamp_t(int,
> - val * 4 + val2 * 4 / MICRO,
> - S16_MIN, S16_MAX);
> -
> - return regmap_write(st->regmap16,
> - AD4695_REG_OFFSET_IN(chan->scan_index),
> - reg_val);
> - default:
> - return -EINVAL;
> - }
> + switch (mask) {
> + case IIO_CHAN_INFO_CALIBSCALE:
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + if (val < 0 || val2 < 0)
> + reg_val = 0;
> + else if (val > 1)
> + reg_val = U16_MAX;
> + else
> + reg_val = (val * (1 << 16) +
> + mul_u64_u32_div(val2, 1 << 16,
> + MICRO)) / 2;
> +
> + return regmap_write(st->regmap16,
> + AD4695_REG_GAIN_IN(chan->scan_index),
> + reg_val);
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_CALIBBIAS:
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + if (val2 >= 0 && val > S16_MAX / 4)
> + reg_val = S16_MAX;
> + else if ((val2 < 0 ? -val : val) < S16_MIN / 4)
> + reg_val = S16_MIN;
> + else if (val2 < 0)
> + reg_val = clamp_t(int,
> + -(val * 4 + -val2 * 4 / MICRO),
> + S16_MIN, S16_MAX);
> + else if (val < 0)
> + reg_val = clamp_t(int,
> + val * 4 - val2 * 4 / MICRO,
> + S16_MIN, S16_MAX);
> + else
> + reg_val = clamp_t(int,
> + val * 4 + val2 * 4 / MICRO,
> + S16_MIN, S16_MAX);
> +
> + return regmap_write(st->regmap16,
> + AD4695_REG_OFFSET_IN(chan->scan_index),
> + reg_val);
> default:
> return -EINVAL;
> }
> + default:
> + return -EINVAL;
> }
> - unreachable();
> +}
> +
> +static int ad4695_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + int ret;
> +
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> + ret = __ad4695_write_raw(indio_dev, chan, val, val2, mask);
> + iio_device_release_direct(indio_dev);
> +
> + return ret;
> }
>
> static int ad4695_read_avail(struct iio_dev *indio_dev,
> @@ -954,26 +977,29 @@ static int ad4695_debugfs_reg_access(struct iio_dev *indio_dev,
> unsigned int *readval)
> {
> struct ad4695_state *st = iio_priv(indio_dev);
> -
> - iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> - if (readval) {
> - if (regmap_check_range_table(st->regmap, reg,
> - &ad4695_regmap_rd_table))
> - return regmap_read(st->regmap, reg, readval);
> - if (regmap_check_range_table(st->regmap16, reg,
> - &ad4695_regmap16_rd_table))
> - return regmap_read(st->regmap16, reg, readval);
> - } else {
> - if (regmap_check_range_table(st->regmap, reg,
> - &ad4695_regmap_wr_table))
> - return regmap_write(st->regmap, reg, writeval);
> - if (regmap_check_range_table(st->regmap16, reg,
> - &ad4695_regmap16_wr_table))
> - return regmap_write(st->regmap16, reg, writeval);
> - }
> + int ret = -EINVAL;
> +
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> +
> + if (readval) {
> + if (regmap_check_range_table(st->regmap, reg,
> + &ad4695_regmap_rd_table))
> + ret = regmap_read(st->regmap, reg, readval);
> + if (regmap_check_range_table(st->regmap16, reg,
> + &ad4695_regmap16_rd_table))
> + ret = regmap_read(st->regmap16, reg, readval);
> + } else {
> + if (regmap_check_range_table(st->regmap, reg,
> + &ad4695_regmap_wr_table))
> + ret = regmap_write(st->regmap, reg, writeval);
> + if (regmap_check_range_table(st->regmap16, reg,
> + &ad4695_regmap16_wr_table))
> + ret = regmap_write(st->regmap16, reg, writeval);
> }
> + iio_device_release_direct(indio_dev);
>
> - return -EINVAL;
> + return ret;
> }
>
> static const struct iio_info ad4695_info = {
