Re: [PATCH v10 2/2] iio: accel: Add driver support for ADXL355

[Andy Shevchenko]

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