[PATCH v2 0/3] add ti tmag5273 driver

[PATCH v2 0/3] 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.

Changes in v3:
 - Added structs for iio types to iio.h. Using these structs for iio type
   arrays such as IIO_AVAIL_LIST makes the code more readable than just
   using (int *). It was suggested by Andy Shevchenko to move these structs
   to the iio headers to avoid different approaches.
 - dt-bindings: dropped quotes from strings
 - Added include <linux/bitfield.h>
   | Reported-by: kernel test robot <lkp@intel.com> 
 - Added include <linux/bits.h>
 - Removed <asm/unaligned.h>
 - Added missing "static const" for tmag5273_avg_table
 - Documented Device ID
 - Fixed index of tmag5273_scale definition
 - Clarify TMAG5273_MAG_CH_EN_X_Y_Z as an index
 - Removed unnecessary print
 - Introduced tmag5273_write_scale() and tmag5273_write_osr() helper
   functions
 - Use of match_string()
 - Format

Changes in v2:
 Thanks to Krzysztof, Andy and Jonathan for your detailed review and
 explanations on the first version. This patch includes all your
 suggestions and some additional cleanup in the probe function.

Gerald Loacker (3):
  iio: add struct declarations for iio types
  dt-bindings: iio: magnetometer: add ti tmag5273 documentation file
  iio: magnetometer: add ti tmag5273 driver

 .../iio/magnetometer/ti,tmag5273.yaml         |  75 ++
 MAINTAINERS                                   |   7 +
 drivers/iio/magnetometer/Kconfig              |  12 +
 drivers/iio/magnetometer/Makefile             |   2 +
 drivers/iio/magnetometer/tmag5273.c           | 736 ++++++++++++++++++
 include/linux/iio/iio.h                       |  15 +
 6 files changed, 847 insertions(+)
 create mode 100644 .../bindings/iio/magnetometer/ti,tmag5273.yaml
 create mode 100644 drivers/iio/magnetometer/tmag5273.c

-- 
2.37.2

[PATCH v3 1/3] iio: add struct declarations for iio types

[Gerald Loacker]

Add structs for iio type arrays such as IIO_AVAIL_LIST which can be used
instead of int arrays.

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
---
 include/linux/iio/iio.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index f0ec8a5e5a7a..eaf6727445a6 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -383,6 +383,21 @@ s64 iio_get_time_ns(const struct iio_dev *indio_dev);
 
 struct iio_trigger; /* forward declaration */
 
