* [PATCH 0/2] iio: magnetometer: Infineon TLV493D-A1B6 support
@ 2017-12-09 23:25 Andreas Färber
2017-12-09 23:25 ` [PATCH 1/2] dt-bindings: Add Infineon TLV493D-A1B6 Andreas Färber
2017-12-09 23:25 ` [PATCH 2/2] iio: magnetometer: " Andreas Färber
0 siblings, 2 replies; 9+ messages in thread
From: Andreas Färber @ 2017-12-09 23:25 UTC (permalink / raw)
To: linux-iio; +Cc: linux-kernel, Andreas Färber, Marius Tarcatu, devicetree
Hello,
This mini-series adds initial support for the Infineon TLV493D-A1B6 3D Magnetic
Sensor found on the Infineon 3D Magnetic Sensor 2Go Kit.
Tested on a Raspberry Pi 3 with a DT Overlay.
https://github.com/afaerber/dt-overlays/blob/master/bcm2837-rpi-3-b%2Btlv493d-a1b6.dts
Have a lot of fun!
Cheers,
Andreas
Cc: Marius Tarcatu <marius.tarcatu@infineon.com>
Cc: devicetree@vger.kernel.org
Cc: linux-iio@vger.kernel.org
Andreas Färber (2):
dt-bindings: Add Infineon TLV493D-A1B6
iio: magnetometer: Add Infineon TLV493D-A1B6
.../devicetree/bindings/trivial-devices.txt | 1 +
drivers/iio/magnetometer/Kconfig | 9 +
drivers/iio/magnetometer/Makefile | 2 +
drivers/iio/magnetometer/tlv493d.c | 328 +++++++++++++++++++++
4 files changed, 340 insertions(+)
create mode 100644 drivers/iio/magnetometer/tlv493d.c
--
2.13.6
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] dt-bindings: Add Infineon TLV493D-A1B6
2017-12-09 23:25 [PATCH 0/2] iio: magnetometer: Infineon TLV493D-A1B6 support Andreas Färber
@ 2017-12-09 23:25 ` Andreas Färber
2017-12-15 21:42 ` Rob Herring
2017-12-09 23:25 ` [PATCH 2/2] iio: magnetometer: " Andreas Färber
1 sibling, 1 reply; 9+ messages in thread
From: Andreas Färber @ 2017-12-09 23:25 UTC (permalink / raw)
To: linux-iio
Cc: linux-kernel, Andreas Färber, Marius Tarcatu, Rob Herring,
Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
The Infineon TLV493D-A1B6 is an I2C-based 3D Magnetic Sensor.
Cc: Marius Tarcatu <marius.tarcatu@infineon.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
Documentation/devicetree/bindings/trivial-devices.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/trivial-devices.txt b/Documentation/devicetree/bindings/trivial-devices.txt
index 5f3143f97098..7ad1939cb0d6 100644
--- a/Documentation/devicetree/bindings/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/trivial-devices.txt
@@ -63,6 +63,7 @@ fsl,sgtl5000 SGTL5000: Ultra Low-Power Audio Codec
gmt,g751 G751: Digital Temperature Sensor and Thermal Watchdog with Two-Wire Interface
infineon,slb9635tt Infineon SLB9635 (Soft-) I2C TPM (old protocol, max 100khz)
infineon,slb9645tt Infineon SLB9645 I2C TPM (new protocol, max 400khz)
+infineon,tlv493d-a1b6 Infineon TLV493D-A1B6 I2C 3D Magnetic Sensor
isil,isl1208 Intersil ISL1208 Low Power RTC with Battery Backed SRAM
isil,isl1218 Intersil ISL1218 Low Power RTC with Battery Backed SRAM
isil,isl12022 Intersil ISL12022 Real-time Clock
--
2.13.6
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] iio: magnetometer: Add Infineon TLV493D-A1B6
2017-12-09 23:25 [PATCH 0/2] iio: magnetometer: Infineon TLV493D-A1B6 support Andreas Färber
2017-12-09 23:25 ` [PATCH 1/2] dt-bindings: Add Infineon TLV493D-A1B6 Andreas Färber
@ 2017-12-09 23:25 ` Andreas Färber
2017-12-10 8:50 ` Philippe Ombredanne
2017-12-10 9:08 ` Peter Meerwald-Stadler
1 sibling, 2 replies; 9+ messages in thread
From: Andreas Färber @ 2017-12-09 23:25 UTC (permalink / raw)
To: linux-iio
Cc: linux-kernel, Andreas Färber, Marius Tarcatu,
Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald-Stadler
The Infineon TLV493D is a 3D magnetic sensor, A1B6 is I2C based.
It is found among others on the Infineon 3D Magnetic Sensor 2Go Kit,
which allows to detach the sensor as a breakout board.
Cc: Marius Tarcatu <marius.tarcatu@infineon.com>
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
drivers/iio/magnetometer/Kconfig | 9 +
drivers/iio/magnetometer/Makefile | 2 +
drivers/iio/magnetometer/tlv493d.c | 328 +++++++++++++++++++++++++++++++++++++
3 files changed, 339 insertions(+)
create mode 100644 drivers/iio/magnetometer/tlv493d.c
diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index ed9d776d01af..5945d88a1595 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -175,4 +175,13 @@ config SENSORS_HMC5843_SPI
- hmc5843_core (core functions)
- hmc5843_spi (support for HMC5983)
+config TLV493D
+ tristate "Infineon TLV493D 3D Magnetic Sensor"
+ depends on I2C
+ help
+ Say Y here to build support for Infineon TLV493D-A1B6 I2C-based
+ 3-axis magnetometer.
+
+ This driver can also be built as a module and will be called tlv493d.
+
endmenu
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index 664b2f866472..df6ad23fee65 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -24,3 +24,5 @@ obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
obj-$(CONFIG_SENSORS_HMC5843) += hmc5843_core.o
obj-$(CONFIG_SENSORS_HMC5843_I2C) += hmc5843_i2c.o
obj-$(CONFIG_SENSORS_HMC5843_SPI) += hmc5843_spi.o
+
+obj-$(CONFIG_TLV493D) += tlv493d.o
diff --git a/drivers/iio/magnetometer/tlv493d.c b/drivers/iio/magnetometer/tlv493d.c
new file mode 100644
index 000000000000..d2fe296b2c80
--- /dev/null
+++ b/drivers/iio/magnetometer/tlv493d.c
@@ -0,0 +1,328 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Infineon TLV493D-A1B6 3D magnetic sensor
+ *
+ * Copyright (c) 2016-2017 Andreas Färber
+ */
+
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+
+#define MOD1_LOW BIT(0)
+#define MOD1_FAST BIT(1)
+#define MOD1_INT BIT(2)
+#define MOD1_P BIT(7)
+
+#define MOD2_PT BIT(5)
+
+struct tlv493d {
+ struct i2c_client *i2c;
+ struct iio_mount_matrix mount_matrix;
+ u8 factset1, factset2, factset3;
+};
+
+static int tlv493d_read_regs(struct tlv493d *s, u8 *buf, int len)
+{
+ int ret;
+
+ ret = i2c_master_recv(s->i2c, buf, len);
+ if (ret < len) {
+ dev_err(&s->i2c->dev, "failed to read registers (%d)", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static unsigned int tlv493d_parity(u32 v)
+{
+ v ^= v >> 16;
+ v ^= v >> 8;
+ v ^= v >> 4;
+ v &= 0xf;
+ return (0x6996 >> v) & 1;
+}
+
+#define MOD1_RES_MASK (0x3 << 3)
+#define MOD1_IICADDR_MASK (0x3 << 5)
+
+static int tlv493d_write_regs(struct tlv493d *s, const u8 *regs, int len)
+{
+ u8 buf[4];
+ int ret;
+
+ if (len != ARRAY_SIZE(buf))
+ return -EINVAL;
+
+ buf[0] = 0;
+ buf[1] = s->factset1 & (MOD1_IICADDR_MASK | MOD1_RES_MASK);
+ buf[1] |= regs[1] & ~(MOD1_P | MOD1_IICADDR_MASK | MOD1_RES_MASK);
+ buf[2] = s->factset2;
+ buf[3] = MOD2_PT | (s->factset3 & 0x1f);
+ buf[3] |= regs[3] & ~(MOD2_PT | 0x1f);
+
+ if ((buf[3] & MOD2_PT) &&
+ !tlv493d_parity((buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3]))
+ buf[1] |= MOD1_P;
+
+ ret = i2c_master_send(s->i2c, buf, 4);
+ if (ret < 4) {
+ dev_err(&s->i2c->dev, "failed to write registers (%d)", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int tlv493d_power_down(struct tlv493d *s)
+{
+ u8 buf[4];
+
+ buf[0] = 0;
+ buf[1] = 0;
+ buf[2] = 0;
+ buf[3] = 0;
+ return tlv493d_write_regs(s, buf, 4);
+}
+
+static const struct iio_mount_matrix *
+tlv493d_get_mount_matrix(const struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct tlv493d *s = iio_priv(indio_dev);
+
+ return &s->mount_matrix;
+}
+
+static const struct iio_chan_spec_ext_info tlv493d_ext_info[] = {
+ IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, tlv493d_get_mount_matrix),
+ {}
+};
+
+#define TLV493D_AXIS_CHANNEL(axis, index) \
+ { \
+ .type = IIO_MAGN, \
+ .channel2 = IIO_MOD_##axis, \
+ .modified = 1, \
+ .address = index, \
+ .scan_index = index, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 12, \
+ .storagebits = 16, \
+ .endianness = IIO_CPU, \
+ }, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_PROCESSED), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+ .ext_info = tlv493d_ext_info, \
+ }
+
+#define TLV493D_TEMP_CHANNEL(index) \
+ { \
+ .type = IIO_TEMP, \
+ .channel2 = IIO_MOD_TEMP_OBJECT, \
+ .modified = 1, \
+ .address = index, \
+ .scan_index = index, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 12, \
+ .storagebits = 16, \
+ .endianness = IIO_CPU, \
+ }, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_OFFSET) | \
+ BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_PROCESSED), \
+ }
+
+static const struct iio_chan_spec tlv493d_channels[] = {
+ TLV493D_AXIS_CHANNEL(X, 0),
+ TLV493D_AXIS_CHANNEL(Y, 1),
+ TLV493D_AXIS_CHANNEL(Z, 2),
+ TLV493D_TEMP_CHANNEL(3),
+ IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+static const unsigned long tlv493d_scan_masks[] = { 0xf, 0 };
+
+static int tlv493d_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct tlv493d *s = iio_priv(indio_dev);
+ u8 buf[7];
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ case IIO_CHAN_INFO_RAW:
+ do {
+ ret = tlv493d_read_regs(s, buf, 7);
+ if (ret)
+ return ret;
+ } while ((buf[3] & 0x3) || (buf[5] & BIT(4)));
+
+ switch (chan->address) {
+ case 0:
+ *val = (((int)(s8)buf[0]) << 4) | (buf[4] >> 4);
+ break;
+ case 1:
+ *val = (((int)(s8)buf[1]) << 4) | (buf[4] & 0xf);
+ break;
+ case 2:
+ *val = (((int)(s8)buf[2]) << 4) | (buf[5] & 0xf);
+ break;
+ case 3:
+ *val = (((int)(s8)(buf[3] & 0xf0)) << 4) | buf[6];
+ if (mask != IIO_CHAN_INFO_RAW)
+ *val -= 340;
+ break;
+ default:
+ return -EINVAL;
+ }
+ *val2 = 0;
+ if (mask == IIO_CHAN_INFO_RAW)
+ return IIO_VAL_INT;
+
+ switch (chan->type) {
+ case IIO_MAGN:
+ *val2 = (*val * 1000000) * 98 / 1000;
+ *val = *val2 / 1000000;
+ *val2 %= 1000000;
+ break;
+ case IIO_TEMP:
+ *val2 = (*val * 1000000) * 11 / 10;
+ *val = *val2 / 1000000;
+ *val2 %= 1000000;
+ /* According to datasheet, LSB offset is for 25°C */
+ *val += 25;
+ break;
+ default:
+ return -EINVAL;
+ }
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_CHAN_INFO_OFFSET:
+ switch (chan->type) {
+ case IIO_TEMP:
+ *val = -340;
+ *val2 = 0;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ case IIO_CHAN_INFO_SCALE:
+ switch (chan->type) {
+ case IIO_MAGN:
+ /* 0.098 mT/LSB */
+ *val = 0;
+ *val2 = 98000;
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_TEMP:
+ /* 1.1 °C/LSB */
+ *val = 1;
+ *val2 = 100000;
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info tlv493d_iio_info = {
+ .read_raw = tlv493d_read_raw,
+};
+
+static int tlv493d_probe(struct i2c_client *i2c)
+{
+ struct iio_dev *indio_dev;
+ struct tlv493d *tlv493d;
+ u8 buf[10];
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*tlv493d));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ tlv493d = iio_priv(indio_dev);
+ i2c_set_clientdata(i2c, indio_dev);
+ tlv493d->i2c = i2c;
+
+ ret = of_iio_read_mount_matrix(&i2c->dev, "mount-matrix",
+ &tlv493d->mount_matrix);
+ if (ret)
+ return ret;
+
+ indio_dev->dev.parent = &i2c->dev;
+ indio_dev->channels = tlv493d_channels;
+ indio_dev->num_channels = ARRAY_SIZE(tlv493d_channels);
+ indio_dev->info = &tlv493d_iio_info;
+ indio_dev->available_scan_masks = tlv493d_scan_masks;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->name = "tlv493d";
+
+ ret = tlv493d_read_regs(tlv493d, buf, 10);
+ if (ret)
+ return ret;
+
+ tlv493d->factset1 = buf[7];
+ tlv493d->factset2 = buf[8];
+ tlv493d->factset3 = buf[9];
+
+ buf[0] = 0;
+ buf[1] = MOD1_FAST | MOD1_LOW;
+ buf[2] = 0;
+ buf[3] = 0;
+ ret = tlv493d_write_regs(tlv493d, buf, 4);
+ if (ret)
+ return ret;
+
+ ret = iio_device_register(indio_dev);
+ if (ret) {
+ dev_err(&i2c->dev, "device registration failed");
+ tlv493d_power_down(tlv493d);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int tlv493d_remove(struct i2c_client *i2c)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
+ struct tlv493d *tlv493d = iio_priv(indio_dev);
+
+ iio_device_unregister(indio_dev);
+
+ tlv493d_power_down(tlv493d);
+
+ return 0;
+}
+
+static const struct of_device_id tlv493d_dt_ids[] = {
+ { .compatible = "infineon,tlv493d-a1b6" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, tlv493d_dt_ids);
+
+static struct i2c_driver tlv493d_i2c_driver = {
+ .driver = {
+ .name = "tlv493d",
+ .of_match_table = tlv493d_dt_ids,
+ },
+ .probe_new = tlv493d_probe,
+ .remove = tlv493d_remove,
+};
+
+module_i2c_driver(tlv493d_i2c_driver);
+
+MODULE_DESCRIPTION("TLV493D I2C driver");
+MODULE_AUTHOR("Andreas Faerber");
+MODULE_LICENSE("GPL");
--
2.13.6
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: magnetometer: Add Infineon TLV493D-A1B6
2017-12-09 23:25 ` [PATCH 2/2] iio: magnetometer: " Andreas Färber
@ 2017-12-10 8:50 ` Philippe Ombredanne
2017-12-10 9:08 ` Peter Meerwald-Stadler
1 sibling, 0 replies; 9+ messages in thread
From: Philippe Ombredanne @ 2017-12-10 8:50 UTC (permalink / raw)
To: Andreas Färber
Cc: linux-iio, LKML, Marius Tarcatu, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler
Andreas,
On Sun, Dec 10, 2017 at 12:25 AM, Andreas Färber <afaerber@suse.de> wrote:
> The Infineon TLV493D is a 3D magnetic sensor, A1B6 is I2C based.
>
> It is found among others on the Infineon 3D Magnetic Sensor 2Go Kit,
> which allows to detach the sensor as a breakout board.
>
> Cc: Marius Tarcatu <marius.tarcatu@infineon.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
[...]
> --- /dev/null
> +++ b/drivers/iio/magnetometer/tlv493d.c
> @@ -0,0 +1,328 @@
> +// SPDX-License-Identifier: GPL-2.0+
Thank you++ for using the new SDPX ids!
Acked-by: Philippe Ombredanne <pombredanne@nexb.com>
--
Cordially
Philippe Ombredanne
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: magnetometer: Add Infineon TLV493D-A1B6
2017-12-09 23:25 ` [PATCH 2/2] iio: magnetometer: " Andreas Färber
2017-12-10 8:50 ` Philippe Ombredanne
@ 2017-12-10 9:08 ` Peter Meerwald-Stadler
2017-12-10 17:11 ` Jonathan Cameron
2017-12-10 17:14 ` Andreas Färber
1 sibling, 2 replies; 9+ messages in thread
From: Peter Meerwald-Stadler @ 2017-12-10 9:08 UTC (permalink / raw)
To: Andreas Färber
Cc: linux-iio, linux-kernel, Marius Tarcatu, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen
[-- Attachment #1: Type: text/plain, Size: 10741 bytes --]
> The Infineon TLV493D is a 3D magnetic sensor, A1B6 is I2C based.
comments below
> It is found among others on the Infineon 3D Magnetic Sensor 2Go Kit,
> which allows to detach the sensor as a breakout board.
>
> Cc: Marius Tarcatu <marius.tarcatu@infineon.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> drivers/iio/magnetometer/Kconfig | 9 +
> drivers/iio/magnetometer/Makefile | 2 +
> drivers/iio/magnetometer/tlv493d.c | 328 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 339 insertions(+)
> create mode 100644 drivers/iio/magnetometer/tlv493d.c
>
> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> index ed9d776d01af..5945d88a1595 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -175,4 +175,13 @@ config SENSORS_HMC5843_SPI
> - hmc5843_core (core functions)
> - hmc5843_spi (support for HMC5983)
>
> +config TLV493D
> + tristate "Infineon TLV493D 3D Magnetic Sensor"
> + depends on I2C
> + help
> + Say Y here to build support for Infineon TLV493D-A1B6 I2C-based
> + 3-axis magnetometer.
> +
> + This driver can also be built as a module and will be called tlv493d.
> +
> endmenu
> diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
> index 664b2f866472..df6ad23fee65 100644
> --- a/drivers/iio/magnetometer/Makefile
> +++ b/drivers/iio/magnetometer/Makefile
> @@ -24,3 +24,5 @@ obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
> obj-$(CONFIG_SENSORS_HMC5843) += hmc5843_core.o
> obj-$(CONFIG_SENSORS_HMC5843_I2C) += hmc5843_i2c.o
> obj-$(CONFIG_SENSORS_HMC5843_SPI) += hmc5843_spi.o
> +
> +obj-$(CONFIG_TLV493D) += tlv493d.o
> diff --git a/drivers/iio/magnetometer/tlv493d.c b/drivers/iio/magnetometer/tlv493d.c
> new file mode 100644
> index 000000000000..d2fe296b2c80
> --- /dev/null
> +++ b/drivers/iio/magnetometer/tlv493d.c
> @@ -0,0 +1,328 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Infineon TLV493D-A1B6 3D magnetic sensor
link to datasheet would be nice
> + *
> + * Copyright (c) 2016-2017 Andreas Färber
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +
> +#define MOD1_LOW BIT(0)
> +#define MOD1_FAST BIT(1)
> +#define MOD1_INT BIT(2)
> +#define MOD1_P BIT(7)
> +
> +#define MOD2_PT BIT(5)
please use TLV493D_ prefix; more descriptive naming or comments would be
nice
> +struct tlv493d {
> + struct i2c_client *i2c;
> + struct iio_mount_matrix mount_matrix;
> + u8 factset1, factset2, factset3;
> +};
> +
> +static int tlv493d_read_regs(struct tlv493d *s, u8 *buf, int len)
> +{
> + int ret;
> +
> + ret = i2c_master_recv(s->i2c, buf, len);
> + if (ret < len) {
> + dev_err(&s->i2c->dev, "failed to read registers (%d)", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static unsigned int tlv493d_parity(u32 v)
> +{
> + v ^= v >> 16;
> + v ^= v >> 8;
> + v ^= v >> 4;
> + v &= 0xf;
> + return (0x6996 >> v) & 1;
> +}
> +
> +#define MOD1_RES_MASK (0x3 << 3)
> +#define MOD1_IICADDR_MASK (0x3 << 5)
maybe move all these #defines on top of the file?
> +static int tlv493d_write_regs(struct tlv493d *s, const u8 *regs, int len)
> +{
> + u8 buf[4];
> + int ret;
> +
> + if (len != ARRAY_SIZE(buf))
> + return -EINVAL;
why have this parameter if it is always 4?
> + buf[0] = 0;
> + buf[1] = s->factset1 & (MOD1_IICADDR_MASK | MOD1_RES_MASK);
> + buf[1] |= regs[1] & ~(MOD1_P | MOD1_IICADDR_MASK | MOD1_RES_MASK);
> + buf[2] = s->factset2;
> + buf[3] = MOD2_PT | (s->factset3 & 0x1f);
> + buf[3] |= regs[3] & ~(MOD2_PT | 0x1f);
> +
> + if ((buf[3] & MOD2_PT) &&
> + !tlv493d_parity((buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3]))
> + buf[1] |= MOD1_P;
> +
> + ret = i2c_master_send(s->i2c, buf, 4);
use sizeof(buf) instead of 4?
> + if (ret < 4) {
> + dev_err(&s->i2c->dev, "failed to write registers (%d)", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int tlv493d_power_down(struct tlv493d *s)
> +{
> + u8 buf[4];
maybe
u8 buf[4] = {0,};
> +
> + buf[0] = 0;
> + buf[1] = 0;
> + buf[2] = 0;
> + buf[3] = 0;
> + return tlv493d_write_regs(s, buf, 4);
> +}
> +
> +static const struct iio_mount_matrix *
> +tlv493d_get_mount_matrix(const struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + struct tlv493d *s = iio_priv(indio_dev);
> +
> + return &s->mount_matrix;
> +}
> +
> +static const struct iio_chan_spec_ext_info tlv493d_ext_info[] = {
> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, tlv493d_get_mount_matrix),
> + {}
> +};
> +
> +#define TLV493D_AXIS_CHANNEL(axis, index) \
> + { \
> + .type = IIO_MAGN, \
> + .channel2 = IIO_MOD_##axis, \
> + .modified = 1, \
> + .address = index, \
> + .scan_index = index, \
> + .scan_type = { \
IIO buffer reads are not supported by the driver, so no need for
.scan_type (here and for _TEMP)
> + .sign = 's', \
> + .realbits = 12, \
> + .storagebits = 16, \
> + .endianness = IIO_CPU, \
> + }, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_PROCESSED), \
either _RAW or _PROCESSED, not both
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .ext_info = tlv493d_ext_info, \
> + }
> +
> +#define TLV493D_TEMP_CHANNEL(index) \
> + { \
> + .type = IIO_TEMP, \
> + .channel2 = IIO_MOD_TEMP_OBJECT, \
> + .modified = 1, \
> + .address = index, \
> + .scan_index = index, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 12, \
> + .storagebits = 16, \
> + .endianness = IIO_CPU, \
> + }, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_OFFSET) | \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_PROCESSED), \
> + }
> +
> +static const struct iio_chan_spec tlv493d_channels[] = {
> + TLV493D_AXIS_CHANNEL(X, 0),
> + TLV493D_AXIS_CHANNEL(Y, 1),
> + TLV493D_AXIS_CHANNEL(Z, 2),
> + TLV493D_TEMP_CHANNEL(3),
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +static const unsigned long tlv493d_scan_masks[] = { 0xf, 0 };
> +
> +static int tlv493d_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct tlv493d *s = iio_priv(indio_dev);
> + u8 buf[7];
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + case IIO_CHAN_INFO_RAW:
this is ugly, why support _PROCESSED?
just _RAW is fine
> + do {
> + ret = tlv493d_read_regs(s, buf, 7);
> + if (ret)
> + return ret;
> + } while ((buf[3] & 0x3) || (buf[5] & BIT(4)));
maybe add a timeout or limit the number of retries
> +
> + switch (chan->address) {
> + case 0:
> + *val = (((int)(s8)buf[0]) << 4) | (buf[4] >> 4);
> + break;
> + case 1:
> + *val = (((int)(s8)buf[1]) << 4) | (buf[4] & 0xf);
> + break;
> + case 2:
> + *val = (((int)(s8)buf[2]) << 4) | (buf[5] & 0xf);
> + break;
> + case 3:
> + *val = (((int)(s8)(buf[3] & 0xf0)) << 4) | buf[6];
> + if (mask != IIO_CHAN_INFO_RAW)
> + *val -= 340;
> + break;
> + default:
> + return -EINVAL;
> + }
> + *val2 = 0;
no need to set val2 if returning VAL_INT
> + if (mask == IIO_CHAN_INFO_RAW)
> + return IIO_VAL_INT;
> +
> + switch (chan->type) {
> + case IIO_MAGN:
> + *val2 = (*val * 1000000) * 98 / 1000;
> + *val = *val2 / 1000000;
> + *val2 %= 1000000;
> + break;
> + case IIO_TEMP:
> + *val2 = (*val * 1000000) * 11 / 10;
> + *val = *val2 / 1000000;
> + *val2 %= 1000000;
> + /* According to datasheet, LSB offset is for 25°C */
> + *val += 25;
> + break;
> + default:
> + return -EINVAL;
> + }
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_OFFSET:
> + switch (chan->type) {
> + case IIO_TEMP:
> + *val = -340;
> + *val2 = 0;
no need to set val2
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_MAGN:
> + /* 0.098 mT/LSB */
> + *val = 0;
> + *val2 = 98000;
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_TEMP:
> + /* 1.1 °C/LSB */
> + *val = 1;
> + *val2 = 100000;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info tlv493d_iio_info = {
> + .read_raw = tlv493d_read_raw,
> +};
> +
> +static int tlv493d_probe(struct i2c_client *i2c)
> +{
> + struct iio_dev *indio_dev;
> + struct tlv493d *tlv493d;
> + u8 buf[10];
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*tlv493d));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + tlv493d = iio_priv(indio_dev);
> + i2c_set_clientdata(i2c, indio_dev);
> + tlv493d->i2c = i2c;
> +
> + ret = of_iio_read_mount_matrix(&i2c->dev, "mount-matrix",
> + &tlv493d->mount_matrix);
> + if (ret)
> + return ret;
> +
> + indio_dev->dev.parent = &i2c->dev;
> + indio_dev->channels = tlv493d_channels;
> + indio_dev->num_channels = ARRAY_SIZE(tlv493d_channels);
> + indio_dev->info = &tlv493d_iio_info;
> + indio_dev->available_scan_masks = tlv493d_scan_masks;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->name = "tlv493d";
> +
> + ret = tlv493d_read_regs(tlv493d, buf, 10);
> + if (ret)
> + return ret;
> +
> + tlv493d->factset1 = buf[7];
> + tlv493d->factset2 = buf[8];
> + tlv493d->factset3 = buf[9];
> +
> + buf[0] = 0;
> + buf[1] = MOD1_FAST | MOD1_LOW;
what does this mean? some comment explaining the default mode?
> + buf[2] = 0;
> + buf[3] = 0;
> + ret = tlv493d_write_regs(tlv493d, buf, 4);
> + if (ret)
> + return ret;
> +
> + ret = iio_device_register(indio_dev);
> + if (ret) {
> + dev_err(&i2c->dev, "device registration failed");
> + tlv493d_power_down(tlv493d);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int tlv493d_remove(struct i2c_client *i2c)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
> + struct tlv493d *tlv493d = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> +
> + tlv493d_power_down(tlv493d);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id tlv493d_dt_ids[] = {
> + { .compatible = "infineon,tlv493d-a1b6" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, tlv493d_dt_ids);
> +
> +static struct i2c_driver tlv493d_i2c_driver = {
> + .driver = {
> + .name = "tlv493d",
> + .of_match_table = tlv493d_dt_ids,
> + },
> + .probe_new = tlv493d_probe,
> + .remove = tlv493d_remove,
> +};
> +
> +module_i2c_driver(tlv493d_i2c_driver);
> +
> +MODULE_DESCRIPTION("TLV493D I2C driver");
> +MODULE_AUTHOR("Andreas Faerber");
> +MODULE_LICENSE("GPL");
>
--
Peter Meerwald-Stadler
Mobile: +43 664 24 44 418
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: magnetometer: Add Infineon TLV493D-A1B6
2017-12-10 9:08 ` Peter Meerwald-Stadler
@ 2017-12-10 17:11 ` Jonathan Cameron
2017-12-10 17:14 ` Andreas Färber
1 sibling, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2017-12-10 17:11 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: Andreas Färber, linux-iio, linux-kernel, Marius Tarcatu,
Hartmut Knaack, Lars-Peter Clausen
On Sun, 10 Dec 2017 10:08:04 +0100 (CET)
Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:
> > The Infineon TLV493D is a 3D magnetic sensor, A1B6 is I2C based.
>
> comments below
A few additions from me, but Peter got most stuff.
You could make the driver safe for i2c DMA but it's borderline
on whether any of the transfers are long enough to make it worth
bothering.
Jonathan
>
> > It is found among others on the Infineon 3D Magnetic Sensor 2Go Kit,
> > which allows to detach the sensor as a breakout board.
> >
> > Cc: Marius Tarcatu <marius.tarcatu@infineon.com>
> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > ---
> > drivers/iio/magnetometer/Kconfig | 9 +
> > drivers/iio/magnetometer/Makefile | 2 +
> > drivers/iio/magnetometer/tlv493d.c | 328 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 339 insertions(+)
> > create mode 100644 drivers/iio/magnetometer/tlv493d.c
> >
> > diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> > index ed9d776d01af..5945d88a1595 100644
> > --- a/drivers/iio/magnetometer/Kconfig
> > +++ b/drivers/iio/magnetometer/Kconfig
> > @@ -175,4 +175,13 @@ config SENSORS_HMC5843_SPI
> > - hmc5843_core (core functions)
> > - hmc5843_spi (support for HMC5983)
> >
> > +config TLV493D
> > + tristate "Infineon TLV493D 3D Magnetic Sensor"
> > + depends on I2C
> > + help
> > + Say Y here to build support for Infineon TLV493D-A1B6 I2C-based
> > + 3-axis magnetometer.
> > +
> > + This driver can also be built as a module and will be called tlv493d.
> > +
> > endmenu
> > diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
> > index 664b2f866472..df6ad23fee65 100644
> > --- a/drivers/iio/magnetometer/Makefile
> > +++ b/drivers/iio/magnetometer/Makefile
> > @@ -24,3 +24,5 @@ obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
> > obj-$(CONFIG_SENSORS_HMC5843) += hmc5843_core.o
> > obj-$(CONFIG_SENSORS_HMC5843_I2C) += hmc5843_i2c.o
> > obj-$(CONFIG_SENSORS_HMC5843_SPI) += hmc5843_spi.o
> > +
> > +obj-$(CONFIG_TLV493D) += tlv493d.o
> > diff --git a/drivers/iio/magnetometer/tlv493d.c b/drivers/iio/magnetometer/tlv493d.c
> > new file mode 100644
> > index 000000000000..d2fe296b2c80
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/tlv493d.c
> > @@ -0,0 +1,328 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Infineon TLV493D-A1B6 3D magnetic sensor
>
> link to datasheet would be nice
>
> > + *
> > + * Copyright (c) 2016-2017 Andreas Färber
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +
> > +#define MOD1_LOW BIT(0)
> > +#define MOD1_FAST BIT(1)
> > +#define MOD1_INT BIT(2)
> > +#define MOD1_P BIT(7)
> > +
> > +#define MOD2_PT BIT(5)
>
> please use TLV493D_ prefix; more descriptive naming or comments would be
> nice
>
> > +struct tlv493d {
> > + struct i2c_client *i2c;
> > + struct iio_mount_matrix mount_matrix;
> > + u8 factset1, factset2, factset3;
> > +};
> > +
> > +static int tlv493d_read_regs(struct tlv493d *s, u8 *buf, int len)
> > +{
> > + int ret;
> > +
> > + ret = i2c_master_recv(s->i2c, buf, len);
> > + if (ret < len) {
> > + dev_err(&s->i2c->dev, "failed to read registers (%d)", ret);
> > + return ret;
> > + }
This wrapper doesn't really add much to my mind. I would just put the
i2c_master_recv directly inline.
> > +
> > + return 0;
> > +}
> > +
> > +static unsigned int tlv493d_parity(u32 v)
> > +{
> > + v ^= v >> 16;
> > + v ^= v >> 8;
> > + v ^= v >> 4;
> > + v &= 0xf;
> > + return (0x6996 >> v) & 1;
> > +}
> > +
> > +#define MOD1_RES_MASK (0x3 << 3)
> > +#define MOD1_IICADDR_MASK (0x3 << 5)
>
> maybe move all these #defines on top of the file?
>
> > +static int tlv493d_write_regs(struct tlv493d *s, const u8 *regs, int len)
> > +{
> > + u8 buf[4];
> > + int ret;
> > +
> > + if (len != ARRAY_SIZE(buf))
> > + return -EINVAL;
>
> why have this parameter if it is always 4?
>
> > + buf[0] = 0;
> > + buf[1] = s->factset1 & (MOD1_IICADDR_MASK | MOD1_RES_MASK);
> > + buf[1] |= regs[1] & ~(MOD1_P | MOD1_IICADDR_MASK | MOD1_RES_MASK);
> > + buf[2] = s->factset2;
> > + buf[3] = MOD2_PT | (s->factset3 & 0x1f);
> > + buf[3] |= regs[3] & ~(MOD2_PT | 0x1f);
> > +
> > + if ((buf[3] & MOD2_PT) &&
> > + !tlv493d_parity((buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3]))
> > + buf[1] |= MOD1_P;
> > +
> > + ret = i2c_master_send(s->i2c, buf, 4);
>
> use sizeof(buf) instead of 4?
>
> > + if (ret < 4) {
> > + dev_err(&s->i2c->dev, "failed to write registers (%d)", ret);
> > + return ret;
If you hit the partial send condition then you should change the ret to
something to indicate this as an error. -EIO perhaps.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int tlv493d_power_down(struct tlv493d *s)
> > +{
> > + u8 buf[4];
>
> maybe
> u8 buf[4] = {0,};
> > +
> > + buf[0] = 0;
> > + buf[1] = 0;
> > + buf[2] = 0;
> > + buf[3] = 0;
> > + return tlv493d_write_regs(s, buf, 4);
> > +}
> > +
> > +static const struct iio_mount_matrix *
> > +tlv493d_get_mount_matrix(const struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan)
> > +{
> > + struct tlv493d *s = iio_priv(indio_dev);
> > +
> > + return &s->mount_matrix;
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info tlv493d_ext_info[] = {
> > + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, tlv493d_get_mount_matrix),
> > + {}
> > +};
> > +
> > +#define TLV493D_AXIS_CHANNEL(axis, index) \
> > + { \
> > + .type = IIO_MAGN, \
> > + .channel2 = IIO_MOD_##axis, \
> > + .modified = 1, \
> > + .address = index, \
> > + .scan_index = index, \
> > + .scan_type = { \
>
> IIO buffer reads are not supported by the driver, so no need for
> .scan_type (here and for _TEMP)
>
> > + .sign = 's', \
> > + .realbits = 12, \
> > + .storagebits = 16, \
> > + .endianness = IIO_CPU, \
> > + }, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_PROCESSED), \
>
> either _RAW or _PROCESSED, not both
>
> > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> > + .ext_info = tlv493d_ext_info, \
> > + }
> > +
> > +#define TLV493D_TEMP_CHANNEL(index) \
> > + { \
> > + .type = IIO_TEMP, \
> > + .channel2 = IIO_MOD_TEMP_OBJECT, \
> > + .modified = 1, \
> > + .address = index, \
> > + .scan_index = index, \
> > + .scan_type = { \
> > + .sign = 's', \
> > + .realbits = 12, \
> > + .storagebits = 16, \
> > + .endianness = IIO_CPU, \
> > + }, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_OFFSET) | \
> > + BIT(IIO_CHAN_INFO_SCALE) | \
> > + BIT(IIO_CHAN_INFO_PROCESSED), \
> > + }
> > +
> > +static const struct iio_chan_spec tlv493d_channels[] = {
> > + TLV493D_AXIS_CHANNEL(X, 0),
> > + TLV493D_AXIS_CHANNEL(Y, 1),
> > + TLV493D_AXIS_CHANNEL(Z, 2),
> > + TLV493D_TEMP_CHANNEL(3),
> > + IIO_CHAN_SOFT_TIMESTAMP(4),
> > +};
> > +
> > +static const unsigned long tlv493d_scan_masks[] = { 0xf, 0 };
> > +
> > +static int tlv493d_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct tlv493d *s = iio_priv(indio_dev);
> > + u8 buf[7];
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + case IIO_CHAN_INFO_RAW:
>
> this is ugly, why support _PROCESSED?
> just _RAW is fine
And preferred. Depending on what userspace is doing with
the values, it may be able to work in the _raw units directly
and avoid the cost of the conversions.
>
> > + do {
> > + ret = tlv493d_read_regs(s, buf, 7);
> > + if (ret)
> > + return ret;
> > + } while ((buf[3] & 0x3) || (buf[5] & BIT(4)));
>
> maybe add a timeout or limit the number of retries
>
> > +
> > + switch (chan->address) {
> > + case 0:
Wow. That is novel data arranging. You have my sympathies!
> > + *val = (((int)(s8)buf[0]) << 4) | (buf[4] >> 4);
> > + break;
> > + case 1:
> > + *val = (((int)(s8)buf[1]) << 4) | (buf[4] & 0xf);
> > + break;
> > + case 2:
> > + *val = (((int)(s8)buf[2]) << 4) | (buf[5] & 0xf);
> > + break;
> > + case 3:
> > + *val = (((int)(s8)(buf[3] & 0xf0)) << 4) | buf[6];
> > + if (mask != IIO_CHAN_INFO_RAW)
> > + *val -= 340;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + *val2 = 0;
>
> no need to set val2 if returning VAL_INT
>
> > + if (mask == IIO_CHAN_INFO_RAW)
> > + return IIO_VAL_INT;
> > +
> > + switch (chan->type) {
> > + case IIO_MAGN:
> > + *val2 = (*val * 1000000) * 98 / 1000;
> > + *val = *val2 / 1000000;
> > + *val2 %= 1000000;
> > + break;
> > + case IIO_TEMP:
> > + *val2 = (*val * 1000000) * 11 / 10;
> > + *val = *val2 / 1000000;
> > + *val2 %= 1000000;
> > + /* According to datasheet, LSB offset is for 25°C */
> > + *val += 25;
> > + break;
We shouldn't be exporting processed values to userspace unless we can't
well represent them using the _raw, _scale, _offset triplet.
It's up to userspace tools to do the conversion if they wish to.
> > + default:
> > + return -EINVAL;
> > + }
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_CHAN_INFO_OFFSET:
> > + switch (chan->type) {
> > + case IIO_TEMP:
> > + *val = -340;
> > + *val2 = 0;
>
> no need to set val2
>
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_SCALE:
> > + switch (chan->type) {
> > + case IIO_MAGN:
> > + /* 0.098 mT/LSB */
Units for magnetic field in IIO are Gauss.
(see doc referenced below).
> > + *val = 0;
> > + *val2 = 98000;
> > + return IIO_VAL_INT_PLUS_MICRO;
> > + case IIO_TEMP:
> > + /* 1.1 °C/LSB */
> > + *val = 1;
> > + *val2 = 100000;
> > + return IIO_VAL_INT_PLUS_MICRO;
See Documentation/ABI/testing/sysfs-bus-iio.
For historical reasons (we copied hwmon) temperature units after
scaling need to be micro degrees Celsius.
It is something I wish we had differently (because it is inconsistent)
but we are stuck with it.
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct iio_info tlv493d_iio_info = {
> > + .read_raw = tlv493d_read_raw,
> > +};
> > +
> > +static int tlv493d_probe(struct i2c_client *i2c)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct tlv493d *tlv493d;
> > + u8 buf[10];
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*tlv493d));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + tlv493d = iio_priv(indio_dev);
> > + i2c_set_clientdata(i2c, indio_dev);
> > + tlv493d->i2c = i2c;
> > +
> > + ret = of_iio_read_mount_matrix(&i2c->dev, "mount-matrix",
> > + &tlv493d->mount_matrix);
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->dev.parent = &i2c->dev;
> > + indio_dev->channels = tlv493d_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(tlv493d_channels);
> > + indio_dev->info = &tlv493d_iio_info;
> > + indio_dev->available_scan_masks = tlv493d_scan_masks;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->name = "tlv493d";
> > +
> > + ret = tlv493d_read_regs(tlv493d, buf, 10);
> > + if (ret)
> > + return ret;
> > +
> > + tlv493d->factset1 = buf[7];
> > + tlv493d->factset2 = buf[8];
> > + tlv493d->factset3 = buf[9];
> > +
> > + buf[0] = 0;
> > + buf[1] = MOD1_FAST | MOD1_LOW;
>
> what does this mean? some comment explaining the default mode?
>
> > + buf[2] = 0;
> > + buf[3] = 0;
> > + ret = tlv493d_write_regs(tlv493d, buf, 4);
> > + if (ret)
> > + return ret;
> > +
> > + ret = iio_device_register(indio_dev);
> > + if (ret) {
> > + dev_err(&i2c->dev, "device registration failed");
> > + tlv493d_power_down(tlv493d);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int tlv493d_remove(struct i2c_client *i2c)
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
> > + struct tlv493d *tlv493d = iio_priv(indio_dev);
> > +
> > + iio_device_unregister(indio_dev);
> > +
> > + tlv493d_power_down(tlv493d);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id tlv493d_dt_ids[] = {
> > + { .compatible = "infineon,tlv493d-a1b6" },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, tlv493d_dt_ids);
> > +
> > +static struct i2c_driver tlv493d_i2c_driver = {
> > + .driver = {
> > + .name = "tlv493d",
> > + .of_match_table = tlv493d_dt_ids,
> > + },
> > + .probe_new = tlv493d_probe,
> > + .remove = tlv493d_remove,
> > +};
> > +
> > +module_i2c_driver(tlv493d_i2c_driver);
> > +
> > +MODULE_DESCRIPTION("TLV493D I2C driver");
> > +MODULE_AUTHOR("Andreas Faerber");
> > +MODULE_LICENSE("GPL");
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: magnetometer: Add Infineon TLV493D-A1B6
2017-12-10 9:08 ` Peter Meerwald-Stadler
2017-12-10 17:11 ` Jonathan Cameron
@ 2017-12-10 17:14 ` Andreas Färber
2017-12-10 17:48 ` Jonathan Cameron
1 sibling, 1 reply; 9+ messages in thread
From: Andreas Färber @ 2017-12-10 17:14 UTC (permalink / raw)
To: Peter Meerwald-Stadler
Cc: linux-iio, linux-kernel, Marius Tarcatu, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen
Hi,
Am 10.12.2017 um 10:08 schrieb Peter Meerwald-Stadler:
>> The Infineon TLV493D is a 3D magnetic sensor, A1B6 is I2C based.
>
> comments below
>
>> It is found among others on the Infineon 3D Magnetic Sensor 2Go Kit,
>> which allows to detach the sensor as a breakout board.
>>
>> Cc: Marius Tarcatu <marius.tarcatu@infineon.com>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>> drivers/iio/magnetometer/Kconfig | 9 +
>> drivers/iio/magnetometer/Makefile | 2 +
>> drivers/iio/magnetometer/tlv493d.c | 328 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 339 insertions(+)
>> create mode 100644 drivers/iio/magnetometer/tlv493d.c
>>
>> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
>> index ed9d776d01af..5945d88a1595 100644
>> --- a/drivers/iio/magnetometer/Kconfig
>> +++ b/drivers/iio/magnetometer/Kconfig
>> @@ -175,4 +175,13 @@ config SENSORS_HMC5843_SPI
>> - hmc5843_core (core functions)
>> - hmc5843_spi (support for HMC5983)
>>
>> +config TLV493D
>> + tristate "Infineon TLV493D 3D Magnetic Sensor"
>> + depends on I2C
>> + help
>> + Say Y here to build support for Infineon TLV493D-A1B6 I2C-based
>> + 3-axis magnetometer.
>> +
>> + This driver can also be built as a module and will be called tlv493d.
>> +
>> endmenu
>> diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
>> index 664b2f866472..df6ad23fee65 100644
>> --- a/drivers/iio/magnetometer/Makefile
>> +++ b/drivers/iio/magnetometer/Makefile
>> @@ -24,3 +24,5 @@ obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
>> obj-$(CONFIG_SENSORS_HMC5843) += hmc5843_core.o
>> obj-$(CONFIG_SENSORS_HMC5843_I2C) += hmc5843_i2c.o
>> obj-$(CONFIG_SENSORS_HMC5843_SPI) += hmc5843_spi.o
>> +
>> +obj-$(CONFIG_TLV493D) += tlv493d.o
>> diff --git a/drivers/iio/magnetometer/tlv493d.c b/drivers/iio/magnetometer/tlv493d.c
>> new file mode 100644
>> index 000000000000..d2fe296b2c80
>> --- /dev/null
>> +++ b/drivers/iio/magnetometer/tlv493d.c
>> @@ -0,0 +1,328 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Infineon TLV493D-A1B6 3D magnetic sensor
>
> link to datasheet would be nice
https://www.infineon.com/cms/en/product/sensor/magnetic-position-sensor/3d-magnetic-sensor/tlv493d-a1b6/
Datasheet:
https://www.infineon.com/dgdl/Infineon-TLV493D-A1B6-DS-v01_00-EN.pdf?fileId=5546d462525dbac40152a6b85c760e80
User Manual:
https://www.infineon.com/dgdl/Infineon-TLV493D-A1B6_UM-UM-v01_02-EN.PDF?fileId=5546d462525dbac401530e81c9db058c
Not sure how stable the latter links are.
>
>> + *
>> + * Copyright (c) 2016-2017 Andreas Färber
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/i2c.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +
>> +#define MOD1_LOW BIT(0)
>> +#define MOD1_FAST BIT(1)
>> +#define MOD1_INT BIT(2)
>> +#define MOD1_P BIT(7)
>> +
>> +#define MOD2_PT BIT(5)
>
> please use TLV493D_ prefix;
Okay.
> more descriptive naming or comments would be
> nice
I'm sure those were the official field names from the manual.
I can look into adding explanatory comments.
>> +struct tlv493d {
>> + struct i2c_client *i2c;
>> + struct iio_mount_matrix mount_matrix;
>> + u8 factset1, factset2, factset3;
>> +};
>> +
>> +static int tlv493d_read_regs(struct tlv493d *s, u8 *buf, int len)
>> +{
>> + int ret;
>> +
>> + ret = i2c_master_recv(s->i2c, buf, len);
>> + if (ret < len) {
>> + dev_err(&s->i2c->dev, "failed to read registers (%d)", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static unsigned int tlv493d_parity(u32 v)
>> +{
>> + v ^= v >> 16;
>> + v ^= v >> 8;
>> + v ^= v >> 4;
>> + v &= 0xf;
>> + return (0x6996 >> v) & 1;
>> +}
>> +
>> +#define MOD1_RES_MASK (0x3 << 3)
>> +#define MOD1_IICADDR_MASK (0x3 << 5)
>
> maybe move all these #defines on top of the file?
Yeah, missed those while cleaning the code up.
>> +static int tlv493d_write_regs(struct tlv493d *s, const u8 *regs, int len)
>> +{
>> + u8 buf[4];
>> + int ret;
>> +
>> + if (len != ARRAY_SIZE(buf))
>> + return -EINVAL;
>
> why have this parameter if it is always 4?
I originally had it as a BUG_ON() but checkpatch.pl complained about it.
const u8 *regs alone does not give us guarantees about its length, and
to stay close to the manual I didn't want to mess with u32 and
endianness. I'll have to re-check the manual where the limit stems from.
>> + buf[0] = 0;
>> + buf[1] = s->factset1 & (MOD1_IICADDR_MASK | MOD1_RES_MASK);
>> + buf[1] |= regs[1] & ~(MOD1_P | MOD1_IICADDR_MASK | MOD1_RES_MASK);
>> + buf[2] = s->factset2;
>> + buf[3] = MOD2_PT | (s->factset3 & 0x1f);
>> + buf[3] |= regs[3] & ~(MOD2_PT | 0x1f);
>> +
>> + if ((buf[3] & MOD2_PT) &&
>> + !tlv493d_parity((buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3]))
>> + buf[1] |= MOD1_P;
>> +
>> + ret = i2c_master_send(s->i2c, buf, 4);
>
> use sizeof(buf) instead of 4?
>
>> + if (ret < 4) {
>> + dev_err(&s->i2c->dev, "failed to write registers (%d)", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tlv493d_power_down(struct tlv493d *s)
>> +{
>> + u8 buf[4];
>
> maybe
> u8 buf[4] = {0,};
>> +
>> + buf[0] = 0;
>> + buf[1] = 0;
>> + buf[2] = 0;
>> + buf[3] = 0;
>> + return tlv493d_write_regs(s, buf, 4);
>> +}
>> +
>> +static const struct iio_mount_matrix *
>> +tlv493d_get_mount_matrix(const struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan)
>> +{
>> + struct tlv493d *s = iio_priv(indio_dev);
>> +
>> + return &s->mount_matrix;
>> +}
>> +
>> +static const struct iio_chan_spec_ext_info tlv493d_ext_info[] = {
>> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, tlv493d_get_mount_matrix),
>> + {}
>> +};
>> +
>> +#define TLV493D_AXIS_CHANNEL(axis, index) \
>> + { \
>> + .type = IIO_MAGN, \
>> + .channel2 = IIO_MOD_##axis, \
>> + .modified = 1, \
>> + .address = index, \
>> + .scan_index = index, \
>> + .scan_type = { \
>
> IIO buffer reads are not supported by the driver, so no need for
> .scan_type (here and for _TEMP)
Hmm, I wanted to get the code out before it catches too much dust here,
I might revisit IIO buffers later.
The general problem I've run into is that the values vary quite a bit
between readings, and the iio API seemed to force me into doing I2C
readings per channel. Do you see a better way (any example?) to read the
three channels once and then return the three axes from one consistent
reading?
>> + .sign = 's', \
>> + .realbits = 12, \
>> + .storagebits = 16, \
>> + .endianness = IIO_CPU, \
>> + }, \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>> + BIT(IIO_CHAN_INFO_PROCESSED), \
>
> either _RAW or _PROCESSED, not both
Can you explain please? I tested this via sysfs and tools and could
access both.
>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
>> + .ext_info = tlv493d_ext_info, \
>> + }
>> +
>> +#define TLV493D_TEMP_CHANNEL(index) \
>> + { \
>> + .type = IIO_TEMP, \
>> + .channel2 = IIO_MOD_TEMP_OBJECT, \
>> + .modified = 1, \
>> + .address = index, \
>> + .scan_index = index, \
>> + .scan_type = { \
>> + .sign = 's', \
>> + .realbits = 12, \
>> + .storagebits = 16, \
>> + .endianness = IIO_CPU, \
>> + }, \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>> + BIT(IIO_CHAN_INFO_OFFSET) | \
>> + BIT(IIO_CHAN_INFO_SCALE) | \
>> + BIT(IIO_CHAN_INFO_PROCESSED), \
>> + }
>> +
>> +static const struct iio_chan_spec tlv493d_channels[] = {
>> + TLV493D_AXIS_CHANNEL(X, 0),
>> + TLV493D_AXIS_CHANNEL(Y, 1),
>> + TLV493D_AXIS_CHANNEL(Z, 2),
>> + TLV493D_TEMP_CHANNEL(3),
>> + IIO_CHAN_SOFT_TIMESTAMP(4),
>> +};
>> +
>> +static const unsigned long tlv493d_scan_masks[] = { 0xf, 0 };
>> +
>> +static int tlv493d_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct tlv493d *s = iio_priv(indio_dev);
>> + u8 buf[7];
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_PROCESSED:
>> + case IIO_CHAN_INFO_RAW:
>
> this is ugly, why support _PROCESSED?
> just _RAW is fine
See below...
>
>> + do {
>> + ret = tlv493d_read_regs(s, buf, 7);
>> + if (ret)
>> + return ret;
>> + } while ((buf[3] & 0x3) || (buf[5] & BIT(4)));
>
> maybe add a timeout or limit the number of retries
>
>> +
>> + switch (chan->address) {
>> + case 0:
>> + *val = (((int)(s8)buf[0]) << 4) | (buf[4] >> 4);
>> + break;
>> + case 1:
>> + *val = (((int)(s8)buf[1]) << 4) | (buf[4] & 0xf);
>> + break;
>> + case 2:
>> + *val = (((int)(s8)buf[2]) << 4) | (buf[5] & 0xf);
>> + break;
>> + case 3:
>> + *val = (((int)(s8)(buf[3] & 0xf0)) << 4) | buf[6];
>> + if (mask != IIO_CHAN_INFO_RAW)
>> + *val -= 340;
Here an offset is being applied to turn the temp value into something
real. More differences below.
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + *val2 = 0;
>
> no need to set val2 if returning VAL_INT
Okay.
>> + if (mask == IIO_CHAN_INFO_RAW)
>> + return IIO_VAL_INT;
Here the RAW value is returned, whereas some more corrections are
applied to the values below. The raw value was mostly for debugging or
if someone wants to apply different correction factors.
>> +
>> + switch (chan->type) {
>> + case IIO_MAGN:
>> + *val2 = (*val * 1000000) * 98 / 1000;
>> + *val = *val2 / 1000000;
>> + *val2 %= 1000000;
>> + break;
>> + case IIO_TEMP:
>> + *val2 = (*val * 1000000) * 11 / 10;
>> + *val = *val2 / 1000000;
>> + *val2 %= 1000000;
>> + /* According to datasheet, LSB offset is for 25°C */
>> + *val += 25;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + return IIO_VAL_INT_PLUS_MICRO;
>> + case IIO_CHAN_INFO_OFFSET:
>> + switch (chan->type) {
>> + case IIO_TEMP:
>> + *val = -340;
>> + *val2 = 0;
>
> no need to set val2
Okay.
>> + return IIO_VAL_INT;
>> + default:
>> + return -EINVAL;
>> + }
>> + case IIO_CHAN_INFO_SCALE:
>> + switch (chan->type) {
>> + case IIO_MAGN:
>> + /* 0.098 mT/LSB */
>> + *val = 0;
>> + *val2 = 98000;
>> + return IIO_VAL_INT_PLUS_MICRO;
>> + case IIO_TEMP:
>> + /* 1.1 °C/LSB */
>> + *val = 1;
>> + *val2 = 100000;
>> + return IIO_VAL_INT_PLUS_MICRO;
>> + default:
>> + return -EINVAL;
>> + }
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static const struct iio_info tlv493d_iio_info = {
>> + .read_raw = tlv493d_read_raw,
>> +};
>> +
>> +static int tlv493d_probe(struct i2c_client *i2c)
>> +{
>> + struct iio_dev *indio_dev;
>> + struct tlv493d *tlv493d;
>> + u8 buf[10];
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*tlv493d));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + tlv493d = iio_priv(indio_dev);
>> + i2c_set_clientdata(i2c, indio_dev);
>> + tlv493d->i2c = i2c;
>> +
>> + ret = of_iio_read_mount_matrix(&i2c->dev, "mount-matrix",
>> + &tlv493d->mount_matrix);
>> + if (ret)
>> + return ret;
>> +
>> + indio_dev->dev.parent = &i2c->dev;
>> + indio_dev->channels = tlv493d_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(tlv493d_channels);
>> + indio_dev->info = &tlv493d_iio_info;
>> + indio_dev->available_scan_masks = tlv493d_scan_masks;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> + indio_dev->name = "tlv493d";
>> +
>> + ret = tlv493d_read_regs(tlv493d, buf, 10);
>> + if (ret)
>> + return ret;
>> +
>> + tlv493d->factset1 = buf[7];
>> + tlv493d->factset2 = buf[8];
>> + tlv493d->factset3 = buf[9];
>> +
>> + buf[0] = 0;
>> + buf[1] = MOD1_FAST | MOD1_LOW;
>
> what does this mean? some comment explaining the default mode?
AFAIR the chip can operate in some non-standard-I2C modes (~four), but
it's been some time since I wrote this... I'll dig into it.
>> + buf[2] = 0;
>> + buf[3] = 0;
>> + ret = tlv493d_write_regs(tlv493d, buf, 4);
>> + if (ret)
>> + return ret;
>> +
>> + ret = iio_device_register(indio_dev);
>> + if (ret) {
>> + dev_err(&i2c->dev, "device registration failed");
>> + tlv493d_power_down(tlv493d);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int tlv493d_remove(struct i2c_client *i2c)
>> +{
>> + struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
>> + struct tlv493d *tlv493d = iio_priv(indio_dev);
>> +
>> + iio_device_unregister(indio_dev);
>> +
>> + tlv493d_power_down(tlv493d);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id tlv493d_dt_ids[] = {
>> + { .compatible = "infineon,tlv493d-a1b6" },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, tlv493d_dt_ids);
>> +
>> +static struct i2c_driver tlv493d_i2c_driver = {
>> + .driver = {
>> + .name = "tlv493d",
>> + .of_match_table = tlv493d_dt_ids,
>> + },
>> + .probe_new = tlv493d_probe,
>> + .remove = tlv493d_remove,
>> +};
>> +
>> +module_i2c_driver(tlv493d_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("TLV493D I2C driver");
>> +MODULE_AUTHOR("Andreas Faerber");
>> +MODULE_LICENSE("GPL");
Thanks for your quick review,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: magnetometer: Add Infineon TLV493D-A1B6
2017-12-10 17:14 ` Andreas Färber
@ 2017-12-10 17:48 ` Jonathan Cameron
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2017-12-10 17:48 UTC (permalink / raw)
To: Andreas Färber
Cc: Peter Meerwald-Stadler, linux-iio, linux-kernel, Marius Tarcatu,
Hartmut Knaack, Lars-Peter Clausen
On Sun, 10 Dec 2017 18:14:44 +0100
Andreas Färber <afaerber@suse.de> wrote:
> Hi,
Some quick notes inline.
Jonathan
>
> Am 10.12.2017 um 10:08 schrieb Peter Meerwald-Stadler:
> >> The Infineon TLV493D is a 3D magnetic sensor, A1B6 is I2C based.
> >
> > comments below
> >
> >> It is found among others on the Infineon 3D Magnetic Sensor 2Go Kit,
> >> which allows to detach the sensor as a breakout board.
> >>
> >> Cc: Marius Tarcatu <marius.tarcatu@infineon.com>
> >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> ---
> >> drivers/iio/magnetometer/Kconfig | 9 +
> >> drivers/iio/magnetometer/Makefile | 2 +
> >> drivers/iio/magnetometer/tlv493d.c | 328 +++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 339 insertions(+)
> >> create mode 100644 drivers/iio/magnetometer/tlv493d.c
> >>
> >> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> >> index ed9d776d01af..5945d88a1595 100644
> >> --- a/drivers/iio/magnetometer/Kconfig
> >> +++ b/drivers/iio/magnetometer/Kconfig
> >> @@ -175,4 +175,13 @@ config SENSORS_HMC5843_SPI
> >> - hmc5843_core (core functions)
> >> - hmc5843_spi (support for HMC5983)
> >>
> >> +config TLV493D
> >> + tristate "Infineon TLV493D 3D Magnetic Sensor"
> >> + depends on I2C
> >> + help
> >> + Say Y here to build support for Infineon TLV493D-A1B6 I2C-based
> >> + 3-axis magnetometer.
> >> +
> >> + This driver can also be built as a module and will be called tlv493d.
> >> +
> >> endmenu
> >> diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
> >> index 664b2f866472..df6ad23fee65 100644
> >> --- a/drivers/iio/magnetometer/Makefile
> >> +++ b/drivers/iio/magnetometer/Makefile
> >> @@ -24,3 +24,5 @@ obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
> >> obj-$(CONFIG_SENSORS_HMC5843) += hmc5843_core.o
> >> obj-$(CONFIG_SENSORS_HMC5843_I2C) += hmc5843_i2c.o
> >> obj-$(CONFIG_SENSORS_HMC5843_SPI) += hmc5843_spi.o
> >> +
> >> +obj-$(CONFIG_TLV493D) += tlv493d.o
> >> diff --git a/drivers/iio/magnetometer/tlv493d.c b/drivers/iio/magnetometer/tlv493d.c
> >> new file mode 100644
> >> index 000000000000..d2fe296b2c80
> >> --- /dev/null
> >> +++ b/drivers/iio/magnetometer/tlv493d.c
> >> @@ -0,0 +1,328 @@
> >> +// SPDX-License-Identifier: GPL-2.0+
> >> +/*
> >> + * Infineon TLV493D-A1B6 3D magnetic sensor
> >
> > link to datasheet would be nice
>
> https://www.infineon.com/cms/en/product/sensor/magnetic-position-sensor/3d-magnetic-sensor/tlv493d-a1b6/
>
> Datasheet:
> https://www.infineon.com/dgdl/Infineon-TLV493D-A1B6-DS-v01_00-EN.pdf?fileId=5546d462525dbac40152a6b85c760e80
>
> User Manual:
> https://www.infineon.com/dgdl/Infineon-TLV493D-A1B6_UM-UM-v01_02-EN.PDF?fileId=5546d462525dbac401530e81c9db058c
>
> Not sure how stable the latter links are.
>
> >
> >> + *
> >> + * Copyright (c) 2016-2017 Andreas Färber
> >> + */
> >> +
> >> +#include <linux/bitfield.h>
> >> +#include <linux/i2c.h>
> >> +#include <linux/iio/iio.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_device.h>
> >> +
> >> +#define MOD1_LOW BIT(0)
> >> +#define MOD1_FAST BIT(1)
> >> +#define MOD1_INT BIT(2)
> >> +#define MOD1_P BIT(7)
> >> +
> >> +#define MOD2_PT BIT(5)
> >
> > please use TLV493D_ prefix;
>
> Okay.
>
> > more descriptive naming or comments would be
> > nice
>
> I'm sure those were the official field names from the manual.
> I can look into adding explanatory comments.
>
> >> +struct tlv493d {
> >> + struct i2c_client *i2c;
> >> + struct iio_mount_matrix mount_matrix;
> >> + u8 factset1, factset2, factset3;
> >> +};
> >> +
> >> +static int tlv493d_read_regs(struct tlv493d *s, u8 *buf, int len)
> >> +{
> >> + int ret;
> >> +
> >> + ret = i2c_master_recv(s->i2c, buf, len);
> >> + if (ret < len) {
> >> + dev_err(&s->i2c->dev, "failed to read registers (%d)", ret);
> >> + return ret;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static unsigned int tlv493d_parity(u32 v)
> >> +{
> >> + v ^= v >> 16;
> >> + v ^= v >> 8;
> >> + v ^= v >> 4;
> >> + v &= 0xf;
> >> + return (0x6996 >> v) & 1;
> >> +}
> >> +
> >> +#define MOD1_RES_MASK (0x3 << 3)
> >> +#define MOD1_IICADDR_MASK (0x3 << 5)
> >
> > maybe move all these #defines on top of the file?
>
> Yeah, missed those while cleaning the code up.
>
> >> +static int tlv493d_write_regs(struct tlv493d *s, const u8 *regs, int len)
> >> +{
> >> + u8 buf[4];
> >> + int ret;
> >> +
> >> + if (len != ARRAY_SIZE(buf))
> >> + return -EINVAL;
> >
> > why have this parameter if it is always 4?
>
> I originally had it as a BUG_ON() but checkpatch.pl complained about it.
>
> const u8 *regs alone does not give us guarantees about its length, and
> to stay close to the manual I didn't want to mess with u32 and
> endianness. I'll have to re-check the manual where the limit stems from.
I think Peter was more meaning that we can do it by review.
This is fiddly but a static analyser such as smatch will probably
pick up any case where it was less than 4 and greater than 4 does
no harm (other than larger registers being ignored).
>
> >> + buf[0] = 0;
> >> + buf[1] = s->factset1 & (MOD1_IICADDR_MASK | MOD1_RES_MASK);
> >> + buf[1] |= regs[1] & ~(MOD1_P | MOD1_IICADDR_MASK | MOD1_RES_MASK);
> >> + buf[2] = s->factset2;
> >> + buf[3] = MOD2_PT | (s->factset3 & 0x1f);
> >> + buf[3] |= regs[3] & ~(MOD2_PT | 0x1f);
> >> +
> >> + if ((buf[3] & MOD2_PT) &&
> >> + !tlv493d_parity((buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3]))
> >> + buf[1] |= MOD1_P;
> >> +
> >> + ret = i2c_master_send(s->i2c, buf, 4);
> >
> > use sizeof(buf) instead of 4?
> >
> >> + if (ret < 4) {
> >> + dev_err(&s->i2c->dev, "failed to write registers (%d)", ret);
> >> + return ret;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int tlv493d_power_down(struct tlv493d *s)
> >> +{
> >> + u8 buf[4];
> >
> > maybe
> > u8 buf[4] = {0,};
> >> +
> >> + buf[0] = 0;
> >> + buf[1] = 0;
> >> + buf[2] = 0;
> >> + buf[3] = 0;
> >> + return tlv493d_write_regs(s, buf, 4);
> >> +}
> >> +
> >> +static const struct iio_mount_matrix *
> >> +tlv493d_get_mount_matrix(const struct iio_dev *indio_dev,
> >> + const struct iio_chan_spec *chan)
> >> +{
> >> + struct tlv493d *s = iio_priv(indio_dev);
> >> +
> >> + return &s->mount_matrix;
> >> +}
> >> +
> >> +static const struct iio_chan_spec_ext_info tlv493d_ext_info[] = {
> >> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, tlv493d_get_mount_matrix),
> >> + {}
> >> +};
> >> +
> >> +#define TLV493D_AXIS_CHANNEL(axis, index) \
> >> + { \
> >> + .type = IIO_MAGN, \
> >> + .channel2 = IIO_MOD_##axis, \
> >> + .modified = 1, \
> >> + .address = index, \
> >> + .scan_index = index, \
> >> + .scan_type = { \
> >
> > IIO buffer reads are not supported by the driver, so no need for
> > .scan_type (here and for _TEMP)
>
> Hmm, I wanted to get the code out before it catches too much dust here,
> I might revisit IIO buffers later.
>
> The general problem I've run into is that the values vary quite a bit
> between readings, and the iio API seemed to force me into doing I2C
> readings per channel. Do you see a better way (any example?) to read the
> three channels once and then return the three axes from one consistent
> reading?
Use the buffered interface. It's that simple - it is near impossible
to define a generic interface for getting a consistent set of values
across an unknown number of channels in sysfs. Unknown in the sense
that the the ABI definition doesn't know - obvious a given driver
does.
Sysfs is one value one attribute. There are odd well defined
multi element things such as quaternions but in those cases they really
aren't separable - with axis measurements they are.
>
> >> + .sign = 's', \
> >> + .realbits = 12, \
> >> + .storagebits = 16, \
> >> + .endianness = IIO_CPU, \
> >> + }, \
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> >> + BIT(IIO_CHAN_INFO_PROCESSED), \
> >
> > either _RAW or _PROCESSED, not both
>
> Can you explain please? I tested this via sysfs and tools and could
> access both.
Indeed. It's possible of course, however the ABI is not supposed
to be redundant. There are a few exceptions where we had existing
ABI due to say supporting raw only, but later it became apparent
a non linear conversion was needed so we ended up with both.
Given IIO also supports some high speed devices where in kernel conversion
is too costly, and others where is in accurate due to not having floating
point maths - the decision from the start years ago was to default to
_raw unless there was a reason we couldn't - typically non linear
conversions on light sensors.
>
> >> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> >> + .ext_info = tlv493d_ext_info, \
> >> + }
> >> +
> >> +#define TLV493D_TEMP_CHANNEL(index) \
> >> + { \
> >> + .type = IIO_TEMP, \
> >> + .channel2 = IIO_MOD_TEMP_OBJECT, \
> >> + .modified = 1, \
> >> + .address = index, \
> >> + .scan_index = index, \
> >> + .scan_type = { \
> >> + .sign = 's', \
> >> + .realbits = 12, \
> >> + .storagebits = 16, \
> >> + .endianness = IIO_CPU, \
> >> + }, \
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> >> + BIT(IIO_CHAN_INFO_OFFSET) | \
> >> + BIT(IIO_CHAN_INFO_SCALE) | \
> >> + BIT(IIO_CHAN_INFO_PROCESSED), \
> >> + }
> >> +
> >> +static const struct iio_chan_spec tlv493d_channels[] = {
> >> + TLV493D_AXIS_CHANNEL(X, 0),
> >> + TLV493D_AXIS_CHANNEL(Y, 1),
> >> + TLV493D_AXIS_CHANNEL(Z, 2),
> >> + TLV493D_TEMP_CHANNEL(3),
> >> + IIO_CHAN_SOFT_TIMESTAMP(4),
> >> +};
> >> +
> >> +static const unsigned long tlv493d_scan_masks[] = { 0xf, 0 };
> >> +
> >> +static int tlv493d_read_raw(struct iio_dev *indio_dev,
> >> + struct iio_chan_spec const *chan,
> >> + int *val, int *val2, long mask)
> >> +{
> >> + struct tlv493d *s = iio_priv(indio_dev);
> >> + u8 buf[7];
> >> + int ret;
> >> +
> >> + switch (mask) {
> >> + case IIO_CHAN_INFO_PROCESSED:
> >> + case IIO_CHAN_INFO_RAW:
> >
> > this is ugly, why support _PROCESSED?
> > just _RAW is fine
>
> See below...
>
> >
> >> + do {
> >> + ret = tlv493d_read_regs(s, buf, 7);
> >> + if (ret)
> >> + return ret;
> >> + } while ((buf[3] & 0x3) || (buf[5] & BIT(4)));
> >
> > maybe add a timeout or limit the number of retries
> >
> >> +
> >> + switch (chan->address) {
> >> + case 0:
> >> + *val = (((int)(s8)buf[0]) << 4) | (buf[4] >> 4);
> >> + break;
> >> + case 1:
> >> + *val = (((int)(s8)buf[1]) << 4) | (buf[4] & 0xf);
> >> + break;
> >> + case 2:
> >> + *val = (((int)(s8)buf[2]) << 4) | (buf[5] & 0xf);
> >> + break;
> >> + case 3:
> >> + *val = (((int)(s8)(buf[3] & 0xf0)) << 4) | buf[6];
> >> + if (mask != IIO_CHAN_INFO_RAW)
> >> + *val -= 340;
>
> Here an offset is being applied to turn the temp value into something
> real. More differences below.
Then represent it in _offset for the channel.
>
> >> + break;
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> + *val2 = 0;
> >
> > no need to set val2 if returning VAL_INT
>
> Okay.
>
> >> + if (mask == IIO_CHAN_INFO_RAW)
> >> + return IIO_VAL_INT;
>
> Here the RAW value is returned, whereas some more corrections are
> applied to the values below. The raw value was mostly for debugging or
> if someone wants to apply different correction factors.
The below can defintiely be represented as scale and offset so just
do that and stick to raw.
>
> >> +
> >> + switch (chan->type) {
> >> + case IIO_MAGN:
> >> + *val2 = (*val * 1000000) * 98 / 1000;
> >> + *val = *val2 / 1000000;
> >> + *val2 %= 1000000;
> >> + break;
> >> + case IIO_TEMP:
> >> + *val2 = (*val * 1000000) * 11 / 10;
> >> + *val = *val2 / 1000000;
> >> + *val2 %= 1000000;
> >> + /* According to datasheet, LSB offset is for 25°C */
> >> + *val += 25;
> >> + break;
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> + return IIO_VAL_INT_PLUS_MICRO;
> >> + case IIO_CHAN_INFO_OFFSET:
> >> + switch (chan->type) {
> >> + case IIO_TEMP:
> >> + *val = -340;
> >> + *val2 = 0;
> >
> > no need to set val2
>
> Okay.
>
> >> + return IIO_VAL_INT;
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> + case IIO_CHAN_INFO_SCALE:
> >> + switch (chan->type) {
> >> + case IIO_MAGN:
> >> + /* 0.098 mT/LSB */
> >> + *val = 0;
> >> + *val2 = 98000;
> >> + return IIO_VAL_INT_PLUS_MICRO;
> >> + case IIO_TEMP:
> >> + /* 1.1 °C/LSB */
> >> + *val = 1;
> >> + *val2 = 100000;
> >> + return IIO_VAL_INT_PLUS_MICRO;
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> +}
> >> +
> >> +static const struct iio_info tlv493d_iio_info = {
> >> + .read_raw = tlv493d_read_raw,
> >> +};
> >> +
> >> +static int tlv493d_probe(struct i2c_client *i2c)
> >> +{
> >> + struct iio_dev *indio_dev;
> >> + struct tlv493d *tlv493d;
> >> + u8 buf[10];
> >> + int ret;
> >> +
> >> + indio_dev = devm_iio_device_alloc(&i2c->dev, sizeof(*tlv493d));
> >> + if (!indio_dev)
> >> + return -ENOMEM;
> >> +
> >> + tlv493d = iio_priv(indio_dev);
> >> + i2c_set_clientdata(i2c, indio_dev);
> >> + tlv493d->i2c = i2c;
> >> +
> >> + ret = of_iio_read_mount_matrix(&i2c->dev, "mount-matrix",
> >> + &tlv493d->mount_matrix);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + indio_dev->dev.parent = &i2c->dev;
> >> + indio_dev->channels = tlv493d_channels;
> >> + indio_dev->num_channels = ARRAY_SIZE(tlv493d_channels);
> >> + indio_dev->info = &tlv493d_iio_info;
> >> + indio_dev->available_scan_masks = tlv493d_scan_masks;
> >> + indio_dev->modes = INDIO_DIRECT_MODE;
> >> + indio_dev->name = "tlv493d";
> >> +
> >> + ret = tlv493d_read_regs(tlv493d, buf, 10);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + tlv493d->factset1 = buf[7];
> >> + tlv493d->factset2 = buf[8];
> >> + tlv493d->factset3 = buf[9];
> >> +
> >> + buf[0] = 0;
> >> + buf[1] = MOD1_FAST | MOD1_LOW;
> >
> > what does this mean? some comment explaining the default mode?
>
> AFAIR the chip can operate in some non-standard-I2C modes (~four), but
> it's been some time since I wrote this... I'll dig into it.
>
> >> + buf[2] = 0;
> >> + buf[3] = 0;
> >> + ret = tlv493d_write_regs(tlv493d, buf, 4);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = iio_device_register(indio_dev);
> >> + if (ret) {
> >> + dev_err(&i2c->dev, "device registration failed");
> >> + tlv493d_power_down(tlv493d);
> >> + return ret;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int tlv493d_remove(struct i2c_client *i2c)
> >> +{
> >> + struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
> >> + struct tlv493d *tlv493d = iio_priv(indio_dev);
> >> +
> >> + iio_device_unregister(indio_dev);
> >> +
> >> + tlv493d_power_down(tlv493d);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static const struct of_device_id tlv493d_dt_ids[] = {
> >> + { .compatible = "infineon,tlv493d-a1b6" },
> >> + {}
> >> +};
> >> +MODULE_DEVICE_TABLE(of, tlv493d_dt_ids);
> >> +
> >> +static struct i2c_driver tlv493d_i2c_driver = {
> >> + .driver = {
> >> + .name = "tlv493d",
> >> + .of_match_table = tlv493d_dt_ids,
> >> + },
> >> + .probe_new = tlv493d_probe,
> >> + .remove = tlv493d_remove,
> >> +};
> >> +
> >> +module_i2c_driver(tlv493d_i2c_driver);
> >> +
> >> +MODULE_DESCRIPTION("TLV493D I2C driver");
> >> +MODULE_AUTHOR("Andreas Faerber");
> >> +MODULE_LICENSE("GPL");
>
> Thanks for your quick review,
>
> Andreas
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] dt-bindings: Add Infineon TLV493D-A1B6
2017-12-09 23:25 ` [PATCH 1/2] dt-bindings: Add Infineon TLV493D-A1B6 Andreas Färber
@ 2017-12-15 21:42 ` Rob Herring
0 siblings, 0 replies; 9+ messages in thread
From: Rob Herring @ 2017-12-15 21:42 UTC (permalink / raw)
To: Andreas Färber
Cc: linux-iio, linux-kernel, Marius Tarcatu, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
On Sun, Dec 10, 2017 at 12:25:06AM +0100, Andreas Färber wrote:
> The Infineon TLV493D-A1B6 is an I2C-based 3D Magnetic Sensor.
>
> Cc: Marius Tarcatu <marius.tarcatu@infineon.com>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> Documentation/devicetree/bindings/trivial-devices.txt | 1 +
> 1 file changed, 1 insertion(+)
I take trivial-devices.txt changes to avoid any conflicts, so I've
applied.
Rob
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-12-15 21:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-09 23:25 [PATCH 0/2] iio: magnetometer: Infineon TLV493D-A1B6 support Andreas Färber
2017-12-09 23:25 ` [PATCH 1/2] dt-bindings: Add Infineon TLV493D-A1B6 Andreas Färber
2017-12-15 21:42 ` Rob Herring
2017-12-09 23:25 ` [PATCH 2/2] iio: magnetometer: " Andreas Färber
2017-12-10 8:50 ` Philippe Ombredanne
2017-12-10 9:08 ` Peter Meerwald-Stadler
2017-12-10 17:11 ` Jonathan Cameron
2017-12-10 17:14 ` Andreas Färber
2017-12-10 17:48 ` Jonathan Cameron
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).