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

[Jean-Baptiste Maneyrol]

Hi Jonathan,

I've given my review tag for the const change of iio_device_get_drvdata(). Would be perfect to have this cleaned up for the v3.

For vddio regulator you are missing something. In all suspend callbacks (system and runtime) I am calling directly regulator_disable to shut vddio off at then end. And in all resume callbacks I am calling inv_icm42600_enable_regulator_vddio() that is turning vddio regulator back on and is sleeping to wait a little for the supply ramp.

Indeed this doesn't look symmetric, but I was not very happy to add a inv_icm42600_disable_regulator_vddio() that would just do regulator_disable, or copy/paste the sleeping value in all resume handlers.

Tell me what you prefer.

Thanks,
JB

From: linux-iio-owner@xxxxxxxxxxxxxxx <linux-iio-owner@xxxxxxxxxxxxxxx> on behalf of Jonathan Cameron <jic23@xxxxxxxxxx>
Sent: Sunday, May 31, 2020 13:34
To: Jean-Baptiste Maneyrol <JManeyrol@xxxxxxxxxxxxxx>
Cc: robh+dt@xxxxxxxxxx <robh+dt@xxxxxxxxxx>; robh@xxxxxxxxxx <robh@xxxxxxxxxx>; mchehab+huawei@xxxxxxxxxx <mchehab+huawei@xxxxxxxxxx>; davem@xxxxxxxxxxxxx <davem@xxxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx <linux-iio@xxxxxxxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx <devicetree@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>
Subject: Re: [PATCH v2 01/12] iio: imu: inv_icm42600: add core of new inv_icm42600 driver 
 
 CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.

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