On Tue, Aug 10, 2021 at 9:45 PM Puranjay Mohan <puranjay12@xxxxxxxxx> 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
