The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
applications includes joysticks, control elements (white goods,
multifunction knops), or electric meters (anti tampering) and any
other application that requires accurate angular measurements at
low power consumptions.
The Sensor is configured over I2C, and as part of Sensor measurement
data it provides 3-Axis magnetic fields and temperature core measurement.
The driver supports raw value read and buffered input via external trigger
to allow streaming values with the same sensing timestamp.
The device can be configured in to different operating modes by optional
device-tree "mode" property. Also, the temperature sensing part requires
raw offset captured at 25°C and that can be specified by "temp-offset"
optional device-tree property.
While sensor has interrupt pin multiplexed with I2C SCL pin. But for bus
configurations interrupt(INT) is not recommended, unless timing constraints
between I2C data transfers and interrupt pulses are monitored and aligned.
The Sensor's I2C register map and mode information is described in product
User Manual[1].
Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
[1] https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf
Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
---
drivers/iio/magnetometer/Kconfig | 14 +
drivers/iio/magnetometer/Makefile | 2 +
drivers/iio/magnetometer/tlv493d.c | 606 +++++++++++++++++++++++++++++++++++++
3 files changed, 622 insertions(+)
diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index 3debf1320ad1..e0070dccc751 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -246,6 +246,20 @@ config SI7210
To compile this driver as a module, choose M here: the module
will be called si7210.
+config TLV493D
+ tristate "Infineon TLV493D Low-Power 3D Magnetic Sensor"
+ depends on I2C
+ select REGMAP_I2C
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ Say Y here to add support for the Infineon TLV493D-A1B6 Low-
+ Power 3D Megnetic Sensor.
+
+ This driver can also be compiled as a module.
+ To compile this driver as a module, choose M here: the module
+ will be called tlv493d.
+
config TI_TMAG5273
tristate "TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor"
depends on I2C
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index 9297723a97d8..39c62dd06db8 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -35,4 +35,6 @@ obj-$(CONFIG_SI7210) += si7210.o
obj-$(CONFIG_TI_TMAG5273) += tmag5273.o
+obj-$(CONFIG_TLV493D) += tlv493d.o
+
obj-$(CONFIG_YAMAHA_YAS530) += yamaha-yas530.o
diff --git a/drivers/iio/magnetometer/tlv493d.c b/drivers/iio/magnetometer/tlv493d.c
new file mode 100644
index 000000000000..f230d6409a4b
--- /dev/null
+++ b/drivers/iio/magnetometer/tlv493d.c
@@ -0,0 +1,606 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/**
+ * Driver for the Infineon TLV493D Low-Power 3D Magnetic Sensor
+ *
+ * Copyright (C) 2025 Dixit Parmar <dixitparmar19@gmail.com>
+ *
+ */
+
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define TLV493D_RD_REG_BX 0x00
+#define TLV493D_RD_REG_BY 0x01
+#define TLV493D_RD_REG_BZ 0x02
+#define TLV493D_RD_REG_TEMP 0x03
+#define TLV493D_RD_REG_BX2 0x04
+#define TLV493D_RD_REG_BZ2 0x05
+#define TLV493D_RD_REG_TEMP2 0x06
+#define TLV493D_RD_REG_RES1 0x07
+#define TLV493D_RD_REG_RES2 0x08
+#define TLV493D_RD_REG_RES3 0x09
+#define TLV493D_RD_REG_MAX 0x0a
+#define TLV493D_WR_REG_RES 0x00
+#define TLV493D_WR_REG_MODE1 0x01
+#define TLV493D_WR_REG_RES2 0x02
+#define TLV493D_WR_REG_MODE2 0x03
+#define TLV493D_WR_REG_MAX 0x04
+#define TLV493D_VAL_MAG_X_AXIS_MSB GENMASK(7, 0)
+#define TLV493D_VAL_MAG_X_AXIS_LSB GENMASK(7, 4)
+#define TLV493D_VAL_MAG_Y_AXIS_MSB GENMASK(7, 0)
+#define TLV493D_VAL_MAG_Y_AXIS_LSB GENMASK(3, 0)
+#define TLV493D_VAL_MAG_Z_AXIS_MSB GENMASK(7, 0)
+#define TLV493D_VAL_MAG_Z_AXIS_LSB GENMASK(3, 0)
+#define TLV493D_VAL_TEMP_MSB GENMASK(7, 4)
+#define TLV493D_VAL_TEMP_LSB GENMASK(7, 0)
+#define TLV493D_VAL_FRAME_COUNTER GENMASK(3, 2)
+#define TLV493D_VAL_CHANNEL GENMASK(1, 0)
+#define TLV493D_VAL_PD_FLAG BIT(4)
+#define TLV493D_RD_REG_RES1_WR_MASK GENMASK(4, 3)
+#define TLV493D_RD_REG_RES2_WR_MASK GENMASK(7, 0)
+#define TLV493D_RD_REG_RES3_WR_MASK GENMASK(4, 0)
+#define TLV493D_MODE1_MOD_FAST BIT(1)
+#define TLV493D_MODE1_MOD_LOW BIT(0)
+#define TLV493D_MODE2_TEMP_CTRL BIT(7)
+#define TLV493D_MODE2_LP_PERIOD BIT(6)
+#define TLV493D_MODE2_PARITY_CTRL BIT(5)
+
+#define SET_BIT(b, bit) (b |= bit)
+#define CLR_BIT(b, bit) (b &= ~bit)
+
+#define TLV493D_DATA_X_GET(b) \
+ sign_extend32(FIELD_GET(TLV493D_VAL_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 | \
+ (FIELD_GET(TLV493D_VAL_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4), 11)
+#define TLV493D_DATA_Y_GET(b) \
+ sign_extend32(FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 | \
+ FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]), 11)
+#define TLV493D_DATA_Z_GET(b) \
+ sign_extend32(FIELD_GET(TLV493D_VAL_MAG_Z_AXIS_MSB, b[TLV493D_RD_REG_BZ]) << 4 | \
+ FIELD_GET(TLV493D_VAL_MAG_Z_AXIS_LSB, b[TLV493D_RD_REG_BZ2]), 11)
+#define TLV493D_DATA_TEMP_GET(b) \
+ sign_extend32(FIELD_GET(TLV493D_VAL_TEMP_MSB, b[TLV493D_RD_REG_TEMP]) << 8 | \
+ FIELD_GET(TLV493D_VAL_TEMP_LSB, b[TLV493D_RD_REG_TEMP2]), 11)
+
+enum tlv493d_channels {
+ AXIS_X = 0,
+ AXIS_Y,
+ AXIS_Z,
+ TEMPERATURE,
+};
+
+enum tlv493d_op_mode {
+ TLV493D_OP_MODE_POWERDOWN = 0,
+ TLV493D_OP_MODE_FAST,
+ TLV493D_OP_MODE_LOWPOWER,
+ TLV493D_OP_MODE_ULTRA_LOWPOWER,
+ TLV493D_OP_MODE_MASTERCONTROLLED,
+ TLV493D_OP_MODE_MAX,
+};
+
+struct tlv493d_mode {
+ u8 m;
+ u32 sleep_us;
+};
+
+struct tlv493d_data {
+ struct device *dev;
+ struct i2c_client *client;
+ struct mutex lock;
+ struct regmap *map;
+ u8 mode;
+ u8 wr_regs[TLV493D_WR_REG_MAX];
+ s32 temp_offset;
+};
+
+/*
+ * Different mode has different measurement cycle time, this time is
+ * used in deriving the sleep and timemout while reading the data from
+ * sensor in polling.
+ * Power-down mode: No measurement.
+ * Fast mode: Freq:3.3 KHz. Measurement time:305 usec.
+ * Low-power mode: Freq:100 Hz. Measurement time:10 msec.
+ * Ultra low-power mode: Freq:10 Hz. Measurement time:100 msec.
+ * Master controlled mode: Freq:3.3 Khz. Measurement time:305 usec.
+ */
+static struct tlv493d_mode modes[TLV493D_OP_MODE_MAX] = {
+ {.m = TLV493D_OP_MODE_POWERDOWN, .sleep_us = 0 },
+ {.m = TLV493D_OP_MODE_FAST, .sleep_us = 305 },
+ {.m = TLV493D_OP_MODE_LOWPOWER, .sleep_us = 10 * USEC_PER_MSEC },
+ {.m = TLV493D_OP_MODE_ULTRA_LOWPOWER, .sleep_us = 100 * USEC_PER_MSEC },
+ {.m = TLV493D_OP_MODE_MASTERCONTROLLED, .sleep_us = 305 },
+};
+
+/*
+ * The datasheet mentions the sensor supports only direct byte-stream write starting from
+ * register address 0x0. So for any modification to be made to any write registers, it must
+ * be written starting from the register address 0x0.
+ * I2C write operation should not contain register address in the I2C frame, it should
+ * contains only raw byte stream for the write registers. As below,
+ * I2C Frame: |S|SlaveAddr Wr|Ack|Byte[0]|Ack|Byte[1]|Ack|.....|Sp|
+ */
+static int tlv493d_write_all_regs(struct tlv493d_data *data)
+{
+ int ret;
+
+ if (!data || !data->client)
+ return -EINVAL;
+
+ /*
+ * As regmap does not provide raw write API which perform I2C write without
+ * specifying register address, direct i2c_master_send() API is used.
+ */
+ ret = i2c_master_send(data->client, data->wr_regs, ARRAY_SIZE(data->wr_regs));
+ if (ret < 0) {
+ dev_err(data->dev, "failed to write registers. error %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int tlv493d_set_operating_mode(struct tlv493d_data *data, u8 mode)
+{
+ if (!data)
+ return -EINVAL;
+
+ u8 *reg_mode1 = &data->wr_regs[TLV493D_WR_REG_MODE1];
+ u8 *reg_mode2 = &data->wr_regs[TLV493D_WR_REG_MODE2];
+
+ switch (mode) {
+ case TLV493D_OP_MODE_POWERDOWN:
+ CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
+ CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
+ break;
+
+ case TLV493D_OP_MODE_FAST:
+ SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
+ CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
+ break;
+
+ case TLV493D_OP_MODE_LOWPOWER:
+ CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
+ SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
+ SET_BIT(*reg_mode2, TLV493D_MODE2_LP_PERIOD);
+ break;
+
+ case TLV493D_OP_MODE_ULTRA_LOWPOWER:
+ CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
+ SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
+ CLR_BIT(*reg_mode2, TLV493D_MODE2_LP_PERIOD);
+ break;
+
+ case TLV493D_OP_MODE_MASTERCONTROLLED:
+ SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
+ SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
+ break;
+
+ default:
+ dev_err(data->dev, "invalid mode configuration\n");
+ return -EINVAL;
+ }
+
+ return tlv493d_write_all_regs(data);
+}
+
+static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
+ s16 *z, s16 *t)
+{
+ u8 buff[7] = {0};
+ int err, ret;
+ struct tlv493d_mode *mode;
+
+ if (!data)
+ return -EINVAL;
+
+ guard(mutex)(&data->lock);
+
+ ret = pm_runtime_resume_and_get(data->dev);
+ if (ret < 0)
+ return ret;
+
+ mode = &modes[data->mode];
+
+ /*
+ * Poll until data is valid,
+ * For a valid data TLV493D_VAL_CHANNEL bit of TLV493D_RD_REG_TEMP should be set to 0.
+ * The sampling time depends on the sensor mode. poll 3x the time of the sampling time.
+ */
+ ret = read_poll_timeout(regmap_bulk_read, err, err ||
+ FIELD_GET(TLV493D_VAL_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
+ mode->sleep_us, (3 * mode->sleep_us), false, data->map, TLV493D_RD_REG_BX,
+ buff, ARRAY_SIZE(buff));
+ if (ret) {
+ dev_err(data->dev, "read poll timeout, error:%d", ret);
+ goto out;
+ }
+ if (err) {
+ dev_err(data->dev, "read data failed, error:%d\n", ret);
+ ret = err;
+ goto out;
+ }
+
+ *x = TLV493D_DATA_X_GET(buff);
+ *y = TLV493D_DATA_Y_GET(buff);
+ *z = TLV493D_DATA_Z_GET(buff);
+ *t = TLV493D_DATA_TEMP_GET(buff);
+
+out:
+ pm_runtime_mark_last_busy(data->dev);
+ pm_runtime_put_autosuspend(data->dev);
+ return ret;
+}
+
+static int tlv493d_init(struct tlv493d_data *data)
+{
+ int ret;
+ u8 buff[TLV493D_RD_REG_MAX];
+
+ if (!data)
+ return -EINVAL;
+
+ /*
+ * The sensor initialization requires below steps to be followed,
+ * 1. Power-up sensor.
+ * 2. Read and store read-registers map (0x0-0x9).
+ * 3. Copy values from read reserved registers to write reserved fields (0x0-0x3).
+ * 4. Set operating mode.
+ * 5. Write to all registers.
+ */
+ ret = regmap_bulk_read(data->map, TLV493D_RD_REG_BX, buff, ARRAY_SIZE(buff));
+ if (ret) {
+ dev_err(data->dev, "bulk read failed, error %d\n", ret);
+ return ret;
+ }
+
+ data->wr_regs[0] = 0; /* Write register 0x0 is reserved. Does not require to be updated.*/
+ data->wr_regs[1] = buff[TLV493D_RD_REG_RES1] & TLV493D_RD_REG_RES1_WR_MASK;
+ data->wr_regs[2] = buff[TLV493D_RD_REG_RES2] & TLV493D_RD_REG_RES2_WR_MASK;
+ data->wr_regs[3] = buff[TLV493D_RD_REG_RES3] & TLV493D_RD_REG_RES3_WR_MASK;
+
+ ret = tlv493d_set_operating_mode(data, data->mode);
+ if (ret < 0) {
+ dev_err(data->dev, "failed to set operating mode\n");
+ return ret;
+ }
+
+ return ret;
+}
+
+static int tlv493d_parse_dt(struct tlv493d_data *data)
+{
+ struct device_node *node;
+ u32 val = 0;
+ int ret;
+
+ if (!data)
+ return -EINVAL;
+
+ node = data->dev->of_node;
+
+ /* Optional 'mode' property to set sensor operation mode */
+ ret = of_property_read_u32(node, "mode", &val);
+ if (ret < 0 || val >= TLV493D_OP_MODE_MAX) {
+ /* Fallback to default mode if property is missing or invalid */
+ data->mode = TLV493D_OP_MODE_MASTERCONTROLLED;
+ } else {
+ data->mode = val;
+ }
+
+ /*
+ * Read temperature offset (raw value at 25°C). If not specified,
+ * default to 340.
+ */
+ ret = of_property_read_u32(node, "temp-offset", &val);
+ if (ret)
+ val = 340;
+ /*
+ * The above is a raw offset; however, IIO expects a single effective offset.
+ * Since final temperature includes an additional fixed 25°C (i.e. 25000 m°C),
+ * we compute a combined offset using scale = 1100 (1.1 * 1000).
+ */
+ data->temp_offset = -val + (s32)div_u64(25000, 1100);
+
+ return 0;
+}
+
+static int tlv493d_read_raw(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
+ int *val, int *val2, long mask)
+{
+ struct tlv493d_data *data = iio_priv(indio_dev);
+ s16 x, y, z, t;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ case IIO_CHAN_INFO_RAW:
+ ret = tlv493d_get_measurements(data, &x, &y, &z, &t);
+ if (ret) {
+ dev_err(data->dev, "failed to read sensor data\n");
+ return ret;
+ }
+ /* Return raw values for requested channel */
+ switch (chan->address) {
+ case AXIS_X:
+ *val = x;
+ return IIO_VAL_INT;
+ case AXIS_Y:
+ *val = y;
+ return IIO_VAL_INT;
+ case AXIS_Z:
+ *val = z;
+ return IIO_VAL_INT;
+ case TEMPERATURE:
+ *val = t;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ case IIO_CHAN_INFO_SCALE:
+ switch (chan->type) {
+ case IIO_MAGN:
+ /*
+ * Magnetic field scale: 0.0098 mTesla (i.e. 9.8 µT)
+ * Expressed as fractional: 98/10 = 9.8 µT.
+ */
+ *val = 98;
+ *val2 = 10;
+ return IIO_VAL_FRACTIONAL;
+ case IIO_TEMP:
+ /*
+ * Temperature scale: 1.1 °C per LSB, expressed as 1100 m°C
+ * Returned as integer for IIO core to apply:
+ * temp = (raw + offset) * scale
+ */
+ *val = 1.1 * MILLI;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ case IIO_CHAN_INFO_OFFSET:
+ switch (chan->type) {
+ case IIO_TEMP:
+ /*
+ * Temperature offset includes sensor-specific raw offset
+ * plus compensation for +25°C bias in formula.
+ * This value is precomputed during probe/init:
+ * offset = -raw_offset + (25000 / scale)
+ */
+ *val = data->temp_offset;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static irqreturn_t tlv493d_trigger_handler(int irq, void *ptr)
+{
+ struct iio_poll_func *pf = ptr;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct tlv493d_data *data = iio_priv(indio_dev);
+
+ struct {
+ s16 channels[3];
+ s16 temperature;
+ aligned_s64 timestamp;
+ } scan;
+
+ s16 x, y, z, t;
+ int ret;
+
+ ret = tlv493d_get_measurements(data, &x, &y, &z, &t);
+ if (ret) {
+ dev_err(data->dev, "failed to read sensor data\n");
+ goto trig_out;
+ }
+
+ scan.channels[0] = x;
+ scan.channels[1] = y;
+ scan.channels[2] = z;
+ scan.temperature = t;
+
+ iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan), pf->timestamp);
+
+trig_out:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+#define TLV493D_AXIS_CHANNEL(axis, index) \
+ { \
+ .type = IIO_MAGN, \
+ .modified = 1, \
+ .channel2 = IIO_MOD_##axis, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .address = index, \
+ .scan_index = index, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 12, \
+ .storagebits = 16, \
+ .endianness = IIO_CPU, \
+ }, \
+ }
+
+static const struct iio_chan_spec tlv493d_channels[] = {
+ TLV493D_AXIS_CHANNEL(X, AXIS_X),
+ TLV493D_AXIS_CHANNEL(Y, AXIS_Y),
+ TLV493D_AXIS_CHANNEL(Z, AXIS_Z),
+ {
+ .type = IIO_TEMP,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_OFFSET),
+ .address = TEMPERATURE,
+ .scan_index = TEMPERATURE,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 12,
+ .storagebits = 16,
+ .endianness = IIO_CPU,
+ },
+ },
+ IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+static const struct regmap_range tlv493d_volatile_reg_ranges[] = {
+ regmap_reg_range(TLV493D_RD_REG_BX, TLV493D_RD_REG_RES3),
+};
+
+static const struct regmap_access_table tlv493d_volatile_regs = {
+ .yes_ranges = tlv493d_volatile_reg_ranges,
+ .n_yes_ranges = ARRAY_SIZE(tlv493d_volatile_reg_ranges),
+};
+
+static const struct iio_info tlv493d_info = {
+ .read_raw = tlv493d_read_raw,
+};
+
+static const struct iio_buffer_setup_ops tlv493d_setup_ops = {};
+
+static const unsigned long tlv493d_scan_masks[] = { GENMASK(3, 0), 0 };
+
+static const struct regmap_config tlv493d_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = TLV493D_RD_REG_RES3,
+ .volatile_table = &tlv493d_volatile_regs,
+};
+
+static int tlv493d_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct iio_dev *indio_dev;
+ struct tlv493d_data *data;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+ data->dev = dev;
+ data->client = client;
+ i2c_set_clientdata(client, indio_dev);
+
+ ret = devm_mutex_init(dev, &data->lock);
+ if (ret)
+ return ret;
+
+ data->map = devm_regmap_init_i2c(client, &tlv493d_regmap_config);
+ if (IS_ERR(data->map))
+ return dev_err_probe(dev, PTR_ERR(data->map), "failed to allocate register map\n");
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to enable regulator\n");
+
+ ret = tlv493d_parse_dt(data);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to parse device-tree\n");
+
+ ret = tlv493d_init(data);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to initialized\n");
+
+ indio_dev->info = &tlv493d_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->name = client->name;
+ indio_dev->channels = tlv493d_channels;
+ indio_dev->num_channels = ARRAY_SIZE(tlv493d_channels);
+ indio_dev->available_scan_masks = tlv493d_scan_masks;
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+ iio_pollfunc_store_time, tlv493d_trigger_handler,
+ &tlv493d_setup_ops);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "iio triggered buffer setup failed\n");
+
+ ret = pm_runtime_set_active(dev);
+ if (ret < 0)
+ return ret;
+
+ ret = devm_pm_runtime_enable(dev);
+ if (ret < 0)
+ return ret;
+
+ pm_runtime_get_noresume(dev);
+ pm_runtime_set_autosuspend_delay(dev, 500);
+ pm_runtime_use_autosuspend(dev);
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret)
+ return dev_err_probe(dev, ret, "iio device register failed\n");
+
+ return 0;
+}
+
+static int tlv493d_runtime_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tlv493d_data *data = iio_priv(indio_dev);
+
+ return tlv493d_set_operating_mode(data, TLV493D_OP_MODE_POWERDOWN);
+}
+
+static int tlv493d_runtime_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct tlv493d_data *data = iio_priv(indio_dev);
+
+ return tlv493d_set_operating_mode(data, data->mode);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(tlv493d_pm_ops,
+ tlv493d_runtime_suspend, tlv493d_runtime_resume, NULL);
+
+static const struct i2c_device_id tlv493d_id[] = {
+ { "tlv493d" },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, tlv493d_id);
+
+static const struct of_device_id tlv493d_of_match[] = {
+ { .compatible = "infineon,tlv493d-a1b6", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, tlv493d_of_match);
+
+static struct i2c_driver tlv493d_driver = {
+ .driver = {
+ .name = "tlv493d",
+ .of_match_table = tlv493d_of_match,
+ .pm = pm_ptr(&tlv493d_pm_ops),
+ },
+ .probe = tlv493d_probe,
+ .id_table = tlv493d_id,
+};
+
+module_i2c_driver(tlv493d_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Infineon TLV493D Low-Power 3D Magnetic Sensor");
+MODULE_AUTHOR("Dixit Parmar <dixitparmar19@gmail.com>");
--
2.43.0
On Sat, 26 Jul 2025 15:07:01 +0530
Dixit Parmar <dixitparmar19@gmail.com> wrote:
Hi Dixit,
Very clean driver for a v1. Nice.
> The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
Slightly odd wrap. Aim for 75 chars for patch descriptions.
> applications includes joysticks, control elements (white goods,
> multifunction knops), or electric meters (anti tampering) and any
> other application that requires accurate angular measurements at
> low power consumptions.
>
> The Sensor is configured over I2C, and as part of Sensor measurement
> data it provides 3-Axis magnetic fields and temperature core measurement.
>
> The driver supports raw value read and buffered input via external trigger
> to allow streaming values with the same sensing timestamp.
>
> The device can be configured in to different operating modes by optional
> device-tree "mode" property. Also, the temperature sensing part requires
> raw offset captured at 25°C and that can be specified by "temp-offset"
> optional device-tree property.
>
> While sensor has interrupt pin multiplexed with I2C SCL pin. But for bus
> configurations interrupt(INT) is not recommended, unless timing constraints
> between I2C data transfers and interrupt pulses are monitored and aligned.
>
> The Sensor's I2C register map and mode information is described in product
> User Manual[1].
>
> Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
Tag, so in the tags block (no blank line to the SoB)
> [1] https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf
Link: https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf #1
So make it a tag with a trailing comment to give the reference number.
>
> Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> ---
> drivers/iio/magnetometer/Kconfig | 14 +
> drivers/iio/magnetometer/Makefile | 2 +
> drivers/iio/magnetometer/tlv493d.c | 606 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 622 insertions(+)
>
> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> index 3debf1320ad1..e0070dccc751 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -246,6 +246,20 @@ config SI7210
> To compile this driver as a module, choose M here: the module
> will be called si7210.
>
> +config TLV493D
> + tristate "Infineon TLV493D Low-Power 3D Magnetic Sensor"
> + depends on I2C
> + select REGMAP_I2C
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + Say Y here to add support for the Infineon TLV493D-A1B6 Low-
> + Power 3D Megnetic Sensor.
> +
> + This driver can also be compiled as a module.
> + To compile this driver as a module, choose M here: the module
> + will be called tlv493d.
> +
> config TI_TMAG5273
> tristate "TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor"
> depends on I2C
> diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
> index 9297723a97d8..39c62dd06db8 100644
> --- a/drivers/iio/magnetometer/Makefile
> +++ b/drivers/iio/magnetometer/Makefile
> @@ -35,4 +35,6 @@ obj-$(CONFIG_SI7210) += si7210.o
>
> obj-$(CONFIG_TI_TMAG5273) += tmag5273.o
>
> +obj-$(CONFIG_TLV493D) += tlv493d.o
As already noted, alphabetical.
> +
> obj-$(CONFIG_YAMAHA_YAS530) += yamaha-yas530.o
> diff --git a/drivers/iio/magnetometer/tlv493d.c b/drivers/iio/magnetometer/tlv493d.c
> new file mode 100644
> index 000000000000..f230d6409a4b
> --- /dev/null
> +++ b/drivers/iio/magnetometer/tlv493d.c
> @@ -0,0 +1,606 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/**
> + * Driver for the Infineon TLV493D Low-Power 3D Magnetic Sensor
> + *
> + * Copyright (C) 2025 Dixit Parmar <dixitparmar19@gmail.com>
> + *
Trivial comment of the day - what does this blank line add? I'd drop it.
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define TLV493D_RD_REG_BX 0x00
> +#define TLV493D_RD_REG_BY 0x01
> +#define TLV493D_RD_REG_BZ 0x02
> +#define TLV493D_RD_REG_TEMP 0x03
> +#define TLV493D_RD_REG_BX2 0x04
> +#define TLV493D_RD_REG_BZ2 0x05
> +#define TLV493D_RD_REG_TEMP2 0x06
> +#define TLV493D_RD_REG_RES1 0x07
> +#define TLV493D_RD_REG_RES2 0x08
> +#define TLV493D_RD_REG_RES3 0x09
> +#define TLV493D_RD_REG_MAX 0x0a
> +#define TLV493D_WR_REG_RES 0x00
> +#define TLV493D_WR_REG_MODE1 0x01
> +#define TLV493D_WR_REG_RES2 0x02
> +#define TLV493D_WR_REG_MODE2 0x03
> +#define TLV493D_WR_REG_MAX 0x04
Add a blank line here.
> +#define TLV493D_VAL_MAG_X_AXIS_MSB GENMASK(7, 0)
Can we name these to relate them to the registers they are in?
If it's the whole register can probably just skip the mask.
> +#define TLV493D_VAL_MAG_X_AXIS_LSB GENMASK(7, 4)
> +#define TLV493D_VAL_MAG_Y_AXIS_MSB GENMASK(7, 0)
> +#define TLV493D_VAL_MAG_Y_AXIS_LSB GENMASK(3, 0)
> +#define TLV493D_VAL_MAG_Z_AXIS_MSB GENMASK(7, 0)
> +#define TLV493D_VAL_MAG_Z_AXIS_LSB GENMASK(3, 0)
> +#define TLV493D_VAL_TEMP_MSB GENMASK(7, 4)
> +#define TLV493D_VAL_TEMP_LSB GENMASK(7, 0)
> +#define TLV493D_VAL_FRAME_COUNTER GENMASK(3, 2)
> +#define TLV493D_VAL_CHANNEL GENMASK(1, 0)
> +#define TLV493D_VAL_PD_FLAG BIT(4)
> +#define TLV493D_RD_REG_RES1_WR_MASK GENMASK(4, 3)
> +#define TLV493D_RD_REG_RES2_WR_MASK GENMASK(7, 0)
> +#define TLV493D_RD_REG_RES3_WR_MASK GENMASK(4, 0)
> +#define TLV493D_MODE1_MOD_FAST BIT(1)
> +#define TLV493D_MODE1_MOD_LOW BIT(0)
> +#define TLV493D_MODE2_TEMP_CTRL BIT(7)
> +#define TLV493D_MODE2_LP_PERIOD BIT(6)
> +#define TLV493D_MODE2_PARITY_CTRL BIT(5)
> +
> +#define SET_BIT(b, bit) (b |= bit)
> +#define CLR_BIT(b, bit) (b &= ~bit)
As others have mentioned. Drop these - they aren't things a kernel reviewer will
expect and just make the code harder to read as a result.
> +
> +#define TLV493D_DATA_X_GET(b) \
> + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 | \
> + (FIELD_GET(TLV493D_VAL_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4), 11)
These are odd enough I'd make them c functions rather than macros. Burn a few lines
for better readability.
> +#define TLV493D_DATA_Y_GET(b) \
> + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 | \
> + FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]), 11)
> +#define TLV493D_DATA_Z_GET(b) \
> + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_Z_AXIS_MSB, b[TLV493D_RD_REG_BZ]) << 4 | \
> + FIELD_GET(TLV493D_VAL_MAG_Z_AXIS_LSB, b[TLV493D_RD_REG_BZ2]), 11)
> +#define TLV493D_DATA_TEMP_GET(b) \
> + sign_extend32(FIELD_GET(TLV493D_VAL_TEMP_MSB, b[TLV493D_RD_REG_TEMP]) << 8 | \
> + FIELD_GET(TLV493D_VAL_TEMP_LSB, b[TLV493D_RD_REG_TEMP2]), 11)
> +
> +enum tlv493d_channels {
> + AXIS_X = 0,
> + AXIS_Y,
> + AXIS_Z,
> + TEMPERATURE,
> +};
> +
> +enum tlv493d_op_mode {
> + TLV493D_OP_MODE_POWERDOWN = 0,
> + TLV493D_OP_MODE_FAST,
> + TLV493D_OP_MODE_LOWPOWER,
> + TLV493D_OP_MODE_ULTRA_LOWPOWER,
> + TLV493D_OP_MODE_MASTERCONTROLLED,
> + TLV493D_OP_MODE_MAX,
No comma on a 'terminating entry'. We don't want anyone to accdientally add
anything after the MAX entry.
> +};
> +
> +struct tlv493d_mode {
> + u8 m;
> + u32 sleep_us;
> +};
> +
> +struct tlv493d_data {
> + struct device *dev;
Isn't this just client->dev? If so don't bother having both.
> + struct i2c_client *client;
> + struct mutex lock;
Needs a comment on what data it protects.
> + struct regmap *map;
> + u8 mode;
> + u8 wr_regs[TLV493D_WR_REG_MAX];
> + s32 temp_offset;
> +};
> +
> +/*
> + * Different mode has different measurement cycle time, this time is
> + * used in deriving the sleep and timemout while reading the data from
> + * sensor in polling.
> + * Power-down mode: No measurement.
> + * Fast mode: Freq:3.3 KHz. Measurement time:305 usec.
> + * Low-power mode: Freq:100 Hz. Measurement time:10 msec.
> + * Ultra low-power mode: Freq:10 Hz. Measurement time:100 msec.
> + * Master controlled mode: Freq:3.3 Khz. Measurement time:305 usec.
> + */
> +static struct tlv493d_mode modes[TLV493D_OP_MODE_MAX] = {
> + {.m = TLV493D_OP_MODE_POWERDOWN, .sleep_us = 0 },
> + {.m = TLV493D_OP_MODE_FAST, .sleep_us = 305 },
> + {.m = TLV493D_OP_MODE_LOWPOWER, .sleep_us = 10 * USEC_PER_MSEC },
> + {.m = TLV493D_OP_MODE_ULTRA_LOWPOWER, .sleep_us = 100 * USEC_PER_MSEC },
> + {.m = TLV493D_OP_MODE_MASTERCONTROLLED, .sleep_us = 305 },
Space before .m
> +};
> +
> +/*
> + * The datasheet mentions the sensor supports only direct byte-stream write starting from
> + * register address 0x0. So for any modification to be made to any write registers, it must
> + * be written starting from the register address 0x0.
> + * I2C write operation should not contain register address in the I2C frame, it should
> + * contains only raw byte stream for the write registers. As below,
> + * I2C Frame: |S|SlaveAddr Wr|Ack|Byte[0]|Ack|Byte[1]|Ack|.....|Sp|
> + */
> +static int tlv493d_write_all_regs(struct tlv493d_data *data)
> +{
> + int ret;
> +
> + if (!data || !data->client)
> + return -EINVAL;
> +
> + /*
> + * As regmap does not provide raw write API which perform I2C write without
> + * specifying register address, direct i2c_master_send() API is used.
> + */
> + ret = i2c_master_send(data->client, data->wr_regs, ARRAY_SIZE(data->wr_regs));
Given we have to do this, I'm a bit doubtful about regmap usage in general in here.
Maybe it's just not a good fit and we should stick to direct use of the i2c stuff
like here?
> + if (ret < 0) {
> + dev_err(data->dev, "failed to write registers. error %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int tlv493d_set_operating_mode(struct tlv493d_data *data, u8 mode)
> +{
> + if (!data)
> + return -EINVAL;
> +
> + u8 *reg_mode1 = &data->wr_regs[TLV493D_WR_REG_MODE1];
> + u8 *reg_mode2 = &data->wr_regs[TLV493D_WR_REG_MODE2];
> +
> + switch (mode) {
> + case TLV493D_OP_MODE_POWERDOWN:
> + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
As already mentioned by others, use FIELD_PREP etc.
> + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> + break;
> +
> + case TLV493D_OP_MODE_FAST:
> + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> + break;
> +
> + case TLV493D_OP_MODE_LOWPOWER:
> + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> + SET_BIT(*reg_mode2, TLV493D_MODE2_LP_PERIOD);
> + break;
> +
> + case TLV493D_OP_MODE_ULTRA_LOWPOWER:
> + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> + CLR_BIT(*reg_mode2, TLV493D_MODE2_LP_PERIOD);
> + break;
> +
> + case TLV493D_OP_MODE_MASTERCONTROLLED:
> + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> + break;
> +
> + default:
> + dev_err(data->dev, "invalid mode configuration\n");
> + return -EINVAL;
> + }
> +
> + return tlv493d_write_all_regs(data);
> +}
> +
> +static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
> + s16 *z, s16 *t)
> +{
> + u8 buff[7] = {0};
The 0 isn't needed and did odd things in older compilers (though only ones
we've since dropped support form.
= { };
is fine.
> + int err, ret;
> + struct tlv493d_mode *mode;
> +
> + if (!data)
> + return -EINVAL;
> +
> + guard(mutex)(&data->lock);
> +
> + ret = pm_runtime_resume_and_get(data->dev);
> + if (ret < 0)
> + return ret;
> +
> + mode = &modes[data->mode];
> +
> + /*
> + * Poll until data is valid,
> + * For a valid data TLV493D_VAL_CHANNEL bit of TLV493D_RD_REG_TEMP should be set to 0.
> + * The sampling time depends on the sensor mode. poll 3x the time of the sampling time.
> + */
> + ret = read_poll_timeout(regmap_bulk_read, err, err ||
> + FIELD_GET(TLV493D_VAL_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> + mode->sleep_us, (3 * mode->sleep_us), false, data->map, TLV493D_RD_REG_BX,
> + buff, ARRAY_SIZE(buff));
> + if (ret) {
> + dev_err(data->dev, "read poll timeout, error:%d", ret);
> + goto out;
> + }
> + if (err) {
> + dev_err(data->dev, "read data failed, error:%d\n", ret);
> + ret = err;
> + goto out;
> + }
> +
> + *x = TLV493D_DATA_X_GET(buff);
> + *y = TLV493D_DATA_Y_GET(buff);
> + *z = TLV493D_DATA_Z_GET(buff);
> + *t = TLV493D_DATA_TEMP_GET(buff);
> +
> +out:
> + pm_runtime_mark_last_busy(data->dev);
As below This should get simpler.
Not directly relevant to this patch:
If this cycle is quiet I plan to propose some cleanup.h based handling for runtime
pm as it's annoying how often we need a goto for it. The new ACQUIRE() / ACQUIRE_ERR()
should work for this.
> + pm_runtime_put_autosuspend(data->dev);
> + return ret;
> +}
> +
> +static int tlv493d_init(struct tlv493d_data *data)
> +{
> + int ret;
> + u8 buff[TLV493D_RD_REG_MAX];
> +
> + if (!data)
> + return -EINVAL;
> +
> + /*
> + * The sensor initialization requires below steps to be followed,
> + * 1. Power-up sensor.
> + * 2. Read and store read-registers map (0x0-0x9).
> + * 3. Copy values from read reserved registers to write reserved fields (0x0-0x3).
> + * 4. Set operating mode.
> + * 5. Write to all registers.
> + */
> + ret = regmap_bulk_read(data->map, TLV493D_RD_REG_BX, buff, ARRAY_SIZE(buff));
> + if (ret) {
> + dev_err(data->dev, "bulk read failed, error %d\n", ret);
> + return ret;
> + }
> +
> + data->wr_regs[0] = 0; /* Write register 0x0 is reserved. Does not require to be updated.*/
> + data->wr_regs[1] = buff[TLV493D_RD_REG_RES1] & TLV493D_RD_REG_RES1_WR_MASK;
> + data->wr_regs[2] = buff[TLV493D_RD_REG_RES2] & TLV493D_RD_REG_RES2_WR_MASK;
> + data->wr_regs[3] = buff[TLV493D_RD_REG_RES3] & TLV493D_RD_REG_RES3_WR_MASK;
> +
> + ret = tlv493d_set_operating_mode(data, data->mode);
> + if (ret < 0) {
> + dev_err(data->dev, "failed to set operating mode\n");
> + return ret;
> + }
> +
> + return ret;
return 0?
> +}
> +
> +static int tlv493d_parse_dt(struct tlv493d_data *data)
> +{
> + struct device_node *node;
> + u32 val = 0;
> + int ret;
> +
> + if (!data)
> + return -EINVAL;
> +
> + node = data->dev->of_node;
> +
> + /* Optional 'mode' property to set sensor operation mode */
> + ret = of_property_read_u32(node, "mode", &val);
Use accessors from property.h not, the of specific ones.
This will go away anyway (see comments on dt-bindings from others) but..
> + if (ret < 0 || val >= TLV493D_OP_MODE_MAX) {
> + /* Fallback to default mode if property is missing or invalid */
> + data->mode = TLV493D_OP_MODE_MASTERCONTROLLED;
If it's invalid error out so we know we have a bad DT for default
a common pattern is.
data->mode = TLV493D_OP_MODE_MASTERCONTROLLED;
device_property_read_u32(data->dev, "mode", &data->mode);
if (data->mode >= TLV493D_OP_MODE_MAX)
return dev_err_probe()...
> + } else {
> + data->mode = val;
> + }
> +
> + /*
> + * Read temperature offset (raw value at 25°C). If not specified,
> + * default to 340.
> + */
> + ret = of_property_read_u32(node, "temp-offset", &val);
As others have mentioned, this sort of calibration thing is normally a userspace
problem unless it's coming from something wiring related? E.g. external components.
> + if (ret)
> + val = 340;
> + /*
> + * The above is a raw offset; however, IIO expects a single effective offset.
> + * Since final temperature includes an additional fixed 25°C (i.e. 25000 m°C),
> + * we compute a combined offset using scale = 1100 (1.1 * 1000).
> + */
> + data->temp_offset = -val + (s32)div_u64(25000, 1100);
> +
> + return 0;
> +}
> +
> +static int tlv493d_read_raw(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
wrap to keep this under 80. Doesn't look like it will hurt readability.
> + int *val, int *val2, long mask)
> +{
> + struct tlv493d_data *data = iio_priv(indio_dev);
> + s16 x, y, z, t;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
Not used.
> + case IIO_CHAN_INFO_RAW:
> + ret = tlv493d_get_measurements(data, &x, &y, &z, &t);
> + if (ret) {
> + dev_err(data->dev, "failed to read sensor data\n");
> + return ret;
> + }
> + /* Return raw values for requested channel */
> + switch (chan->address) {
> + case AXIS_X:
> + *val = x;
> + return IIO_VAL_INT;
> + case AXIS_Y:
> + *val = y;
> + return IIO_VAL_INT;
> + case AXIS_Z:
> + *val = z;
> + return IIO_VAL_INT;
> + case TEMPERATURE:
> + *val = t;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_MAGN:
> + /*
> + * Magnetic field scale: 0.0098 mTesla (i.e. 9.8 µT)
> + * Expressed as fractional: 98/10 = 9.8 µT.
> + */
> + *val = 98;
> + *val2 = 10;
> + return IIO_VAL_FRACTIONAL;
> + case IIO_TEMP:
> + /*
> + * Temperature scale: 1.1 °C per LSB, expressed as 1100 m°C
> + * Returned as integer for IIO core to apply:
> + * temp = (raw + offset) * scale
> + */
> + *val = 1.1 * MILLI;
Whilst a compiler should figure this out, maybe safer to just do 1100
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_OFFSET:
> + switch (chan->type) {
> + case IIO_TEMP:
> + /*
> + * Temperature offset includes sensor-specific raw offset
> + * plus compensation for +25°C bias in formula.
> + * This value is precomputed during probe/init:
> + * offset = -raw_offset + (25000 / scale)
> + */
> + *val = data->temp_offset;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +static int tlv493d_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct iio_dev *indio_dev;
> + struct tlv493d_data *data;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->dev = dev;
> + data->client = client;
> + i2c_set_clientdata(client, indio_dev);
> +
> + ret = devm_mutex_init(dev, &data->lock);
> + if (ret)
> + return ret;
> +
> + data->map = devm_regmap_init_i2c(client, &tlv493d_regmap_config);
> + if (IS_ERR(data->map))
> + return dev_err_probe(dev, PTR_ERR(data->map), "failed to allocate register map\n");
Long line, break before the string. For IIO we 'aim' for 80 chars
but let that slip if there is a good readability reason.
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to enable regulator\n");
> +
> + ret = tlv493d_parse_dt(data);
Rename as parse_firmware() or similar after changing to property.h.
For a case as simple as this there is no advantage in limiting the firmware
handling to DT.
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to parse device-tree\n");
> +
> + ret = tlv493d_init(data);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to initialized\n");
> +
> + indio_dev->info = &tlv493d_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->name = client->name;
> + indio_dev->channels = tlv493d_channels;
> + indio_dev->num_channels = ARRAY_SIZE(tlv493d_channels);
> + indio_dev->available_scan_masks = tlv493d_scan_masks;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> + iio_pollfunc_store_time, tlv493d_trigger_handler,
> + &tlv493d_setup_ops);
Align as per comments below.
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "iio triggered buffer setup failed\n");
> +
> + ret = pm_runtime_set_active(dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = devm_pm_runtime_enable(dev);
> + if (ret < 0)
> + return ret;
> +
> + pm_runtime_get_noresume(dev);
> + pm_runtime_set_autosuspend_delay(dev, 500);
> + pm_runtime_use_autosuspend(dev);
> +
> + pm_runtime_mark_last_busy(dev);
By the time this lands, a series to make pm_runtime_put_autosuspend()
include a call to pm_runtime_mark_last_busy() should be upstream.
I'll hopefully remember to drop this when applying or it'll get
caught by a follow up series tidying up the ones that crossed with that
series.
> + pm_runtime_put_autosuspend(dev);
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "iio device register failed\n");
> +
> + return 0;
> +}
> +static DEFINE_RUNTIME_DEV_PM_OPS(tlv493d_pm_ops,
> + tlv493d_runtime_suspend, tlv493d_runtime_resume, NULL);
Align typically as
static DEFINE_RUNTIME_DEV_PM_OPS(tlv493d_pm_ops, tlv493d_runtime_suspend,
tlv493d_runtime_resume, NULL);
If you do have a very long line, perhaps due to a verbose parameter name
and need to not align after the ( then use just one tab more than the line
above, not 2.
> +
> +static const struct i2c_device_id tlv493d_id[] = {
> + { "tlv493d" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, tlv493d_id);
> +
> +static const struct of_device_id tlv493d_of_match[] = {
> + { .compatible = "infineon,tlv493d-a1b6", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, tlv493d_of_match);
> +
> +static struct i2c_driver tlv493d_driver = {
> + .driver = {
> + .name = "tlv493d",
> + .of_match_table = tlv493d_of_match,
> + .pm = pm_ptr(&tlv493d_pm_ops),
> + },
> + .probe = tlv493d_probe,
> + .id_table = tlv493d_id,
> +};
> +
> +module_i2c_driver(tlv493d_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Infineon TLV493D Low-Power 3D Magnetic Sensor");
> +MODULE_AUTHOR("Dixit Parmar <dixitparmar19@gmail.com>");
>
On Sun, Jul 27, 2025 at 02:05:59PM +0100, Jonathan Cameron wrote:
> On Sat, 26 Jul 2025 15:07:01 +0530
> Dixit Parmar <dixitparmar19@gmail.com> wrote:
>
> Hi Dixit,
>
> Very clean driver for a v1. Nice.
>
> > The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
>
> Slightly odd wrap. Aim for 75 chars for patch descriptions.
>
Okay, 75.
> > applications includes joysticks, control elements (white goods,
> > multifunction knops), or electric meters (anti tampering) and any
> > other application that requires accurate angular measurements at
> > low power consumptions.
> >
> > The Sensor is configured over I2C, and as part of Sensor measurement
> > data it provides 3-Axis magnetic fields and temperature core measurement.
> >
> > The driver supports raw value read and buffered input via external trigger
> > to allow streaming values with the same sensing timestamp.
> >
> > The device can be configured in to different operating modes by optional
> > device-tree "mode" property. Also, the temperature sensing part requires
> > raw offset captured at 25°C and that can be specified by "temp-offset"
> > optional device-tree property.
> >
> > While sensor has interrupt pin multiplexed with I2C SCL pin. But for bus
> > configurations interrupt(INT) is not recommended, unless timing constraints
> > between I2C data transfers and interrupt pulses are monitored and aligned.
> >
> > The Sensor's I2C register map and mode information is described in product
> > User Manual[1].
> >
> > Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> Tag, so in the tags block (no blank line to the SoB)
Sorry, didn't quite get it.
> > [1] https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf
> Link: https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf #1
>
> So make it a tag with a trailing comment to give the reference number.
>
> >
> > Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
>
>
> > ---
> > drivers/iio/magnetometer/Kconfig | 14 +
> > drivers/iio/magnetometer/Makefile | 2 +
> > drivers/iio/magnetometer/tlv493d.c | 606 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 622 insertions(+)
> >
> > diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> > index 3debf1320ad1..e0070dccc751 100644
> > --- a/drivers/iio/magnetometer/Kconfig
> > +++ b/drivers/iio/magnetometer/Kconfig
> > @@ -246,6 +246,20 @@ config SI7210
> > To compile this driver as a module, choose M here: the module
> > will be called si7210.
> >
> > +config TLV493D
> > + tristate "Infineon TLV493D Low-Power 3D Magnetic Sensor"
> > + depends on I2C
> > + select REGMAP_I2C
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > + help
> > + Say Y here to add support for the Infineon TLV493D-A1B6 Low-
> > + Power 3D Megnetic Sensor.
> > +
> > + This driver can also be compiled as a module.
> > + To compile this driver as a module, choose M here: the module
> > + will be called tlv493d.
> > +
> > config TI_TMAG5273
> > tristate "TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor"
> > depends on I2C
> > diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
> > index 9297723a97d8..39c62dd06db8 100644
> > --- a/drivers/iio/magnetometer/Makefile
> > +++ b/drivers/iio/magnetometer/Makefile
> > @@ -35,4 +35,6 @@ obj-$(CONFIG_SI7210) += si7210.o
> >
> > obj-$(CONFIG_TI_TMAG5273) += tmag5273.o
> >
> > +obj-$(CONFIG_TLV493D) += tlv493d.o
>
> As already noted, alphabetical.
>
Ack.
> > +
> > obj-$(CONFIG_YAMAHA_YAS530) += yamaha-yas530.o
> > diff --git a/drivers/iio/magnetometer/tlv493d.c b/drivers/iio/magnetometer/tlv493d.c
> > new file mode 100644
> > index 000000000000..f230d6409a4b
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/tlv493d.c
> > @@ -0,0 +1,606 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/**
> > + * Driver for the Infineon TLV493D Low-Power 3D Magnetic Sensor
> > + *
> > + * Copyright (C) 2025 Dixit Parmar <dixitparmar19@gmail.com>
> > + *
>
> Trivial comment of the day - what does this blank line add? I'd drop it.
>
okay, will drop last blank line.
> > + */
> > +
> > +#include <linux/bits.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/types.h>
> > +#include <linux/units.h>
> > +
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +
> > +#define TLV493D_RD_REG_BX 0x00
> > +#define TLV493D_RD_REG_BY 0x01
> > +#define TLV493D_RD_REG_BZ 0x02
> > +#define TLV493D_RD_REG_TEMP 0x03
> > +#define TLV493D_RD_REG_BX2 0x04
> > +#define TLV493D_RD_REG_BZ2 0x05
> > +#define TLV493D_RD_REG_TEMP2 0x06
> > +#define TLV493D_RD_REG_RES1 0x07
> > +#define TLV493D_RD_REG_RES2 0x08
> > +#define TLV493D_RD_REG_RES3 0x09
> > +#define TLV493D_RD_REG_MAX 0x0a
> > +#define TLV493D_WR_REG_RES 0x00
> > +#define TLV493D_WR_REG_MODE1 0x01
> > +#define TLV493D_WR_REG_RES2 0x02
> > +#define TLV493D_WR_REG_MODE2 0x03
> > +#define TLV493D_WR_REG_MAX 0x04
>
> Add a blank line here.
>
Ack.
> > +#define TLV493D_VAL_MAG_X_AXIS_MSB GENMASK(7, 0)
>
> Can we name these to relate them to the registers they are in?
> If it's the whole register can probably just skip the mask.
>
Will try to fit the registers name.
> > +#define TLV493D_VAL_MAG_X_AXIS_LSB GENMASK(7, 4)
> > +#define TLV493D_VAL_MAG_Y_AXIS_MSB GENMASK(7, 0)
> > +#define TLV493D_VAL_MAG_Y_AXIS_LSB GENMASK(3, 0)
> > +#define TLV493D_VAL_MAG_Z_AXIS_MSB GENMASK(7, 0)
> > +#define TLV493D_VAL_MAG_Z_AXIS_LSB GENMASK(3, 0)
> > +#define TLV493D_VAL_TEMP_MSB GENMASK(7, 4)
> > +#define TLV493D_VAL_TEMP_LSB GENMASK(7, 0)
> > +#define TLV493D_VAL_FRAME_COUNTER GENMASK(3, 2)
> > +#define TLV493D_VAL_CHANNEL GENMASK(1, 0)
> > +#define TLV493D_VAL_PD_FLAG BIT(4)
> > +#define TLV493D_RD_REG_RES1_WR_MASK GENMASK(4, 3)
> > +#define TLV493D_RD_REG_RES2_WR_MASK GENMASK(7, 0)
> > +#define TLV493D_RD_REG_RES3_WR_MASK GENMASK(4, 0)
> > +#define TLV493D_MODE1_MOD_FAST BIT(1)
> > +#define TLV493D_MODE1_MOD_LOW BIT(0)
> > +#define TLV493D_MODE2_TEMP_CTRL BIT(7)
> > +#define TLV493D_MODE2_LP_PERIOD BIT(6)
> > +#define TLV493D_MODE2_PARITY_CTRL BIT(5)
> > +
> > +#define SET_BIT(b, bit) (b |= bit)
> > +#define CLR_BIT(b, bit) (b &= ~bit)
>
> As others have mentioned. Drop these - they aren't things a kernel reviewer will
> expect and just make the code harder to read as a result.
>
Understood.
> > +
> > +#define TLV493D_DATA_X_GET(b) \
> > + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 | \
> > + (FIELD_GET(TLV493D_VAL_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4), 11)
>
> These are odd enough I'd make them c functions rather than macros. Burn a few lines
> for better readability.
>
I saw this kind of data retrival and formation from registers as macros so I sticked to
it. Having all these as function will also require a seperate function
for each channel coz the masks and the layout of the bits changes over
the register. Do you still recommend it as c functions?
> > +#define TLV493D_DATA_Y_GET(b) \
> > + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 | \
> > + FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]), 11)
> > +#define TLV493D_DATA_Z_GET(b) \
> > + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_Z_AXIS_MSB, b[TLV493D_RD_REG_BZ]) << 4 | \
> > + FIELD_GET(TLV493D_VAL_MAG_Z_AXIS_LSB, b[TLV493D_RD_REG_BZ2]), 11)
> > +#define TLV493D_DATA_TEMP_GET(b) \
> > + sign_extend32(FIELD_GET(TLV493D_VAL_TEMP_MSB, b[TLV493D_RD_REG_TEMP]) << 8 | \
> > + FIELD_GET(TLV493D_VAL_TEMP_LSB, b[TLV493D_RD_REG_TEMP2]), 11)
> > +
> > +enum tlv493d_channels {
> > + AXIS_X = 0,
> > + AXIS_Y,
> > + AXIS_Z,
> > + TEMPERATURE,
> > +};
> > +
> > +enum tlv493d_op_mode {
> > + TLV493D_OP_MODE_POWERDOWN = 0,
> > + TLV493D_OP_MODE_FAST,
> > + TLV493D_OP_MODE_LOWPOWER,
> > + TLV493D_OP_MODE_ULTRA_LOWPOWER,
> > + TLV493D_OP_MODE_MASTERCONTROLLED,
> > + TLV493D_OP_MODE_MAX,
> No comma on a 'terminating entry'. We don't want anyone to accdientally add
> anything after the MAX entry.
>
Makes sense.
> > +};
> > +
> > +struct tlv493d_mode {
> > + u8 m;
> > + u32 sleep_us;
> > +};
> > +
> > +struct tlv493d_data {
> > + struct device *dev;
>
> Isn't this just client->dev? If so don't bother having both.
>
Ack.
> > + struct i2c_client *client;
> > + struct mutex lock;
> Needs a comment on what data it protects.
>
Ack.
> > + struct regmap *map;
> > + u8 mode;
> > + u8 wr_regs[TLV493D_WR_REG_MAX];
> > + s32 temp_offset;
> > +};
> > +
> > +/*
> > + * Different mode has different measurement cycle time, this time is
> > + * used in deriving the sleep and timemout while reading the data from
> > + * sensor in polling.
> > + * Power-down mode: No measurement.
> > + * Fast mode: Freq:3.3 KHz. Measurement time:305 usec.
> > + * Low-power mode: Freq:100 Hz. Measurement time:10 msec.
> > + * Ultra low-power mode: Freq:10 Hz. Measurement time:100 msec.
> > + * Master controlled mode: Freq:3.3 Khz. Measurement time:305 usec.
> > + */
> > +static struct tlv493d_mode modes[TLV493D_OP_MODE_MAX] = {
> > + {.m = TLV493D_OP_MODE_POWERDOWN, .sleep_us = 0 },
> > + {.m = TLV493D_OP_MODE_FAST, .sleep_us = 305 },
> > + {.m = TLV493D_OP_MODE_LOWPOWER, .sleep_us = 10 * USEC_PER_MSEC },
> > + {.m = TLV493D_OP_MODE_ULTRA_LOWPOWER, .sleep_us = 100 * USEC_PER_MSEC },
> > + {.m = TLV493D_OP_MODE_MASTERCONTROLLED, .sleep_us = 305 },
>
> Space before .m
>
Ack.
> > +};
> > +
> > +/*
> > + * The datasheet mentions the sensor supports only direct byte-stream write starting from
> > + * register address 0x0. So for any modification to be made to any write registers, it must
> > + * be written starting from the register address 0x0.
> > + * I2C write operation should not contain register address in the I2C frame, it should
> > + * contains only raw byte stream for the write registers. As below,
> > + * I2C Frame: |S|SlaveAddr Wr|Ack|Byte[0]|Ack|Byte[1]|Ack|.....|Sp|
> > + */
> > +static int tlv493d_write_all_regs(struct tlv493d_data *data)
> > +{
> > + int ret;
> > +
> > + if (!data || !data->client)
> > + return -EINVAL;
> > +
> > + /*
> > + * As regmap does not provide raw write API which perform I2C write without
> > + * specifying register address, direct i2c_master_send() API is used.
> > + */
> > + ret = i2c_master_send(data->client, data->wr_regs, ARRAY_SIZE(data->wr_regs));
>
> Given we have to do this, I'm a bit doubtful about regmap usage in general in here.
> Maybe it's just not a good fit and we should stick to direct use of the i2c stuff
> like here?
>
Sorry, didn't get entirely? From what I understood, you meant we could
drop regmap from this driver entirely and use direct I2C APIs. I believe
that would be too much, coz of the frequency we perform operations and
regmap is easier and clean imo.
Also, we could have used regmap_raw_write() API by specifying register
0x0 as address and rest of the 3 bytes as data. regmap will perform raw
write of these byte stream over the I2C the same way sensor expects. But
the problem with that approach is we are not using it as per the API
convention. let me know your thoughts? Is it a good option, it'll look
like this:
regmap_raw_write(data->map, data->wr_regs[0], &data->wr_regs[1],
ARRAY_SIZE(data->wr_regs) - 1);
> > + if (ret < 0) {
> > + dev_err(data->dev, "failed to write registers. error %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int tlv493d_set_operating_mode(struct tlv493d_data *data, u8 mode)
> > +{
> > + if (!data)
> > + return -EINVAL;
> > +
> > + u8 *reg_mode1 = &data->wr_regs[TLV493D_WR_REG_MODE1];
> > + u8 *reg_mode2 = &data->wr_regs[TLV493D_WR_REG_MODE2];
> > +
> > + switch (mode) {
> > + case TLV493D_OP_MODE_POWERDOWN:
> > + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
>
> As already mentioned by others, use FIELD_PREP etc.
>
Ack.
> > + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> > + break;
> > +
> > + case TLV493D_OP_MODE_FAST:
> > + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> > + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> > + break;
> > +
> > + case TLV493D_OP_MODE_LOWPOWER:
> > + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> > + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> > + SET_BIT(*reg_mode2, TLV493D_MODE2_LP_PERIOD);
> > + break;
> > +
> > + case TLV493D_OP_MODE_ULTRA_LOWPOWER:
> > + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> > + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> > + CLR_BIT(*reg_mode2, TLV493D_MODE2_LP_PERIOD);
> > + break;
> > +
> > + case TLV493D_OP_MODE_MASTERCONTROLLED:
> > + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> > + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> > + break;
> > +
> > + default:
> > + dev_err(data->dev, "invalid mode configuration\n");
> > + return -EINVAL;
> > + }
> > +
> > + return tlv493d_write_all_regs(data);
> > +}
> > +
> > +static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
> > + s16 *z, s16 *t)
> > +{
> > + u8 buff[7] = {0};
> The 0 isn't needed and did odd things in older compilers (though only ones
> we've since dropped support form.
> = { };
>
> is fine.
>
Okay.
> > + int err, ret;
> > + struct tlv493d_mode *mode;
> > +
> > + if (!data)
> > + return -EINVAL;
> > +
> > + guard(mutex)(&data->lock);
> > +
> > + ret = pm_runtime_resume_and_get(data->dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + mode = &modes[data->mode];
> > +
> > + /*
> > + * Poll until data is valid,
> > + * For a valid data TLV493D_VAL_CHANNEL bit of TLV493D_RD_REG_TEMP should be set to 0.
> > + * The sampling time depends on the sensor mode. poll 3x the time of the sampling time.
> > + */
> > + ret = read_poll_timeout(regmap_bulk_read, err, err ||
> > + FIELD_GET(TLV493D_VAL_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> > + mode->sleep_us, (3 * mode->sleep_us), false, data->map, TLV493D_RD_REG_BX,
> > + buff, ARRAY_SIZE(buff));
> > + if (ret) {
> > + dev_err(data->dev, "read poll timeout, error:%d", ret);
> > + goto out;
> > + }
> > + if (err) {
> > + dev_err(data->dev, "read data failed, error:%d\n", ret);
> > + ret = err;
> > + goto out;
> > + }
> > +
> > + *x = TLV493D_DATA_X_GET(buff);
> > + *y = TLV493D_DATA_Y_GET(buff);
> > + *z = TLV493D_DATA_Z_GET(buff);
> > + *t = TLV493D_DATA_TEMP_GET(buff);
> > +
> > +out:
> > + pm_runtime_mark_last_busy(data->dev);
>
> As below This should get simpler.
>
> Not directly relevant to this patch:
>
> If this cycle is quiet I plan to propose some cleanup.h based handling for runtime
> pm as it's annoying how often we need a goto for it. The new ACQUIRE() / ACQUIRE_ERR()
> should work for this.
>
Does this need any modifications with current codebase?
>
> > + pm_runtime_put_autosuspend(data->dev);
> > + return ret;
> > +}
> > +
> > +static int tlv493d_init(struct tlv493d_data *data)
> > +{
> > + int ret;
> > + u8 buff[TLV493D_RD_REG_MAX];
> > +
> > + if (!data)
> > + return -EINVAL;
> > +
> > + /*
> > + * The sensor initialization requires below steps to be followed,
> > + * 1. Power-up sensor.
> > + * 2. Read and store read-registers map (0x0-0x9).
> > + * 3. Copy values from read reserved registers to write reserved fields (0x0-0x3).
> > + * 4. Set operating mode.
> > + * 5. Write to all registers.
> > + */
> > + ret = regmap_bulk_read(data->map, TLV493D_RD_REG_BX, buff, ARRAY_SIZE(buff));
> > + if (ret) {
> > + dev_err(data->dev, "bulk read failed, error %d\n", ret);
> > + return ret;
> > + }
> > +
> > + data->wr_regs[0] = 0; /* Write register 0x0 is reserved. Does not require to be updated.*/
> > + data->wr_regs[1] = buff[TLV493D_RD_REG_RES1] & TLV493D_RD_REG_RES1_WR_MASK;
> > + data->wr_regs[2] = buff[TLV493D_RD_REG_RES2] & TLV493D_RD_REG_RES2_WR_MASK;
> > + data->wr_regs[3] = buff[TLV493D_RD_REG_RES3] & TLV493D_RD_REG_RES3_WR_MASK;
> > +
> > + ret = tlv493d_set_operating_mode(data, data->mode);
> > + if (ret < 0) {
> > + dev_err(data->dev, "failed to set operating mode\n");
> > + return ret;
> > + }
> > +
> > + return ret;
> return 0?
Its the same though. ret from tlv493d_set_operating_mode is 0 on
success and -ve on failure.
>
> > +}
> > +
> > +static int tlv493d_parse_dt(struct tlv493d_data *data)
> > +{
> > + struct device_node *node;
> > + u32 val = 0;
> > + int ret;
> > +
> > + if (!data)
> > + return -EINVAL;
> > +
> > + node = data->dev->of_node;
> > +
> > + /* Optional 'mode' property to set sensor operation mode */
> > + ret = of_property_read_u32(node, "mode", &val);
> Use accessors from property.h not, the of specific ones.
>
> This will go away anyway (see comments on dt-bindings from others) but..
>
> > + if (ret < 0 || val >= TLV493D_OP_MODE_MAX) {
> > + /* Fallback to default mode if property is missing or invalid */
> > + data->mode = TLV493D_OP_MODE_MASTERCONTROLLED;
> If it's invalid error out so we know we have a bad DT for default
> a common pattern is.
>
> data->mode = TLV493D_OP_MODE_MASTERCONTROLLED;
> device_property_read_u32(data->dev, "mode", &data->mode);
> if (data->mode >= TLV493D_OP_MODE_MAX)
> return dev_err_probe()...
>
>
>
> > + } else {
> > + data->mode = val;
> > + }
> > +
> > + /*
> > + * Read temperature offset (raw value at 25°C). If not specified,
> > + * default to 340.
> > + */
> > + ret = of_property_read_u32(node, "temp-offset", &val);
>
> As others have mentioned, this sort of calibration thing is normally a userspace
> problem unless it's coming from something wiring related? E.g. external components.
>
This whole tlv493d_parse_dt() will be dropped.
> > + if (ret)
> > + val = 340;
> > + /*
> > + * The above is a raw offset; however, IIO expects a single effective offset.
> > + * Since final temperature includes an additional fixed 25°C (i.e. 25000 m°C),
> > + * we compute a combined offset using scale = 1100 (1.1 * 1000).
> > + */
> > + data->temp_offset = -val + (s32)div_u64(25000, 1100);
> > +
> > + return 0;
> > +}
> > +
> > +static int tlv493d_read_raw(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
>
> wrap to keep this under 80. Doesn't look like it will hurt readability.
>
Ack. Is 80 standard for whole kernel or iio only?
> > + int *val, int *val2, long mask)
> > +{
> > + struct tlv493d_data *data = iio_priv(indio_dev);
> > + s16 x, y, z, t;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
>
> Not used.
Ack.
>
> > + case IIO_CHAN_INFO_RAW:
> > + ret = tlv493d_get_measurements(data, &x, &y, &z, &t);
> > + if (ret) {
> > + dev_err(data->dev, "failed to read sensor data\n");
> > + return ret;
> > + }
> > + /* Return raw values for requested channel */
> > + switch (chan->address) {
> > + case AXIS_X:
> > + *val = x;
> > + return IIO_VAL_INT;
> > + case AXIS_Y:
> > + *val = y;
> > + return IIO_VAL_INT;
> > + case AXIS_Z:
> > + *val = z;
> > + return IIO_VAL_INT;
> > + case TEMPERATURE:
> > + *val = t;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_SCALE:
> > + switch (chan->type) {
> > + case IIO_MAGN:
> > + /*
> > + * Magnetic field scale: 0.0098 mTesla (i.e. 9.8 µT)
> > + * Expressed as fractional: 98/10 = 9.8 µT.
> > + */
> > + *val = 98;
> > + *val2 = 10;
> > + return IIO_VAL_FRACTIONAL;
> > + case IIO_TEMP:
> > + /*
> > + * Temperature scale: 1.1 °C per LSB, expressed as 1100 m°C
> > + * Returned as integer for IIO core to apply:
> > + * temp = (raw + offset) * scale
> > + */
> > + *val = 1.1 * MILLI;
> Whilst a compiler should figure this out, maybe safer to just do 1100
>
As acked already.
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_OFFSET:
> > + switch (chan->type) {
> > + case IIO_TEMP:
> > + /*
> > + * Temperature offset includes sensor-specific raw offset
> > + * plus compensation for +25°C bias in formula.
> > + * This value is precomputed during probe/init:
> > + * offset = -raw_offset + (25000 / scale)
> > + */
> > + *val = data->temp_offset;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
>
>
>
> > +static int tlv493d_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct iio_dev *indio_dev;
> > + struct tlv493d_data *data;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + data = iio_priv(indio_dev);
> > + data->dev = dev;
> > + data->client = client;
> > + i2c_set_clientdata(client, indio_dev);
> > +
> > + ret = devm_mutex_init(dev, &data->lock);
> > + if (ret)
> > + return ret;
> > +
> > + data->map = devm_regmap_init_i2c(client, &tlv493d_regmap_config);
> > + if (IS_ERR(data->map))
> > + return dev_err_probe(dev, PTR_ERR(data->map), "failed to allocate register map\n");
>
> Long line, break before the string. For IIO we 'aim' for 80 chars
> but let that slip if there is a good readability reason.
>
Got it.
>
> > +
> > + ret = devm_regulator_get_enable(dev, "vdd");
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to enable regulator\n");
> > +
> > + ret = tlv493d_parse_dt(data);
>
> Rename as parse_firmware() or similar after changing to property.h.
> For a case as simple as this there is no advantage in limiting the firmware
> handling to DT.
>
The function itself will be dropped.
>
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to parse device-tree\n");
> > +
> > + ret = tlv493d_init(data);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to initialized\n");
> > +
> > + indio_dev->info = &tlv493d_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->name = client->name;
> > + indio_dev->channels = tlv493d_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(tlv493d_channels);
> > + indio_dev->available_scan_masks = tlv493d_scan_masks;
> > +
> > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> > + iio_pollfunc_store_time, tlv493d_trigger_handler,
> > + &tlv493d_setup_ops);
>
> Align as per comments below.
>
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret, "iio triggered buffer setup failed\n");
> > +
> > + ret = pm_runtime_set_active(dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = devm_pm_runtime_enable(dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + pm_runtime_get_noresume(dev);
> > + pm_runtime_set_autosuspend_delay(dev, 500);
> > + pm_runtime_use_autosuspend(dev);
> > +
> > + pm_runtime_mark_last_busy(dev);
>
> By the time this lands, a series to make pm_runtime_put_autosuspend()
> include a call to pm_runtime_mark_last_busy() should be upstream.
> I'll hopefully remember to drop this when applying or it'll get
> caught by a follow up series tidying up the ones that crossed with that
> series.
>
Good to know.
> > + pm_runtime_put_autosuspend(dev);
> > +
> > + ret = devm_iio_device_register(dev, indio_dev);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "iio device register failed\n");
> > +
> > + return 0;
> > +}
>
> > +static DEFINE_RUNTIME_DEV_PM_OPS(tlv493d_pm_ops,
> > + tlv493d_runtime_suspend, tlv493d_runtime_resume, NULL);
> Align typically as
> static DEFINE_RUNTIME_DEV_PM_OPS(tlv493d_pm_ops, tlv493d_runtime_suspend,
> tlv493d_runtime_resume, NULL);
>
> If you do have a very long line, perhaps due to a verbose parameter name
> and need to not align after the ( then use just one tab more than the line
> above, not 2.
>
Ack.
> > +
> > +static const struct i2c_device_id tlv493d_id[] = {
> > + { "tlv493d" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, tlv493d_id);
> > +
> > +static const struct of_device_id tlv493d_of_match[] = {
> > + { .compatible = "infineon,tlv493d-a1b6", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, tlv493d_of_match);
> > +
> > +static struct i2c_driver tlv493d_driver = {
> > + .driver = {
> > + .name = "tlv493d",
> > + .of_match_table = tlv493d_of_match,
> > + .pm = pm_ptr(&tlv493d_pm_ops),
> > + },
> > + .probe = tlv493d_probe,
> > + .id_table = tlv493d_id,
> > +};
> > +
> > +module_i2c_driver(tlv493d_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Infineon TLV493D Low-Power 3D Magnetic Sensor");
> > +MODULE_AUTHOR("Dixit Parmar <dixitparmar19@gmail.com>");
> >
>
Thank you for such detailed review. Appriciate it,
Dixit
On Tue, 29 Jul 2025 09:19:59 +0530
Dixit Parmar <dixitparmar19@gmail.com> wrote:
> On Sun, Jul 27, 2025 at 02:05:59PM +0100, Jonathan Cameron wrote:
> > On Sat, 26 Jul 2025 15:07:01 +0530
> > Dixit Parmar <dixitparmar19@gmail.com> wrote:
> >
> > Hi Dixit,
> >
> > Very clean driver for a v1. Nice.
> >
> > > The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
> >
> > Slightly odd wrap. Aim for 75 chars for patch descriptions.
> >
> Okay, 75.
> > > applications includes joysticks, control elements (white goods,
> > > multifunction knops), or electric meters (anti tampering) and any
> > > other application that requires accurate angular measurements at
> > > low power consumptions.
> > >
> > > The Sensor is configured over I2C, and as part of Sensor measurement
> > > data it provides 3-Axis magnetic fields and temperature core measurement.
> > >
> > > The driver supports raw value read and buffered input via external trigger
> > > to allow streaming values with the same sensing timestamp.
> > >
> > > The device can be configured in to different operating modes by optional
> > > device-tree "mode" property. Also, the temperature sensing part requires
> > > raw offset captured at 25°C and that can be specified by "temp-offset"
> > > optional device-tree property.
> > >
> > > While sensor has interrupt pin multiplexed with I2C SCL pin. But for bus
> > > configurations interrupt(INT) is not recommended, unless timing constraints
> > > between I2C data transfers and interrupt pulses are monitored and aligned.
> > >
> > > The Sensor's I2C register map and mode information is described in product
> > > User Manual[1].
> > >
> > > Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> > Tag, so in the tags block (no blank line to the SoB)
> Sorry, didn't quite get it.
You should have it as Datasheet: <link>
That will then be a formal 'tag' so belongs alongside the Signed-off-by: etc with no blank
lines in that block.
Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
Link: https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf #1
Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> > > [1] https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf
> > Link: https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf #1
> >
> > So make it a tag with a trailing comment to give the reference number.
> >
> > >
> > > Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> > > +
> > > +#define TLV493D_DATA_X_GET(b) \
> > > + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 | \
> > > + (FIELD_GET(TLV493D_VAL_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4), 11)
> >
> > These are odd enough I'd make them c functions rather than macros. Burn a few lines
> > for better readability.
> >
> I saw this kind of data retrival and formation from registers as macros so I sticked to
> it. Having all these as function will also require a seperate function
> for each channel coz the masks and the layout of the bits changes over
> the register. Do you still recommend it as c functions?
Is it more than 4 short functions? I'd burn the few lines that costs.
s32 tlv493d_data_y_get(u8 *buff)
{
u16 val = FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
return sign_extend32(val, 11);
}
> > > +/*
> > > + * The datasheet mentions the sensor supports only direct byte-stream write starting from
> > > + * register address 0x0. So for any modification to be made to any write registers, it must
> > > + * be written starting from the register address 0x0.
> > > + * I2C write operation should not contain register address in the I2C frame, it should
> > > + * contains only raw byte stream for the write registers. As below,
> > > + * I2C Frame: |S|SlaveAddr Wr|Ack|Byte[0]|Ack|Byte[1]|Ack|.....|Sp|
> > > + */
> > > +static int tlv493d_write_all_regs(struct tlv493d_data *data)
> > > +{
> > > + int ret;
> > > +
> > > + if (!data || !data->client)
> > > + return -EINVAL;
> > > +
> > > + /*
> > > + * As regmap does not provide raw write API which perform I2C write without
> > > + * specifying register address, direct i2c_master_send() API is used.
> > > + */
> > > + ret = i2c_master_send(data->client, data->wr_regs, ARRAY_SIZE(data->wr_regs));
> >
> > Given we have to do this, I'm a bit doubtful about regmap usage in general in here.
> > Maybe it's just not a good fit and we should stick to direct use of the i2c stuff
> > like here?
> >
> Sorry, didn't get entirely? From what I understood, you meant we could
> drop regmap from this driver entirely and use direct I2C APIs. I believe
> that would be too much, coz of the frequency we perform operations and
> regmap is easier and clean imo.
The mixture is nasty though :(
> Also, we could have used regmap_raw_write() API by specifying register
> 0x0 as address and rest of the 3 bytes as data. regmap will perform raw
> write of these byte stream over the I2C the same way sensor expects. But
> the problem with that approach is we are not using it as per the API
> convention. let me know your thoughts? Is it a good option, it'll look
> like this:
> regmap_raw_write(data->map, data->wr_regs[0], &data->wr_regs[1],
> ARRAY_SIZE(data->wr_regs) - 1);
I'm not keen on that either.
If you really want to mix i2c and regmap then that's fine, I was just dubious
whether we were getting value for money from regmap here.
> > > + if (ret < 0) {
> > > + dev_err(data->dev, "failed to write registers. error %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > + *x = TLV493D_DATA_X_GET(buff);
> > > + *y = TLV493D_DATA_Y_GET(buff);
> > > + *z = TLV493D_DATA_Z_GET(buff);
> > > + *t = TLV493D_DATA_TEMP_GET(buff);
> > > +
> > > +out:
> > > + pm_runtime_mark_last_busy(data->dev);
> >
> > As below This should get simpler.
> >
> > Not directly relevant to this patch:
> >
> > If this cycle is quiet I plan to propose some cleanup.h based handling for runtime
> > pm as it's annoying how often we need a goto for it. The new ACQUIRE() / ACQUIRE_ERR()
> > should work for this.
> >
> Does this need any modifications with current codebase?
Needs a bunch of work to show generality across many drivers and
convincing Rafael (PM maintainer) it's a good idea.
Don't try to do it in this series!
> >
> > > + pm_runtime_put_autosuspend(data->dev);
> > > + return ret;
> > > +}
> > > +
> > > +static int tlv493d_init(struct tlv493d_data *data)
> > > +{
> > > + int ret;
> > > + u8 buff[TLV493D_RD_REG_MAX];
> > > +
> > > + if (!data)
> > > + return -EINVAL;
> > > +
> > > + /*
> > > + * The sensor initialization requires below steps to be followed,
> > > + * 1. Power-up sensor.
> > > + * 2. Read and store read-registers map (0x0-0x9).
> > > + * 3. Copy values from read reserved registers to write reserved fields (0x0-0x3).
> > > + * 4. Set operating mode.
> > > + * 5. Write to all registers.
> > > + */
> > > + ret = regmap_bulk_read(data->map, TLV493D_RD_REG_BX, buff, ARRAY_SIZE(buff));
> > > + if (ret) {
> > > + dev_err(data->dev, "bulk read failed, error %d\n", ret);
> > > + return ret;
> > > + }
> > > +
> > > + data->wr_regs[0] = 0; /* Write register 0x0 is reserved. Does not require to be updated.*/
> > > + data->wr_regs[1] = buff[TLV493D_RD_REG_RES1] & TLV493D_RD_REG_RES1_WR_MASK;
> > > + data->wr_regs[2] = buff[TLV493D_RD_REG_RES2] & TLV493D_RD_REG_RES2_WR_MASK;
> > > + data->wr_regs[3] = buff[TLV493D_RD_REG_RES3] & TLV493D_RD_REG_RES3_WR_MASK;
> > > +
> > > + ret = tlv493d_set_operating_mode(data, data->mode);
> > > + if (ret < 0) {
> > > + dev_err(data->dev, "failed to set operating mode\n");
> > > + return ret;
> > > + }
> > > +
> > > + return ret;
> > return 0?
> Its the same though. ret from tlv493d_set_operating_mode is 0 on
> success and -ve on failure.
Then return 0 to make it explicit that if we get here we only return 0.
That can be useful to compilers.
Also check above as if (ret) is cleaner still.
> > > + if (ret)
> > > + val = 340;
> > > + /*
> > > + * The above is a raw offset; however, IIO expects a single effective offset.
> > > + * Since final temperature includes an additional fixed 25°C (i.e. 25000 m°C),
> > > + * we compute a combined offset using scale = 1100 (1.1 * 1000).
> > > + */
> > > + data->temp_offset = -val + (s32)div_u64(25000, 1100);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int tlv493d_read_raw(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
> >
> > wrap to keep this under 80. Doesn't look like it will hurt readability.
> >
> Ack. Is 80 standard for whole kernel or iio only?
It's kind of the the 'old' standard. Used to be a fairly hard limit, but
over time there has been some relaxation. So, if your code is nice and readable
you will rarely get anyone complaining if you stick to 80 chars.
>
> Thank you for such detailed review. Appriciate it,
You are welcome.
J
> Dixit
On Tue, Jul 29, 2025 at 08:05:13PM +0100, Jonathan Cameron wrote:
> On Tue, 29 Jul 2025 09:19:59 +0530
> Dixit Parmar <dixitparmar19@gmail.com> wrote:
>
> > On Sun, Jul 27, 2025 at 02:05:59PM +0100, Jonathan Cameron wrote:
> > > On Sat, 26 Jul 2025 15:07:01 +0530
> > > Dixit Parmar <dixitparmar19@gmail.com> wrote:
> > >
> > > Hi Dixit,
> > >
> > > Very clean driver for a v1. Nice.
> > >
> > > > The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
> > >
> > > Slightly odd wrap. Aim for 75 chars for patch descriptions.
> > >
> > Okay, 75.
> > > > applications includes joysticks, control elements (white goods,
> > > > multifunction knops), or electric meters (anti tampering) and any
> > > > other application that requires accurate angular measurements at
> > > > low power consumptions.
> > > >
> > > > The Sensor is configured over I2C, and as part of Sensor measurement
> > > > data it provides 3-Axis magnetic fields and temperature core measurement.
> > > >
> > > > The driver supports raw value read and buffered input via external trigger
> > > > to allow streaming values with the same sensing timestamp.
> > > >
> > > > The device can be configured in to different operating modes by optional
> > > > device-tree "mode" property. Also, the temperature sensing part requires
> > > > raw offset captured at 25°C and that can be specified by "temp-offset"
> > > > optional device-tree property.
> > > >
> > > > While sensor has interrupt pin multiplexed with I2C SCL pin. But for bus
> > > > configurations interrupt(INT) is not recommended, unless timing constraints
> > > > between I2C data transfers and interrupt pulses are monitored and aligned.
> > > >
> > > > The Sensor's I2C register map and mode information is described in product
> > > > User Manual[1].
> > > >
> > > > Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> > > Tag, so in the tags block (no blank line to the SoB)
> > Sorry, didn't quite get it.
>
> You should have it as Datasheet: <link>
> That will then be a formal 'tag' so belongs alongside the Signed-off-by: etc with no blank
> lines in that block.
>
> Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> Link: https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf #1
> Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
>
> > > > [1] https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf
> > > Link: https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf #1
> > >
> > > So make it a tag with a trailing comment to give the reference number.
> > >
> > > >
> > > > Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
>
> > > > +
> > > > +#define TLV493D_DATA_X_GET(b) \
> > > > + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 | \
> > > > + (FIELD_GET(TLV493D_VAL_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4), 11)
> > >
> > > These are odd enough I'd make them c functions rather than macros. Burn a few lines
> > > for better readability.
> > >
> > I saw this kind of data retrival and formation from registers as macros so I sticked to
> > it. Having all these as function will also require a seperate function
> > for each channel coz the masks and the layout of the bits changes over
> > the register. Do you still recommend it as c functions?
>
> Is it more than 4 short functions? I'd burn the few lines that costs.
>
> s32 tlv493d_data_y_get(u8 *buff)
> {
> u16 val = FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
> FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
>
> return sign_extend32(val, 11);
> }
Okay.
Will a single function with channel as arguments will be better?
> > > > +/*
> > > > + * The datasheet mentions the sensor supports only direct byte-stream write starting from
> > > > + * register address 0x0. So for any modification to be made to any write registers, it must
> > > > + * be written starting from the register address 0x0.
> > > > + * I2C write operation should not contain register address in the I2C frame, it should
> > > > + * contains only raw byte stream for the write registers. As below,
> > > > + * I2C Frame: |S|SlaveAddr Wr|Ack|Byte[0]|Ack|Byte[1]|Ack|.....|Sp|
> > > > + */
> > > > +static int tlv493d_write_all_regs(struct tlv493d_data *data)
> > > > +{
> > > > + int ret;
> > > > +
> > > > + if (!data || !data->client)
> > > > + return -EINVAL;
> > > > +
> > > > + /*
> > > > + * As regmap does not provide raw write API which perform I2C write without
> > > > + * specifying register address, direct i2c_master_send() API is used.
> > > > + */
> > > > + ret = i2c_master_send(data->client, data->wr_regs, ARRAY_SIZE(data->wr_regs));
> > >
> > > Given we have to do this, I'm a bit doubtful about regmap usage in general in here.
> > > Maybe it's just not a good fit and we should stick to direct use of the i2c stuff
> > > like here?
> > >
> > Sorry, didn't get entirely? From what I understood, you meant we could
> > drop regmap from this driver entirely and use direct I2C APIs. I believe
> > that would be too much, coz of the frequency we perform operations and
> > regmap is easier and clean imo.
>
> The mixture is nasty though :(
Indeed.
> > Also, we could have used regmap_raw_write() API by specifying register
> > 0x0 as address and rest of the 3 bytes as data. regmap will perform raw
> > write of these byte stream over the I2C the same way sensor expects. But
> > the problem with that approach is we are not using it as per the API
> > convention. let me know your thoughts? Is it a good option, it'll look
> > like this:
> > regmap_raw_write(data->map, data->wr_regs[0], &data->wr_regs[1],
> > ARRAY_SIZE(data->wr_regs) - 1);
>
> I'm not keen on that either.
>
> If you really want to mix i2c and regmap then that's fine, I was just dubious
> whether we were getting value for money from regmap here.
>
Agree. Let's switch to i2c APIs and drop regmap.
> > > > + if (ret < 0) {
> > > > + dev_err(data->dev, "failed to write registers. error %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > + *x = TLV493D_DATA_X_GET(buff);
> > > > + *y = TLV493D_DATA_Y_GET(buff);
> > > > + *z = TLV493D_DATA_Z_GET(buff);
> > > > + *t = TLV493D_DATA_TEMP_GET(buff);
> > > > +
> > > > +out:
> > > > + pm_runtime_mark_last_busy(data->dev);
> > >
> > > As below This should get simpler.
> > >
> > > Not directly relevant to this patch:
> > >
> > > If this cycle is quiet I plan to propose some cleanup.h based handling for runtime
> > > pm as it's annoying how often we need a goto for it. The new ACQUIRE() / ACQUIRE_ERR()
> > > should work for this.
> > >
> > Does this need any modifications with current codebase?
>
> Needs a bunch of work to show generality across many drivers and
> convincing Rafael (PM maintainer) it's a good idea.
> Don't try to do it in this series!
>
> > >
> > > > + pm_runtime_put_autosuspend(data->dev);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int tlv493d_init(struct tlv493d_data *data)
> > > > +{
> > > > + int ret;
> > > > + u8 buff[TLV493D_RD_REG_MAX];
> > > > +
> > > > + if (!data)
> > > > + return -EINVAL;
> > > > +
> > > > + /*
> > > > + * The sensor initialization requires below steps to be followed,
> > > > + * 1. Power-up sensor.
> > > > + * 2. Read and store read-registers map (0x0-0x9).
> > > > + * 3. Copy values from read reserved registers to write reserved fields (0x0-0x3).
> > > > + * 4. Set operating mode.
> > > > + * 5. Write to all registers.
> > > > + */
> > > > + ret = regmap_bulk_read(data->map, TLV493D_RD_REG_BX, buff, ARRAY_SIZE(buff));
> > > > + if (ret) {
> > > > + dev_err(data->dev, "bulk read failed, error %d\n", ret);
> > > > + return ret;
> > > > + }
> > > > +
> > > > + data->wr_regs[0] = 0; /* Write register 0x0 is reserved. Does not require to be updated.*/
> > > > + data->wr_regs[1] = buff[TLV493D_RD_REG_RES1] & TLV493D_RD_REG_RES1_WR_MASK;
> > > > + data->wr_regs[2] = buff[TLV493D_RD_REG_RES2] & TLV493D_RD_REG_RES2_WR_MASK;
> > > > + data->wr_regs[3] = buff[TLV493D_RD_REG_RES3] & TLV493D_RD_REG_RES3_WR_MASK;
> > > > +
> > > > + ret = tlv493d_set_operating_mode(data, data->mode);
> > > > + if (ret < 0) {
> > > > + dev_err(data->dev, "failed to set operating mode\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + return ret;
> > > return 0?
> > Its the same though. ret from tlv493d_set_operating_mode is 0 on
> > success and -ve on failure.
>
> Then return 0 to make it explicit that if we get here we only return 0.
> That can be useful to compilers.
>
> Also check above as if (ret) is cleaner still.
>
>
> > > > + if (ret)
> > > > + val = 340;
> > > > + /*
> > > > + * The above is a raw offset; however, IIO expects a single effective offset.
> > > > + * Since final temperature includes an additional fixed 25°C (i.e. 25000 m°C),
> > > > + * we compute a combined offset using scale = 1100 (1.1 * 1000).
> > > > + */
> > > > + data->temp_offset = -val + (s32)div_u64(25000, 1100);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int tlv493d_read_raw(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
> > >
> > > wrap to keep this under 80. Doesn't look like it will hurt readability.
> > >
> > Ack. Is 80 standard for whole kernel or iio only?
>
> It's kind of the the 'old' standard. Used to be a fairly hard limit, but
> over time there has been some relaxation. So, if your code is nice and readable
> you will rarely get anyone complaining if you stick to 80 chars.
>
> >
> > Thank you for such detailed review. Appriciate it,
> You are welcome.
>
> J
> > Dixit
>
Please crop to the remaining discussion points.
> > > > > +
> > > > > +#define TLV493D_DATA_X_GET(b) \
> > > > > + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 | \
> > > > > + (FIELD_GET(TLV493D_VAL_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4), 11)
> > > >
> > > > These are odd enough I'd make them c functions rather than macros. Burn a few lines
> > > > for better readability.
> > > >
> > > I saw this kind of data retrival and formation from registers as macros so I sticked to
> > > it. Having all these as function will also require a seperate function
> > > for each channel coz the masks and the layout of the bits changes over
> > > the register. Do you still recommend it as c functions?
> >
> > Is it more than 4 short functions? I'd burn the few lines that costs.
> >
> > s32 tlv493d_data_y_get(u8 *buff)
> > {
> > u16 val = FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
> > FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
> >
> > return sign_extend32(val, 11);
> > }
> Okay.
> Will a single function with channel as arguments will be better?
IIRC I gave that a go as my first try before falling back to this.
You either need a look up table, or you need to pass
a lot of parameters. In the end it felt simpler to just have 4 small functions.
If you can come up with a clean and readable way of doing so, go for it!
Jonathan
Le 26/07/2025 à 11:37, Dixit Parmar a écrit :
> The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
> applications includes joysticks, control elements (white goods,
> multifunction knops), or electric meters (anti tampering) and any
> other application that requires accurate angular measurements at
> low power consumptions.
Hi,
> + ret = read_poll_timeout(regmap_bulk_read, err, err ||
> + FIELD_GET(TLV493D_VAL_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> + mode->sleep_us, (3 * mode->sleep_us), false, data->map, TLV493D_RD_REG_BX,
> + buff, ARRAY_SIZE(buff));
> + if (ret) {
> + dev_err(data->dev, "read poll timeout, error:%d", ret);
Nitpick: missing trailing \n
> + goto out;
> + }
> + if (err) {
> + dev_err(data->dev, "read data failed, error:%d\n", ret);
> + ret = err;
> + goto out;
> + }
...
> + ret = tlv493d_init(data);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to initialized\n");
Nitpick: to initialize (without a d)
> +
> + indio_dev->info = &tlv493d_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
...
CJ
On Sun, Jul 27, 2025 at 12:03:59AM +0200, Christophe JAILLET wrote:
> Le 26/07/2025 à 11:37, Dixit Parmar a écrit :
> > The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
> > applications includes joysticks, control elements (white goods,
> > multifunction knops), or electric meters (anti tampering) and any
> > other application that requires accurate angular measurements at
> > low power consumptions.
>
> Hi,
>
> > + ret = read_poll_timeout(regmap_bulk_read, err, err ||
> > + FIELD_GET(TLV493D_VAL_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> > + mode->sleep_us, (3 * mode->sleep_us), false, data->map, TLV493D_RD_REG_BX,
> > + buff, ARRAY_SIZE(buff));
> > + if (ret) {
> > + dev_err(data->dev, "read poll timeout, error:%d", ret);
>
> Nitpick: missing trailing \n
>
Ack.
> > + goto out;
> > + }
> > + if (err) {
> > + dev_err(data->dev, "read data failed, error:%d\n", ret);
> > + ret = err;
> > + goto out;
> > + }
>
> ...
>
> > + ret = tlv493d_init(data);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to initialized\n");
>
> Nitpick: to initialize (without a d)
>
Ack.
> > +
> > + indio_dev->info = &tlv493d_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
>
> ...
>
> CJ
On 7/26/25 4:37 AM, Dixit Parmar wrote:
> The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
> applications includes joysticks, control elements (white goods,
> multifunction knops), or electric meters (anti tampering) and any
> other application that requires accurate angular measurements at
> low power consumptions.
>
> The Sensor is configured over I2C, and as part of Sensor measurement
> data it provides 3-Axis magnetic fields and temperature core measurement.
>
> The driver supports raw value read and buffered input via external trigger
> to allow streaming values with the same sensing timestamp.
>
> The device can be configured in to different operating modes by optional
> device-tree "mode" property. Also, the temperature sensing part requires
> raw offset captured at 25°C and that can be specified by "temp-offset"
> optional device-tree property.
>
> While sensor has interrupt pin multiplexed with I2C SCL pin. But for bus
> configurations interrupt(INT) is not recommended, unless timing constraints
> between I2C data transfers and interrupt pulses are monitored and aligned.
>
> The Sensor's I2C register map and mode information is described in product
> User Manual[1].
>
> Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> [1] https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf
>
> Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> ---
> drivers/iio/magnetometer/Kconfig | 14 +
> drivers/iio/magnetometer/Makefile | 2 +
> drivers/iio/magnetometer/tlv493d.c | 606 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 622 insertions(+)
>
> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> index 3debf1320ad1..e0070dccc751 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -246,6 +246,20 @@ config SI7210
> To compile this driver as a module, choose M here: the module
> will be called si7210.
>
> +config TLV493D
> + tristate "Infineon TLV493D Low-Power 3D Magnetic Sensor"
> + depends on I2C
> + select REGMAP_I2C
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + Say Y here to add support for the Infineon TLV493D-A1B6 Low-
> + Power 3D Megnetic Sensor.
> +
> + This driver can also be compiled as a module.
> + To compile this driver as a module, choose M here: the module
> + will be called tlv493d.
> +
> config TI_TMAG5273
> tristate "TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor"
> depends on I2C
> diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
> index 9297723a97d8..39c62dd06db8 100644
> --- a/drivers/iio/magnetometer/Makefile
> +++ b/drivers/iio/magnetometer/Makefile
> @@ -35,4 +35,6 @@ obj-$(CONFIG_SI7210) += si7210.o
>
> obj-$(CONFIG_TI_TMAG5273) += tmag5273.o
>
> +obj-$(CONFIG_TLV493D) += tlv493d.o
We try to keep these in alphabetical order.
> +
> obj-$(CONFIG_YAMAHA_YAS530) += yamaha-yas530.o
> diff --git a/drivers/iio/magnetometer/tlv493d.c b/drivers/iio/magnetometer/tlv493d.c
> new file mode 100644
> index 000000000000..f230d6409a4b
> --- /dev/null
> +++ b/drivers/iio/magnetometer/tlv493d.c
> @@ -0,0 +1,606 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/**
> + * Driver for the Infineon TLV493D Low-Power 3D Magnetic Sensor
> + *
> + * Copyright (C) 2025 Dixit Parmar <dixitparmar19@gmail.com>
> + *
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define TLV493D_RD_REG_BX 0x00
> +#define TLV493D_RD_REG_BY 0x01
> +#define TLV493D_RD_REG_BZ 0x02
> +#define TLV493D_RD_REG_TEMP 0x03
> +#define TLV493D_RD_REG_BX2 0x04
> +#define TLV493D_RD_REG_BZ2 0x05
> +#define TLV493D_RD_REG_TEMP2 0x06
> +#define TLV493D_RD_REG_RES1 0x07
> +#define TLV493D_RD_REG_RES2 0x08
> +#define TLV493D_RD_REG_RES3 0x09
> +#define TLV493D_RD_REG_MAX 0x0a
> +#define TLV493D_WR_REG_RES 0x00
> +#define TLV493D_WR_REG_MODE1 0x01
> +#define TLV493D_WR_REG_RES2 0x02
> +#define TLV493D_WR_REG_MODE2 0x03
> +#define TLV493D_WR_REG_MAX 0x04
> +#define TLV493D_VAL_MAG_X_AXIS_MSB GENMASK(7, 0)
> +#define TLV493D_VAL_MAG_X_AXIS_LSB GENMASK(7, 4)
> +#define TLV493D_VAL_MAG_Y_AXIS_MSB GENMASK(7, 0)
> +#define TLV493D_VAL_MAG_Y_AXIS_LSB GENMASK(3, 0)
> +#define TLV493D_VAL_MAG_Z_AXIS_MSB GENMASK(7, 0)
> +#define TLV493D_VAL_MAG_Z_AXIS_LSB GENMASK(3, 0)
> +#define TLV493D_VAL_TEMP_MSB GENMASK(7, 4)
> +#define TLV493D_VAL_TEMP_LSB GENMASK(7, 0)
> +#define TLV493D_VAL_FRAME_COUNTER GENMASK(3, 2)
> +#define TLV493D_VAL_CHANNEL GENMASK(1, 0)
> +#define TLV493D_VAL_PD_FLAG BIT(4)
> +#define TLV493D_RD_REG_RES1_WR_MASK GENMASK(4, 3)
> +#define TLV493D_RD_REG_RES2_WR_MASK GENMASK(7, 0)
> +#define TLV493D_RD_REG_RES3_WR_MASK GENMASK(4, 0)
> +#define TLV493D_MODE1_MOD_FAST BIT(1)
> +#define TLV493D_MODE1_MOD_LOW BIT(0)
> +#define TLV493D_MODE2_TEMP_CTRL BIT(7)
> +#define TLV493D_MODE2_LP_PERIOD BIT(6)
> +#define TLV493D_MODE2_PARITY_CTRL BIT(5)
> +
> +#define SET_BIT(b, bit) (b |= bit)
> +#define CLR_BIT(b, bit) (b &= ~bit)
I think most readers of the code would fare better without these macros.
And it could simplify things to make a
#define TLV493D_MODE1_MOD GENMASK(1, 0)
and use FIELD_PREP() instead of
> +
> +#define TLV493D_DATA_X_GET(b) \
> + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 | \
> + (FIELD_GET(TLV493D_VAL_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4), 11)
> +#define TLV493D_DATA_Y_GET(b) \
> + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 | \
> + FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]), 11)
> +#define TLV493D_DATA_Z_GET(b) \
> + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_Z_AXIS_MSB, b[TLV493D_RD_REG_BZ]) << 4 | \
> + FIELD_GET(TLV493D_VAL_MAG_Z_AXIS_LSB, b[TLV493D_RD_REG_BZ2]), 11)
> +#define TLV493D_DATA_TEMP_GET(b) \
> + sign_extend32(FIELD_GET(TLV493D_VAL_TEMP_MSB, b[TLV493D_RD_REG_TEMP]) << 8 | \
> + FIELD_GET(TLV493D_VAL_TEMP_LSB, b[TLV493D_RD_REG_TEMP2]), 11)
> +
> +enum tlv493d_channels {
> + AXIS_X = 0,
> + AXIS_Y,
> + AXIS_Z,
> + TEMPERATURE,
These are very generic names. Please add TLV493D_ prefix.
> +};
> +
> +enum tlv493d_op_mode {
> + TLV493D_OP_MODE_POWERDOWN = 0,
> + TLV493D_OP_MODE_FAST,
> + TLV493D_OP_MODE_LOWPOWER,
> + TLV493D_OP_MODE_ULTRA_LOWPOWER,
> + TLV493D_OP_MODE_MASTERCONTROLLED,
> + TLV493D_OP_MODE_MAX,
> +};
> +
> +struct tlv493d_mode {
> + u8 m;
> + u32 sleep_us;
> +};
> +
> +struct tlv493d_data {
> + struct device *dev;
> + struct i2c_client *client;
> + struct mutex lock;
Lock needs a comment describing what it is supposed to protect.
> + struct regmap *map;
> + u8 mode;
> + u8 wr_regs[TLV493D_WR_REG_MAX];
> + s32 temp_offset;
> +};
> +
> +/*
> + * Different mode has different measurement cycle time, this time is
> + * used in deriving the sleep and timemout while reading the data from
> + * sensor in polling.
> + * Power-down mode: No measurement.
> + * Fast mode: Freq:3.3 KHz. Measurement time:305 usec.
> + * Low-power mode: Freq:100 Hz. Measurement time:10 msec.
> + * Ultra low-power mode: Freq:10 Hz. Measurement time:100 msec.
> + * Master controlled mode: Freq:3.3 Khz. Measurement time:305 usec.
> + */
> +static struct tlv493d_mode modes[TLV493D_OP_MODE_MAX] = {
Let's add the tlv493d_ prefix to modes.
> + {.m = TLV493D_OP_MODE_POWERDOWN, .sleep_us = 0 },
> + {.m = TLV493D_OP_MODE_FAST, .sleep_us = 305 },
> + {.m = TLV493D_OP_MODE_LOWPOWER, .sleep_us = 10 * USEC_PER_MSEC },
> + {.m = TLV493D_OP_MODE_ULTRA_LOWPOWER, .sleep_us = 100 * USEC_PER_MSEC },
> + {.m = TLV493D_OP_MODE_MASTERCONTROLLED, .sleep_us = 305 },
> +};
> +
> +/*
> + * The datasheet mentions the sensor supports only direct byte-stream write starting from
> + * register address 0x0. So for any modification to be made to any write registers, it must
> + * be written starting from the register address 0x0.
> + * I2C write operation should not contain register address in the I2C frame, it should
> + * contains only raw byte stream for the write registers. As below,
> + * I2C Frame: |S|SlaveAddr Wr|Ack|Byte[0]|Ack|Byte[1]|Ack|.....|Sp|
> + */
> +static int tlv493d_write_all_regs(struct tlv493d_data *data)
> +{
> + int ret;
> +
> + if (!data || !data->client)
> + return -EINVAL;
> +
> + /*
> + * As regmap does not provide raw write API which perform I2C write without
> + * specifying register address, direct i2c_master_send() API is used.
> + */
> + ret = i2c_master_send(data->client, data->wr_regs, ARRAY_SIZE(data->wr_regs));
> + if (ret < 0) {
> + dev_err(data->dev, "failed to write registers. error %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int tlv493d_set_operating_mode(struct tlv493d_data *data, u8 mode)
> +{
> + if (!data)
> + return -EINVAL;
> +
> + u8 *reg_mode1 = &data->wr_regs[TLV493D_WR_REG_MODE1];
> + u8 *reg_mode2 = &data->wr_regs[TLV493D_WR_REG_MODE2];
> +
> + switch (mode) {
> + case TLV493D_OP_MODE_POWERDOWN:
> + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> + break;
> +
> + case TLV493D_OP_MODE_FAST:
> + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> + break;
> +
> + case TLV493D_OP_MODE_LOWPOWER:
> + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> + SET_BIT(*reg_mode2, TLV493D_MODE2_LP_PERIOD);
> + break;
> +
> + case TLV493D_OP_MODE_ULTRA_LOWPOWER:
> + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> + CLR_BIT(*reg_mode2, TLV493D_MODE2_LP_PERIOD);
> + break;
> +
> + case TLV493D_OP_MODE_MASTERCONTROLLED:
> + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> + break;
> +
> + default:
> + dev_err(data->dev, "invalid mode configuration\n");
> + return -EINVAL;
> + }
> +
> + return tlv493d_write_all_regs(data);
> +}
> +
> +static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
> + s16 *z, s16 *t)
> +{
> + u8 buff[7] = {0};
> + int err, ret;
> + struct tlv493d_mode *mode;
> +
> + if (!data)
> + return -EINVAL;
> +
> + guard(mutex)(&data->lock);
> +
> + ret = pm_runtime_resume_and_get(data->dev);
> + if (ret < 0)
> + return ret;
> +
> + mode = &modes[data->mode];
> +
> + /*
> + * Poll until data is valid,
> + * For a valid data TLV493D_VAL_CHANNEL bit of TLV493D_RD_REG_TEMP should be set to 0.
> + * The sampling time depends on the sensor mode. poll 3x the time of the sampling time.
> + */
> + ret = read_poll_timeout(regmap_bulk_read, err, err ||
Why not regmap_read_poll_timeout()?
> + FIELD_GET(TLV493D_VAL_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> + mode->sleep_us, (3 * mode->sleep_us), false, data->map, TLV493D_RD_REG_BX,
> + buff, ARRAY_SIZE(buff));
> + if (ret) {
> + dev_err(data->dev, "read poll timeout, error:%d", ret);
> + goto out;
> + }
> + if (err) {
> + dev_err(data->dev, "read data failed, error:%d\n", ret);
> + ret = err;
> + goto out;
> + }
> +
> + *x = TLV493D_DATA_X_GET(buff);
> + *y = TLV493D_DATA_Y_GET(buff);
> + *z = TLV493D_DATA_Z_GET(buff);
> + *t = TLV493D_DATA_TEMP_GET(buff);
> +
> +out:
> + pm_runtime_mark_last_busy(data->dev);
> + pm_runtime_put_autosuspend(data->dev);
> + return ret;
> +}
> +
> +static int tlv493d_init(struct tlv493d_data *data)
> +{
> + int ret;
> + u8 buff[TLV493D_RD_REG_MAX];
> +
> + if (!data)
> + return -EINVAL;
> +
> + /*
> + * The sensor initialization requires below steps to be followed,
> + * 1. Power-up sensor.
> + * 2. Read and store read-registers map (0x0-0x9).
> + * 3. Copy values from read reserved registers to write reserved fields (0x0-0x3).
> + * 4. Set operating mode.
> + * 5. Write to all registers.
> + */
> + ret = regmap_bulk_read(data->map, TLV493D_RD_REG_BX, buff, ARRAY_SIZE(buff));
> + if (ret) {
> + dev_err(data->dev, "bulk read failed, error %d\n", ret);
> + return ret;
> + }
> +
> + data->wr_regs[0] = 0; /* Write register 0x0 is reserved. Does not require to be updated.*/
> + data->wr_regs[1] = buff[TLV493D_RD_REG_RES1] & TLV493D_RD_REG_RES1_WR_MASK;
> + data->wr_regs[2] = buff[TLV493D_RD_REG_RES2] & TLV493D_RD_REG_RES2_WR_MASK;
> + data->wr_regs[3] = buff[TLV493D_RD_REG_RES3] & TLV493D_RD_REG_RES3_WR_MASK;
> +
> + ret = tlv493d_set_operating_mode(data, data->mode);
> + if (ret < 0) {
> + dev_err(data->dev, "failed to set operating mode\n");
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +static int tlv493d_parse_dt(struct tlv493d_data *data)
> +{
> + struct device_node *node;
> + u32 val = 0;
> + int ret;
> +
> + if (!data)
> + return -EINVAL;
> +
> + node = data->dev->of_node;
> +
> + /* Optional 'mode' property to set sensor operation mode */
> + ret = of_property_read_u32(node, "mode", &val);
> + if (ret < 0 || val >= TLV493D_OP_MODE_MAX) {
> + /* Fallback to default mode if property is missing or invalid */
> + data->mode = TLV493D_OP_MODE_MASTERCONTROLLED;
> + } else {
> + data->mode = val;
> + }
As mentioned in the dt-bindings review, we already control this paritially
with the power management runtime, so having devicetree specify a power mode
doesn't really make sense.
In fact, since the mode determines the sample rate, if we ever implemented
handling the interrupt pin, it would make sense for the sampling_freqency
to control mode rather than hard-coding it.
> +
> + /*
> + * Read temperature offset (raw value at 25°C). If not specified,
> + * default to 340.
> + */
> + ret = of_property_read_u32(node, "temp-offset", &val);
> + if (ret)
> + val = 340;
As mentioned in the dt-bindings review, this sounds like a calibbias rather
than something that should be hard-coded in a devicetree.
> + /*
> + * The above is a raw offset; however, IIO expects a single effective offset.
> + * Since final temperature includes an additional fixed 25°C (i.e. 25000 m°C),
> + * we compute a combined offset using scale = 1100 (1.1 * 1000).
> + */
> + data->temp_offset = -val + (s32)div_u64(25000, 1100);
> +
> + return 0;
> +}
> +
> +static int tlv493d_read_raw(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct tlv493d_data *data = iio_priv(indio_dev);
> + s16 x, y, z, t;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
Processed is not the same as raw. Just drop it.
> + case IIO_CHAN_INFO_RAW:
> + ret = tlv493d_get_measurements(data, &x, &y, &z, &t);
> + if (ret) {
> + dev_err(data->dev, "failed to read sensor data\n");
The error gets returned to usespace, so we don't need to log errors here.
> + return ret;
> + }
> + /* Return raw values for requested channel */
> + switch (chan->address) {
> + case AXIS_X:
> + *val = x;
> + return IIO_VAL_INT;
> + case AXIS_Y:
> + *val = y;
> + return IIO_VAL_INT;
> + case AXIS_Z:
> + *val = z;
> + return IIO_VAL_INT;
> + case TEMPERATURE:
> + *val = t;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_MAGN:
> + /*
> + * Magnetic field scale: 0.0098 mTesla (i.e. 9.8 µT)
> + * Expressed as fractional: 98/10 = 9.8 µT.
> + */
> + *val = 98;
> + *val2 = 10;
We use SI units, so this needs to be gauss, not tesela.
> + return IIO_VAL_FRACTIONAL;
> + case IIO_TEMP:
> + /*
> + * Temperature scale: 1.1 °C per LSB, expressed as 1100 m°C
> + * Returned as integer for IIO core to apply:
> + * temp = (raw + offset) * scale
> + */
> + *val = 1.1 * MILLI;
I guess this works since it is a constant, but we usually don't
have floating point in the kernel. I would probably just write this
as 1100. It doesn't have too many zeros that we can't easily see
how many there are.
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_OFFSET:
> + switch (chan->type) {
> + case IIO_TEMP:
> + /*
> + * Temperature offset includes sensor-specific raw offset
> + * plus compensation for +25°C bias in formula.
> + * This value is precomputed during probe/init:
> + * offset = -raw_offset + (25000 / scale)
> + */
> + *val = data->temp_offset;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
There is no break; so this is unreachable and can be removed.
> +}
> +
> +static irqreturn_t tlv493d_trigger_handler(int irq, void *ptr)
> +{
> + struct iio_poll_func *pf = ptr;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct tlv493d_data *data = iio_priv(indio_dev);
> +
> + struct {
> + s16 channels[3];
> + s16 temperature;
> + aligned_s64 timestamp;
> + } scan;
} scan = { };
Technically not needed here since we assign all values and there shuold
not be any holes in the struct, but good to have for when others copy this
and modify it in a new driver.
> +
> + s16 x, y, z, t;
> + int ret;
> +
> + ret = tlv493d_get_measurements(data, &x, &y, &z, &t);
> + if (ret) {
> + dev_err(data->dev, "failed to read sensor data\n");
> + goto trig_out;
> + }
> +
> + scan.channels[0] = x;
> + scan.channels[1] = y;
> + scan.channels[2] = z;
> + scan.temperature = t;
Why not just pass these directly to tlv493d_get_measurements() and avoid this
assigment and extra local variables?
> +
> + iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan), pf->timestamp);
> +
> +trig_out:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +#define TLV493D_AXIS_CHANNEL(axis, index) \
> + { \
> + .type = IIO_MAGN, \
> + .modified = 1, \
> + .channel2 = IIO_MOD_##axis, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .address = index, \
Setting address the same as scan_index is redundant. We can just
use scan_index everywhere.
> + .scan_index = index, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 12, \
> + .storagebits = 16, \
> + .endianness = IIO_CPU, \
> + }, \
> + }
> +
> +static const struct iio_chan_spec tlv493d_channels[] = {
> + TLV493D_AXIS_CHANNEL(X, AXIS_X),
> + TLV493D_AXIS_CHANNEL(Y, AXIS_Y),
> + TLV493D_AXIS_CHANNEL(Z, AXIS_Z),
> + {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_OFFSET),
> + .address = TEMPERATURE,
> + .scan_index = TEMPERATURE,
> + .scan_type = {
> + .sign = 's',
> + .realbits = 12,
> + .storagebits = 16,
> + .endianness = IIO_CPU,
> + },
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +static const struct regmap_range tlv493d_volatile_reg_ranges[] = {
> + regmap_reg_range(TLV493D_RD_REG_BX, TLV493D_RD_REG_RES3),
> +};
> +
> +static const struct regmap_access_table tlv493d_volatile_regs = {
> + .yes_ranges = tlv493d_volatile_reg_ranges,
> + .n_yes_ranges = ARRAY_SIZE(tlv493d_volatile_reg_ranges),
> +};
Would make sense to have these closer to tlv493d_regmap_config.
> +
> +static const struct iio_info tlv493d_info = {
> + .read_raw = tlv493d_read_raw,
> +};
> +
> +static const struct iio_buffer_setup_ops tlv493d_setup_ops = {};
Just pass NULL if there are no ops.
> +
> +static const unsigned long tlv493d_scan_masks[] = { GENMASK(3, 0), 0 };
> +
> +static const struct regmap_config tlv493d_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = TLV493D_RD_REG_RES3,
> + .volatile_table = &tlv493d_volatile_regs,
> +};
> +
> +static int tlv493d_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct iio_dev *indio_dev;
> + struct tlv493d_data *data;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + data->dev = dev;
> + data->client = client;
> + i2c_set_clientdata(client, indio_dev);
There is not i2c_get_clientdata(), so I don't think we need this.
> +
> + ret = devm_mutex_init(dev, &data->lock);
> + if (ret)
> + return ret;
> +
> + data->map = devm_regmap_init_i2c(client, &tlv493d_regmap_config);
> + if (IS_ERR(data->map))
> + return dev_err_probe(dev, PTR_ERR(data->map), "failed to allocate register map\n");
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to enable regulator\n");
> +
> + ret = tlv493d_parse_dt(data);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to parse device-tree\n");
> +
> + ret = tlv493d_init(data);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to initialized\n");
> +
> + indio_dev->info = &tlv493d_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->name = client->name;
> + indio_dev->channels = tlv493d_channels;
> + indio_dev->num_channels = ARRAY_SIZE(tlv493d_channels);
> + indio_dev->available_scan_masks = tlv493d_scan_masks;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> + iio_pollfunc_store_time, tlv493d_trigger_handler,
> + &tlv493d_setup_ops);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "iio triggered buffer setup failed\n");
> +
> + ret = pm_runtime_set_active(dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = devm_pm_runtime_enable(dev);
> + if (ret < 0)
> + return ret;
> +
> + pm_runtime_get_noresume(dev);
> + pm_runtime_set_autosuspend_delay(dev, 500);
> + pm_runtime_use_autosuspend(dev);
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +
> + ret = devm_iio_device_register(dev, indio_dev);
random extra space
> + if (ret)
> + return dev_err_probe(dev, ret, "iio device register failed\n");
> +
> + return 0;
> +}
> +
> +static int tlv493d_runtime_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tlv493d_data *data = iio_priv(indio_dev);
> +
> + return tlv493d_set_operating_mode(data, TLV493D_OP_MODE_POWERDOWN);
> +}
> +
> +static int tlv493d_runtime_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct tlv493d_data *data = iio_priv(indio_dev);
> +
> + return tlv493d_set_operating_mode(data, data->mode);
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(tlv493d_pm_ops,
> + tlv493d_runtime_suspend, tlv493d_runtime_resume, NULL);
> +
> +static const struct i2c_device_id tlv493d_id[] = {
> + { "tlv493d" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, tlv493d_id);
> +
> +static const struct of_device_id tlv493d_of_match[] = {
> + { .compatible = "infineon,tlv493d-a1b6", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, tlv493d_of_match);
> +
> +static struct i2c_driver tlv493d_driver = {
> + .driver = {
> + .name = "tlv493d",
> + .of_match_table = tlv493d_of_match,
> + .pm = pm_ptr(&tlv493d_pm_ops),
> + },
> + .probe = tlv493d_probe,
> + .id_table = tlv493d_id,
> +};
> +
> +module_i2c_driver(tlv493d_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Infineon TLV493D Low-Power 3D Magnetic Sensor");
> +MODULE_AUTHOR("Dixit Parmar <dixitparmar19@gmail.com>");
>
On Sat, Jul 26, 2025 at 03:44:03PM -0500, David Lechner wrote:
> On 7/26/25 4:37 AM, Dixit Parmar wrote:
> > The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
> > applications includes joysticks, control elements (white goods,
> > multifunction knops), or electric meters (anti tampering) and any
> > other application that requires accurate angular measurements at
> > low power consumptions.
> >
> > The Sensor is configured over I2C, and as part of Sensor measurement
> > data it provides 3-Axis magnetic fields and temperature core measurement.
> >
> > The driver supports raw value read and buffered input via external trigger
> > to allow streaming values with the same sensing timestamp.
> >
> > The device can be configured in to different operating modes by optional
> > device-tree "mode" property. Also, the temperature sensing part requires
> > raw offset captured at 25°C and that can be specified by "temp-offset"
> > optional device-tree property.
> >
> > While sensor has interrupt pin multiplexed with I2C SCL pin. But for bus
> > configurations interrupt(INT) is not recommended, unless timing constraints
> > between I2C data transfers and interrupt pulses are monitored and aligned.
> >
> > The Sensor's I2C register map and mode information is described in product
> > User Manual[1].
> >
> > Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> > [1] https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf
> >
> > Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> > ---
> > drivers/iio/magnetometer/Kconfig | 14 +
> > drivers/iio/magnetometer/Makefile | 2 +
> > drivers/iio/magnetometer/tlv493d.c | 606 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 622 insertions(+)
> >
> > diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> > index 3debf1320ad1..e0070dccc751 100644
> > --- a/drivers/iio/magnetometer/Kconfig
> > +++ b/drivers/iio/magnetometer/Kconfig
> > @@ -246,6 +246,20 @@ config SI7210
> > To compile this driver as a module, choose M here: the module
> > will be called si7210.
> >
> > +config TLV493D
> > + tristate "Infineon TLV493D Low-Power 3D Magnetic Sensor"
> > + depends on I2C
> > + select REGMAP_I2C
> > + select IIO_BUFFER
> > + select IIO_TRIGGERED_BUFFER
> > + help
> > + Say Y here to add support for the Infineon TLV493D-A1B6 Low-
> > + Power 3D Megnetic Sensor.
> > +
> > + This driver can also be compiled as a module.
> > + To compile this driver as a module, choose M here: the module
> > + will be called tlv493d.
> > +
> > config TI_TMAG5273
> > tristate "TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor"
> > depends on I2C
> > diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
> > index 9297723a97d8..39c62dd06db8 100644
> > --- a/drivers/iio/magnetometer/Makefile
> > +++ b/drivers/iio/magnetometer/Makefile
> > @@ -35,4 +35,6 @@ obj-$(CONFIG_SI7210) += si7210.o
> >
> > obj-$(CONFIG_TI_TMAG5273) += tmag5273.o
> >
> > +obj-$(CONFIG_TLV493D) += tlv493d.o
>
> We try to keep these in alphabetical order.
>
Ofcourse, I considered TI_TMAG5273 as whole. Will move it above that.
> > +
> > obj-$(CONFIG_YAMAHA_YAS530) += yamaha-yas530.o
> > diff --git a/drivers/iio/magnetometer/tlv493d.c b/drivers/iio/magnetometer/tlv493d.c
> > new file mode 100644
> > index 000000000000..f230d6409a4b
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/tlv493d.c
> > @@ -0,0 +1,606 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/**
> > + * Driver for the Infineon TLV493D Low-Power 3D Magnetic Sensor
> > + *
> > + * Copyright (C) 2025 Dixit Parmar <dixitparmar19@gmail.com>
> > + *
> > + */
> > +
> > +#include <linux/bits.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/types.h>
> > +#include <linux/units.h>
> > +
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +
> > +#define TLV493D_RD_REG_BX 0x00
> > +#define TLV493D_RD_REG_BY 0x01
> > +#define TLV493D_RD_REG_BZ 0x02
> > +#define TLV493D_RD_REG_TEMP 0x03
> > +#define TLV493D_RD_REG_BX2 0x04
> > +#define TLV493D_RD_REG_BZ2 0x05
> > +#define TLV493D_RD_REG_TEMP2 0x06
> > +#define TLV493D_RD_REG_RES1 0x07
> > +#define TLV493D_RD_REG_RES2 0x08
> > +#define TLV493D_RD_REG_RES3 0x09
> > +#define TLV493D_RD_REG_MAX 0x0a
> > +#define TLV493D_WR_REG_RES 0x00
> > +#define TLV493D_WR_REG_MODE1 0x01
> > +#define TLV493D_WR_REG_RES2 0x02
> > +#define TLV493D_WR_REG_MODE2 0x03
> > +#define TLV493D_WR_REG_MAX 0x04
> > +#define TLV493D_VAL_MAG_X_AXIS_MSB GENMASK(7, 0)
> > +#define TLV493D_VAL_MAG_X_AXIS_LSB GENMASK(7, 4)
> > +#define TLV493D_VAL_MAG_Y_AXIS_MSB GENMASK(7, 0)
> > +#define TLV493D_VAL_MAG_Y_AXIS_LSB GENMASK(3, 0)
> > +#define TLV493D_VAL_MAG_Z_AXIS_MSB GENMASK(7, 0)
> > +#define TLV493D_VAL_MAG_Z_AXIS_LSB GENMASK(3, 0)
> > +#define TLV493D_VAL_TEMP_MSB GENMASK(7, 4)
> > +#define TLV493D_VAL_TEMP_LSB GENMASK(7, 0)
> > +#define TLV493D_VAL_FRAME_COUNTER GENMASK(3, 2)
> > +#define TLV493D_VAL_CHANNEL GENMASK(1, 0)
> > +#define TLV493D_VAL_PD_FLAG BIT(4)
> > +#define TLV493D_RD_REG_RES1_WR_MASK GENMASK(4, 3)
> > +#define TLV493D_RD_REG_RES2_WR_MASK GENMASK(7, 0)
> > +#define TLV493D_RD_REG_RES3_WR_MASK GENMASK(4, 0)
> > +#define TLV493D_MODE1_MOD_FAST BIT(1)
> > +#define TLV493D_MODE1_MOD_LOW BIT(0)
> > +#define TLV493D_MODE2_TEMP_CTRL BIT(7)
> > +#define TLV493D_MODE2_LP_PERIOD BIT(6)
> > +#define TLV493D_MODE2_PARITY_CTRL BIT(5)
> > +
> > +#define SET_BIT(b, bit) (b |= bit)
> > +#define CLR_BIT(b, bit) (b &= ~bit)
>
> I think most readers of the code would fare better without these macros.
>
> And it could simplify things to make a
>
> #define TLV493D_MODE1_MOD GENMASK(1, 0)
>
> and use FIELD_PREP() instead of
>
Ack.
> > +
> > +#define TLV493D_DATA_X_GET(b) \
> > + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 | \
> > + (FIELD_GET(TLV493D_VAL_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4), 11)
> > +#define TLV493D_DATA_Y_GET(b) \
> > + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 | \
> > + FIELD_GET(TLV493D_VAL_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]), 11)
> > +#define TLV493D_DATA_Z_GET(b) \
> > + sign_extend32(FIELD_GET(TLV493D_VAL_MAG_Z_AXIS_MSB, b[TLV493D_RD_REG_BZ]) << 4 | \
> > + FIELD_GET(TLV493D_VAL_MAG_Z_AXIS_LSB, b[TLV493D_RD_REG_BZ2]), 11)
> > +#define TLV493D_DATA_TEMP_GET(b) \
> > + sign_extend32(FIELD_GET(TLV493D_VAL_TEMP_MSB, b[TLV493D_RD_REG_TEMP]) << 8 | \
> > + FIELD_GET(TLV493D_VAL_TEMP_LSB, b[TLV493D_RD_REG_TEMP2]), 11)
> > +
> > +enum tlv493d_channels {
> > + AXIS_X = 0,
> > + AXIS_Y,
> > + AXIS_Z,
> > + TEMPERATURE,
>
> These are very generic names. Please add TLV493D_ prefix.
>
Indeed.
> > +};
> > +
> > +enum tlv493d_op_mode {
> > + TLV493D_OP_MODE_POWERDOWN = 0,
> > + TLV493D_OP_MODE_FAST,
> > + TLV493D_OP_MODE_LOWPOWER,
> > + TLV493D_OP_MODE_ULTRA_LOWPOWER,
> > + TLV493D_OP_MODE_MASTERCONTROLLED,
> > + TLV493D_OP_MODE_MAX,
> > +};
> > +
> > +struct tlv493d_mode {
> > + u8 m;
> > + u32 sleep_us;
> > +};
> > +
> > +struct tlv493d_data {
> > + struct device *dev;
> > + struct i2c_client *client;
> > + struct mutex lock;
>
> Lock needs a comment describing what it is supposed to protect.
>
Ack.
> > + struct regmap *map;
> > + u8 mode;
> > + u8 wr_regs[TLV493D_WR_REG_MAX];
> > + s32 temp_offset;
> > +};
> > +
> > +/*
> > + * Different mode has different measurement cycle time, this time is
> > + * used in deriving the sleep and timemout while reading the data from
> > + * sensor in polling.
> > + * Power-down mode: No measurement.
> > + * Fast mode: Freq:3.3 KHz. Measurement time:305 usec.
> > + * Low-power mode: Freq:100 Hz. Measurement time:10 msec.
> > + * Ultra low-power mode: Freq:10 Hz. Measurement time:100 msec.
> > + * Master controlled mode: Freq:3.3 Khz. Measurement time:305 usec.
> > + */
> > +static struct tlv493d_mode modes[TLV493D_OP_MODE_MAX] = {
>
> Let's add the tlv493d_ prefix to modes.
>
Will rename is to tlv493d_modes_info.
> > + {.m = TLV493D_OP_MODE_POWERDOWN, .sleep_us = 0 },
> > + {.m = TLV493D_OP_MODE_FAST, .sleep_us = 305 },
> > + {.m = TLV493D_OP_MODE_LOWPOWER, .sleep_us = 10 * USEC_PER_MSEC },
> > + {.m = TLV493D_OP_MODE_ULTRA_LOWPOWER, .sleep_us = 100 * USEC_PER_MSEC },
> > + {.m = TLV493D_OP_MODE_MASTERCONTROLLED, .sleep_us = 305 },
> > +};
> > +
> > +/*
> > + * The datasheet mentions the sensor supports only direct byte-stream write starting from
> > + * register address 0x0. So for any modification to be made to any write registers, it must
> > + * be written starting from the register address 0x0.
> > + * I2C write operation should not contain register address in the I2C frame, it should
> > + * contains only raw byte stream for the write registers. As below,
> > + * I2C Frame: |S|SlaveAddr Wr|Ack|Byte[0]|Ack|Byte[1]|Ack|.....|Sp|
> > + */
> > +static int tlv493d_write_all_regs(struct tlv493d_data *data)
> > +{
> > + int ret;
> > +
> > + if (!data || !data->client)
> > + return -EINVAL;
> > +
> > + /*
> > + * As regmap does not provide raw write API which perform I2C write without
> > + * specifying register address, direct i2c_master_send() API is used.
> > + */
> > + ret = i2c_master_send(data->client, data->wr_regs, ARRAY_SIZE(data->wr_regs));
> > + if (ret < 0) {
> > + dev_err(data->dev, "failed to write registers. error %d\n", ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int tlv493d_set_operating_mode(struct tlv493d_data *data, u8 mode)
> > +{
> > + if (!data)
> > + return -EINVAL;
> > +
> > + u8 *reg_mode1 = &data->wr_regs[TLV493D_WR_REG_MODE1];
> > + u8 *reg_mode2 = &data->wr_regs[TLV493D_WR_REG_MODE2];
> > +
> > + switch (mode) {
> > + case TLV493D_OP_MODE_POWERDOWN:
> > + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> > + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> > + break;
> > +
> > + case TLV493D_OP_MODE_FAST:
> > + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> > + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> > + break;
> > +
> > + case TLV493D_OP_MODE_LOWPOWER:
> > + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> > + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> > + SET_BIT(*reg_mode2, TLV493D_MODE2_LP_PERIOD);
> > + break;
> > +
> > + case TLV493D_OP_MODE_ULTRA_LOWPOWER:
> > + CLR_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> > + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> > + CLR_BIT(*reg_mode2, TLV493D_MODE2_LP_PERIOD);
> > + break;
> > +
> > + case TLV493D_OP_MODE_MASTERCONTROLLED:
> > + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_FAST);
> > + SET_BIT(*reg_mode1, TLV493D_MODE1_MOD_LOW);
> > + break;
> > +
> > + default:
> > + dev_err(data->dev, "invalid mode configuration\n");
> > + return -EINVAL;
> > + }
> > +
> > + return tlv493d_write_all_regs(data);
> > +}
> > +
> > +static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
> > + s16 *z, s16 *t)
> > +{
> > + u8 buff[7] = {0};
> > + int err, ret;
> > + struct tlv493d_mode *mode;
> > +
> > + if (!data)
> > + return -EINVAL;
> > +
> > + guard(mutex)(&data->lock);
> > +
> > + ret = pm_runtime_resume_and_get(data->dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + mode = &modes[data->mode];
> > +
> > + /*
> > + * Poll until data is valid,
> > + * For a valid data TLV493D_VAL_CHANNEL bit of TLV493D_RD_REG_TEMP should be set to 0.
> > + * The sampling time depends on the sensor mode. poll 3x the time of the sampling time.
> > + */
> > + ret = read_poll_timeout(regmap_bulk_read, err, err ||
>
> Why not regmap_read_poll_timeout()?
>
We want to read all 7 register in single operation to make sure that we
read it from the same measurement cycle. out of those 7 registers
TLV493d_RD_REG_TEMP has a bit field TLV493D_VAL_CHANNEL which stats if
the measurement cycle was complete or not. Per my understanding
regmap_read_poll_timeout() does not support bulk/multi register read so,
read_poll_timeout is used with regmap_bulk_read().
> > + FIELD_GET(TLV493D_VAL_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> > + mode->sleep_us, (3 * mode->sleep_us), false, data->map, TLV493D_RD_REG_BX,
> > + buff, ARRAY_SIZE(buff));
> > + if (ret) {
> > + dev_err(data->dev, "read poll timeout, error:%d", ret);
> > + goto out;
> > + }
> > + if (err) {
> > + dev_err(data->dev, "read data failed, error:%d\n", ret);
> > + ret = err;
> > + goto out;
> > + }
> > +
> > + *x = TLV493D_DATA_X_GET(buff);
> > + *y = TLV493D_DATA_Y_GET(buff);
> > + *z = TLV493D_DATA_Z_GET(buff);
> > + *t = TLV493D_DATA_TEMP_GET(buff);
> > +
> > +out:
> > + pm_runtime_mark_last_busy(data->dev);
> > + pm_runtime_put_autosuspend(data->dev);
> > + return ret;
> > +}
> > +
> > +static int tlv493d_init(struct tlv493d_data *data)
> > +{
> > + int ret;
> > + u8 buff[TLV493D_RD_REG_MAX];
> > +
> > + if (!data)
> > + return -EINVAL;
> > +
> > + /*
> > + * The sensor initialization requires below steps to be followed,
> > + * 1. Power-up sensor.
> > + * 2. Read and store read-registers map (0x0-0x9).
> > + * 3. Copy values from read reserved registers to write reserved fields (0x0-0x3).
> > + * 4. Set operating mode.
> > + * 5. Write to all registers.
> > + */
> > + ret = regmap_bulk_read(data->map, TLV493D_RD_REG_BX, buff, ARRAY_SIZE(buff));
> > + if (ret) {
> > + dev_err(data->dev, "bulk read failed, error %d\n", ret);
> > + return ret;
> > + }
> > +
> > + data->wr_regs[0] = 0; /* Write register 0x0 is reserved. Does not require to be updated.*/
> > + data->wr_regs[1] = buff[TLV493D_RD_REG_RES1] & TLV493D_RD_REG_RES1_WR_MASK;
> > + data->wr_regs[2] = buff[TLV493D_RD_REG_RES2] & TLV493D_RD_REG_RES2_WR_MASK;
> > + data->wr_regs[3] = buff[TLV493D_RD_REG_RES3] & TLV493D_RD_REG_RES3_WR_MASK;
> > +
> > + ret = tlv493d_set_operating_mode(data, data->mode);
> > + if (ret < 0) {
> > + dev_err(data->dev, "failed to set operating mode\n");
> > + return ret;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int tlv493d_parse_dt(struct tlv493d_data *data)
> > +{
> > + struct device_node *node;
> > + u32 val = 0;
> > + int ret;
> > +
> > + if (!data)
> > + return -EINVAL;
> > +
> > + node = data->dev->of_node;
> > +
> > + /* Optional 'mode' property to set sensor operation mode */
> > + ret = of_property_read_u32(node, "mode", &val);
> > + if (ret < 0 || val >= TLV493D_OP_MODE_MAX) {
> > + /* Fallback to default mode if property is missing or invalid */
> > + data->mode = TLV493D_OP_MODE_MASTERCONTROLLED;
> > + } else {
> > + data->mode = val;
> > + }
>
> As mentioned in the dt-bindings review, we already control this paritially
> with the power management runtime, so having devicetree specify a power mode
> doesn't really make sense.
>
> In fact, since the mode determines the sample rate, if we ever implemented
> handling the interrupt pin, it would make sense for the sampling_freqency
> to control mode rather than hard-coding it.
>
Agreed, will drop the mode and temp-offset setting in DT.
> > +
> > + /*
> > + * Read temperature offset (raw value at 25°C). If not specified,
> > + * default to 340.
> > + */
> > + ret = of_property_read_u32(node, "temp-offset", &val);
> > + if (ret)
> > + val = 340;
>
> As mentioned in the dt-bindings review, this sounds like a calibbias rather
> than something that should be hard-coded in a devicetree.
>
Agreed, will drop it.
> > + /*
> > + * The above is a raw offset; however, IIO expects a single effective offset.
> > + * Since final temperature includes an additional fixed 25°C (i.e. 25000 m°C),
> > + * we compute a combined offset using scale = 1100 (1.1 * 1000).
> > + */
> > + data->temp_offset = -val + (s32)div_u64(25000, 1100);
> > +
> > + return 0;
> > +}
> > +
> > +static int tlv493d_read_raw(struct iio_dev *indio_dev, const struct iio_chan_spec *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + struct tlv493d_data *data = iio_priv(indio_dev);
> > + s16 x, y, z, t;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
>
> Processed is not the same as raw. Just drop it.
>
Ack.
> > + case IIO_CHAN_INFO_RAW:
> > + ret = tlv493d_get_measurements(data, &x, &y, &z, &t);
> > + if (ret) {
> > + dev_err(data->dev, "failed to read sensor data\n");
>
> The error gets returned to usespace, so we don't need to log errors here.
>
Ack.
> > + return ret;
> > + }
> > + /* Return raw values for requested channel */
> > + switch (chan->address) {
> > + case AXIS_X:
> > + *val = x;
> > + return IIO_VAL_INT;
> > + case AXIS_Y:
> > + *val = y;
> > + return IIO_VAL_INT;
> > + case AXIS_Z:
> > + *val = z;
> > + return IIO_VAL_INT;
> > + case TEMPERATURE:
> > + *val = t;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_SCALE:
> > + switch (chan->type) {
> > + case IIO_MAGN:
> > + /*
> > + * Magnetic field scale: 0.0098 mTesla (i.e. 9.8 µT)
> > + * Expressed as fractional: 98/10 = 9.8 µT.
> > + */
> > + *val = 98;
> > + *val2 = 10;
>
> We use SI units, so this needs to be gauss, not tesela.
>
Sure, Is there any documentation/reference this details are mentioned?
> > + return IIO_VAL_FRACTIONAL;
> > + case IIO_TEMP:
> > + /*
> > + * Temperature scale: 1.1 °C per LSB, expressed as 1100 m°C
> > + * Returned as integer for IIO core to apply:
> > + * temp = (raw + offset) * scale
> > + */
> > + *val = 1.1 * MILLI;
>
> I guess this works since it is a constant, but we usually don't
> have floating point in the kernel. I would probably just write this
> as 1100. It doesn't have too many zeros that we can't easily see
> how many there are.
>
Ack. Will make it 1100.
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_OFFSET:
> > + switch (chan->type) {
> > + case IIO_TEMP:
> > + /*
> > + * Temperature offset includes sensor-specific raw offset
> > + * plus compensation for +25°C bias in formula.
> > + * This value is precomputed during probe/init:
> > + * offset = -raw_offset + (25000 / scale)
> > + */
> > + *val = data->temp_offset;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
>
> There is no break; so this is unreachable and can be removed.
>
Ack.
> > +}
> > +
> > +static irqreturn_t tlv493d_trigger_handler(int irq, void *ptr)
> > +{
> > + struct iio_poll_func *pf = ptr;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct tlv493d_data *data = iio_priv(indio_dev);
> > +
> > + struct {
> > + s16 channels[3];
> > + s16 temperature;
> > + aligned_s64 timestamp;
> > + } scan;
>
> } scan = { };
>
> Technically not needed here since we assign all values and there shuold
> not be any holes in the struct, but good to have for when others copy this
> and modify it in a new driver.
>
Okay.
> > +
> > + s16 x, y, z, t;
> > + int ret;
> > +
> > + ret = tlv493d_get_measurements(data, &x, &y, &z, &t);
> > + if (ret) {
> > + dev_err(data->dev, "failed to read sensor data\n");
> > + goto trig_out;
> > + }
> > +
> > + scan.channels[0] = x;
> > + scan.channels[1] = y;
> > + scan.channels[2] = z;
> > + scan.temperature = t;
>
> Why not just pass these directly to tlv493d_get_measurements() and avoid this
> assigment and extra local variables?
>
Bcoz the same measurement function is used from raw_read where the
individiual channels values are needed. hence kept as independed args
based for each channel.
> > +
> > + iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan), pf->timestamp);
> > +
> > +trig_out:
> > + iio_trigger_notify_done(indio_dev->trig);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +#define TLV493D_AXIS_CHANNEL(axis, index) \
> > + { \
> > + .type = IIO_MAGN, \
> > + .modified = 1, \
> > + .channel2 = IIO_MOD_##axis, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_SCALE), \
> > + .address = index, \
>
> Setting address the same as scan_index is redundant. We can just
> use scan_index everywhere.
>
we can, I hope this does not break anything on userspace iiolib's side.
> > + .scan_index = index, \
> > + .scan_type = { \
> > + .sign = 's', \
> > + .realbits = 12, \
> > + .storagebits = 16, \
> > + .endianness = IIO_CPU, \
> > + }, \
> > + }
> > +
> > +static const struct iio_chan_spec tlv493d_channels[] = {
> > + TLV493D_AXIS_CHANNEL(X, AXIS_X),
> > + TLV493D_AXIS_CHANNEL(Y, AXIS_Y),
> > + TLV493D_AXIS_CHANNEL(Z, AXIS_Z),
> > + {
> > + .type = IIO_TEMP,
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_OFFSET),
> > + .address = TEMPERATURE,
> > + .scan_index = TEMPERATURE,
> > + .scan_type = {
> > + .sign = 's',
> > + .realbits = 12,
> > + .storagebits = 16,
> > + .endianness = IIO_CPU,
> > + },
> > + },
> > + IIO_CHAN_SOFT_TIMESTAMP(4),
> > +};
> > +
> > +static const struct regmap_range tlv493d_volatile_reg_ranges[] = {
> > + regmap_reg_range(TLV493D_RD_REG_BX, TLV493D_RD_REG_RES3),
> > +};
> > +
> > +static const struct regmap_access_table tlv493d_volatile_regs = {
> > + .yes_ranges = tlv493d_volatile_reg_ranges,
> > + .n_yes_ranges = ARRAY_SIZE(tlv493d_volatile_reg_ranges),
> > +};
>
> Would make sense to have these closer to tlv493d_regmap_config.
>
Ack.
> > +
> > +static const struct iio_info tlv493d_info = {
> > + .read_raw = tlv493d_read_raw,
> > +};
> > +
> > +static const struct iio_buffer_setup_ops tlv493d_setup_ops = {};
>
> Just pass NULL if there are no ops.
>
Ack.
> > +
> > +static const unsigned long tlv493d_scan_masks[] = { GENMASK(3, 0), 0 };
> > +
> > +static const struct regmap_config tlv493d_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = TLV493D_RD_REG_RES3,
> > + .volatile_table = &tlv493d_volatile_regs,
> > +};
> > +
> > +static int tlv493d_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct iio_dev *indio_dev;
> > + struct tlv493d_data *data;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + data = iio_priv(indio_dev);
> > + data->dev = dev;
> > + data->client = client;
> > + i2c_set_clientdata(client, indio_dev);
>
> There is not i2c_get_clientdata(), so I don't think we need this.
>
Correct, will remove it.
> > +
> > + ret = devm_mutex_init(dev, &data->lock);
> > + if (ret)
> > + return ret;
> > +
> > + data->map = devm_regmap_init_i2c(client, &tlv493d_regmap_config);
> > + if (IS_ERR(data->map))
> > + return dev_err_probe(dev, PTR_ERR(data->map), "failed to allocate register map\n");
> > +
> > + ret = devm_regulator_get_enable(dev, "vdd");
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to enable regulator\n");
> > +
> > + ret = tlv493d_parse_dt(data);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to parse device-tree\n");
> > +
> > + ret = tlv493d_init(data);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to initialized\n");
> > +
> > + indio_dev->info = &tlv493d_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->name = client->name;
> > + indio_dev->channels = tlv493d_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(tlv493d_channels);
> > + indio_dev->available_scan_masks = tlv493d_scan_masks;
> > +
> > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> > + iio_pollfunc_store_time, tlv493d_trigger_handler,
> > + &tlv493d_setup_ops);
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret, "iio triggered buffer setup failed\n");
> > +
> > + ret = pm_runtime_set_active(dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = devm_pm_runtime_enable(dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + pm_runtime_get_noresume(dev);
> > + pm_runtime_set_autosuspend_delay(dev, 500);
> > + pm_runtime_use_autosuspend(dev);
> > +
> > + pm_runtime_mark_last_busy(dev);
> > + pm_runtime_put_autosuspend(dev);
> > +
> > + ret = devm_iio_device_register(dev, indio_dev);
>
> random extra space
>
Ack.
> > + if (ret)
> > + return dev_err_probe(dev, ret, "iio device register failed\n");
> > +
> > + return 0;
> > +}
> > +
> > +static int tlv493d_runtime_suspend(struct device *dev)
> > +{
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct tlv493d_data *data = iio_priv(indio_dev);
> > +
> > + return tlv493d_set_operating_mode(data, TLV493D_OP_MODE_POWERDOWN);
> > +}
> > +
> > +static int tlv493d_runtime_resume(struct device *dev)
> > +{
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct tlv493d_data *data = iio_priv(indio_dev);
> > +
> > + return tlv493d_set_operating_mode(data, data->mode);
> > +}
> > +
> > +static DEFINE_RUNTIME_DEV_PM_OPS(tlv493d_pm_ops,
> > + tlv493d_runtime_suspend, tlv493d_runtime_resume, NULL);
> > +
> > +static const struct i2c_device_id tlv493d_id[] = {
> > + { "tlv493d" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, tlv493d_id);
> > +
> > +static const struct of_device_id tlv493d_of_match[] = {
> > + { .compatible = "infineon,tlv493d-a1b6", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, tlv493d_of_match);
> > +
> > +static struct i2c_driver tlv493d_driver = {
> > + .driver = {
> > + .name = "tlv493d",
> > + .of_match_table = tlv493d_of_match,
> > + .pm = pm_ptr(&tlv493d_pm_ops),
> > + },
> > + .probe = tlv493d_probe,
> > + .id_table = tlv493d_id,
> > +};
> > +
> > +module_i2c_driver(tlv493d_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Infineon TLV493D Low-Power 3D Magnetic Sensor");
> > +MODULE_AUTHOR("Dixit Parmar <dixitparmar19@gmail.com>");
> >
>
Thank you for the review,
Dixit
On 7/28/25 10:26 PM, Dixit Parmar wrote:
...
>>> + case IIO_CHAN_INFO_SCALE:
>>> + switch (chan->type) {
>>> + case IIO_MAGN:
>>> + /*
>>> + * Magnetic field scale: 0.0098 mTesla (i.e. 9.8 µT)
>>> + * Expressed as fractional: 98/10 = 9.8 µT.
>>> + */
>>> + *val = 98;
>>> + *val2 = 10;
>> We use SI units, so this needs to be gauss, not tesela.
>>
> Sure, Is there any documentation/reference this details are mentioned?
>>> + return IIO_VAL_FRACTIONAL;
Most of the sysfs attribute documentation is in
Documentation/ABI/testing/sysfs-bus-iio. Specifically for this
case, it says:
What: /sys/bus/iio/devices/iio:deviceX/in_magn_x_raw
What: /sys/bus/iio/devices/iio:deviceX/in_magn_y_raw
What: /sys/bus/iio/devices/iio:deviceX/in_magn_z_raw
KernelVersion: 2.6.35
Contact: linux-iio@vger.kernel.org
Description:
Magnetic field along axis x, y or z (may be arbitrarily
assigned). Data converted by application of offset
then scale to Gauss.
On Tue, 29 Jul 2025 08:56:00 +0530
Dixit Parmar <dixitparmar19@gmail.com> wrote:
> On Sat, Jul 26, 2025 at 03:44:03PM -0500, David Lechner wrote:
> > On 7/26/25 4:37 AM, Dixit Parmar wrote:
> > > The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
> > > applications includes joysticks, control elements (white goods,
> > > multifunction knops), or electric meters (anti tampering) and any
> > > other application that requires accurate angular measurements at
> > > low power consumptions.
> > >
> > > The Sensor is configured over I2C, and as part of Sensor measurement
> > > data it provides 3-Axis magnetic fields and temperature core measurement.
> > >
> > > The driver supports raw value read and buffered input via external trigger
> > > to allow streaming values with the same sensing timestamp.
> > >
> > > The device can be configured in to different operating modes by optional
> > > device-tree "mode" property. Also, the temperature sensing part requires
> > > raw offset captured at 25°C and that can be specified by "temp-offset"
> > > optional device-tree property.
> > >
> > > While sensor has interrupt pin multiplexed with I2C SCL pin. But for bus
> > > configurations interrupt(INT) is not recommended, unless timing constraints
> > > between I2C data transfers and interrupt pulses are monitored and aligned.
> > >
> > > The Sensor's I2C register map and mode information is described in product
> > > User Manual[1].
> > >
> > > Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> > > [1] https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf
> > >
> > > Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
Picking out one question for a quick answer...
> > > + return ret;
> > > + }
> > > + /* Return raw values for requested channel */
> > > + switch (chan->address) {
> > > + case AXIS_X:
> > > + *val = x;
> > > + return IIO_VAL_INT;
> > > + case AXIS_Y:
> > > + *val = y;
> > > + return IIO_VAL_INT;
> > > + case AXIS_Z:
> > > + *val = z;
> > > + return IIO_VAL_INT;
> > > + case TEMPERATURE:
> > > + *val = t;
> > > + return IIO_VAL_INT;
> > > + default:
> > > + return -EINVAL;
> > > + }
> > > + case IIO_CHAN_INFO_SCALE:
> > > + switch (chan->type) {
> > > + case IIO_MAGN:
> > > + /*
> > > + * Magnetic field scale: 0.0098 mTesla (i.e. 9.8 µT)
> > > + * Expressed as fractional: 98/10 = 9.8 µT.
> > > + */
> > > + *val = 98;
> > > + *val2 = 10;
> >
> > We use SI units, so this needs to be gauss, not tesela.
> >
> Sure, Is there any documentation/reference this details are mentioned?
Documentation/ABI/testing/sysfs-bus-iio
https://elixir.bootlin.com/linux/v6.16/source/Documentation/ABI/testing/sysfs-bus-iio#L342
is the specific entry for magnetic fields.
Otherwise a small process thing - where you are agreeing with review
feedback, no need to put it in your reply. Much better to just fix
it and have it in the change log for the next version. That will let
you crop away much more of the thread, so we can focus in on questions.
Jonathan
On Tue, Jul 29, 2025 at 5:26 AM Dixit Parmar <dixitparmar19@gmail.com> wrote: > On Sat, Jul 26, 2025 at 03:44:03PM -0500, David Lechner wrote: > > On 7/26/25 4:37 AM, Dixit Parmar wrote: ... > > > config SI7210 > > > To compile this driver as a module, choose M here: the module > > > will be called si7210. > > > > > > +config TLV493D > > > + tristate "Infineon TLV493D Low-Power 3D Magnetic Sensor" > > > + depends on I2C > > > + select REGMAP_I2C > > > + select IIO_BUFFER > > > + select IIO_TRIGGERED_BUFFER > > > + help > > > + Say Y here to add support for the Infineon TLV493D-A1B6 Low- > > > + Power 3D Megnetic Sensor. > > > + > > > + This driver can also be compiled as a module. > > > + To compile this driver as a module, choose M here: the module > > > + will be called tlv493d. > > > + > > > config TI_TMAG5273 > > > tristate "TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor" > > > depends on I2C ... > > > @@ -35,4 +35,6 @@ obj-$(CONFIG_SI7210) += si7210.o > > > > > > obj-$(CONFIG_TI_TMAG5273) += tmag5273.o > > > > > > +obj-$(CONFIG_TLV493D) += tlv493d.o > > > > We try to keep these in alphabetical order. > > > Ofcourse, I considered TI_TMAG5273 as whole. Will move it above that. David, Jonathan, I remember I have asked Jonathan once already about these cases and unfortunately I forgot what was the conclusion about this. The filename has no vendor prefix, and I think we prefer the order done by filename. > > > obj-$(CONFIG_YAMAHA_YAS530) += yamaha-yas530.o -- With Best Regards, Andy Shevchenko
On Sat, 26 Jul 2025 15:44:03 -0500
David Lechner <dlechner@baylibre.com> wrote:
> On 7/26/25 4:37 AM, Dixit Parmar wrote:
> > The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
> > applications includes joysticks, control elements (white goods,
> > multifunction knops), or electric meters (anti tampering) and any
> > other application that requires accurate angular measurements at
> > low power consumptions.
> >
> > The Sensor is configured over I2C, and as part of Sensor measurement
> > data it provides 3-Axis magnetic fields and temperature core measurement.
> >
> > The driver supports raw value read and buffered input via external trigger
> > to allow streaming values with the same sensing timestamp.
> >
> > The device can be configured in to different operating modes by optional
> > device-tree "mode" property. Also, the temperature sensing part requires
> > raw offset captured at 25°C and that can be specified by "temp-offset"
> > optional device-tree property.
> >
> > While sensor has interrupt pin multiplexed with I2C SCL pin. But for bus
> > configurations interrupt(INT) is not recommended, unless timing constraints
> > between I2C data transfers and interrupt pulses are monitored and aligned.
> >
> > The Sensor's I2C register map and mode information is described in product
> > User Manual[1].
> >
> > Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> > [1] https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf
> >
> > Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> > +static const struct regmap_config tlv493d_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = TLV493D_RD_REG_RES3,
> > + .volatile_table = &tlv493d_volatile_regs,
> > +};
> > +
> > +static int tlv493d_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct iio_dev *indio_dev;
> > + struct tlv493d_data *data;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + data = iio_priv(indio_dev);
> > + data->dev = dev;
> > + data->client = client;
> > + i2c_set_clientdata(client, indio_dev);
>
> There is not i2c_get_clientdata(), so I don't think we need this.
There's a dev_get_drvdata() that pairs with this in suspend and resume.
I kind of wish the bus specific accessor would go away but there
is too much history behind them :(
> > +static int tlv493d_runtime_suspend(struct device *dev)
> > +{
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct tlv493d_data *data = iio_priv(indio_dev);
> > +
> > + return tlv493d_set_operating_mode(data, TLV493D_OP_MODE_POWERDOWN);
> > +}
> > +
> > +static int tlv493d_runtime_resume(struct device *dev)
> > +{
> > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > + struct tlv493d_data *data = iio_priv(indio_dev);
> > +
> > + return tlv493d_set_operating_mode(data, data->mode);
> > +}
> > +
> > +static DEFINE_RUNTIME_DEV_PM_OPS(tlv493d_pm_ops,
> > + tlv493d_runtime_suspend, tlv493d_runtime_resume, NULL);
> > +
> > +static const struct i2c_device_id tlv493d_id[] = {
> > + { "tlv493d" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, tlv493d_id);
> > +
> > +static const struct of_device_id tlv493d_of_match[] = {
> > + { .compatible = "infineon,tlv493d-a1b6", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, tlv493d_of_match);
> > +
> > +static struct i2c_driver tlv493d_driver = {
> > + .driver = {
> > + .name = "tlv493d",
> > + .of_match_table = tlv493d_of_match,
> > + .pm = pm_ptr(&tlv493d_pm_ops),
> > + },
> > + .probe = tlv493d_probe,
> > + .id_table = tlv493d_id,
> > +};
> > +
> > +module_i2c_driver(tlv493d_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Infineon TLV493D Low-Power 3D Magnetic Sensor");
> > +MODULE_AUTHOR("Dixit Parmar <dixitparmar19@gmail.com>");
> >
>
On Sun, Jul 27, 2025 at 01:38:52PM +0100, Jonathan Cameron wrote:
> On Sat, 26 Jul 2025 15:44:03 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
> > On 7/26/25 4:37 AM, Dixit Parmar wrote:
> > > The Infineon TLV493D is a Low-Power 3D Magnetic Sensor. The Sensor
> > > applications includes joysticks, control elements (white goods,
> > > multifunction knops), or electric meters (anti tampering) and any
> > > other application that requires accurate angular measurements at
> > > low power consumptions.
> > >
> > > The Sensor is configured over I2C, and as part of Sensor measurement
> > > data it provides 3-Axis magnetic fields and temperature core measurement.
> > >
> > > The driver supports raw value read and buffered input via external trigger
> > > to allow streaming values with the same sensing timestamp.
> > >
> > > The device can be configured in to different operating modes by optional
> > > device-tree "mode" property. Also, the temperature sensing part requires
> > > raw offset captured at 25°C and that can be specified by "temp-offset"
> > > optional device-tree property.
> > >
> > > While sensor has interrupt pin multiplexed with I2C SCL pin. But for bus
> > > configurations interrupt(INT) is not recommended, unless timing constraints
> > > between I2C data transfers and interrupt pulses are monitored and aligned.
> > >
> > > The Sensor's I2C register map and mode information is described in product
> > > User Manual[1].
> > >
> > > Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> > > [1] https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf
> > >
> > > Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
>
> > > +static const struct regmap_config tlv493d_regmap_config = {
> > > + .reg_bits = 8,
> > > + .val_bits = 8,
> > > + .max_register = TLV493D_RD_REG_RES3,
> > > + .volatile_table = &tlv493d_volatile_regs,
> > > +};
> > > +
> > > +static int tlv493d_probe(struct i2c_client *client)
> > > +{
> > > + struct device *dev = &client->dev;
> > > + struct iio_dev *indio_dev;
> > > + struct tlv493d_data *data;
> > > + int ret;
> > > +
> > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > > + if (!indio_dev)
> > > + return -ENOMEM;
> > > +
> > > + data = iio_priv(indio_dev);
> > > + data->dev = dev;
> > > + data->client = client;
> > > + i2c_set_clientdata(client, indio_dev);
> >
> > There is not i2c_get_clientdata(), so I don't think we need this.
> There's a dev_get_drvdata() that pairs with this in suspend and resume.
>
> I kind of wish the bus specific accessor would go away but there
> is too much history behind them :(
>
Thank you for the confirmation. Will keep it.
> > > +static int tlv493d_runtime_suspend(struct device *dev)
> > > +{
> > > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > > + struct tlv493d_data *data = iio_priv(indio_dev);
> > > +
> > > + return tlv493d_set_operating_mode(data, TLV493D_OP_MODE_POWERDOWN);
> > > +}
> > > +
> > > +static int tlv493d_runtime_resume(struct device *dev)
> > > +{
> > > + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > > + struct tlv493d_data *data = iio_priv(indio_dev);
> > > +
> > > + return tlv493d_set_operating_mode(data, data->mode);
> > > +}
> > > +
> > > +static DEFINE_RUNTIME_DEV_PM_OPS(tlv493d_pm_ops,
> > > + tlv493d_runtime_suspend, tlv493d_runtime_resume, NULL);
> > > +
> > > +static const struct i2c_device_id tlv493d_id[] = {
> > > + { "tlv493d" },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, tlv493d_id);
> > > +
> > > +static const struct of_device_id tlv493d_of_match[] = {
> > > + { .compatible = "infineon,tlv493d-a1b6", },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, tlv493d_of_match);
> > > +
> > > +static struct i2c_driver tlv493d_driver = {
> > > + .driver = {
> > > + .name = "tlv493d",
> > > + .of_match_table = tlv493d_of_match,
> > > + .pm = pm_ptr(&tlv493d_pm_ops),
> > > + },
> > > + .probe = tlv493d_probe,
> > > + .id_table = tlv493d_id,
> > > +};
> > > +
> > > +module_i2c_driver(tlv493d_driver);
> > > +
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_DESCRIPTION("Infineon TLV493D Low-Power 3D Magnetic Sensor");
> > > +MODULE_AUTHOR("Dixit Parmar <dixitparmar19@gmail.com>");
> > >
> >
>
© 2016 - 2025 Red Hat, Inc.