Skip to contentSkip to conversation

PATCH Superseded Nikhil Gautam 5 participants 9 messages Jun 27 – Jun 29

Subsystems (2)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

Subject: [PATCH v3 0/2] iio: magnetometer: add support for Melexis MLX90393

Hi,

This series adds initial Industrial I/O subsystem support for the Melexis MLX90393 3-axis magnetometer and temperature sensor.

The MLX90393 supports both I2C and SPI interfaces. This series implements support for the I2C interface while keeping the driver structure transport-independent to simplify future SPI support.

The device uses a command-based communication protocol rather than a conventional register-addressed interface. A small transport abstraction layer is therefore used instead of regmap to share the common sensor logic between the current I2C implementation and future SPI support.

Currently supported features:

* Raw magnetic field measurements for X/Y/Z axes
* Raw temperature measurements
* Configurable gain/scale selection
* Configurable oversampling ratio
* Direct mode operation through the IIO subsystem
* I2C interface support

The driver has been tested on Raspberry Pi 5 hardware using an MLX90393 sensor connected over I2C. Magnetic field and temperature measurements were verified through the IIO sysfs interface.

Datasheet:
https://www.melexis.com/en/documents/documentation/datasheets/datasheet-mlx90393

Previous submission:

Changes in v3:
[DT]

* Made vdd-supply and vddio-supply required properties.
* Moved the MAINTAINERS entry to the correct alphabetical position.

[IIO]

* Added the publicly available MLX90393 datasheet reference.
* Clarified the motivation for introducing a dedicated MLX90393 driver.
* Added the I2C device ID table and switched to designated initializers.
* Added a forward declaration for struct device to reduce unnecessary header dependencies.
* Added symbol namespaces.
* Improved include usage.
* Improved status validation and consistency of conditional checks.
* Added comments documenting protocol encoding, initialization delays, and temperature conversion constants derived from the datasheet.
* Improved naming, formatting, indentation and general coding style.
* Addressed all review comments from Andy Shevchenko,
Uwe Kleine-König and Krzysztof Kozlowski.

Changes in v2:
[DT]

* Extended the DT binding to document power supply regulators and optional interrupt and trigger GPIOs.

[IIO]

* Removed the RFC tag based on reviewer feedback.
* Added a MAINTAINERS entry as part of the initial submission and expanded it in the driver patch.
* Reworked the scale availability implementation to simplify the data layout and eliminate the need for constructing a temporary table, avoiding potential race conditions.
* Replaced usleep_range() with fsleep() where appropriate and documented initialization delays.
* Simplified helper functions and improved error handling by returning directly where appropriate.
* Reduced unnecessary local variables and line wrapping to improve readability and align with kernel coding style.
* Added comments for lock protection and command definitions to improve code clarity.
* Switched to devm_mutex_init() and cleaned up include usage in accordance with the "include what you use" principle.
* Improved consistency across the driver, including conditional handling, switch statements, formatting and general code style.
* Addressed all review comments from Jonathan Cameron.

Many thanks to Jonathan Cameron, Andy Shevchenko,
Uwe Kleine-König and Krzysztof Kozlowski for their detailed reviews and valuable feedback, which significantly improved this series.

Further review and comments are greatly appreciated.

Thanks,
Nikhil Gautam

Nikhil Gautam (2):
dt-bindings: iio: magnetometer: add Melexis MLX90393
iio: magnetometer: add support for Melexis MLX90393

 .../iio/magnetometer/melexis,mlx90393.yaml    |  54 ++
 MAINTAINERS                                   |   7 +
 drivers/iio/magnetometer/Kconfig              |  10 +
 drivers/iio/magnetometer/Makefile             |   2 +
 drivers/iio/magnetometer/mlx90393.h           |  73 ++
 drivers/iio/magnetometer/mlx90393_core.c      | 701 ++++++++++++++++++
 drivers/iio/magnetometer/mlx90393_i2c.c       |  76 ++
 7 files changed, 923 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml
 create mode 100644 drivers/iio/magnetometer/mlx90393.h
 create mode 100644 drivers/iio/magnetometer/mlx90393_core.c
 create mode 100644 drivers/iio/magnetometer/mlx90393_i2c.c
Signature · 1 line
2.39.5
#1 lore raw

PATCH v3 1/2 dt-bindings: iio: magnetometer: add Melexis MLX90393

Add devicetree bindings for the Melexis MLX90393
3-axis magnetometer and temperature sensor.

The device supports magnetic field and temperature measurements over I2C and SPI interfaces.

This initial binding documents the I2C interface.

---
 .../iio/magnetometer/melexis,mlx90393.yaml    | 54 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml
Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml new file +54 -0
new file mode 100644
index 000000000000..33ab0be66345
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/magnetometer/melexis,mlx90393.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Melexis MLX90393 magnetometer sensor
+
+maintainers:
+  - Nikhil Gautam <[email protected]>
+
+description:
+  Melexis MLX90393 3-axis magnetometer and temperature sensor.
+
+properties:
+  compatible:
+    const: melexis,mlx90393
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  trigger-gpios:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - vddio-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        magnetometer@c {
+            compatible = "melexis,mlx90393";
+            reg = <0x0c>;
+
+            interrupt-parent = <&gpio>;
+            interrupts = <17 IRQ_TYPE_EDGE_RISING>;
+
+            trigger-gpios = <&gpio 18 GPIO_ACTIVE_HIGH>;
+        };
+    };
MAINTAINERS +6 -0
index a92290fffa16..469ae3f9aa0f 100644
@@ -15501,6 +15501,12 @@ F:	Documentation/scsi/megaraid.rst
 F:	drivers/scsi/megaraid.*
 F:	drivers/scsi/megaraid/
 
+MELEXIS MLX90393 MAGNETOMETER DRIVER
+M:	Nikhil Gautam <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml
+
 MELEXIS MLX90614 DRIVER
 M:	Crt Mori <[email protected]>
 L:	[email protected]
Signature · 1 line
2.39.5
#2 lore raw

Re: [PATCH v3 1/2] dt-bindings: iio: magnetometer: add Melexis MLX90393

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The required properties `vdd-supply` and `vddio-supply` are omitted from the `properties:` block…

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The required properties vdd-supply and vddio-supply are omitted from the properties: block and missing from the examples: node, causing schema validation failures. --

commit c1fe5f8c39a4b8113c50e154a6674ac296637f70
Author: Nikhil Gautam <[email protected]>

dt-bindings: iio: magnetometer: add Melexis MLX90393

Add devicetree bindings for the Melexis MLX90393 3-axis magnetometer and temperature sensor.

Quoted 5 lines
Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml new file +0 -0
new file mode 100644
index 0000000000000..33ab0be663450

[ ... ]

Quoted 10 lines
Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml new file +10 -0
+properties:
+  compatible:
+    const: melexis,mlx90393
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+

Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml line 34

Quoted 10 lines
Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml new file +10 -0
+  trigger-gpios:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - vddio-supply
+
+additionalProperties: false

[Severity: Low]
Will this binding fail schema validation when running dt_binding_check?

The vdd-supply and vddio-supply properties are listed as required, but they are not defined in the properties block.

Because additionalProperties is set to false, wouldn't the schema reject these supply properties during validation?

Quoted 9 lines
Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml new file +9 -0
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+

Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml line 54

Quoted 10 lines
Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml new file +10 -0
+        magnetometer@c {
+            compatible = "melexis,mlx90393";
+            reg = <0x0c>;
+
+            interrupt-parent = <&gpio>;
+            interrupts = <17 IRQ_TYPE_EDGE_RISING>;
+
+            trigger-gpios = <&gpio 18 GPIO_ACTIVE_HIGH>;
+        };
+    };

[Severity: Low]
Does this example node need to include the vdd-supply and vddio-supply properties to pass validation, since they are marked as required above?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
#3 lore raw

Re: [PATCH v3 1/2] dt-bindings: iio: magnetometer: add Melexis MLX90393

In reply to Nikhil Gautam
Quoted 6 lines · Nikhil Gautam

Add devicetree bindings for the Melexis MLX90393
3-axis magnetometer and temperature sensor.

The device supports magnetic field and temperature measurements over I2C and SPI interfaces.

Quoted 9 lines

This initial binding documents the I2C interface.

---
 .../iio/magnetometer/melexis,mlx90393.yaml    | 54 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml

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

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.example.dtb: magnetometer@c (melexis,mlx90393): 'vdd-supply' is a required property
from schema $id: http://devicetree.org/schemas/iio/magnetometer/melexis,mlx90393.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.example.dtb: magnetometer@c (melexis,mlx90393): 'vddio-supply' is a required property
from schema $id: http://devicetree.org/schemas/iio/magnetometer/melexis,mlx90393.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.kernel.org/project/devicetree/patch/[email protected]

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

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

pip3 install dtschema --upgrade

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

reviewer
#4 lore raw

Re: [PATCH v3 1/2] dt-bindings: iio: magnetometer: add Melexis MLX90393

In reply to Nikhil Gautam
Quoted 43 lines · Nikhil Gautam

Add devicetree bindings for the Melexis MLX90393
3-axis magnetometer and temperature sensor.

The device supports magnetic field and temperature measurements over I2C and SPI interfaces.

This initial binding documents the I2C interface.

---
 .../iio/magnetometer/melexis,mlx90393.yaml    | 54 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml
Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml new file +22 -0
new file mode 100644
index 000000000000..33ab0be66345
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/magnetometer/melexis,mlx90393.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Melexis MLX90393 magnetometer sensor
+
+maintainers:
+  - Nikhil Gautam <[email protected]>
+
+description:
+  Melexis MLX90393 3-axis magnetometer and temperature sensor.
+
+properties:
+  compatible:
+    const: melexis,mlx90393
+
+  reg:
+    maxItems: 1
+
+  interrupts:

Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml line 32

Quoted 10 lines
Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml new file +10 -0
+    maxItems: 1
+
+  trigger-gpios:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - vddio-supply

The supplies are required but are missing from the properties and the example. Running make dt_binding_check before sending would have caught this.

See: https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html

Quoted 39 lines
Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml new file +22 -0
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        magnetometer@c {
+            compatible = "melexis,mlx90393";
+            reg = <0x0c>;
+
+            interrupt-parent = <&gpio>;
+            interrupts = <17 IRQ_TYPE_EDGE_RISING>;
+
+            trigger-gpios = <&gpio 18 GPIO_ACTIVE_HIGH>;
+        };
+    };
MAINTAINERS +6 -0
index a92290fffa16..469ae3f9aa0f 100644
@@ -15501,6 +15501,12 @@ F:	Documentation/scsi/megaraid.rst
 F:	drivers/scsi/megaraid.*
 F:	drivers/scsi/megaraid/
 
+MELEXIS MLX90393 MAGNETOMETER DRIVER
+M:	Nikhil Gautam <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml
+
 MELEXIS MLX90614 DRIVER
 M:	Crt Mori <[email protected]>
 L:	[email protected]
#5 lore raw

PATCH v3 2/2 iio: magnetometer: add support for Melexis MLX90393

In reply to Nikhil Gautam

Add Industrial I/O subsystem support for the Melexis MLX90393 3-axis magnetometer and temperature sensor.

The driver currently supports:

raw magnetic field measurements
raw temperature measurements
configurable gain/scale selection
configurable oversampling ratio
direct mode operation

The MLX90393 supports both I2C and SPI interfaces. This initial implementation adds support for the I2C interface.

The device uses a command-based communication protocol rather than a conventional register-addressed interface. A small transport abstraction layer is therefore used instead of regmap to share the common sensor logic between the current I2C implementation and future SPI support without duplicating code.

