| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VeKfHQH3LMCjVdjd=kJA4s6-Ux1-WHNzOsYA-f4QkkUyw@mail.gmail.com>
Date: Sat, 30 Aug 2025 23:42:26 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Bharadwaj Raju <bharadwaj.raju777@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, shuah@...nel.org,
linux-kernel-mentees@...ts.linux.dev
Subject: Re: [PATCH 2/5] iio: imu: add inv_icm20948
On Sat, Aug 30, 2025 at 9:43 PM Bharadwaj Raju
<bharadwaj.raju777@...il.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@...il.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
Powered by blists - more mailing lists