| 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: <aKbgt_g3FsLMM8-g@smile.fi.intel.com>
Date: Thu, 21 Aug 2025 12:02:47 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: remi.buisson@....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-kernel@...r.kernel.org,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v5 2/9] iio: imu: inv_icm45600: add new inv_icm45600
driver
On Wed, Aug 20, 2025 at 02:24:20PM +0000, Remi Buisson via B4 Relay wrote:
>
> Core component of a new driver for InvenSense ICM-45600 devices.
> It includes registers definition, main probe/setup, and device
> utility functions.
>
> ICM-456xx devices are latest generation of 6-axis IMU,
> gyroscope+accelerometer and temperature sensor. This device
> includes a 8K FIFO, supports I2C/I3C/SPI, and provides
> intelligent motion features like pedometer, tilt detection,
> and tap detection.
...
> +#ifndef INV_ICM45600_H_
> +#define INV_ICM45600_H_
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/iio/common/inv_sensors_timestamp.h>
> +#include <linux/iio/iio.h>
> +#include <linux/types.h>
Please, follow IWYU principle. Also, it's better to split out the IIO group as
it's part of the subsystem this driver is for.
#include <linux/bitfield.h>
#include <linux/bits.h>
#include <linux/types.h>
#include <linux/iio/common/inv_sensors_timestamp.h>
#include <linux/iio/iio.h>
(but again, the list of the headers seems incorrect / incomplete).
...
> +struct inv_icm45600_state {
> + struct mutex lock;
No header for this.
> + struct regmap *map;
No forward declaration.
> + struct regulator *vddio_supply;
Ditto.
> + struct iio_mount_matrix orientation;
> + struct iio_dev *indio_gyro;
> + struct iio_dev *indio_accel;
> + const struct inv_icm45600_chip_info *chip_info;
> + struct {
> + s64 gyro;
> + s64 accel;
> + } timestamp;
> + union {
> + u8 buff[2];
> + __le16 u16;
> + u8 ireg[3];
> + } buffer __aligned(IIO_DMA_MINALIGN);
> +};
...
> +#define INV_ICM45600_FIFO_SIZE_MAX (8 * 1024)
SZ_8K from sizes.h ?
...
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/limits.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
As per above, please double check for IWYU principle.
...
> +static int inv_icm45600_ireg_read(struct regmap *map, unsigned int reg,
> + u8 *data, size_t count)
> +{
> + const struct device *dev = regmap_get_device(map);
> + struct inv_icm45600_state *st = dev_get_drvdata(dev);
> + unsigned int d;
> + ssize_t i;
Why signed? Same comment for all similar cases.
> + int ret;
> +
> + st->buffer.ireg[0] = FIELD_GET(INV_ICM45600_REG_BANK_MASK, reg);
> + st->buffer.ireg[1] = FIELD_GET(INV_ICM45600_REG_ADDR_MASK, reg);
> +
> + /* Burst write address. */
> + ret = regmap_bulk_write(map, INV_ICM45600_REG_IREG_ADDR, st->buffer.ireg, 2);
> + /* Wait while the device is busy processing the address. */
> + fsleep(INV_ICM45600_IREG_DELAY_US);
> + if (ret)
> + return ret;
> +
> + /* Read the data. */
> + for (i = 0; i < count; i++) {
> + ret = regmap_read(map, INV_ICM45600_REG_IREG_DATA, &d);
> + /* Wait while the device is busy processing the data. */
> + fsleep(INV_ICM45600_IREG_DELAY_US);
> + if (ret)
> + return ret;
> + data[i] = d;
> + }
> +
> + return 0;
> +}
...
> + if (FIELD_GET(INV_ICM45600_REG_BANK_MASK, reg) == 0)
Why not using positive conditional?
> + return regmap_bulk_read(map, FIELD_GET(INV_ICM45600_REG_ADDR_MASK, reg),
> + val_buf, val_size);
> +
> + return inv_icm45600_ireg_read(map, reg, val_buf, val_size);
if (FIELD_GET(INV_ICM45600_REG_BANK_MASK, reg))
return inv_icm45600_ireg_read(map, reg, val_buf, val_size);
Ditto for other similar cases.
...
> +static int inv_icm45600_write(void *context, const void *data,
> + size_t count)
This is perfectly 1 line, please, check that the code utilises exactly 80 limit
when there is a room. It's probably a wrapping done by the (mis)configured editor.
...
> +static const struct regmap_config inv_icm45600_regmap_config = {
> + .reg_bits = 16,
> + .val_bits = 8,
No cache?
> +};
...
> +static const struct inv_icm45600_conf inv_icm45600_default_conf = {
> + .gyro = {
> + .mode = INV_ICM45600_SENSOR_MODE_OFF,
> + .fs = INV_ICM45686_GYRO_FS_2000DPS,
> + .odr = INV_ICM45600_ODR_800HZ_LN,
> + .filter = INV_ICM45600_GYRO_LP_AVG_SEL_8X,
> + },
> + .accel = {
> + .mode = INV_ICM45600_SENSOR_MODE_OFF,
> + .fs = INV_ICM45686_ACCEL_FS_16G,
> + .odr = INV_ICM45600_ODR_800HZ_LN,
> + .filter = INV_ICM45600_ACCEL_LP_AVG_SEL_4X,
> + },
> +};
Can you split the patch adding accel or gyro separately? I haven't checked all
the details, so it might be not worth it, just consider it.
...
> +u32 inv_icm45600_odr_to_period(enum inv_icm45600_odr odr)
> +{
> + static const u32 odr_periods[INV_ICM45600_ODR_MAX] = {
> + 0, 0, 0, /* reserved values */
Make it one per line as the rest.
> + 156250, /* 6.4kHz */
> + 312500, /* 3.2kHz */
> + 625000, /* 1.6kHz */
> + 1250000, /* 800kHz */
> + 2500000, /* 400Hz */
> + 5000000, /* 200Hz */
> + 10000000, /* 100Hz */
> + 20000000, /* 50Hz */
> + 40000000, /* 25Hz */
> + 80000000, /* 12.5Hz */
> + 160000000, /* 6.25Hz */
> + 320000000, /* 3.125Hz */
> + 640000000, /* 1.5625Hz */
These seem to be times or so, can you use proper naming instead of _periods?
> + };
> +
> + return odr_periods[odr];
> +}
...
> +int inv_icm45600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
> + unsigned int writeval, unsigned int *readval)
> +{
> + struct inv_icm45600_state *st = iio_device_get_drvdata(indio_dev);
> + int ret;
Useless, just return directly.
> + guard(mutex)(&st->lock);
> + if (readval)
> + ret = regmap_read(st->map, reg, readval);
> + else
> + ret = regmap_write(st->map, reg, writeval);
> +
> + return ret;
> +}
...
> +/**
> + * inv_icm45600_setup() - check and setup chip
> + * @st: driver internal state
> + * @chip_info: detected chip description
> + * @reset: define whether a reset is required or not
> + * @bus_setup: callback for setting up bus specific registers
> + *
> + * Returns 0 on success, a negative error code otherwise.
Please, run kernel-doc validator. It's not happy (Return section is missing)
> + */
...
> + if (val != chip_info->whoami) {
> + if (val == U8_MAX || val == 0)
Hmm... Perhaps in_range() ?
> + return dev_err_probe(dev, -ENODEV,
> + "Invalid whoami %#02x expected %#02x (%s)\n",
> + val, chip_info->whoami, chip_info->name);
> + else
Redundant 'else'.
> + dev_warn(dev, "Unexpected whoami %#02x expected %#02x (%s)\n",
> + val, chip_info->whoami, chip_info->name);
> + }
...
> + ret = regmap_write(st->map, INV_ICM45600_REG_MISC2,
> + INV_ICM45600_MISC2_SOFT_RESET);
> + if (ret)
> + return ret;
> + /* IMU reset time: 1ms. */
> + fsleep(1000);
Use 1 * USEC_PER_MSEC and drop useless comment after that.
You will need time.h for it.
> +
> + if (bus_setup) {
> + ret = bus_setup(st);
> + if (ret)
> + return ret;
> + }
> +
> + ret = regmap_read(st->map, INV_ICM45600_REG_INT_STATUS, &val);
> + if (ret)
> + return ret;
> + if (!(val & INV_ICM45600_INT_STATUS_RESET_DONE)) {
> + dev_err(dev, "reset error, reset done bit not set\n");
> + return -ENODEV;
> + }
...
> +static int inv_icm45600_enable_regulator_vddio(struct inv_icm45600_state *st)
> +{
> + int ret;
> +
> + ret = regulator_enable(st->vddio_supply);
> + if (ret)
> + return ret;
> +
> + /* Wait a little for supply ramp. */
> + fsleep(3000);
As per above.
> + return 0;
> +}
...
> +int inv_icm45600_core_probe(struct regmap *regmap, const struct inv_icm45600_chip_info *chip_info,
> + bool reset, inv_icm45600_bus_setup bus_setup)
> +{
> + struct device *dev = regmap_get_device(regmap);
> + struct fwnode_handle *fwnode;
> + struct inv_icm45600_state *st;
> + struct regmap *regmap_custom;
> + int ret;
> +
> + regmap_custom = devm_regmap_init(dev, &inv_icm45600_regmap_bus,
> + regmap, &inv_icm45600_regmap_config);
There is room for 'regmap' on the previous line.
> + if (IS_ERR(regmap_custom))
> + return dev_err_probe(dev, PTR_ERR(regmap_custom), "Failed to register regmap\n");
> +
> + st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> + if (!st)
> + return dev_err_probe(dev, -ENOMEM, "Cannot allocate memory\n");
> +
> + dev_set_drvdata(dev, st);
> +
> + ret = devm_mutex_init(dev, &st->lock);
> + if (ret)
> + return ret;
> +
> + st->map = regmap_custom;
> +
> + ret = iio_read_mount_matrix(dev, &st->orientation);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to retrieve mounting matrix\n");
> +
> + st->vddio_supply = devm_regulator_get(dev, "vddio");
> + if (IS_ERR(st->vddio_supply))
> + return PTR_ERR(st->vddio_supply);
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to get vdd regulator\n");
> +
> + /* IMU start-up time. */
> + fsleep(100000);
100 * USEC_PER_MSEC
> + ret = inv_icm45600_enable_regulator_vddio(st);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(dev, inv_icm45600_disable_vddio_reg, st);
> + if (ret)
> + return ret;
> +
> + ret = inv_icm45600_setup(st, chip_info, reset, bus_setup);
> + if (ret)
> + return ret;
> +
> + ret = inv_icm45600_timestamp_setup(st);
> + if (ret)
> + return ret;
> +
> + /* Setup runtime power management. */
> + ret = devm_pm_runtime_set_active_enabled(dev);
> + if (ret)
> + return ret;
> +
> + pm_runtime_get_noresume(dev);
> + /* Suspend after 2 seconds. */
> + pm_runtime_set_autosuspend_delay(dev, 2000);
2 * MSEC_PER_SEC and drop yet another useless comment.
> + pm_runtime_use_autosuspend(dev);
> + pm_runtime_put(dev);
> +
> + return 0;
> +}
...
> +static int inv_icm45600_resume(struct device *dev)
> +{
> + struct inv_icm45600_state *st = dev_get_drvdata(dev);
> + int ret = 0;
Why assignment?
> + ret = pm_runtime_force_resume(dev);
> + if (ret)
> + return ret;
> +
> + scoped_guard(mutex, &st->lock)
> + /* Restore sensors state. */
> + ret = inv_icm45600_set_pwr_mgmt0(st, st->suspended.gyro,
> + st->suspended.accel, NULL);
With guard()() this whole construction will look better.
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists