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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 9, 2021 at 10:46 AM Puranjay Mohan <puranjay12@xxxxxxxxx> wrote:
>
> ADXL355 is 3-axis MEMS Accelerometer. It offers low noise density,

is a 3-axis

> low 0g offset drift, low power with selectable measurement ranges.
> It also features programmable high-pass and low-pass filters.

...

> +F:     drivers/iio/accel/adxl355.h
> +F:     drivers/iio/accel/adxl355_core.c
> +F:     drivers/iio/accel/adxl355_i2c.c
> +F:     drivers/iio/accel/adxl355_spi.c
> +F:     Documentation/devicetree/bindings/iio/accel/adi,adxl355.yaml

Have you run checkpatch?

...

> +#ifndef _ADXL355_H_
> +#define _ADXL355_H_
> +
> +#include <linux/regmap.h>

Missed declaration for struct device.

> +extern const struct regmap_access_table adxl355_readable_regs_tbl;
> +

I think you may drop this blank line.

> +extern const struct regmap_access_table adxl355_writeable_regs_tbl;
> +
> +int adxl355_core_probe(struct device *dev, struct regmap *regmap,
> +                      const char *name);

+ blank line?

> +#endif /* _ADXL355_H_ */

...

> +#include <asm/unaligned.h>

asm/* is less generic, can you move it after linux/*?

> +#include <linux/bitfield.h>

Does this imply bits.h? If not, add the latter.

> +#include <linux/iio/iio.h>
> +#include <linux/limits.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regmap.h>

...

> +static const struct regmap_range adxl355_read_reg_range[] = {
> +       regmap_reg_range(ADXL355_DEVID_AD_REG, ADXL355_FIFO_DATA_REG),
> +       regmap_reg_range(ADXL355_OFFSET_X_H_REG, ADXL355_SELF_TEST_REG)

Leave commas in non-terminator lines.

> +};

...

> +static const struct regmap_range adxl355_write_reg_range[] = {
> +       regmap_reg_range(ADXL355_OFFSET_X_H_REG, ADXL355_RESET_REG)

Ditto.

> +};

...

> +enum adxl355_op_mode {

> +       ADXL355_TEMP_OFF

Ditto.

> +};
> +
> +enum adxl355_odr {

> +       ADXL355_ODR_3_906HZ

Ditto.

> +};
> +
> +enum adxl355_hpf_3db {

> +       ADXL355_HPF_0_0238

Ditto.

> +};
> +
> +static const int adxl355_odr_table[][2] = {

> +       [10] = {3, 906000}

Ditto.

> +};
> +
> +static const int adxl355_hpf_3db_multipliers[] = {

> +       238

Ditto.

> +};
> +
> +enum adxl355_chans {
> +       chan_x, chan_y, chan_z

Ditto.

> +};

> +static const struct adxl355_chan_info adxl355_chans[] = {

> +       [chan_z] = {
> +               .data_reg = ADXL355_ZDATA3_REG,
> +               .offset_reg = ADXL355_OFFSET_Z_H_REG
> +       }

Ditto.

> +};

To avoid adding extra entries, consider using static_assert():s.

...

> +static void adxl355_fill_3db_frequency_table(struct adxl355_data *data)
> +{
> +       int i;
> +       u64 rem;
> +       u64 div;
> +       u32 multiplier;

Reversed xmas tree order?

> +       u64 odr = mul_u64_u32_shr(adxl355_odr_table[data->odr][0], 1000000, 0) +
> +                                 adxl355_odr_table[data->odr][1];

Split definition and assignment.

> +       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);
> +       }

Do all those power-of-ten constants have a meaning? Shouldn't it be
better to use named definitions?

> +}

...

> +       /*
> +        * Perform a software reset to make sure the device is in a consistent
> +        * state after start up.

start-up

> +        */

...

> +       ret = regmap_bulk_read(data->regmap, addr, data->transf_buf, 3);

ARRAY_SIZE() ?

> +       if (ret < 0)
> +               return ret;

...

> +               /*
> +                * The datasheet defines an intercept of 1885 LSB at 25 degC
> +                * and a slope of -9.05 LSB/C. The following formula can be used
> +                * to find the temperature:
> +                * Temp = ((RAW - 1885)/(-9.05)) + 25 but this doesn't follow
> +                * the format of the IIO which is Temp = (RAW + OFFSET) * SCALE.
> +                * Hence using some rearranging we get the scale as -110.497238
> +                * and offset as -2111.25

Missed period.

> +                */

...

> +               /*
> +                * At +/- 2g with 20-bit resolution, scale is given in datasheet
> +                * as 3.9ug/LSB = 0.0000039 * 9.80665 = 0.00003824593 m/s^2

Ditto for all multi-line comments.

> +                */

...

> +static const struct regmap_config adxl355_i2c_regmap_config = {
> +       .reg_bits = 8,
> +       .val_bits = 8,
> +       .max_register = 0x2F,
> +       .rd_table = &adxl355_readable_regs_tbl,
> +       .wr_table = &adxl355_writeable_regs_tbl

+ comma

> +};

...

> +       regmap = devm_regmap_init_i2c(client, &adxl355_i2c_regmap_config);
> +       if (IS_ERR(regmap)) {
> +               dev_err(&client->dev, "Error initializing i2c regmap: %ld\n",
> +                       PTR_ERR(regmap));

One line?

> +               return PTR_ERR(regmap);
> +       }

...

> +static const struct i2c_device_id adxl355_i2c_id[] = {
> +       { "adxl355", 0 },
> +       { }
> +};

> +

Redundant blank line.

> +MODULE_DEVICE_TABLE(i2c, adxl355_i2c_id);
> +
> +static const struct of_device_id adxl355_of_match[] = {
> +       { .compatible = "adi,adxl355" },
> +       { }
> +};

> +

Ditto.

> +MODULE_DEVICE_TABLE(of, adxl355_of_match);
> +
> +static struct i2c_driver adxl355_i2c_driver = {
> +       .driver = {
> +               .name   = "adxl355_i2c",
> +               .of_match_table = adxl355_of_match,
> +       },
> +       .probe_new      = adxl355_i2c_probe,
> +       .id_table       = adxl355_i2c_id,
> +};

> +

Ditto.

> +module_i2c_driver(adxl355_i2c_driver);

For SPI part the very same bunch of comments.

-- 
With Best Regards,
Andy Shevchenko



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux