From: Lorenzo Bianconi <lorenzo@kernel.org>
To: Martin Kepplinger <martin.kepplinger@puri.sm>
Cc: lorenzo.bianconi83@gmail.com, jic23@kernel.org, knaack.h@gmx.de,
lars@metafoo.de, pmeerw@pmeerw.net, robh+dt@kernel.org,
mark.rutland@arm.com, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org
Subject: Re: [PATCH v4 2/3] iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9sd1
Date: Mon, 19 Aug 2019 11:48:45 +0200 [thread overview]
Message-ID: <20190819094845.GB17835@localhost.localdomain> (raw)
In-Reply-To: <20190813073533.8007-3-martin.kepplinger@puri.sm>
[-- Attachment #1: Type: text/plain, Size: 8646 bytes --]
> The LSM9DS1's accelerometer / gyroscope unit and it's magnetometer (separately
> supported in iio/magnetometer/st_magn*) are located on a separate i2c addresses
> on the bus.
>
> For the datasheet, see https://www.st.com/resource/en/datasheet/lsm9ds1.pdf
>
> Treat it just like the LSM6* devices and, despite it's name, hook it up
> to the st_lsm6dsx driver, using it's basic functionality.
>
> accelerometer and gyroscope are not independently clocked. It runs at the gyro
> frequencies if both are enabled, see chapter 7.12 of the datasheet.
> We could have handled this as a single IIO device but we have split
> it up to be more consistent with the other more flexible devices.
>
> Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
Hi Martin,
most of comments are nitpicks (inline), the only issue I can see here is we can enable
hw fifo for lsm6ds0/lsm9ds1 and read_fifo routine pointer is not currently
initialized so we will end up with a NULL pointer dereference. Since we will
need a different update FIFO routine for lsm6ds0/lsm9ds1 I am adding an
update_fifo function pointer in fifo_ops in order to fix this issue.
Regards,
Lorenzo
> ---
> drivers/iio/imu/st_lsm6dsx/Kconfig | 2 +-
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 2 +
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 87 ++++++++++++++++++++
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c | 5 ++
> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c | 5 ++
> 5 files changed, 100 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> index 939058b27746..77aa0e77212d 100644
> --- a/drivers/iio/imu/st_lsm6dsx/Kconfig
> +++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
> @@ -12,7 +12,7 @@ config IIO_ST_LSM6DSX
> Say yes here to build support for STMicroelectronics LSM6DSx imu
> sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,
> ism330dlc, lsm6dso, lsm6dsox, asm330lhh, lsm6dsr, lsm6ds3tr-c,
> - ism330dhcx
> + ism330dhcx and the accelerometer/gyroscope of lsm9ds1.
>
> To compile this driver as a module, choose M here: the module
> will be called st_lsm6dsx.
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index c8f333902eb7..d03b5a2d8549 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -24,6 +24,7 @@
> #define ST_LSM6DSR_DEV_NAME "lsm6dsr"
> #define ST_LSM6DS3TRC_DEV_NAME "lsm6ds3tr-c"
> #define ST_ISM330DHCX_DEV_NAME "ism330dhcx"
> +#define ST_LSM9DS1_DEV_NAME "lsm9ds1"
should be called 'lsm9ds1_imu' since lsm9ds1 is a 9-axis device? what do you
think?
>
> enum st_lsm6dsx_hw_id {
> ST_LSM6DS3_ID,
> @@ -37,6 +38,7 @@ enum st_lsm6dsx_hw_id {
> ST_LSM6DSR_ID,
> ST_LSM6DS3TRC_ID,
> ST_ISM330DHCX_ID,
> + ST_LSM9DS1_ID,
same here..ST_LSM9DS1_IMU_ID
> ST_LSM6DSX_MAX_ID,
> };
>
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 56e1c5262a2c..f038bb06f635 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -10,6 +10,8 @@
> * +-125/+-245/+-500/+-1000/+-2000 dps
> * LSM6DSx series has an integrated First-In-First-Out (FIFO) buffer
> * allowing dynamic batching of sensor data.
> + * LSM9DSx series is similar but includes an additional magnetometer, handled
> + * by a different driver.
> *
> * Supported sensors:
> * - LSM6DS3:
> @@ -30,6 +32,13 @@
> * - Gyroscope supported full-scale [dps]: +-125/+-245/+-500/+-1000/+-2000
> * - FIFO size: 3KB
> *
> + * - LSM9DS1:
> + * - Accelerometer supported ODR [Hz]: 10, 50, 119, 238, 476, 952
> + * - Accelerometer supported full-scale [g]: +-2/+-4/+-8/+-16
> + * - Gyroscope supported ODR [Hz]: 15, 60, 119, 238, 476, 952
> + * - Gyroscope supported full-scale [dps]: +-245/+-500/+-2000
> + * - FIFO size: 32
> + *
> * Copyright 2016 STMicroelectronics Inc.
> *
> * Lorenzo Bianconi <lorenzo.bianconi@st.com>
> @@ -70,7 +79,85 @@ static const struct iio_chan_spec st_lsm6dsx_gyro_channels[] = {
> IIO_CHAN_SOFT_TIMESTAMP(3),
> };
>
> +static const struct iio_chan_spec st_lsm9dsx_gyro_channels[] = {
> + ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x18, IIO_MOD_X, 0),
> + ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x1a, IIO_MOD_Y, 1),
> + ST_LSM6DSX_CHANNEL(IIO_ANGL_VEL, 0x1c, IIO_MOD_Z, 2),
> + IIO_CHAN_SOFT_TIMESTAMP(3),
> +};
> +
why not st_lsm6ds0_gyro_channels?
> static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> + {
> + .wai = 0x68,
> + .int1_addr = 0x0c,
> + .int2_addr = 0x0d,
> + .reset_addr = 0x22,
> + .max_fifo_size = 32,
> + .id = {
> + {
> + .hw_id = ST_LSM9DS1_ID,
> + .name = ST_LSM9DS1_DEV_NAME,
> + },
> + },
> + .channels = {
> + [ST_LSM6DSX_ID_ACC] = {
> + .chan = st_lsm6dsx_acc_channels,
> + .len = ARRAY_SIZE(st_lsm6dsx_acc_channels),
> + },
> + [ST_LSM6DSX_ID_GYRO] = {
> + .chan = st_lsm9dsx_gyro_channels,
> + .len = ARRAY_SIZE(st_lsm9dsx_gyro_channels),
> + },
> + },
> + .odr_table = {
> + [ST_LSM6DSX_ID_ACC] = {
> + .reg = {
> + .addr = 0x20,
> + .mask = GENMASK(7, 5),
> + },
> + .odr_avl[0] = { 10, 0x01 },
> + .odr_avl[1] = { 50, 0x02 },
> + .odr_avl[2] = { 119, 0x03 },
> + .odr_avl[3] = { 238, 0x04 },
> + .odr_avl[4] = { 476, 0x05 },
> + .odr_avl[5] = { 952, 0x06 },
> + },
> + [ST_LSM6DSX_ID_GYRO] = {
> + .reg = {
> + .addr = 0x10,
> + .mask = GENMASK(7, 5),
> + },
> + .odr_avl[0] = { 15, 0x01 },
> + .odr_avl[1] = { 60, 0x02 },
> + .odr_avl[2] = { 119, 0x03 },
> + .odr_avl[3] = { 238, 0x04 },
> + .odr_avl[4] = { 476, 0x05 },
> + .odr_avl[5] = { 952, 0x06 },
> + },
> + },
> + .fs_table = {
> + [ST_LSM6DSX_ID_ACC] = {
> + .reg = {
> + .addr = 0x20,
> + .mask = GENMASK(4, 3),
> + },
> + .fs_avl[0] = { 599, 0x0 },
> + .fs_avl[1] = { 1197, 0x2 },
> + .fs_avl[2] = { 2394, 0x3 },
> + .fs_avl[3] = { 4788, 0x1 },
> + },
> + [ST_LSM6DSX_ID_GYRO] = {
> + .reg = {
> + .addr = 0x10,
> + .mask = GENMASK(4, 3),
> + },
> + .fs_avl[0] = { IIO_DEGREE_TO_RAD(245), 0x0 },
> + .fs_avl[1] = { IIO_DEGREE_TO_RAD(500), 0x1 },
> + .fs_avl[2] = { IIO_DEGREE_TO_RAD(0), 0x2 },
> + .fs_avl[3] = { IIO_DEGREE_TO_RAD(2000), 0x3 },
> + },
> + },
> + },
> {
> .wai = 0x69,
> .int1_addr = 0x0d,
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
> index 15c6aa5b6caa..2f1b30ff083b 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
> @@ -83,6 +83,10 @@ static const struct of_device_id st_lsm6dsx_i2c_of_match[] = {
> .compatible = "st,ism330dhcx",
> .data = (void *)ST_ISM330DHCX_ID,
> },
> + {
> + .compatible = "st,lsm9ds1",
same here, what is the right compatible string? "st,lsm9ds1 or
"st,lsm9ds1_imu"?
> + .data = (void *)ST_LSM9DS1_ID,
> + },
> {},
> };
> MODULE_DEVICE_TABLE(of, st_lsm6dsx_i2c_of_match);
> @@ -99,6 +103,7 @@ static const struct i2c_device_id st_lsm6dsx_i2c_id_table[] = {
> { ST_LSM6DSR_DEV_NAME, ST_LSM6DSR_ID },
> { ST_LSM6DS3TRC_DEV_NAME, ST_LSM6DS3TRC_ID },
> { ST_ISM330DHCX_DEV_NAME, ST_ISM330DHCX_ID },
> + { ST_LSM9DS1_DEV_NAME, ST_LSM9DS1_ID },
> {},
> };
> MODULE_DEVICE_TABLE(i2c, st_lsm6dsx_i2c_id_table);
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
> index a8430ee11310..421ce704f346 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
> @@ -83,6 +83,10 @@ static const struct of_device_id st_lsm6dsx_spi_of_match[] = {
> .compatible = "st,ism330dhcx",
> .data = (void *)ST_ISM330DHCX_ID,
> },
> + {
> + .compatible = "st,lsm9ds1",
> + .data = (void *)ST_LSM9DS1_ID,
> + },
> {},
> };
> MODULE_DEVICE_TABLE(of, st_lsm6dsx_spi_of_match);
> @@ -99,6 +103,7 @@ static const struct spi_device_id st_lsm6dsx_spi_id_table[] = {
> { ST_LSM6DSR_DEV_NAME, ST_LSM6DSR_ID },
> { ST_LSM6DS3TRC_DEV_NAME, ST_LSM6DS3TRC_ID },
> { ST_ISM330DHCX_DEV_NAME, ST_ISM330DHCX_ID },
> + { ST_LSM9DS1_DEV_NAME, ST_LSM9DS1_ID },
> {},
> };
> MODULE_DEVICE_TABLE(spi, st_lsm6dsx_spi_id_table);
> --
> 2.20.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2019-08-19 9:48 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-13 7:35 [PATCH v4 0/3] iio: imu: st_lsm6dsx: Add support for LSM9DS1 Martin Kepplinger
2019-08-13 7:35 ` [PATCH v4 1/3] iio: imu: st_lsm6sdx: move register definitions to sensor_settings struct Martin Kepplinger
2019-08-19 8:47 ` Lorenzo Bianconi
2019-08-13 7:35 ` [PATCH v4 2/3] iio: imu: st_lsm6dsx: add support for accel/gyro unit of lsm9sd1 Martin Kepplinger
2019-08-19 9:48 ` Lorenzo Bianconi [this message]
2019-08-20 15:06 ` Martin Kepplinger
2019-08-21 9:08 ` Lorenzo Bianconi
2019-08-25 17:34 ` Jonathan Cameron
2019-08-13 7:35 ` [PATCH v4 3/3] dt-bindings: iio: imu: st_lsm6dsx: add lsm9ds1 device bindings Martin Kepplinger
2019-08-18 19:10 ` [PATCH v4 0/3] iio: imu: st_lsm6dsx: Add support for LSM9DS1 Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190819094845.GB17835@localhost.localdomain \
--to=lorenzo@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=lorenzo.bianconi83@gmail.com \
--cc=mark.rutland@arm.com \
--cc=martin.kepplinger@puri.sm \
--cc=pmeerw@pmeerw.net \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).