[PATCH 0/2] add ti tmag5273 driver

[PATCH 0/2] add ti tmag5273 driver

[Gerald Loacker]

Hi all,

This patch set adds support for the TI TMAG5273 Low-Power Linear 3D Hall-
Effect Sensor. Additionally to temperature and magnetic X, Y and Z-axes the
angle and magnitude are reported. The sensor is operating in continuous
measurement mode and changes to sleep mode if not used for 5 seconds.

Tests were done on a ROCK3 Model A board using the TMAG5273 evaluation
module.

Regards,
Gerald Loacker

Gerald Loacker (2):
  dt-bindings: iio: magnetometer: add ti tmag5273 documentation file
  iio: magnetometer: add ti tmag5273 driver

 .../iio/magnetometer/ti,tmag5273.yaml         |  72 ++
 MAINTAINERS                                   |   7 +
 drivers/iio/magnetometer/Kconfig              |  12 +
 drivers/iio/magnetometer/Makefile             |   2 +
 drivers/iio/magnetometer/tmag5273.c           | 809 ++++++++++++++++++
 5 files changed, 902 insertions(+)
 create mode 100644 .../bindings/iio/magnetometer/ti,tmag5273.yaml
 create mode 100644 drivers/iio/magnetometer/tmag5273.c

-- 
2.37.2

[PATCH 1/2] dt-bindings: iio: magnetometer: add ti tmag5273 documentation file

[Gerald Loacker]

Add bindings documentation file for TI TMAG5273.

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 .../iio/magnetometer/ti,tmag5273.yaml         | 72 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml

diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
new file mode 100644
index 000000000000..2f5b0a4d2f40
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
@@ -0,0 +1,72 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/magnetometer/ti,tmag5273.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor
+
+maintainers:
+  - Gerald Loacker <gerald.loacker@wolfvision.net>
+
+description:
+  The TI TMAG5273 is a low-power linear 3D Hall-effect sensor. This device
+  integrates three independent Hall-effect sensors in the X, Y, and Z axes.
+  The device has an integrated temperature sensor available. The TMAG5273
+  can be configured through the I2C interface to enable any combination of
+  magnetic axes and temperature measurements. An integrated angle calculation
+  engine (CORDIC) provides full 360° angular position information for both
+  on-axis and off-axis angle measurement topologies. The angle calculation is
+  performed using two user-selected magnetic axes.
+
+properties:
+  $nodename:
+    pattern: '^magnetometer@[0-9a-f]+$'
+
+  compatible:
+    const: ti,tmag5273
+
+  reg:
+    maxItems: 1
+
+  "#io-channel-cells":
+    const: 1
+
+  ti,angle-enable:
+    description:
+      Enables angle measurement in the selected plane.
+      0 = OFF
+      1 = X-Y (default)
+      2 = Y-Z
+      3 = X-Z
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 3
+
+  vcc-supply:
+    description:
+      A regulator providing 1.7 V to 3.6 V supply voltage on the VCC pin,
+      typically 3.3 V.
+
+required:
+  - compatible
+  - reg
+  - vcc-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c-0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        magnetometer@35 {
+            compatible = "ti,tmag5273";
+            reg = <0x35>;
+            #io-channel-cells = <1>;
+            ti,angle-enable = <3>;
+            vcc-supply = <&vcc3v3>;
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index cf0f18502372..ea7acec52f8b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20613,6 +20613,12 @@ L:	alsa-devel@alsa-project.org (moderated for non-subscribers)
 S:	Odd Fixes
 F:	sound/soc/codecs/tas571x*
 
+TI TMAG5273 MAGNETOMETER DRIVER
+M:	Gerald Loacker <gerald.loacker@wolfvision.net>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
+
 TI TRF7970A NFC DRIVER
 M:	Mark Greer <mgreer@animalcreek.com>
 L:	linux-wireless@vger.kernel.org
-- 
2.37.2

[PATCH 2/2] iio: magnetometer: add ti tmag5273 driver

[Gerald Loacker]

Add support for TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor.
Additionally to temperature and magnetic X, Y and Z-axes the angle and
magnitude are reported.
The sensor is operating in continuous measurement mode and changes to sleep
mode if not used for 5 seconds.

Datasheet: https://www.ti.com/lit/gpn/tmag5273

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 MAINTAINERS                         |   1 +
 drivers/iio/magnetometer/Kconfig    |  12 +
 drivers/iio/magnetometer/Makefile   |   2 +
 drivers/iio/magnetometer/tmag5273.c | 809 ++++++++++++++++++++++++++++
 4 files changed, 824 insertions(+)
 create mode 100644 drivers/iio/magnetometer/tmag5273.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ea7acec52f8b..9d20b5780051 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20618,6 +20618,7 @@ M:	Gerald Loacker <gerald.loacker@wolfvision.net>
 L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
+F:	drivers/iio/magnetometer/tmag5273.c
 
 TI TRF7970A NFC DRIVER
 M:	Mark Greer <mgreer@animalcreek.com>
diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index b91fc5e6a26e..467819335588 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -208,6 +208,18 @@ config SENSORS_RM3100_SPI
 	  To compile this driver as a module, choose M here: the module
 	  will be called rm3100-spi.
 
+config TI_TMAG5273
+	tristate "TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say Y here to add support for the TI TMAG5273 Low-Power
+	  Linear 3D Hall-Effect 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 tmag5273.
+
 config YAMAHA_YAS530
 	tristate "Yamaha YAS530 family of 3-Axis Magnetometers (I2C)"
 	depends on I2C
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index b9f45b7fafc3..b1c784ea71c8 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -29,4 +29,6 @@ obj-$(CONFIG_SENSORS_RM3100)		+= rm3100-core.o
 obj-$(CONFIG_SENSORS_RM3100_I2C)	+= rm3100-i2c.o
 obj-$(CONFIG_SENSORS_RM3100_SPI)	+= rm3100-spi.o
 
+obj-$(CONFIG_TI_TMAG5273)		+= tmag5273.o
+
 obj-$(CONFIG_YAMAHA_YAS530)		+= yamaha-yas530.o
diff --git a/drivers/iio/magnetometer/tmag5273.c b/drivers/iio/magnetometer/tmag5273.c
new file mode 100644
index 000000000000..549ed1ddba61
--- /dev/null
+++ b/drivers/iio/magnetometer/tmag5273.c
@@ -0,0 +1,809 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for the TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor
+ *
+ * Copyright (C) 2022 WolfVision GmbH
+ *
+ * Author: Gerald Loacker <gerald.loacker@wolfvision.net>
+ */
+
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/pm_runtime.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#include <asm/unaligned.h>
+
+#define TMAG5273_DEVICE_CONFIG_1	 0x00
+#define TMAG5273_DEVICE_CONFIG_2	 0x01
+#define TMAG5273_SENSOR_CONFIG_1	 0x02
+#define TMAG5273_SENSOR_CONFIG_2	 0x03
+#define TMAG5273_X_THR_CONFIG		 0x04
+#define TMAG5273_Y_THR_CONFIG		 0x05
+#define TMAG5273_Z_THR_CONFIG		 0x06
+#define TMAG5273_T_CONFIG		 0x07
+#define TMAG5273_INT_CONFIG_1		 0x08
+#define TMAG5273_MAG_GAIN_CONFIG	 0x09
+#define TMAG5273_MAG_OFFSET_CONFIG_1	 0x0A
+#define TMAG5273_MAG_OFFSET_CONFIG_2	 0x0B
+#define TMAG5273_I2C_ADDRESS		 0x0C
+#define TMAG5273_DEVICE_ID		 0x0D
+#define TMAG5273_MANUFACTURER_ID_LSB	 0x0E
+#define TMAG5273_MANUFACTURER_ID_MSB	 0x0F
+#define TMAG5273_T_MSB_RESULT		 0x10
+#define TMAG5273_T_LSB_RESULT		 0x11
+#define TMAG5273_X_MSB_RESULT		 0x12
+#define TMAG5273_X_LSB_RESULT		 0x13
+#define TMAG5273_Y_MSB_RESULT		 0x14
+#define TMAG5273_Y_LSB_RESULT		 0x15
+#define TMAG5273_Z_MSB_RESULT		 0x16
+#define TMAG5273_Z_LSB_RESULT		 0x17
+#define TMAG5273_CONV_STATUS		 0x18
+#define TMAG5273_ANGLE_RESULT_MSB	 0x19
+#define TMAG5273_ANGLE_RESULT_LSB	 0x1A
+#define TMAG5273_MAGNITUDE_RESULT	 0x1B
+#define TMAG5273_DEVICE_STATUS		 0x1C
+
+#define TMAG5273_MANUFACTURER_ID	 0x5449
+
+#define TMAG5273_AUTOSLEEP_DELAY	 5000
+
+/* Bits in the TMAG5273_DEVICE_CONFIG_1 register */
+#define TMAG5273_AVG_MODE_MASK		 GENMASK(4, 2)
+#define TMAG5273_AVG_1_MODE		 FIELD_PREP(TMAG5273_AVG_MODE_MASK, 0)
+#define TMAG5273_AVG_2_MODE		 FIELD_PREP(TMAG5273_AVG_MODE_MASK, 1)
+#define TMAG5273_AVG_4_MODE		 FIELD_PREP(TMAG5273_AVG_MODE_MASK, 2)
+#define TMAG5273_AVG_8_MODE		 FIELD_PREP(TMAG5273_AVG_MODE_MASK, 3)
+#define TMAG5273_AVG_16_MODE		 FIELD_PREP(TMAG5273_AVG_MODE_MASK, 4)
+#define TMAG5273_AVG_32_MODE		 FIELD_PREP(TMAG5273_AVG_MODE_MASK, 5)
+
+/* bits in the TMAG5273_DEVICE_CONFIG_2 register */
+#define TMAG5273_OP_MODE_MASK		 GENMASK(1, 0)
+#define TMAG5273_OP_MODE_STANDBY	 FIELD_PREP(TMAG5273_OP_MODE_MASK, 0)
+#define TMAG5273_OP_MODE_SLEEP		 FIELD_PREP(TMAG5273_OP_MODE_MASK, 1)
+#define TMAG5273_OP_MODE_CONT		 FIELD_PREP(TMAG5273_OP_MODE_MASK, 2)
+#define TMAG5273_OP_MODE_WAKEUP		 FIELD_PREP(TMAG5273_OP_MODE_MASK, 3)
+
+/* bits in the TMAG5273_SENSOR_CONFIG_1 register */
+#define TMAG5273_MAG_CH_EN_MASK		 GENMASK(7, 4)
+#define TMAG5273_MAG_CH_EN_X_Y_Z	 0x07
+
+/* bits in the TMAG5273_SENSOR_CONFIG_2 register */
+#define TMAG5273_Z_RANGE_MASK		 BIT(0)
+#define TMAG5273_X_Y_RANGE_MASK		 BIT(1)
+#define TMAG5273_ANGLE_EN_MASK		 GENMASK(3, 2)
+#define TMAG5273_ANGLE_EN_X_Y		 1
+#define TMAG5273_ANGLE_EN_Y_Z		 2
+#define TMAG5273_ANGLE_EN_X_Z		 3
+
+/* bits in the TMAG5273_T_CONFIG register */
+#define TMAG5273_T_CH_EN		 BIT(0)
+
+/* bits in the TMAG5273_DEVICE_ID register */
+#define TMAG5273_VERSION_MASK		 GENMASK(1, 0)
+
+/* bits in the TMAG5273_CONV_STATUS register */
+#define TMAG5273_CONV_STATUS_COMPLETE	 BIT(0)
+
+/* state container for the TMAG5273 driver */
+struct tmag5273_data {
+	struct device *dev;
+	unsigned int devid;
+	unsigned int version;
+	char name[16];
+	int conv_avg;
+	int max_avg;
+	int range;
+	u32 angle_en;
+	struct regmap *map;
+	struct regulator *vcc;
+	/* Locks the sensor for exclusive use during a measurement (which
+	 * involves several register transactions so the regmap lock is not
+	 * enough) so that measurements get serialized in a first-come-first-
+	 * serve manner.
+	 */
+	struct mutex lock;
+};
+
+static const struct {
+	int avg;
+	u8 reg_val;
+} tmag5273_avg_table[] = {
+	{ 1, 0x00 }, { 2, 0x01 },  { 4, 0x02 },
+	{ 8, 0x03 }, { 16, 0x04 }, { 32, 0x05 },
+};
+
+/*
+ * magnetic range in mT for different TMAG5273 versions
+ * only version 1 and 2 are valid, version 0 and 3 are reserved
+ */
+static const struct {
+	int range;
+	u8 reg_val;
+} tmag5273_range_table[4][2] = {
+	{ { 0, 0 }, { 0, 3 } },
+	{ { 40, 0 }, { 80, 3 } },
+	{ { 133, 0 }, { 266, 3 } },
+	{ { 0, 0 }, { 0, 3 } },
+};
+
+/*
+ * tmag5273_measure() - Make a measure from the hardware
+ * @tmag5273: The device state
+ * @t: the processed temperature measurement
+ * @x: the raw x axis measurement
+ * @y: the raw x axis measurement
+ * @z: the raw x axis measurement
+ * @angle: the calculated angle
+ * @magnitude: the calculated magnitude
+ * @return: 0 on success or error code
+ */
+static int tmag5273_measure(struct tmag5273_data *data, u16 *t, u16 *x, u16 *y,
+			    u16 *z, u16 *angle, u16 *magnitude)
+{
+	unsigned int status;
+	u8 reg_data[12];
+	int ret;
+	u16 val;
+
+	mutex_lock(&data->lock);
+	ret = regmap_read(data->map, TMAG5273_CONV_STATUS, &status);
+	if (ret < 0)
+		goto out_unlock;
+
+	/*
+	 * Conversion time is 2425 us in 32x averaging mode for all three
+	 * channels. Since we are in continuous measurement mode, a measurement
+	 * may already be there, so poll for completed measurement with
+	 * timeout.
+	 */
+	ret = regmap_read_poll_timeout(data->map, TMAG5273_CONV_STATUS, status,
+				       status & TMAG5273_CONV_STATUS_COMPLETE,
+				       100, 10000);
+	if (ret) {
+		dev_err_probe(data->dev, ret, "timeout waiting for measurement\n");
+		goto out_unlock;
+	}
+
+	ret = regmap_bulk_read(data->map, TMAG5273_T_MSB_RESULT, reg_data,
+			       sizeof(reg_data));
+	if (ret)
+		goto out_unlock;
+
+	ret = regmap_read(data->map, TMAG5273_CONV_STATUS, &status);
+	if (ret < 0)
+		goto out_unlock;
+
+	mutex_unlock(&data->lock);
+
+	*t = get_unaligned_be16(&reg_data[0]);
+	*x = get_unaligned_be16(&reg_data[2]);
+	*y = get_unaligned_be16(&reg_data[4]);
+	*z = get_unaligned_be16(&reg_data[6]);
+	/*
+	 * angle has 9 bits integer value and 4 bits fractional part
+	 * 15 14 13 12 11 10 9  8  7  6  5  4  3  2  1  0
+	 * 0  0  0  a  a  a  a  a  a  a  a  a  f  f  f  f
+	 */
+	val = get_unaligned_be16(&reg_data[9]);
+	*angle = FIELD_GET(GENMASK(12, 0), val);
+	*magnitude = reg_data[11];
+	return ret;
+
+out_unlock:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+/*
+ * tmag5273_get_measure() - Measure a sample of all axis and process
+ * @tmag5273: The device state
+ * @to: Temperature out
+ * @xo: X axis out
+ * @yo: Y axis out
+ * @zo: Z axis out
+ * @ao: Angle out
+ * @mo: Magnitude out
+ * @return: 0 on success or error code
+ */
+static int tmag5273_get_measure(struct tmag5273_data *data, s32 *to, s32 *xo,
+				s32 *yo, s32 *zo, u16 *ao, u16 *mo)
+{
+	u16 t, x, y, z, angle, magnitude;
+	int ret;
+
+	ret = tmag5273_measure(data, &t, &x, &y, &z, &angle, &magnitude);
+	if (ret)
+		return ret;
+
+	/*
+	 * convert device specific value to millicelsius
+	 * use multiply by 16639 and divide by 10000 to achieve divide by 60.1
+	 *   and convert to millicelsius
+	 */
+	*to = (((s32)t - 17508) * 16639) / 1000 + 25000;
+	*xo = (s16)x;
+	*yo = (s16)y;
+	*zo = (s16)z;
+	*ao = angle;
+	*mo = magnitude;
+	return 0;
+}
+
+static int tmag5273_read_raw(struct iio_dev *indio_dev,
+			     const struct iio_chan_spec *chan, int *val,
+			     int *val2, long mask)
+{
+	struct tmag5273_data *data = iio_priv(indio_dev);
+	s32 t, x, y, z;
+	u16 angle, magnitude;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+	case IIO_CHAN_INFO_RAW:
+		ret = pm_runtime_resume_and_get(data->dev);
+		if (ret < 0)
+			return ret;
+		ret = tmag5273_get_measure(data, &t, &x, &y, &z, &angle,
+					   &magnitude);
+		if (ret)
+			return ret;
+		pm_runtime_mark_last_busy(data->dev);
+		pm_runtime_put_autosuspend(data->dev);
+		switch (chan->scan_index) {
+		case 0:
+			*val = t;
+			break;
+		case 1:
+			*val = x;
+			break;
+		case 2:
+			*val = y;
+			break;
+		case 3:
+			*val = z;
+			break;
+		case 4:
+			*val = angle;
+			break;
+		case 5:
+			*val = magnitude;
+			break;
+		default:
+			return dev_err_probe(data->dev, -EINVAL, "unknown channel\n");
+		}
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_TEMP:
+			/* 1 LSB = 1 millidegree Celsius */
+			*val = 1;
+			return IIO_VAL_INT;
+		case IIO_MAGN:
+			/*
+			 * The axis values are in stored in 2^15 / range LSB/mT.
+			 * Since 1 mT = 10 Gauss, we need to multiply by 10 and
+			 * divide by [range] to get Gauss from the raw value.
+			 */
+			*val = data->range * 10;
+			*val2 = 32768;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_ANGL:
+			/*
+			 * Angle is in degrees and has four fractional bits,
+			 * therefore use 1/16 * pi/180 to convert to radiants.
+			 */
+			*val = 1000;
+			*val2 = 916732;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_DISTANCE:
+			/* Magnitude is unscaled */
+			*val = 1;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		/* Unknown request */
+		return -EINVAL;
+	}
+}
+
+static ssize_t tmag5273_show_conv_avg(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct tmag5273_data *data = iio_priv(indio_dev);
+
+	return sprintf(buf, "%d\n", data->conv_avg);
+}
+
+static ssize_t tmag5273_write_conv_avg(struct tmag5273_data *data,
+				       unsigned int val)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tmag5273_avg_table); i++) {
+		if (tmag5273_avg_table[i].avg == val)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(tmag5273_avg_table))
+		return -EINVAL;
+
+	ret = regmap_update_bits(data->map, TMAG5273_DEVICE_CONFIG_1,
+				 TMAG5273_AVG_MODE_MASK,
+				 tmag5273_avg_table[i].reg_val);
+	if (ret)
+		return ret;
+
+	data->conv_avg = val;
+
+	return 0;
+}
+
+static ssize_t tmag5273_store_conv_avg(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct tmag5273_data *data = iio_priv(indio_dev);
+	int ret;
+	int integer;
+
+	ret = kstrtoint(buf, 0, &integer);
+	if (ret)
+		return ret;
+
+	ret = tmag5273_write_conv_avg(data, integer);
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(conv_avg, 0644, tmag5273_show_conv_avg,
+		       tmag5273_store_conv_avg, 0);
+
+static ssize_t tmag5273_conv_avg_available(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct tmag5273_data *data = iio_priv(indio_dev);
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tmag5273_avg_table); i++) {
+		if (tmag5273_avg_table[i].avg > data->max_avg)
+			break;
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d ",
+				 tmag5273_avg_table[i].avg);
+	}
+	/* replace last space with a newline */
+	if (len > 0)
+		buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(conv_avg_available, 0444, tmag5273_conv_avg_available,
+		       NULL, 0);
+
+static ssize_t tmag5273_show_range(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct tmag5273_data *data = iio_priv(indio_dev);
+
+	return sprintf(buf, "%d\n", data->range);
+}
+
+static ssize_t tmag5273_write_range(struct tmag5273_data *data,
+				    unsigned int val)
+{
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(tmag5273_range_table[0]); i++) {
+		if (tmag5273_range_table[data->version][i].range == val)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(tmag5273_range_table[0]))
+		return -EINVAL;
+
+	ret = regmap_update_bits(data->map,
+				 TMAG5273_SENSOR_CONFIG_2,
+				 TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK,
+				 tmag5273_range_table[data->version][i].reg_val);
+	if (ret)
+		return ret;
+
+	data->range = val;
+
+	return 0;
+}
+
+static ssize_t tmag5273_store_range(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct tmag5273_data *data = iio_priv(indio_dev);
+	int range, ret;
+
+	ret = kstrtoint(buf, 0, &range);
+	if (ret)
+		return ret;
+
+	ret = tmag5273_write_range(data, range);
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(range, 0644, tmag5273_show_range,
+		       tmag5273_store_range, 0);
+
+static ssize_t tmag5273_range_available(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct tmag5273_data *data = iio_priv(indio_dev);
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tmag5273_range_table[0]); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d ",
+				 tmag5273_range_table[data->version][i].range);
+	}
+	/* replace last space with a newline */
+	if (len > 0)
+		buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(range_available, 0444, tmag5273_range_available, NULL,
+		       0);
+
+static struct attribute *tmag5273_attributes[] = {
+	&iio_dev_attr_conv_avg.dev_attr.attr,
+	&iio_dev_attr_conv_avg_available.dev_attr.attr,
+	&iio_dev_attr_range.dev_attr.attr,
+	&iio_dev_attr_range_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group tmag5273_attrs_group = {
+	.attrs = tmag5273_attributes,
+};
+
+#define TMAG5273_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),    \
+		.scan_index = index,                               \
+		.scan_type = {                                     \
+			.sign = 's',                               \
+			.realbits = 32,                            \
+			.storagebits = 32,                         \
+			.endianness = IIO_CPU,                     \
+		},                                                 \
+	}
+
+static const struct iio_chan_spec tmag5273_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
+	},
+	TMAG5273_AXIS_CHANNEL(X, 1),
+	TMAG5273_AXIS_CHANNEL(Y, 2),
+	TMAG5273_AXIS_CHANNEL(Z, 3),
+	{
+		.type = IIO_ANGL,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = 4,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_DISTANCE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.scan_index = 5,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(6),
+};
+
+static const struct iio_info tmag5273_info = {
+	.attrs = &tmag5273_attrs_group,
+	.read_raw = &tmag5273_read_raw,
+};
+
+static bool tmag5273_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return (reg >= TMAG5273_T_MSB_RESULT &&
+		reg <= TMAG5273_MAGNITUDE_RESULT);
+}
+
+static const struct regmap_config tmag5273_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0xff,
+	.volatile_reg = tmag5273_volatile_reg,
+};
+
+static int tmag5273_set_operating_mode(struct tmag5273_data *data,
+				       unsigned int val)
+{
+	return regmap_write(data->map, TMAG5273_DEVICE_CONFIG_2, val);
+}
+
+static int tmag5273_chip_init(struct tmag5273_data *data)
+{
+	int ret;
+
+	ret = regmap_write(data->map, TMAG5273_DEVICE_CONFIG_1,
+			   TMAG5273_AVG_32_MODE);
+	if (ret)
+		return ret;
+	data->conv_avg = 32;
+	ret = regmap_write(data->map, TMAG5273_DEVICE_CONFIG_2,
+			   TMAG5273_OP_MODE_CONT);
+	if (ret)
+		return ret;
+	ret = regmap_write(data->map, TMAG5273_SENSOR_CONFIG_1,
+			   FIELD_PREP(TMAG5273_MAG_CH_EN_MASK,
+				      TMAG5273_MAG_CH_EN_X_Y_Z));
+	if (ret)
+		return ret;
+	ret = regmap_write(data->map, TMAG5273_SENSOR_CONFIG_2,
+			   FIELD_PREP(TMAG5273_ANGLE_EN_MASK, data->angle_en));
+	if (ret)
+		return ret;
+
+	data->range = tmag5273_range_table[data->version][0].range;
+	return regmap_write(data->map, TMAG5273_T_CONFIG, TMAG5273_T_CH_EN);
+}
+
+static int tmag5273_probe(struct i2c_client *i2c,
+			  const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct device *dev = &i2c->dev;
+	struct device_node *node = dev->of_node;
+	struct tmag5273_data *data;
+	int val, ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(i2c, indio_dev);
+	data->dev = dev;
+	mutex_init(&data->lock);
+
+	data->vcc = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(data->vcc))
+		return dev_err_probe(dev, PTR_ERR(data->vcc),
+				     "failed to get VCC regulator\n");
+
+	/* Operating voltage 1.7V .. 3.6V according to datasheet */
+	ret = regulator_set_voltage(data->vcc, 1700000, 3600000);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to set VCC voltage\n");
+
+	ret = regulator_enable(data->vcc);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to enable VCC regulator\n");
+
+	/*
+	 * Regulators have to ramp up with 3V/ms, additional time to go to
+	 * stand-by mode is 270us typically. We give 1 ms to 2 ms time.
+	 */
+	usleep_range(1000, 2000);
+
+	data->map = devm_regmap_init_i2c(i2c, &tmag5273_regmap_config);
+	if (IS_ERR(data->map)) {
+		ret = PTR_ERR(data->map);
+		dev_err_probe(dev, ret, "failed to allocate register map\n");
+		goto out_disable_vcc;
+	}
+
+	ret = regmap_read(data->map, TMAG5273_DEVICE_ID, &val);
+	if (ret) {
+		/*
+		 * If we come from sleep with power already activated, the
+		 * first I2C command wakes up the chip but will fail.
+		 * Time to go to stand-by mode from sleep mode is 50us
+		 * typically. During this time no I2C access is possible.
+		 */
+		usleep_range(80, 200);
+
+		ret = regmap_read(data->map, TMAG5273_DEVICE_ID, &val);
+		if (ret)
+			goto out_disable_vcc;
+	}
+	data->version = FIELD_PREP(TMAG5273_VERSION_MASK, val);
+
+	ret = regmap_bulk_read(data->map, TMAG5273_MANUFACTURER_ID_LSB,
+			       &data->devid, 2);
+	if (ret)
+		goto out_disable_vcc;
+
+	switch (data->devid) {
+	case TMAG5273_MANUFACTURER_ID:
+		strncpy(data->name, "TMAG5273", sizeof(data->name) - 2);
+		switch (data->version) {
+		case 1:
+			strncat(data->name, "x1", 2);
+			break;
+		case 2:
+			strncat(data->name, "x2", 2);
+			break;
+		default:
+			break;
+		}
+		dev_info(dev, "%s", data->name);
+		data->max_avg = 32;
+		break;
+	default:
+		ret = -ENODEV;
+		dev_err_probe(dev, ret, "unhandled device ID 0x%x\n", data->devid);
+		goto out_disable_vcc;
+	}
+
+	/*
+	 * Angle-enable is optional and set to 1 (enable X-Y plane) by default,
+	 * the value is modified only if a valid u32 value can be decoded.
+	 */
+	data->angle_en = TMAG5273_ANGLE_EN_X_Y;
+	of_property_read_u32(node, "tmag5273,angle-enable", &data->angle_en);
+
+	ret = tmag5273_chip_init(data);
+	if (ret)
+		goto out_disable_vcc;
+
+	indio_dev->info = &tmag5273_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->name = data->name;
+	indio_dev->channels = tmag5273_channels;
+	indio_dev->num_channels = ARRAY_SIZE(tmag5273_channels);
+
+	pm_runtime_set_autosuspend_delay(dev, TMAG5273_AUTOSLEEP_DELAY);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_mark_last_busy(dev);
+	ret = pm_runtime_set_active(dev);
+	if (ret < 0)
+		goto out_disable_vcc;
+	pm_runtime_enable(dev);
+	pm_runtime_idle(dev);
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err_probe(dev, ret, "device register failed\n");
+		goto cleanup_runtime;
+	}
+
+	return 0;
+
+cleanup_runtime:
+	pm_runtime_dont_use_autosuspend(dev);
+	pm_runtime_disable(dev);
+out_disable_vcc:
+	tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_SLEEP);
+	regulator_disable(data->vcc);
+	return ret;
+}
+
+static void tmag5273_remove(struct i2c_client *i2c)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
+	struct tmag5273_data *data = iio_priv(indio_dev);
+	struct device *dev = &i2c->dev;
+
+	iio_device_unregister(indio_dev);
+
+	pm_runtime_dont_use_autosuspend(dev);
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+
+	tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_SLEEP);
+	regulator_disable(data->vcc);
+}
+
+static int tmag5273_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct tmag5273_data *data = iio_priv(indio_dev);
+
+	tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_SLEEP);
+
+	return 0;
+}
+
+static int tmag5273_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct tmag5273_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_CONT);
+	if (ret) {
+		/*
+		 * Time to go to stand-by mode from sleep mode is 50us
+		 * typically. During this time no I2C access is possible.
+		 */
+		usleep_range(80, 200);
+		ret = tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_CONT);
+	}
+
+	return ret;
+}
+
+static const struct dev_pm_ops tmag5273_pm_ops = {
+	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
+	RUNTIME_PM_OPS(tmag5273_runtime_suspend, tmag5273_runtime_resume, NULL)
+};
+
+static const struct i2c_device_id tmag5273_id[] = {
+	{
+		"tmag5273",
+	},
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(i2c, tmag5273_id);
+
+static const struct of_device_id tmag5273_of_match[] = {
+	{
+		.compatible = "ti,tmag5273",
+	},
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, tmag5273_of_match);
+
+static struct i2c_driver tmag5273_driver = {
+	.driver	 = {
+		.name	= "tmag5273",
+		.of_match_table = tmag5273_of_match,
+		.pm = pm_ptr(&tmag5273_pm_ops),
+	},
+	.probe	  = tmag5273_probe,
+	.remove	  = tmag5273_remove,
+	.id_table = tmag5273_id,
+};
+module_i2c_driver(tmag5273_driver);
+
+MODULE_DESCRIPTION("TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor driver");
+MODULE_AUTHOR("Gerald Loacker <gerald.loacker@wolfvision.net>");
+MODULE_LICENSE("GPL");
-- 
2.37.2

Re: [PATCH 1/2] dt-bindings: iio: magnetometer: add ti tmag5273 documentation file

[Krzysztof Kozlowski]

On 15/11/2022 08:37, Gerald Loacker wrote:
> Add bindings documentation file for TI TMAG5273.

Subject - drop "documentation file".

> 
> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> ---
>  .../iio/magnetometer/ti,tmag5273.yaml         | 72 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
> new file mode 100644
> index 000000000000..2f5b0a4d2f40
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/magnetometer/ti,tmag5273.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor
> +
> +maintainers:
> +  - Gerald Loacker <gerald.loacker@wolfvision.net>
> +
> +description:
> +  The TI TMAG5273 is a low-power linear 3D Hall-effect sensor. This device
> +  integrates three independent Hall-effect sensors in the X, Y, and Z axes.
> +  The device has an integrated temperature sensor available. The TMAG5273
> +  can be configured through the I2C interface to enable any combination of
> +  magnetic axes and temperature measurements. An integrated angle calculation
> +  engine (CORDIC) provides full 360° angular position information for both
> +  on-axis and off-axis angle measurement topologies. The angle calculation is
> +  performed using two user-selected magnetic axes.
> +
> +properties:
> +  $nodename:
> +    pattern: '^magnetometer@[0-9a-f]+$'

Device schemas do not need to enforce the names.

> +
> +  compatible:
> +    const: ti,tmag5273
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +  ti,angle-enable:

ti,angle-measurement

> +    description:
> +      Enables angle measurement in the selected plane.
> +      0 = OFF
> +      1 = X-Y (default)
> +      2 = Y-Z
> +      3 = X-Z

Why not strings which are easier for humans? off/x-y/y-z/x-z? How anyone
reading DTS can remember what is "3" in this and in thousands of other
devices?

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 3

default: 1

> +
> +  vcc-supply:
> +    description:
> +      A regulator providing 1.7 V to 3.6 V supply voltage on the VCC pin,
> +      typically 3.3 V.
> +
> +required:
> +  - compatible
> +  - reg
> +  - vcc-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c-0 {

Just: i2c


Best regards,
Krzysztof

Re: [PATCH 2/2] iio: magnetometer: add ti tmag5273 driver

[Andy Shevchenko]

On Tue, Nov 15, 2022 at 08:37:18AM +0100, Gerald Loacker wrote:
> Add support for TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor.
> Additionally to temperature and magnetic X, Y and Z-axes the angle and
> magnitude are reported.
> The sensor is operating in continuous measurement mode and changes to sleep
> mode if not used for 5 seconds.
> 
> Datasheet: https://www.ti.com/lit/gpn/tmag5273

> 

Drop this blank line to make above to be a tag.

> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>

...

> +#define TMAG5273_MANUFACTURER_ID        0x5449

Can you add ASCII comment on that magic value?

...

> +#define TMAG5273_AUTOSLEEP_DELAY	 5000

Units?

> +struct tmag5273_data {
> +	struct device *dev;
> +	unsigned int devid;
> +	unsigned int version;

> +	char name[16];
> +	int conv_avg;
> +	int max_avg;
> +	int range;
> +	u32 angle_en;
> +	struct regmap *map;
> +	struct regulator *vcc;

+ Blank line

> +	/* Locks the sensor for exclusive use during a measurement (which
> +	 * involves several register transactions so the regmap lock is not
> +	 * enough) so that measurements get serialized in a first-come-first-
> +	 * serve manner.
> +	 */

/*
 * Wrong multi-line
 * comment style. Fix
 * it accordingly.
 */

> +	struct mutex lock;
> +};

...

> +static const struct {
> +	int avg;
> +	u8 reg_val;
> +} tmag5273_avg_table[] = {
> +	{ 1, 0x00 }, { 2, 0x01 },  { 4, 0x02 },
> +	{ 8, 0x03 }, { 16, 0x04 }, { 32, 0x05 },

Isn't the second one is just an index?

> +};


...

> +/*
> + * magnetic range in mT for different TMAG5273 versions

Magnetic

> + * only version 1 and 2 are valid, version 0 and 3 are reserved
> + */
> +static const struct {
> +	int range;
> +	u8 reg_val;
> +} tmag5273_range_table[4][2] = {

I believe you can drop 4.

> +	{ { 0, 0 }, { 0, 3 } },
> +	{ { 40, 0 }, { 80, 3 } },
> +	{ { 133, 0 }, { 266, 3 } },
> +	{ { 0, 0 }, { 0, 3 } },
> +};

...

> +/*

Shouldn't it be a kernel doc? Ditto for the rest.

> + * tmag5273_measure() - Make a measure from the hardware
> + * @tmag5273: The device state
> + * @t: the processed temperature measurement
> + * @x: the raw x axis measurement
> + * @y: the raw x axis measurement
> + * @z: the raw x axis measurement
> + * @angle: the calculated angle
> + * @magnitude: the calculated magnitude
> + * @return: 0 on success or error code
> + */

...

> +	/*
> +	 * convert device specific value to millicelsius

Convert

> +	 * use multiply by 16639 and divide by 10000 to achieve divide by 60.1
> +	 *   and convert to millicelsius

One space is enough and missing period.

> +	 */

> +}

...

> +static ssize_t tmag5273_show_conv_avg(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct tmag5273_data *data = iio_priv(indio_dev);
> +
> +	return sprintf(buf, "%d\n", data->conv_avg);

Must be sysfs_emit().

> +}

...

> +	for (i = 0; i < ARRAY_SIZE(tmag5273_avg_table); i++) {
> +		if (tmag5273_avg_table[i].avg == val)
> +			break;
> +	}

> +

Redundant blank line.

> +	if (i == ARRAY_SIZE(tmag5273_avg_table))
> +		return -EINVAL;

...

> +static IIO_DEVICE_ATTR(conv_avg, 0644, tmag5273_show_conv_avg,
> +		       tmag5273_store_conv_avg, 0);

IIO_DEVICE_ATTR_RW() ?

...

> +	for (i = 0; i < ARRAY_SIZE(tmag5273_avg_table); i++) {
> +		if (tmag5273_avg_table[i].avg > data->max_avg)
> +			break;
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d ",
> +				 tmag5273_avg_table[i].avg);

sysfs_emit_at()

> +	}
> +	/* replace last space with a newline */
> +	if (len > 0)
> +		buf[len - 1] = '\n';

...

> +static IIO_DEVICE_ATTR(conv_avg_available, 0444, tmag5273_conv_avg_available,
> +		       NULL, 0);

IIO_DEVICE_ATTR_RO()

...

> +	return sprintf(buf, "%d\n", data->range);

sysfs_emit().

...

> +	for (i = 0; i < ARRAY_SIZE(tmag5273_range_table[0]); i++) {
> +		if (tmag5273_range_table[data->version][i].range == val)
> +			break;
> +	}

> +

Redundant blank line.

> +	if (i == ARRAY_SIZE(tmag5273_range_table[0]))
> +		return -EINVAL;

...

> +static ssize_t tmag5273_store_range(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct tmag5273_data *data = iio_priv(indio_dev);
> +	int range, ret;
> +
> +	ret = kstrtoint(buf, 0, &range);

Here and in the other ->store() do you really have negative values possible?
Can you revisit all those kstrtoX() calls and check the arguments, so you can
narrow down the range of the input where it's appropriate?

> +	if (ret)
> +		return ret;
> +
> +	ret = tmag5273_write_range(data, range);
> +	if (ret < 0)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(range, 0644, tmag5273_show_range,
> +		       tmag5273_store_range, 0);

IIO_DEVICE_ATTR_RW()

> +	for (i = 0; i < ARRAY_SIZE(tmag5273_range_table[0]); i++) {
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d ",
> +				 tmag5273_range_table[data->version][i].range);
> +	}