+struct iio_val_int_plus_micro {
+	int val_int;
+	int val_micro;
+};
+
+struct iio_val_int_plus_nano {
+	int val_int;
+	int val_nano;
+};
+
+struct iio_val_int_plus_micro_db {
+	int val_int;
+	int val_micro_db;
+};
+
 /**
  * struct iio_info - constant information about device
  * @event_attrs:	event control attributes
-- 
2.37.2

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

[Gerald Loacker]

Add bindings for TI TMAG5273.

Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Changes in v3:
 - dropped quotes from strings
 
Changes in v2:
 - Removed nodename
 - Changed angle-enable to angle-measurement and used strings
 - Added interrupts
 - Removed vcc-supply from required properties
 - Changed i2c-0 to i2c in examples
 .../iio/magnetometer/ti,tmag5273.yaml         | 75 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 81 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..121d540b7b6e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/ti,tmag5273.yaml
@@ -0,0 +1,75 @@
+# 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:
+  compatible:
+    const: ti,tmag5273
+
+  reg:
+    maxItems: 1
+
+  "#io-channel-cells":
+    const: 1
+
+  ti,angle-measurement:
+    $ref: /schemas/types.yaml#/definitions/string
+    description:
+      Enables angle measurement in the selected plane.
+      If not specified, "x-y" will be anables as default.
+    enum:
+      - off
+      - x-y
+      - y-z
+      - x-z
+
+  vcc-supply:
+    description:
+      A regulator providing 1.7 V to 3.6 V supply voltage on the VCC pin,
+      typically 3.3 V.
+
+  interrupts:
+    description:
+      The low active interrupt can be configured to be fixed width or latched.
+      Interrupt events can be configured to be generated from magnetic
+      thresholds or when a conversion is completed.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        magnetometer@35 {
+            compatible = "ti,tmag5273";
+            reg = <0x35>;
+            #io-channel-cells = <1>;
+            ti,angle-measurement = "x-z";
+            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 v3 3/3] 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>
---
Changes in v3:
 - Added include <linux/bitfield.h>
   | Reported-by: kernel test robot <lkp@intel.com> 
 - Added include <linux/bits.h>
 - Removed <asm/unaligned.h>
 - Added missing "static const" for tmag5273_avg_table
 - Documented Device ID
 - Fixed index of tmag5273_scale definition
 - Clarify TMAG5273_MAG_CH_EN_X_Y_Z as an index
 - Removed unnecessary print
 - Introduced tmag5273_write_scale() and tmag5273_write_osr() helper
    functions
 - Use of match_string()
 - Format

Changes in v2:
 - Implemented suggestions from review and cleaned up probe function. This
   results in changes all over the tmag5273.c code.
 MAINTAINERS                         |   1 +
 drivers/iio/magnetometer/Kconfig    |  12 +
 drivers/iio/magnetometer/Makefile   |   2 +
 drivers/iio/magnetometer/tmag5273.c | 736 ++++++++++++++++++++++++++++
 4 files changed, 751 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..d61a99fea9fe
--- /dev/null
+++ b/drivers/iio/magnetometer/tmag5273.c
@@ -0,0 +1,736 @@
+// 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/bitfield.h>
+#include <linux/bits.h>
+#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>
+
+#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_AUTOSLEEP_DELAY_MS	 5000
+#define TMAG5273_MAX_AVERAGE             32
+
+/*
+ * bits in the TMAG5273_MANUFACTURER_ID_LSB / MSB register
+ * 16-bit unique manufacturer ID 0x49 / 0x54 = "TI"
+ */
+#define TMAG5273_MANUFACTURER_ID	 0x5449
+
+/* 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	 7
+
+/* 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_OFF		 0
+#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)
+
+enum tmag5273_channels {
+	TEMPERATURE = 0,
+	AXIS_X,
+	AXIS_Y,
+	AXIS_Z,
+	ANGLE,
+	MAGNITUDE,
+};
+
+enum tmag5273_scale_index {
+	MAGN_RANGE_LOW = 0,
+	MAGN_RANGE_HIGH,
+	MAGN_RANGE_NUM
+};
+
+/* state container for the TMAG5273 driver */
+struct tmag5273_data {
+	struct device *dev;
+	unsigned int devid;
+	unsigned int version;
+	char name[16];
+	unsigned int conv_avg;
+	unsigned int scale;
+	enum tmag5273_scale_index scale_index;
+	unsigned int angle_measurement;
+	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 char *const tmag5273_angle_names[] = { "off", "x-y", "y-z", "x-z" };
+
+/*
+ * Averaging enables additional sampling of the sensor data to reduce the noise
+ * effect, but also increases conversion time.
+ */
+static const unsigned int tmag5273_avg_table[] = {
+	1, 2, 4, 8, 16, 32,
+};
+
+/*
+ * Magnetic resolution in Gauss for different TMAG5273 versions.
+ * Scale[Gauss] = Range[mT] * 1000 / 2^15 * 10, (1 mT = 10 Gauss)
+ * Only version 1 and 2 are valid, version 0 and 3 are reserved.
+ */
+static const struct iio_val_int_plus_micro tmag5273_scale[][MAGN_RANGE_NUM] = {
+	{ { 0,     0 }, { 0,     0 } },
+	{ { 0, 12200 }, { 0, 24400 } },
+	{ { 0, 40600 }, { 0, 81200 } },
+	{ { 0,     0 }, { 0,     0 } },
+};
+
+static int tmag5273_get_measure(struct tmag5273_data *data, s16 *t, s16 *x,
+				s16 *y, s16 *z, u16 *angle, u16 *magnitude)
+{
+	unsigned int status, val;
+	__be16 reg_data[4];
+	int ret;
+
+	mutex_lock(&data->lock);
+
+	/*
+	 * Max. 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;
+	*t = be16_to_cpu(reg_data[0]);
+	*x = be16_to_cpu(reg_data[1]);
+	*y = be16_to_cpu(reg_data[2]);
+	*z = be16_to_cpu(reg_data[3]);
+
+	ret = regmap_bulk_read(data->map, TMAG5273_ANGLE_RESULT_MSB,
+			       &reg_data[0], sizeof(reg_data[0]));
+	if (ret)
+		goto out_unlock;
+	/*
+	 * 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
+	 */
+	*angle = be16_to_cpu(reg_data[0]);
+
+	ret = regmap_read(data->map, TMAG5273_MAGNITUDE_RESULT, &val);
+	if (ret < 0)
+		goto out_unlock;
+	*magnitude = val;
+
+out_unlock:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static int tmag5273_write_osr(struct tmag5273_data *data, int val)
+{
+	int i;
+
+	if (val == data->conv_avg)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(tmag5273_avg_table); i++) {
+		if (tmag5273_avg_table[i] == val)
+			break;
+	}
+	if (i == ARRAY_SIZE(tmag5273_avg_table))
+		return -EINVAL;
+	data->conv_avg = val;
+
+	return regmap_update_bits(data->map, TMAG5273_DEVICE_CONFIG_1,
+				  TMAG5273_AVG_MODE_MASK,
+				  FIELD_PREP(TMAG5273_AVG_MODE_MASK, i));
+}
+
+static int tmag5273_write_scale(struct tmag5273_data *data, int scale_micro)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tmag5273_scale[0]); i++) {
+		if (tmag5273_scale[data->version][i].val_micro == scale_micro)
+			break;
+	}
+	if (i == ARRAY_SIZE(tmag5273_scale[0]))
+		return -EINVAL;
+	data->scale_index = i;
+
+	return regmap_update_bits(data->map,
+		TMAG5273_SENSOR_CONFIG_2,
+		TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK,
+		data->scale_index == MAGN_RANGE_LOW ? 0 :
+			TMAG5273_Z_RANGE_MASK |
+			TMAG5273_X_Y_RANGE_MASK);
+}
+
+static int tmag5273_read_avail(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       const int **vals, int *type, int *length,
+			       long mask)
+{
+	struct tmag5273_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*vals = tmag5273_avg_table;
+		*type = IIO_VAL_INT;
+		*length = ARRAY_SIZE(tmag5273_avg_table);
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_MAGN:
+			*type = IIO_VAL_INT_PLUS_MICRO;
+			*vals = (int *)tmag5273_scale[data->version];
+			*length = ARRAY_SIZE(tmag5273_scale[data->version]) *
+				  MAGN_RANGE_NUM;
+			return IIO_AVAIL_LIST;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+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);
+	s16 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->address) {
+		case TEMPERATURE:
+			*val = t;
+			return IIO_VAL_INT;
+		case AXIS_X:
+			*val = x;
+			return IIO_VAL_INT;
+		case AXIS_Y:
+			*val = y;
+			return IIO_VAL_INT;
+		case AXIS_Z:
+			*val = z;
+			return IIO_VAL_INT;
+		case ANGLE:
+			*val = angle;
+			return IIO_VAL_INT;
+		case MAGNITUDE:
+			*val = magnitude;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_TEMP:
+			/*
+			 * Convert device specific value to millicelsius.
+			 * Resolution from the sensor is 60.1 LSB/celsius and
+			 * the reference value at 25 celsius is 17508 LSBs.
+			 */
+			*val = 10000;
+			*val2 = 601;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_MAGN:
+			/* Magnetic resolution in uT */
+			*val = 0;
+			*val2 = tmag5273_scale[data->version]
+					      [data->scale_index].val_micro;
+			return IIO_VAL_INT_PLUS_MICRO;
+		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;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_OFFSET:
+		switch (chan->type) {
+		case IIO_TEMP:
+			*val = -266314;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		*val = data->conv_avg;
+		return IIO_VAL_INT;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int tmag5273_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, int val,
+			      int val2, long mask)
+{
+	struct tmag5273_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		return tmag5273_write_osr(data, val);
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_MAGN:
+			if (val != 0)
+				return -EINVAL;
+			return tmag5273_write_scale(data, val2);
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+#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),		     \
+		.info_mask_shared_by_type_available =			     \
+				      BIT(IIO_CHAN_INFO_SCALE),		     \
+		.info_mask_shared_by_all =				     \
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+		.info_mask_shared_by_all_available =			     \
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+		.address = index,					     \
+		.scan_index = index,					     \
+		.scan_type = {						     \
+			.sign = 's',					     \
+			.realbits = 16,					     \
+			.storagebits = 16,				     \
+			.endianness = IIO_CPU,				     \
+		},							     \
+	}
+
+static const struct iio_chan_spec tmag5273_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE) |
+			BIT(IIO_CHAN_INFO_OFFSET),
+		.address = TEMPERATURE,
+		.scan_index = TEMPERATURE,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	TMAG5273_AXIS_CHANNEL(X, AXIS_X),
+	TMAG5273_AXIS_CHANNEL(Y, AXIS_Y),
+	TMAG5273_AXIS_CHANNEL(Z, AXIS_Z),
+	{
+		.type = IIO_ANGL,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all =
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all_available =
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.address = ANGLE,
+		.scan_index = ANGLE,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	{
+		.type = IIO_DISTANCE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_all =
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.info_mask_shared_by_all_available =
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.address = MAGNITUDE,
+		.scan_index = MAGNITUDE,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(6),
+};
+
+static const struct iio_info tmag5273_info = {
+	.read_avail = &tmag5273_read_avail,
+	.read_raw = &tmag5273_read_raw,
+	.write_raw = &tmag5273_write_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 void tmag5273_read_device_property(struct tmag5273_data *data)
+{
+	const char *str;
+	int ret;
+
+	data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;
+
+	if (!device_property_read_string(data->dev, "ti,angle-measurement", &str)) {
+		ret = match_string(tmag5273_angle_names,
+				   ARRAY_SIZE(tmag5273_angle_names), str);
+		if (ret < 0)
+			dev_warn(data->dev,
+				 "unexpected read angle-measurement property: %s\n", str);
+		else
+			data->angle_measurement = ret;
+	}
+}
+
+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_measurement));
+	if (ret)
+		return ret;
+	data->scale_index = MAGN_RANGE_LOW;
+
+	return regmap_write(data->map, TMAG5273_T_CONFIG, TMAG5273_T_CH_EN);
+}
+
+static int tmag5273_wake_up_and_check_device_id(struct tmag5273_data *data)
+{
+	__le16 devid;
+	int val, 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.
+	 */
+	regmap_read(data->map, TMAG5273_DEVICE_ID, &val);
+	usleep_range(80, 200);
+	ret = regmap_read(data->map, TMAG5273_DEVICE_ID, &val);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "failed to power on device\n");
+	data->version = FIELD_PREP(TMAG5273_VERSION_MASK, val);
+
+	ret = regmap_bulk_read(data->map, TMAG5273_MANUFACTURER_ID_LSB, &devid,
+			       sizeof(devid));
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "failed to read device ID\n");
+	data->devid = le16_to_cpu(devid);
+
+	switch (data->devid) {
+	case TMAG5273_MANUFACTURER_ID:
+		snprintf(data->name, sizeof(data->name), "tmag5273x%1u",
+			 data->version);
+		if (data->version < 1 || data->version > 2)
+			dev_warn(data->dev, "Unsupported device %s\n",
+				 data->name);
+		break;
+	default:
+		dev_warn(data->dev, "Unknown device ID 0x%x\n", data->devid);
+		break;
+	}
+
+	return 0;
+}
+
+static void tmag5273_power_down(void *data)
+{
+	tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_SLEEP);
+}
+
+static int tmag5273_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct tmag5273_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return dev_err_probe(dev, -ENOMEM,
+				     "IIO device allocation failed\n");
+
+	data = iio_priv(indio_dev);
+	data->dev = dev;
+	i2c_set_clientdata(i2c, indio_dev);
+
+	data->map = devm_regmap_init_i2c(i2c, &tmag5273_regmap_config);
+	if (IS_ERR(data->map))
+		return dev_err_probe(dev, PTR_ERR(data->map),
+				     "failed to allocate register map\n");
+
+	mutex_init(&data->lock);
+
+	ret = devm_regulator_get_enable(dev, "vcc");
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable regulator\n");
+
+	ret = tmag5273_wake_up_and_check_device_id(data);
+	if (ret)
+		return ret;
+
+	ret = tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_CONT);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to power on device\n");
+
+	/*
+	 * Register powerdown deferred callback which suspends the chip
+	 * after module unloaded.
+	 *
+	 * TMAG5273 should be in SUSPEND mode in the two cases:
+	 * 1) When driver is loaded, but we do not have any data or
+	 *    configuration requests to it (we are solving it using
+	 *    autosuspend feature).
+	 * 2) When driver is unloaded and device is not used (devm action is
+	 *    used in this case).
+	 */
+	ret = devm_add_action_or_reset(dev, tmag5273_power_down, data);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to add powerdown action\n");
+
+	ret = pm_runtime_set_active(dev);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_autosuspend_delay(dev, TMAG5273_AUTOSLEEP_DELAY_MS);
+	pm_runtime_use_autosuspend(dev);
+
+	tmag5273_read_device_property(data);
+
+	ret = tmag5273_chip_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to init device\n");
+
+	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_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, "device register failed\n");
+
+	return 0;
+}
+
+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);
+	int ret;
+
+	ret = tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_SLEEP);
+	if (ret)
+		dev_err(dev, "failed to power off device (%pe)\n",
+			ERR_PTR(ret));
+
+	return ret;
+}
+
+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;
+
+	/*
+	 * Time to go to stand-by mode from sleep mode is 50us
+	 * typically. During this time no I2C access is possible.
+	 */
+	tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_CONT);
+	usleep_range(80, 200);
+	ret = tmag5273_set_operating_mode(data, TMAG5273_OP_MODE_CONT);
+	if (ret)
+		dev_err(dev, "failed to power on device (%pe)\n", ERR_PTR(ret));
+
+	return ret;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(tmag5273_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_new = tmag5273_probe,
+	.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 v3 1/3] iio: add struct declarations for iio types

[Andy Shevchenko]

On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote:
> Add structs for iio type arrays such as IIO_AVAIL_LIST which can be used
> instead of int arrays.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

And thank you for doing this!

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
(one comment below)

> Signed-off-by: Gerald Loacker <gerald.loacker@wolfvision.net>
> ---
>  include/linux/iio/iio.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index f0ec8a5e5a7a..eaf6727445a6 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -383,6 +383,21 @@ s64 iio_get_time_ns(const struct iio_dev *indio_dev);
>  
>  struct iio_trigger; /* forward declaration */
>  
> +struct iio_val_int_plus_micro {
> +	int val_int;
> +	int val_micro;
> +};
> +
> +struct iio_val_int_plus_nano {
> +	int val_int;
> +	int val_nano;
> +};
> +
> +struct iio_val_int_plus_micro_db {
> +	int val_int;

	int val_int_db; ?

> +	int val_micro_db;
> +};

Actually why can't we simply do

typedef iio_val_int_plus_micro_db iio_val_int_plus_micro;

?

>  /**
>   * struct iio_info - constant information about device
>   * @event_attrs:	event control attributes
> -- 
> 2.37.2
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Nov 25, 2022 at 09:35:26AM +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.

Much better now, my comments below.

...

> +static int tmag5273_write_scale(struct tmag5273_data *data, int scale_micro)
> +{

What about

	u32 mask;

> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tmag5273_scale[0]); i++) {
> +		if (tmag5273_scale[data->version][i].val_micro == scale_micro)
> +			break;
> +	}
> +	if (i == ARRAY_SIZE(tmag5273_scale[0]))
> +		return -EINVAL;
> +	data->scale_index = i;

	if (data->scale_index == MAGN_RANGE_LOW)
		mask = 0;
	else
		mask = TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK;

> +	return regmap_update_bits(data->map,
> +		TMAG5273_SENSOR_CONFIG_2,
> +		TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK,

		mask);

> +		data->scale_index == MAGN_RANGE_LOW ? 0 :
> +			TMAG5273_Z_RANGE_MASK |
> +			TMAG5273_X_Y_RANGE_MASK);

?

> +}

...

> +		switch (chan->type) {
> +		case IIO_MAGN:

> +			if (val != 0)

			if (val)

> +				return -EINVAL;
> +			return tmag5273_write_scale(data, val2);
> +		default:
> +			return -EINVAL;
> +		}

...

> +static const struct regmap_config tmag5273_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,

> +	.max_register = 0xff,

Does it indeed have 256 registers?

> +	.volatile_reg = tmag5273_volatile_reg,
> +};

...

> +static void tmag5273_read_device_property(struct tmag5273_data *data)
> +{

	struct device *dev = data->dev;

> +	const char *str;
> +	int ret;
> +
> +	data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;

	ret = device_property_read_string(dev, "ti,angle-measurement", &str);
	if (ret)
		return;

> +	if (!device_property_read_string(data->dev, "ti,angle-measurement", &str)) {
> +		ret = match_string(tmag5273_angle_names,
> +				   ARRAY_SIZE(tmag5273_angle_names), str);
> +		if (ret < 0)
> +			dev_warn(data->dev,
> +				 "unexpected read angle-measurement property: %s\n", str);
> +		else
> +			data->angle_measurement = ret;
> +	}

	ret = match_string(tmag5273_angle_names, ARRAY_SIZE(tmag5273_angle_names), str);
	if (ret < 0)
		dev_warn(dev, "unexpected read angle-measurement property: %s\n", str);
	else
		data->angle_measurement = ret;

> +}

...

> +		return dev_err_probe(data->dev, ret,
> +				     "failed to power on device\n");

I would leave on one line (only 84 characters long).

...

> +		return dev_err_probe(data->dev, ret,
> +				     "failed to read device ID\n");

Ditto.

...

> +	switch (data->devid) {
> +	case TMAG5273_MANUFACTURER_ID:
> +		snprintf(data->name, sizeof(data->name), "tmag5273x%1u",
> +			 data->version);
> +		if (data->version < 1 || data->version > 2)
> +			dev_warn(data->dev, "Unsupported device %s\n",
> +				 data->name);
> +		break;
> +	default:
> +		dev_warn(data->dev, "Unknown device ID 0x%x\n", data->devid);
> +		break;
> +	}
> +
> +	return 0;

'break;':s above can be replaced by direct 'return 0;':s. It's up to you.

...

> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "IIO device allocation failed\n");

We don't print ENOMEM error messages in the drivers, core does this for us.
Otherwise you have to explain why this message is so important.

...

> +	/*
> +	 * Register powerdown deferred callback which suspends the chip
> +	 * after module unloaded.
> +	 *
> +	 * TMAG5273 should be in SUSPEND mode in the two cases:
> +	 * 1) When driver is loaded, but we do not have any data or
> +	 *    configuration requests to it (we are solving it using
> +	 *    autosuspend feature).
> +	 * 2) When driver is unloaded and device is not used (devm action is

Something with indentation of this or other lines.

> +	 *    used in this case).
> +	 */

...

> +		return dev_err_probe(dev, ret,
> +				     "failed to add powerdown action\n");

One line?

...

> +		dev_err(dev, "failed to power off device (%pe)\n",
> +			ERR_PTR(ret));

Ditto.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Nov 25, 2022 at 12:59:01PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 25, 2022 at 09:35:26AM +0100, Gerald Loacker wrote:

...

> > +static int tmag5273_write_scale(struct tmag5273_data *data, int scale_micro)
> > +{
> 
> What about
> 
> 	u32 mask;

After looking again, I guess it should be

	u32 value;

> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(tmag5273_scale[0]); i++) {
> > +		if (tmag5273_scale[data->version][i].val_micro == scale_micro)
> > +			break;
> > +	}
> > +	if (i == ARRAY_SIZE(tmag5273_scale[0]))
> > +		return -EINVAL;
> > +	data->scale_index = i;
> 
> 	if (data->scale_index == MAGN_RANGE_LOW)
> 		mask = 0;
> 	else
> 		mask = TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK;
> 
> > +	return regmap_update_bits(data->map,
> > +		TMAG5273_SENSOR_CONFIG_2,
> > +		TMAG5273_Z_RANGE_MASK | TMAG5273_X_Y_RANGE_MASK,
> 
> 		mask);
> 
> > +		data->scale_index == MAGN_RANGE_LOW ? 0 :
> > +			TMAG5273_Z_RANGE_MASK |
> > +			TMAG5273_X_Y_RANGE_MASK);
> 
> ?
> 
> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] iio: add struct declarations for iio types

[Andy Shevchenko]

On Fri, Nov 25, 2022 at 12:45:06PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote:

...

> > +struct iio_val_int_plus_micro {
> > +	int val_int;
> > +	int val_micro;
> > +};

Thinking more about naming, why not drop val_ completely?

	int integer;
	int micro;

?

> > +struct iio_val_int_plus_nano {
> > +	int val_int;
> > +	int val_nano;
> > +};
> > +
> > +struct iio_val_int_plus_micro_db {
> > +	int val_int;
> 
> 	int val_int_db; ?
> 
> > +	int val_micro_db;
> > +};
> 
> Actually why can't we simply do
> 
> typedef iio_val_int_plus_micro_db iio_val_int_plus_micro;
> 
> ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] iio: add struct declarations for iio types

[Gerald Loacker]

Am 25.11.2022 um 12:01 schrieb Andy Shevchenko:
> On Fri, Nov 25, 2022 at 12:45:06PM +0200, Andy Shevchenko wrote:
>> On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote:
> 
> ...
> 
>>> +struct iio_val_int_plus_micro {
>>> +	int val_int;
>>> +	int val_micro;
>>> +};
> 
> Thinking more about naming, why not drop val_ completely?
> 
> 	int integer;
> 	int micro;
> 
> ?
> 

Yes, this sounds good to me. I think of adding only

	typedef struct {
		int integer;
		int micro;
	} iio_val_int_plus_micro;

for now, and one can add similar structures when needed, like

	typedef struct {
		int integer;
		int nano;
	} iio_val_int_plus_nano;

or
	
	typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;

If you think it's better to add them all, I can do that, of course.

>>> +struct iio_val_int_plus_nano {
>>> +	int val_int;
>>> +	int val_nano;
>>> +};
>>> +
>>> +struct iio_val_int_plus_micro_db {
>>> +	int val_int;
>>
>> 	int val_int_db; ?
>>
>>> +	int val_micro_db;
>>> +};
>>
>> Actually why can't we simply do
>>
>> typedef iio_val_int_plus_micro_db iio_val_int_plus_micro;
>>
>> ?
> 

Re: [PATCH v3 1/3] iio: add struct declarations for iio types

[Andy Shevchenko]

On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote:
> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko:
> > On Fri, Nov 25, 2022 at 12:45:06PM +0200, Andy Shevchenko wrote:
> >> On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote:

...

> >>> +struct iio_val_int_plus_micro {
> >>> +	int val_int;
> >>> +	int val_micro;
> >>> +};
> > 
> > Thinking more about naming, why not drop val_ completely?
> > 
> > 	int integer;
> > 	int micro;
> > 
> > ?
> 
> Yes, this sounds good to me. I think of adding only
> 
> 	typedef struct {
> 		int integer;
> 		int micro;
> 	} iio_val_int_plus_micro;
> 
> for now, and one can add similar structures when needed, like
> 
> 	typedef struct {
> 		int integer;
> 		int nano;
> 	} iio_val_int_plus_nano;

It's a rule to use _t for typedef:s in the kernel. That's why
I suggested to leave struct definition and only typedef the same structures
(existing) to new names (if needed).

> or

> 	typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;

This is better as explained above.

> If you think it's better to add them all, I can do that, of course.
> 
> >>> +struct iio_val_int_plus_nano {
> >>> +	int val_int;
> >>> +	int val_nano;
> >>> +};
> >>> +
> >>> +struct iio_val_int_plus_micro_db {
> >>> +	int val_int;
> >>
> >> 	int val_int_db; ?
> >>
> >>> +	int val_micro_db;
> >>> +};
> >>
> >> Actually why can't we simply do
> >>
> >> typedef iio_val_int_plus_micro_db iio_val_int_plus_micro;
> >>
> >> ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] iio: add struct declarations for iio types

[Michael Riesch]

Hi Gerald, Andy,

On 11/28/22 14:27, Andy Shevchenko wrote:
> On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote:
>> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko:
>>> On Fri, Nov 25, 2022 at 12:45:06PM +0200, Andy Shevchenko wrote:
>>>> On Fri, Nov 25, 2022 at 09:35:24AM +0100, Gerald Loacker wrote:
> 
> ...
> 
>>>>> +struct iio_val_int_plus_micro {
>>>>> +	int val_int;
>>>>> +	int val_micro;
>>>>> +};
>>>
>>> Thinking more about naming, why not drop val_ completely?
>>>
>>> 	int integer;
>>> 	int micro;
>>>
>>> ?
>>
>> Yes, this sounds good to me. I think of adding only
>>
>> 	typedef struct {
>> 		int integer;
>> 		int micro;
>> 	} iio_val_int_plus_micro;

I think we actually want

struct iio_val_int_plus_micro {
	int integer;
	int micro;
};

here, right?

>> for now, and one can add similar structures when needed, like
>>
>> 	typedef struct {
>> 		int integer;
>> 		int nano;
>> 	} iio_val_int_plus_nano;

+1 for introducing things when they are actually used.

> It's a rule to use _t for typedef:s in the kernel. That's why
> I suggested to leave struct definition and only typedef the same structures
> (existing) to new names (if needed).

Andy, excuse our ignorance but we are not sure how this typedef approach
is supposed to look like...

>> or
> 
>> 	typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;

... because

#include <stdio.h>

struct iio_val_int_plus_micro {
	int integer;
	int micro;
};

typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;

int main()
{
  struct iio_val_int_plus_micro a = { .integer = 100, .micro = 10, };
  struct iio_val_int_plus_micro_db b = { .integer = 20, .micro = 10, };
  return 0;
}

won't compile.

> This is better as explained above.
> 
>> If you think it's better to add them all, I can do that, of course.

Anyway, seeing that only struct iio_val_int_plus_micro is used at the
moment, I believe the best path forward is to introduce only this struct
and move on.

Best regards,
Michael

>>>>> +struct iio_val_int_plus_nano {
>>>>> +	int val_int;
>>>>> +	int val_nano;
>>>>> +};
>>>>> +
>>>>> +struct iio_val_int_plus_micro_db {
>>>>> +	int val_int;
>>>>
>>>> 	int val_int_db; ?
>>>>
>>>>> +	int val_micro_db;
>>>>> +};
>>>>
>>>> Actually why can't we simply do
>>>>
>>>> typedef iio_val_int_plus_micro_db iio_val_int_plus_micro;
>>>>
>>>> ?
> 

Re: [PATCH v3 1/3] iio: add struct declarations for iio types

[Andy Shevchenko]

On Mon, Nov 28, 2022 at 02:48:48PM +0100, Michael Riesch wrote:
> On 11/28/22 14:27, Andy Shevchenko wrote:
> > On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote:
> >> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko:

...

> > It's a rule to use _t for typedef:s in the kernel. That's why
> > I suggested to leave struct definition and only typedef the same structures
> > (existing) to new names (if needed).
> 
> Andy, excuse our ignorance but we are not sure how this typedef approach
> is supposed to look like...
> 
> >> or
> > 
> >> 	typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;
> 
> ... because
> 
> #include <stdio.h>
> 
> struct iio_val_int_plus_micro {
> 	int integer;
> 	int micro;
> };
> 
> typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;
> 
> int main()
> {
>   struct iio_val_int_plus_micro a = { .integer = 100, .micro = 10, };
>   struct iio_val_int_plus_micro_db b = { .integer = 20, .micro = 10, };
>   return 0;
> }
> 
> won't compile.

