Re: [PATCH v2 2/2] iio: magnetometer: add driver for MEMSIC MMC5983MA

[David Lechner <dlechner@baylibre.com>]

On 5/7/26 3:50 PM, Vladislav Kulikov wrote:
> Add support for the MEMSIC MMC5983MA 3-axis magnetometer. The driver
> provides raw magnetic field readings via IIO sysfs with SET/RESET
> offset cancellation for each measurement.
> 
> Signed-off-by: Vladislav Kulikov <vlad.kulikov.c@gmail.com>
> ---

I gave my RB tag already, but if we do another revision, a couple of
suggestions.

> +static int mmc5983_take_measurement(struct mmc5983_data *data, int m[3])
> +{
> +	unsigned int status;
> +	u8 buf[7];
> +	int ret;
> +
> +	ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
> +			   MMC5983_CTRL0_TM_M_BIT);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Datasheet page 15: measurement time is 8 ms at BW=00 (default,
> +	 * slowest setting). Use a 50 ms timeout for margin.
> +	 */
> +	ret = regmap_read_poll_timeout(data->regmap, MMC5983_REG_STATUS,
> +				       status,
> +				       status & MMC5983_STATUS_MEAS_M_DONE_BIT,
> +				       10000, 50000);

I wouldn't mind seeing 10 * KILO, 50 * KILO here to make it easier to read.

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_bulk_read(data->regmap, MMC5983_REG_XOUT0, buf,
> +			       sizeof(buf));
> +	if (ret)
> +		return ret;
> +
> +	m[0] = (buf[0] << 10) | (buf[1] << 2) | ((buf[6] >> 6) & 0x3);
> +	m[1] = (buf[2] << 10) | (buf[3] << 2) | ((buf[6] >> 4) & 0x3);
> +	m[2] = (buf[4] << 10) | (buf[5] << 2) | ((buf[6] >> 2) & 0x3);
> +
> +	return 0;
> +}
> +
> +static int mmc5983_read_raw(struct iio_dev *indio_dev,
> +			     const struct iio_chan_spec *chan, int *val,
> +			     int *val2, long mask)
> +{
> +	struct mmc5983_data *data = iio_priv(indio_dev);
> +	int m1[3], m2[3];
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		guard(mutex)(&data->mutex);
> +
> +		ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
> +				   MMC5983_CTRL0_SET_BIT);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Datasheet page 15: SET/RESET coil pulse is 500 ns.
> +		 * Vendor sample code waits 500 us before the next operation.
> +		 */
> +		fsleep(500);
> +
> +		ret = mmc5983_take_measurement(data, m1);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(data->regmap, MMC5983_REG_CTRL0,
> +				   MMC5983_CTRL0_RESET_BIT);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Datasheet page 15: SET/RESET coil pulse is 500 ns.
> +		 * Vendor sample code waits 500 us before the next operation.
> +		 */
> +		fsleep(500);

It looks like this SET/RESET sequence is also repeated during init. Maybe
refactor that out into a separate function.

> +
> +		ret = mmc5983_take_measurement(data, m2);
> +		if (ret)
> +			return ret;
> +
> +		*val = (m1[chan->address] - m2[chan->address]) / 2;
> +		return IIO_VAL_INT;
> +	}
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 0;
> +		*val2 = 61035;
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +