Re: [PATCH v9 1/2] iio: accel: Add driver support for ADXL355
From: Andy Shevchenko
Date: Tue Aug 10 2021 - 08:36:59 EST
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