sysfs_emit();

> +	/* replace last space with a newline */
> +	if (len > 0)
> +		buf[len - 1] = '\n';
> +
> +	return len;
> +}

...

> +static IIO_DEVICE_ATTR(range_available, 0444, tmag5273_range_available, NULL,
> +		       0);

One line.

...

> +static struct attribute *tmag5273_attributes[] = {
> +	&iio_dev_attr_conv_avg.dev_attr.attr,
> +	&iio_dev_attr_conv_avg_available.dev_attr.attr,
> +	&iio_dev_attr_range.dev_attr.attr,
> +	&iio_dev_attr_range_available.dev_attr.attr,

> +	NULL,

No comma in terminator entry.

> +};

> +static const struct attribute_group tmag5273_attrs_group = {
> +	.attrs = tmag5273_attributes,
> +};

...

> +static bool tmag5273_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	return (reg >= TMAG5273_T_MSB_RESULT &&
> +		reg <= TMAG5273_MAGNITUDE_RESULT);

Drop parentheses and make it one line.

> +}

...

> +static int tmag5273_probe(struct i2c_client *i2c,
> +			  const struct i2c_device_id *id)

Why ->probe_new() can't be utilized?

...

> +	struct device_node *node = dev->of_node;

What for? Use device property API (see below).

...

> +	data->map = devm_regmap_init_i2c(i2c, &tmag5273_regmap_config);
> +	if (IS_ERR(data->map)) {

> +		ret = PTR_ERR(data->map);
> +		dev_err_probe(dev, ret, "failed to allocate register map\n");

	ret = dev_err_probe();

But don't you have an ordering issue here?

> +		goto out_disable_vcc;
> +	}

...

> +		strncpy(data->name, "TMAG5273", sizeof(data->name) - 2);
> +		switch (data->version) {
> +		case 1:
> +			strncat(data->name, "x1", 2);
> +			break;
> +		case 2:
> +			strncat(data->name, "x2", 2);
> +			break;
> +		default:
> +			break;
> +		}

This all can be replaced with fewer lines:

		if (data->version < 1 || data->version > 2)
			snprintf(data->name, "TMAG5273", sizeof(data->name));
		else
			snprintf(data->name, "TMAG5273x%.1u", sizeof(data->name), data->version);

...

> +		dev_info(dev, "%s", data->name);

Useless?

...

> +	of_property_read_u32(node, "tmag5273,angle-enable", &data->angle_en);

Missing header for that, but the question is why? What's wrong with device
property API instead?

...

> +static const struct i2c_device_id tmag5273_id[] = {
> +	{
> +		"tmag5273",
> +	},

Can be one line.

> +	{ /* sentinel */ },

No comma for terminator entry.

> +};

...

> +	{ /* sentinel */ },

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: magnetometer: add ti tmag5273 driver

[Jonathan Cameron]

On Tue, 15 Nov 2022 08:37:18 +0100
Gerald Loacker <gerald.loacker@wolfvision.net> wrote:

> Add support for TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor.
> Additionally to temperature and magnetic X, Y and Z-axes the angle and
> magnitude are reported.
> The sensor is operating in continuous measurement mode and changes to sleep
> mode if not used for 5 seconds.
> 
> Datasheet: https://www.ti.com/lit/gpn/tmag5273
> 
> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
Hi Gerald,

Some comments inline.

Jonathan


> diff --git a/drivers/iio/magnetometer/tmag5273.c b/drivers/iio/magnetometer/tmag5273.c
> new file mode 100644
> index 000000000000..549ed1ddba61
> --- /dev/null
> +++ b/drivers/iio/magnetometer/tmag5273.c
> @@ -0,0 +1,809 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for the TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor
> + *
> + * Copyright (C) 2022 WolfVision GmbH
> + *
> + * Author: Gerald Loacker <gerald.loacker@wolfvision.net>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#include <asm/unaligned.h>
> +
> +#define TMAG5273_DEVICE_CONFIG_1	 0x00
> +#define TMAG5273_DEVICE_CONFIG_2	 0x01
> +#define TMAG5273_SENSOR_CONFIG_1	 0x02
> +#define TMAG5273_SENSOR_CONFIG_2	 0x03
> +#define TMAG5273_X_THR_CONFIG		 0x04
> +#define TMAG5273_Y_THR_CONFIG		 0x05
> +#define TMAG5273_Z_THR_CONFIG		 0x06
> +#define TMAG5273_T_CONFIG		 0x07
> +#define TMAG5273_INT_CONFIG_1		 0x08
> +#define TMAG5273_MAG_GAIN_CONFIG	 0x09
> +#define TMAG5273_MAG_OFFSET_CONFIG_1	 0x0A
> +#define TMAG5273_MAG_OFFSET_CONFIG_2	 0x0B
> +#define TMAG5273_I2C_ADDRESS		 0x0C
> +#define TMAG5273_DEVICE_ID		 0x0D
> +#define TMAG5273_MANUFACTURER_ID_LSB	 0x0E
> +#define TMAG5273_MANUFACTURER_ID_MSB	 0x0F
> +#define TMAG5273_T_MSB_RESULT		 0x10
> +#define TMAG5273_T_LSB_RESULT		 0x11
> +#define TMAG5273_X_MSB_RESULT		 0x12
> +#define TMAG5273_X_LSB_RESULT		 0x13
> +#define TMAG5273_Y_MSB_RESULT		 0x14
> +#define TMAG5273_Y_LSB_RESULT		 0x15
> +#define TMAG5273_Z_MSB_RESULT		 0x16
> +#define TMAG5273_Z_LSB_RESULT		 0x17
> +#define TMAG5273_CONV_STATUS		 0x18
> +#define TMAG5273_ANGLE_RESULT_MSB	 0x19
> +#define TMAG5273_ANGLE_RESULT_LSB	 0x1A
> +#define TMAG5273_MAGNITUDE_RESULT	 0x1B
> +#define TMAG5273_DEVICE_STATUS		 0x1C
> +
> +#define TMAG5273_MANUFACTURER_ID	 0x5449
> +
> +#define TMAG5273_AUTOSLEEP_DELAY	 5000
> +
> +/* Bits in the TMAG5273_DEVICE_CONFIG_1 register */
> +#define TMAG5273_AVG_MODE_MASK		 GENMASK(4, 2)
> +#define TMAG5273_AVG_1_MODE		 FIELD_PREP(TMAG5273_AVG_MODE_MASK, 0)
> +#define TMAG5273_AVG_2_MODE		 FIELD_PREP(TMAG5273_AVG_MODE_MASK, 1)
> +#define TMAG5273_AVG_4_MODE		 FIELD_PREP(TMAG5273_AVG_MODE_MASK, 2)
> +#define TMAG5273_AVG_8_MODE		 FIELD_PREP(TMAG5273_AVG_MODE_MASK, 3)
> +#define TMAG5273_AVG_16_MODE		 FIELD_PREP(TMAG5273_AVG_MODE_MASK, 4)
> +#define TMAG5273_AVG_32_MODE		 FIELD_PREP(TMAG5273_AVG_MODE_MASK, 5)
> +
> +/* bits in the TMAG5273_DEVICE_CONFIG_2 register */
> +#define TMAG5273_OP_MODE_MASK		 GENMASK(1, 0)
> +#define TMAG5273_OP_MODE_STANDBY	 FIELD_PREP(TMAG5273_OP_MODE_MASK, 0)
> +#define TMAG5273_OP_MODE_SLEEP		 FIELD_PREP(TMAG5273_OP_MODE_MASK, 1)
> +#define TMAG5273_OP_MODE_CONT		 FIELD_PREP(TMAG5273_OP_MODE_MASK, 2)
> +#define TMAG5273_OP_MODE_WAKEUP		 FIELD_PREP(TMAG5273_OP_MODE_MASK, 3)
> +
> +/* bits in the TMAG5273_SENSOR_CONFIG_1 register */
> +#define TMAG5273_MAG_CH_EN_MASK		 GENMASK(7, 4)
> +#define TMAG5273_MAG_CH_EN_X_Y_Z	 0x07
> +
> +/* bits in the TMAG5273_SENSOR_CONFIG_2 register */
> +#define TMAG5273_Z_RANGE_MASK		 BIT(0)
> +#define TMAG5273_X_Y_RANGE_MASK		 BIT(1)
> +#define TMAG5273_ANGLE_EN_MASK		 GENMASK(3, 2)
> +#define TMAG5273_ANGLE_EN_X_Y		 1
> +#define TMAG5273_ANGLE_EN_Y_Z		 2
> +#define TMAG5273_ANGLE_EN_X_Z		 3
> +
> +/* bits in the TMAG5273_T_CONFIG register */
> +#define TMAG5273_T_CH_EN		 BIT(0)
> +
> +/* bits in the TMAG5273_DEVICE_ID register */
> +#define TMAG5273_VERSION_MASK		 GENMASK(1, 0)
> +
> +/* bits in the TMAG5273_CONV_STATUS register */
> +#define TMAG5273_CONV_STATUS_COMPLETE	 BIT(0)
> +
> +/* state container for the TMAG5273 driver */
> +struct tmag5273_data {
> +	struct device *dev;
> +	unsigned int devid;
> +	unsigned int version;
> +	char name[16];
> +	int conv_avg;
> +	int max_avg;
> +	int range;
> +	u32 angle_en;
> +	struct regmap *map;
> +	struct regulator *vcc;
> +	/* Locks the sensor for exclusive use during a measurement (which
> +	 * involves several register transactions so the regmap lock is not
> +	 * enough) so that measurements get serialized in a first-come-first-
> +	 * serve manner.
> +	 */
> +	struct mutex lock;
> +};
> +
> +static const struct {
> +	int avg;
> +	u8 reg_val;
> +} tmag5273_avg_table[] = {
> +	{ 1, 0x00 }, { 2, 0x01 },  { 4, 0x02 },
> +	{ 8, 0x03 }, { 16, 0x04 }, { 32, 0x05 },
> +};
> +
> +/*
> + * magnetic range in mT for different TMAG5273 versions
> + * only version 1 and 2 are valid, version 0 and 3 are reserved
> + */
> +static const struct {
> +	int range;
> +	u8 reg_val;
> +} tmag5273_range_table[4][2] = {
> +	{ { 0, 0 }, { 0, 3 } },
> +	{ { 40, 0 }, { 80, 3 } },
> +	{ { 133, 0 }, { 266, 3 } },
> +	{ { 0, 0 }, { 0, 3 } },
> +};
> +
> +/*
/**

It's kernel doc style, so mark it as such, but also run the kernel doc
script over it and fix any warnings.

> + * tmag5273_measure() - Make a measure from the hardware
> + * @tmag5273: The device state
> + * @t: the processed temperature measurement
> + * @x: the raw x axis measurement
> + * @y: the raw x axis measurement
> + * @z: the raw x axis measurement
> + * @angle: the calculated angle
> + * @magnitude: the calculated magnitude
> + * @return: 0 on success or error code
> + */
> +static int tmag5273_measure(struct tmag5273_data *data, u16 *t, u16 *x, u16 *y,
> +			    u16 *z, u16 *angle, u16 *magnitude)
> +{
> +	unsigned int status;
> +	u8 reg_data[12];
> +	int ret;
> +	u16 val;
> +
> +	mutex_lock(&data->lock);
> +	ret = regmap_read(data->map, TMAG5273_CONV_STATUS, &status);
> +	if (ret < 0)
> +		goto out_unlock;
> +
> +	/*
> +	 * Conversion time is 2425 us in 32x averaging mode for all three
> +	 * channels. Since we are in continuous measurement mode, a measurement
> +	 * may already be there, so poll for completed measurement with
> +	 * timeout.

Ideally adjust timeout to match what we expect for current mode. Can be a later
optimization if you prefer.

> +	 */
> +	ret = regmap_read_poll_timeout(data->map, TMAG5273_CONV_STATUS, status,
> +				       status & TMAG5273_CONV_STATUS_COMPLETE,
> +				       100, 10000);
> +	if (ret) {
> +		dev_err_probe(data->dev, ret, "timeout waiting for measurement\n");
> +		goto out_unlock;
> +	}
> +
> +	ret = regmap_bulk_read(data->map, TMAG5273_T_MSB_RESULT, reg_data,
> +			       sizeof(reg_data));
Is there a need to read it in bulk?  Seems like it makes sense for the first 8 registers.
If you split it there those can go directly into __be16 reg_data[4]
then you can do a 2 byte read to get the angle and a separate one byte read to get the
magnitude.

Not as efficient though so maybe this dance is worthwhile.

> +	if (ret)
> +		goto out_unlock;
> +
> +	ret = regmap_read(data->map, TMAG5273_CONV_STATUS, &status);
> +	if (ret < 0)
> +		goto out_unlock;
> +
> +	mutex_unlock(&data->lock);
> +
> +	*t = get_unaligned_be16(&reg_data[0]);
> +	*x = get_unaligned_be16(&reg_data[2]);
> +	*y = get_unaligned_be16(&reg_data[4]);
> +	*z = get_unaligned_be16(&reg_data[6]);
As below - drop these directly into the s16 variables rather than going via a u16.
> +	/*
> +	 * angle has 9 bits integer value and 4 bits fractional part
> +	 * 15 14 13 12 11 10 9  8  7  6  5  4  3  2  1  0
> +	 * 0  0  0  a  a  a  a  a  a  a  a  a  f  f  f  f
> +	 */
> +	val = get_unaligned_be16(&reg_data[9]);
> +	*angle = FIELD_GET(GENMASK(12, 0), val);
> +	*magnitude = reg_data[11];
> +	return ret;
> +
> +out_unlock:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +/*
> + * tmag5273_get_measure() - Measure a sample of all axis and process
> + * @tmag5273: The device state
> + * @to: Temperature out
> + * @xo: X axis out
> + * @yo: Y axis out
> + * @zo: Z axis out
> + * @ao: Angle out
> + * @mo: Magnitude out
> + * @return: 0 on success or error code
> + */
> +static int tmag5273_get_measure(struct tmag5273_data *data, s32 *to, s32 *xo,
> +				s32 *yo, s32 *zo, u16 *ao, u16 *mo)
> +{
> +	u16 t, x, y, z, angle, magnitude;

If these are signed, use signed variables.

> +	int ret;
> +
> +	ret = tmag5273_measure(data, &t, &x, &y, &z, &angle, &magnitude);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * convert device specific value to millicelsius
> +	 * use multiply by 16639 and divide by 10000 to achieve divide by 60.1
> +	 *   and convert to millicelsius
> +	 */
> +	*to = (((s32)t - 17508) * 16639) / 1000 + 25000;

That looks like it should be presented as a _raw channel and the offset
and scale exported to userspace (which is much better at doign this sort of maths)

> +	*xo = (s16)x;
> +	*yo = (s16)y;
> +	*zo = (s16)z;

If these are 16 bit, why expand to 32 bits?  Also if you are doing that
do it with sign_extend32() not like this so it is clear wht is going on.

> +	*ao = angle;
> +	*mo = magnitude;
> +	return 0;
> +}
> +
> +static int tmag5273_read_raw(struct iio_dev *indio_dev,
> +			     const struct iio_chan_spec *chan, int *val,
> +			     int *val2, long mask)
> +{
> +	struct tmag5273_data *data = iio_priv(indio_dev);
> +	s32 t, x, y, z;
> +	u16 angle, magnitude;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_PROCESSED:
> +	case IIO_CHAN_INFO_RAW:
> +		ret = pm_runtime_resume_and_get(data->dev);
> +		if (ret < 0)
> +			return ret;

blank line here.

> +		ret = tmag5273_get_measure(data, &t, &x, &y, &z, &angle,
> +					   &magnitude);
> +		if (ret)
> +			return ret;

blank line here.

> +		pm_runtime_mark_last_busy(data->dev);
> +		pm_runtime_put_autosuspend(data->dev);
> +		switch (chan->scan_index) {
Better to use chan->addr and put enum values in there for the
different channels.  scan_index has a different purpose that you
aren't yet supporting in this driver (buffered mode / chardev
access).

> +		case 0:
> +			*val = t;
return IIO_VAL_INT;
in each of these rather than break.

> +			break;
> +		case 1:
> +			*val = x;
> +			break;
> +		case 2:
> +			*val = y;
> +			break;
> +		case 3:
> +			*val = z;
> +			break;
> +		case 4:
> +			*val = angle;
> +			break;
> +		case 5:
> +			*val = magnitude;
> +			break;
> +		default:
> +			return dev_err_probe(data->dev, -EINVAL, "unknown channel\n");

Not in probe, so not appropriate to use dev_err_probe().

> +		}
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_TEMP:
> +			/* 1 LSB = 1 millidegree Celsius */

Don't expose scale if it's already in the base units.
You can't get here anyway as you haven't set relevant bit in the mask 
for the temp channesl.

> +			*val = 1;
> +			return IIO_VAL_INT;
> +		case IIO_MAGN:
> +			/*
> +			 * The axis values are in stored in 2^15 / range LSB/mT.
> +			 * Since 1 mT = 10 Gauss, we need to multiply by 10 and
> +			 * divide by [range] to get Gauss from the raw value.
> +			 */
> +			*val = data->range * 10;
> +			*val2 = 32768;
> +			return IIO_VAL_FRACTIONAL;
> +		case IIO_ANGL:
> +			/*
> +			 * Angle is in degrees and has four fractional bits,
> +			 * therefore use 1/16 * pi/180 to convert to radiants.
> +			 */
> +			*val = 1000;
> +			*val2 = 916732;
> +			return IIO_VAL_FRACTIONAL;
> +		case IIO_DISTANCE:
> +			/* Magnitude is unscaled */

If it's unscaled then don't provide a scale and make it a raw reading
not a processed one.  This is common for various types of proximity sensor
where we don't have any way to know the scaling.

> +			*val = 1;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		/* Unknown request */
> +		return -EINVAL;
> +	}
> +}

> +static struct attribute *tmag5273_attributes[] = {
> +	&iio_dev_attr_conv_avg.dev_attr.attr,
> +	&iio_dev_attr_conv_avg_available.dev_attr.attr,

Needs documentation so we can review what it is for and see if it either
fits with existing ABI or we need new generic ABI.
Documentation/ABI/testing/sysfs-bus-iio-*

Superficially I'm guessing this is an oversampling control and we
have standard ABI for that.


> +	&iio_dev_attr_range.dev_attr.attr,
> +	&iio_dev_attr_range_available.dev_attr.attr,

It's rare we allow a range attribute. I most cases the same can be derived
from SCALE plus available for the raw channel.
There have been a few really obscure corner cases where it has been only way
of exposing things, but those always came with detailed justification.

> +	NULL,

No trailing comma on NULL terminators.

> +};
> +
> +static const struct attribute_group tmag5273_attrs_group = {
> +	.attrs = tmag5273_attributes,
> +};
> +
> +#define TMAG5273_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),    \
> +		.scan_index = index,                               \
> +		.scan_type = {                                     \
> +			.sign = 's',                               \
> +			.realbits = 32,                            \
> +			.storagebits = 32,                         \

From above these seem to be 16 bit...

> +			.endianness = IIO_CPU,                     \
> +		},                                                 \
> +	}
> +
> +static const struct iio_chan_spec tmag5273_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 's',
> +			.realbits = 32,
> +			.storagebits = 32,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	TMAG5273_AXIS_CHANNEL(X, 1),
> +	TMAG5273_AXIS_CHANNEL(Y, 2),
> +	TMAG5273_AXIS_CHANNEL(Z, 3),
> +	{
> +		.type = IIO_ANGL,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = 4,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	{
> +		.type = IIO_DISTANCE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
Is it processed, or is it 
> +		.scan_index = 5,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(6),
> +};

...

> +
> +static int tmag5273_chip_init(struct tmag5273_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_write(data->map, TMAG5273_DEVICE_CONFIG_1,
> +			   TMAG5273_AVG_32_MODE);
> +	if (ret)
> +		return ret;
> +	data->conv_avg = 32;
Blank line here to separate blocks doing more or less unconnected things.

> +	ret = regmap_write(data->map, TMAG5273_DEVICE_CONFIG_2,
> +			   TMAG5273_OP_MODE_CONT);

Register a devm_add_action_or_reset() to handle putting it in 
sleep in error paths / remove().

> +	if (ret)
> +		return ret;
Blank line here helps readability a little.
> +	ret = regmap_write(data->map, TMAG5273_SENSOR_CONFIG_1,
> +			   FIELD_PREP(TMAG5273_MAG_CH_EN_MASK,
> +				      TMAG5273_MAG_CH_EN_X_Y_Z));
> +	if (ret)
> +		return ret;
and here

> +	ret = regmap_write(data->map, TMAG5273_SENSOR_CONFIG_2,
> +			   FIELD_PREP(TMAG5273_ANGLE_EN_MASK, data->angle_en));
> +	if (ret)
> +		return ret;
> +
> +	data->range = tmag5273_range_table[data->version][0].range;
> +	return regmap_write(data->map, TMAG5273_T_CONFIG, TMAG5273_T_CH_EN);
> +}
> +
> +static int tmag5273_probe(struct i2c_client *i2c,
> +			  const struct i2c_device_id *id)
> +{
> +	struct iio_dev *indio_dev;
> +	struct device *dev = &i2c->dev;
> +	struct device_node *node = dev->of_node;
> +	struct tmag5273_data *data;
> +	int val, ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(i2c, indio_dev);
> +	data->dev = dev;
> +	mutex_init(&data->lock);
> +
> +	data->vcc = devm_regulator_get(dev, "vcc");
> +	if (IS_ERR(data->vcc))
> +		return dev_err_probe(dev, PTR_ERR(data->vcc),
> +				     "failed to get VCC regulator\n");
> +
> +	/* Operating voltage 1.7V .. 3.6V according to datasheet */

That is normally considered a job for device tree binding or similar rather than
a driver to configure it. AS such, can probably just use
devm_regulator_get_enabled() and drop the need to manually turn it off again



> +	ret = regulator_set_voltage(data->vcc, 1700000, 3600000);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to set VCC voltage\n");
> +
> +	ret = regulator_enable(data->vcc);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to enable VCC regulator\n");
> +
> +	/*
> +	 * Regulators have to ramp up with 3V/ms, additional time to go to
> +	 * stand-by mode is 270us typically. We give 1 ms to 2 ms time.

That should be a problem for the regulator driver, not this one.
The regulator_enable() should not return until it is powered up and we can carry
on.

> +	 */
> +	usleep_range(1000, 2000);
> +
> +	data->map = devm_regmap_init_i2c(i2c, &tmag5273_regmap_config);
> +	if (IS_ERR(data->map)) {
> +		ret = PTR_ERR(data->map);
> +		dev_err_probe(dev, ret, "failed to allocate register map\n");
> +		goto out_disable_vcc;
> +	}
> +
> +	ret = regmap_read(data->map, TMAG5273_DEVICE_ID, &val);
> +	if (ret) {
> +		/*
> +		 * If we come from sleep with power already activated, the
> +		 * first I2C command wakes up the chip but will fail.

Minor but I'd make this unconditional. It's a small cost in probe() time but
simplifies the code flow a little.


> +		 * Time to go to stand-by mode from sleep mode is 50us
> +		 * typically. During this time no I2C access is possible.
> +		 */
> +		usleep_range(80, 200);
> +
> +		ret = regmap_read(data->map, TMAG5273_DEVICE_ID, &val);
> +		if (ret)
> +			goto out_disable_vcc;
> +	}
> +	data->version = FIELD_PREP(TMAG5273_VERSION_MASK, val);
> +
> +	ret = regmap_bulk_read(data->map, TMAG5273_MANUFACTURER_ID_LSB,
> +			       &data->devid, 2);
> +	if (ret)
> +		goto out_disable_vcc;
> +
> +	switch (data->devid) {
> +	case TMAG5273_MANUFACTURER_ID:
> +		strncpy(data->name, "TMAG5273", sizeof(data->name) - 2);

Lowercase preferred as much more common for IIO device names.
(no idea why, but we have ended up like that ;)

I'd also be lazy / efficient as there are only two names

		switch (data->version) {
		case 1:
			indio_dev->name = "tmag5273x1";
			break;
		case 2:
			indio_dev->name = "tmag5273x2";
			break;
		default:
		 	return -ENODEV; //after switch to devm for everything.
		}	
> +		switch (data->version) {
> +		case 1:
> +			strncat(data->name, "x1", 2);
> +			break;
> +		case 2:
> +			strncat(data->name, "x2", 2);
> +			break;
> +		default:
> +			break;
> +		}
> +		dev_info(dev, "%s", data->name);
		noisy so don't print it. Can read from sysfs anyway.
> +		data->max_avg = 32;

		Given this is only valid option, why is it in here?

> +		break;
> +	default:
> +		ret = -ENODEV;

We generally only warn on such cases.  Often manufacturers spin new versions
of chips that are compatible enough that we give them a fallback compatible
in device tree.  That is only a useful thing to do if they'll 'work' with old
drivers.   A warning that it might be the wrong device is considered fine though.
"Unknown device ID...

Bit tricky in this case as we have to pick between two values for some things.
Chopse a default maybe?


> +		dev_err_probe(dev, ret, "unhandled device ID 0x%x\n", data->devid);
> +		goto out_disable_vcc;
> +	}
> +
> +	/*
> +	 * Angle-enable is optional and set to 1 (enable X-Y plane) by default,
> +	 * the value is modified only if a valid u32 value can be decoded.
> +	 */
> +	data->angle_en = TMAG5273_ANGLE_EN_X_Y;
> +	of_property_read_u32(node, "tmag5273,angle-enable", &data->angle_en);

Use generic firmware properties linux/property.h so this will work with other
types of firmware.

> +
> +	ret = tmag5273_chip_init(data);
> +	if (ret)
> +		goto out_disable_vcc;
> +
> +	indio_dev->info = &tmag5273_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->name = data->name;
> +	indio_dev->channels = tmag5273_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(tmag5273_channels);
> +
> +	pm_runtime_set_autosuspend_delay(dev, TMAG5273_AUTOSLEEP_DELAY);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_mark_last_busy(dev);
> +	ret = pm_runtime_set_active(dev);
> +	if (ret < 0)
> +		goto out_disable_vcc;
> +	pm_runtime_enable(dev);

devm_pm_runtime_enable()

> +	pm_runtime_idle(dev);

Why idle rather than a pm_runtime_put_autosuspend()?

> +
> +	ret = iio_device_register(indio_dev);
devm_iio_device_register()

> +	if (ret) {
> +		dev_err_probe(dev, ret, "device register failed\n");
> +		goto cleanup_runtime;
> +	}
> +
> +	return 0;
> +
> +cleanup_runtime:
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
> +out_disable_vcc:
> +	tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_SLEEP);
> +	regulator_disable(data->vcc);
> +	return ret;
> +}
> +
> +static void tmag5273_remove(struct i2c_client *i2c)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
> +	struct tmag5273_data *data = iio_priv(indio_dev);
> +	struct device *dev = &i2c->dev;
> +
> +	iio_device_unregister(indio_dev);
> +
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +
> +	tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_SLEEP);

