Core parts of the new ICM20948 driver.
Add register definitions, probing, setup, and an IIO device for
reading the onboard temperature sensor.
Signed-off-by: Bharadwaj Raju <bharadwaj.raju777@gmail.com>
---
drivers/iio/imu/Kconfig | 1 +
drivers/iio/imu/Makefile | 1 +
drivers/iio/imu/inv_icm20948/Kconfig | 17 ++++
drivers/iio/imu/inv_icm20948/Makefile | 8 ++
drivers/iio/imu/inv_icm20948/inv_icm20948.h | 47 +++++++++
drivers/iio/imu/inv_icm20948/inv_icm20948_core.c | 122 +++++++++++++++++++++++
drivers/iio/imu/inv_icm20948/inv_icm20948_i2c.c | 48 +++++++++
drivers/iio/imu/inv_icm20948/inv_icm20948_temp.c | 108 ++++++++++++++++++++
8 files changed, 352 insertions(+)
diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
index 15612f0f189b5114deb414ef840339678abdc562..d59e5b0087398cfbd2719ca914fd147ab067155f 100644
--- a/drivers/iio/imu/Kconfig
+++ b/drivers/iio/imu/Kconfig
@@ -109,6 +109,7 @@ config KMX61
be called kmx61.
source "drivers/iio/imu/inv_icm42600/Kconfig"
+source "drivers/iio/imu/inv_icm20948/Kconfig"
source "drivers/iio/imu/inv_mpu6050/Kconfig"
config SMI240
diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
index e901aea498d37e5897e8b71268356a19eac2cb59..79e49bae59038c1ca1d54a64cf49b6ca5f57cb0b 100644
--- a/drivers/iio/imu/Makefile
+++ b/drivers/iio/imu/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_FXOS8700_I2C) += fxos8700_i2c.o
obj-$(CONFIG_FXOS8700_SPI) += fxos8700_spi.o
obj-y += inv_icm42600/
+obj-y += inv_icm20948/
obj-y += inv_mpu6050/
obj-$(CONFIG_KMX61) += kmx61.o
diff --git a/drivers/iio/imu/inv_icm20948/Kconfig b/drivers/iio/imu/inv_icm20948/Kconfig
new file mode 100644
index 0000000000000000000000000000000000000000..79ffbe273e71f27ec33fffa0286eafd7dd11aa29
--- /dev/null
+++ b/drivers/iio/imu/inv_icm20948/Kconfig
@@ -0,0 +1,17 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+config INV_ICM20948
+ tristate
+
+config INV_ICM20948_I2C
+ tristate "InvenSense ICM-20948 I2C driver"
+ depends on I2C
+ select INV_ICM20948
+ select REGMAP_I2C
+ help
+ This driver supports the InvenSense ICM-20948 motion tracking
+ device over I2C.
+
+ This driver can be built as a module. The module will be called
+ inv-icm20948-i2c.
+
diff --git a/drivers/iio/imu/inv_icm20948/Makefile b/drivers/iio/imu/inv_icm20948/Makefile
new file mode 100644
index 0000000000000000000000000000000000000000..c508c2dc3eee2c32be20067e3e0868a203d8aa1a
--- /dev/null
+++ b/drivers/iio/imu/inv_icm20948/Makefile
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+obj-$(CONFIG_INV_ICM20948) += inv-icm20948.o
+inv-icm20948-y += inv_icm20948_core.o
+inv-icm20948-y += inv_icm20948_temp.o
+
+obj-$(CONFIG_INV_ICM20948_I2C) += inv-icm20948-i2c.o
+inv-icm20948-i2c-y += inv_icm20948_i2c.o
diff --git a/drivers/iio/imu/inv_icm20948/inv_icm20948.h b/drivers/iio/imu/inv_icm20948/inv_icm20948.h
new file mode 100644
index 0000000000000000000000000000000000000000..f9830645fbe96fd02eef7c54d1e5908647d5a0fe
--- /dev/null
+++ b/drivers/iio/imu/inv_icm20948/inv_icm20948.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2025 Bharadwaj Raju <bharadwaj.raju777@gmail.com>
+ */
+
+#ifndef INV_ICM20948_H_
+#define INV_ICM20948_H_
+
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/err.h>
+
+/* accel takes 20ms, gyro takes 35ms to wake from full-chip sleep */
+#define INV_ICM20948_SLEEP_WAKEUP_MS 35
+
+#define INV_ICM20948_REG_BANK_SEL 0x7F
+#define INV_ICM20948_BANK_SEL_MASK GENMASK(5, 4)
+
+#define INV_ICM20948_REG_WHOAMI 0x0000
+#define INV_ICM20948_WHOAMI 0xEA
+
+#define INV_ICM20948_REG_FIFO_RW 0x0072
+
+#define INV_ICM20948_REG_PWR_MGMT_1 0x0006
+#define INV_ICM20948_PWR_MGMT_1_DEV_RESET BIT(7)
+#define INV_ICM20948_PWR_MGMT_1_SLEEP BIT(6)
+
+#define INV_ICM20948_REG_TEMP_DATA 0x0039
+
+extern const struct regmap_config inv_icm20948_regmap_config;
+
+struct inv_icm20948_state {
+ struct device *dev;
+ struct regmap *regmap;
+ struct iio_dev *temp_dev;
+ struct mutex lock;
+};
+
+extern int inv_icm20948_core_probe(struct regmap *regmap);
+
+struct iio_dev *inv_icm20948_temp_init(struct inv_icm20948_state *state);
+
+#endif
diff --git a/drivers/iio/imu/inv_icm20948/inv_icm20948_core.c b/drivers/iio/imu/inv_icm20948/inv_icm20948_core.c
new file mode 100644
index 0000000000000000000000000000000000000000..ee9e4159cffa261f0326b146a4b3df2cbfbd7697
--- /dev/null
+++ b/drivers/iio/imu/inv_icm20948/inv_icm20948_core.c
@@ -0,0 +1,122 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2025 Bharadwaj Raju <bharadwaj.raju777@gmail.com>
+ */
+
+#include "inv_icm20948.h"
+
+static const struct regmap_range_cfg inv_icm20948_regmap_ranges[] = {
+ {
+ .name = "user banks",
+ .range_min = 0x0000,
+ .range_max = 0x3FFF,
+ .selector_reg = INV_ICM20948_REG_BANK_SEL,
+ .selector_mask = INV_ICM20948_BANK_SEL_MASK,
+ .window_start = 0,
+ .window_len = 0x1000,
+ },
+};
+
+static const struct regmap_range inv_icm20948_regmap_volatile_yes_ranges[] = {
+ /* WHOAMI */
+ regmap_reg_range(0x0000, 0x0000),
+ /* PWR_MGMT_1 */
+ regmap_reg_range(0x0006, 0x0006),
+ /* I2C and INT status */
+ regmap_reg_range(0x0017, 0x001C),
+ /* Sensor readouts */
+ regmap_reg_range(0x0028, 0x0052),
+ /* FIFO count and data */
+ regmap_reg_range(0x0070, 0x0072),
+ /* Data ready status */
+ regmap_reg_range(0x0074, 0x0074),
+ /* GYRO_CONFIG_1 */
+ regmap_reg_range(0x2001, 0x2001),
+ /* I2C SLV4 data in */
+ regmap_reg_range(0x307F, 0x307F),
+};
+
+static const struct regmap_access_table inv_icm20948_regmap_volatile_accesses = {
+ .yes_ranges = inv_icm20948_regmap_volatile_yes_ranges,
+ .n_yes_ranges = ARRAY_SIZE(inv_icm20948_regmap_volatile_yes_ranges),
+};
+
+static const struct regmap_range inv_icm20948_rd_noinc_no_ranges[] = {
+ regmap_reg_range(0x0000, INV_ICM20948_REG_FIFO_RW - 1),
+ regmap_reg_range(INV_ICM20948_REG_FIFO_RW + 1, 0x3FFF),
+};
+
+static const struct regmap_access_table inv_icm20948_regmap_rd_noinc_table = {
+ .no_ranges = inv_icm20948_rd_noinc_no_ranges,
+ .n_no_ranges = ARRAY_SIZE(inv_icm20948_rd_noinc_no_ranges),
+};
+
+const struct regmap_config inv_icm20948_regmap_config = {
+ .name = "inv_icm20948",
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = 0x3FFF,
+ .ranges = inv_icm20948_regmap_ranges,
+ .num_ranges = ARRAY_SIZE(inv_icm20948_regmap_ranges),
+ .volatile_table = &inv_icm20948_regmap_volatile_accesses,
+ .rd_noinc_table = &inv_icm20948_regmap_rd_noinc_table,
+ .cache_type = REGCACHE_MAPLE,
+};
+EXPORT_SYMBOL_NS_GPL(inv_icm20948_regmap_config, "IIO_ICM20948");
+
+static int inv_icm20948_setup(struct inv_icm20948_state *state)
+{
+ guard(mutex)(&state->lock);
+
+ int reported_whoami;
+ int ret = regmap_read(state->regmap, INV_ICM20948_REG_WHOAMI,
+ &reported_whoami);
+ if (ret)
+ return ret;
+ if (reported_whoami != INV_ICM20948_WHOAMI) {
+ dev_err(state->dev, "invalid whoami %d, expected %d\n",
+ reported_whoami, INV_ICM20948_WHOAMI);
+ return -ENODEV;
+ }
+
+ ret = regmap_write_bits(state->regmap, INV_ICM20948_REG_PWR_MGMT_1,
+ INV_ICM20948_PWR_MGMT_1_DEV_RESET,
+ INV_ICM20948_PWR_MGMT_1_DEV_RESET);
+ if (ret)
+ return ret;
+ msleep(INV_ICM20948_SLEEP_WAKEUP_MS);
+
+ ret = regmap_write_bits(state->regmap, INV_ICM20948_REG_PWR_MGMT_1,
+ INV_ICM20948_PWR_MGMT_1_SLEEP, 0);
+ if (ret)
+ return ret;
+ msleep(INV_ICM20948_SLEEP_WAKEUP_MS);
+
+ state->temp_dev = inv_icm20948_temp_init(state);
+ if (IS_ERR(state->temp_dev))
+ return PTR_ERR(state->temp_dev);
+
+ return 0;
+}
+
+int inv_icm20948_core_probe(struct regmap *regmap)
+{
+ struct device *dev = regmap_get_device(regmap);
+
+ struct inv_icm20948_state *state;
+
+ state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return -ENOMEM;
+
+ state->regmap = regmap;
+ state->dev = dev;
+
+ mutex_init(&state->lock);
+
+ return inv_icm20948_setup(state);
+}
+
+MODULE_AUTHOR("Bharadwaj Raju <bharadwaj.raju777@gmail.com>");
+MODULE_DESCRIPTION("InvenSense ICM-20948 device driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/imu/inv_icm20948/inv_icm20948_i2c.c b/drivers/iio/imu/inv_icm20948/inv_icm20948_i2c.c
new file mode 100644
index 0000000000000000000000000000000000000000..cf04d82e014a2497592c9a15bbde6e36f431dd56
--- /dev/null
+++ b/drivers/iio/imu/inv_icm20948/inv_icm20948_i2c.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2025 Bharadwaj Raju <bharadwaj.raju777@gmail.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/property.h>
+
+#include "inv_icm20948.h"
+
+static int inv_icm20948_probe(struct i2c_client *client)
+{
+ struct regmap *regmap =
+ devm_regmap_init_i2c(client, &inv_icm20948_regmap_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ return inv_icm20948_core_probe(regmap);
+}
+
+static const struct i2c_device_id inv_icm20948_id[] = { { "icm20948" }, {} };
+MODULE_DEVICE_TABLE(i2c, inv_icm20948_id);
+
+static const struct of_device_id inv_icm20948_of_matches[] = {
+ { .compatible = "invensense,icm20948" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, inv_icm20948_of_matches);
+
+static struct i2c_driver inv_icm20948_driver = {
+ .driver = {
+ .name = "icm20948",
+ .of_match_table = inv_icm20948_of_matches,
+ },
+ .probe = inv_icm20948_probe,
+ .id_table = inv_icm20948_id,
+};
+module_i2c_driver(inv_icm20948_driver);
+
+MODULE_AUTHOR("Bharadwaj Raju <bharadwaj.raju777@gmail.com>");
+MODULE_DESCRIPTION("InvenSense ICM-20948 device driver (I2C)");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("IIO_ICM20948");
diff --git a/drivers/iio/imu/inv_icm20948/inv_icm20948_temp.c b/drivers/iio/imu/inv_icm20948/inv_icm20948_temp.c
new file mode 100644
index 0000000000000000000000000000000000000000..916053740cc5acda0316c76504d4086eff5ec7f0
--- /dev/null
+++ b/drivers/iio/imu/inv_icm20948/inv_icm20948_temp.c
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2025 Bharadwaj Raju <bharadwaj.raju777@gmail.com>
+ */
+
+#include <linux/bits.h>
+
+#include <linux/iio/iio.h>
+
+#include "inv_icm20948.h"
+
+static const struct iio_chan_spec
+ inv_icm20948_temp_chan = { .type = IIO_TEMP,
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_OFFSET) |
+ BIT(IIO_CHAN_INFO_SCALE),
+ .scan_index = 0,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 16,
+ } };
+
+static int inv_icm20948_temp_read_sensor(struct inv_icm20948_state *state,
+ s16 *temp)
+{
+ guard(mutex)(&state->lock);
+
+ __be16 raw;
+ int ret = regmap_bulk_read(state->regmap, INV_ICM20948_REG_TEMP_DATA,
+ &raw, sizeof(raw));
+ if (ret)
+ return ret;
+
+ *temp = __be16_to_cpu(raw);
+
+ return 0;
+}
+
+static int inv_icm20948_temp_read_raw(struct iio_dev *temp_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct inv_icm20948_state *state = iio_device_get_drvdata(temp_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ if (!iio_device_claim_direct(temp_dev))
+ return -EBUSY;
+ s16 temp;
+ int ret = inv_icm20948_temp_read_sensor(state, &temp);
+
+ if (ret)
+ return ret;
+ iio_device_release_direct(temp_dev);
+ *val = temp;
+ return IIO_VAL_INT;
+ /*
+ * Sensitivity = 333.87
+ * RoomTempOff = 21
+ * T_degC = ((T_raw - RoomTempOff) / Sensitivity) + RoomTempOff
+ * T_degC = ((T_raw - 21) / 333.87) + 21
+ * T_milliDegC = 1000 * (((T_raw - 21) / 333.87) + 21)
+ * T_milliDegC = (1000 / 333.87) * (T_raw - 21 + (21 * 333.87))
+ * T_milliDegC = (T_raw + 6990.27) * 2.995177
+
+ * scale = 2.995177
+ * offset = 6990.27
+ */
+ case IIO_CHAN_INFO_SCALE:
+ *val = 2;
+ *val2 = 995177;
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_CHAN_INFO_OFFSET:
+ *val = 6990;
+ *val2 = 270000;
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info inv_icm20948_temp_info = {
+ .read_raw = inv_icm20948_temp_read_raw,
+};
+
+struct iio_dev *inv_icm20948_temp_init(struct inv_icm20948_state *state)
+{
+ struct iio_dev *temp_dev = devm_iio_device_alloc(state->dev, 0);
+
+ if (!temp_dev)
+ return ERR_PTR(-ENOMEM);
+
+ iio_device_set_drvdata(temp_dev, state);
+
+ temp_dev->name = "icm20948-temp";
+ temp_dev->info = &inv_icm20948_temp_info;
+ temp_dev->modes = INDIO_DIRECT_MODE;
+ temp_dev->channels = &inv_icm20948_temp_chan;
+ temp_dev->num_channels = 1;
+
+ int ret = devm_iio_device_register(state->dev, temp_dev);
+
+ if (ret)
+ return ERR_PTR(ret);
+
+ return temp_dev;
+}
--
2.51.0On Sun, 31 Aug 2025 00:12:46 +0530
Bharadwaj Raju <bharadwaj.raju777@gmail.com> wrote:
> Core parts of the new ICM20948 driver.
>
> Add register definitions, probing, setup, and an IIO device for
> reading the onboard temperature sensor.
>
> Signed-off-by: Bharadwaj Raju <bharadwaj.raju777@gmail.com>
Hi Bharadwaj,
I see you've already gotten some reviews on this so I'll take only
a fairly quick look. I may also overlap somewhat with the other reviewers.
Biggest question is I'm not seeing why you need an IIO device just
for the on die (and that is important vs on board) temperature sensor.
Jonathan
> ---
> drivers/iio/imu/Kconfig | 1 +
> drivers/iio/imu/Makefile | 1 +
> drivers/iio/imu/inv_icm20948/Kconfig | 17 ++++
> drivers/iio/imu/inv_icm20948/Makefile | 8 ++
> drivers/iio/imu/inv_icm20948/inv_icm20948.h | 47 +++++++++
> drivers/iio/imu/inv_icm20948/inv_icm20948_core.c | 122 +++++++++++++++++++++++
> drivers/iio/imu/inv_icm20948/inv_icm20948_i2c.c | 48 +++++++++
> drivers/iio/imu/inv_icm20948/inv_icm20948_temp.c | 108 ++++++++++++++++++++
> 8 files changed, 352 insertions(+)
>
> diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
> index 15612f0f189b5114deb414ef840339678abdc562..d59e5b0087398cfbd2719ca914fd147ab067155f 100644
> --- a/drivers/iio/imu/Kconfig
> +++ b/drivers/iio/imu/Kconfig
> @@ -109,6 +109,7 @@ config KMX61
> be called kmx61.
>
> source "drivers/iio/imu/inv_icm42600/Kconfig"
> +source "drivers/iio/imu/inv_icm20948/Kconfig"
Alphabetical/numeric order.
> source "drivers/iio/imu/inv_mpu6050/Kconfig"
>
> config SMI240
> diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
> index e901aea498d37e5897e8b71268356a19eac2cb59..79e49bae59038c1ca1d54a64cf49b6ca5f57cb0b 100644
> --- a/drivers/iio/imu/Makefile
> +++ b/drivers/iio/imu/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_FXOS8700_I2C) += fxos8700_i2c.o
> obj-$(CONFIG_FXOS8700_SPI) += fxos8700_spi.o
>
> obj-y += inv_icm42600/
> +obj-y += inv_icm20948/
here as well.
> obj-y += inv_mpu6050/
>
> obj-$(CONFIG_KMX61) += kmx61.o
> diff --git a/drivers/iio/imu/inv_icm20948/inv_icm20948.h b/drivers/iio/imu/inv_icm20948/inv_icm20948.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..f9830645fbe96fd02eef7c54d1e5908647d5a0fe
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm20948/inv_icm20948.h
> @@ -0,0 +1,47 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2025 Bharadwaj Raju <bharadwaj.raju777@gmail.com>
> + */
> +
> +#ifndef INV_ICM20948_H_
> +#define INV_ICM20948_H_
> +
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/err.h>
> +
> +/* accel takes 20ms, gyro takes 35ms to wake from full-chip sleep */
> +#define INV_ICM20948_SLEEP_WAKEUP_MS 35
> +
> +#define INV_ICM20948_REG_BANK_SEL 0x7F
> +#define INV_ICM20948_BANK_SEL_MASK GENMASK(5, 4)
> +
> +#define INV_ICM20948_REG_WHOAMI 0x0000
> +#define INV_ICM20948_WHOAMI 0xEA
> +
> +#define INV_ICM20948_REG_FIFO_RW 0x0072
> +
> +#define INV_ICM20948_REG_PWR_MGMT_1 0x0006
> +#define INV_ICM20948_PWR_MGMT_1_DEV_RESET BIT(7)
> +#define INV_ICM20948_PWR_MGMT_1_SLEEP BIT(6)
> +
> +#define INV_ICM20948_REG_TEMP_DATA 0x0039
> +
> +extern const struct regmap_config inv_icm20948_regmap_config;
> +
> +struct inv_icm20948_state {
> + struct device *dev;
> + struct regmap *regmap;
> + struct iio_dev *temp_dev;
> + struct mutex lock;
Lock scope needs to always have a comment. What 'data' is this protecting
from concurrent accesses?
> +};
> +
> +extern int inv_icm20948_core_probe(struct regmap *regmap);
> +
> +struct iio_dev *inv_icm20948_temp_init(struct inv_icm20948_state *state);
> +
> +#endif
> diff --git a/drivers/iio/imu/inv_icm20948/inv_icm20948_core.c b/drivers/iio/imu/inv_icm20948/inv_icm20948_core.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..ee9e4159cffa261f0326b146a4b3df2cbfbd7697
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm20948/inv_icm20948_core.c
> +
> +static int inv_icm20948_setup(struct inv_icm20948_state *state)
> +{
> + guard(mutex)(&state->lock);
> +
As below.
> + int reported_whoami;
> + int ret = regmap_read(state->regmap, INV_ICM20948_REG_WHOAMI,
> + &reported_whoami);
> + if (ret)
> + return ret;
> + if (reported_whoami != INV_ICM20948_WHOAMI) {
> + dev_err(state->dev, "invalid whoami %d, expected %d\n",
> + reported_whoami, INV_ICM20948_WHOAMI);
This breaks fallback compatibles used in device tree to support
newer parts that the driver has not yet been updated for, as long
as they are backwards compatible with older ones.
We still have some old drivers with hard checks like this, but current
practice is to at most print a message and then carry on anyway.
> + return -ENODEV;
> + }
> +
> + ret = regmap_write_bits(state->regmap, INV_ICM20948_REG_PWR_MGMT_1,
> + INV_ICM20948_PWR_MGMT_1_DEV_RESET,
> + INV_ICM20948_PWR_MGMT_1_DEV_RESET);
> + if (ret)
> + return ret;
> + msleep(INV_ICM20948_SLEEP_WAKEUP_MS);
> +
> + ret = regmap_write_bits(state->regmap, INV_ICM20948_REG_PWR_MGMT_1,
> + INV_ICM20948_PWR_MGMT_1_SLEEP, 0);
> + if (ret)
> + return ret;
> + msleep(INV_ICM20948_SLEEP_WAKEUP_MS);
> +
> + state->temp_dev = inv_icm20948_temp_init(state);
> + if (IS_ERR(state->temp_dev))
> + return PTR_ERR(state->temp_dev);
> +
> + return 0;
> +}
> +
> +int inv_icm20948_core_probe(struct regmap *regmap)
> +{
> + struct device *dev = regmap_get_device(regmap);
> +
> + struct inv_icm20948_state *state;
> +
> + state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
Given question on why you need separate struct iio_dev instances below
I suspect this will change a lot and we won't end up with another state
structure here.
> + if (!state)
> + return -ENOMEM;
> +
> + state->regmap = regmap;
> + state->dev = dev;
> +
> + mutex_init(&state->lock);
ret = devm_mutex_init()
if (ret)
return ret;
The advantages of this are small, but it costs us little so
lets enable the lock debugging stuff that the destroy_mutex() this
will call enables.
> +
> + return inv_icm20948_setup(state);
> +}
> diff --git a/drivers/iio/imu/inv_icm20948/inv_icm20948_i2c.c b/drivers/iio/imu/inv_icm20948/inv_icm20948_i2c.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..cf04d82e014a2497592c9a15bbde6e36f431dd56
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm20948/inv_icm20948_i2c.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2025 Bharadwaj Raju <bharadwaj.raju777@gmail.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/property.h>
> +
> +#include "inv_icm20948.h"
> +
> +static int inv_icm20948_probe(struct i2c_client *client)
> +{
> + struct regmap *regmap =
> + devm_regmap_init_i2c(client, &inv_icm20948_regmap_config);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + return inv_icm20948_core_probe(regmap);
> +}
> +
> +static const struct i2c_device_id inv_icm20948_id[] = { { "icm20948" }, {} };
static const struct i2c_device_id inv_icm20948_id[] = {
{ "icm20948" },
{ }
};
> +MODULE_DEVICE_TABLE(i2c, inv_icm20948_id);
> +
> +static const struct of_device_id inv_icm20948_of_matches[] = {
> + { .compatible = "invensense,icm20948" },
> + {}
Trivial style preference that I'm trying to generalize
across IIO for
{ }
> +};
> +MODULE_DEVICE_TABLE(of, inv_icm20948_of_matches);
> diff --git a/drivers/iio/imu/inv_icm20948/inv_icm20948_temp.c b/drivers/iio/imu/inv_icm20948/inv_icm20948_temp.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..916053740cc5acda0316c76504d4086eff5ec7f0
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm20948/inv_icm20948_temp.c
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2025 Bharadwaj Raju <bharadwaj.raju777@gmail.com>
> + */
> +
> +#include <linux/bits.h>
> +
> +#include <linux/iio/iio.h>
Andy covered the need to follow include what you use principles for includes.
> +
> +#include "inv_icm20948.h"
> +
> +static const struct iio_chan_spec
> + inv_icm20948_temp_chan = { .type = IIO_TEMP,
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_OFFSET) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .scan_index = 0,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 16,
> + } };
> +
Preferred format is something like this.
static const struct iio_chan_spec inv_icm20948_temp_chan = {
.type = IIO_TEMP,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_OFFSET) |
BIT(IIO_CHAN_INFO_SCALE),
.scan_index = 0,
.scan_type = {
.sign = 's',
.realbits = 16,
},
};
Note the trailing comma on scan_type that will make this easier to extent
in future.
> +
> +static int inv_icm20948_temp_read_raw(struct iio_dev *temp_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct inv_icm20948_state *state = iio_device_get_drvdata(temp_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (!iio_device_claim_direct(temp_dev))
> + return -EBUSY;
> + s16 temp;
> + int ret = inv_icm20948_temp_read_sensor(state, &temp);
As already pointed out we normally don't mix declarations and code.
There is an exception for cleanup.h cases but that doesn't apply here.
> +
> + if (ret)
> + return ret;
> + iio_device_release_direct(temp_dev);
> + *val = temp;
> + return IIO_VAL_INT;
> +}
> +struct iio_dev *inv_icm20948_temp_init(struct inv_icm20948_state *state)
> +{
> + struct iio_dev *temp_dev = devm_iio_device_alloc(state->dev, 0);
> +
> + if (!temp_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + iio_device_set_drvdata(temp_dev, state);
> +
> + temp_dev->name = "icm20948-temp";
> + temp_dev->info = &inv_icm20948_temp_info;
> + temp_dev->modes = INDIO_DIRECT_MODE;
> + temp_dev->channels = &inv_icm20948_temp_chan;
> + temp_dev->num_channels = 1;
> +
> + int ret = devm_iio_device_register(state->dev, temp_dev);
It's unusual for it to make sense to register a separate iio device instance for the
temperature sensor in an IMU. They tend to be there only to allow
temperature compensation based on measures of die temperature.
Why does it make sense for this device? There are a few reasons
I could think of that might justify it but I took at quick read
of the datasheet and I can't see a reason to split it up at all. Looks
like one iio_dev will do for the whole thing.
Jonathan
> +
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return temp_dev;
> +}
>
On Sat, Aug 30, 2025 at 9:43 PM Bharadwaj Raju
<bharadwaj.raju777@gmail.com> wrote:
>
> Core parts of the new ICM20948 driver.
>
> Add register definitions, probing, setup, and an IIO device for
> reading the onboard temperature sensor.
...
> drivers/iio/imu/Kconfig | 1 +
> drivers/iio/imu/Makefile | 1 +
> drivers/iio/imu/inv_icm20948/Kconfig | 17 ++++
> drivers/iio/imu/inv_icm20948/Makefile | 8 ++
> drivers/iio/imu/inv_icm20948/inv_icm20948.h | 47 +++++++++
> drivers/iio/imu/inv_icm20948/inv_icm20948_core.c | 122 +++++++++++++++++++++++
> drivers/iio/imu/inv_icm20948/inv_icm20948_i2c.c | 48 +++++++++
> drivers/iio/imu/inv_icm20948/inv_icm20948_temp.c | 108 ++++++++++++++++++++
> 8 files changed, 352 insertions(+)
This looks strange. The few files are created here, and there is no
comment like "new file ..."
That said, the MAINTAINER is not updated here and hence the file will
be orphaned till the MAINTAINER changes. So, update incrementally
MAINTAINERS starting from the first patch.
...
> +#ifndef INV_ICM20948_H_
> +#define INV_ICM20948_H_
> +
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
Unused.
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
Can be replaced with proper forward declarations.
> +#include <linux/i2c.h>
Unused
> +#include <linux/iio/iio.h>
Forward declaration.
> +#include <linux/err.h>
Unused.
Keep it ordered and follow the IWYU principle (include what you use).
Currently this is a semi-random list of the inclusions. And missed
forward declarations.
...
> +#define INV_ICM20948_REG_BANK_SEL 0x7F
Make all register offsets to be fixed-width, like 0x007F
> +#define INV_ICM20948_BANK_SEL_MASK GENMASK(5, 4)
> +
> +#define INV_ICM20948_REG_WHOAMI 0x0000
> +#define INV_ICM20948_WHOAMI 0xEA
> +
> +#define INV_ICM20948_REG_FIFO_RW 0x0072
> +
> +#define INV_ICM20948_REG_PWR_MGMT_1 0x0006
> +#define INV_ICM20948_PWR_MGMT_1_DEV_RESET BIT(7)
> +#define INV_ICM20948_PWR_MGMT_1_SLEEP BIT(6)
> +
> +#define INV_ICM20948_REG_TEMP_DATA 0x0039
> +
> +extern const struct regmap_config inv_icm20948_regmap_config;
> +
> +struct inv_icm20948_state {
> + struct device *dev;
> + struct regmap *regmap;
> + struct iio_dev *temp_dev;
> + struct mutex lock;
> +};
> +
> +extern int inv_icm20948_core_probe(struct regmap *regmap);
> +
> +struct iio_dev *inv_icm20948_temp_init(struct inv_icm20948_state *state);
> +
> +#endif
...
> +#include "inv_icm20948.h"
This file uses much more than the private header. Please, fix this properly.
...
> +static const struct regmap_range_cfg inv_icm20948_regmap_ranges[] = {
> + {
> + .name = "user banks",
> + .range_min = 0x0000,
> + .range_max = 0x3FFF,
Are you sure about range_min? This will cause circular caching and
unpleasant side-effects.
> + .selector_reg = INV_ICM20948_REG_BANK_SEL,
> + .selector_mask = INV_ICM20948_BANK_SEL_MASK,
> + .window_start = 0,
> + .window_len = 0x1000,
> + },
> +};
> +
> +static const struct regmap_range inv_icm20948_regmap_volatile_yes_ranges[] = {
> + /* WHOAMI */
> + regmap_reg_range(0x0000, 0x0000),
> + /* PWR_MGMT_1 */
> + regmap_reg_range(0x0006, 0x0006),
> + /* I2C and INT status */
> + regmap_reg_range(0x0017, 0x001C),
> + /* Sensor readouts */
> + regmap_reg_range(0x0028, 0x0052),
> + /* FIFO count and data */
> + regmap_reg_range(0x0070, 0x0072),
> + /* Data ready status */
> + regmap_reg_range(0x0074, 0x0074),
So, the above are real, the below are virtual? How does it work?
> + /* GYRO_CONFIG_1 */
> + regmap_reg_range(0x2001, 0x2001),
> + /* I2C SLV4 data in */
> + regmap_reg_range(0x307F, 0x307F),
> +};
...
> +static const struct regmap_range inv_icm20948_rd_noinc_no_ranges[] = {
> + regmap_reg_range(0x0000, INV_ICM20948_REG_FIFO_RW - 1),
> + regmap_reg_range(INV_ICM20948_REG_FIFO_RW + 1, 0x3FFF),
Make 0x3fff a constant and use it everywhere.
> +};
...
> +const struct regmap_config inv_icm20948_regmap_config = {
> + .name = "inv_icm20948",
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = 0x3FFF,
See above.
> + .ranges = inv_icm20948_regmap_ranges,
> + .num_ranges = ARRAY_SIZE(inv_icm20948_regmap_ranges),
> + .volatile_table = &inv_icm20948_regmap_volatile_accesses,
> + .rd_noinc_table = &inv_icm20948_regmap_rd_noinc_table,
> + .cache_type = REGCACHE_MAPLE,
> +};
...
> +static int inv_icm20948_setup(struct inv_icm20948_state *state)
> +{
> + guard(mutex)(&state->lock);
> +
> + int reported_whoami;
> + int ret = regmap_read(state->regmap, INV_ICM20948_REG_WHOAMI,
> + &reported_whoami);
Just no. We almost do not allow a mixture of the definitions and code.
> + if (ret)
> + return ret;
> + if (reported_whoami != INV_ICM20948_WHOAMI) {
> + dev_err(state->dev, "invalid whoami %d, expected %d\n",
> + reported_whoami, INV_ICM20948_WHOAMI);
> + return -ENODEV;
> + }
> +
> + ret = regmap_write_bits(state->regmap, INV_ICM20948_REG_PWR_MGMT_1,
> + INV_ICM20948_PWR_MGMT_1_DEV_RESET,
> + INV_ICM20948_PWR_MGMT_1_DEV_RESET);
> + if (ret)
> + return ret;
Any long sleeps (and even 1ms is already long on modern fast CPUs)
must be explained.
> + msleep(INV_ICM20948_SLEEP_WAKEUP_MS);
> +
> + ret = regmap_write_bits(state->regmap, INV_ICM20948_REG_PWR_MGMT_1,
> + INV_ICM20948_PWR_MGMT_1_SLEEP, 0);
> + if (ret)
> + return ret;
> + msleep(INV_ICM20948_SLEEP_WAKEUP_MS);
Ditto.
> + state->temp_dev = inv_icm20948_temp_init(state);
> + if (IS_ERR(state->temp_dev))
> + return PTR_ERR(state->temp_dev);
> +
> + return 0;
return PTR_ERR_OR_ZERO(...);
> +}
> +
> +int inv_icm20948_core_probe(struct regmap *regmap)
> +{
> + struct device *dev = regmap_get_device(regmap);
> +
Have you ever run checkpatch.pl? We do not allow blank lines in the
definition blocks.
> + struct inv_icm20948_state *state;
> +
> + state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> + if (!state)
> + return -ENOMEM;
> +
> + state->regmap = regmap;
> + state->dev = dev;
> + mutex_init(&state->lock);
devm_mutex_init()
> + return inv_icm20948_setup(state);
> +}
...
> +#include <linux/kernel.h>
This header must not be used in the regular drivers. Follow the IWYU principle.
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/property.h>
Keep them ordered.
...
> +static int inv_icm20948_probe(struct i2c_client *client)
> +{
> + struct regmap *regmap =
> + devm_regmap_init_i2c(client, &inv_icm20948_regmap_config);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
This is wrong stylistically.
First of all, there must be a blank line between definition block and
the code, BUT in this case the code will be harder to maintain and it
will be prone to subtle errors, that's why it's recommended to split
definition and the assignment and move assignment closest to the
check.
> + return inv_icm20948_core_probe(regmap);
> +}
> +
> +static const struct i2c_device_id inv_icm20948_id[] = { { "icm20948" }, {} };
No, please make it readable.
> +MODULE_DEVICE_TABLE(i2c, inv_icm20948_id);
> +
> +static const struct of_device_id inv_icm20948_of_matches[] = {
> + { .compatible = "invensense,icm20948" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, inv_icm20948_of_matches);
> +
> +static struct i2c_driver inv_icm20948_driver = {
> + .driver = {
> + .name = "icm20948",
> + .of_match_table = inv_icm20948_of_matches,
> + },
> + .probe = inv_icm20948_probe,
> + .id_table = inv_icm20948_id,
> +};
> +module_i2c_driver(inv_icm20948_driver);
> +
> +MODULE_AUTHOR("Bharadwaj Raju <bharadwaj.raju777@gmail.com>");
> +MODULE_DESCRIPTION("InvenSense ICM-20948 device driver (I2C)");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("IIO_ICM20948");
I stop here as I believe the whole series needs much more work to
follow IWYU, the style, coding standards, etc...
When you are done with that, come up with a new version.
Thanks!
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.