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

[Jonathan Cameron]

On Sat, 27 Jun 2026 15:25:19 +0530
Nikhil Gautam <nikhilgtr@xxxxxxxxx> wrote:

> 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.
> 
> Signed-off-by: Nikhil Gautam <nikhilgtr@xxxxxxxxx>
Hi Nikhil,

You have a lot of review already so I'm only going to take a very quick
look at this version.

Also take a look at:
https://sashiko.dev/#/patchset/20260627095519.8377-1-nikhilgtr%40gmail.com

The osr2_avail comment at least looks superficially valid as an oversampling
ratio of 0 means you don't read the value at all.

I2C dma safety one is wrong (that keeps coming up) as you have to opt in
to use DMA directly on buffers when using the Linux I2C stack.

Some of the later ones look plausible but over to you to analyse them.
It can be useful to address any you don't agree with by replying to this
thread to quote them and say why they are wrong

Thanks,

Jonathan


> diff --git a/drivers/iio/magnetometer/mlx90393_core.c b/drivers/iio/magnetometer/mlx90393_core.c
> new file mode 100644
> index 000000000000..78fcab8a2b56
> --- /dev/null
> +++ b/drivers/iio/magnetometer/mlx90393_core.c

> +static const int mlx90393_scale_table[MLX90393_AXIS_MAX][MLX90393_GAIN_MAX]
> +				      [MLX90393_RES_MAX] = {
> +	/* 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 */
> +	{
> +		{ 1210, 2420, 4840, 9680},

Space before } on all these

> +		{ 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},
> +	}
> +};
> +
> +static const int mlx90393_osr2_avail[MLX90393_OSR2_MAX] = {
> +	0, 1, 2, 3,
> +};

> +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);
> +		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);
> +
> +		case IIO_TEMP:
> +			/*
> +			 * Datasheet Table 7: Thermal Specification
> +			 */

Single line comment.

> +			*val = 0;
> +			*val2 = 22124;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	case IIO_CHAN_INFO_OFFSET:
> +		if (chan->type != IIO_TEMP)
> +			return -EINVAL;
> +		/*
> +		 * Datasheet Table 7: Thermal Specification

Single line comment.

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

> diff --git a/drivers/iio/magnetometer/mlx90393_i2c.c b/drivers/iio/magnetometer/mlx90393_i2c.c
> new file mode 100644
> index 000000000000..a9f0a40d15e8
> --- /dev/null
> +++ b/drivers/iio/magnetometer/mlx90393_i2c.c
>
...

> +
> +static const struct mlx90393_transfer_ops mlx90393_i2c_ops = {
> +	.xfer = mlx90393_i2c_xfer,
> +};
> +
> +static int mlx90393_i2c_probe(struct i2c_client *client)
> +{
> +	return mlx90393_core_probe(&client->dev, &mlx90393_i2c_ops, client);
> +}


Drop this blank line. It is a common convention to not have one here
as it visually associates the macro with the data it is using
(which isn't used anywhere else!)

> +module_i2c_driver(mlx90393_i2c_driver);
> +
> +MODULE_IMPORT_NS("IIO_MLX90393");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Nikhil Gautam <nikhilgtr@xxxxxxxxx>");
> +MODULE_DESCRIPTION("MLX90393 magnetometer sensor driver");