linux-kernel - RE: [PATCH v5 3/9] iio: imu: inv_icm45600: add buffer support in iio devices

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:
 <FR2PPF4571F02BCEB6C4FA4B3641299A72C8C03A@FR2PPF4571F02BC.DEUP281.PROD.OUTLOOK.COM>
Date: Fri, 5 Sep 2025 12:44:23 +0000
From: Remi Buisson <Remi.Buisson@....com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>,
        Alexandre Belloni
	<alexandre.belloni@...tlin.com>,
        Frank Li <Frank.Li@....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 3/9] iio: imu: inv_icm45600: add buffer support in iio
 devices

>
>
>From: Andy Shevchenko <andriy.shevchenko@...el.com> 
>Sent: Thursday, September 4, 2025 3:49 PM
>To: Remi Buisson <Remi.Buisson@....com>; Alexandre Belloni <alexandre.belloni@...tlin.com>; Frank Li <Frank.Li@....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 3/9] iio: imu: inv_icm45600: add buffer support in iio devices
>
>+Cc I³C people to comment on the returned values on the regmap. See below.
>
>On Thu, Sep 04, 2025 at 01:01:32PM +0000, Remi Buisson wrote:
>> >From: Andy Shevchenko <andriy.shevchenko@...el.com> 
>> >Sent: Thursday, August 21, 2025 11:21 AM
>> >On Wed, Aug 20, 2025 at 02:24:21PM +0000, Remi Buisson via B4 Relay wrote:
>
>...
>
>> >> +#define INV_ICM45600_SENSOR_CONF_KEEP_VALUES {U8_MAX, U8_MAX, U8_MAX, U8_MAX, }
>> >
>> >When one line, no need to have inner trailing comma, besides that missed
>> >space.
>> The trailing comma was a request from Jonathan Cameron on the v2 of patchset.
>> Let me know, if you disagree with him and I'll fix.
>
>I see, then let's ask him, because it's a usual pattern for
>the one-line arrays like this to have no inner trailing commas.
Ok let's wait Jonathan Cameron's feedback.

>
>> And I'll add a space before first element.
>
>...
>
>> >> +	/* Try to read all FIFO data in internal buffer. */
>> >> +	st->fifo.count = fifo_nb * packet_size;
>> >> +	ret = regmap_noinc_read(st->map, INV_ICM45600_REG_FIFO_DATA,
>> >> +				st->fifo.data, st->fifo.count);
>> >> +	if (ret == -ENOTSUPP || ret == -EFBIG) {
>> >
>> >Strictly speaking this is a bit of layering issue, do we have other means to
>> >check the support before even trying?
>> 
>> No unfortunately, we can't with current I3C regmap implementation.
>> I2C and SPI regmaps are able to split transfers according to max_read_len.
>> But for I3C, it is left to the controller driver, which usually only returns an error.
>
>Have it been discussed with I³C maintainers / stakeholders? Because this kind of APIs
>will be hard to follow and even change for both sides caller and callee.
Thanks for opening the discussion.
We implemented the management of error, in a way that (we think) makes sense,
whatever the below layers are doing.

>
>> >> +		/* Read full fifo is not supported, read samples one by one. */
>> >> +		ret = 0;
>> >> +		for (i = 0; i < st->fifo.count && ret == 0; i += packet_size)
>> >> +			ret = regmap_noinc_read(st->map, INV_ICM45600_REG_FIFO_DATA,
>> >> +						&st->fifo.data[i], packet_size);
>> >> +	}
>> >> +	if (ret)
>> >> +		return ret;
>
>...
>
>> >> +	/* Disable FIFO and set depth. */
>> >> +	val = FIELD_PREP(INV_ICM45600_FIFO_CONFIG0_MODE_MASK,
>> >> +			 INV_ICM45600_FIFO_CONFIG0_MODE_BYPASS);
>> >> +	val |= INV_ICM45600_FIFO_CONFIG0_FIFO_DEPTH_MAX;
>> >
>> >FIELD_MODIFY()
>> Ok, great.
>
>Actually this is not a modification per se, it's just an assignment (PREP)
>split to two lines, can you just make it a single expression (wrapped on a few
>lines, though)?
>
You mean droping the FIELD_MODIFY suggestion and doing something like:
val = FIELD_PREP(INV_ICM45600_FIFO_CONFIG0_MODE_MASK,
		 INV_ICM45600_FIFO_CONFIG0_MODE_BYPASS) |
         FIELD_PREP(INV_ICM45600_FIFO_CONFIG0_FIFO_DEPTH_MASK,
		 INV_ICM45600_FIFO_CONFIG0_FIFO_DEPTH_MAX);
>...
>
>> >asm/byteorder.h ?
>> Yes.
>> Is linux/byteorder/generic.h OK?
>
>No, as I put it.
Ok, clear :)

>
>linux/*
>...blank line...
>asm/*
>...blank line...
>linux/iio/*
>...blank line...
>
>...
>
>> >> -	scoped_guard(mutex, &st->lock)
>> >> +	scoped_guard(mutex, &st->lock) {
>> >
>> >Ah, nice. It should have been done in the first place and put a comment to that
>> >patch that scoped_guard() {} used specifically for limiting churn in the next
>> >changes.
>> If ok for you, I'll keep that as it is.
>> If I add a comment in previous patch, I'll anyway have to delete it this patch.
>
>"Comment" is to be added to the email and not the code. It's a free words to
>the cover letter and/or to this email after '---' line but before the actual
>diff.
>
>But {} should be added as even in the first patch this is multi-line body.
Okay
>
>> >> +	}
>
>-- 
>With Best Regards,
>Andy Shevchenko
>
>
>