Re: [PATCH v3 5/5] iio: magnetometer: Add mmc5633 sensor

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 30 Sep 2025 15:34:24 -0400
Frank Li <Frank.Li@nxp.com> wrote:

> Add mmc5633 sensor basic support.
> - Support read 20 bits X/Y/Z magnetic.
> - Support I3C HDR mode to send start measurememt command.
> - Support I3C HDR mode to read all sensors data by one command.
> 
> Co-developed-by: Carlos Song <carlos.song@nxp.com>
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

Hi Frank,

A few more minor things inline.

Thanks,

Jonathan

> ---
> Change in v3
> - remove mmc5633_hw_set
> - make -> Make
> - change indention for mmc5633_samp_freq
> - use u8 arrary to handle dword data
> - get_unaligned_be16() to get raw data
> - add helper function to check if i3c support hdr
> - use read_avail() callback
> 
> change in v2
> - new patch

> diff --git a/drivers/iio/magnetometer/mmc5633.c b/drivers/iio/magnetometer/mmc5633.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..bcd79ab6053d50026961f7cf9da296c30c720399
> --- /dev/null
> +++ b/drivers/iio/magnetometer/mmc5633.c
> @@ -0,0 +1,534 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MMC5633 - MEMSIC 3-axis Magnetic Sensor
> + *
> + * Copyright (c) 2015, Intel Corporation.
> + * Copyright (c) 2025, NXP
> + *
> + * IIO driver for MMC5633, base on mmc35240.c
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/i3c/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/init.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/unaligned.h>
> +
> +#define MMC5633_REG_XOUT_L	0x00
> +#define MMC5633_REG_XOUT_H	0x01
> +#define MMC5633_REG_YOUT_L	0x02
> +#define MMC5633_REG_YOUT_H	0x03
> +#define MMC5633_REG_ZOUT_L	0x04
> +#define MMC5633_REG_ZOUT_H	0x05
> +#define MMC5633_REG_XOUT_2	0x06
> +#define MMC5633_REG_YOUT_2	0x07
> +#define MMC5633_REG_ZOUT_2	0x08
> +
> +#define MMC5633_REG_STATUS1	0x18
> +#define MMC5633_REG_STATUS0	0x19
> +#define MMC5633_REG_CTRL0	0x1b
> +#define MMC5633_REG_CTRL1	0x1c
> +#define MMC5633_REG_CTRL2	0x1d
> +
> +#define MMC5633_REG_ID		0x39
> +
> +#define MMC5633_STATUS1_MEAS_M_DONE_BIT	BIT(6)
> +
> +#define MMC5633_CTRL0_CMM_FREQ_EN	BIT(7)
> +#define MMC5633_CTRL0_AUTO_ST_EN	BIT(6)
> +#define MMC5633_CTRL0_AUTO_SR_EN	BIT(5)
> +#define MMC5633_CTRL0_RESET		BIT(4)
> +#define MMC5633_CTRL0_SET		BIT(3)
> +#define MMC5633_CTRL0_MEAS_T		BIT(1)
> +#define MMC5633_CTRL0_MEAS_M		BIT(0)
> +
> +#define MMC5633_CTRL1_BW0_BIT		BIT(0)
> +#define MMC5633_CTRL1_BW1_BIT		BIT(1)

Drop these and

> +
> +#define MMC5633_CTRL1_BW_MASK		(MMC5633_CTRL1_BW0_BIT | \
> +					 MMC5633_CTRL1_BW1_BIT)

use GENMASK(1, 0) here

> +
> +#define MMC5633_WAIT_CHARGE_PUMP	50000	/* us */

Name them so the unit is in the define and then you don't need
the comment + we can see the unit where they are used.
*PUMP_USECS for example

> +#define MMC5633_WAIT_SET_RESET		1000	/* us */
> +
> +#define MMC5633_HDR_CTRL0_MEAS_M	0x01
> +#define MMC5633_HDR_CTRL0_MEAS_T	0x03
> +#define MMC5633_HDR_CTRL0_SET		0X05
> +#define MMC5633_HDR_CTRL0_RESET		0x07
> +

> +static const struct {
> +	int val;
> +	int val2;
> +} mmc5633_samp_freq[] = {
> +	{1, 200000},
> +	{2, 0},
> +	{3, 500000},
> +	{6, 600000},

My preference for style in IIO is
	{ 1, 200000 },
	{ 2, 0 },
etc.

> +};


> +static bool mmc5633_is_support_hdr(struct mmc5633_data *data)
> +{
> +	if (!data->i3cdev)
> +		return false;
> +
> +	return !!(i3c_device_get_supported_xfer_mode(data->i3cdev) & BIT(I3C_HDR_DDR));
The !! isn't needed given assigning an integer to the bool will have same effect.
> +}



> +static const struct of_device_id mmc5633_of_match[] = {
> +	{ .compatible = "memsic,mmc5633", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, mmc5633_of_match);
> +
> +static const struct i2c_device_id mmc5633_i2c_id[] = {
> +	{ "mmc5633" },
> +	{}
	{ }
Both for consistency and because I'm more broadly trying to standardize
on that formatting for IIO.

> +};
> +MODULE_DEVICE_TABLE(i2c, mmc5633_i2c_id);
> +
> +static struct i2c_driver mmc5633_i2c_driver = {
> +	.driver = {
> +		.name = "mmc5633_i2c",
> +		.of_match_table = mmc5633_of_match,
> +		.pm = pm_sleep_ptr(&mmc5633_pm_ops),
> +	},
> +	.probe	  = mmc5633_i2c_probe,
> +	.id_table       = mmc5633_i2c_id,

Mixture of tabs and spacing here.   I'd just use a single space
before the = and don't try to align them.

> +};

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c