[PATCH 0/2] iio: magnetometer: add support for Infineon TLV493D 3D Magnetic Sensor

[PATCH 0/2] iio: magnetometer: add support for Infineon TLV493D 3D Magnetic Sensor

[Dixit Parmar]

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>
---
Dixit Parmar (2):
      iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor
      dt-bindings: iio: magnetometer: document Infineon TLV493D 3D Magnetic sensor

 .../iio/magnetometer/infineon,tlv493d.yaml         |  57 ++
 drivers/iio/magnetometer/Kconfig                   |  14 +
 drivers/iio/magnetometer/Makefile                  |   2 +
 drivers/iio/magnetometer/tlv493d.c                 | 606 +++++++++++++++++++++
 4 files changed, 679 insertions(+)
---
base-commit: d7b8f8e20813f0179d8ef519541a3527e7661d3a
change-id: 20250726-tlv493d-sensor-v6_16-rc5-18c712093b27

Best regards,
-- 
Dixit Parmar <dixitparmar19@gmail.com>

[PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Dixit Parmar]

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

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[David Lechner]

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>");
> 

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Jonathan Cameron]

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>");
> >   
> 

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Dixit Parmar]

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>");
> > >   
> > 
> 

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Dixit Parmar]

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

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Andy Shevchenko]

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

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Jonathan Cameron]

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

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[David Lechner]

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.

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Christophe JAILLET]

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

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Dixit Parmar]

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

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Jonathan Cameron]

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>");
> 

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Dixit Parmar]

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

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Jonathan Cameron]

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

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Dixit Parmar]

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
> 

Re: [PATCH 1/2] iio: magnetometer: add support for Infineon TLV493D 3D Magentic sensor

[Jonathan Cameron]

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

[PATCH 2/2] dt-bindings: iio: magnetometer: document Infineon TLV493D 3D Magnetic sensor

[Dixit Parmar]

Document the bindings for Infineon TLV493D Low-Power 3D Magnetic Sensor
controlled by I2C interface. Main applications includes joysticks, control
elements (white goods, multifunction knops), or electric meters (anti
tampering).

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.

Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf

Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
---
 .../iio/magnetometer/infineon,tlv493d.yaml         | 57 ++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml b/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml
new file mode 100644
index 000000000000..0442cf41503b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/magnetometer/infineon,tlv493d.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Infineon Technologies TLV493D Low-Power 3D Magnetic Sensor
+
+maintainers:
+  - Dixit Parmar <dixitparmar19@gmail.com>
+
+properties:
+  $nodename:
+    pattern: '^magnetometer@[0-9a-f]+$'
+
+  compatible:
+    const: infineon,tlv493d-a1b6
+
+  reg:
+    maxItems: 1
+
+  vdd-supply:
+    description: 2.8V to 3.5V supply
+
+  mode:
+    description: Sensor operating mode. Must be one of the defined enum values.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum:
+      - 0 # Power Down Mode. No measurement.
+      - 1 # Fast Mode
+      - 2 # Low-Power Mode
+      - 3 # Ultra Low-Power Mode
+      - 4 # Master Controlled Mode
+    default: 4
+
+  temp-offset:
+    description: Raw temperature offset at 25°C to apply before applying scale and correction.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 340
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+example:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      magnetometer@5e {
+        compatible = "infineon,tlv493d-a1b6";
+        reg = <0x5e>;
+        vdd = <&hall_vcc>;
+      };
+    };

-- 
2.43.0

Re: [PATCH 2/2] dt-bindings: iio: magnetometer: document Infineon TLV493D 3D Magnetic sensor

[David Lechner]

On 7/26/25 4:37 AM, Dixit Parmar wrote:
> Document the bindings for Infineon TLV493D Low-Power 3D Magnetic Sensor
> controlled by I2C interface. Main applications includes joysticks, control
> elements (white goods, multifunction knops), or electric meters (anti
> tampering).
> 
> 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.
> 
> Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> 
> Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> ---
>  .../iio/magnetometer/infineon,tlv493d.yaml         | 57 ++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml b/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml
> new file mode 100644
> index 000000000000..0442cf41503b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/magnetometer/infineon,tlv493d.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Infineon Technologies TLV493D Low-Power 3D Magnetic Sensor
> +
> +maintainers:
> +  - Dixit Parmar <dixitparmar19@gmail.com>
> +
> +properties:
> +  $nodename:
> +    pattern: '^magnetometer@[0-9a-f]+$'
> +
> +  compatible:
> +    const: infineon,tlv493d-a1b6
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: 2.8V to 3.5V supply

