On Sun, Aug 15, 2021 at 9:14 PM Jonathan Cameron [off-list ref] wrote:

On Wed, 11 Aug 2021 13:00:27 +0530
Puranjay Mohan [off-list ref] wrote:

quoted

ADXL355 is a 3-axis MEMS Accelerometer. It offers low noise density,
low 0g offset drift, low power with selectable measurement ranges.
It also features programmable high-pass and low-pass filters.

Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adxl354_adxl355.pdf
Reviewed-by: Alexandru Ardelean <redacted>
Reviewed-by: Andy Shevchenko <redacted>
Signed-off-by: Puranjay Mohan <redacted>

Hi Puranjay,

I took one last look at this so I can apply it without looking again assuming
the dt review is fine.  Noticed one issue with error handling, but I can tidy
that up whilst applying assuming you aren't doing a v13 for some other reason.
If you are please incorporate these changes as well.

Hi Jonathan, It would be great if you could make these changes while
applying. I am not doing a v13 as all comments have been covered
earlier.
I shall be thankful to you.

Thanks,

Jonathan


...

quoted

+
+static int adxl355_set_odr(struct adxl355_data *data,
+                        enum adxl355_odr odr)
+{
+     int ret = 0;
+
+     mutex_lock(&data->lock);
+
+     if (data->odr == odr)
+             goto out_unlock;
+
+     ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
+     if (ret < 0)
+             goto out_unlock;
+
+     ret = regmap_update_bits(data->regmap, ADXL355_FILTER_REG,
+                              ADXL355_FILTER_ODR_MSK,
+                              FIELD_PREP(ADXL355_FILTER_ODR_MSK, odr));
+     if (ret < 0)
+             goto out_unlock;
+
+     data->odr = odr;
+     adxl355_fill_3db_frequency_table(data);
+
+out_unlock:
+     ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);

As below, we should do this because it risks returning success when a failure
actually occured.  Again, unless you are respinning for some other reason I'll
add the logic whilst applying.

quoted

+     mutex_unlock(&data->lock);
+     return ret;
+}
+
+static int adxl355_set_hpf_3db(struct adxl355_data *data,
+                            enum adxl355_hpf_3db hpf)
+{
+     int ret = 0;
+
+     mutex_lock(&data->lock);
+
+     if (data->hpf_3db == hpf)
+             goto unlock;
+
+     ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
+     if (ret < 0)
+             goto set_opmode_unlock;
+
+     ret = regmap_update_bits(data->regmap, ADXL355_FILTER_REG,
+                              ADXL355_FILTER_HPF_MSK,
+                              FIELD_PREP(ADXL355_FILTER_HPF_MSK, hpf));
+     if (!ret)
+             data->hpf_3db = hpf;
+
+set_opmode_unlock:
+     ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);

We can't do this as it might potentially eat an error that meant the regmap
update didn't occur.  To avoid that a little dance is needed using a second
return value and we only set ret = ret2 if ret == 0

Alternatively we just have a separate error handling path which doesn't set
ret for the adxl355_set_op_mode(). I'll probably go with that as it's more
code but easier to read.

quoted

+unlock:
+     mutex_unlock(&data->lock);
+     return ret;
+}
+

...

quoted

+static int adxl355_write_raw(struct iio_dev *indio_dev,
+                          struct iio_chan_spec const *chan,
+                          int val, int val2, long mask)
+{
+     struct adxl355_data *data = iio_priv(indio_dev);
+     int odr_idx, hpf_idx, calibbias;
+
+     switch (mask) {
+     case IIO_CHAN_INFO_SAMP_FREQ:
+             odr_idx = adxl355_find_match(adxl355_odr_table,
+                                          ARRAY_SIZE(adxl355_odr_table),
+                                          val, val2);
+             if (odr_idx < 0)
+                     return odr_idx;
+
+             return adxl355_set_odr(data, odr_idx);
+     case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+             hpf_idx = adxl355_find_match(data->adxl355_hpf_3db_table,
+                                     ARRAY_SIZE(data->adxl355_hpf_3db_table),

Mixing different indentation styles isn't very nice for readability.
I'll tweak this whilst applying.

quoted

+                                          val, val2);
+             if (hpf_idx < 0)
+                     return hpf_idx;
+
+             return adxl355_set_hpf_3db(data, hpf_idx);
+     case IIO_CHAN_INFO_CALIBBIAS:
+             calibbias = clamp_t(int, val, S16_MIN, S16_MAX);
+
+             return adxl355_set_calibbias(data, chan->address, calibbias);
+     default:
+             return -EINVAL;
+     }
+}

...

-- 
Thanks and Regards

Yours Truly,

Puranjay Mohan