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

[Jonathan Cameron]

On Tue, 2 Jun 2020 07:56:49 +0000
Jean-Baptiste Maneyrol <JManeyrol@xxxxxxxxxxxxxx> wrote:

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

It's in my testing branch now..

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

Indeed I missed that function for some reason.

It's fine as is.

Jonathan

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