---
 MAINTAINERS                              |   1 +
 drivers/iio/magnetometer/Kconfig         |  10 +
 drivers/iio/magnetometer/Makefile        |   2 +
 drivers/iio/magnetometer/mlx90393.h      |  73 +++
 drivers/iio/magnetometer/mlx90393_core.c | 701 +++++++++++++++++++++++
 drivers/iio/magnetometer/mlx90393_i2c.c  |  76 +++
 6 files changed, 863 insertions(+)
 create mode 100644 drivers/iio/magnetometer/mlx90393.h
 create mode 100644 drivers/iio/magnetometer/mlx90393_core.c
 create mode 100644 drivers/iio/magnetometer/mlx90393_i2c.c
MAINTAINERS +1 -0
index 469ae3f9aa0f..177dda261801 100644
@@ -15506,6 +15506,7 @@ M:	Nikhil Gautam <[email protected]>
 L:	[email protected]
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/magnetometer/melexis,mlx90393.yaml
+F:	drivers/iio/magnetometer/mlx90393*
 
 MELEXIS MLX90614 DRIVER
 M:	Crt Mori <[email protected]>
drivers/iio/magnetometer/Kconfig +10 -0
index 3debf1320ad1..e6b74e7e3317 100644
@@ -128,6 +128,16 @@ config HID_SENSOR_MAGNETOMETER_3D
 	  Say yes here to build support for the HID SENSOR
 	  Magnetometer 3D.
 
+config MLX90393
+	tristate "MELEXIS MLX90393 3-axis magnetometer sensor"
+	depends on I2C
+	help
+	  Say yes here to build support for the MELEXIS MLX90393 3-axis
+	  magnetometer.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mlx90393.
+
 config MMC35240
 	tristate "MEMSIC MMC35240 3-axis magnetic sensor"
 	select REGMAP_I2C
drivers/iio/magnetometer/Makefile +2 -0
index 9297723a97d8..542c89d38a59 100644
@@ -14,6 +14,8 @@ obj-$(CONFIG_BMC150_MAGN_SPI) += bmc150_magn_spi.o
 
 obj-$(CONFIG_MAG3110)	+= mag3110.o
 obj-$(CONFIG_HID_SENSOR_MAGNETOMETER_3D) += hid-sensor-magn-3d.o
+obj-$(CONFIG_MLX90393)		+= mlx90393_core.o
+obj-$(CONFIG_MLX90393)		+= mlx90393_i2c.o
 obj-$(CONFIG_MMC35240)	+= mmc35240.o
 
 obj-$(CONFIG_IIO_ST_MAGN_3AXIS) += st_magn.o
drivers/iio/magnetometer/mlx90393.h new file +73 -0

73 lines not on this page · raw message

drivers/iio/magnetometer/mlx90393_core.c new file +701 -0

701 lines not on this page · raw message

drivers/iio/magnetometer/mlx90393_i2c.c new file +76 -0

76 lines not on this page · raw message

Signature · 1 line
2.39.5
#6 lore raw

Re: [PATCH v3 2/2] iio: magnetometer: add support for Melexis MLX90393

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider: - [High] TOCTOU data race in `mlx90393_read_avail` due to static array. - [High] Stack-allocated buffers…

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] TOCTOU data race in mlx90393_read_avail due to static array.
- [High] Stack-allocated buffers passed to I2C transport violate DMA rules.
- [High] mlx90393_check_status rejects valid responses for 4-axis reads.
- [Medium] Writing to in_temp_scale silently corrupts magnetometer gain configuration.
- [Medium] Incorrect physical units exposed for IIO scales (ABI violation).
- [Medium] Temperature oversampling ratio exposes internal register indices (ABI violation).
- [Low] Use of floating-point literal in fsleep duration.
- [Low] Misuse of MLX90393_AXIS_MAX for scale array length.
- [Low] Data race on configuration variables in mlx90393_read_raw.
--

commit 615b644e2712a4949015c117e64d449d197438d8
Author: Nikhil Gautam <[email protected]>

iio: magnetometer: add support for Melexis MLX90393

Add Industrial I/O subsystem support for the Melexis MLX90393 3-axis magnetometer and temperature sensor.

Quoted 4 lines
drivers/iio/magnetometer/mlx90393_core.c new file +0 -0
index 0000000000000..78fcab8a2b566 100644

[ ... ]

drivers/iio/magnetometer/mlx90393_core.c line 84

Quoted 3 lines
drivers/iio/magnetometer/mlx90393_core.c new file +3 -0
+static const int mlx90393_osr2_avail[MLX90393_OSR2_MAX] = {
+	0, 1, 2, 3,
+};

[Severity: Medium]
Should this array expose the physical oversampling ratios (e.g., 1, 2, 4, 8) rather than the internal register values, to comply with IIO ABI standards?

[ ... ]

