Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 10 Aug 2021 22:10:14 +0300 | Subject | Re: [PATCH v10 2/2] iio: accel: Add driver support for ADXL355 |
| |
On Tue, Aug 10, 2021 at 9:45 PM Puranjay Mohan <puranjay12@gmail.com> 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>
...
> + u32 multiplier; > + u64 div; > + u64 odr; > + u64 rem;
div and rem seems of the same nature, you may put them on one line.
> + 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.
> +}
...
> +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...
> + 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.
> + 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.
> + ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT); > + mutex_unlock(&data->lock); > + return ret; > +}
...
> +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
| |