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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID:
 <FR2PPF4571F02BC69DF6807BAA188B2B3A08C57A@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Tue, 15 Jul 2025 09:11:47 +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: Friday, July 11, 2025 1:56 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 v2 2/8] iio: imu: inv_icm45600: add new inv_icm45600 driver
>On Fri, Jul 11, 2025 at 11:32:48AM +0000, Remi Buisson wrote:
>> >From: Andy Shevchenko andriy.shevchenko@...el.com<mailto:andriy.shevchenko@...el.com>
>> >Sent: Thursday, July 10, 2025 11:30 AM
>> >To: Remi Buisson Remi.Buisson@....com<mailto:Remi.Buisson@....com>
>> >On Thu, Jul 10, 2025 at 08:57:57AM +0000, Remi Buisson via B4 Relay wrote:
>
>...
>
>> >> +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.
>
>I see, but can't it be as simple as "big number" with the proper type instead?
>
>#define ..._KEEP_VALUE		U8_MAX // or whatever type
Yes, I can surely fix it this way.
I agree it will be clearer.
>
>> >> +};
>
>...
>
>> >> +#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.
>
>Yes, unused in this code. You should compile the series incrementally,
>so each patch will get a compilation test. This is called compile-time
>bisectability. Also run the system each time to confirm no regressions
>(this is called run-time bisectability).
Yes I did that for each patch, everything build successfully.
In that case, nothing is broken due to this early definition of the macro.
But I'll definitely move it to later patch for clarity. 
>
>...
>
>> >> +                      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?
>
>Not everywhere, but in general yes, if in doubt, use it (but the author is
>the one who knows their the code and answer to this question).
Ok thanks!
>
>...
>
>> It's probably safer to keep the delay even in case of failure to make sure
>> the device is ready before next operation.
>
>I am not sure about it. Why? This has to be well justified as it's quite
>unusual pattern.
Ok I understand, the hardware needs that delay if the access was actually done on the bus (to not jeopardize next access).
If a regmap error means that no real access occured then the delay is avoidable.
>
>-- 
>With Best Regards,
>Andy Shevchenko
>
>
>