Re: [PATCH v10 2/2] iio: accel: Add driver support for ADXL355
From: Andy Shevchenko <hidden>
Date: 2021-08-10 19:10:54
Also in:
linux-iio, lkml
On Tue, Aug 10, 2021 at 9:45 PM Puranjay Mohan [off-list ref] wrote:
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.
Thanks for an update. Seems some of the comments left unaddressed. ...
+#include <linux/device.h>
No need. There is no user for that. A forward declaration is enough.
+#include <linux/regmap.h>
...
quoted hunk ↗ jump to hunk
+ u32 multiplier; + u64 div; + u64 odr; + u64 rem;
div and rem seems of the same nature, you may put them on one line.
quoted hunk ↗ jump to hunk
+ int i; + + odr = mul_u64_u32_shr(adxl355_odr_table[data->odr][0], 1000000, 0) + + adxl355_odr_table[data->odr][1]; + + for (i = 0; i < ARRAY_SIZE(adxl355_hpf_3db_multipliers); i++) { + multiplier = adxl355_hpf_3db_multipliers[i]; + div = div64_u64_rem(mul_u64_u32_shr(odr, multiplier, 0), + 100000000000000UL, &rem); + + data->adxl355_hpf_3db_table[i][0] = div; + data->adxl355_hpf_3db_table[i][1] = div_u64(rem, 100000000); + }
Here, I didn't get if those powers-of-two are some kind of units? If no, we may have locally defined SI prefexes for them.
+}
...
quoted hunk ↗ jump to hunk
+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;
This...
quoted hunk ↗ jump to hunk
+ ret = adxl355_set_op_mode(data, ADXL355_STANDBY); + if (ret < 0) + goto out_unlock;
...and this are kinda easy to confuse. Please, give them better names.
quoted hunk ↗ jump to hunk
+ 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; + +out_unlock: + ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT); +unlock: + mutex_unlock(&data->lock); + return ret; +} + +static int adxl355_set_calibbias(struct adxl355_data *data, + enum adxl355_chans chan, int calibbias) +{
+ int ret = 0;
Redundant assignment. ...
+out_unlock:
And as per above function, this one does more than simple unlock.
quoted hunk ↗ jump to hunk
+ ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT); + mutex_unlock(&data->lock); + return ret; +}
...
quoted hunk ↗ jump to hunk
+static const struct iio_info adxl355_info = { + .read_raw = adxl355_read_raw, + .write_raw = adxl355_write_raw, + .read_avail = &adxl355_read_avail
+ comma.
+};
-- With Best Regards, Andy Shevchenko