[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<FR2PPF4571F02BC601FD554873EE9C90C848C4BA@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Fri, 11 Jul 2025 11:39:31 +0000
From: Remi Buisson <Remi.Buisson@....com>
To: Andy Shevchenko <andriy.shevchenko@...el.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-kernel@...r.kernel.org>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [PATCH v2 2/8] iio: imu: inv_icm45600: add new inv_icm45600
driver
>
>
>From: Andy Shevchenko andriy.shevchenko@...el.com
>Sent: Thursday, July 10, 2025 11:30 AM
>To: Remi Buisson 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?
Hi Andy,
Thanks for your review !
I will rename it to something more explicit like INV_ICM45600_SENSOR_MODE_NUMBER
>
>...
>
>> +struct inv_icm45600_sensor_conf {
>> + int mode;
>> + int fs;
>> + int odr;
>> + int filter;
>
>Any of them can hold negative value?
Yes, when setting the configuration, a negative value means "keep previous configured value".
Like in the INV_ICM45600_SENSOR_CONF_INIT macro below.
>
>> +};
>
>...
>
>> +#define INV_ICM45600_SENSOR_CONF_INIT {-1, -1, -1, -1}
>
>Unused.
This is used in later patch of the serie.
I will move this definition to the patch using it.
>
>
>> +#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()?
Sure, will be better.
>
>> + udelay(INV_ICM45600_IREG_DELAY_US);
>
>See below. This is also weird.
This function implement indirect register access, requiring following sequence to be applied:
1. write indirect register address
2. wait for the data register to be updated
3. read register address
I will add a comment to explain what's is going on here.
>
>> + 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?
I will use fsleep there.
Is it recommended to use fsleep everywhere else in place of other sleep APIs?
>
>> + 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.
Agreed, data assignment must be avoided in case of error.
It's probably safer to keep the delay even in case of failure to make sure the device is ready before next operation.
>
>> + }
>> +
>> + return 0;
>> +}
>
>--
>With Best Regards,
>Andy Shevchenko
>
>
>
Powered by blists - more mailing lists