The SDA pin can also be a /INT signal, so we need to have an
optional interrupts property as well.

> +
> +  mode:
> +    description: Sensor operating mode. Must be one of the defined enum values.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum:
> +      - 0 # Power Down Mode. No measurement.
> +      - 1 # Fast Mode
> +      - 2 # Low-Power Mode
> +      - 3 # Ultra Low-Power Mode
> +      - 4 # Master Controlled Mode
> +    default: 4

This is not the sort of thing that really belongs in a devicetree.
We should be describing here how the chip is wired up, and only
control how it works based on that.

If there are any wiring conditions that could affect this setting,
they could go here. For example, if the power supply doesn't have
enough current, then we can only operate in one of the low power
modes. Otherwise generally we just stick to the best performing
mode. And specifying the power down mode here really doesn't make
sense - you could never use the sensor!

> +
> +  temp-offset:
> +    description: Raw temperature offset at 25°C to apply before applying scale and correction.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 340

This is another one that likely doesn't belong in the devicetree.
There is a standard *_calibbias attribute that can be used for
such a calibration if needed.

> +
> +required:
> +  - compatible
> +  - reg

Power supplies are usually required.

> +
> +additionalProperties: false
> +
> +example:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      magnetometer@5e {
> +        compatible = "infineon,tlv493d-a1b6";
> +        reg = <0x5e>;
> +        vdd = <&hall_vcc>;
> +      };
> +    };
> 

Re: [PATCH 2/2] dt-bindings: iio: magnetometer: document Infineon TLV493D 3D Magnetic sensor

[Dixit Parmar]

On Sat, Jul 26, 2025 at 03:43:56PM -0500, David Lechner wrote:
> On 7/26/25 4:37 AM, Dixit Parmar wrote:
> > Document the bindings for Infineon TLV493D Low-Power 3D Magnetic Sensor
> > controlled by I2C interface. Main applications includes joysticks, control
> > elements (white goods, multifunction knops), or electric meters (anti
> > tampering).
> > 
> > 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.
> > 
> > Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> > 
> > Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> > ---
> >  .../iio/magnetometer/infineon,tlv493d.yaml         | 57 ++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml b/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml
> > new file mode 100644
> > index 000000000000..0442cf41503b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/magnetometer/infineon,tlv493d.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Infineon Technologies TLV493D Low-Power 3D Magnetic Sensor
> > +
> > +maintainers:
> > +  - Dixit Parmar <dixitparmar19@gmail.com>
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: '^magnetometer@[0-9a-f]+$'
> > +
> > +  compatible:
> > +    const: infineon,tlv493d-a1b6
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vdd-supply:
> > +    description: 2.8V to 3.5V supply
> 
> The SDA pin can also be a /INT signal, so we need to have an
> optional interrupts property as well.
>
Okay. Will add it.
> > +
> > +  mode:
> > +    description: Sensor operating mode. Must be one of the defined enum values.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum:
> > +      - 0 # Power Down Mode. No measurement.
> > +      - 1 # Fast Mode
> > +      - 2 # Low-Power Mode
> > +      - 3 # Ultra Low-Power Mode
> > +      - 4 # Master Controlled Mode
> > +    default: 4
> 
> This is not the sort of thing that really belongs in a devicetree.
> We should be describing here how the chip is wired up, and only
> control how it works based on that.
> 
> If there are any wiring conditions that could affect this setting,
> they could go here. For example, if the power supply doesn't have
> enough current, then we can only operate in one of the low power
> modes. Otherwise generally we just stick to the best performing
> mode. And specifying the power down mode here really doesn't make
> sense - you could never use the sensor!
> 
Got it. Will remove it.
> > +
> > +  temp-offset:
> > +    description: Raw temperature offset at 25°C to apply before applying scale and correction.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 340
> 
> This is another one that likely doesn't belong in the devicetree.
> There is a standard *_calibbias attribute that can be used for
> such a calibration if needed.
> 
Its factory setting so I thought if there is any deviation from that
than we can handle it like this but as you pointed out, its not the
right way, so will stick to 340 default factory value as per the
datasheet.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> 
> Power supplies are usually required.
> 
Ack.
> > +
> > +additionalProperties: false
> > +
> > +example:
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +      magnetometer@5e {
> > +        compatible = "infineon,tlv493d-a1b6";
> > +        reg = <0x5e>;
> > +        vdd = <&hall_vcc>;
> > +      };
> > +    };
> > 
> 
Thanks for the review,
Dixit