Quoted 6 lines
drivers/iio/magnetometer/mlx90393_core.c new file +6 -0
+static int mlx90393_check_status(u8 cmd, u8 status)
+{
+	/* Datasheet: Table 12: Status byte definition */
+
+	/* Always validate error bit */
+	if (status & MLX90393_STATUS_ERROR)

drivers/iio/magnetometer/mlx90393_core.c line 183

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +10 -0
+		return -EIO;
+
+	switch (cmd & MLX90393_CMD_MASK) {
+	case MLX90393_CMD_RM:
+		/*
+		 * D1:D0 indicates response availability
+		 * 00 means invalid/no measurement
+		 */
+		if ((status & MLX90393_STATUS_RESP) == 0)
+			return -EIO;

[Severity: High]
Does this validation break 4-axis reads?

If MLX90393_MEASURE_ALL requests 4 words, could the 2-bit counter overflow to 00, causing the driver to incorrectly reject valid responses?

[ ... ]

drivers/iio/magnetometer/mlx90393_core.c line 253

Quoted 12 lines
drivers/iio/magnetometer/mlx90393_core.c new file +12 -0
+static int mlx90393_write_reg(struct mlx90393_data *data, u8 reg, u16 val)
+{
+	u8 tx[4];
+	u8 status;
+	int ret;
+
+	tx[0] = MLX90393_CMD_WR;
+	put_unaligned_be16(val, &tx[1]);
+	/* Register address is encoded in bits [7:2] */
+	tx[3] = reg << 2;
+
+	ret = mlx90393_xfer(data, tx, sizeof(tx), &status, 1);

[Severity: High]
Can passing stack-allocated buffers to the I2C transport cause memory corruption or a kernel panic?

If the underlying I2C controller uses DMA, mapping stack memory (which might be CONFIG_VMAP_STACK) is generally not allowed. This also applies to the rx buffer in mlx90393_read_measurement().

[ ... ]

Quoted 9 lines
drivers/iio/magnetometer/mlx90393_core.c new file +9 -0
+static int mlx90393_get_scale(struct mlx90393_data *data,
+			      const struct iio_chan_spec *chan,
+			      int *val, int *val2)
+{
+	enum mlx90393_axis_type axis;
+	u8 res;
+
+	if (chan->channel2 == IIO_MOD_Z) {
+		axis = MLX90393_AXIS_TYPE_Z;

drivers/iio/magnetometer/mlx90393_core.c line 340

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +10 -0
+		res = data->res_z;
+	} else {
+		axis = MLX90393_AXIS_TYPE_XY;
+		res = data->res_xy;
+	}
+
+	*val = 0;
+	*val2 = mlx90393_scale_table[axis][data->gain_sel][res];
+
+	return IIO_VAL_INT_PLUS_NANO;

[Severity: Medium]
Do these return values violate the IIO ABI?

Is the magnetometer scale exposing Tesla (nano scale) instead of Gauss?

[ ... ]

drivers/iio/magnetometer/mlx90393_core.c line 486

Quoted 12 lines
drivers/iio/magnetometer/mlx90393_core.c new file +12 -0
+static int mlx90393_write_raw(struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan,
+			      int val, int val2,
+			      long mask)
+{
+	struct mlx90393_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE: {
+		guard(mutex)(&data->lock);
+		return mlx90393_set_scale(data, chan, val, val2);
+	}

[Severity: Medium]
Does this lack of chan->type checking mean writes to the temperature scale sysfs attribute will fall through to mlx90393_set_scale()?

Could this unintentionally alter the magnetometer gain configuration?

[ ... ]

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +10 -0
+static int mlx90393_read_raw(struct iio_dev *indio_dev,
+			     const struct iio_chan_spec *chan,
+			     int *val, int *val2, long mask)
+{
+	struct mlx90393_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW: {
+		guard(mutex)(&data->lock);

drivers/iio/magnetometer/mlx90393_core.c line 525

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +10 -0
+		ret = mlx90393_read_measurement(data, chan->address, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_MAGN:
+			return mlx90393_get_scale(data, chan, val, val2);

[Severity: Low]
Does reading driver configuration fields like data->gain_sel in mlx90393_get_scale() without holding data->lock introduce a data race with concurrent sysfs writes in mlx90393_write_raw()?

drivers/iio/magnetometer/mlx90393_core.c line 533

Quoted 8 lines
drivers/iio/magnetometer/mlx90393_core.c new file +8 -0
+
+		case IIO_TEMP:
+			/*
+			 * Datasheet Table 7: Thermal Specification
+			 */
+			*val = 0;
+			*val2 = 22124;
+			return IIO_VAL_INT_PLUS_MICRO;

[Severity: Medium]
Does this violate the IIO ABI by exposing Celsius instead of milli-Celsius?

[ ... ]

drivers/iio/magnetometer/mlx90393_core.c line 571

Quoted 9 lines
drivers/iio/magnetometer/mlx90393_core.c new file +9 -0
+static int mlx90393_read_avail(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       const int **vals,
+			       int *type,
+			       int *length,
+			       long mask)
+{
+	struct mlx90393_data *data = iio_priv(indio_dev);
+	static int scale_avail[MLX90393_GAIN_MAX][MLX90393_AXIS_MAX];

[Severity: High]
Is it safe to populate and return a pointer to a static array here?

Since the IIO core reads this array outside the driver's lock, couldn't a concurrent sysfs read from another thread or device overwrite the array contents while it's being read?

[ ... ]

drivers/iio/magnetometer/mlx90393_core.c line 589

Quoted 3 lines
drivers/iio/magnetometer/mlx90393_core.c new file +3 -0
+		*vals = &scale_avail[0][0];
+		*type = IIO_VAL_INT_PLUS_NANO;
+		*length = MLX90393_GAIN_MAX * MLX90393_AXIS_MAX;

[Severity: Low]
Is MLX90393_AXIS_MAX being misused here to calculate the length?

While it evaluates to 2, shouldn't length represent the number of values (entries * 2 elements per IIO value pair) rather than axis count?

[ ... ]

Quoted 16 lines
drivers/iio/magnetometer/mlx90393_core.c new file +16 -0
+static int mlx90393_init(struct mlx90393_data *data)
+{
+	int ret;
+	u16 reg;
+
+	/* Exit mode */
+	ret = mlx90393_write_cmd(data, MLX90393_CMD_EX);
+	if (ret)
+		return ret;
+
+	/*
+	 * Datasheet section 15.4.1.2 (RT command), Figure 16:
+	 * Wait 1 ms after EX command before issuing RT.
+	 */
+	fsleep(1 * USEC_PER_MSEC);
+

drivers/iio/magnetometer/mlx90393_core.c line 642

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +10 -0
+	/* Reset device */
+	ret = mlx90393_write_cmd(data, MLX90393_CMD_RT);
+	if (ret)
+		return ret;
+
+	/*
+	 * Datasheet section 15.4.1.2 (RT command), Figure 16:
+	 * Wait 1.5 ms for the start-up sequence to complete.
+	 */
+	fsleep(1.5 * USEC_PER_MSEC);

[Severity: Low]
Could using a floating-point literal (1.5) cause build or linkage errors on architectures where hardware floating-point arithmetic is not supported in the kernel?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
#7 lore raw

Re: [PATCH v3 2/2] iio: magnetometer: add support for Melexis MLX90393

In reply to Nikhil Gautam
Quoted 10 lines · Nikhil Gautam

Add Industrial I/O subsystem support for the Melexis MLX90393 3-axis magnetometer and temperature sensor.

The driver currently supports:

raw magnetic field measurements
raw temperature measurements
configurable gain/scale selection
configurable oversampling ratio
direct mode operation

Can you add '-' or '*' in front of each item in the list?

Quoted 9 lines

The MLX90393 supports both I2C and SPI interfaces. This initial implementation adds support for the I2C interface.

The device uses a command-based communication protocol rather than a conventional register-addressed interface. A small transport abstraction layer is therefore used instead of regmap to share the common sensor logic between the current I2C implementation and future SPI support without duplicating code.

The commit message seems wrapped around 56 characters. We have capacity up to ~72, please use it.

...

Quoted 1 line

+ * Datasheet: https://media.melexis.com/-/media/files/documents/datasheets/mlx90393-datasheet-melexis.pdf

Also add this line as a tag in the commit message (before your SoB).

...

drivers/iio/magnetometer/mlx90393_core.c line 14

Quoted 5 lines
drivers/iio/magnetometer/mlx90393_core.c new file +5 -0
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/errno.h>
+#include <linux/delay.h>

+ dev_printk.h // dev_err_probe()

drivers/iio/magnetometer/mlx90393_core.c line 18

Quoted 5 lines
drivers/iio/magnetometer/mlx90393_core.c new file +4 -0
+#include <linux/module.h>
+#include <linux/mutex.h>
 
+#include <linux/types.h>
+#include <linux/time64.h>

Please, keep them sorted alphabetically.

Quoted 2 lines
drivers/iio/magnetometer/mlx90393_core.c new file +2 -0
+#include <linux/unaligned.h>
+#include <linux/units.h>

...

drivers/iio/magnetometer/mlx90393_core.c line 29

Quoted 4 lines
drivers/iio/magnetometer/mlx90393_core.c new file +4 -0
+struct mlx90393_data {
+	/* Protects sensor configuration and measurement operations */
+	struct mutex lock;
+	struct device *dev;

You need a forward declaration for struct device.

Quoted 12 lines
drivers/iio/magnetometer/mlx90393_core.c new file +12 -0
+	void *bus_context;
+	const struct mlx90393_transfer_ops *ops;
+	u8 gain_sel;
+	u8 hallconf;
+
+	u8 res_xy;
+	u8 res_z;
+
+	u8 dig_filt;
+	u8 osr;
+	u8 osr2;
+};

...

drivers/iio/magnetometer/mlx90393_core.c line 57

Quoted 3 lines
drivers/iio/magnetometer/mlx90393_core.c new file +3 -0
+/* Datasheet: Table no.17 */
+static const int mlx90393_scale_table[MLX90393_AXIS_MAX][MLX90393_GAIN_MAX]
+				      [MLX90393_RES_MAX] = {

Not good indentation. Just make it

static const int mlx90393_scale_table[][MLX90393_GAIN_MAX][MLX90393_RES_MAX] = {

which is precisely a single line.

Quoted 12 lines
drivers/iio/magnetometer/mlx90393_core.c new file +12 -0
+	/* XY axis */
+	{
+		{ 751, 1502, 3004, 6009},
+		{ 601, 1202, 2403, 4840},
+		{ 451, 901, 1803, 3605},
+		{ 376, 751, 1502, 3004},
+		{ 300, 601, 1202, 2403},
+		{ 250, 501, 1001, 2003},
+		{ 200, 401, 801, 1602},
+		{ 150, 300, 601, 1202},
+	},
+	/* Z axis */

drivers/iio/magnetometer/mlx90393_core.c line 79

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +10 -0
+	{
+		{ 1210, 2420, 4840, 9680},
+		{ 968, 1936, 3872, 7744},
+		{ 726, 1452, 2904, 5808},
+		{ 605, 1210, 2420, 4840},
+		{ 484, 968, 1936, 3872},
+		{ 403, 807, 1613, 3227},
+		{ 323, 645, 1291, 2581},
+		{ 242, 484, 968, 1936},
+	}

Keep trailing comma(s) for non-terminator entries.

Quoted 1 line

+};

...

drivers/iio/magnetometer/mlx90393_core.c line 97

Quoted 9 lines
drivers/iio/magnetometer/mlx90393_core.c new file +8 -0
+#define MLX90393_CHAN(idx, axis, addr) { \
+	.type = IIO_MAGN, \
+	.modified = 1, \
+	.channel = idx, \
+	.address = addr, \
+	.channel2 = IIO_MOD_##axis, \
 
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
+		BIT(IIO_CHAN_INFO_SCALE), \

It's harder to read, make it either

	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |	\
			      BIT(IIO_CHAN_INFO_SCALE), \

or (which is also my preference)

	.info_mask_separate = \
		BIT(IIO_CHAN_INFO_RAW) |	\
		BIT(IIO_CHAN_INFO_SCALE), \

Do it for all such cases.

Quoted 6 lines
drivers/iio/magnetometer/mlx90393_core.c new file +6 -0
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),\
+	.info_mask_separate_available = \
+		BIT(IIO_CHAN_INFO_SCALE),  \
+	.info_mask_shared_by_type_available = \
+		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \
+}

...

Quoted 1 line

+ * Datasheet: Table 8, Page no. 12

If Table has a title, also mention it here

		Table 8 "...title of the table..."

in such a case the page reference most likely may be dropped. Same comment for all the similar cases.

...

Quoted 1 line

+static int mlx90393_get_tconv_us(struct mlx90393_data *data)

Can it return negative? What will be the meaning?

drivers/iio/magnetometer/mlx90393_core.c line 139

Quoted 6 lines
drivers/iio/magnetometer/mlx90393_core.c new file +5 -0
+{
+	const int osr = data->osr;
+	const int osr2 = data->osr2;
+	const int df = data->dig_filt;
 
+

No blank line in the definition block.

drivers/iio/magnetometer/mlx90393_core.c line 142

Quoted 3 lines
drivers/iio/magnetometer/mlx90393_core.c new file +3 -0
+	int tconvm;
+	int tconvt;
+

Ditto, and why are tconv* signed?

Quoted 17 lines
drivers/iio/magnetometer/mlx90393_core.c new file +17 -0
+	int m = 3; /* X,Y,Z */
+
+	/*
+	 * TCONVM = 67 + 64 * 2^OSR * (2 + 2^DIG_FILT)
+	 */
+	tconvm = 67 + (64 * BIT(osr) * (2 + BIT(df)));
+
+	/*
+	 * TCONVT = 67 + 192 * 2^OSR2
+	 */
+	tconvt = 67 + (192 * BIT(osr2));
+	/*
+	 * Total conversion time:
+	 * TSTBY + TACTIVE + m * TCONVM + TCONVT + TCONV_END
+	 */
+	return 220 + 360 + (m * tconvm) + tconvt + 120;
+}

...

drivers/iio/magnetometer/mlx90393_core.c line 163

Quoted 4 lines
drivers/iio/magnetometer/mlx90393_core.c new file +3 -0
+static int mlx90393_xfer(struct mlx90393_data *data,
 
+			 const u8 *tx, int tx_len,
+			 u8 *rx, int rx_len)

The last two lines may be joined.

Quoted 3 lines
drivers/iio/magnetometer/mlx90393_core.c new file +3 -0
+{
+	return data->ops->xfer(data->bus_context, tx, tx_len, rx, rx_len);
+}

...

drivers/iio/magnetometer/mlx90393_core.c line 271

Quoted 12 lines
drivers/iio/magnetometer/mlx90393_core.c new file +12 -0
+static int mlx90393_update_bits(struct mlx90393_data *data, u8 reg,
+				u16 mask, u16 val)
+{
+	u16 reg_val;
+	int ret;
+
+	ret = mlx90393_read_reg(data, reg, &reg_val);
+	if (ret)
+		return ret;
+
+	reg_val &= ~mask;
+	reg_val |= (val << __ffs(mask)) & mask;

Isn't it field_prep() reinvention? (Note small letters in the name!)

Quoted 2 lines
(quoted diff) +2 -0
+	return mlx90393_write_reg(data, reg, reg_val);
+}

...

drivers/iio/magnetometer/mlx90393_core.c line 287

Quoted 1 line
drivers/iio/magnetometer/mlx90393_core.c new file +1 -0
+	/* Wait conversion */
	/* Wait for conversion to be done */

("wait for" is an English stanza that in great majority of the cases is used).

Quoted 1 line
drivers/iio/magnetometer/mlx90393_core.c new file +1 -0
+	fsleep(mlx90393_get_tconv_us(data));

...

drivers/iio/magnetometer/mlx90393_core.c line 348

Quoted 6 lines
drivers/iio/magnetometer/mlx90393_core.c new file +6 -0
+static int mlx90393_find_scale(struct mlx90393_data *data, bool z_axis,
+			       int val, int val2,
+			       int *gain)
+{
+	u8 res;
+	enum mlx90393_axis_type axis;

Prefer reversed xmas tree order.

drivers/iio/magnetometer/mlx90393_core.c line 359

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +9 -0
+	if (z_axis) {
+		axis = MLX90393_AXIS_TYPE_Z;
+		res = data->res_z;
+	} else {
+		axis = MLX90393_AXIS_TYPE_XY;
+		res = data->res_xy;
+	}
 
+	if (val != 0)
+		return -EINVAL;

This doesn't use res or axis, move it above.

drivers/iio/magnetometer/mlx90393_core.c line 361

Quoted 1 line
drivers/iio/magnetometer/mlx90393_core.c new file +1 -0
+	for (unsigned int i = 0; i < ARRAY_SIZE(mlx90393_scale_table[0]); i++) {

Use [axis] instead of [0] for the consistency's sake.

Quoted 9 lines
drivers/iio/magnetometer/mlx90393_core.c new file +8 -0
+		if (mlx90393_scale_table[axis][i][res] == val2) {
+			*gain = i;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
 

drivers/iio/magnetometer/mlx90393_core.c line 379

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +8 -0
 
+static int mlx90393_set_scale(struct mlx90393_data *data,
+			      const struct iio_chan_spec *chan,
+			      int val, int val2)
+{
+	bool z_axis;
+	int gain;
+	int ret;
 
+	z_axis = chan->channel2 == IIO_MOD_Z;

Can we rather split _find_scale() to two and replace this with

	if (chan->channel2 == IIO_MOD_Z)
		ret = mlx90393_find_z_scale(data, val, val2, &gain);
	else
		ret = mlx90393_find_xy_scale(data, val, val2, &gain);

? I believe it will be less LoC.

Quoted 12 lines
drivers/iio/magnetometer/mlx90393_core.c new file +12 -0
+	ret = mlx90393_find_scale(data, z_axis, val, val2, &gain);
+	if (ret)
+		return ret;
+
+	ret = mlx90393_update_bits(data, MLX90393_REG_CONF1, MLX90393_CONF1_GAIN_SEL,
+				   gain);
+	if (ret)
+		return ret;
+
+	data->gain_sel = gain;
+	return 0;
+}

...

drivers/iio/magnetometer/mlx90393_core.c line 402

Quoted 3 lines
drivers/iio/magnetometer/mlx90393_core.c new file +3 -0
+static int mlx90393_find_osr(int val, int *osr)
+{
+	for (unsigned int i = 0; i < MLX90393_OSR_MAX;  i++) {

One space too many.

Quoted 8 lines
drivers/iio/magnetometer/mlx90393_core.c new file +8 -0
+		if (mlx90393_osr_avail[i] == val) {
+			*osr = i;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}

...

Quoted 4 lines
drivers/iio/magnetometer/mlx90393_core.c new file +4 -0
+static int mlx90393_set_osr(struct mlx90393_data *data, int val)
+{
+	int osr;
+	int ret;

drivers/iio/magnetometer/mlx90393_core.c line 431

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +10 -0
+
+	ret = mlx90393_find_osr(val, &osr);
+	if (ret)
+		return ret;
+
+	if (osr == data->osr)
+		return 0;
+
+	ret = mlx90393_update_bits(data, MLX90393_REG_CONF3, MLX90393_CONF3_OSR,
+				   osr);

Just make line longer, in this case it will be better to read in my opinion.

Quoted 6 lines
drivers/iio/magnetometer/mlx90393_core.c new file +6 -0
+	if (ret)
+		return ret;
+
+	data->osr = osr;
+	return 0;
+}

...

drivers/iio/magnetometer/mlx90393_core.c line 450

Quoted 12 lines
drivers/iio/magnetometer/mlx90393_core.c new file +12 -0
+static int mlx90393_set_temp_osr2(struct mlx90393_data *data, int val)
+{
+	int ret;
+
+	if (val < 0 || val >= MLX90393_OSR2_MAX)
+		return -EINVAL;
+
+	if (val == data->osr2)
+		return 0;
+
+	ret = mlx90393_update_bits(data, MLX90393_REG_CONF3, MLX90393_CONF3_OSR2,
+				   val);

Ditto.

Quoted 7 lines
drivers/iio/magnetometer/mlx90393_core.c new file +7 -0
+	if (ret)
+		return ret;
+
+	data->osr2 = val;
+
+	return 0;
+}

...

Quoted 4 lines
drivers/iio/magnetometer/mlx90393_core.c new file +4 -0
+static int mlx90393_write_raw(struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan,
+			      int val, int val2,
+			      long mask)

drivers/iio/magnetometer/mlx90393_core.c line 487

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +9 -0
+{
+	struct mlx90393_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE: {
+		guard(mutex)(&data->lock);
+		return mlx90393_set_scale(data, chan, val, val2);
+	}
 
+

Be consistent with the style of switch-case. Either add blank lines in all of them in all cases, or drop everywhere.

Quoted 17 lines
drivers/iio/magnetometer/mlx90393_core.c new file +17 -0
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO: {
+		guard(mutex)(&data->lock);
+		switch (chan->type) {
+		case IIO_TEMP:
+			return mlx90393_set_temp_osr2(data, val);
+
+		case IIO_MAGN:
+			return mlx90393_set_osr(data, val);
+
+		default:
+			return -EINVAL;
+		}
+	}
+	default:
+		return -EINVAL;
+	}
+}

...

Quoted 7 lines
drivers/iio/magnetometer/mlx90393_core.c new file +7 -0
+static int mlx90393_read_raw(struct iio_dev *indio_dev,
+			     const struct iio_chan_spec *chan,
+			     int *val, int *val2, long mask)
+{
+	struct mlx90393_data *data = iio_priv(indio_dev);
+	int ret;
+

drivers/iio/magnetometer/mlx90393_core.c line 521

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +9 -0
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW: {
+		guard(mutex)(&data->lock);
+		ret = mlx90393_read_measurement(data, chan->address, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
 
+		}

Misindented.

Quoted 14 lines
drivers/iio/magnetometer/mlx90393_core.c new file +14 -0
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_MAGN:
+			return mlx90393_get_scale(data, chan, val, val2);
+
+		case IIO_TEMP:
+			/*
+			 * Datasheet Table 7: Thermal Specification
+			 */
+			*val = 0;
+			*val2 = 22124;
+			return IIO_VAL_INT_PLUS_MICRO;
+
+		default:

drivers/iio/magnetometer/mlx90393_core.c line 544

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +9 -0
+			return -EINVAL;
+		}
+
+	case IIO_CHAN_INFO_OFFSET:
+		if (chan->type != IIO_TEMP)
+			return -EINVAL;
 
+		/*
+		 * Datasheet Table 7: Thermal Specification
+		 */

This is a single line comment.

Quoted 1 line

+

This blank line should be before the comment and not after.

Quoted 16 lines
drivers/iio/magnetometer/mlx90393_core.c new file +16 -0
+		*val = -45114;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		switch (chan->type) {
+		case IIO_TEMP:
+			return mlx90393_get_temp_osr2(data, val);
+		case IIO_MAGN:
+			return mlx90393_get_osr(data, val);
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}

...

Quoted 7 lines
drivers/iio/magnetometer/mlx90393_core.c new file +7 -0
+static int mlx90393_read_avail(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       const int **vals,
+			       int *type,
+			       int *length,
+			       long mask)
+{

drivers/iio/magnetometer/mlx90393_core.c line 579

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +10 -0
+	struct mlx90393_data *data = iio_priv(indio_dev);
+	static int scale_avail[MLX90393_GAIN_MAX][MLX90393_AXIS_MAX];
+	enum mlx90393_axis_type axis;
+	u8 res;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE: {
+		guard(mutex)(&data->lock);
+		axis = chan->channel2 == IIO_MOD_Z;
+		res = axis ? data->res_z : data->res_xy;
		res = chan->channel2 == IIO_MOD_Z ? data->res_z : data->res_xy;

This fits a single line, no axis variable is needed.

Quoted 28 lines
drivers/iio/magnetometer/mlx90393_core.c new file +28 -0
+		for (unsigned int i = 0; i < MLX90393_GAIN_MAX; i++) {
+			scale_avail[i][0] = 0;
+			scale_avail[i][1] =
+				mlx90393_scale_table[axis][i][res];
+		}
+
+		*vals = &scale_avail[0][0];
+		*type = IIO_VAL_INT_PLUS_NANO;
+		*length = MLX90393_GAIN_MAX * MLX90393_AXIS_MAX;
+		return IIO_AVAIL_LIST;
+	}
+
+	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+		if (chan->type == IIO_TEMP) {
+			*vals = mlx90393_osr2_avail;
+			*type = IIO_VAL_INT;
+			*length = MLX90393_OSR2_MAX;
+		} else {
+			*vals = mlx90393_osr_avail;
+			*type = IIO_VAL_INT;
+			*length = MLX90393_OSR_MAX;
+		}
+		return IIO_AVAIL_LIST;
+
+	default:
+		return -EINVAL;
+	}
+}

...

Quoted 16 lines
drivers/iio/magnetometer/mlx90393_core.c new file +16 -0
+static int mlx90393_init(struct mlx90393_data *data)
+{
+	int ret;
+	u16 reg;
+
+	/* Exit mode */
+	ret = mlx90393_write_cmd(data, MLX90393_CMD_EX);
+	if (ret)
+		return ret;
+
+	/*
+	 * Datasheet section 15.4.1.2 (RT command), Figure 16:
+	 * Wait 1 ms after EX command before issuing RT.
+	 */
+	fsleep(1 * USEC_PER_MSEC);
+

drivers/iio/magnetometer/mlx90393_core.c line 642

Quoted 10 lines
drivers/iio/magnetometer/mlx90393_core.c new file +10 -0
+	/* Reset device */
+	ret = mlx90393_write_cmd(data, MLX90393_CMD_RT);
+	if (ret)
+		return ret;
+
+	/*
+	 * Datasheet section 15.4.1.2 (RT command), Figure 16:
+	 * Wait 1.5 ms for the start-up sequence to complete.
+	 */
+	fsleep(1.5 * USEC_PER_MSEC);

He-he, this is a float number. While it might work, it's better to use integer numbers. Yeah, for the sake of consistency the above also better with 1000.

Quoted 20 lines
drivers/iio/magnetometer/mlx90393_core.c new file +20 -0
+
+	ret = mlx90393_read_reg(data, MLX90393_REG_CONF1, &reg);
+	if (ret)
+		return ret;
+
+	data->gain_sel = FIELD_GET(MLX90393_CONF1_GAIN_SEL, reg);
+	data->hallconf = FIELD_GET(MLX90393_CONF1_HALLCONF, reg);
+
+	ret = mlx90393_read_reg(data, MLX90393_REG_CONF3, &reg);
+	if (ret)
+		return ret;
+
+	data->res_xy = FIELD_GET(MLX90393_CONF3_RES_X, reg);
+	data->res_z = FIELD_GET(MLX90393_CONF3_RES_Z, reg);
+	data->dig_filt = FIELD_GET(MLX90393_CONF3_DIG_FILT, reg);
+	data->osr = FIELD_GET(MLX90393_CONF3_OSR, reg);
+	data->osr2 = FIELD_GET(MLX90393_CONF3_OSR2, reg);
+
+	return 0;
+}

...

drivers/iio/magnetometer/mlx90393_i2c.c line 6

Quoted 5 lines
drivers/iio/magnetometer/mlx90393_i2c.c new file +5 -0
+#include <linux/array_size.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>

+ types.h // uXX

...

drivers/iio/magnetometer/mlx90393_i2c.c line 20

Quoted 3 lines
drivers/iio/magnetometer/mlx90393_i2c.c new file +3 -0
+	struct i2c_client *client = context;
+	int ret;
+	struct i2c_msg msgs[2] = {

Keep reversed xmas tree order.

Quoted 12 lines
drivers/iio/magnetometer/mlx90393_i2c.c new file +12 -0
+		[0] = {
+			.addr = client->addr,
+			.len = tx_len,
+			.buf = (u8 *)tx,
+		},
+		[1] = {
+			.addr = client->addr,
+			.flags = I2C_M_RD,
+			.len = rx_len,
+			.buf = rx,
+		},
+	};

...

drivers/iio/magnetometer/mlx90393_i2c.c line 38

Quoted 5 lines
drivers/iio/magnetometer/mlx90393_i2c.c new file +5 -0
+	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret != ARRAY_SIZE(msgs))
+		return ret < 0 ? ret : -EIO;
+
+	return 0;

Do it in a regular pattern

	if (ret < 0)
		return ret;
	if (ret != ARRAY_SIZE(msgs))
		return -EIO;

drivers/iio/magnetometer/mlx90393_i2c.c line 70

Quoted 12 lines
drivers/iio/magnetometer/mlx90393_i2c.c new file +10 -0
+}
 
+static struct i2c_driver mlx90393_i2c_driver = {
+	.driver = {
+		.name = "mlx90393",
+		.of_match_table = mlx90393_of_match,
+	},
+	.probe = mlx90393_i2c_probe,
+	.id_table = mlx90393_id,
+};
 
+

Redundant blank line.

Quoted 1 line

+module_i2c_driver(mlx90393_i2c_driver);

Signature · 2 lines
With Best Regards,
Andy Shevchenko
reviewer
#8 lore raw

Re: [PATCH v3 0/2] iio: magnetometer: add support for Melexis MLX90393

In reply to Nikhil Gautam
Quoted 2 lines · Nikhil Gautam

Subject: [PATCH v3 0/2] iio: magnetometer: add support for Melexis MLX90393

It is best not to manually edit the emails before sending. Somehow the subject ended up in the email body.