[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aLmY2mKg_FsPOpsq@smile.fi.intel.com>
Date: Thu, 4 Sep 2025 16:49:14 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
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-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
+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.
> 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.
> >> + /* 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)?
...
> >asm/byteorder.h ?
> Yes.
> Is linux/byteorder/generic.h OK?
No, as I put it.
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.
> >> + }
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists