On Mon, Aug 9, 2021 at 10:46 AM Puranjay Mohan [off-list ref] 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