Re: [PATCH v2 2/8] iio: imu: inv_icm45600: add new inv_icm45600 driver

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Thu, Jul 10, 2025 at 08:57:57AM +0000, Remi Buisson via B4 Relay wrote:
> From: Remi Buisson <remi.buisson@tdk.com>
> 
> 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.

...

> +	INV_ICM45600_SENSOR_MODE_NB

What does the _NB stand for? Number of Bullets?

...

> +struct inv_icm45600_sensor_conf {
> +	int mode;
> +	int fs;
> +	int odr;
> +	int filter;

Any of them can hold negative value?

> +};

...

> +#define INV_ICM45600_SENSOR_CONF_INIT		{-1, -1, -1, -1}

Unused.


> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.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>

...

> +static int inv_icm45600_ireg_read(struct regmap *map, unsigned int reg,
> +				   u8 *data, size_t count)
> +{
> +	int ret;
> +	u8 addr[2];
> +	ssize_t i;
> +	unsigned int d;
> +
> +	addr[0] = FIELD_GET(INV_ICM45600_REG_BANK_MASK, reg);
> +	addr[1] = FIELD_GET(INV_ICM45600_REG_ADDR_MASK, reg);
> +
> +	/* Burst write address. */
> +	ret = regmap_bulk_write(map, INV_ICM45600_REG_IREG_ADDR, addr, 2);

sizeof()?

> +	udelay(INV_ICM45600_IREG_DELAY_US);

See below. This is also weird.

> +	if (ret)
> +		return ret;
> +
> +	/* Read the data. */
> +	for (i = 0; i < count; i++) {
> +		ret = regmap_read(map, INV_ICM45600_REG_IREG_DATA, &d);
> +		data[i] = d;
> +		udelay(INV_ICM45600_IREG_DELAY_US);

Can fsleep() be used here?

> +		if (ret)
> +			return ret;

This is weird. First you assign a garbage to the output, delay and return
an error. It seems entire code is broken...
Please, fix all these and try again, I stop my review here.

> +	}
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko