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

[PATCH v3 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.

While the sensor has an interrupt pin multiplexed with an 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
Link: https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf [1]
Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
---
Changes in v3:
- dt-binding: rename to infineon,tlv493d-a1b6.yaml.
- dt-binding: fix convention errors.
- Switch to using enums for mode and channel where applicable.
- Drop unnecesary input argument checks in functions.
- Switch to u32 array for mode sample rate timing from single member struct.
- Add note describing the I2C communication characteristics at the top.
- Drop tlv493d_setup_ops and use NULL directly.
- Fix typos in comments and commit messages.
- Link to v2: https://lore.kernel.org/r/20250802-tlv493d-sensor-v6_16-rc5-v2-0-e867df86ad93@gmail.com

Changes in v2:
- Drop regmap implementation in favor of using direct i2c APIs to
  have uniform communication APIs across the driver.
- Remove custom device-tree properties as suggested and hardcode
  setting operating mode in probe().
- Derive and hardcode temperature offset from raw offset and compensation.
- Add missing device name(tlv493_) prefix in global variables.
- Change float operation with multiplier to fixed value(1100).
- Change Magnetic field reporting to Guass SI unit.
- User FIELD_PREP instead of direct bitwise ops.
- Convert sensor channel parsing logic from Macro to function for
  better readability.
- Discard unused #define's.
- Discard IIO_CHAN_INFO_PROCESSED.
- Maintain alphabetical order of config options in Makefile and Kconfig.
- Readability fixes.
- Link to v1: https://lore.kernel.org/r/20250726-tlv493d-sensor-v6_16-rc5-v1-0-deac027e6f32@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-a1b6.yaml    |  45 ++
 .../devicetree/bindings/trivial-devices.yaml       |   2 -
 drivers/iio/magnetometer/Kconfig                   |  13 +
 drivers/iio/magnetometer/Makefile                  |   2 +
 drivers/iio/magnetometer/tlv493d.c                 | 524 +++++++++++++++++++++
 5 files changed, 584 insertions(+), 2 deletions(-)
---
base-commit: d7b8f8e20813f0179d8ef519541a3527e7661d3a
change-id: 20250726-tlv493d-sensor-v6_16-rc5-18c712093b27

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

[PATCH v3 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.

While the sensor has an interrupt pin multiplexed with an 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
Link: https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf [1]
Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>
---
 drivers/iio/magnetometer/Kconfig   |  13 +
 drivers/iio/magnetometer/Makefile  |   2 +
 drivers/iio/magnetometer/tlv493d.c | 524 +++++++++++++++++++++++++++++++++++++
 3 files changed, 539 insertions(+)

diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index 3debf1320ad1..e073ebe11810 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -173,6 +173,19 @@ config IIO_ST_MAGN_SPI_3AXIS
 	  To compile this driver as a module, choose M here. The module
 	  will be called st_magn_spi.
 
+config INFINEON_TLV493D
+	tristate "Infineon TLV493D Low-Power 3D Magnetic Sensor"
+	depends on 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 SENSORS_HMC5843
 	tristate
 	select IIO_BUFFER
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index 9297723a97d8..dfe970fcacb8 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -23,6 +23,8 @@ st_magn-$(CONFIG_IIO_BUFFER) += st_magn_buffer.o
 obj-$(CONFIG_IIO_ST_MAGN_I2C_3AXIS) += st_magn_i2c.o
 obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
 
+obj-$(CONFIG_INFINEON_TLV493D)		+= tlv493d.o
+
 obj-$(CONFIG_SENSORS_HMC5843)		+= hmc5843_core.o
 obj-$(CONFIG_SENSORS_HMC5843_I2C)	+= hmc5843_i2c.o
 obj-$(CONFIG_SENSORS_HMC5843_SPI)	+= hmc5843_spi.o
diff --git a/drivers/iio/magnetometer/tlv493d.c b/drivers/iio/magnetometer/tlv493d.c
new file mode 100644
index 000000000000..8611664f0237
--- /dev/null
+++ b/drivers/iio/magnetometer/tlv493d.c
@@ -0,0 +1,524 @@
+// 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/iopoll.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.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>
+
+/*
+ * TLV493D sensor I2C communication note:
+ *
+ * The sensor supports only direct byte-stream write starting from the
+ * 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 the register address in the I2C
+ * frame, it should contain only raw byte stream for the write registers.
+ * I2C Frame: |S|SlaveAddr Wr|Ack|Byte[0]|Ack|Byte[1]|Ack|.....|Sp|
+ *
+ * Same as the write operation, reading from the sensor registers is also
+ * performed starting from the register address 0x0 for as many bytes as
+ * need to be read.
+ * I2C read operation should not contain the register address in the I2C frame.
+ * I2C Frame: |S|SlaveAddr Rd|Ack|Byte[0]|Ack|Byte[1]|Ack|.....|Sp|
+ */
+
+#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_BX_MAG_X_AXIS_MSB	GENMASK(7, 0)
+#define TLV493D_BX2_MAG_X_AXIS_LSB	GENMASK(7, 4)
+#define TLV493D_BY_MAG_Y_AXIS_MSB	GENMASK(7, 0)
+#define TLV493D_BX2_MAG_Y_AXIS_LSB	GENMASK(3, 0)
+#define TLV493D_BZ_MAG_Z_AXIS_MSB	GENMASK(7, 0)
+#define TLV493D_BZ2_MAG_Z_AXIS_LSB	GENMASK(3, 0)
+#define TLV493D_TEMP_TEMP_MSB		GENMASK(7, 4)
+#define TLV493D_TEMP2_TEMP_LSB		GENMASK(7, 0)
+#define TLV493D_TEMP_CHANNEL		GENMASK(1, 0)
+#define TLV493D_MODE1_MOD_LOWFAST	GENMASK(1, 0)
+#define TLV493D_MODE2_LP_PERIOD	BIT(6)
+#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)
+
+enum tlv493d_channels {
+	TLV493D_AXIS_X = 0,
+	TLV493D_AXIS_Y,
+	TLV493D_AXIS_Z,
+	TLV493D_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
+};
+
+struct tlv493d_data {
+	struct device *dev;
+	struct i2c_client *client;
+	/* protects from simultaneous sensor access and register readings */
+	struct mutex lock;
+	enum tlv493d_op_mode mode;
+	u8 wr_regs[TLV493D_WR_REG_MAX];
+};
+
+/*
+ * Different mode has different measurement sampling time, this time is
+ * used in deriving the sleep and timeout 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 u32 tlv493d_sample_rate_us[] = { 0, 305, 10000, 100000, 305 };
+
+static int tlv493d_write_all_regs(struct tlv493d_data *data)
+{
+	int ret;
+
+	ret = i2c_master_send(data->client, data->wr_regs, ARRAY_SIZE(data->wr_regs));
+	if (ret < 0) {
+		dev_err(data->dev, "i2c write registers failed, error: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tlv493d_set_operating_mode(struct tlv493d_data *data, enum tlv493d_op_mode mode)
+{
+	u8 mode1_cfg, mode2_cfg;
+
+	switch (mode) {
+	case TLV493D_OP_MODE_POWERDOWN:
+		mode1_cfg = FIELD_PREP(TLV493D_MODE1_MOD_LOWFAST, 0);
+		mode2_cfg = FIELD_PREP(TLV493D_MODE2_LP_PERIOD, 0);
+		break;
+
+	case TLV493D_OP_MODE_FAST:
+		mode1_cfg = FIELD_PREP(TLV493D_MODE1_MOD_LOWFAST, 1);
+		mode2_cfg = FIELD_PREP(TLV493D_MODE2_LP_PERIOD, 0);
+		break;
+
+	case TLV493D_OP_MODE_LOWPOWER:
+		mode1_cfg = FIELD_PREP(TLV493D_MODE1_MOD_LOWFAST, 2);
+		mode2_cfg = FIELD_PREP(TLV493D_MODE2_LP_PERIOD, 1);
+		break;
+
+	case TLV493D_OP_MODE_ULTRA_LOWPOWER:
+		mode1_cfg = FIELD_PREP(TLV493D_MODE1_MOD_LOWFAST, 2);
+		mode2_cfg = FIELD_PREP(TLV493D_MODE2_LP_PERIOD, 0);
+		break;
+
+	case TLV493D_OP_MODE_MASTERCONTROLLED:
+		mode1_cfg = FIELD_PREP(TLV493D_MODE1_MOD_LOWFAST, 3);
+		mode2_cfg = FIELD_PREP(TLV493D_MODE2_LP_PERIOD, 0);
+		break;
+	}
+
+	data->wr_regs[TLV493D_WR_REG_MODE1] |= mode1_cfg;
+	data->wr_regs[TLV493D_WR_REG_MODE2] |= mode2_cfg;
+
+	return tlv493d_write_all_regs(data);
+}
+
+static s16 tlv493d_get_channel_data(u8 *b, enum tlv493d_channels ch)
+{
+	u16 val = 0;
+
+	switch (ch) {
+	case TLV493D_AXIS_X:
+		val = FIELD_GET(TLV493D_BX_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 |
+			FIELD_GET(TLV493D_BX2_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4;
+		break;
+	case TLV493D_AXIS_Y:
+		val = FIELD_GET(TLV493D_BY_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
+			FIELD_GET(TLV493D_BX2_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
+		break;
+	case TLV493D_AXIS_Z:
+		val = FIELD_GET(TLV493D_BZ_MAG_Z_AXIS_MSB, b[TLV493D_RD_REG_BZ]) << 4 |
+			FIELD_GET(TLV493D_BZ2_MAG_Z_AXIS_LSB, b[TLV493D_RD_REG_BZ2]);
+		break;
+	case TLV493D_TEMPERATURE:
+		val = FIELD_GET(TLV493D_TEMP_TEMP_MSB, b[TLV493D_RD_REG_TEMP]) << 8 |
+			FIELD_GET(TLV493D_TEMP2_TEMP_LSB, b[TLV493D_RD_REG_TEMP2]);
+		break;
+	}
+
+	return sign_extend32(val, 11);
+}
+
+static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
+				s16 *z, s16 *t)
+{
+	u8 buff[7] = {};
+	int err, ret;
+	u32 sleep_us = tlv493d_sample_rate_us[data->mode];
+
+	guard(mutex)(&data->lock);
+
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Poll until data is valid,
+	 * For a valid data TLV493D_TEMP_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(i2c_master_recv, err, err ||
+			FIELD_GET(TLV493D_TEMP_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
+			sleep_us, (3 * sleep_us), false, data->client, buff,
+			ARRAY_SIZE(buff));
+	if (ret) {
+		dev_err(data->dev, "i2c read poll timeout, error:%d\n", ret);
+		goto out;
+	}
+	if (err < 0) {
+		dev_err(data->dev, "i2c read data failed, error:%d\n", err);
+		ret = err;
+		goto out;
+	}
+
+	*x = tlv493d_get_channel_data(buff, TLV493D_AXIS_X);
+	*y = tlv493d_get_channel_data(buff, TLV493D_AXIS_Y);
+	*z = tlv493d_get_channel_data(buff, TLV493D_AXIS_Z);
+	*t = tlv493d_get_channel_data(buff, TLV493D_TEMPERATURE);
+
+out:
+	pm_runtime_put_autosuspend(data->dev);
+	return ret;
+}
+
+static int tlv493d_init(struct tlv493d_data *data)
+{
+	int ret;
+	u8 buff[TLV493D_RD_REG_MAX];
+
+	/*
+	 * 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 = i2c_master_recv(data->client, buff, ARRAY_SIZE(buff));
+	if (ret < 0) {
+		dev_err(data->dev, "i2c read failed, error %d\n", ret);
+		return ret;
+	}
+
+	/* Write register 0x0 is reserved. Does not require to be updated.*/
+	data->wr_regs[0] = 0;
+	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 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_RAW:
+		ret = tlv493d_get_measurements(data, &x, &y, &z, &t);
+		if (ret)
+			return ret;
+
+		/* Return raw values for requested channel */
+		switch (chan->address) {
+		case TLV493D_AXIS_X:
+			*val = x;
+			return IIO_VAL_INT;
+		case TLV493D_AXIS_Y:
+			*val = y;
+			return IIO_VAL_INT;
+		case TLV493D_AXIS_Z:
+			*val = z;
+			return IIO_VAL_INT;
+		case TLV493D_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)
+			 * Magnetic field in Gauss: mT * 10 = 0.098.
+			 */
+			*val = 98;
+			*val2 = 1000;
+			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 = 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.
+			 * offset = -raw_offset + (25000 / 1100)
+			 * -340 + 22.72 = -317.28
+			 */
+			*val = -31728;
+			*val2 = 100;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
+
+	default:
+		return -EINVAL;
+	}
+}
+
+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, TLV493D_AXIS_X),
+	TLV493D_AXIS_CHANNEL(Y, TLV493D_AXIS_Y),
+	TLV493D_AXIS_CHANNEL(Z, TLV493D_AXIS_Z),
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				BIT(IIO_CHAN_INFO_SCALE) |
+				BIT(IIO_CHAN_INFO_OFFSET),
+		.address = TLV493D_TEMPERATURE,
+		.scan_index = TLV493D_TEMPERATURE,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 12,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+static const struct iio_info tlv493d_info = {
+	.read_raw = tlv493d_read_raw,
+};
+
+static const unsigned long tlv493d_scan_masks[] = { GENMASK(3, 0), 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;
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable regulator\n");
+
+	/*
+	 * Setting Sensor default operating mode to Master-Controlled mode since
+	 * it performs measurement cycle only on-request and stays in Power-Down
+	 * state until next cycle is initiated.
+	 */
+	data->mode = TLV493D_OP_MODE_MASTERCONTROLLED;
+	ret = tlv493d_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to initialize\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,
+				NULL);
+	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 tlv493d_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return tlv493d_set_operating_mode(data, TLV493D_OP_MODE_POWERDOWN);
+}
+
+static int tlv493d_runtime_resume(struct device *dev)
+{
+	struct tlv493d_data *data = iio_priv(dev_get_drvdata(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

[PATCH v3 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).
Drop duplicate entry for infineon,tlv493d from trivial-devices.yaml as
its documented in this seperate dt-binding file now.

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-a1b6.yaml    | 45 ++++++++++++++++++++++
 .../devicetree/bindings/trivial-devices.yaml       |  2 -
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d-a1b6.yaml b/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d-a1b6.yaml
new file mode 100644
index 000000000000..dd23a9370a71
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/infineon,tlv493d-a1b6.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/magnetometer/infineon,tlv493d-a1b6.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 VDD supply
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      magnetometer@5e {
+        compatible = "infineon,tlv493d-a1b6";
+        reg = <0x5e>;
+        vdd-supply = <&hall_vcc>;
+      };
+    };
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 27930708ccd5..9e0eb5c873d2 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -125,8 +125,6 @@ properties:
           - infineon,ir36021
             # Infineon IRPS5401 Voltage Regulator (PMIC)
           - infineon,irps5401
-            # Infineon TLV493D-A1B6 I2C 3D Magnetic Sensor
-          - infineon,tlv493d-a1b6
             # Infineon Hot-swap controller xdp710
           - infineon,xdp710
             # Infineon Multi-phase Digital VR Controller xdpe11280

-- 
2.43.0

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

[Krzysztof Kozlowski]

On Thu, Aug 07, 2025 at 08:26:36AM +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).
> Drop duplicate entry for infineon,tlv493d from trivial-devices.yaml as
> its documented in this seperate dt-binding file now.

Typo, separate

> 
> 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>

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

<form letter>
This is an automated instruction, just in case, because many review
tags are being ignored. If you know the process, just skip it entirely
(please do not feel offended by me posting it here - no bad intentions
intended, no patronizing, I just want to avoid wasted efforts). If you
do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions of patchset, under or above your Signed-off-by tag, unless
patch changed significantly (e.g. new properties added to the DT
bindings). Tag is "received", when provided in a message replied to you
on the mailing list. Tools like b4 can help here ('b4 trailers -u ...').
However, there's no need to repost patches *only* to add the tags. The
upstream maintainer will do that for tags received on the version they
apply.

https://elixir.bootlin.com/linux/v6.15/source/Documentation/process/submitting-patches.rst#L591
</form letter>

Best regards,
Krzysztof

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

[Andy Shevchenko]

On Thu, Aug 7, 2025 at 4:57 AM Dixit Parmar <dixitparmar19@gmail.com> 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.
>
> While the sensor has an interrupt pin multiplexed with an 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].

