On Tue, 2 Jun 2020 07:56:49 +0000
Jean-Baptiste Maneyrol <jmaney...@invensense.com> 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-ow...@vger.kernel.org <linux-iio-ow...@vger.kernel.org> on
> behalf of Jonathan Cameron <ji...@kernel.org>
> Sent: Sunday, May 31, 2020 13:34
> To: Jean-Baptiste Maneyrol <jmaney...@invensense.com>
> Cc: robh...@kernel.org <robh...@kernel.org>; r...@kernel.org
> <r...@kernel.org>; mchehab+hua...@kernel.org <mchehab+hua...@kernel.org>;
> da...@davemloft.net <da...@davemloft.net>; gre...@linuxfoundation.org
> <gre...@linuxfoundation.org>; linux-...@vger.kernel.org
> <linux-...@vger.kernel.org>; devicet...@vger.kernel.org
> <devicet...@vger.kernel.org>; linux-kernel@vger.kernel.org
> <linux-kernel@vger.kernel.org>
> 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 <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");