Re: [PATCH 2/2] dt-bindings: iio: magnetometer: document Infineon TLV493D 3D Magnetic sensor

[kernel test robot]

Hi Dixit,

kernel test robot noticed the following build warnings:

[auto build test WARNING on d7b8f8e20813f0179d8ef519541a3527e7661d3a]

url:    https://github.com/intel-lab-lkp/linux/commits/Dixit-Parmar/iio-magnetometer-add-support-for-Infineon-TLV493D-3D-Magentic-sensor/20250726-173919
base:   d7b8f8e20813f0179d8ef519541a3527e7661d3a
patch link:    https://lore.kernel.org/r/20250726-tlv493d-sensor-v6_16-rc5-v1-2-deac027e6f32%40gmail.com
patch subject: [PATCH 2/2] dt-bindings: iio: magnetometer: document Infineon TLV493D 3D Magnetic sensor
config: csky-randconfig-051-20250727 (https://download.01.org/0day-ci/archive/20250727/202507270929.aFInjuAM-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 11.5.0
dtschema version: 2025.6.2.dev4+g8f79ddd
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250727/202507270929.aFInjuAM-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507270929.aFInjuAM-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: Duplicate compatible "infineon,tlv493d-a1b6" found in schemas matching "$id":
   	http://devicetree.org/schemas/trivial-devices.yaml#
   	http://devicetree.org/schemas/iio/magnetometer/infineon,tlv493d.yaml#

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] dt-bindings: iio: magnetometer: document Infineon TLV493D 3D Magnetic sensor

[Krzysztof Kozlowski]

On 26/07/2025 11:37, Dixit Parmar wrote:
> Document the bindings for Infineon TLV493D Low-Power 3D Magnetic Sensor
> controlled by I2C interface. Main applications includes joysticks, control
> elements (white goods, multifunction knops), or electric meters (anti
> tampering).

You are duplicating existing binding, need to remove it as well.

> 
> 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.
> 
> Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> 
> Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> ---
>  .../iio/magnetometer/infineon,tlv493d.yaml         | 57 ++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml b/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml
> new file mode 100644
> index 000000000000..0442cf41503b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/magnetometer/infineon,tlv493d.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Infineon Technologies TLV493D Low-Power 3D Magnetic Sensor
> +
> +maintainers:
> +  - Dixit Parmar <dixitparmar19@gmail.com>
> +
> +properties:
> +  $nodename:
> +    pattern: '^magnetometer@[0-9a-f]+$'
> +
> +  compatible:
> +    const: infineon,tlv493d-a1b6
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: 2.8V to 3.5V supply
> +
> +  mode:
> +    description: Sensor operating mode. Must be one of the defined enum values.

Why is this board level property? I imagine you want to change it runtime.


> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum:
> +      - 0 # Power Down Mode. No measurement.
> +      - 1 # Fast Mode
> +      - 2 # Low-Power Mode
> +      - 3 # Ultra Low-Power Mode
> +      - 4 # Master Controlled Mode
> +    default: 4
> +
> +  temp-offset:
> +    description: Raw temperature offset at 25°C to apply before applying scale and correction.

Does not look wrapped according to Linux coding style.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 340

Missing vendor prefix, missing unit suffix. I am also not sure what is
the board design choice to offset it.



> +
> +required:
> +  - compatible
> +  - reg

Missing supplies.



Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: iio: magnetometer: document Infineon TLV493D 3D Magnetic sensor

[Dixit Parmar]