...

> +       help
> +         Say Y here to add support for the Infineon TLV493D-A1B6 Low-
> +         Power 3D Megnetic Sensor.

Megnetic?

> +         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.

...

> +#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

+ blank line

> +#define TLV493D_WR_REG_RES     0x00

I would name it _RES0 in analogue with the _RES2 below.

> +#define TLV493D_WR_REG_MODE1   0x01
> +#define TLV493D_WR_REG_RES2    0x02
> +#define TLV493D_WR_REG_MODE2   0x03
> +#define TLV493D_WR_REG_MAX     0x04

...

> +enum tlv493d_channels {
> +       TLV493D_AXIS_X = 0,

Why assignment? Is this HW defined value? Then you must assign all of
them explicitly to make code robust to changes.

> +       TLV493D_AXIS_Y,
> +       TLV493D_AXIS_Z,
> +       TLV493D_TEMPERATURE
> +};
> +
> +enum tlv493d_op_mode {
> +       TLV493D_OP_MODE_POWERDOWN = 0,

Ditto.

> +       TLV493D_OP_MODE_FAST,
> +       TLV493D_OP_MODE_LOWPOWER,
> +       TLV493D_OP_MODE_ULTRA_LOWPOWER,
> +       TLV493D_OP_MODE_MASTERCONTROLLED
> +};

