Re: [PATCH 04/12] iio: imu: inv_icm42600: add gyroscope IIO device
On Mon, 18 May 2020 15:27:28 +
Jean-Baptiste Maneyrol wrote:
> Hi Jonathan,
>
> I agree with all comments.
>
> For regmap_bulk_read, by looking at source code it doesn't seem to requires
> specific alignment, except if bus read callback is expecting that. But I can
> see numerous drivers calling regmap_bulk_read with a data buffer on the stack
> and not particularly aligned.
Absolutely. e.g. i2c you don't need to be aligned for
but for SPI you do. If there are drivers
using regmap for spi that don't there is probably
a bug there.
Jonathan
>
> And we definitely can read calibration offset registers while running, the
> lock is indeed not needed.
>
> Thanks,
> JB
>
> From: Jonathan Cameron
> Sent: Friday, May 8, 2020 16:01
> To: Jean-Baptiste Maneyrol
> Cc: ji...@kernel.org ; robh...@kernel.org
> ; r...@kernel.org ;
> mchehab+hua...@kernel.org ; da...@davemloft.net
> ; gre...@linuxfoundation.org
> ; linux-...@vger.kernel.org
> ; devicet...@vger.kernel.org
> ; linux-kernel@vger.kernel.org
>
> Subject: Re: [PATCH 04/12] iio: imu: inv_icm42600: add gyroscope IIO device
>
> 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 Thu, 7 May 2020 16:42:14 +0200
> Jean-Baptiste Maneyrol wrote:
>
> > Add IIO device for gyroscope sensor with data polling interface.
> > Attributes: raw, scale, sampling_frequency, calibbias.
> >
> > Gyroscope in low noise mode.
> >
> > Signed-off-by: Jean-Baptiste Maneyrol
> Few trivial things and questions inline.
>
> J
>
> > ---
> > drivers/iio/imu/inv_icm42600/inv_icm42600.h | 4 +
> > .../iio/imu/inv_icm42600/inv_icm42600_core.c | 5 +
> > .../iio/imu/inv_icm42600/inv_icm42600_gyro.c | 549 ++
> > 3 files changed, 558 insertions(+)
> > create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> >
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > index 8da4c8249aed..ca41a9d6404a 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > @@ -120,6 +120,7 @@ struct inv_icm42600_suspended {
> > * @orientation:sensor chip orientation relative to main hardware.
> > * @conf: chip sensors configurations.
> > * @suspended: suspended sensors configuration.
> > + * @indio_gyro: gyroscope IIO device.
> > */
> > struct inv_icm42600_state {
> >struct mutex lock;
> > @@ -131,6 +132,7 @@ struct inv_icm42600_state {
> >struct iio_mount_matrix orientation;
> >struct inv_icm42600_conf conf;
> >struct inv_icm42600_suspended suspended;
> > + struct iio_dev *indio_gyro;
> > };
> >
> > /* Virtual register addresses: @bank on MSB (4 upper bits), @address on
> > LSB */
> > @@ -369,4 +371,6 @@ int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev,
> > unsigned int reg,
> > int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> >inv_icm42600_bus_setup bus_setup);
> >
> > +int inv_icm42600_gyro_init(struct inv_icm42600_state *st);
> > +
> > #endif
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > index 35bdf4f9d31e..151257652ce6 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > @@ -503,6 +503,11 @@ int inv_icm42600_core_probe(struct regmap *regmap, int
> > chip,
> >if (ret)
> >return ret;
> >
> > + /* create and init gyroscope iio device */
>
> 'Kind' of obvious from function name? Maybe drop the comment?
>
> > + ret = inv_icm42600_gyro_init(st);
> > + if (ret)
> > + return ret;
> > +
> >/* setup runtime power management */
> >ret = pm_runtime_set_active(dev);
> >if (ret)
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> > b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> > new file mode 100644
> > index ..74aa2b5fa611
> > --- /dev/null
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> > @@ -0,0 +1,549 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
>
Re: [PATCH 04/12] iio: imu: inv_icm42600: add gyroscope IIO device
Hi Jonathan,
I agree with all comments.
For regmap_bulk_read, by looking at source code it doesn't seem to requires
specific alignment, except if bus read callback is expecting that. But I can
see numerous drivers calling regmap_bulk_read with a data buffer on the stack
and not particularly aligned.
And we definitely can read calibration offset registers while running, the lock
is indeed not needed.
Thanks,
JB
From: Jonathan Cameron
Sent: Friday, May 8, 2020 16:01
To: Jean-Baptiste Maneyrol
Cc: ji...@kernel.org ; robh...@kernel.org
; r...@kernel.org ;
mchehab+hua...@kernel.org ; da...@davemloft.net
; gre...@linuxfoundation.org ;
linux-...@vger.kernel.org ;
devicet...@vger.kernel.org ;
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 04/12] iio: imu: inv_icm42600: add gyroscope IIO device
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 Thu, 7 May 2020 16:42:14 +0200
Jean-Baptiste Maneyrol wrote:
> Add IIO device for gyroscope sensor with data polling interface.
> Attributes: raw, scale, sampling_frequency, calibbias.
>
> Gyroscope in low noise mode.
>
> Signed-off-by: Jean-Baptiste Maneyrol
Few trivial things and questions inline.
J
> ---
> drivers/iio/imu/inv_icm42600/inv_icm42600.h | 4 +
> .../iio/imu/inv_icm42600/inv_icm42600_core.c | 5 +
> .../iio/imu/inv_icm42600/inv_icm42600_gyro.c | 549 ++
> 3 files changed, 558 insertions(+)
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 8da4c8249aed..ca41a9d6404a 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -120,6 +120,7 @@ struct inv_icm42600_suspended {
> * @orientation: sensor chip orientation relative to main hardware.
> * @conf: chip sensors configurations.
> * @suspended: suspended sensors configuration.
> + * @indio_gyro: gyroscope IIO device.
> */
> struct inv_icm42600_state {
> struct mutex lock;
> @@ -131,6 +132,7 @@ struct inv_icm42600_state {
> struct iio_mount_matrix orientation;
> struct inv_icm42600_conf conf;
> struct inv_icm42600_suspended suspended;
> + struct iio_dev *indio_gyro;
> };
>
> /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB
>*/
> @@ -369,4 +371,6 @@ int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev,
> unsigned int reg,
> int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> inv_icm42600_bus_setup bus_setup);
>
> +int inv_icm42600_gyro_init(struct inv_icm42600_state *st);
> +
> #endif
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 35bdf4f9d31e..151257652ce6 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -503,6 +503,11 @@ int inv_icm42600_core_probe(struct regmap *regmap, int
> chip,
> if (ret)
> return ret;
>
> + /* create and init gyroscope iio device */
'Kind' of obvious from function name? Maybe drop the comment?
> + ret = inv_icm42600_gyro_init(st);
> + if (ret)
> + return ret;
> +
> /* setup runtime power management */
> ret = pm_runtime_set_active(dev);
> if (ret)
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> new file mode 100644
> index ..74aa2b5fa611
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> @@ -0,0 +1,549 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Invensense, Inc.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "inv_icm42600.h"
> +
> +#define INV_ICM42600_GYRO_CHAN(_modifier, _index, _ext_info) \
> + { \
> + .type = IIO_ANGL_VEL, \
> + .modified = 1, \
> + .channel2 = _modifier, \
> + .info_mask_separate =
Re: [PATCH 04/12] iio: imu: inv_icm42600: add gyroscope IIO device
On Thu, 7 May 2020 16:42:14 +0200
Jean-Baptiste Maneyrol wrote:
> Add IIO device for gyroscope sensor with data polling interface.
> Attributes: raw, scale, sampling_frequency, calibbias.
>
> Gyroscope in low noise mode.
>
> Signed-off-by: Jean-Baptiste Maneyrol
Few trivial things and questions inline.
J
> ---
> drivers/iio/imu/inv_icm42600/inv_icm42600.h | 4 +
> .../iio/imu/inv_icm42600/inv_icm42600_core.c | 5 +
> .../iio/imu/inv_icm42600/inv_icm42600_gyro.c | 549 ++
> 3 files changed, 558 insertions(+)
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 8da4c8249aed..ca41a9d6404a 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -120,6 +120,7 @@ struct inv_icm42600_suspended {
> * @orientation:sensor chip orientation relative to main hardware.
> * @conf: chip sensors configurations.
> * @suspended: suspended sensors configuration.
> + * @indio_gyro: gyroscope IIO device.
> */
> struct inv_icm42600_state {
> struct mutex lock;
> @@ -131,6 +132,7 @@ struct inv_icm42600_state {
> struct iio_mount_matrix orientation;
> struct inv_icm42600_conf conf;
> struct inv_icm42600_suspended suspended;
> + struct iio_dev *indio_gyro;
> };
>
> /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB
> */
> @@ -369,4 +371,6 @@ int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev,
> unsigned int reg,
> int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> inv_icm42600_bus_setup bus_setup);
>
> +int inv_icm42600_gyro_init(struct inv_icm42600_state *st);
> +
> #endif
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 35bdf4f9d31e..151257652ce6 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -503,6 +503,11 @@ int inv_icm42600_core_probe(struct regmap *regmap, int
> chip,
> if (ret)
> return ret;
>
> + /* create and init gyroscope iio device */
'Kind' of obvious from function name? Maybe drop the comment?
> + ret = inv_icm42600_gyro_init(st);
> + if (ret)
> + return ret;
> +
> /* setup runtime power management */
> ret = pm_runtime_set_active(dev);
> if (ret)
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> new file mode 100644
> index ..74aa2b5fa611
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> @@ -0,0 +1,549 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Invensense, Inc.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "inv_icm42600.h"
> +
> +#define INV_ICM42600_GYRO_CHAN(_modifier, _index, _ext_info) \
> + { \
> + .type = IIO_ANGL_VEL, \
> + .modified = 1, \
> + .channel2 = _modifier, \
> + .info_mask_separate = \
> + BIT(IIO_CHAN_INFO_RAW) |\
> + BIT(IIO_CHAN_INFO_CALIBBIAS), \
> + .info_mask_shared_by_type = \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_all = \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .info_mask_shared_by_all_available =\
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .scan_index = _index, \
> + .scan_type = { \
> + .sign = 's',\
> + .realbits = 16, \
> + .storagebits = 16, \
> + .shift = 0, \
Shift has the 'obviously' default of 0, so normally we don't bother explicitly
setting it to 0 like this.
> + .endianness = IIO_BE, \
> + }, \
> + .ext_info =
[PATCH 04/12] iio: imu: inv_icm42600: add gyroscope IIO device
Add IIO device for gyroscope sensor with data polling interface.
Attributes: raw, scale, sampling_frequency, calibbias.
Gyroscope in low noise mode.
Signed-off-by: Jean-Baptiste Maneyrol
---
drivers/iio/imu/inv_icm42600/inv_icm42600.h | 4 +
.../iio/imu/inv_icm42600/inv_icm42600_core.c | 5 +
.../iio/imu/inv_icm42600/inv_icm42600_gyro.c | 549 ++
3 files changed, 558 insertions(+)
create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index 8da4c8249aed..ca41a9d6404a 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
@@ -120,6 +120,7 @@ struct inv_icm42600_suspended {
* @orientation: sensor chip orientation relative to main hardware.
* @conf: chip sensors configurations.
* @suspended:suspended sensors configuration.
+ * @indio_gyro: gyroscope IIO device.
*/
struct inv_icm42600_state {
struct mutex lock;
@@ -131,6 +132,7 @@ struct inv_icm42600_state {
struct iio_mount_matrix orientation;
struct inv_icm42600_conf conf;
struct inv_icm42600_suspended suspended;
+ struct iio_dev *indio_gyro;
};
/* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
@@ -369,4 +371,6 @@ int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev,
unsigned int reg,
int inv_icm42600_core_probe(struct regmap *regmap, int chip,
inv_icm42600_bus_setup bus_setup);
+int inv_icm42600_gyro_init(struct inv_icm42600_state *st);
+
#endif
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index 35bdf4f9d31e..151257652ce6 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -503,6 +503,11 @@ int inv_icm42600_core_probe(struct regmap *regmap, int
chip,
if (ret)
return ret;
+ /* create and init gyroscope iio device */
+ ret = inv_icm42600_gyro_init(st);
+ if (ret)
+ return ret;
+
/* setup runtime power management */
ret = pm_runtime_set_active(dev);
if (ret)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
new file mode 100644
index ..74aa2b5fa611
--- /dev/null
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
@@ -0,0 +1,549 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2020 Invensense, Inc.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "inv_icm42600.h"
+
+#define INV_ICM42600_GYRO_CHAN(_modifier, _index, _ext_info) \
+ { \
+ .type = IIO_ANGL_VEL, \
+ .modified = 1, \
+ .channel2 = _modifier, \
+ .info_mask_separate = \
+ BIT(IIO_CHAN_INFO_RAW) |\
+ BIT(IIO_CHAN_INFO_CALIBBIAS), \
+ .info_mask_shared_by_type = \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_shared_by_type_available = \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_shared_by_all = \
+ BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+ .info_mask_shared_by_all_available =\
+ BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+ .scan_index = _index, \
+ .scan_type = { \
+ .sign = 's',\
+ .realbits = 16, \
+ .storagebits = 16, \
+ .shift = 0, \
+ .endianness = IIO_BE, \
+ }, \
+ .ext_info = _ext_info, \
+ }
+
+enum inv_icm42600_gyro_scan {
+ INV_ICM42600_GYRO_SCAN_X,
+ INV_ICM42600_GYRO_SCAN_Y,
+ INV_ICM42600_GYRO_SCAN_Z,
+};
+
+static const struct iio_chan_spec_ext_info inv_icm42600_gyro_ext_infos[] = {
+ IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, inv_icm42600_get_mount_matrix),
+ {},
+};
+
+static const struct iio_chan_spec
