linux-kernel - Re: [PATCH v2 2/8] iio: imu: inv_icm45600: add new inv

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aG-ID7O3HgVc1EOX@smile.fi.intel.com>
Date: Thu, 10 Jul 2025 12:29:51 +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 v2 2/8] iio: imu: inv_icm45600: add new inv_icm45600
 driver

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