With use of devm_add_action_or_reset() for this you should be able to 
move to a simpler fully devm_ managed flow and not have a remove()
callback at all.

> +	regulator_disable(data->vcc);
> +}
> +
> +static int tmag5273_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct tmag5273_data *data = iio_priv(indio_dev);
> +
> +	tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_SLEEP);
> +
> +	return 0;
> +}
> +
> +static int tmag5273_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct tmag5273_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_CONT);
> +	if (ret) {
> +		/*
> +		 * Time to go to stand-by mode from sleep mode is 50us
> +		 * typically. During this time no I2C access is possible.
> +		 */
How would we ever not have entered sleep mode and yet be calling runtime
resume?  I'd just make the i2c access twice unconditionally.

> +		usleep_range(80, 200);
> +		ret = tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_CONT);
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops tmag5273_pm_ops = {
> +	SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> +	RUNTIME_PM_OPS(tmag5273_runtime_suspend, tmag5273_runtime_resume, NULL)
> +};

This looks familiar. 
static const DEFINE_RUNTIME_DEV_PM_OPS(...)

> +
> +static const struct i2c_device_id tmag5273_id[] = {
> +	{
> +		"tmag5273",
> +	},
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(i2c, tmag5273_id);
> +
> +static const struct of_device_id tmag5273_of_match[] = {
> +	{
> +		.compatible = "ti,tmag5273",
> +	},
> +	{ /* sentinel */ },

Drop the trailing commas on these entries as we can never add anything after them
so the comma never makes sense.


> +};
> +MODULE_DEVICE_TABLE(of, tmag5273_of_match);

Thanks,

Jonathan

Re: [PATCH 1/2] dt-bindings: iio: magnetometer: add ti tmag5273 documentation file

[Jonathan Cameron]

On Tue, 15 Nov 2022 08:37:17 +0100
Gerald Loacker <gerald.loacker@wolfvision.net> wrote:

> Add bindings documentation file for TI TMAG5273.
> 
> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> ---
>  .../iio/magnetometer/ti,tmag5273.yaml         | 72 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
> new file mode 100644
> index 000000000000..2f5b0a4d2f40
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/magnetometer/ti,tmag5273.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor
> +
> +maintainers:
> +  - Gerald Loacker <gerald.loacker@wolfvision.net>
> +
> +description:
> +  The TI TMAG5273 is a low-power linear 3D Hall-effect sensor. This device
> +  integrates three independent Hall-effect sensors in the X, Y, and Z axes.
> +  The device has an integrated temperature sensor available. The TMAG5273
> +  can be configured through the I2C interface to enable any combination of
> +  magnetic axes and temperature measurements. An integrated angle calculation
> +  engine (CORDIC) provides full 360° angular position information for both
> +  on-axis and off-axis angle measurement topologies. The angle calculation is
> +  performed using two user-selected magnetic axes.
> +
> +properties:
> +  $nodename:
> +    pattern: '^magnetometer@[0-9a-f]+$'

What Krzysztof said on this ;)

> +
> +  compatible:
> +    const: ti,tmag5273
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +  ti,angle-enable:
> +    description:
> +      Enables angle measurement in the selected plane.
> +      0 = OFF
> +      1 = X-Y (default)
> +      2 = Y-Z
> +      3 = X-Z

This feels like something we should be configuring at runtime rather that
DT, or is it driven by board design or similar?

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 3
> +
> +  vcc-supply:
> +    description:
> +      A regulator providing 1.7 V to 3.6 V supply voltage on the VCC pin,
> +      typically 3.3 V.
> +

The dt binding should attempt to describe the hardware, not what we happen
to support in the driver so far. So I'd expect to also see an interrupt.
That way if someone ships a dts file today, and we enable it sometime in the
future they will be ready for it.

> +required:
> +  - compatible
> +  - reg
> +  - vcc-supply

Ah. This is presumably as side effect of having the driver set the voltage.
Normally we only need to 'require' a supply if we need to read it's voltage
(for scaling on ADCs and similar). That's not the case here so I wouldn't
expect to see it.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c-0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        magnetometer@35 {
> +            compatible = "ti,tmag5273";
> +            reg = <0x35>;
> +            #io-channel-cells = <1>;
> +            ti,angle-enable = <3>;
> +            vcc-supply = <&vcc3v3>;
> +        };
> +    };
> +...

Re: [PATCH 1/2] dt-bindings: iio: magnetometer: add ti tmag5273 documentation file

[Gerald Loacker]

Am 15.11.2022 um 18:43 schrieb Jonathan Cameron:
> On Tue, 15 Nov 2022 08:37:17 +0100
> Gerald Loacker <gerald.loacker@wolfvision.net> wrote:
> 
>> Add bindings documentation file for TI TMAG5273.
>>
>> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
>> ---
>>  .../iio/magnetometer/ti,tmag5273.yaml         | 72 +++++++++++++++++++
>>  MAINTAINERS                                   |  6 ++
>>  2 files changed, 78 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
>> new file mode 100644
>> index 000000000000..2f5b0a4d2f40
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
>> @@ -0,0 +1,72 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: https://eur04.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fiio%2Fmagnetometer%2Fti%2Ctmag5273.yaml%23&amp;data=05%7C01%7Cgerald.loacker%40wolfvision.net%7C9788e9788f344fcff9b808dac730f926%7Ce94ec9da9183471e83b351baa8eb804f%7C1%7C0%7C638041310400330990%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=nczO1QC74gD6eGXAkm%2B6LRrc7fyEsr62r%2B3aoW%2Bcfu4%3D&amp;reserved=0
>> +$schema: https://eur04.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=05%7C01%7Cgerald.loacker%40wolfvision.net%7C9788e9788f344fcff9b808dac730f926%7Ce94ec9da9183471e83b351baa8eb804f%7C1%7C0%7C638041310400330990%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=raKUiSntfhvdSSnqiR1Wm%2Fqr9cI3XEu5HCprqvISlLE%3D&amp;reserved=0
>> +
>> +title: TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor
>> +
>> +maintainers:
>> +  - Gerald Loacker <gerald.loacker@wolfvision.net>
>> +
>> +description:
>> +  The TI TMAG5273 is a low-power linear 3D Hall-effect sensor. This device
>> +  integrates three independent Hall-effect sensors in the X, Y, and Z axes.
>> +  The device has an integrated temperature sensor available. The TMAG5273
>> +  can be configured through the I2C interface to enable any combination of
>> +  magnetic axes and temperature measurements. An integrated angle calculation
>> +  engine (CORDIC) provides full 360° angular position information for both
>> +  on-axis and off-axis angle measurement topologies. The angle calculation is
>> +  performed using two user-selected magnetic axes.
>> +
>> +properties:
>> +  $nodename:
>> +    pattern: '^magnetometer@[0-9a-f]+$'
> 
> What Krzysztof said on this ;)
> 
>> +
>> +  compatible:
>> +    const: ti,tmag5273
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  "#io-channel-cells":
>> +    const: 1
>> +
>> +  ti,angle-enable:
>> +    description:
>> +      Enables angle measurement in the selected plane.
>> +      0 = OFF
>> +      1 = X-Y (default)
>> +      2 = Y-Z
>> +      3 = X-Z
> 
> This feels like something we should be configuring at runtime rather that
> DT, or is it driven by board design or similar?
> 

We use this sensor for a zoom wheel application, there is an EVM from TI
for this as well. So this is for setting the mounting position of the wheel.

>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 0
>> +    maximum: 3
>> +
>> +  vcc-supply:
>> +    description:
>> +      A regulator providing 1.7 V to 3.6 V supply voltage on the VCC pin,
>> +      typically 3.3 V.
>> +
> 
> The dt binding should attempt to describe the hardware, not what we happen
> to support in the driver so far. So I'd expect to also see an interrupt.
> That way if someone ships a dts file today, and we enable it sometime in the
> future they will be ready for it.
>

Is it fine to add just the description without example then? The
interrupt has many options such as low active or low pulse.

>> +required:
>> +  - compatible
>> +  - reg
>> +  - vcc-supply
> 
> Ah. This is presumably as side effect of having the driver set the voltage.
> Normally we only need to 'require' a supply if we need to read it's voltage
> (for scaling on ADCs and similar). That's not the case here so I wouldn't
> expect to see it.
> 
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c-0 {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        magnetometer@35 {
>> +            compatible = "ti,tmag5273";
>> +            reg = <0x35>;
>> +            #io-channel-cells = <1>;
>> +            ti,angle-enable = <3>;
>> +            vcc-supply = <&vcc3v3>;
>> +        };
>> +    };
>> +...
> 
> 

Thanks for the review,
Gerald

Re: [PATCH 1/2] dt-bindings: iio: magnetometer: add ti tmag5273 documentation file

[Krzysztof Kozlowski]

