RE: [PATCH 2/2] iio:magnetometer: Support for ST magnetic sensors
Hi Andy, Thanks a lot for your input, I just extend the existing driver in patch V2 for the sersor. Best Regards ** Li Qingwu (Terry) Senior Embedded Software Engineer Leica Geosystems(Shanghai)Co.,Limited (Tel): +86 21 61061036 (FAX): +86 21 61061008 (Mobile): +86 187 0185 9600 E-mail: qing-wu...@leica-geosystems.com.cn Http: www.leica-geosystems.com.cn ** -Original Message- From: Andy Shevchenko Sent: Tuesday, March 9, 2021 7:01 PM To: Jonathan Cameron Cc: LI Qingwu ; Lars-Peter Clausen ; Peter Meerwald ; Rob Herring ; Denis Ciocca ; linux-iio ; devicetree ; Linux Kernel Mailing List ; TERTYCHNYI Grygorii ; ZHIZHIKIN Andrey ; Lorenzo Bianconi Subject: Re: [PATCH 2/2] iio:magnetometer: Support for ST magnetic sensors This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email. On Sat, Mar 6, 2021 at 6:44 PM Jonathan Cameron wrote: > On Fri, 5 Mar 2021 07:05:36 + > LI Qingwu wrote: > > > Add support for STMicroelectronics digital magnetic sensors, > > LSM303AH,LSM303AGR,LIS2MDL,ISM303DAC,IIS2MDC. > > > > The patch tested with IIS2MDC instrument. > > > > Signed-off-by: LI Qingwu > > Hi, > > Given that at least two parts in here is supported by the existing > driver in iio/magnetometers/st_magn_*.c (lsm303agr) can you confirm > that it doesn't make sense to simply extend that driver to support the > other parts? This is particularly true when the WHO AM I register > reads 0x40 for all these parts. > > I've done a fairly superficial review whilst here, but please check > you can't just add the relevant entries to the existing driver. I even hadn't looked into the code because this one needs a very good justification why it's a new driver rather than extension of the existing one. -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] iio:magnetometer: Support for ST magnetic sensors
On Sat, Mar 6, 2021 at 6:44 PM Jonathan Cameron wrote: > On Fri, 5 Mar 2021 07:05:36 + > LI Qingwu wrote: > > > Add support for STMicroelectronics digital magnetic sensors, > > LSM303AH,LSM303AGR,LIS2MDL,ISM303DAC,IIS2MDC. > > > > The patch tested with IIS2MDC instrument. > > > > Signed-off-by: LI Qingwu > > Hi, > > Given that at least two parts in here is supported by the existing > driver in > iio/magnetometers/st_magn_*.c (lsm303agr) can you confirm that it > doesn't make sense to simply extend that driver to support the > other parts? This is particularly true when the WHO AM I register > reads 0x40 for all these parts. > > I've done a fairly superficial review whilst here, but please check > you can't just add the relevant entries to the existing driver. I even hadn't looked into the code because this one needs a very good justification why it's a new driver rather than extension of the existing one. -- With Best Regards, Andy Shevchenko
RE: [PATCH 2/2] iio:magnetometer: Support for ST magnetic sensors
Hi Jonathan, Thanks for your input, I will integrate into existing magnetometers driver! Best Regards ** Li Qingwu (Terry) ** -Original Message- From: Jonathan Cameron Sent: Sunday, March 7, 2021 12:42 AM To: LI Qingwu Cc: l...@metafoo.de; pme...@pmeerw.net; robh...@kernel.org; denis.cio...@st.com; linux-...@vger.kernel.org; devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; TERTYCHNYI Grygorii ; ZHIZHIKIN Andrey ; Lorenzo Bianconi Subject: Re: [PATCH 2/2] iio:magnetometer: Support for ST magnetic sensors This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email. On Fri, 5 Mar 2021 07:05:36 + LI Qingwu wrote: > Add support for STMicroelectronics digital magnetic sensors, > LSM303AH,LSM303AGR,LIS2MDL,ISM303DAC,IIS2MDC. > > The patch tested with IIS2MDC instrument. > > Signed-off-by: LI Qingwu Hi, Given that at least two parts in here is supported by the existing driver in iio/magnetometers/st_magn_*.c (lsm303agr) can you confirm that it doesn't make sense to simply extend that driver to support the other parts? This is particularly true when the WHO AM I register reads 0x40 for all these parts. I've done a fairly superficial review whilst here, but please check you can't just add the relevant entries to the existing driver. Jonathan +CC Lorenzo > --- > drivers/iio/magnetometer/Kconfig | 23 ++ > drivers/iio/magnetometer/Makefile | 4 + > drivers/iio/magnetometer/st_mag40_buffer.c | 191 +++ > drivers/iio/magnetometer/st_mag40_core.c | 371 + > drivers/iio/magnetometer/st_mag40_core.h | 136 > drivers/iio/magnetometer/st_mag40_i2c.c| 180 ++ > drivers/iio/magnetometer/st_mag40_spi.c| 188 +++ > 7 files changed, 1093 insertions(+) > create mode 100644 drivers/iio/magnetometer/st_mag40_buffer.c > create mode 100644 drivers/iio/magnetometer/st_mag40_core.c > create mode 100644 drivers/iio/magnetometer/st_mag40_core.h > create mode 100644 drivers/iio/magnetometer/st_mag40_i2c.c > create mode 100644 drivers/iio/magnetometer/st_mag40_spi.c > > diff --git a/drivers/iio/magnetometer/Kconfig > b/drivers/iio/magnetometer/Kconfig > index 1697a8c03506..bfd2866faa99 100644 > --- a/drivers/iio/magnetometer/Kconfig > +++ b/drivers/iio/magnetometer/Kconfig > @@ -205,4 +205,27 @@ config SENSORS_RM3100_SPI > To compile this driver as a module, choose M here: the module > will be called rm3100-spi. > > +config ST_MAG40_IIO > + tristate "STMicroelectronics > LIS2MDL/LSM303AH/LSM303AGR/ISM303DAC/IIS2MDC sensor" > + depends on (I2C || SPI) && SYSFS > + select IIO_BUFFER > + select IIO_TRIGGERED_BUFFER > + select ST_MAG40_I2C_IIO if (I2C) > + select ST_MAG40_SPI_IIO if (SPI) > + help > + Say yes here to build support for STMicroelectronics magnetometers: > + LIS2MDL, LSM303AH, LSM303AGR, ISM303DAC, IIS2MDC. > + > + To compile this driver as a module, choose M here. The module > + will be called st_mag40. > + > +config ST_MAG40_I2C_IIO > + tristate > + depends on ST_MAG40_IIO > + depends on I2C > + > +config ST_MAG40_SPI_IIO > + tristate > + depends on ST_MAG40_IIO > + depends on SPI > endmenu > diff --git a/drivers/iio/magnetometer/Makefile > b/drivers/iio/magnetometer/Makefile > index ba1bc34b82fa..b6b427cfc284 100644 > --- a/drivers/iio/magnetometer/Makefile > +++ b/drivers/iio/magnetometer/Makefile > @@ -25,6 +25,10 @@ obj-$(CONFIG_SENSORS_HMC5843) += > hmc5843_core.o > obj-$(CONFIG_SENSORS_HMC5843_I2C)+= hmc5843_i2c.o > obj-$(CONFIG_SENSORS_HMC5843_SPI)+= hmc5843_spi.o > > +st_mag40-y += st_mag40_buffer.o st_mag40_core.o > +obj-$(CONFIG_ST_MAG40_IIO) += st_mag40.o > +obj-$(CONFIG_ST_MAG40_I2C_IIO) += st_mag40_i2c.o > +obj-$(CONFIG_ST_MAG40_SPI_IIO) += st_mag40_spi.o > obj-$(CONFIG_SENSORS_RM3100) += rm3100-core.o > obj-$(CONFIG_SENSORS_RM3100_I2C) += rm3100-i2c.o > obj-$(CONFIG_SENSORS_RM3100_SPI) += rm3100-spi.o > diff --git a/drivers/iio/magnetometer/st_mag40_buffer.c > b/drivers/iio/magnetometer/st_mag40_buffer.c > new file mode 100644 > index ..d2a67c9dae5e > --- /dev/null > +++ b/drivers/iio/magnetometer/st_mag40_buffer.c > @@ -0,0 +1,191 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * STMicroelectronics st_mag40 driver > + * > + * Copyright 2016 STMicroelectronics Inc. > + * > + * Matteo Dameno > + * Armando Visconti > + * Lorenzo Bianconi > + * > + * Licensed und
Re: [PATCH 2/2] iio:magnetometer: Support for ST magnetic sensors
On Fri, 5 Mar 2021 07:05:36 +
LI Qingwu wrote:
> Add support for STMicroelectronics digital magnetic sensors,
> LSM303AH,LSM303AGR,LIS2MDL,ISM303DAC,IIS2MDC.
>
> The patch tested with IIS2MDC instrument.
>
> Signed-off-by: LI Qingwu
Hi,
Given that at least two parts in here is supported by the existing
driver in
iio/magnetometers/st_magn_*.c (lsm303agr) can you confirm that it
doesn't make sense to simply extend that driver to support the
other parts? This is particularly true when the WHO AM I register
reads 0x40 for all these parts.
I've done a fairly superficial review whilst here, but please check
you can't just add the relevant entries to the existing driver.
Jonathan
+CC Lorenzo
> ---
> drivers/iio/magnetometer/Kconfig | 23 ++
> drivers/iio/magnetometer/Makefile | 4 +
> drivers/iio/magnetometer/st_mag40_buffer.c | 191 +++
> drivers/iio/magnetometer/st_mag40_core.c | 371 +
> drivers/iio/magnetometer/st_mag40_core.h | 136
> drivers/iio/magnetometer/st_mag40_i2c.c| 180 ++
> drivers/iio/magnetometer/st_mag40_spi.c| 188 +++
> 7 files changed, 1093 insertions(+)
> create mode 100644 drivers/iio/magnetometer/st_mag40_buffer.c
> create mode 100644 drivers/iio/magnetometer/st_mag40_core.c
> create mode 100644 drivers/iio/magnetometer/st_mag40_core.h
> create mode 100644 drivers/iio/magnetometer/st_mag40_i2c.c
> create mode 100644 drivers/iio/magnetometer/st_mag40_spi.c
>
> diff --git a/drivers/iio/magnetometer/Kconfig
> b/drivers/iio/magnetometer/Kconfig
> index 1697a8c03506..bfd2866faa99 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -205,4 +205,27 @@ config SENSORS_RM3100_SPI
> To compile this driver as a module, choose M here: the module
> will be called rm3100-spi.
>
> +config ST_MAG40_IIO
> + tristate "STMicroelectronics
> LIS2MDL/LSM303AH/LSM303AGR/ISM303DAC/IIS2MDC sensor"
> + depends on (I2C || SPI) && SYSFS
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + select ST_MAG40_I2C_IIO if (I2C)
> + select ST_MAG40_SPI_IIO if (SPI)
> + help
> + Say yes here to build support for STMicroelectronics magnetometers:
> + LIS2MDL, LSM303AH, LSM303AGR, ISM303DAC, IIS2MDC.
> +
> + To compile this driver as a module, choose M here. The module
> + will be called st_mag40.
> +
> +config ST_MAG40_I2C_IIO
> + tristate
> + depends on ST_MAG40_IIO
> + depends on I2C
> +
> +config ST_MAG40_SPI_IIO
> + tristate
> + depends on ST_MAG40_IIO
> + depends on SPI
> endmenu
> diff --git a/drivers/iio/magnetometer/Makefile
> b/drivers/iio/magnetometer/Makefile
> index ba1bc34b82fa..b6b427cfc284 100644
> --- a/drivers/iio/magnetometer/Makefile
> +++ b/drivers/iio/magnetometer/Makefile
> @@ -25,6 +25,10 @@ obj-$(CONFIG_SENSORS_HMC5843) +=
> hmc5843_core.o
> obj-$(CONFIG_SENSORS_HMC5843_I2C)+= hmc5843_i2c.o
> obj-$(CONFIG_SENSORS_HMC5843_SPI)+= hmc5843_spi.o
>
> +st_mag40-y += st_mag40_buffer.o st_mag40_core.o
> +obj-$(CONFIG_ST_MAG40_IIO) += st_mag40.o
> +obj-$(CONFIG_ST_MAG40_I2C_IIO) += st_mag40_i2c.o
> +obj-$(CONFIG_ST_MAG40_SPI_IIO) += st_mag40_spi.o
> obj-$(CONFIG_SENSORS_RM3100) += rm3100-core.o
> obj-$(CONFIG_SENSORS_RM3100_I2C) += rm3100-i2c.o
> obj-$(CONFIG_SENSORS_RM3100_SPI) += rm3100-spi.o
> diff --git a/drivers/iio/magnetometer/st_mag40_buffer.c
> b/drivers/iio/magnetometer/st_mag40_buffer.c
> new file mode 100644
> index ..d2a67c9dae5e
> --- /dev/null
> +++ b/drivers/iio/magnetometer/st_mag40_buffer.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * STMicroelectronics st_mag40 driver
> + *
> + * Copyright 2016 STMicroelectronics Inc.
> + *
> + * Matteo Dameno
> + * Armando Visconti
> + * Lorenzo Bianconi
> + *
> + * Licensed under the GPL-2.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "st_mag40_core.h"
> +
> +#define ST_MAG40_EWMA_DIV128
> +static inline s64 st_mag40_ewma(s64 old, s64 new, int weight)
> +{
> + s64 diff, incr;
> +
> + diff = new - old;
> + incr = div_s64((ST_MAG40_EWMA_DIV - weight) * diff,
> + ST_MAG40_EWMA_DIV);
> +
> + return old + incr;
> +}
> +
> +static irqreturn_t st_mag40_trigger_irq_handler(int irq, void *private)
> +{
> + struct st_mag40_data *cdata = private;
> + s64 ts;
> + u8 weight = (cdata->odr >= 50) ? 96 : 0;
> +
> + ts = st_mag40_get_timestamp();
> + cdata->delta_ts = st_mag40_ewma(cdata->delta_ts, ts - cdata->ts_irq,
> weight);
> + cdata->ts_irq = ts;
> +
> + return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t st_mag40_trigger_thread_handler(int irq, void *private)
> +{
> + struct st_mag40_data *cdata = private;
> + u8
