| 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:
<FR2PPF4571F02BC9E34D445208601939C798C03A@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Fri, 5 Sep 2025 12:43:02 +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 v5 2/9] iio: imu: inv_icm45600: add new inv_icm45600
driver
>
>
>From: Andy Shevchenko <andriy.shevchenko@...el.com>
>Sent: Thursday, September 4, 2025 3:17 PM
>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 v5 2/9] iio: imu: inv_icm45600: add new inv_icm45600 driver
>
>On Thu, Sep 04, 2025 at 12:58:10PM +0000, Remi Buisson wrote:
>> >From: Andy Shevchenko <andriy.shevchenko@...el.com>
>> >Sent: Thursday, August 21, 2025 11:03 AM
>> >On Wed, Aug 20, 2025 at 02:24:20PM +0000, Remi Buisson via B4 Relay wrote:
>
>...
>
Hello Andy,
Thanks for your quick reply.
>> >> +struct inv_icm45600_state {
>> >> + struct mutex lock;
>> >
>> >No header for this.
>>
>> Correct
>
>Please, add.
>
>...
>
>> >> + struct regmap *map;
>> >
>> >No forward declaration.
>>
>> Correct again
>
>Ditto.
>
>...
>
>> >> + struct regulator *vddio_supply;
>> >
>> >Ditto.
>>
>> Correct
>
>Ditto.
>
>...
>
>> >> +static const struct regmap_config inv_icm45600_regmap_config = {
>> >> + .reg_bits = 16,
>> >> + .val_bits = 8,
>> >
>> >No cache?
>> >
>> If OK for you, we prefer to push this patch without cache.
>> And introduce it in another patchset.
>
>Fine to me if there is a comment given (in the email, not in the code) to
>justify this split. Enabling cache is one line, but, of cource, it might
>require a cache handling in the corner or special cases.
Ok I'll update the cover letter.
>
>> >> +};
>
>...
>
>> >> +/**
>> >> + * 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)
>>
>> kernel-doc does not complain on this, on my side.
>> I ran kernel-doc.py -v -none drivers/iio/imu/inv_icm45600/*
>> Is there any option I'm missing.
>> Anyway, I will add the missing colon and check the result.
>
>-Wall is missed in the command line.
Ok Thanks!
>
>> >> + */
>
>...
>
>> >> + if (val == U8_MAX || val == 0)
>> >
>> >Hmm... Perhaps in_range() ?
>>
>> Not sure of the benefit of this change.
>> I prefer to keep it this way if OK for you.
>
>It depends on the semantics of the value in the 'val'. And hence semantics of 0
>and U8_MAX.
With SPI interface, there is no ACK mechanism.
So 0x00 or 0xFF means that the device is not responding on the bus.
I'll add a comment to explain why the driver checks these specific values.
>
>> >> + return dev_err_probe(dev, -ENODEV,
>> >> + "Invalid 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.
>>
>> Thanks for the tip, clear improvement.
>> >
>> >> +
>> >> + 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.
>> Yes.
>
>For both cases actually you can leave a comment, but rewrite it in a way that
>it refers to the datasheet. This will be useful.
Ok, I'll do it when the delay is actually documented.
Unfortunately some of them are missing from the released doc.
>
>> >> + return 0;
>> >> +}
>
>...
>
>> >> + /* IMU start-up time. */
>> >> + fsleep(100000);
>> >
>> >100 * USEC_PER_MSEC
>> Yes.
>
>As per above.
Ok.
>
>...
>
>> >> + 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.
>>
>> It's coming in later patch.
>> I thought it would better follow coding guidelines this way.
>> But let me know if it is not the case.
>
>Ah, yes, but weren't {} missing?
OK, I'll add them.
>
>> >> + return ret;
>
>--
>With Best Regards,
>Andy Shevchenko
>
>
>
Powered by blists - more mailing lists