On Wed, 27 May 2020 20:57:00 +0200
Jean-Baptiste Maneyrol <jmaney...@invensense.com> 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 <jmaney...@invensense.com>

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");

Reply via email to