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

[Nikhil Gautam <nikhilgtr@gmail.com>]

On 19-06-2026 12:56 am, Andy Shevchenko wrote:
> On Thu, Jun 18, 2026 at 09:31:41PM +0530, Nikhil Gautam wrote:
Hi Andy,

Thank you very much for taking the time to do such a thorough review of 
this patch series.

Your detailed comments and suggestions are very helpful.
I'll address the issues you've pointed out, update the cover letter to 
better explain the design decisions,
and incorporate the requested coding style and API changes in the next 
revision.

I appreciate your review and feedback.
>> +config MLX90393
>> +	tristate "MELEXIS MLX90393 3-axis magnetometer sensor"
>> +	depends on I2C
> Why not a regmap?
The MLX90393 uses both register-based and command-based transactions.
Since regmap does not naturally model the command-based interface,
using it would require workarounds such as virtual registers or bypass 
paths.
A custom transport abstraction is therefore simpler and better suited 
for this device.

I already discussed this on thread, Link : 
https://lore.kernel.org/linux-iio/20260423121834.4244-1-nikhilgtr@gmail.com/ 

>> +#ifndef MLX90393_H
>> +#define MLX90393_H
>>
>> +#include <linux/bitops.h>
>> +#include <linux/bits.h>
> Not required, it's covered by bitops.h.
Noted. Thanks

>> +#include <linux/types.h>
> ...
>
>> +#define MLX90393_MEASURE_ALL \
>> +	(MLX90393_MEASURE_TEMP | MLX90393_MEASURE_X | \
>> +	MLX90393_MEASURE_Y | MLX90393_MEASURE_Z)
> Split logically.
>
> #define MLX90393_MEASURE_ALL \
> 	(MLX90393_MEASURE_TEMP | \
> 	 MLX90393_MEASURE_X | MLX90393_MEASURE_Y | MLX90393_MEASURE_Z)
>
> Or just a (long) single line.
Agreed. Thanks

>> +	int (*xfer)(void *context, const u8 *tx, int tx_len,
>> +		    u8 *rx, int rx_len);
> One line, it's only 81 characters.
Agreed Thanks

>> +};
>> +
>> +int mlx90393_core_probe(struct device *dev,
> You want forward declaration for struct device.
Agreed, Thanks

> + array_size.h
> + bitfield.h // FIELD_GET()
>
>> +#include <linux/delay.h>
> + errno.h // -Exxx
>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
> + types.h // uXX
>
>> +#include <linux/unaligned.h>
>> +#include <linux/units.h>
> IWYU, please (just pointed out a few missing above, there are more).
Sure, will do it.

>> +/* Datasheet: Table no.17 */
>> +static const int mlx90393_scale_table[MLX90393_AXIS_MAX]
>> +				[MLX90393_GAIN_MAX]
>> +				[MLX90393_RES_MAX] = {
> This is broken indentation.
Noted, Thanks

>> +/*
>> + * Calculate total conversion time in microseconds.
>> + *
>> + * Formula derived from datasheet timing equations.
> Which formula? Where is datasheet? What if I have no access to it?
> Always repeat the important details in the comment in the code.
Got it, will add all details from datasheet

>> + */
>> +
> Unneeded blank line.
Noted, Thanks

>> +static int mlx90393_get_tconv_us(struct mlx90393_data *data)
>> +{
>> +	const int osr = data->osr;
>> +	const int osr2 = data->osr2;
>> +	const int df = data->dig_filt;
>> +
>> +	int tconvm;
>> +	int tconvt;
>> +
>> +	int m = 3; /* X,Y,Z */
>> +
>> +	/*
>> +	 * Datasheet:
> What chapter/section/table name? Page number?
Sure will add.

>> +	 * TCONVM = 67 + 64 * 2^OSR * (2 + 2^DIG_FILT)
> What does this cryptic message mean? Please, accompany this with more English
> plain text.
Agreed, Thanks

>> +static int mlx90393_xfer(struct mlx90393_data *data,
>> +			 const u8 *tx, int tx_len,
>> +			 u8 *rx, int rx_len)
>> +{
>> +	return data->ops->xfer(data->bus_context,
>> +			tx, tx_len,
>> +			rx, rx_len);
> It's perfectly one line.
>
> Also you might want to have
>
> 	if (!...->xfer)
> 		return -E...;
>
>> +}
Noted, Thanks

>> +static int mlx90393_check_status(u8 cmd, u8 status)
>> +{
>> +	/* Always validate error bit */
>> +	if (status & MLX90393_STATUS_ERROR)
>> +		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;
>> +		return 0;
>> +	case MLX90393_CMD_RT:
>> +		/* Reset acknowledge */
>> +		if (!(status & MLX90393_STATUS_RT))
> For sake of consistency you might want to also compare to 0 here.
Thanks, I'll make that change.

>> +static int mlx90393_write_reg(struct mlx90393_data *data, u8 reg, u16 val)
> Here the variable is named 'reg' there is 'reg_addr'. As I can see the code is
> full of inconsistencies (like 2+ people with different style guidelines wrote
> it). Please. take your time and check the code and make it consistent.
Got it, will make it consistent across whole file.

>> +{
>> +	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);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return mlx90393_check_status(tx[0], status);
>> +}
>> +
>> +static int mlx90393_update_bits(struct mlx90393_data *data, u8 reg_addr,
>> +				u16 mask, u16 val)
>> +{
>> +	u16 reg;
>> +	int ret;
>> +
>> +	ret = mlx90393_read_reg(data, reg_addr, &reg);
>> +	if (ret)
>> +		return ret;
>>
>> +	reg &= ~mask;
>> +	reg |= (val << __ffs(mask)) & mask;
> bitfield.h has macros for this.
Noted, Thanks
>> +static int mlx90393_find_osr(int val, int *osr)
>> +{
>> +	for (unsigned int i = 0; i < MLX90393_OSR_MAX;  i++)
>> +		if (mlx90393_osr_avail[i] == val) {
>> +			*osr = i;
>> +			return 0;
>> +		}
> Missing {}.
Agreed, removed intentionally as single statement, will add as per 
guidelines all the places
where needed

>> +static int mlx90393_get_temp_osr2(struct mlx90393_data *data, int *val)
>> +{
>> +	*val = mlx90393_osr2_avail[data->osr2];
> Missing blank line.
Noted, Thanks

>> 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);
>> +	int ret;
> Not needed, return directly.
Agreed.

>> +			/* Datasheet: 22124 millidegC/LSB */
>> +		/* Datasheet: temperature offset */
> Again, at least put a page, better to have Section/Table/et cetera title.
Sure, will do it

> +			*vals = mlx90393_osr_avail;
> +			*type = IIO_VAL_INT;
> +			*length = MLX90393_OSR_MAX;
> +		}
> +		return IIO_AVAIL_LIST;
> +
> +	default:
> +		return -EINVAL;
> +	}
>> +	return -EINVAL;
> Besides missing blank line, this is actually a dead code.
Correct, this will never be called. will fix

>> +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;
>> +
>> +	/* Wait for device comes out of reset */
> Datasheet? Empirical?
Agreed, Thanks

>> +	fsleep(1000);
> 1 * USEC_PER_MSEC
> (will require time.h to be included).
Noted, Thanks

>> +	/* Reset device */
>> +	ret = mlx90393_write_cmd(data, MLX90393_CMD_RT);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Wait for device to reset */
>> +	fsleep(6000);
> As per above.
This delay is not 6000ms it is 1500ms as per datasheet, forgot to update 
in code
Thanks for pointing out, will update all places delay requirement with 
datasheet reference.

>> +int mlx90393_core_probe(struct device *dev,
>> +			const struct mlx90393_transfer_ops *ops,
>> +			void *context)
>> +{
>> +	struct iio_dev *indio_dev;
>> +	struct mlx90393_data *data;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	data = iio_priv(indio_dev);
>> +	devm_mutex_init(dev, &data->lock);
> Nonsense. If we don't check the return code of devm_*(), there is a little
> reason to use it in the first place. But then one should not use devm further.
> Easy fix: check for returned errors.
Agreed, I'll fix this in the next revision.

>> +	data->dev = dev;
>> +	data->ops = ops;
>> +	data->bus_context = context;
>> +
>> +	indio_dev->name = "mlx90393";
>> +	indio_dev->info = &mlx90393_info;
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->channels = mlx90393_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(mlx90393_channels);
>> +
>> +	ret = mlx90393_init(data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to initialize device\n");
>> +
>> +	return devm_iio_device_register(dev, indio_dev);
>> +}
>> +EXPORT_SYMBOL_GPL(mlx90393_core_probe);
> Make it namespaces.
Okay, will do it
> + array_size.h
> + errno.h
>
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/mod_devicetable.h>
> ...and so on.
>
> Same, follow the IWYU principle.
Got it, will take care
>> +/*
>> + * MLX90393 commands use repeated-start transfers where
>> + * every command is followed by a status/data response.
>> + */
>> +static int mlx90393_i2c_xfer(void *context,
>> +			     const u8 *tx, int tx_len,
>> +			     u8 *rx, int rx_len)
>> +{
>> +	struct i2c_client *client = context;
>> +	int ret;
>> +	struct i2c_msg msgs[2] = {
>> +		[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;
> Please, make this to be the regular pattern
>
> 	if (ret < 0)
> 		return ret;
> 	if (ret != ARRAY_SIZE(msgs))
> 		return -EIO;
>
>> +	return 0;
>> +}
Thanks, I'll make that change.
>> +static struct i2c_driver mlx90393_i2c_driver = {
>> +	.driver = {
>> +		.name = "mlx90393",
>> +		.of_match_table = mlx90393_of_match,
>> +	},
>> +	.probe = mlx90393_i2c_probe,
>> +};
> Remove this blank line.
Noted
>> +module_i2c_driver(mlx90393_i2c_driver);
>>

Thanks & Regards,
Nikhil