On Sun, Jul 27, 2025 at 11:23:04AM +0200, Krzysztof Kozlowski wrote:
> On 26/07/2025 11:37, Dixit Parmar wrote:
> > Document the bindings for Infineon TLV493D Low-Power 3D Magnetic Sensor
> > controlled by I2C interface. Main applications includes joysticks, control
> > elements (white goods, multifunction knops), or electric meters (anti
> > tampering).
> 
> You are duplicating existing binding, need to remove it as well.
>
Yeah, looks like some previous stalled entry is there. I will remove it.
> > 
> > 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.
> > 
> > Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> > 
> > Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> > ---
> >  .../iio/magnetometer/infineon,tlv493d.yaml         | 57 ++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml b/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml
> > new file mode 100644
> > index 000000000000..0442cf41503b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/magnetometer/infineon,tlv493d.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Infineon Technologies TLV493D Low-Power 3D Magnetic Sensor
> > +
> > +maintainers:
> > +  - Dixit Parmar <dixitparmar19@gmail.com>
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: '^magnetometer@[0-9a-f]+$'
> > +
> > +  compatible:
> > +    const: infineon,tlv493d-a1b6
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vdd-supply:
> > +    description: 2.8V to 3.5V supply
> > +
> > +  mode:
> > +    description: Sensor operating mode. Must be one of the defined enum values.
> 
> Why is this board level property? I imagine you want to change it runtime.
> 
Acked.
> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    enum:
> > +      - 0 # Power Down Mode. No measurement.
> > +      - 1 # Fast Mode
> > +      - 2 # Low-Power Mode
> > +      - 3 # Ultra Low-Power Mode
> > +      - 4 # Master Controlled Mode
> > +    default: 4
> > +
> > +  temp-offset:
> > +    description: Raw temperature offset at 25°C to apply before applying scale and correction.
> 
> Does not look wrapped according to Linux coding style.
> 
As mentioned in previous thread, the property will get removed.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 340
> 
> Missing vendor prefix, missing unit suffix. I am also not sure what is
> the board design choice to offset it.
> 
> 
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> 
> Missing supplies.
> 
Ack.
> 
> 
> Best regards,
> Krzysztof

Thanks for the review,
Dixit

Re: [PATCH 2/2] dt-bindings: iio: magnetometer: document Infineon TLV493D 3D Magnetic sensor

[Rob Herring (Arm)]

On Sat, 26 Jul 2025 15:07:02 +0530, Dixit Parmar wrote:
> Document the bindings for Infineon TLV493D Low-Power 3D Magnetic Sensor
> controlled by I2C interface. Main applications includes joysticks, control
> elements (white goods, multifunction knops), or electric meters (anti
> tampering).
> 
> 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.
> 
> Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> 
> Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> ---
>  .../iio/magnetometer/infineon,tlv493d.yaml         | 57 ++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Warning: Duplicate compatible "infineon,tlv493d-a1b6" found in schemas matching "$id":
	http://devicetree.org/schemas/iio/magnetometer/infineon,tlv493d.yaml#
	http://devicetree.org/schemas/trivial-devices.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml: 'example' is not one of ['$id', '$schema', 'title', 'description', 'examples', 'required', 'allOf', 'anyOf', 'oneOf', 'definitions', '$defs', 'additionalProperties', 'dependencies', 'dependentRequired', 'dependentSchemas', 'patternProperties', 'properties', 'not', 'if', 'then', 'else', 'unevaluatedProperties', 'deprecated', 'maintainers', 'select', '$ref']
	from schema $id: http://devicetree.org/meta-schemas/base.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250726-tlv493d-sensor-v6_16-rc5-v1-2-deac027e6f32@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 2/2] dt-bindings: iio: magnetometer: document Infineon TLV493D 3D Magnetic sensor

[Dixit Parmar]

On Sun, Jul 27, 2025 at 03:27:39PM -0500, Rob Herring (Arm) wrote:
> 
> On Sat, 26 Jul 2025 15:07:02 +0530, Dixit Parmar wrote:
> > Document the bindings for Infineon TLV493D Low-Power 3D Magnetic Sensor
> > controlled by I2C interface. Main applications includes joysticks, control
> > elements (white goods, multifunction knops), or electric meters (anti
> > tampering).
> > 
> > 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.
> > 
> > Datasheet: https://www.infineon.com/assets/row/public/documents/24/49/infineon-tlv493d-a1b6-datasheet-en.pdf
> > 
> > Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
> > ---
> >  .../iio/magnetometer/infineon,tlv493d.yaml         | 57 ++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
>
Ack.
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Warning: Duplicate compatible "infineon,tlv493d-a1b6" found in schemas matching "$id":
> 	http://devicetree.org/schemas/iio/magnetometer/infineon,tlv493d.yaml#
> 	http://devicetree.org/schemas/trivial-devices.yaml#
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d.yaml: 'example' is not one of ['$id', '$schema', 'title', 'description', 'examples', 'required', 'allOf', 'anyOf', 'oneOf', 'definitions', '$defs', 'additionalProperties', 'dependencies', 'dependentRequired', 'dependentSchemas', 'patternProperties', 'properties', 'not', 'if', 'then', 'else', 'unevaluatedProperties', 'deprecated', 'maintainers', 'select', '$ref']
> 	from schema $id: http://devicetree.org/meta-schemas/base.yaml#
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250726-tlv493d-sensor-v6_16-rc5-v1-2-deac027e6f32@gmail.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>