[PATCH V11 3/9] iio: imu: inv_icm42607: Add inv_icm42607 Core Driver
Chris Morgan
macromorgan at hotmail.com
Thu Jun 11 07:28:55 PDT 2026
On Thu, Jun 11, 2026 at 10:35:49AM +0300, Andy Shevchenko wrote:
> On Wed, Jun 10, 2026 at 12:54:47PM -0500, Chris Morgan wrote:
>
> > Add the core component of a new inv_icm42607 driver. This includes
> > a few setup functions and the full register definition in the
> > header file.
>
> ...
>
> > 2 files changed, 531 insertions(+)
> > create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607.h
> > create mode 100644 drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
>
> I'm not sure this is correct split by files. These files are not:
> - being build
> - being mentioned in MAINTAINERS
I can merge patches v3 and v4. Early on they were pretty big but now
that they've been paired down some I think it may be manageable.
Early on I was told to try and keep each patch around ~750 lines.
I can add these to together and get it to about ~850 lines, but
it will be compileable at that commit rather than waiting on a
second one.
>
> ...
>
> > +#ifndef INV_ICM42607_H_
> > +#define INV_ICM42607_H_
> > +
> > +#include <linux/bits.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/mutex.h>
>
> mutex_types.h ?
>
I'm not sure I understand this, can you elaborate why I'd need it?
> > +#include <linux/regmap.h>
> > +#include <linux/types.h>
>
> Do you need forward declaration for regulator data type?
>
I think earlier I had regulator-consumer and stripped it out, but I
might have cut a bit too deep so I can add that back.
> ...
>
> > +#define INV_ICM42607_POWER_UP_TIME_US 100000
>
> 100 * USEC_PER_MSEC (will require time.h)?
>
Understood.
> ...
>
> > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
>
> + bitfields.h
>
Understood.
> > +#include <linux/delay.h>
> > +#include <linux/dev_printk.h>
>
> + device/devres.h
> + err.h
>
Understood.
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/time.h>
>
> Also check if you need types.h.
>
I'll add it, I'm honestly not sure. I never got iwyu working it seems.
> ...
>
> > + fsleep(INV_ICM42607_RESET_TIME_MS * USEC_PER_MSEC);
> > +
> > + /*
> > + * No polling interval specified in datasheet, so use reset time as
> > + * polling interval and 10x reset time as timeout period.
> > + */
> > + ret = regmap_read_poll_timeout(st->map, INV_ICM42607_REG_INT_STATUS,
> > + val, val & INV_ICM42607_INT_STATUS_RESET_DONE,
> > + (INV_ICM42607_RESET_TIME_MS * USEC_PER_MSEC),
> > + (INV_ICM42607_RESET_TIME_MS * USEC_PER_MSEC * 10));
>
> Besides too many parentheses, this can be switched to regular patter of "num * what".
> Also, TBH, the plain values would be better here
I'll switch to plain numbers and let the comment above do the explaining.
>
> fsleep(1 * USEC_PER_MSEC);
>
> /*
> * No polling interval specified in datasheet, so use reset time as
> * polling interval and 10x reset time as timeout period.
> */
> ret = regmap_read_poll_timeout(st->map, INV_ICM42607_REG_INT_STATUS,
> val, val & INV_ICM42607_INT_STATUS_RESET_DONE,
> 1 * USEC_PER_MSEC, 10 * USEC_PER_MSEC);
>
> And in the similar way in other fsleep() / _read_poll_timeout() cases.
>
Plain numbers, gotcha.
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "reset error, reset done bit not set\n");
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thank you,
Chris
More information about the Linux-rockchip
mailing list