On 17/11/2022 17:12, Gerald Loacker wrote:
>>
>>> +
>>> +  compatible:
>>> +    const: ti,tmag5273
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  "#io-channel-cells":
>>> +    const: 1
>>> +
>>> +  ti,angle-enable:
>>> +    description:
>>> +      Enables angle measurement in the selected plane.
>>> +      0 = OFF
>>> +      1 = X-Y (default)
>>> +      2 = Y-Z
>>> +      3 = X-Z
>>
>> This feels like something we should be configuring at runtime rather that
>> DT, or is it driven by board design or similar?
>>
> 
> We use this sensor for a zoom wheel application, there is an EVM from TI
> for this as well. So this is for setting the mounting position of the wheel.

That's ok, but does not explain why choice of angle measurement should
be a property of the hardware. I could imagine configuring device to
measure sometimes X-Y and sometimes X-Z, depending on the use case. Use
case can change runtime.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: iio: magnetometer: add ti tmag5273 documentation file

[Jonathan Cameron]

On Thu, 17 Nov 2022 17:12:33 +0100
Gerald Loacker <gerald.loacker@wolfvision.net> wrote:

> Am 15.11.2022 um 18:43 schrieb Jonathan Cameron:
> > On Tue, 15 Nov 2022 08:37:17 +0100
> > Gerald Loacker <gerald.loacker@wolfvision.net> wrote:
> >   
> >> Add bindings documentation file for TI TMAG5273.
> >>
> >> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> >> ---
> >>  .../iio/magnetometer/ti,tmag5273.yaml         | 72 +++++++++++++++++++
> >>  MAINTAINERS                                   |  6 ++
> >>  2 files changed, 78 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
> >> new file mode 100644
> >> index 000000000000..2f5b0a4d2f40
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
> >> @@ -0,0 +1,72 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: https://eur04.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fiio%2Fmagnetometer%2Fti%2Ctmag5273.yaml%23&amp;data=05%7C01%7Cgerald.loacker%40wolfvision.net%7C9788e9788f344fcff9b808dac730f926%7Ce94ec9da9183471e83b351baa8eb804f%7C1%7C0%7C638041310400330990%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=nczO1QC74gD6eGXAkm%2B6LRrc7fyEsr62r%2B3aoW%2Bcfu4%3D&amp;reserved=0
> >> +$schema: https://eur04.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=05%7C01%7Cgerald.loacker%40wolfvision.net%7C9788e9788f344fcff9b808dac730f926%7Ce94ec9da9183471e83b351baa8eb804f%7C1%7C0%7C638041310400330990%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=raKUiSntfhvdSSnqiR1Wm%2Fqr9cI3XEu5HCprqvISlLE%3D&amp;reserved=0
> >> +
> >> +title: TI TMAG5273 Low-Power Linear 3D Hall-Effect Sensor
> >> +
> >> +maintainers:
> >> +  - Gerald Loacker <gerald.loacker@wolfvision.net>
> >> +
> >> +description:
> >> +  The TI TMAG5273 is a low-power linear 3D Hall-effect sensor. This device
> >> +  integrates three independent Hall-effect sensors in the X, Y, and Z axes.
> >> +  The device has an integrated temperature sensor available. The TMAG5273
> >> +  can be configured through the I2C interface to enable any combination of
> >> +  magnetic axes and temperature measurements. An integrated angle calculation
> >> +  engine (CORDIC) provides full 360° angular position information for both
> >> +  on-axis and off-axis angle measurement topologies. The angle calculation is
> >> +  performed using two user-selected magnetic axes.
> >> +
> >> +properties:
> >> +  $nodename:
> >> +    pattern: '^magnetometer@[0-9a-f]+$'  
> > 
> > What Krzysztof said on this ;)
> >   
> >> +
> >> +  compatible:
> >> +    const: ti,tmag5273
> >> +
> >> +  reg:
> >> +    maxItems: 1
> >> +
> >> +  "#io-channel-cells":
> >> +    const: 1
> >> +
> >> +  ti,angle-enable:
> >> +    description:
> >> +      Enables angle measurement in the selected plane.
> >> +      0 = OFF
> >> +      1 = X-Y (default)
> >> +      2 = Y-Z
> >> +      3 = X-Z  
> > 
> > This feels like something we should be configuring at runtime rather that
> > DT, or is it driven by board design or similar?
> >   
> 
> We use this sensor for a zoom wheel application, there is an EVM from TI
> for this as well. So this is for setting the mounting position of the wheel.

Ok. Thanks for explanation. Makes sense.

> 
> >> +    $ref: /schemas/types.yaml#/definitions/uint32
> >> +    minimum: 0
> >> +    maximum: 3
> >> +
> >> +  vcc-supply:
> >> +    description:
> >> +      A regulator providing 1.7 V to 3.6 V supply voltage on the VCC pin,
> >> +      typically 3.3 V.
> >> +  
> > 
> > The dt binding should attempt to describe the hardware, not what we happen
> > to support in the driver so far. So I'd expect to also see an interrupt.
> > That way if someone ships a dts file today, and we enable it sometime in the
> > future they will be ready for it.
> >  
> 
> Is it fine to add just the description without example then? The
> interrupt has many options such as low active or low pulse.

Yes.  Just add description. The details of type etc depend on wiring
to some degree (inverters in the path etc). Nice to add an example later
but for now we can assume anyone who is providing the interrupt set it
up correctly.

Jonathan

Re: [PATCH 1/2] dt-bindings: iio: magnetometer: add ti tmag5273 documentation file

[Jonathan Cameron]

On Thu, 17 Nov 2022 17:17:24 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 17/11/2022 17:12, Gerald Loacker wrote:
> >>  
> >>> +
> >>> +  compatible:
> >>> +    const: ti,tmag5273
> >>> +
> >>> +  reg:
> >>> +    maxItems: 1
> >>> +
> >>> +  "#io-channel-cells":
> >>> +    const: 1
> >>> +
> >>> +  ti,angle-enable:
> >>> +    description:
> >>> +      Enables angle measurement in the selected plane.
> >>> +      0 = OFF
> >>> +      1 = X-Y (default)
> >>> +      2 = Y-Z
> >>> +      3 = X-Z  
> >>
> >> This feels like something we should be configuring at runtime rather that
> >> DT, or is it driven by board design or similar?
> >>  
> > 
> > We use this sensor for a zoom wheel application, there is an EVM from TI
> > for this as well. So this is for setting the mounting position of the wheel.  
> 
> That's ok, but does not explain why choice of angle measurement should
> be a property of the hardware. I could imagine configuring device to
> measure sometimes X-Y and sometimes X-Z, depending on the use case. Use
> case can change runtime.

If it's part of a physical device mounting, may well not be changeable at runtime
(at least not with out a screw driver / hacksaw etc.  Not really different form
arguing someone might rewire a sensor at runtime.  In theory possible but if they
do they are on their own - mostly we don't bother supporting them doing that.

So I think this is probably valid as a DT property.


> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/2] dt-bindings: iio: magnetometer: add ti tmag5273 documentation file

[Michael Riesch]

Hi Krzysztof,

On 11/17/22 17:17, Krzysztof Kozlowski wrote:
> On 17/11/2022 17:12, Gerald Loacker wrote:
>>>
>>>> +
>>>> +  compatible:
>>>> +    const: ti,tmag5273
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  "#io-channel-cells":
>>>> +    const: 1
>>>> +
>>>> +  ti,angle-enable:
>>>> +    description:
>>>> +      Enables angle measurement in the selected plane.
>>>> +      0 = OFF
>>>> +      1 = X-Y (default)
>>>> +      2 = Y-Z
>>>> +      3 = X-Z
>>>
>>> This feels like something we should be configuring at runtime rather that
>>> DT, or is it driven by board design or similar?
>>>
>>
>> We use this sensor for a zoom wheel application, there is an EVM from TI
>> for this as well. So this is for setting the mounting position of the wheel.
> 
> That's ok, but does not explain why choice of angle measurement should
> be a property of the hardware. I could imagine configuring device to
> measure sometimes X-Y and sometimes X-Z, depending on the use case. Use
> case can change runtime.

If I may chime in here: in our use case the angle channel is used
directly as an input to adc-joystick, so that the combination of the two
is an input device. We feel that in this scenario this angle measurement
setting *has* to be a hardware property because the correct function of
the input device depends on the correct choice of the angle property
(which does not change during runtime, of course).

If we were to create a different input device in which the magnetometer
was tilted by 90° (for example), then the angle property could be easily
changed in the device tree. The user space, on the other hand, couldn't
possibly know the correct angle property.

That said, I agree that there may be use cases in which the angle
property should be changed during runtime. Would it be acceptable to
create an IIO property that is initialized by the device tree property?
(Please note that the implementation of the IIO property may not be in
our scope, though)

Best regards,
Michael

Re: [PATCH 1/2] dt-bindings: iio: magnetometer: add ti tmag5273 documentation file

[Krzysztof Kozlowski]

On 17/11/2022 18:01, Michael Riesch wrote:
> Hi Krzysztof,
> 
> On 11/17/22 17:17, Krzysztof Kozlowski wrote:
>> On 17/11/2022 17:12, Gerald Loacker wrote:
>>>>
>>>>> +
>>>>> +  compatible:
>>>>> +    const: ti,tmag5273
>>>>> +
>>>>> +  reg:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  "#io-channel-cells":
>>>>> +    const: 1
>>>>> +
>>>>> +  ti,angle-enable:
>>>>> +    description:
>>>>> +      Enables angle measurement in the selected plane.
>>>>> +      0 = OFF
>>>>> +      1 = X-Y (default)
>>>>> +      2 = Y-Z
>>>>> +      3 = X-Z
>>>>
>>>> This feels like something we should be configuring at runtime rather that
>>>> DT, or is it driven by board design or similar?
>>>>
>>>
>>> We use this sensor for a zoom wheel application, there is an EVM from TI
>>> for this as well. So this is for setting the mounting position of the wheel.
>>
>> That's ok, but does not explain why choice of angle measurement should
>> be a property of the hardware. I could imagine configuring device to
>> measure sometimes X-Y and sometimes X-Z, depending on the use case. Use
>> case can change runtime.
> 
> If I may chime in here: in our use case the angle channel is used
> directly as an input to adc-joystick, so that the combination of the two
> is an input device. We feel that in this scenario this angle measurement
> setting *has* to be a hardware property because the correct function of
> the input device depends on the correct choice of the angle property
> (which does not change during runtime, of course).
> 
> If we were to create a different input device in which the magnetometer
> was tilted by 90° (for example), then the angle property could be easily
> changed in the device tree. The user space, on the other hand, couldn't
> possibly know the correct angle property.
> 
> That said, I agree that there may be use cases in which the angle
> property should be changed during runtime. Would it be acceptable to
> create an IIO property that is initialized by the device tree property?
> (Please note that the implementation of the IIO property may not be in
> our scope, though)

It's fine. Thanks for the explanations. For runtime-configurable setups
this still can be changed via some other interface.

Best regards,
Krzysztof