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>
Co-developed-by: Adrian Fluturel <fluturel.adrian@gmail.com>
Signed-off-by: Adrian Fluturel <fluturel.adrian@gmail.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change in v7
- add missed *.h file
- remove reduntant empty line
- add comments about delay 1us after SET
- use USEC_PER_MSEC for timeout value
Change in v6:
- remove acpi part
- return 0 for success path at mmc5633_write_raw
Change in V4
- use { 1, 2000 }
- Add _US for timeout
- Use GEN_MASK for MMC5633_CTRL1_BW_MASK
- Use { } for terminator.
- remove !!
- fix mix tab and space
- add mmc5603 (merge https://lore.kernel.org/all/20251003000731.22927-1-fluturel.adrian@gmail.com/)
- add tempature measure support
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
---
drivers/iio/magnetometer/Kconfig | 12 +
drivers/iio/magnetometer/Makefile | 1 +
drivers/iio/magnetometer/mmc5633.c | 588 +++++++++++++++++++++++++++++++++++++
3 files changed, 601 insertions(+)
diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index 81b812a29044e2b0b9ff84889c21aa3ebc20be35..cfb74a4a083630678a1db1132a14264de451a31a 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -139,6 +139,18 @@ config MMC35240
To compile this driver as a module, choose M here: the module
will be called mmc35240.
+config MMC5633
+ tristate "MEMSIC MMC5633 3-axis magnetic sensor"
+ select REGMAP_I2C
+ select REGMAP_I3C
+ depends on I2C || I3C
+ help
+ Say yes here to build support for the MEMSIC MMC5633 3-axis
+ magnetic sensor.
+
+ To compile this driver as a module, choose M here: the module
+ will be called mmc5633
+
config IIO_ST_MAGN_3AXIS
tristate "STMicroelectronics magnetometers 3-Axis Driver"
depends on (I2C || SPI_MASTER) && SYSFS
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index dfe970fcacb8664b293af84893f7d3e3e8d7bf7e..5bd227f8c1204bdd8b8a43da180833eedca1457b 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_BMC150_MAGN_SPI) += bmc150_magn_spi.o
obj-$(CONFIG_MAG3110) += mag3110.o
obj-$(CONFIG_HID_SENSOR_MAGNETOMETER_3D) += hid-sensor-magn-3d.o
obj-$(CONFIG_MMC35240) += mmc35240.o
+obj-$(CONFIG_MMC5633) += mmc5633.o
obj-$(CONFIG_IIO_ST_MAGN_3AXIS) += st_magn.o
st_magn-y := st_magn_core.o
diff --git a/drivers/iio/magnetometer/mmc5633.c b/drivers/iio/magnetometer/mmc5633.c
new file mode 100644
index 0000000000000000000000000000000000000000..3e29324f0720a2b8268ccd63409483a60e48802b
--- /dev/null
+++ b/drivers/iio/magnetometer/mmc5633.c
@@ -0,0 +1,588 @@
+// 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/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/errno.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/mutex.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/types.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_TOUT 0x09
+
+#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_T_DONE_BIT BIT(7)
+#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_BW_MASK GENMASK(1, 0)
+
+#define MMC5633_WAIT_SET_RESET_US (1 * USEC_PER_MSEC)
+
+#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
+
+enum mmc5633_axis {
+ MMC5633_AXIS_X,
+ MMC5633_AXIS_Y,
+ MMC5633_AXIS_Z,
+ MMC5633_TEMPERATURE,
+};
+
+struct mmc5633_data {
+ struct device *dev;
+ struct i3c_device *i3cdev;
+ struct mutex mutex; /* protect to finish one whole measurement */
+ struct regmap *regmap;
+};
+
+static const struct {
+ int val;
+ int val2;
+} mmc5633_samp_freq[] = {
+ { 1, 200000 },
+ { 2, 0 },
+ { 3, 500000 },
+ { 6, 600000 },
+};
+
+#define MMC5633_CHANNEL(_axis) { \
+ .type = IIO_MAGN, \
+ .modified = 1, \
+ .channel2 = IIO_MOD_ ## _axis, \
+ .address = MMC5633_AXIS_ ## _axis, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+}
+
+static const struct iio_chan_spec mmc5633_channels[] = {
+ MMC5633_CHANNEL(X),
+ MMC5633_CHANNEL(Y),
+ MMC5633_CHANNEL(Z),
+ {
+ .type = IIO_TEMP,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_OFFSET),
+ .address = MMC5633_TEMPERATURE,
+ },
+};
+
+static int mmc5633_get_samp_freq_index(struct mmc5633_data *data,
+ int val, int val2)
+{
+ u32 i;
+
+ for (i = 0; i < ARRAY_SIZE(mmc5633_samp_freq); i++)
+ if (mmc5633_samp_freq[i].val == val &&
+ mmc5633_samp_freq[i].val2 == val2)
+ return i;
+ return -EINVAL;
+}
+
+static int mmc5633_init(struct mmc5633_data *data)
+{
+ unsigned int reg_id;
+ int ret;
+
+ ret = regmap_read(data->regmap, MMC5633_REG_ID, ®_id);
+ if (ret < 0)
+ return dev_err_probe(data->dev, ret,
+ "Error reading product id\n");
+
+ /*
+ * Make sure we restore sensor characteristics, by doing
+ * a SET/RESET sequence, the axis polarity being naturally
+ * aligned after RESET.
+ */
+ ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_SET);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * Minimum time interval between SET or RESET to other operations is
+ * 1ms according to Operating Timing Diagram in datasheet.
+ */
+ fsleep(MMC5633_WAIT_SET_RESET_US);
+
+ ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_RESET);
+ if (ret < 0)
+ return ret;
+
+ /* set default sampling frequency */
+ return regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
+ MMC5633_CTRL1_BW_MASK,
+ FIELD_PREP(MMC5633_CTRL1_BW_MASK, 0));
+}
+
+static int mmc5633_take_measurement(struct mmc5633_data *data, int address)
+{
+ unsigned int reg_status;
+ int ret;
+ int val;
+
+ val = (address == MMC5633_TEMPERATURE) ? MMC5633_CTRL0_MEAS_T : MMC5633_CTRL0_MEAS_M;
+ ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, val);
+ if (ret < 0)
+ return ret;
+
+ val = (address == MMC5633_TEMPERATURE) ?
+ MMC5633_STATUS1_MEAS_T_DONE_BIT : MMC5633_STATUS1_MEAS_M_DONE_BIT;
+ ret = regmap_read_poll_timeout(data->regmap, MMC5633_REG_STATUS1, reg_status,
+ reg_status & val,
+ 10 * USEC_PER_MSEC,
+ 100 * 10 * USEC_PER_MSEC);
+ if (ret) {
+ dev_err(data->dev, "data not ready\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+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);
+}
+
+static int mmc5633_read_measurement(struct mmc5633_data *data, int address, void *buf, size_t sz)
+{
+ u8 data_cmd[2], status[2];
+ int ret, val, ready;
+
+ if (mmc5633_is_support_hdr(data)) {
+ struct i3c_xfer xfers_wr_cmd[] = {
+ {
+ .cmd = 0x3b,
+ .len = 2,
+ .data.out = data_cmd,
+ }
+ };
+ struct i3c_xfer xfers_rd_sta_cmd[] = {
+ {
+ .cmd = 0x23 | BIT(7), /* RDSTA CMD */
+ .len = 2,
+ .data.in = status,
+ },
+ };
+ struct i3c_xfer xfers_rd_data_cmd[] = {
+ {
+ .cmd = 0x22 | BIT(7), /* RDLONG CMD */
+ .len = sz,
+ .data.in = buf,
+ },
+ };
+
+ data_cmd[0] = 0;
+ data_cmd[1] = (address == MMC5633_TEMPERATURE) ?
+ MMC5633_HDR_CTRL0_MEAS_T : MMC5633_HDR_CTRL0_MEAS_M;
+
+ ret = i3c_device_do_xfers(data->i3cdev, xfers_wr_cmd, 1, I3C_HDR_DDR);
+ if (ret < 0)
+ return ret;
+
+ ready = (address == MMC5633_TEMPERATURE) ?
+ MMC5633_STATUS1_MEAS_T_DONE_BIT : MMC5633_STATUS1_MEAS_M_DONE_BIT;
+ ret = read_poll_timeout(i3c_device_do_xfers, val,
+ val ||
+ status[0] & ready,
+ 10 * USEC_PER_MSEC,
+ 100 * 10 * USEC_PER_MSEC, 0,
+ data->i3cdev, xfers_rd_sta_cmd, 1, I3C_HDR_DDR);
+ if (ret) {
+ dev_err(data->dev, "data not ready\n");
+ return ret;
+ }
+ if (val) {
+ dev_err(data->dev, "i3c transfer error\n");
+ return val;
+ }
+ return i3c_device_do_xfers(data->i3cdev, xfers_rd_data_cmd, 1, I3C_HDR_DDR);
+ }
+
+ /* Fallback to use SDR/I2C mode */
+ ret = mmc5633_take_measurement(data, address);
+ if (ret < 0)
+ return ret;
+
+ if (address == MMC5633_TEMPERATURE)
+ /*
+ * Put tempeature to last byte of buff to align HDR case.
+ * I3C will early terminate data read if previous data is not
+ * available.
+ */
+ return regmap_bulk_read(data->regmap, MMC5633_REG_TOUT, buf + sz - 1, 1);
+
+ return regmap_bulk_read(data->regmap, MMC5633_REG_XOUT_L, buf, sz);
+}
+
+/* X,Y,Z 3 channels, each channel has 3 byte and TEMP */
+#define MMC5633_ALL_SIZE (3 * 3 + 1)
+
+static int mmc5633_get_raw(struct mmc5633_data *data, int index, unsigned char *buf, int *val)
+{
+ if (index == MMC5633_TEMPERATURE) {
+ *val = buf[MMC5633_ALL_SIZE - 1];
+ return 0;
+ }
+ /*
+ * X[19..12] X[11..4] Y[19..12] Y[11..4] Z[19..12] Z[11..4] X[3..0] Y[3..0] Z[3..0]
+ */
+ *val = get_unaligned_be16(buf + 2 * index) << 4;
+ *val |= buf[index + 6] >> 4;
+
+ return 0;
+}
+
+static int mmc5633_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ struct mmc5633_data *data = iio_priv(indio_dev);
+ char buf[MMC5633_ALL_SIZE];
+ unsigned int reg, i;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ scoped_guard(mutex, &data->mutex) {
+ ret = mmc5633_read_measurement(data, chan->address, buf, MMC5633_ALL_SIZE);
+ if (ret < 0)
+ return ret;
+ }
+
+ ret = mmc5633_get_raw(data, chan->address, buf, val);
+ if (ret < 0)
+ return ret;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ if (chan->type == IIO_MAGN) {
+ *val = 0;
+ *val2 = 62500;
+ } else {
+ *val = 0;
+ *val2 = 800000000; /* 0.8C */
+ }
+ return IIO_VAL_INT_PLUS_NANO;
+ case IIO_CHAN_INFO_OFFSET:
+ if (chan->type == IIO_TEMP) {
+ *val = -75;
+ return IIO_VAL_INT;
+ }
+ return -EINVAL;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ scoped_guard(mutex, &data->mutex) {
+ ret = regmap_read(data->regmap, MMC5633_REG_CTRL1, ®);
+ if (ret < 0)
+ return ret;
+ }
+
+ i = FIELD_GET(MMC5633_CTRL1_BW_MASK, reg);
+ if (i >= ARRAY_SIZE(mmc5633_samp_freq))
+ return -EINVAL;
+
+ *val = mmc5633_samp_freq[i].val;
+ *val2 = mmc5633_samp_freq[i].val2;
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int mmc5633_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val,
+ int val2, long mask)
+{
+ struct mmc5633_data *data = iio_priv(indio_dev);
+ int i, ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ i = mmc5633_get_samp_freq_index(data, val, val2);
+ if (i < 0)
+ return -EINVAL;
+
+ scoped_guard(mutex, &data->mutex) {
+ ret = regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
+ MMC5633_CTRL1_BW_MASK,
+ FIELD_PREP(MMC5633_CTRL1_BW_MASK, i));
+ if (ret)
+ return ret;
+ };
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int mmc5633_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals,
+ int *type,
+ int *length,
+ long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *vals = (const int *)mmc5633_samp_freq;
+ *length = ARRAY_SIZE(mmc5633_samp_freq) * 2;
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info mmc5633_info = {
+ .read_raw = mmc5633_read_raw,
+ .write_raw = mmc5633_write_raw,
+ .read_avail = mmc5633_read_avail,
+};
+
+static bool mmc5633_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case MMC5633_REG_CTRL0:
+ case MMC5633_REG_CTRL1:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool mmc5633_is_readable_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case MMC5633_REG_XOUT_L:
+ case MMC5633_REG_XOUT_H:
+ case MMC5633_REG_YOUT_L:
+ case MMC5633_REG_YOUT_H:
+ case MMC5633_REG_ZOUT_L:
+ case MMC5633_REG_ZOUT_H:
+ case MMC5633_REG_XOUT_2:
+ case MMC5633_REG_YOUT_2:
+ case MMC5633_REG_ZOUT_2:
+ case MMC5633_REG_TOUT:
+ case MMC5633_REG_STATUS1:
+ case MMC5633_REG_ID:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool mmc5633_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case MMC5633_REG_CTRL0:
+ case MMC5633_REG_CTRL1:
+ return false;
+ default:
+ return true;
+ }
+}
+
+static const struct reg_default mmc5633_reg_defaults[] = {
+ { MMC5633_REG_CTRL0, 0x00 },
+ { MMC5633_REG_CTRL1, 0x00 },
+};
+
+static const struct regmap_config mmc5633_regmap_config = {
+ .name = "mmc5633_regmap",
+
+ .reg_bits = 8,
+ .val_bits = 8,
+
+ .max_register = MMC5633_REG_ID,
+ .cache_type = REGCACHE_MAPLE,
+
+ .writeable_reg = mmc5633_is_writeable_reg,
+ .readable_reg = mmc5633_is_readable_reg,
+ .volatile_reg = mmc5633_is_volatile_reg,
+
+ .reg_defaults = mmc5633_reg_defaults,
+ .num_reg_defaults = ARRAY_SIZE(mmc5633_reg_defaults),
+};
+
+static int mmc5633_common_probe(struct device *dev, struct regmap *regmap,
+ char *name, struct i3c_device *i3cdev)
+{
+ struct mmc5633_data *data;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ dev_set_drvdata(dev, indio_dev);
+
+ data = iio_priv(indio_dev);
+
+ data->regmap = regmap;
+ data->i3cdev = i3cdev;
+ data->dev = dev;
+
+ ret = devm_mutex_init(dev, &data->mutex);
+ if (ret)
+ return ret;
+
+ indio_dev->info = &mmc5633_info;
+ indio_dev->name = name;
+ indio_dev->channels = mmc5633_channels;
+ indio_dev->num_channels = ARRAY_SIZE(mmc5633_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = mmc5633_init(data);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "mmc5633 chip init failed\n");
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+
+static int mmc5633_suspend(struct device *dev)
+{
+ struct mmc5633_data *data = iio_priv(dev_get_drvdata(dev));
+
+ regcache_cache_only(data->regmap, true);
+
+ return 0;
+}
+
+static int mmc5633_resume(struct device *dev)
+{
+ struct mmc5633_data *data = iio_priv(dev_get_drvdata(dev));
+ int ret;
+
+ regcache_mark_dirty(data->regmap);
+ ret = regcache_sync_region(data->regmap, MMC5633_REG_CTRL0,
+ MMC5633_REG_CTRL1);
+ if (ret < 0)
+ dev_err(dev, "Failed to restore control registers\n");
+
+ regcache_cache_only(data->regmap, false);
+
+ return 0;
+}
+
+static int mmc5633_i2c_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init_i2c(client, &mmc5633_regmap_config);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed");
+
+ return mmc5633_common_probe(dev, regmap, client->name, NULL);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(mmc5633_pm_ops, mmc5633_suspend, mmc5633_resume);
+
+static const struct of_device_id mmc5633_of_match[] = {
+ { .compatible = "memsic,mmc5603" },
+ { .compatible = "memsic,mmc5633" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, mmc5633_of_match);
+
+static const struct i2c_device_id mmc5633_i2c_id[] = {
+ { "mmc5603" },
+ { "mmc5633" },
+ { }
+};
+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,
+};
+
+static const struct i3c_device_id mmc5633_i3c_ids[] = {
+ I3C_DEVICE(0x0251, 0x0000, NULL),
+ { }
+};
+MODULE_DEVICE_TABLE(i3c, mmc5633_i3c_ids);
+
+static int mmc5633_i3c_probe(struct i3c_device *i3cdev)
+{
+ struct device *dev = i3cdev_to_dev(i3cdev);
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init_i3c(i3cdev, &mmc5633_regmap_config);
+ if (IS_ERR(regmap))
+ return dev_err_probe(dev, PTR_ERR(regmap),
+ "Failed to register i3c regmap\n");
+
+ return mmc5633_common_probe(dev, regmap, "mmc5633_i3c", i3cdev);
+}
+
+static struct i3c_driver mmc5633_i3c_driver = {
+ .driver = {
+ .name = "mmc5633_i3c",
+ },
+ .probe = mmc5633_i3c_probe,
+ .id_table = mmc5633_i3c_ids,
+};
+module_i3c_i2c_driver(mmc5633_i3c_driver, &mmc5633_i2c_driver)
+
+MODULE_AUTHOR("Frank Li <Frank.li@nxp.com>");
+MODULE_DESCRIPTION("MEMSIC MMC5633 magnetic sensor driver");
+MODULE_LICENSE("GPL");
--
2.34.1On Mon, Oct 27, 2025 at 04:08:33PM -0400, Frank Li 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.
...
+ time.h // for time constants
...
> +struct mmc5633_data {
> + struct device *dev;
> + struct i3c_device *i3cdev;
> + struct mutex mutex; /* protect to finish one whole measurement */
> + struct regmap *regmap;
regmap has struct device, i3c_device presumable also, and here is struct
device. Don't we have some overhead?
> +};
...
> +static const struct {
> + int val;
> + int val2;
No need. Just
> +} mmc5633_samp_freq[] = {
static const int mmc5633_samp_freq[][2] = {
> + { 1, 200000 },
> + { 2, 0 },
> + { 3, 500000 },
> + { 6, 600000 },
> +};
...
> +static int mmc5633_get_samp_freq_index(struct mmc5633_data *data,
> + int val, int val2)
> +{
> + u32 i;
unsigned int is enough. uXX rather would suggest we use it as a such, but here
you compare with size_t and return an int.
> +
> + for (i = 0; i < ARRAY_SIZE(mmc5633_samp_freq); i++)
> + if (mmc5633_samp_freq[i].val == val &&
> + mmc5633_samp_freq[i].val2 == val2)
> + return i;
> + return -EINVAL;
> +}
...
> +static int mmc5633_init(struct mmc5633_data *data)
> +{
> + unsigned int reg_id;
> + int ret;
> +
> + ret = regmap_read(data->regmap, MMC5633_REG_ID, ®_id);
> + if (ret < 0)
These ' < 0' have an explanation? Or do we expect regmap_*() return positive
numbers here? If the latter, it would be a least problem here.
> + return dev_err_probe(data->dev, ret,
> + "Error reading product id\n");
> +
> + /*
> + * Make sure we restore sensor characteristics, by doing
> + * a SET/RESET sequence, the axis polarity being naturally
> + * aligned after RESET.
> + */
> + ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_SET);
> + if (ret < 0)
> + return ret;
Ditto and so on...
> + /*
> + * Minimum time interval between SET or RESET to other operations is
> + * 1ms according to Operating Timing Diagram in datasheet.
> + */
> + fsleep(MMC5633_WAIT_SET_RESET_US);
> +
> + ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_RESET);
> + if (ret < 0)
> + return ret;
> +
> + /* set default sampling frequency */
> + return regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
> + MMC5633_CTRL1_BW_MASK,
> + FIELD_PREP(MMC5633_CTRL1_BW_MASK, 0));
> +}
...
> +static int mmc5633_read_measurement(struct mmc5633_data *data, int address, void *buf, size_t sz)
> +{
> + u8 data_cmd[2], status[2];
> + int ret, val, ready;
> +
> + if (mmc5633_is_support_hdr(data)) {
> + struct i3c_xfer xfers_wr_cmd[] = {
> + {
> + .cmd = 0x3b,
> + .len = 2,
> + .data.out = data_cmd,
> + }
> + };
> + struct i3c_xfer xfers_rd_sta_cmd[] = {
> + {
> + .cmd = 0x23 | BIT(7), /* RDSTA CMD */
> + .len = 2,
> + .data.in = status,
> + },
> + };
> + struct i3c_xfer xfers_rd_data_cmd[] = {
> + {
> + .cmd = 0x22 | BIT(7), /* RDLONG CMD */
> + .len = sz,
> + .data.in = buf,
> + },
> + };
> +
> + data_cmd[0] = 0;
> + data_cmd[1] = (address == MMC5633_TEMPERATURE) ?
> + MMC5633_HDR_CTRL0_MEAS_T : MMC5633_HDR_CTRL0_MEAS_M;
> +
> + ret = i3c_device_do_xfers(data->i3cdev, xfers_wr_cmd, 1, I3C_HDR_DDR);
1 --> ARRAY_SIZE(), but TBH I do not see the point them to be an array to begin
with. Any elaboration on the data type choice?
> + if (ret < 0)
> + return ret;
> +
> + ready = (address == MMC5633_TEMPERATURE) ?
> + MMC5633_STATUS1_MEAS_T_DONE_BIT : MMC5633_STATUS1_MEAS_M_DONE_BIT;
> + ret = read_poll_timeout(i3c_device_do_xfers, val,
> + val ||
> + status[0] & ready,
> + 10 * USEC_PER_MSEC,
> + 100 * 10 * USEC_PER_MSEC, 0,
> + data->i3cdev, xfers_rd_sta_cmd, 1, I3C_HDR_DDR);
> + if (ret) {
> + dev_err(data->dev, "data not ready\n");
> + return ret;
> + }
> + if (val) {
> + dev_err(data->dev, "i3c transfer error\n");
> + return val;
> + }
> + return i3c_device_do_xfers(data->i3cdev, xfers_rd_data_cmd, 1, I3C_HDR_DDR);
> + }
> +
> + /* Fallback to use SDR/I2C mode */
> + ret = mmc5633_take_measurement(data, address);
> + if (ret < 0)
> + return ret;
> +
> + if (address == MMC5633_TEMPERATURE)
> + /*
> + * Put tempeature to last byte of buff to align HDR case.
> + * I3C will early terminate data read if previous data is not
> + * available.
> + */
> + return regmap_bulk_read(data->regmap, MMC5633_REG_TOUT, buf + sz - 1, 1);
> +
> + return regmap_bulk_read(data->regmap, MMC5633_REG_XOUT_L, buf, sz);
> +}
...
> +static int mmc5633_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct mmc5633_data *data = iio_priv(indio_dev);
> + int i, ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + i = mmc5633_get_samp_freq_index(data, val, val2);
> + if (i < 0)
> + return -EINVAL;
Hmm... Can't that return already an error and we just pass it to the caller?
> + scoped_guard(mutex, &data->mutex) {
> + ret = regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
> + MMC5633_CTRL1_BW_MASK,
> + FIELD_PREP(MMC5633_CTRL1_BW_MASK, i));
> + if (ret)
> + return ret;
> + };
> + return 0;
I didn't get why scoped_guard() is used when simple guard()() will do in
shorter all this.
guard(mutex)(&data->mutex);
return regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
MMC5633_CTRL1_BW_MASK,
FIELD_PREP(MMC5633_CTRL1_BW_MASK, i));
With that you may drop 'int i' part and use ret in the above.
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int mmc5633_common_probe(struct device *dev, struct regmap *regmap,
> + char *name, struct i3c_device *i3cdev)
> +{
> + struct mmc5633_data *data;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> + dev_set_drvdata(dev, indio_dev);
If you use regmap stored device this won't be needed. See below.
> + data = iio_priv(indio_dev);
> +
> + data->regmap = regmap;
> + data->i3cdev = i3cdev;
> + data->dev = dev;
> +
> + ret = devm_mutex_init(dev, &data->mutex);
> + if (ret)
> + return ret;
> +
> + indio_dev->info = &mmc5633_info;
> + indio_dev->name = name;
> + indio_dev->channels = mmc5633_channels;
> + indio_dev->num_channels = ARRAY_SIZE(mmc5633_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = mmc5633_init(data);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "mmc5633 chip init failed\n");
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static int mmc5633_suspend(struct device *dev)
> +{
> + struct mmc5633_data *data = iio_priv(dev_get_drvdata(dev));
Than regmap will be derived directly from a device.
> + regcache_cache_only(data->regmap, true);
> +
> + return 0;
> +}
> +
> +static int mmc5633_resume(struct device *dev)
> +{
Ditto.
> + struct mmc5633_data *data = iio_priv(dev_get_drvdata(dev));
> + int ret;
> +
> + regcache_mark_dirty(data->regmap);
> + ret = regcache_sync_region(data->regmap, MMC5633_REG_CTRL0,
> + MMC5633_REG_CTRL1);
> + if (ret < 0)
> + dev_err(dev, "Failed to restore control registers\n");
> +
> + regcache_cache_only(data->regmap, false);
> +
> + return 0;
> +}
...
> + return mmc5633_common_probe(dev, regmap, client->name, NULL);
dev can be derived from regmap.
...
> + return mmc5633_common_probe(dev, regmap, "mmc5633_i3c", i3cdev);
Ditto.
struct i3c_device doesn't have a name, does it?
--
With Best Regards,
Andy Shevchenko
On Tue, Oct 28, 2025 at 12:50:55PM +0200, Andy Shevchenko wrote:
> On Mon, Oct 27, 2025 at 04:08:33PM -0400, Frank Li wrote:
After update patch, I met some problem, so reply at prevous your email.
> > + struct device *dev;
> > + struct i3c_device *i3cdev;
> > + struct mutex mutex; /* protect to finish one whole measurement */
> > + struct regmap *regmap;
>
> regmap has struct device, i3c_device presumable also, and here is struct
> device. Don't we have some overhead?
*dev already removed. i3cdev is kept as my reply at yesterday.
>
> > +static const struct {
> > + int val;
> > + int val2;
>
> No need. Just
>
> > +} mmc5633_samp_freq[] = {
There are some place will like
if (mmc5633_samp_freq[i][0] == val &&
mmc5633_samp_freq[i][1] == val2)
previous
if (mmc5633_samp_freq[i].val == val &&
mmc5633_samp_freq[i].val2 == val2)
Previous version seem have better readablity. But it is not big deal, if
you like, I change to [0][1].
>
> struct i3c_device doesn't have a name, does it?
It has name, but it is hexnumber (VID+PID), like 0-4a20000f000.
So use friend/readable name here.
Frank
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Wed, Oct 29, 2025 at 11:54:39AM -0400, Frank Li wrote:
> On Tue, Oct 28, 2025 at 12:50:55PM +0200, Andy Shevchenko wrote:
> > On Mon, Oct 27, 2025 at 04:08:33PM -0400, Frank Li wrote:
...
> > > +static const struct {
> > > + int val;
> > > + int val2;
> >
> > No need. Just
> >
> > > +} mmc5633_samp_freq[] = {
>
> There are some place will like
>
> if (mmc5633_samp_freq[i][0] == val &&
> mmc5633_samp_freq[i][1] == val2)
>
> previous
> if (mmc5633_samp_freq[i].val == val &&
> mmc5633_samp_freq[i].val2 == val2)
>
> Previous version seem have better readablity. But it is not big deal, if
> you like, I change to [0][1].
It's not my preference, it's how all but this driver do in IIO, it's about
consistency of the style / patterns.
...
> > struct i3c_device doesn't have a name, does it?
>
> It has name, but it is hexnumber (VID+PID), like 0-4a20000f000.
> So use friend/readable name here.
I think it's better to use the name provided by the subsystem. This way we may
guarantee the unique one. The hard coded values potentially might collide
(imagine some I³C driver for the very similar chip, let's say, in hwmon).
--
With Best Regards,
Andy Shevchenko
On Tue, Oct 28, 2025 at 12:50:55PM +0200, Andy Shevchenko wrote:
> On Mon, Oct 27, 2025 at 04:08:33PM -0400, Frank Li 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.
>
> ...
>
> + time.h // for time constants
>
> ...
>
> > +struct mmc5633_data {
> > + struct device *dev;
> > + struct i3c_device *i3cdev;
> > + struct mutex mutex; /* protect to finish one whole measurement */
> > + struct regmap *regmap;
>
> regmap has struct device, i3c_device presumable also, and here is struct
> device. Don't we have some overhead?
i3cdev is used for check it is i2c host or i3c host. If device connect to
i2c host, i3cdev will be NULL.
Only if connect to i3c host, driver can use i3c transfer api. The HDR
command is quite difference with SDR or I2C, which hard to wrap into regmap.
Anyway we need varible to indicate i3c or i2c. struct i3c_device *i3cdev
will be simple and needn't force convert struct device in regmap.
>
> > +};
>
> ...
>
...
>
> > +static int mmc5633_common_probe(struct device *dev, struct regmap *regmap,
> > + char *name, struct i3c_device *i3cdev)
> > +{
> > + struct mmc5633_data *data;
> > + struct iio_dev *indio_dev;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > + if (!indio_dev)
> > + return -ENOMEM;
>
> > + dev_set_drvdata(dev, indio_dev);
>
> If you use regmap stored device this won't be needed. See below.
>
> > + data = iio_priv(indio_dev);
> > +
> > + data->regmap = regmap;
> > + data->i3cdev = i3cdev;
> > + data->dev = dev;
> > +
> > + ret = devm_mutex_init(dev, &data->mutex);
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->info = &mmc5633_info;
> > + indio_dev->name = name;
> > + indio_dev->channels = mmc5633_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(mmc5633_channels);
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + ret = mmc5633_init(data);
> > + if (ret < 0)
> > + return dev_err_probe(dev, ret, "mmc5633 chip init failed\n");
> > +
> > + return devm_iio_device_register(dev, indio_dev);
> > +}
> > +
> > +static int mmc5633_suspend(struct device *dev)
> > +{
> > + struct mmc5633_data *data = iio_priv(dev_get_drvdata(dev));
>
> Than regmap will be derived directly from a device.
I have not got your idea. Can you point me a example?
Frank
>
> > + regcache_cache_only(data->regmap, true);
> > +
> > + return 0;
> > +}
> > +
> > +static int mmc5633_resume(struct device *dev)
> > +{
>
> Ditto.
>
> > + struct mmc5633_data *data = iio_priv(dev_get_drvdata(dev));
> > + int ret;
> > +
> > + regcache_mark_dirty(data->regmap);
> > + ret = regcache_sync_region(data->regmap, MMC5633_REG_CTRL0,
> > + MMC5633_REG_CTRL1);
> > + if (ret < 0)
> > + dev_err(dev, "Failed to restore control registers\n");
> > +
> > + regcache_cache_only(data->regmap, false);
> > +
> > + return 0;
> > +}
>
> ...
>
> > + return mmc5633_common_probe(dev, regmap, client->name, NULL);
>
> dev can be derived from regmap.
>
> ...
>
>
> > + return mmc5633_common_probe(dev, regmap, "mmc5633_i3c", i3cdev);
>
> Ditto.
>
> struct i3c_device doesn't have a name, does it?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
On Tue, Oct 28, 2025 at 11:31:11AM -0400, Frank Li wrote:
> On Tue, Oct 28, 2025 at 12:50:55PM +0200, Andy Shevchenko wrote:
> > On Mon, Oct 27, 2025 at 04:08:33PM -0400, Frank Li wrote:
...
> > + time.h // for time constants
First of all, please, please remove the context you are agree with
(non-replying to).
...
> > > +struct mmc5633_data {
> > > + struct device *dev;
> > > + struct i3c_device *i3cdev;
> > > + struct mutex mutex; /* protect to finish one whole measurement */
> > > + struct regmap *regmap;
> >
> > regmap has struct device, i3c_device presumable also, and here is struct
> > device. Don't we have some overhead?
>
> i3cdev is used for check it is i2c host or i3c host. If device connect to
> i2c host, i3cdev will be NULL.
>
> Only if connect to i3c host, driver can use i3c transfer api. The HDR
> command is quite difference with SDR or I2C, which hard to wrap into regmap.
>
> Anyway we need varible to indicate i3c or i2c. struct i3c_device *i3cdev
> will be simple and needn't force convert struct device in regmap.
This answers only to part of my question. Okay, let's leave it there, why do we
need an explicit struct device then?
> > > +};
...
> > > +static int mmc5633_suspend(struct device *dev)
> > > +{
> > > + struct mmc5633_data *data = iio_priv(dev_get_drvdata(dev));
> >
> > Than regmap will be derived directly from a device.
>
> I have not got your idea. Can you point me a example?
dev_get_regmap()
> > > + regcache_cache_only(data->regmap, true);
> > > +
> > > + return 0;
> > > +}
...
> > > + return mmc5633_common_probe(dev, regmap, "mmc5633_i3c", i3cdev);
> >
> > struct i3c_device doesn't have a name, does it?
No answer?
--
With Best Regards,
Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.