linux-kernel - Re: [PATCH v5 2/9] 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: <aLmRWHZ-fNYjeYll@smile.fi.intel.com>
Date: Thu, 4 Sep 2025 16:17:12 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
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-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

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:

...

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

> >> +};

...

> >> +/**
> >> + *  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.

> >> + */

...

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

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

> >> +	return 0;
> >> +}

...

> >> +	/* IMU start-up time. */
> >> +	fsleep(100000);
> >
> >100 * USEC_PER_MSEC
> Yes.

As per above.

...

> >> +	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?

> >> +	return ret;

-- 
With Best Regards,
Andy Shevchenko