Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
To:     Jonathan Cameron <jic23@kernel.org>
CC:     "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "robh@kernel.org" <robh@kernel.org>,
        "mchehab+huawei@kernel.org" <mchehab+huawei@kernel.org>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@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
Thread-Topic: [PATCH v2 01/12] iio: imu: inv_icm42600: add core of new
 inv_icm42600 driver
Thread-Index: AQHWNFiy1AUblJwCrUiUeaBFQvIyCqjCFjyAgALmB2A=
Date:   Tue, 2 Jun 2020 07:56:49 +0000
Message-ID: <SA0PR12MB442938879EF3BDB4B813EC59C48B0@SA0PR12MB4429.namprd12.prod.outlook.com>
References: <20200527185711.21331-1-jmaneyrol@invensense.com>
        <20200527185711.21331-2-jmaneyrol@invensense.com>,<20200531123437.05b3df36@archlinux>
In-Reply-To: <20200531123437.05b3df36@archlinux>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: fec10d23-6153-4050-5b27-08d806ca817b
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Jun 2020 07:56:49.7056
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 462b3b3b-e42b-47ea-801a-f1581aac892d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: c4H0Cf/es08SBCO2anPSkwHKRsWVpid5+6Vzja8oOOBmKxHMyMlnJO9xT5kDt01eQBfstnYA4L/IqakKek3rvhIH1O+WcwH3+jjPfGWoa9c=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR12MB4592
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.216,18.0.687
 definitions=2020-06-02_08:2020-06-01,2020-06-02 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 bulkscore=0 clxscore=1015
 mlxlogscore=999 phishscore=0 adultscore=0 malwarescore=0
 cotscore=-2147483648 suspectscore=0 mlxscore=0 priorityscore=1501
 lowpriorityscore=0 impostorscore=0 spamscore=0 classifier=spam adjust=0
 reason=mlx scancount=1 engine=8.12.0-2004280000
 definitions=main-2006020050
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Jonathan,=0A=
=0A=
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.=0A=
=0A=
For vddio regulator you are missing something. In all suspend callbacks (sy=
stem 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 t=
o wait a little for the supply ramp.=0A=
=0A=
Indeed this doesn't look symmetric, but I was not very happy to add a inv_i=
cm42600_disable_regulator_vddio() that would just do regulator_disable, or =
copy/paste the sleeping value in all resume handlers.=0A=
=0A=
Tell me what you prefer.=0A=
=0A=
Thanks,=0A=
JB=0A=
=0A=
From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> on =
behalf of Jonathan Cameron <jic23@kernel.org>=0A=
Sent: Sunday, May 31, 2020 13:34=0A=
To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>=0A=
Cc: robh+dt@kernel.org <robh+dt@kernel.org>; robh@kernel.org <robh@kernel.o=
rg>; mchehab+huawei@kernel.org <mchehab+huawei@kernel.org>; davem@davemloft=
.net <davem@davemloft.net>; gregkh@linuxfoundation.org <gregkh@linuxfoundat=
ion.org>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; devicetree=
@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org=
 <linux-kernel@vger.kernel.org>=0A=
Subject: Re: [PATCH v2 01/12] iio: imu: inv_icm42600: add core of new inv_i=
cm42600 driver =0A=
=A0=0A=
=A0CAUTION: 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 ope=
n attachments unless you recognize the sender and know the content is safe.=
=0A=
=0A=
On Wed, 27 May 2020 20:57:00 +0200=0A=
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:=0A=
=0A=
> Core component of a new driver for InvenSense ICM-426xx devices.=0A=
> It includes registers definition, main probe/setup, and device=0A=
> utility functions.=0A=
> =0A=
> ICM-426xx devices are latest generation of 6-axis IMU,=0A=
> gyroscope+accelerometer and temperature sensor. This device=0A=
> includes a 2K FIFO, supports I2C/I3C/SPI, and provides=0A=
> intelligent motion features like pedometer, tilt detection,=0A=
> and tap detection.=0A=
> =0A=
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>=0A=
=0A=
A few things inline.=0A=
=0A=
Either I'm missing something or I'm guessing vddio is not controllable=0A=
on your test board.=0A=
=0A=
> ---=0A=
>=A0 drivers/iio/imu/inv_icm42600/inv_icm42600.h=A0=A0 | 372 ++++++++++=0A=
>=A0 .../iio/imu/inv_icm42600/inv_icm42600_core.c=A0 | 635 ++++++++++++++++=
++=0A=
>=A0 2 files changed, 1007 insertions(+)=0A=
>=A0 create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600.h=0A=
>=A0 create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c=0A=
> =0A=
=0A=
...=0A=
=0A=
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/i=
io/imu/inv_icm42600/inv_icm42600_core.c=0A=
> new file mode 100644=0A=
> index 000000000000..81b171d6782c=0A=
> --- /dev/null=0A=
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c=0A=
=0A=
> +const struct iio_mount_matrix *=0A=
> +inv_icm42600_get_mount_matrix(const struct iio_dev *indio_dev,=0A=
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0 const struct iio_chan_spec *chan)=0A=
> +{=0A=
> +=A0=A0=A0=A0 const struct inv_icm42600_state *st =3D=0A=
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 iio_device_=
get_drvdata((struct iio_dev *)indio_dev);=0A=
=0A=
If you review my patch to the core, I can get that applied and we can drop=
=0A=
the ugly cast from here!=0A=
=0A=
Just waiting for someone to sanity check it.=0A=
> +=0A=
> +=A0=A0=A0=A0 return &st->orientation;=0A=
> +}=0A=
...=0A=
=0A=
> +/* Runtime suspend will turn off sensors that are enabled by iio devices=
. */=0A=
> +static int __maybe_unused inv_icm42600_runtime_suspend(struct device *de=
v)=0A=
> +{=0A=
> +=A0=A0=A0=A0 struct inv_icm42600_state *st =3D dev_get_drvdata(dev);=0A=
> +=A0=A0=A0=A0 int ret;=0A=
> +=0A=
> +=A0=A0=A0=A0 mutex_lock(&st->lock);=0A=
> +=0A=
> +=A0=A0=A0=A0 /* disable all sensors */=0A=
> +=A0=A0=A0=A0 ret =3D inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_=
MODE_OFF,=0A=
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 INV_ICM42600_SENSOR_MODE_OFF, false=
,=0A=
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 NULL);=0A=
> +=A0=A0=A0=A0 if (ret)=0A=
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 goto error_unlock;=0A=
> +=0A=
> +=A0=A0=A0=A0 regulator_disable(st->vddio_supply);=0A=
=0A=
Don't seem to turn this on again in runtime_resume..=0A=
Why?=A0 Definitely needs at least a comment.=0A=
=0A=
> +=0A=
> +error_unlock:=0A=
> +=A0=A0=A0=A0 mutex_unlock(&st->lock);=0A=
> +=A0=A0=A0=A0 return ret;=0A=
> +}=0A=
> +=0A=
> +/* Sensors are enabled by iio devices, no need to turn them back on here=
. */=0A=
> +static int __maybe_unused inv_icm42600_runtime_resume(struct device *dev=
)=0A=
> +{=0A=
> +=A0=A0=A0=A0 struct inv_icm42600_state *st =3D dev_get_drvdata(dev);=0A=
> +=A0=A0=A0=A0 int ret;=0A=
> +=0A=
> +=A0=A0=A0=A0 mutex_lock(&st->lock);=0A=
> +=0A=
> +=A0=A0=A0=A0 ret =3D inv_icm42600_enable_regulator_vddio(st);=0A=
> +=0A=
> +=A0=A0=A0=A0 mutex_unlock(&st->lock);=0A=
> +=A0=A0=A0=A0 return ret;=0A=
> +}=0A=
> +=0A=
> +const struct dev_pm_ops inv_icm42600_pm_ops =3D {=0A=
> +=A0=A0=A0=A0 SET_SYSTEM_SLEEP_PM_OPS(inv_icm42600_suspend, inv_icm42600_=
resume)=0A=
> +=A0=A0=A0=A0 SET_RUNTIME_PM_OPS(inv_icm42600_runtime_suspend,=0A=
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 in=
v_icm42600_runtime_resume, NULL)=0A=
> +};=0A=
> +EXPORT_SYMBOL_GPL(inv_icm42600_pm_ops);=0A=
> +=0A=
> +MODULE_AUTHOR("InvenSense, Inc.");=0A=
> +MODULE_DESCRIPTION("InvenSense ICM-426xx device driver");=0A=
> +MODULE_LICENSE("GPL");=0A=