I see. Thanks for pointing this out.

Then the question is why do we need the two same structures with different
names?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/3] iio: add struct declarations for iio types

[Michael Riesch]

Hi Andy,

On 11/28/22 15:05, Andy Shevchenko wrote:
> On Mon, Nov 28, 2022 at 02:48:48PM +0100, Michael Riesch wrote:
>> On 11/28/22 14:27, Andy Shevchenko wrote:
>>> On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote:
>>>> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko:
> 
> ...
> 
>>> It's a rule to use _t for typedef:s in the kernel. That's why
>>> I suggested to leave struct definition and only typedef the same structures
>>> (existing) to new names (if needed).
>>
>> Andy, excuse our ignorance but we are not sure how this typedef approach
>> is supposed to look like...
>>
>>>> or
>>>
>>>> 	typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;
>>
>> ... because
>>
>> #include <stdio.h>
>>
>> struct iio_val_int_plus_micro {
>> 	int integer;
>> 	int micro;
>> };
>>
>> typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;
>>
>> int main()
>> {
>>   struct iio_val_int_plus_micro a = { .integer = 100, .micro = 10, };
>>   struct iio_val_int_plus_micro_db b = { .integer = 20, .micro = 10, };
>>   return 0;
>> }
>>
>> won't compile.
> 
> I see. Thanks for pointing this out.
> 
> Then the question is why do we need the two same structures with different
> names?

Most probably we don't need "struct iio_val_int_plus_micro_db" at all
since IIO_VAL_INT_PLUS_MICRO_DB and IIO_VAL_INT_PLUS_MICRO get the same
treatment in industrialio-core.c. At least it should not be introduced
in the scope of this series. In the end this is up to whoever writes the
first driver using the common data structures and IIO_VAL_INT_PLUS_MICRO_DB.

Best regards,
Michael

Re: [PATCH v3 1/3] iio: add struct declarations for iio types

[Jonathan Cameron]

On Mon, 28 Nov 2022 15:26:51 +0100
Michael Riesch <michael.riesch@wolfvision.net> wrote:

> Hi Andy,
> 
> On 11/28/22 15:05, Andy Shevchenko wrote:
> > On Mon, Nov 28, 2022 at 02:48:48PM +0100, Michael Riesch wrote:  
> >> On 11/28/22 14:27, Andy Shevchenko wrote:  
> >>> On Mon, Nov 28, 2022 at 01:18:04PM +0100, Gerald Loacker wrote:  
> >>>> Am 25.11.2022 um 12:01 schrieb Andy Shevchenko:  
> > 
> > ...
> >   
> >>> It's a rule to use _t for typedef:s in the kernel. That's why
> >>> I suggested to leave struct definition and only typedef the same structures
> >>> (existing) to new names (if needed).  
> >>
> >> Andy, excuse our ignorance but we are not sure how this typedef approach
> >> is supposed to look like...
> >>  
> >>>> or  
> >>>  
> >>>> 	typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;  
> >>
> >> ... because
> >>
> >> #include <stdio.h>
> >>
> >> struct iio_val_int_plus_micro {
> >> 	int integer;
> >> 	int micro;
> >> };
> >>
> >> typedef iio_val_int_plus_micro iio_val_int_plus_micro_db;
> >>
> >> int main()
> >> {
> >>   struct iio_val_int_plus_micro a = { .integer = 100, .micro = 10, };
> >>   struct iio_val_int_plus_micro_db b = { .integer = 20, .micro = 10, };
> >>   return 0;
> >> }
> >>
> >> won't compile.  
> > 
> > I see. Thanks for pointing this out.
> > 
> > Then the question is why do we need the two same structures with different
> > names?  
> 
> Most probably we don't need "struct iio_val_int_plus_micro_db" at all
> since IIO_VAL_INT_PLUS_MICRO_DB and IIO_VAL_INT_PLUS_MICRO get the same
> treatment in industrialio-core.c. At least it should not be introduced
> in the scope of this series. In the end this is up to whoever writes the
> first driver using the common data structures and IIO_VAL_INT_PLUS_MICRO_DB.

They get same treatment today because we don't attempt to deal with
IIO_VAL_INT_PLUS_MICRO_DB in conjunction with any of the analog circuit type
front ends yet. Mind you, even though the handling in iio-rescale.c will be
different if anyone ever adds support for the DB form (I shudder at the maths
of combining this with other scale factors), I don't see the possibility meaning
we need a different structure.  

Jonathan


> 
> Best regards,
> Michael
>