Re: [PATCH v2 01/12] iio: imu: inv_icm42600: add core of new inv_icm42600 driver

[Jonathan Cameron]

On Wed, 27 May 2020 20:57:00 +0200
Jean-Baptiste Maneyrol <jmaneyrol@xxxxxxxxxxxxxx> wrote:

> Core component of a new driver for InvenSense ICM-426xx devices.
> It includes registers definition, main probe/setup, and device
> utility functions.
> 
> ICM-426xx devices are latest generation of 6-axis IMU,
> gyroscope+accelerometer and temperature sensor. This device
> includes a 2K FIFO, supports I2C/I3C/SPI, and provides
> intelligent motion features like pedometer, tilt detection,
> and tap detection.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@xxxxxxxxxxxxxx>

A few things inline.

Either I'm missing something or I'm guessing vddio is not controllable
on your test board.

> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600.h   | 372 ++++++++++
>  .../iio/imu/inv_icm42600/inv_icm42600_core.c  | 635 ++++++++++++++++++
>  2 files changed, 1007 insertions(+)
>  create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600.h
>  create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> 

...

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> new file mode 100644
> index 000000000000..81b171d6782c
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c

> +const struct iio_mount_matrix *
> +inv_icm42600_get_mount_matrix(const struct iio_dev *indio_dev,
> +			      const struct iio_chan_spec *chan)
> +{
> +	const struct inv_icm42600_state *st =
> +			iio_device_get_drvdata((struct iio_dev *)indio_dev);

If you review my patch to the core, I can get that applied and we can drop
the ugly cast from here!

Just waiting for someone to sanity check it.
> +
> +	return &st->orientation;
> +}
...

> +/* Runtime suspend will turn off sensors that are enabled by iio devices. */
> +static int __maybe_unused inv_icm42600_runtime_suspend(struct device *dev)
> +{
> +	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	/* disable all sensors */
> +	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> +					 INV_ICM42600_SENSOR_MODE_OFF, false,
> +					 NULL);
> +	if (ret)
> +		goto error_unlock;
> +
> +	regulator_disable(st->vddio_supply);

Don't seem to turn this on again in runtime_resume..
Why?  Definitely needs at least a comment.

> +
> +error_unlock:
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}
> +
> +/* Sensors are enabled by iio devices, no need to turn them back on here. */
> +static int __maybe_unused inv_icm42600_runtime_resume(struct device *dev)
> +{
> +	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +
> +	ret = inv_icm42600_enable_regulator_vddio(st);
> +
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}
> +
> +const struct dev_pm_ops inv_icm42600_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(inv_icm42600_suspend, inv_icm42600_resume)
> +	SET_RUNTIME_PM_OPS(inv_icm42600_runtime_suspend,
> +			   inv_icm42600_runtime_resume, NULL)
> +};
> +EXPORT_SYMBOL_GPL(inv_icm42600_pm_ops);
> +
> +MODULE_AUTHOR("InvenSense, Inc.");
> +MODULE_DESCRIPTION("InvenSense ICM-426xx device driver");
> +MODULE_LICENSE("GPL");