...

> +struct tlv493d_data {
> +       struct device *dev;
> +       struct i2c_client *client;

Why do you need both?

> +       /* protects from simultaneous sensor access and register readings */
> +       struct mutex lock;
> +       enum tlv493d_op_mode mode;

> +       u8 wr_regs[TLV493D_WR_REG_MAX];
> +};

...

> +       data->wr_regs[TLV493D_WR_REG_MODE1] |= mode1_cfg;
> +       data->wr_regs[TLV493D_WR_REG_MODE2] |= mode2_cfg;

No mask for the existing values in the respective wr_regs? Wouldn't
you need to use FIELD_MODIFY() instead?

...

> +static s16 tlv493d_get_channel_data(u8 *b, enum tlv493d_channels ch)
> +{
> +       u16 val = 0;

I would move the default assignment to the 'default' case. This makes
the intention clearer.

> +       switch (ch) {
> +       case TLV493D_AXIS_X:
> +               val = FIELD_GET(TLV493D_BX_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 |
> +                       FIELD_GET(TLV493D_BX2_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4;
> +               break;
> +       case TLV493D_AXIS_Y:
> +               val = FIELD_GET(TLV493D_BY_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
> +                       FIELD_GET(TLV493D_BX2_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
> +               break;
> +       case TLV493D_AXIS_Z:
> +               val = FIELD_GET(TLV493D_BZ_MAG_Z_AXIS_MSB, b[TLV493D_RD_REG_BZ]) << 4 |
> +                       FIELD_GET(TLV493D_BZ2_MAG_Z_AXIS_LSB, b[TLV493D_RD_REG_BZ2]);
> +               break;
> +       case TLV493D_TEMPERATURE:
> +               val = FIELD_GET(TLV493D_TEMP_TEMP_MSB, b[TLV493D_RD_REG_TEMP]) << 8 |
> +                       FIELD_GET(TLV493D_TEMP2_TEMP_LSB, b[TLV493D_RD_REG_TEMP2]);
> +               break;
> +       }
> +
> +       return sign_extend32(val, 11);
> +}

...

> +static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
> +                               s16 *z, s16 *t)
> +{
> +       u8 buff[7] = {};
> +       int err, ret;
> +       u32 sleep_us = tlv493d_sample_rate_us[data->mode];
> +
> +       guard(mutex)(&data->lock);

No include for this API.

> +       ret = pm_runtime_resume_and_get(data->dev);
> +       if (ret < 0)
> +               return ret;
> +
> +       /*
> +        * Poll until data is valid,
> +        * For a valid data TLV493D_TEMP_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(i2c_master_recv, err, err ||
> +                       FIELD_GET(TLV493D_TEMP_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> +                       sleep_us, (3 * sleep_us), false, data->client, buff,

Redundant parentheses.

> +                       ARRAY_SIZE(buff));

Missing include for this macro.

> +       if (ret) {
> +               dev_err(data->dev, "i2c read poll timeout, error:%d\n", ret);
> +               goto out;
> +       }
> +       if (err < 0) {
> +               dev_err(data->dev, "i2c read data failed, error:%d\n", err);
> +               ret = err;
> +               goto out;
> +       }
> +
> +       *x = tlv493d_get_channel_data(buff, TLV493D_AXIS_X);
> +       *y = tlv493d_get_channel_data(buff, TLV493D_AXIS_Y);
> +       *z = tlv493d_get_channel_data(buff, TLV493D_AXIS_Z);
> +       *t = tlv493d_get_channel_data(buff, TLV493D_TEMPERATURE);
> +
> +out:

Labels are better made when they define what they are going to perform.

out_put_autosuspend:

> +       pm_runtime_put_autosuspend(data->dev);
> +       return ret;
> +}

...

> +       ret = tlv493d_set_operating_mode(data, data->mode);
> +       if (ret < 0) {

Is ' < 0' part required here?

> +               dev_err(data->dev, "failed to set operating mode\n");
> +               return ret;
> +       }
> +
> +       return 0;

If not, these all lines can be transformed to just

  return ret;

> +}

...

> +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;

> +

No blank lines in the definition block.

> +       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:

Make sure you use a consistent pattern for labels.

out_trigger_notify:

> +       iio_trigger_notify_done(indio_dev->trig);
> +
> +       return IRQ_HANDLED;
> +}

...

> +       data->dev = dev;
> +       data->client = client;

Choose one of them, the other can be derived.

...

> +               return dev_err_probe(dev, ret, "failed to initialize\n");

Missing include for this API.

...

> +static const struct i2c_device_id tlv493d_id[] = {
> +       { "tlv493d" },
> +       { }
> +};

> +static const struct of_device_id tlv493d_of_match[] = {
> +       { .compatible = "infineon,tlv493d-a1b6", },

Inner comma is redundant.

> +       { }
> +};

Missing include for both of the ID tables.

...

> +static struct i2c_driver tlv493d_driver = {
> +       .driver = {
> +               .name = "tlv493d",
> +               .of_match_table = tlv493d_of_match,

> +               .pm = pm_ptr(&tlv493d_pm_ops),

Missing include for this macro I believe.

> +       },
> +       .probe = tlv493d_probe,
> +       .id_table = tlv493d_id,
> +};

> +

Remove this blank line.

> +module_i2c_driver(tlv493d_driver);

-- 
With Best Regards,
Andy Shevchenko

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

[Dixit Parmar]

On Thu, Aug 07, 2025 at 10:57:16PM +0200, Andy Shevchenko wrote:
> On Thu, Aug 7, 2025 at 4:57 AM Dixit Parmar <dixitparmar19@gmail.com> wrote:
> >
> 
> > +enum tlv493d_channels {
> > +       TLV493D_AXIS_X = 0,
> 
> Why assignment? Is this HW defined value? Then you must assign all of
> them explicitly to make code robust to changes.
>
No, this is not HW defined value, these are used for iio channel and
some internall indexing. Most of the driver I referred had this enum
having assigned to 0 which i think gives clear intention and better
understanding. either I can keep as it is assuming its good for
readabilty or keep it unassigned. What do you suggest?
> > +       TLV493D_AXIS_Y,
> > +       TLV493D_AXIS_Z,
> > +       TLV493D_TEMPERATURE
> > +};
> > +
> > +enum tlv493d_op_mode {
> > +       TLV493D_OP_MODE_POWERDOWN = 0,
> 
> Ditto.
>
Same as above. Just different usecase as this is driver specific enums.
> > +       TLV493D_OP_MODE_FAST,
> > +       TLV493D_OP_MODE_LOWPOWER,
> > +       TLV493D_OP_MODE_ULTRA_LOWPOWER,
> > +       TLV493D_OP_MODE_MASTERCONTROLLED
> > +};
> 
> ...
> 
> > +struct tlv493d_data {
> > +       struct device *dev;
> > +       struct i2c_client *client;
> 
> Why do you need both?
Indeed, I should drop struct device *dev member.
> 
> > +       /* protects from simultaneous sensor access and register readings */
> > +       struct mutex lock;
> > +       enum tlv493d_op_mode mode;
> 
> > +       u8 wr_regs[TLV493D_WR_REG_MAX];
> > +};
> 
> ...
> 
> > +       data->wr_regs[TLV493D_WR_REG_MODE1] |= mode1_cfg;
> > +       data->wr_regs[TLV493D_WR_REG_MODE2] |= mode2_cfg;
> 
> No mask for the existing values in the respective wr_regs? Wouldn't
> you need to use FIELD_MODIFY() instead?
>
I believe, we are doing OR op with the value created using FIELD_PREP,
so it should not interefere with the existing non-masked values.
However, as FIELD_MODIFY is there, I should utilize it.
> ...
> 
> > +static s16 tlv493d_get_channel_data(u8 *b, enum tlv493d_channels ch)
> > +{
> > +       u16 val = 0;
> 
> I would move the default assignment to the 'default' case. This makes
> the intention clearer.
> 
As per the suggestion on privious version of the patch, we are having
ch datatype as enum and as suggested, with enum as swicth-case, it
should not have default case. so I think this initialisation to 0 at the
beginning should be fine.
> > +       switch (ch) {
> > +       case TLV493D_AXIS_X:
> > +               val = FIELD_GET(TLV493D_BX_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 |
> > +                       FIELD_GET(TLV493D_BX2_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4;
> > +               break;
> > +       case TLV493D_AXIS_Y:
> > +               val = FIELD_GET(TLV493D_BY_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
> > +                       FIELD_GET(TLV493D_BX2_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
> > +               break;
> > +       case TLV493D_AXIS_Z:
> > +               val = FIELD_GET(TLV493D_BZ_MAG_Z_AXIS_MSB, b[TLV493D_RD_REG_BZ]) << 4 |
> > +                       FIELD_GET(TLV493D_BZ2_MAG_Z_AXIS_LSB, b[TLV493D_RD_REG_BZ2]);
> > +               break;
> > +       case TLV493D_TEMPERATURE:
> > +               val = FIELD_GET(TLV493D_TEMP_TEMP_MSB, b[TLV493D_RD_REG_TEMP]) << 8 |
> > +                       FIELD_GET(TLV493D_TEMP2_TEMP_LSB, b[TLV493D_RD_REG_TEMP2]);
> > +               break;
> > +       }
> > +
> > +       return sign_extend32(val, 11);
> > +}
> 
> ...
> 
> > +static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
> > +                               s16 *z, s16 *t)
> > +{
> > +       u8 buff[7] = {};
> > +       int err, ret;
> > +       u32 sleep_us = tlv493d_sample_rate_us[data->mode];
> > +
> > +       guard(mutex)(&data->lock);
> 
> No include for this API.
> 
General question for all the include related suggestions, all the
required headers are being included by one of the included header(i2c.h,
iio.h etc), in such case, is it necessary to have specific include for
given API mentioned in source file? Will it not make it more clumsy
in terms of repeatative header includes? I understand having all the
includes mentioned in given source file makes it clear to understand the
dependency the driver is having. Just want to undertand it bit more as
learning.
> > +       ret = pm_runtime_resume_and_get(data->dev);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       /*
> > +        * Poll until data is valid,
> > +        * For a valid data TLV493D_TEMP_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(i2c_master_recv, err, err ||
> > +                       FIELD_GET(TLV493D_TEMP_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> > +                       sleep_us, (3 * sleep_us), false, data->client, buff,
> 
> Redundant parentheses.
> 
For (3 * sleep_us)?
> > +                       ARRAY_SIZE(buff));
> 
> Missing include for this macro.
> 
> > +       if (ret) {
> > +               dev_err(data->dev, "i2c read poll timeout, error:%d\n", ret);
> > +               goto out;
> > +       }
> > +       if (err < 0) {
> > +               dev_err(data->dev, "i2c read data failed, error:%d\n", err);
> > +               ret = err;
> > +               goto out;
> > +       }
> > +
> > +       *x = tlv493d_get_channel_data(buff, TLV493D_AXIS_X);
> > +       *y = tlv493d_get_channel_data(buff, TLV493D_AXIS_Y);
> > +       *z = tlv493d_get_channel_data(buff, TLV493D_AXIS_Z);
> > +       *t = tlv493d_get_channel_data(buff, TLV493D_TEMPERATURE);
> > +
> > +out:
> 
> Labels are better made when they define what they are going to perform.
> 
> out_put_autosuspend:
> 
Does it mean it should have whatever is being skipped in the flow?
> > +       pm_runtime_put_autosuspend(data->dev);
> > +       return ret;
> > +}
> 
> ...
> 
> > +       ret = tlv493d_set_operating_mode(data, data->mode);
> > +       if (ret < 0) {
> 
> Is ' < 0' part required here?
> 
> > +               dev_err(data->dev, "failed to set operating mode\n");
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> 
> If not, these all lines can be transformed to just
> 
>   return ret;
> 
Yes, return 0 and the check for ret has been kept as per the previous
review suggestion. The return value from the tlv493d_set_operating_mode
is returned from i2c_master_send() via few function inbetween and its result
has to be conveyed to the caller API as we are in initialization phase.
> > +}
> 
> ...
> 
> > +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;
> 
> > +
> 
> No blank lines in the definition block.
> 
> > +       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:
> 
> Make sure you use a consistent pattern for labels.
> 
> out_trigger_notify:
> 
> > +       iio_trigger_notify_done(indio_dev->trig);
> > +
> > +       return IRQ_HANDLED;
> > +}
> 
> ...
> 
> > +       data->dev = dev;
> > +       data->client = client;
> 
> Choose one of them, the other can be derived.
> 
ACK.
> ...
> 
> > +               return dev_err_probe(dev, ret, "failed to initialize\n");
> 
> Missing include for this API.
> 
> ...
> 
> > +static const struct i2c_device_id tlv493d_id[] = {
> > +       { "tlv493d" },
> > +       { }
> > +};
> 
> > +static const struct of_device_id tlv493d_of_match[] = {
> > +       { .compatible = "infineon,tlv493d-a1b6", },
> 
> Inner comma is redundant.
> 
> > +       { }
> > +};
> 
> Missing include for both of the ID tables.
The ID tables are defined mod_devicetable.h in which intern gets
included in i2c.h and i2c.h is already included in this driver file,
should I explicitely include mod_devicetable.h here?
> ...
> 
> > +static struct i2c_driver tlv493d_driver = {
> > +       .driver = {
> > +               .name = "tlv493d",
> > +               .of_match_table = tlv493d_of_match,
> 
> > +               .pm = pm_ptr(&tlv493d_pm_ops),
> 
> Missing include for this macro I believe.
> 
No I guess. DEFINE_RUNTIME_DEV_PM_OPS is part of pm_runtime.h and its
already included.
> > +       },
> > +       .probe = tlv493d_probe,
> > +       .id_table = tlv493d_id,
> > +};
> 
> > +
> 
> Remove this blank line.
> 
> > +module_i2c_driver(tlv493d_driver);
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Thank you for careful review,
Dixit

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

[Dixit Parmar]

On Thu, Aug 07, 2025 at 10:57:16PM +0200, Andy Shevchenko wrote:
> On Thu, Aug 7, 2025 at 4:57 AM Dixit Parmar <dixitparmar19@gmail.com> 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.
> >
> > While the sensor has an interrupt pin multiplexed with an 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].
> 
> ...
> 
> > +       help
> > +         Say Y here to add support for the Infineon TLV493D-A1B6 Low-
> > +         Power 3D Megnetic Sensor.
> 
> Megnetic?
> 
> > +         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.
> 
> ...
> 
> > +#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
> 
> + blank line
> 
> > +#define TLV493D_WR_REG_RES     0x00
> 
> I would name it _RES0 in analogue with the _RES2 below.
>
We are not using these TLV493D_WR_REG_RES* registers anywhere,
so I shall drop TLV493D_WR_REG_RES2 too.
> > +#define TLV493D_WR_REG_MODE1   0x01
> > +#define TLV493D_WR_REG_RES2    0x02
> > +#define TLV493D_WR_REG_MODE2   0x03
> > +#define TLV493D_WR_REG_MAX     0x04
> 
> ...
> 
> > +enum tlv493d_channels {
> > +       TLV493D_AXIS_X = 0,
> 
> Why assignment? Is this HW defined value? Then you must assign all of
> them explicitly to make code robust to changes.
> 
> > +       TLV493D_AXIS_Y,
> > +       TLV493D_AXIS_Z,
> > +       TLV493D_TEMPERATURE
> > +};
> > +
> > +enum tlv493d_op_mode {
> > +       TLV493D_OP_MODE_POWERDOWN = 0,
> 
> Ditto.
> 
> > +       TLV493D_OP_MODE_FAST,
> > +       TLV493D_OP_MODE_LOWPOWER,
> > +       TLV493D_OP_MODE_ULTRA_LOWPOWER,
> > +       TLV493D_OP_MODE_MASTERCONTROLLED
> > +};
> 
> ...
> 
> > +struct tlv493d_data {
> > +       struct device *dev;
> > +       struct i2c_client *client;
> 
> Why do you need both?
> 
> > +       /* protects from simultaneous sensor access and register readings */
> > +       struct mutex lock;
> > +       enum tlv493d_op_mode mode;
> 
> > +       u8 wr_regs[TLV493D_WR_REG_MAX];
> > +};
> 
> ...
> 
> > +       data->wr_regs[TLV493D_WR_REG_MODE1] |= mode1_cfg;
> > +       data->wr_regs[TLV493D_WR_REG_MODE2] |= mode2_cfg;
> 
> No mask for the existing values in the respective wr_regs? Wouldn't
> you need to use FIELD_MODIFY() instead?
> 
> ...
> 
> > +static s16 tlv493d_get_channel_data(u8 *b, enum tlv493d_channels ch)
> > +{
> > +       u16 val = 0;
> 
> I would move the default assignment to the 'default' case. This makes
> the intention clearer.
> 
> > +       switch (ch) {
> > +       case TLV493D_AXIS_X:
> > +               val = FIELD_GET(TLV493D_BX_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 |
> > +                       FIELD_GET(TLV493D_BX2_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4;
> > +               break;
> > +       case TLV493D_AXIS_Y:
> > +               val = FIELD_GET(TLV493D_BY_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
> > +                       FIELD_GET(TLV493D_BX2_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
> > +               break;
> > +       case TLV493D_AXIS_Z:
> > +               val = FIELD_GET(TLV493D_BZ_MAG_Z_AXIS_MSB, b[TLV493D_RD_REG_BZ]) << 4 |
> > +                       FIELD_GET(TLV493D_BZ2_MAG_Z_AXIS_LSB, b[TLV493D_RD_REG_BZ2]);
> > +               break;
> > +       case TLV493D_TEMPERATURE:
> > +               val = FIELD_GET(TLV493D_TEMP_TEMP_MSB, b[TLV493D_RD_REG_TEMP]) << 8 |
> > +                       FIELD_GET(TLV493D_TEMP2_TEMP_LSB, b[TLV493D_RD_REG_TEMP2]);
> > +               break;
> > +       }
> > +
> > +       return sign_extend32(val, 11);
> > +}
> 
> ...
> 
> > +static int tlv493d_get_measurements(struct tlv493d_data *data, s16 *x, s16 *y,
> > +                               s16 *z, s16 *t)
> > +{
> > +       u8 buff[7] = {};
> > +       int err, ret;
> > +       u32 sleep_us = tlv493d_sample_rate_us[data->mode];
> > +
> > +       guard(mutex)(&data->lock);
> 
> No include for this API.
> 
> > +       ret = pm_runtime_resume_and_get(data->dev);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       /*
> > +        * Poll until data is valid,
> > +        * For a valid data TLV493D_TEMP_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(i2c_master_recv, err, err ||
> > +                       FIELD_GET(TLV493D_TEMP_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> > +                       sleep_us, (3 * sleep_us), false, data->client, buff,
> 
> Redundant parentheses.
> 
> > +                       ARRAY_SIZE(buff));
> 
> Missing include for this macro.
> 
> > +       if (ret) {
> > +               dev_err(data->dev, "i2c read poll timeout, error:%d\n", ret);
> > +               goto out;
> > +       }
> > +       if (err < 0) {
> > +               dev_err(data->dev, "i2c read data failed, error:%d\n", err);
> > +               ret = err;
> > +               goto out;
> > +       }
> > +
> > +       *x = tlv493d_get_channel_data(buff, TLV493D_AXIS_X);
> > +       *y = tlv493d_get_channel_data(buff, TLV493D_AXIS_Y);
> > +       *z = tlv493d_get_channel_data(buff, TLV493D_AXIS_Z);
> > +       *t = tlv493d_get_channel_data(buff, TLV493D_TEMPERATURE);
> > +
> > +out:
> 
> Labels are better made when they define what they are going to perform.
> 
> out_put_autosuspend:
> 
> > +       pm_runtime_put_autosuspend(data->dev);
> > +       return ret;
> > +}
> 
> ...
> 
> > +       ret = tlv493d_set_operating_mode(data, data->mode);
> > +       if (ret < 0) {
> 
> Is ' < 0' part required here?
> 
> > +               dev_err(data->dev, "failed to set operating mode\n");
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> 
> If not, these all lines can be transformed to just
> 
>   return ret;
> 
> > +}
> 
> ...
> 
> > +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;
> 
> > +
> 
> No blank lines in the definition block.
> 
> > +       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:
> 
> Make sure you use a consistent pattern for labels.
> 
> out_trigger_notify:
> 
> > +       iio_trigger_notify_done(indio_dev->trig);
> > +
> > +       return IRQ_HANDLED;
> > +}
> 
> ...
> 
> > +       data->dev = dev;
> > +       data->client = client;
> 
> Choose one of them, the other can be derived.
> 
> ...
> 
> > +               return dev_err_probe(dev, ret, "failed to initialize\n");
> 
> Missing include for this API.
> 
> ...
> 
> > +static const struct i2c_device_id tlv493d_id[] = {
> > +       { "tlv493d" },
> > +       { }
> > +};
> 
> > +static const struct of_device_id tlv493d_of_match[] = {
> > +       { .compatible = "infineon,tlv493d-a1b6", },
> 
> Inner comma is redundant.
> 
> > +       { }
> > +};
> 
> Missing include for both of the ID tables.
> 
> ...
> 
> > +static struct i2c_driver tlv493d_driver = {
> > +       .driver = {
> > +               .name = "tlv493d",
> > +               .of_match_table = tlv493d_of_match,
> 
> > +               .pm = pm_ptr(&tlv493d_pm_ops),
> 
> Missing include for this macro I believe.
> 
> > +       },
> > +       .probe = tlv493d_probe,
> > +       .id_table = tlv493d_id,
> > +};
> 
> > +
> 
> Remove this blank line.
> 
> > +module_i2c_driver(tlv493d_driver);
> 
> -- 
> With Best Regards,
> Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Aug 9, 2025 at 1:29 PM Dixit Parmar <dixitparmar19@gmail.com> wrote:
> On Thu, Aug 07, 2025 at 10:57:16PM +0200, Andy Shevchenko wrote:
> > On Thu, Aug 7, 2025 at 4:57 AM Dixit Parmar <dixitparmar19@gmail.com> wrote:

...

> > > +enum tlv493d_channels {
> > > +       TLV493D_AXIS_X = 0,
> >
> > Why assignment? Is this HW defined value? Then you must assign all of
> > them explicitly to make code robust to changes.
> >
> No, this is not HW defined value, these are used for iio channel and
> some internall indexing. Most of the driver I referred had this enum
> having assigned to 0 which i think gives clear intention and better
> understanding. either I can keep as it is assuming its good for
> readabilty or keep it unassigned. What do you suggest?

Read C standard?
If it's only driver specific things and not related to HW, assigning
to any value makes no sense, The only requirement here they should be
unique and that's what enum does.

> > > +       TLV493D_AXIS_Y,
> > > +       TLV493D_AXIS_Z,
> > > +       TLV493D_TEMPERATURE
> > > +};
> > > +
> > > +enum tlv493d_op_mode {
> > > +       TLV493D_OP_MODE_POWERDOWN = 0,
> >
> > Ditto.
> >
> Same as above. Just different usecase as this is driver specific enums.

Same as above.

> > > +       TLV493D_OP_MODE_FAST,
> > > +       TLV493D_OP_MODE_LOWPOWER,
> > > +       TLV493D_OP_MODE_ULTRA_LOWPOWER,
> > > +       TLV493D_OP_MODE_MASTERCONTROLLED
> > > +};

...

> > > +       data->wr_regs[TLV493D_WR_REG_MODE1] |= mode1_cfg;
> > > +       data->wr_regs[TLV493D_WR_REG_MODE2] |= mode2_cfg;
> >
> > No mask for the existing values in the respective wr_regs? Wouldn't
> > you need to use FIELD_MODIFY() instead?
> >
> I believe, we are doing OR op with the value created using FIELD_PREP,
> so it should not interefere with the existing non-masked values.

I am talking about existing values in the array.

> However, as FIELD_MODIFY is there, I should utilize it.

...

> > > +static s16 tlv493d_get_channel_data(u8 *b, enum tlv493d_channels ch)
> > > +{
> > > +       u16 val = 0;
> >
> > I would move the default assignment to the 'default' case. This makes
> > the intention clearer.
> >
> As per the suggestion on privious version of the patch, we are having
> ch datatype as enum and as suggested, with enum as swicth-case, it
> should not have default case. so I think this initialisation to 0 at the
> beginning should be fine.

It will make no sense. Please, remove it. and perhaps the compiler
won't warn, otherwise the default case will be needed.

> > > +       switch (ch) {
> > > +       case TLV493D_AXIS_X:
> > > +               val = FIELD_GET(TLV493D_BX_MAG_X_AXIS_MSB, b[TLV493D_RD_REG_BX]) << 4 |
> > > +                       FIELD_GET(TLV493D_BX2_MAG_X_AXIS_LSB, b[TLV493D_RD_REG_BX2]) >> 4;
> > > +               break;
> > > +       case TLV493D_AXIS_Y:
> > > +               val = FIELD_GET(TLV493D_BY_MAG_Y_AXIS_MSB, b[TLV493D_RD_REG_BY]) << 4 |
> > > +                       FIELD_GET(TLV493D_BX2_MAG_Y_AXIS_LSB, b[TLV493D_RD_REG_BX2]);
> > > +               break;
> > > +       case TLV493D_AXIS_Z:
> > > +               val = FIELD_GET(TLV493D_BZ_MAG_Z_AXIS_MSB, b[TLV493D_RD_REG_BZ]) << 4 |
> > > +                       FIELD_GET(TLV493D_BZ2_MAG_Z_AXIS_LSB, b[TLV493D_RD_REG_BZ2]);
> > > +               break;
> > > +       case TLV493D_TEMPERATURE:
> > > +               val = FIELD_GET(TLV493D_TEMP_TEMP_MSB, b[TLV493D_RD_REG_TEMP]) << 8 |
> > > +                       FIELD_GET(TLV493D_TEMP2_TEMP_LSB, b[TLV493D_RD_REG_TEMP2]);
> > > +               break;
> > > +       }
> > > +
> > > +       return sign_extend32(val, 11);
> > > +}

...

> > > +       guard(mutex)(&data->lock);
> >
> > No include for this API.
> >
> General question for all the include related suggestions, all the
> required headers are being included by one of the included header(i2c.h,
> iio.h etc), in such case, is it necessary to have specific include for
> given API mentioned in source file? Will it not make it more clumsy
> in terms of repeatative header includes? I understand having all the
> includes mentioned in given source file makes it clear to understand the
> dependency the driver is having. Just want to undertand it bit more as
> learning.

It's called the IWYU principle.
Read more about it here: https://include-what-you-use.org/

...

> > > +       ret = read_poll_timeout(i2c_master_recv, err, err ||
> > > +                       FIELD_GET(TLV493D_TEMP_CHANNEL, buff[TLV493D_RD_REG_TEMP]) == 0,
> > > +                       sleep_us, (3 * sleep_us), false, data->client, buff,
> >
> > Redundant parentheses.
> >
> For (3 * sleep_us)?

Yes.

> > > +                       ARRAY_SIZE(buff));

...

> > > +out:
> >
> > Labels are better made when they define what they are going to perform.
> >
> > out_put_autosuspend:
> >
> Does it mean it should have whatever is being skipped in the flow?

I don't understand the question. They should answer the question:
"What will happen if I goto $LABEL?"

> > > +       pm_runtime_put_autosuspend(data->dev);
> > > +       return ret;

...

> > > +       data->dev = dev;
> > > +       data->client = client;
> >
> > Choose one of them, the other can be derived.
> >
> ACK.

Many of the comments were being aimply ignored. Are they ACKed or
what? The rule of thumb is to completely remove the pieces you fully
agree with, this will reduce churn in the emails and make a reviewer
happier.

...

> > Missing include for both of the ID tables.
> The ID tables are defined mod_devicetable.h in which intern gets
> included in i2c.h and i2c.h is already included in this driver file,
> should I explicitely include mod_devicetable.h here?

See above, follow IWYU.

...

> > > +static struct i2c_driver tlv493d_driver = {
> > > +       .driver = {
> > > +               .name = "tlv493d",
> > > +               .of_match_table = tlv493d_of_match,
> >
> > > +               .pm = pm_ptr(&tlv493d_pm_ops),
> >
> > Missing include for this macro I believe.
> >
> No I guess. DEFINE_RUNTIME_DEV_PM_OPS is part of pm_runtime.h and its
> already included.

And how is it related to my comment _here_ in the code?

> > > +       },
> > > +       .probe = tlv493d_probe,
> > > +       .id_table = tlv493d_id,
> > > +};

-- 
With Best Regards,
Andy Shevchenko

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

[Dixit Parmar]

On Sat, Aug 09, 2025 at 02:44:00PM +0200, Andy Shevchenko wrote:
> > > > +       data->wr_regs[TLV493D_WR_REG_MODE1] |= mode1_cfg;
> > > > +       data->wr_regs[TLV493D_WR_REG_MODE2] |= mode2_cfg;
> > >
> > > No mask for the existing values in the respective wr_regs? Wouldn't
> > > you need to use FIELD_MODIFY() instead?
> > >
> > I believe, we are doing OR op with the value created using FIELD_PREP,
> > so it should not interefere with the existing non-masked values.
> 
> I am talking about existing values in the array.
>
Right. So in that I think it will make more sense to directly use
FIELD_MODIFY instead of using FIELD_PREP first and then doing this OR
op. Right?
> > However, as FIELD_MODIFY is there, I should utilize it.
> 
> > > > +       u16 val = 0;
> > >
> > > I would move the default assignment to the 'default' case. This makes
> > > the intention clearer.
> > >
> > As per the suggestion on privious version of the patch, we are having
> > ch datatype as enum and as suggested, with enum as swicth-case, it
> > should not have default case. so I think this initialisation to 0 at the
> > beginning should be fine.
> 
> It will make no sense. Please, remove it. and perhaps the compiler
> won't warn, otherwise the default case will be needed.
>
Understood. Will keep it uninitialized.

> > > Missing include for this macro I believe.
> > >
> > No I guess. DEFINE_RUNTIME_DEV_PM_OPS is part of pm_runtime.h and its
> > already included.
> 
> And how is it related to my comment _here_ in the code?
Pardon my misunderstanding. Please ignore.

> > > > +       },
> > > > +       .probe = tlv493d_probe,
> > > > +       .id_table = tlv493d_id,
> > > > +};
> 
> -- 
> With Best Regards,
> Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Aug 09, 2025 at 08:03:54PM +0530, Dixit Parmar wrote:
> On Sat, Aug 09, 2025 at 02:44:00PM +0200, Andy Shevchenko wrote:

...

> > > > > +       data->wr_regs[TLV493D_WR_REG_MODE1] |= mode1_cfg;
> > > > > +       data->wr_regs[TLV493D_WR_REG_MODE2] |= mode2_cfg;
> > > >
> > > > No mask for the existing values in the respective wr_regs? Wouldn't
> > > > you need to use FIELD_MODIFY() instead?
> > > >
> > > I believe, we are doing OR op with the value created using FIELD_PREP,
> > > so it should not interefere with the existing non-masked values.
> > 
> > I am talking about existing values in the array.
> >
> Right. So in that I think it will make more sense to directly use
> FIELD_MODIFY instead of using FIELD_PREP first and then doing this OR
> op. Right?

Just double check carefully, but sounds about right.

> > > However, as FIELD_MODIFY is there, I should utilize it.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Thu, 07 Aug 2025 08:26:35 +0530
Dixit Parmar <dixitparmar19@gmail.com> 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.
> 
> While the sensor has an interrupt pin multiplexed with an 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
> Link: https://www.mouser.com/pdfDocs/Infineon-TLV493D-A1B6_3DMagnetic-UserManual-v01_03-EN.pdf [1]
> Signed-off-by: Dixit Parmar <dixitparmar19@gmail.com>

Andy did a detailed review, so I only took a quick glance at this version.
One additional thing I noticed though.

> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 500);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	pm_runtime_mark_last_busy(dev);

Drop the mark last busy.  That's now always called in pm_runtime_put_autosuspend()
after a patch that just merged in this merge window.

Seems you got it for the other cases but maybe just missed this call.

> +	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;
> +}
>