Re: [PATCH v3 2/2] iio: magnetometer: add support for Melexis MLX90393
From: Andy Shevchenko
Date: Mon Jun 29 2026 - 09:53:27 EST
On Sat, Jun 27, 2026 at 06:28:43AM +0530, Nikhil Gautam 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
Can you add '-' or '*' in front of each item in the list?
> 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.
...
> + * 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).
...
> +#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()
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +#include <linux/time64.h>
Please, keep them sorted alphabetically.
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
...
> +struct mlx90393_data {
> + /* Protects sensor configuration and measurement operations */
> + struct mutex lock;
> + struct device *dev;
You need a forward declaration for struct device.
> + 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;
> +};
...
> +/* 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.
> + /* 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},
> + { 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.
> +};
...
> +#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.
> + .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), \
> +}
...
> + * 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.
...
> +static int mlx90393_get_tconv_us(struct mlx90393_data *data)
Can it return negative? What will be the meaning?
> +{
> + const int osr = data->osr;
> + const int osr2 = data->osr2;
> + const int df = data->dig_filt;
> +
No blank line in the definition block.
> + int tconvm;
> + int tconvt;
> +
Ditto, and why are tconv* signed?
> + 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;
> +}
...
> +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.
> +{
> + return data->ops->xfer(data->bus_context, tx, tx_len, rx, rx_len);
> +}
...
> +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, ®_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!)
> + return mlx90393_write_reg(data, reg, reg_val);
> +}
...
> + /* Wait conversion */
/* Wait for conversion to be done */
("wait for" is an English stanza that in great majority of the cases is used).
> + fsleep(mlx90393_get_tconv_us(data));
...
> +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.
> + 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.
> + for (unsigned int i = 0; i < ARRAY_SIZE(mlx90393_scale_table[0]); i++) {
Use [axis] instead of [0] for the consistency's sake.
> + if (mlx90393_scale_table[axis][i][res] == val2) {
> + *gain = i;
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +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.
> + 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;
> +}
...
> +static int mlx90393_find_osr(int val, int *osr)
> +{
> + for (unsigned int i = 0; i < MLX90393_OSR_MAX; i++) {
One space too many.
> + if (mlx90393_osr_avail[i] == val) {
> + *osr = i;
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
...
> +static int mlx90393_set_osr(struct mlx90393_data *data, int val)
> +{
> + int osr;
> + int ret;
> +
> + 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.
> + if (ret)
> + return ret;
> +
> + data->osr = osr;
> + return 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.
> + if (ret)
> + return ret;
> +
> + data->osr2 = val;
> +
> + return 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);
> + }
> +
Be consistent with the style of switch-case. Either add blank lines in all of
them in all cases, or drop everywhere.
> + 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;
> + }
> +}
...
> +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;
> + }
Misindented.
> + 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:
> + 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.
> +
This blank line should be before the comment and not after.
> + *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;
> + }
> +}
...
> +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];
> + 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.
> + 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;
> + }
> +}
...
> +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);
> +
> + /* 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.
> +
> + ret = mlx90393_read_reg(data, MLX90393_REG_CONF1, ®);
> + 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, ®);
> + 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;
> +}
...
> +#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
...
> + struct i2c_client *client = context;
> + int ret;
> + struct i2c_msg msgs[2] = {
Keep reversed xmas tree order.
> + [0] = {
> + .addr = client->addr,
> + .len = tx_len,
> + .buf = (u8 *)tx,
> + },
> + [1] = {
> + .addr = client->addr,
> + .flags = I2C_M_RD,
> + .len = rx_len,
> + .buf = rx,
> + },
> + };
...
> + 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;
> +}
> +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.
> +module_i2c_driver(mlx90393_i2c_driver);
--
With Best Regards,
Andy Shevchenko