From: Lorenzo Bianconi <lorenzo@kernel.org>
To: Jonathan Cameron <jic23@kernel.org>
Cc: linux-iio@vger.kernel.org, lorenzo.bianconi@redhat.com,
devicetree@vger.kernel.org, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org
Subject: Re: [PATCH 1/2] iio: imu: st_lsm6dsx: add support to ASM330LHHXG1
Date: Sun, 28 Jan 2024 11:38:22 +0100 [thread overview]
Message-ID: <ZbYungPhhzx2xREi@lore-desk> (raw)
In-Reply-To: <20240127150029.1a9b49b2@jic23-huawei>
[-- Attachment #1: Type: text/plain, Size: 9647 bytes --]
> On Wed, 24 Jan 2024 11:52:33 +0100
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> > Add support to STM ASM330LHHXG1 (accelerometer and gyroscope) Mems
> > sensor.
> > The ASM330LHHXG1 sensor can use ASM330LHHX as fallback device since it
> > implements all the ASM330LHHXG1 features currently implemented in
> > st_lsm6dsx.
> >
> > Link: https://www.st.com/resource/en/datasheet/asm330lhhxg1.pdf
> Datasheet:
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
>
> Hi Lorenzo.
>
> A few comments inline. Mostly about make it less noisy to
> add additional devices in future.
>
> > ---
> > drivers/iio/imu/st_lsm6dsx/Kconfig | 4 +-
> > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h | 48 ++++++++++---------
> > .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c | 2 +-
> > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 6 ++-
> > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c | 5 ++
> > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c | 5 ++
> > 6 files changed, 43 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/iio/imu/st_lsm6dsx/Kconfig b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > index 5865a295a4df..645039edd606 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/Kconfig
> > +++ b/drivers/iio/imu/st_lsm6dsx/Kconfig
> > @@ -14,8 +14,8 @@ config IIO_ST_LSM6DSX
> > sensor. Supported devices: lsm6ds3, lsm6ds3h, lsm6dsl, lsm6dsm,
> > ism330dlc, lsm6dso, lsm6dsox, asm330lhh, asm330lhhx, lsm6dsr,
> > lsm6ds3tr-c, ism330dhcx, lsm6dsrx, lsm6ds0, lsm6dsop, lsm6dstx,
> > - lsm6dsv, lsm6dsv16x, lsm6dso16is, ism330is, asm330lhb, lsm6dst
> > - and the accelerometer/gyroscope of lsm9ds1.
> > + lsm6dsv, lsm6dsv16x, lsm6dso16is, ism330is, asm330lhb, asm330lhhxg1,
> > + lsm6dst and the accelerometer/gyroscope of lsm9ds1.
>
> I think it is worth a precursor to reformat this into a one entry per line
> list so that changes become less noisy (and easier to deal with merge conflicts)
> There are some examples of lists in drivers/hid/Kconfig and they look
> find in menuconfig etc.
>
> If I were not requesting other changes I'd have suggested this as a follow up
> patch but given you are going to be respinning again, it would be neater
> as a precursor!
ack, I will fix it in v2.
>
>
> >
> > 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 c19237717e81..78d12d3c2759 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > @@ -15,29 +15,30 @@
> > #include <linux/iio/iio.h>
> > #include <linux/regulator/consumer.h>
> >
> > -#define ST_LSM6DS3_DEV_NAME "lsm6ds3"
> > -#define ST_LSM6DS3H_DEV_NAME "lsm6ds3h"
> > -#define ST_LSM6DSL_DEV_NAME "lsm6dsl"
> > -#define ST_LSM6DSM_DEV_NAME "lsm6dsm"
> > -#define ST_ISM330DLC_DEV_NAME "ism330dlc"
> > -#define ST_LSM6DSO_DEV_NAME "lsm6dso"
> > -#define ST_ASM330LHH_DEV_NAME "asm330lhh"
> > -#define ST_LSM6DSOX_DEV_NAME "lsm6dsox"
> > -#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-imu"
> > -#define ST_LSM6DS0_DEV_NAME "lsm6ds0"
> > -#define ST_LSM6DSRX_DEV_NAME "lsm6dsrx"
> > -#define ST_LSM6DST_DEV_NAME "lsm6dst"
> > -#define ST_LSM6DSOP_DEV_NAME "lsm6dsop"
> > -#define ST_ASM330LHHX_DEV_NAME "asm330lhhx"
> > -#define ST_LSM6DSTX_DEV_NAME "lsm6dstx"
> > -#define ST_LSM6DSV_DEV_NAME "lsm6dsv"
> > -#define ST_LSM6DSV16X_DEV_NAME "lsm6dsv16x"
> > -#define ST_LSM6DSO16IS_DEV_NAME "lsm6dso16is"
> > -#define ST_ISM330IS_DEV_NAME "ism330is"
> > -#define ST_ASM330LHB_DEV_NAME "asm330lhb"
> > +#define ST_LSM6DS3_DEV_NAME "lsm6ds3"
> > +#define ST_LSM6DS3H_DEV_NAME "lsm6ds3h"
> > +#define ST_LSM6DSL_DEV_NAME "lsm6dsl"
> > +#define ST_LSM6DSM_DEV_NAME "lsm6dsm"
> > +#define ST_ISM330DLC_DEV_NAME "ism330dlc"
> > +#define ST_LSM6DSO_DEV_NAME "lsm6dso"
> > +#define ST_ASM330LHH_DEV_NAME "asm330lhh"
> > +#define ST_LSM6DSOX_DEV_NAME "lsm6dsox"
> > +#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-imu"
> > +#define ST_LSM6DS0_DEV_NAME "lsm6ds0"
> > +#define ST_LSM6DSRX_DEV_NAME "lsm6dsrx"
> > +#define ST_LSM6DST_DEV_NAME "lsm6dst"
> > +#define ST_LSM6DSOP_DEV_NAME "lsm6dsop"
> > +#define ST_ASM330LHHX_DEV_NAME "asm330lhhx"
> > +#define ST_LSM6DSTX_DEV_NAME "lsm6dstx"
> > +#define ST_LSM6DSV_DEV_NAME "lsm6dsv"
> > +#define ST_LSM6DSV16X_DEV_NAME "lsm6dsv16x"
> > +#define ST_LSM6DSO16IS_DEV_NAME "lsm6dso16is"
> > +#define ST_ISM330IS_DEV_NAME "ism330is"
> > +#define ST_ASM330LHB_DEV_NAME "asm330lhb"
> > +#define ST_ASM330LHHXG1_DEV_NAME "asm330lhhxg1"
>
> Too much noise. I don't care if they are all aligned the same.
> If you really want to do this, break it out to a precursor patch.
> I'd just have one entry with a different indent :)
ack, I will fix it in v2.
>
>
> >
> > enum st_lsm6dsx_hw_id {
> > ST_LSM6DS3_ID = 1,
> > @@ -63,6 +64,7 @@ enum st_lsm6dsx_hw_id {
> > ST_LSM6DSO16IS_ID,
> > ST_ISM330IS_ID,
> > ST_ASM330LHB_ID,
> > + ST_ASM330LHHXG1_ID,
> > ST_LSM6DSX_MAX_ID,
> > };
> >
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > index 066fe561c5e8..5d9539822ec6 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> > @@ -15,7 +15,7 @@
> > * value of the decimation factor and ODR set for each FIFO data set.
> > *
> > * LSM6DSO/LSM6DSOX/ASM330LHH/ASM330LHHX/LSM6DSR/LSM6DSRX/ISM330DHCX/
> > - * LSM6DST/LSM6DSOP/LSM6DSTX/LSM6DSV/ASM330LHB:
> > + * LSM6DST/LSM6DSOP/LSM6DSTX/LSM6DSV/ASM330LHB/ASM330LHHXG1:
>
> A list here as well would help.
ack, I will fix it in v2.
>
> > * The FIFO buffer can be configured to store data from gyroscope and
> > * accelerometer. Each sample is queued with a tag (1B) indicating data
> > * source (gyroscope, accelerometer, hw timer).
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > index b6e6b1df8a61..27ecf2a5d0bc 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > @@ -27,7 +27,7 @@
> > * - FIFO size: 4KB
> > *
> > * - LSM6DSO/LSM6DSOX/ASM330LHH/ASM330LHHX/LSM6DSR/ISM330DHCX/LSM6DST/LSM6DSOP/
> > - * LSM6DSTX/LSM6DSO16IS/ISM330IS:
> > + * LSM6DSTX/LSM6DSO16IS/ISM330IS/ASM330LHHXG1:
>
> Another place where reformatting as a list would make adding new entries less noisy
> + whilst doing that, please fix the ordering to be alphabetical.
ack, I will fix it in v2.
Regards,
Lorenzo
>
> > * - Accelerometer/Gyroscope supported ODR [Hz]: 12.5, 26, 52, 104, 208, 416,
> > * 833
> > * - Accelerometer supported full-scale [g]: +-2/+-4/+-8/+-16
> > @@ -820,6 +820,10 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> > .hw_id = ST_ASM330LHHX_ID,
> > .name = ST_ASM330LHHX_DEV_NAME,
> > .wai = 0x6b,
> > + }, {
> > + .hw_id = ST_ASM330LHHXG1_ID,
> > + .name = ST_ASM330LHHXG1_DEV_NAME,
> > + .wai = 0x6b,
> > }, {
> > .hw_id = ST_LSM6DSTX_ID,
> > .name = ST_LSM6DSTX_DEV_NAME,
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
> > index 911444ec57c0..cddf41cc0ca9 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_i2c.c
> > @@ -134,6 +134,10 @@ static const struct of_device_id st_lsm6dsx_i2c_of_match[] = {
> > .compatible = "st,asm330lhb",
> > .data = (void *)ST_ASM330LHB_ID,
> > },
> > + {
> > + .compatible = "st,asm330lhhxg1",
> > + .data = (void *)ST_ASM330LHHXG1_ID,
> > + },
> > {},
> > };
> > MODULE_DEVICE_TABLE(of, st_lsm6dsx_i2c_of_match);
> > @@ -168,6 +172,7 @@ static const struct i2c_device_id st_lsm6dsx_i2c_id_table[] = {
> > { ST_LSM6DSO16IS_DEV_NAME, ST_LSM6DSO16IS_ID },
> > { ST_ISM330IS_DEV_NAME, ST_ISM330IS_ID },
> > { ST_ASM330LHB_DEV_NAME, ST_ASM330LHB_ID },
> > + { ST_ASM330LHHXG1_DEV_NAME, ST_ASM330LHHXG1_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 f56c170c41a9..c122c8831365 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_spi.c
> > @@ -129,6 +129,10 @@ static const struct of_device_id st_lsm6dsx_spi_of_match[] = {
> > .compatible = "st,asm330lhb",
> > .data = (void *)ST_ASM330LHB_ID,
> > },
> > + {
> > + .compatible = "st,asm330lhhxg1",
> > + .data = (void *)ST_ASM330LHHXG1_ID,
> > + },
> > {},
> > };
> > MODULE_DEVICE_TABLE(of, st_lsm6dsx_spi_of_match);
> > @@ -157,6 +161,7 @@ static const struct spi_device_id st_lsm6dsx_spi_id_table[] = {
> > { ST_LSM6DSO16IS_DEV_NAME, ST_LSM6DSO16IS_ID },
> > { ST_ISM330IS_DEV_NAME, ST_ISM330IS_ID },
> > { ST_ASM330LHB_DEV_NAME, ST_ASM330LHB_ID },
> > + { ST_ASM330LHHXG1_DEV_NAME, ST_ASM330LHHXG1_ID },
> > {},
> > };
> > MODULE_DEVICE_TABLE(spi, st_lsm6dsx_spi_id_table);
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
next prev parent reply other threads:[~2024-01-28 10:38 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-24 10:52 [PATCH 0/2] add support for ASM330LHHXG1 Lorenzo Bianconi
2024-01-24 10:52 ` [PATCH 1/2] iio: imu: st_lsm6dsx: add support to ASM330LHHXG1 Lorenzo Bianconi
2024-01-27 15:00 ` Jonathan Cameron
2024-01-28 10:38 ` Lorenzo Bianconi [this message]
2024-01-24 10:52 ` [PATCH 2/2] dt-bindings: iio: imu: st_lsm6dsx: add asm330lhhxg1 Lorenzo Bianconi
2024-01-27 15:04 ` Jonathan Cameron
2024-01-28 10:35 ` Lorenzo Bianconi
2024-01-28 13:38 ` 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=ZbYungPhhzx2xREi@lore-desk \
--to=lorenzo@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-iio@vger.kernel.org \
--cc=lorenzo.bianconi@redhat.com \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.