Re: [PATCH v5 3/9] iio: imu: inv_icm45600: add buffer support in iio devices

[Andy Shevchenko <andriy.shevchenko@intel.com>]

+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@intel.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