* [PATCH v2 1/4] i3c: Add HDR API support
2025-09-24 14:30 [PATCH v2 0/4] i3c: Add basic HDR mode support Frank Li
@ 2025-09-24 14:30 ` Frank Li
2025-09-24 14:30 ` [PATCH v2 2/4] i3c: master: svc: Add basic HDR mode support Frank Li
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Frank Li @ 2025-09-24 14:30 UTC (permalink / raw)
To: Alexandre Belloni, Miquel Raynal, Jonathan Cameron, David Lechner,
Nuno Sá, Andy Shevchenko
Cc: linux-i3c, linux-kernel, imx, linux-iio, Frank Li
Rename struct i3c_priv_xfer to struct i3c_xfer, since private xfer in the
I3C spec refers only to SDR transfers. Ref: i3c spec ver1.2, section 3,
Technical Overview.
i3c_xfer will be used for both SDR and HDR.
Add i3c_device_do_xfers() with an HDR mode argument, while keeping
i3c_device_do_priv_xfers() as a wrapper that calls i3c_device_do_xfers()
with I3C_SDR for backward compatibility.
Introduce a 'cmd' field in struct i3c_xfer as an anonymous union with
'rnw', since HDR mode uses read/write commands instead of the SDR address
bit.
Add .i3c_xfers() callback for master controllers. If not implemented, fall
back to SDR with .priv_xfers(). The .priv_xfers() API can be removed once
all controllers switch to .i3c_xfers().
Add 'mode_mask' bitmask to advertise controller capability.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Why not add hdr mode in struct i3c_priv_xfer because mode can't be mixed in
one i3c transfer. for example, can't send a HDR follow one SDR between
START and STOP.
i3c_priv_xfer should be treat as whole i3c transactions. If user want send
HDR follow SDR, should be call i3c_device_do_priv_xfers_mode() twice,
instead put into a big i3c_priv_xfer[n].
change in v2
- don't use 'priv_' since it is refer to sdr mode transfer in spec.
- add 'mode_mask' indicate controller's capibility.
- add helper function to check master's supported transfer mode.
---
drivers/i3c/device.c | 27 ++++++++++++++++++++-------
drivers/i3c/internals.h | 6 +++---
drivers/i3c/master.c | 22 ++++++++++++++++++----
include/linux/i3c/device.h | 28 ++++++++++++++++++++++------
include/linux/i3c/master.h | 5 +++++
5 files changed, 68 insertions(+), 20 deletions(-)
diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
index 2396545763ff853097d9f0173787e087f7a6e688..00706b47758bc164178a5578a018b36c5c433f5f 100644
--- a/drivers/i3c/device.c
+++ b/drivers/i3c/device.c
@@ -15,12 +15,12 @@
#include "internals.h"
/**
- * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a
- * specific device
+ * i3c_device_do_xfers() - do I3C transfers directed to a specific device
*
* @dev: device with which the transfers should be done
* @xfers: array of transfers
* @nxfers: number of transfers
+ * @mode: transfer mode
*
* Initiate one or several private SDR transfers with @dev.
*
@@ -33,9 +33,8 @@
* 'xfers' some time later. See I3C spec ver 1.1.1 09-Jun-2021. Section:
* 5.1.2.2.3.
*/
-int i3c_device_do_priv_xfers(struct i3c_device *dev,
- struct i3c_priv_xfer *xfers,
- int nxfers)
+int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers,
+ int nxfers, enum i3c_hdr_mode mode)
{
int ret, i;
@@ -48,12 +47,12 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev,
}
i3c_bus_normaluse_lock(dev->bus);
- ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
+ ret = i3c_dev_do_xfers_locked(dev->desc, xfers, nxfers, mode);
i3c_bus_normaluse_unlock(dev->bus);
return ret;
}
-EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);
+EXPORT_SYMBOL_GPL(i3c_device_do_xfers);
/**
* i3c_device_do_setdasa() - do I3C dynamic address assignement with
@@ -260,6 +259,20 @@ i3c_device_match_id(struct i3c_device *i3cdev,
}
EXPORT_SYMBOL_GPL(i3c_device_match_id);
+/**
+ * i3c_device_get_supported_xfer_mode - Returns the supported transfer mode by
+ * connected master controller.
+ * @dev: I3C device
+ *
+ * Return: a bit mask, which supported transfer mode, bit position is defined at
+ * enum i3c_hdr_mode
+ */
+u32 i3c_device_get_supported_xfer_mode(struct i3c_device *dev)
+{
+ return i3c_dev_get_master(dev->desc)->mode_mask;
+}
+EXPORT_SYMBOL_GPL(i3c_device_get_supported_xfer_mode);
+
/**
* i3c_driver_register_with_owner() - register an I3C device driver
*
diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 0d857cc68cc5d473db733b12ffcec0c1c28d9def..2adba9136f3d147b82c58bd9b491d6d1bc6bfdf7 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -15,9 +15,9 @@ void i3c_bus_normaluse_lock(struct i3c_bus *bus);
void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
int i3c_dev_setdasa_locked(struct i3c_dev_desc *dev);
-int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
- struct i3c_priv_xfer *xfers,
- int nxfers);
+int i3c_dev_do_xfers_locked(struct i3c_dev_desc *dev,
+ struct i3c_xfer *xfers,
+ int nxfers, enum i3c_hdr_mode mode);
int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev);
int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);
int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 2ef898a8fd8065032b68c97c52dcb12e771525a4..1ba21fd737a31386704e47afb3026c4fc8fc7305 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2749,10 +2749,13 @@ EXPORT_SYMBOL_GPL(i3c_generic_ibi_recycle_slot);
static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
{
- if (!ops || !ops->bus_init || !ops->priv_xfers ||
+ if (!ops || !ops->bus_init ||
!ops->send_ccc_cmd || !ops->do_daa || !ops->i2c_xfers)
return -EINVAL;
+ if (!ops->priv_xfers && !ops->i3c_xfers)
+ return -EINVAL;
+
if (ops->request_ibi &&
(!ops->enable_ibi || !ops->disable_ibi || !ops->free_ibi ||
!ops->recycle_ibi_slot))
@@ -2808,6 +2811,9 @@ int i3c_master_register(struct i3c_master_controller *master,
master->dev.release = i3c_masterdev_release;
master->ops = ops;
master->secondary = secondary;
+ /* Spec require must support SDR mode */
+ master->mode_mask |= BIT(I3C_SDR);
+
INIT_LIST_HEAD(&master->boardinfo.i2c);
INIT_LIST_HEAD(&master->boardinfo.i3c);
@@ -2942,9 +2948,8 @@ int i3c_dev_setdasa_locked(struct i3c_dev_desc *dev)
dev->boardinfo->init_dyn_addr);
}
-int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
- struct i3c_priv_xfer *xfers,
- int nxfers)
+int i3c_dev_do_xfers_locked(struct i3c_dev_desc *dev, struct i3c_xfer *xfers,
+ int nxfers, enum i3c_hdr_mode mode)
{
struct i3c_master_controller *master;
@@ -2955,9 +2960,18 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
if (!master || !xfers)
return -EINVAL;
+ if (!(master->mode_mask & BIT(mode)))
+ return -EOPNOTSUPP;
+
+ if (master->ops->i3c_xfers)
+ return master->ops->i3c_xfers(dev, xfers, nxfers, mode);
+
if (!master->ops->priv_xfers)
return -EOPNOTSUPP;
+ if (mode != I3C_SDR)
+ return -EINVAL;
+
return master->ops->priv_xfers(dev, xfers, nxfers);
}
diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
index 7f136de4b73ef839fb4a1837a87b1aebbddbfe93..be7d9e4c98e09ec29357d19dc73d1f050d7bde1e 100644
--- a/include/linux/i3c/device.h
+++ b/include/linux/i3c/device.h
@@ -40,19 +40,22 @@ enum i3c_error_code {
/**
* enum i3c_hdr_mode - HDR mode ids
+ * @I3C_SDR: SDR mode (NOT HDR mode)
* @I3C_HDR_DDR: DDR mode
* @I3C_HDR_TSP: TSP mode
* @I3C_HDR_TSL: TSL mode
*/
enum i3c_hdr_mode {
+ I3C_SDR,
I3C_HDR_DDR,
I3C_HDR_TSP,
I3C_HDR_TSL,
};
/**
- * struct i3c_priv_xfer - I3C SDR private transfer
+ * struct i3c_xfer - I3C data transfer
* @rnw: encodes the transfer direction. true for a read, false for a write
+ * @cmd: Read/Write command in HDR mode, read: 0x80 - 0xff, write: 0x00 - 0x7f
* @len: transfer length in bytes of the transfer
* @actual_len: actual length in bytes are transferred by the controller
* @data: input/output buffer
@@ -60,8 +63,11 @@ enum i3c_hdr_mode {
* @data.out: output buffer. Must point to a DMA-able buffer
* @err: I3C error code
*/
-struct i3c_priv_xfer {
- u8 rnw;
+struct i3c_xfer {
+ union {
+ u8 rnw;
+ u8 cmd;
+ };
u16 len;
u16 actual_len;
union {
@@ -71,6 +77,9 @@ struct i3c_priv_xfer {
enum i3c_error_code err;
};
+/* keep back compatible */
+#define i3c_priv_xfer i3c_xfer
+
/**
* enum i3c_dcr - I3C DCR values
* @I3C_DCR_GENERIC_DEVICE: generic I3C device
@@ -297,9 +306,15 @@ static __always_inline void i3c_i2c_driver_unregister(struct i3c_driver *i3cdrv,
i3c_i2c_driver_unregister, \
__i2cdrv)
-int i3c_device_do_priv_xfers(struct i3c_device *dev,
- struct i3c_priv_xfer *xfers,
- int nxfers);
+int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_xfer *xfers,
+ int nxfers, enum i3c_hdr_mode mode);
+
+static inline int i3c_device_do_priv_xfers(struct i3c_device *dev,
+ struct i3c_priv_xfer *xfers,
+ int nxfers)
+{
+ return i3c_device_do_xfers(dev, xfers, nxfers, I3C_SDR);
+}
int i3c_device_do_setdasa(struct i3c_device *dev);
@@ -341,5 +356,6 @@ int i3c_device_request_ibi(struct i3c_device *dev,
void i3c_device_free_ibi(struct i3c_device *dev);
int i3c_device_enable_ibi(struct i3c_device *dev);
int i3c_device_disable_ibi(struct i3c_device *dev);
+u32 i3c_device_get_supported_xfer_mode(struct i3c_device *dev);
#endif /* I3C_DEV_H */
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 043f5c7ff398ff631f1eea6acfc54a2e871016d8..379e789637ae261c009d13601642f9a1c9199540 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -477,6 +477,9 @@ struct i3c_master_controller_ops {
int (*priv_xfers)(struct i3c_dev_desc *dev,
struct i3c_priv_xfer *xfers,
int nxfers);
+ int (*i3c_xfers)(struct i3c_dev_desc *dev,
+ struct i3c_priv_xfer *xfers,
+ int nxfers, enum i3c_hdr_mode mode);
int (*attach_i2c_dev)(struct i2c_dev_desc *dev);
void (*detach_i2c_dev)(struct i2c_dev_desc *dev);
int (*i2c_xfers)(struct i2c_dev_desc *dev,
@@ -505,6 +508,7 @@ struct i3c_master_controller_ops {
* @secondary: true if the master is a secondary master
* @init_done: true when the bus initialization is done
* @hotjoin: true if the master support hotjoin
+ * @mode_mask: bit mask for supported transfer mode
* @boardinfo.i3c: list of I3C boardinfo objects
* @boardinfo.i2c: list of I2C boardinfo objects
* @boardinfo: board-level information attached to devices connected on the bus
@@ -528,6 +532,7 @@ struct i3c_master_controller {
unsigned int secondary : 1;
unsigned int init_done : 1;
unsigned int hotjoin: 1;
+ unsigned int mode_mask;
struct {
struct list_head i3c;
struct list_head i2c;
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 2/4] i3c: master: svc: Add basic HDR mode support
2025-09-24 14:30 [PATCH v2 0/4] i3c: Add basic HDR mode support Frank Li
2025-09-24 14:30 ` [PATCH v2 1/4] i3c: Add HDR API support Frank Li
@ 2025-09-24 14:30 ` Frank Li
2025-09-29 15:38 ` Miquel Raynal
2025-09-24 14:30 ` [PATCH v2 3/4] dt-bindings: trivial-devices: add MEMSIC 3-axis magnetometer Frank Li
2025-09-24 14:30 ` [PATCH v2 4/4] iio: magnetometer: Add mmc5633 sensor Frank Li
3 siblings, 1 reply; 11+ messages in thread
From: Frank Li @ 2025-09-24 14:30 UTC (permalink / raw)
To: Alexandre Belloni, Miquel Raynal, Jonathan Cameron, David Lechner,
Nuno Sá, Andy Shevchenko
Cc: linux-i3c, linux-kernel, imx, linux-iio, Frank Li, Carlos Song
Add basic HDR mode support for the svs I3C master driver.
Only support for private transfers and does not support sending CCC
commands in HDR mode.
Key differences:
- HDR uses commands (0x00-0x7F for write, 0x80-0xFF for read) to
distinguish transfer direction.
- HDR read/write commands must be written to FIFO before issuing the I3C
address command. The hardware automatically sends the standard CCC command
to enter HDR mode.
- HDR exit pattern must be sent instead of send a stop after transfer
completion.
- Read/write data size must be an even number.
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>
---
change in v2
- support HDR DDR write
- rdterm unit is byte, not words (RM is wrong).
---
drivers/i3c/master/svc-i3c-master.c | 95 +++++++++++++++++++++++++++++--------
1 file changed, 76 insertions(+), 19 deletions(-)
diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 701ae165b25b7991360f3a862b34cc1870a9a2ba..3885edea90db4c943a5f13ec4a291ed15cf9decb 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -40,11 +40,13 @@
#define SVC_I3C_MCTRL_REQUEST_NONE 0
#define SVC_I3C_MCTRL_REQUEST_START_ADDR 1
#define SVC_I3C_MCTRL_REQUEST_STOP 2
+#define SVC_I3C_MCTRL_REQUEST_FORCE_EXIT 6
#define SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK 3
#define SVC_I3C_MCTRL_REQUEST_PROC_DAA 4
#define SVC_I3C_MCTRL_REQUEST_AUTO_IBI 7
#define SVC_I3C_MCTRL_TYPE_I3C 0
#define SVC_I3C_MCTRL_TYPE_I2C BIT(4)
+#define SVC_I3C_MCTRL_TYPE_DDR BIT(5)
#define SVC_I3C_MCTRL_IBIRESP_AUTO 0
#define SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE 0
#define SVC_I3C_MCTRL_IBIRESP_ACK_WITH_BYTE BIT(7)
@@ -95,6 +97,7 @@
#define SVC_I3C_MINTMASKED 0x098
#define SVC_I3C_MERRWARN 0x09C
#define SVC_I3C_MERRWARN_NACK BIT(2)
+#define SVC_I3C_MERRWARN_CRC BIT(10)
#define SVC_I3C_MERRWARN_TIMEOUT BIT(20)
#define SVC_I3C_MDMACTRL 0x0A0
#define SVC_I3C_MDATACTRL 0x0AC
@@ -165,7 +168,7 @@
struct svc_i3c_cmd {
u8 addr;
- bool rnw;
+ u8 rnw;
u8 *in;
const void *out;
unsigned int len;
@@ -383,6 +386,21 @@ svc_i3c_master_dev_from_addr(struct svc_i3c_master *master,
return master->descs[i];
}
+static bool svc_is_read(u8 rnw_cmd, u32 type)
+{
+ return (type == SVC_I3C_MCTRL_TYPE_DDR) ? !!(rnw_cmd & 0x80) : rnw_cmd;
+}
+
+static void svc_i3c_master_emit_force_exit(struct svc_i3c_master *master)
+{
+ u32 reg = 0;
+
+ writel(SVC_I3C_MCTRL_REQUEST_FORCE_EXIT, master->regs + SVC_I3C_MCTRL);
+ readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
+ SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
+ udelay(1);
+}
+
static void svc_i3c_master_emit_stop(struct svc_i3c_master *master)
{
writel(SVC_I3C_MCTRL_REQUEST_STOP, master->regs + SVC_I3C_MCTRL);
@@ -1272,7 +1290,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
}
static int svc_i3c_master_xfer(struct svc_i3c_master *master,
- bool rnw, unsigned int xfer_type, u8 addr,
+ u8 rnw, unsigned int xfer_type, u8 addr,
u8 *in, const u8 *out, unsigned int xfer_len,
unsigned int *actual_len, bool continued, bool repeat_start)
{
@@ -1283,12 +1301,22 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
/* clean SVC_I3C_MINT_IBIWON w1c bits */
writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
+ if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR) {
+ /* DDR command need prefill into FIFO */
+ writel(rnw, master->regs + SVC_I3C_MWDATAB);
+ if (!svc_is_read(rnw, xfer_type)) {
+ /* write data also need prefill into FIFO */
+ ret = svc_i3c_master_write(master, out, xfer_len);
+ if (ret)
+ goto emit_stop;
+ }
+ }
while (retry--) {
writel(SVC_I3C_MCTRL_REQUEST_START_ADDR |
xfer_type |
SVC_I3C_MCTRL_IBIRESP_NACK |
- SVC_I3C_MCTRL_DIR(rnw) |
+ SVC_I3C_MCTRL_DIR(svc_is_read(rnw, xfer_type)) |
SVC_I3C_MCTRL_ADDR(addr) |
SVC_I3C_MCTRL_RDTERM(*actual_len),
master->regs + SVC_I3C_MCTRL);
@@ -1373,15 +1401,14 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
break;
}
}
-
- if (rnw)
+ if (svc_is_read(rnw, xfer_type))
ret = svc_i3c_master_read(master, in, xfer_len);
- else
+ else if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR)
ret = svc_i3c_master_write(master, out, xfer_len);
if (ret < 0)
goto emit_stop;
- if (rnw)
+ if (svc_is_read(rnw, xfer_type))
*actual_len = ret;
ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
@@ -1389,10 +1416,19 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
if (ret)
goto emit_stop;
+ if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR &&
+ (readl(master->regs + SVC_I3C_MERRWARN) & SVC_I3C_MERRWARN_CRC)) {
+ ret = -ENXIO;
+ goto emit_stop;
+ }
+
writel(SVC_I3C_MINT_COMPLETE, master->regs + SVC_I3C_MSTATUS);
if (!continued) {
- svc_i3c_master_emit_stop(master);
+ if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR)
+ svc_i3c_master_emit_stop(master);
+ else
+ svc_i3c_master_emit_force_exit(master);
/* Wait idle if stop is sent. */
readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
@@ -1402,7 +1438,11 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
return 0;
emit_stop:
- svc_i3c_master_emit_stop(master);
+ if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR)
+ svc_i3c_master_emit_stop(master);
+ else
+ svc_i3c_master_emit_force_exit(master);
+
svc_i3c_master_clear_merrwarn(master);
svc_i3c_master_flush_fifo(master);
@@ -1449,6 +1489,11 @@ static void svc_i3c_master_dequeue_xfer(struct svc_i3c_master *master,
spin_unlock_irqrestore(&master->xferqueue.lock, flags);
}
+static int mode_to_type(enum i3c_hdr_mode mode)
+{
+ return (mode == I3C_SDR) ? SVC_I3C_MCTRL_TYPE_I3C : SVC_I3C_MCTRL_TYPE_DDR;
+}
+
static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
{
struct svc_i3c_xfer *xfer = master->xferqueue.cur;
@@ -1638,9 +1683,8 @@ static int svc_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
return ret;
}
-static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
- struct i3c_priv_xfer *xfers,
- int nxfers)
+static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
+ int nxfers, enum i3c_hdr_mode mode)
{
struct i3c_master_controller *m = i3c_dev_get_master(dev);
struct svc_i3c_master *master = to_svc_i3c_master(m);
@@ -1648,22 +1692,33 @@ static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
struct svc_i3c_xfer *xfer;
int ret, i;
+ if (mode != I3C_SDR) {
+ /*
+ * Only support data size less than FIFO SIZE when use DDR mode.
+ * First entry is cmd in FIFO, so actual available FIFO for data
+ * is SVC_I3C_FIFO_SIZE - 2 since DDR only support even length.
+ */
+ for (i = 0; i < nxfers; i++)
+ if (xfers[i].len > SVC_I3C_FIFO_SIZE - 2)
+ return -EINVAL;
+ }
+
xfer = svc_i3c_master_alloc_xfer(master, nxfers);
if (!xfer)
return -ENOMEM;
- xfer->type = SVC_I3C_MCTRL_TYPE_I3C;
+ xfer->type = mode_to_type(mode);
for (i = 0; i < nxfers; i++) {
+ u8 rnw_cmd = (mode == I3C_SDR) ? xfers[i].rnw : xfers[i].cmd;
struct svc_i3c_cmd *cmd = &xfer->cmds[i];
-
cmd->xfer = &xfers[i];
cmd->addr = master->addrs[data->index];
- cmd->rnw = xfers[i].rnw;
- cmd->in = xfers[i].rnw ? xfers[i].data.in : NULL;
- cmd->out = xfers[i].rnw ? NULL : xfers[i].data.out;
+ cmd->rnw = rnw_cmd;
+ cmd->in = svc_is_read(rnw_cmd, mode_to_type(mode)) ? xfers[i].data.in : NULL;
+ cmd->out = svc_is_read(rnw_cmd, mode_to_type(mode)) ? NULL : xfers[i].data.out;
cmd->len = xfers[i].len;
- cmd->actual_len = xfers[i].rnw ? xfers[i].len : 0;
+ cmd->actual_len = svc_is_read(rnw_cmd, mode_to_type(mode)) ? xfers[i].len : 0;
cmd->continued = (i + 1) < nxfers;
}
@@ -1858,7 +1913,7 @@ static const struct i3c_master_controller_ops svc_i3c_master_ops = {
.do_daa = svc_i3c_master_do_daa,
.supports_ccc_cmd = svc_i3c_master_supports_ccc_cmd,
.send_ccc_cmd = svc_i3c_master_send_ccc_cmd,
- .priv_xfers = svc_i3c_master_priv_xfers,
+ .i3c_xfers = svc_i3c_master_i3c_xfers,
.i2c_xfers = svc_i3c_master_i2c_xfers,
.request_ibi = svc_i3c_master_request_ibi,
.free_ibi = svc_i3c_master_free_ibi,
@@ -1947,6 +2002,8 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
svc_i3c_master_reset(master);
+ master->base.mode_mask = BIT(I3C_SDR) | BIT(I3C_HDR_DDR);
+
/* Register the master */
ret = i3c_master_register(&master->base, &pdev->dev,
&svc_i3c_master_ops, false);
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/4] i3c: master: svc: Add basic HDR mode support
2025-09-24 14:30 ` [PATCH v2 2/4] i3c: master: svc: Add basic HDR mode support Frank Li
@ 2025-09-29 15:38 ` Miquel Raynal
2025-09-29 15:55 ` Frank Li
0 siblings, 1 reply; 11+ messages in thread
From: Miquel Raynal @ 2025-09-29 15:38 UTC (permalink / raw)
To: Frank Li
Cc: Alexandre Belloni, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, linux-i3c, linux-kernel, imx, linux-iio,
Carlos Song
Hi Frank,
> struct svc_i3c_cmd {
> u8 addr;
> - bool rnw;
> + u8 rnw;
You used a union in the core, which makes sense I believe. I guess you
should do it here as well?
> u8 *in;
> const void *out;
> unsigned int len;
> @@ -383,6 +386,21 @@ svc_i3c_master_dev_from_addr(struct svc_i3c_master *master,
> return master->descs[i];
> }
Maybe we should:
- First change the type of the command and make use of the helper to
derive the fact that it is a read
then
- Introduce HDR support.
Because there seems to be a lot of changes which are induced by this
internal API change and not related to HDR introduction at all.
>
> +static bool svc_is_read(u8 rnw_cmd, u32 type)
Can we name this helper svc_cmd_is_read() ?
> +{
> + return (type == SVC_I3C_MCTRL_TYPE_DDR) ? !!(rnw_cmd & 0x80) : rnw_cmd;
> +}
> +
> +static void svc_i3c_master_emit_force_exit(struct svc_i3c_master *master)
> +{
> + u32 reg = 0;
> +
> + writel(SVC_I3C_MCTRL_REQUEST_FORCE_EXIT, master->regs + SVC_I3C_MCTRL);
> + readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> + SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
> + udelay(1);
> +}
> +
> static void svc_i3c_master_emit_stop(struct svc_i3c_master *master)
> {
> writel(SVC_I3C_MCTRL_REQUEST_STOP, master->regs + SVC_I3C_MCTRL);
> @@ -1272,7 +1290,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
> }
>
> static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> - bool rnw, unsigned int xfer_type, u8 addr,
> + u8 rnw, unsigned int xfer_type, u8 addr,
> u8 *in, const u8 *out, unsigned int xfer_len,
> unsigned int *actual_len, bool continued, bool repeat_start)
> {
> @@ -1283,12 +1301,22 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> /* clean SVC_I3C_MINT_IBIWON w1c bits */
> writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
>
> + if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR) {
> + /* DDR command need prefill into FIFO */
> + writel(rnw, master->regs + SVC_I3C_MWDATAB);
> + if (!svc_is_read(rnw, xfer_type)) {
> + /* write data also need prefill into FIFO */
> + ret = svc_i3c_master_write(master, out, xfer_len);
> + if (ret)
> + goto emit_stop;
> + }
> + }
>
> while (retry--) {
> writel(SVC_I3C_MCTRL_REQUEST_START_ADDR |
> xfer_type |
> SVC_I3C_MCTRL_IBIRESP_NACK |
> - SVC_I3C_MCTRL_DIR(rnw) |
> + SVC_I3C_MCTRL_DIR(svc_is_read(rnw, xfer_type)) |
> SVC_I3C_MCTRL_ADDR(addr) |
> SVC_I3C_MCTRL_RDTERM(*actual_len),
> master->regs + SVC_I3C_MCTRL);
> @@ -1373,15 +1401,14 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> break;
> }
> }
> -
> - if (rnw)
> + if (svc_is_read(rnw, xfer_type))
> ret = svc_i3c_master_read(master, in, xfer_len);
> - else
> + else if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR)
> ret = svc_i3c_master_write(master, out, xfer_len);
> if (ret < 0)
> goto emit_stop;
>
> - if (rnw)
> + if (svc_is_read(rnw, xfer_type))
> *actual_len = ret;
>
> ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> @@ -1389,10 +1416,19 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> if (ret)
> goto emit_stop;
>
> + if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR &&
> + (readl(master->regs + SVC_I3C_MERRWARN) & SVC_I3C_MERRWARN_CRC)) {
> + ret = -ENXIO;
> + goto emit_stop;
> + }
> +
> writel(SVC_I3C_MINT_COMPLETE, master->regs + SVC_I3C_MSTATUS);
>
> if (!continued) {
> - svc_i3c_master_emit_stop(master);
> + if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR)
> + svc_i3c_master_emit_stop(master);
> + else
> + svc_i3c_master_emit_force_exit(master);
>
> /* Wait idle if stop is sent. */
> readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> @@ -1402,7 +1438,11 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> return 0;
>
> emit_stop:
> - svc_i3c_master_emit_stop(master);
> + if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR)
> + svc_i3c_master_emit_stop(master);
> + else
> + svc_i3c_master_emit_force_exit(master);
> +
> svc_i3c_master_clear_merrwarn(master);
> svc_i3c_master_flush_fifo(master);
>
> @@ -1449,6 +1489,11 @@ static void svc_i3c_master_dequeue_xfer(struct svc_i3c_master *master,
> spin_unlock_irqrestore(&master->xferqueue.lock, flags);
> }
>
> +static int mode_to_type(enum i3c_hdr_mode mode)
Maybe "i3c_mode_to_svc_type()"?
> +{
> + return (mode == I3C_SDR) ? SVC_I3C_MCTRL_TYPE_I3C : SVC_I3C_MCTRL_TYPE_DDR;
> +}
> +
> static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
> {
> struct svc_i3c_xfer *xfer = master->xferqueue.cur;
> @@ -1638,9 +1683,8 @@ static int svc_i3c_master_send_ccc_cmd(struct i3c_master_controller *m,
> return ret;
> }
>
> -static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> - struct i3c_priv_xfer *xfers,
> - int nxfers)
> +static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
> + int nxfers, enum i3c_hdr_mode mode)
> {
> struct i3c_master_controller *m = i3c_dev_get_master(dev);
> struct svc_i3c_master *master = to_svc_i3c_master(m);
> @@ -1648,22 +1692,33 @@ static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> struct svc_i3c_xfer *xfer;
> int ret, i;
>
> + if (mode != I3C_SDR) {
> + /*
> + * Only support data size less than FIFO SIZE when use
> DDR mode.
when using DDR mode.
> + * First entry is cmd in FIFO, so actual available FIFO for data
> + * is SVC_I3C_FIFO_SIZE - 2 since DDR only support even
> length.
supports
> + */
> + for (i = 0; i < nxfers; i++)
> + if (xfers[i].len > SVC_I3C_FIFO_SIZE - 2)
> + return -EINVAL;
> + }
> +
> xfer = svc_i3c_master_alloc_xfer(master, nxfers);
> if (!xfer)
> return -ENOMEM;
>
> - xfer->type = SVC_I3C_MCTRL_TYPE_I3C;
> + xfer->type = mode_to_type(mode);
>
> for (i = 0; i < nxfers; i++) {
> + u8 rnw_cmd = (mode == I3C_SDR) ? xfers[i].rnw : xfers[i].cmd;
> struct svc_i3c_cmd *cmd = &xfer->cmds[i];
> -
Spurious change?
> cmd->xfer = &xfers[i];
> cmd->addr = master->addrs[data->index];
> - cmd->rnw = xfers[i].rnw;
> - cmd->in = xfers[i].rnw ? xfers[i].data.in : NULL;
> - cmd->out = xfers[i].rnw ? NULL : xfers[i].data.out;
> + cmd->rnw = rnw_cmd;
> + cmd->in = svc_is_read(rnw_cmd, mode_to_type(mode)) ? xfers[i].data.in : NULL;
> + cmd->out = svc_is_read(rnw_cmd, mode_to_type(mode)) ? NULL : xfers[i].data.out;
> cmd->len = xfers[i].len;
> - cmd->actual_len = xfers[i].rnw ? xfers[i].len : 0;
> + cmd->actual_len = svc_is_read(rnw_cmd, mode_to_type(mode)) ? xfers[i].len : 0;
> cmd->continued = (i + 1) < nxfers;
> }
>
> @@ -1858,7 +1913,7 @@ static const struct i3c_master_controller_ops svc_i3c_master_ops = {
> .do_daa = svc_i3c_master_do_daa,
> .supports_ccc_cmd = svc_i3c_master_supports_ccc_cmd,
> .send_ccc_cmd = svc_i3c_master_send_ccc_cmd,
> - .priv_xfers = svc_i3c_master_priv_xfers,
> + .i3c_xfers = svc_i3c_master_i3c_xfers,
Didn't you change this name in patch 1? If you kept both naming, it's
fine, otherwise you must do the switch in patch 1 as well to make sure
you don't break the build in the middle of the series.
> .i2c_xfers = svc_i3c_master_i2c_xfers,
> .request_ibi = svc_i3c_master_request_ibi,
> .free_ibi = svc_i3c_master_free_ibi,
> @@ -1947,6 +2002,8 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
>
> svc_i3c_master_reset(master);
>
> + master->base.mode_mask = BIT(I3C_SDR) | BIT(I3C_HDR_DDR);
> +
> /* Register the master */
> ret = i3c_master_register(&master->base, &pdev->dev,
> &svc_i3c_master_ops, false);
Thanks,
Miquèl
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/4] i3c: master: svc: Add basic HDR mode support
2025-09-29 15:38 ` Miquel Raynal
@ 2025-09-29 15:55 ` Frank Li
2025-09-29 16:04 ` Miquel Raynal
0 siblings, 1 reply; 11+ messages in thread
From: Frank Li @ 2025-09-29 15:55 UTC (permalink / raw)
To: Miquel Raynal
Cc: Alexandre Belloni, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, linux-i3c, linux-kernel, imx, linux-iio,
Carlos Song
On Mon, Sep 29, 2025 at 05:38:03PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
...
> > @@ -1858,7 +1913,7 @@ static const struct i3c_master_controller_ops svc_i3c_master_ops = {
> > .do_daa = svc_i3c_master_do_daa,
> > .supports_ccc_cmd = svc_i3c_master_supports_ccc_cmd,
> > .send_ccc_cmd = svc_i3c_master_send_ccc_cmd,
> > - .priv_xfers = svc_i3c_master_priv_xfers,
> > + .i3c_xfers = svc_i3c_master_i3c_xfers,
>
> Didn't you change this name in patch 1? If you kept both naming, it's
> fine, otherwise you must do the switch in patch 1 as well to make sure
> you don't break the build in the middle of the series.
We keep both API for while. After first patch apply, I change all drivers
to i3c_xfers, then remove old .priv_xfers.
Frank
>
> > .i2c_xfers = svc_i3c_master_i2c_xfers,
> > .request_ibi = svc_i3c_master_request_ibi,
> > .free_ibi = svc_i3c_master_free_ibi,
> > @@ -1947,6 +2002,8 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
> >
> > svc_i3c_master_reset(master);
> >
> > + master->base.mode_mask = BIT(I3C_SDR) | BIT(I3C_HDR_DDR);
> > +
> > /* Register the master */
> > ret = i3c_master_register(&master->base, &pdev->dev,
> > &svc_i3c_master_ops, false);
>
> Thanks,
> Miquèl
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/4] i3c: master: svc: Add basic HDR mode support
2025-09-29 15:55 ` Frank Li
@ 2025-09-29 16:04 ` Miquel Raynal
0 siblings, 0 replies; 11+ messages in thread
From: Miquel Raynal @ 2025-09-29 16:04 UTC (permalink / raw)
To: Frank Li
Cc: Alexandre Belloni, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, linux-i3c, linux-kernel, imx, linux-iio,
Carlos Song
On 29/09/2025 at 11:55:31 -04, Frank Li <Frank.li@nxp.com> wrote:
> On Mon, Sep 29, 2025 at 05:38:03PM +0200, Miquel Raynal wrote:
>> Hi Frank,
>>
> ...
>
>> > @@ -1858,7 +1913,7 @@ static const struct i3c_master_controller_ops svc_i3c_master_ops = {
>> > .do_daa = svc_i3c_master_do_daa,
>> > .supports_ccc_cmd = svc_i3c_master_supports_ccc_cmd,
>> > .send_ccc_cmd = svc_i3c_master_send_ccc_cmd,
>> > - .priv_xfers = svc_i3c_master_priv_xfers,
>> > + .i3c_xfers = svc_i3c_master_i3c_xfers,
>>
>> Didn't you change this name in patch 1? If you kept both naming, it's
>> fine, otherwise you must do the switch in patch 1 as well to make sure
>> you don't break the build in the middle of the series.
>
> We keep both API for while. After first patch apply, I change all drivers
> to i3c_xfers, then remove old .priv_xfers.
Fine. Maybe mark it deprecated with a comment in the core? (I haven't
checked).
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/4] dt-bindings: trivial-devices: add MEMSIC 3-axis magnetometer
2025-09-24 14:30 [PATCH v2 0/4] i3c: Add basic HDR mode support Frank Li
2025-09-24 14:30 ` [PATCH v2 1/4] i3c: Add HDR API support Frank Li
2025-09-24 14:30 ` [PATCH v2 2/4] i3c: master: svc: Add basic HDR mode support Frank Li
@ 2025-09-24 14:30 ` Frank Li
2025-09-24 14:30 ` [PATCH v2 4/4] iio: magnetometer: Add mmc5633 sensor Frank Li
3 siblings, 0 replies; 11+ messages in thread
From: Frank Li @ 2025-09-24 14:30 UTC (permalink / raw)
To: Alexandre Belloni, Miquel Raynal, Jonathan Cameron, David Lechner,
Nuno Sá, Andy Shevchenko
Cc: linux-i3c, linux-kernel, imx, linux-iio, Frank Li
Add compatible string 'memsic,mmc5633' for MEMSIC 3-axis magnetometer.
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 7609acaa752d5c1c89a26bb007fa38357dee1a28..373e36e5460c033c0c39a51c3c2c1cdee3970982 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -225,6 +225,8 @@ properties:
- meas,tsys01
# MEMSIC magnetometer
- memsic,mmc35240
+ # MEMSIC 3-axis magnetometer
+ - memsic,mmc5633
# MEMSIC 3-axis accelerometer
- memsic,mxc4005
# MEMSIC 2-axis 8-bit digital accelerometer
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 4/4] iio: magnetometer: Add mmc5633 sensor
2025-09-24 14:30 [PATCH v2 0/4] i3c: Add basic HDR mode support Frank Li
` (2 preceding siblings ...)
2025-09-24 14:30 ` [PATCH v2 3/4] dt-bindings: trivial-devices: add MEMSIC 3-axis magnetometer Frank Li
@ 2025-09-24 14:30 ` Frank Li
2025-09-24 21:22 ` Adrian Fluturel
2025-09-27 18:58 ` Jonathan Cameron
3 siblings, 2 replies; 11+ messages in thread
From: Frank Li @ 2025-09-24 14:30 UTC (permalink / raw)
To: Alexandre Belloni, Miquel Raynal, Jonathan Cameron, David Lechner,
Nuno Sá, Andy Shevchenko
Cc: linux-i3c, linux-kernel, imx, linux-iio, Frank Li, Carlos Song
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>
---
change in v2
- new patch
---
drivers/iio/magnetometer/Kconfig | 12 +
drivers/iio/magnetometer/Makefile | 1 +
drivers/iio/magnetometer/mmc5633.c | 543 +++++++++++++++++++++++++++++++++++++
3 files changed, 556 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..b1a6973ea175634bbc2247ff84488ea5393eba0e
--- /dev/null
+++ b/drivers/iio/magnetometer/mmc5633.c
@@ -0,0 +1,543 @@
+// 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>
+
+#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)
+
+#define MMC5633_CTRL1_BW_MASK (MMC5633_CTRL1_BW0_BIT | \
+ MMC5633_CTRL1_BW1_BIT)
+
+#define MMC5633_WAIT_CHARGE_PUMP 50000 /* us */
+#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
+
+enum mmc5633_axis {
+ AXIS_X,
+ AXIS_Y,
+ AXIS_Z,
+};
+
+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},
+ };
+
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("1.2 2.0 3.5 6.6");
+
+#define MMC5633_CHANNEL(_axis) { \
+ .type = IIO_MAGN, \
+ .modified = 1, \
+ .channel2 = IIO_MOD_ ## _axis, \
+ .address = 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),
+};
+
+static struct attribute *mmc5633_attributes[] = {
+ &iio_const_attr_sampling_frequency_available.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group mmc5633_attribute_group = {
+ .attrs = mmc5633_attributes,
+};
+
+static int mmc5633_get_samp_freq_index(struct mmc5633_data *data,
+ int val, int val2)
+{
+ int 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_hw_set(struct mmc5633_data *data, bool set)
+{
+ u8 coil_bit;
+
+ if (set)
+ coil_bit = MMC5633_CTRL0_SET;
+ else
+ coil_bit = MMC5633_CTRL0_RESET;
+
+ return regmap_write(data->regmap, MMC5633_REG_CTRL0, coil_bit);
+}
+
+static int mmc5633_init(struct mmc5633_data *data)
+{
+ unsigned int reg_id, 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 = mmc5633_hw_set(data, true);
+ if (ret < 0)
+ return ret;
+
+ usleep_range(MMC5633_WAIT_SET_RESET, MMC5633_WAIT_SET_RESET + 1);
+
+ ret = mmc5633_hw_set(data, false);
+ if (ret < 0)
+ return ret;
+
+ /* set default sampling frequency */
+ ret = regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
+ MMC5633_CTRL1_BW_MASK,
+ FIELD_PREP(MMC5633_CTRL1_BW_MASK, 0));
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int mmc5633_take_measurement(struct mmc5633_data *data)
+{
+ unsigned int reg_status;
+ int ret;
+
+ ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_MEAS_M);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_read_poll_timeout(data->regmap, MMC5633_REG_STATUS1, reg_status,
+ reg_status & MMC5633_STATUS1_MEAS_M_DONE_BIT,
+ 10000, 10000 * 100);
+
+ if (ret) {
+ dev_err(data->dev, "data not ready\n");
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int mmc5633_read_measurement(struct mmc5633_data *data, void *buf, size_t sz)
+{
+ __le16 data_word;
+ __le16 status;
+ int ret, val;
+
+ if (data->i3cdev &&
+ (i3c_device_get_supported_xfer_mode(data->i3cdev) & BIT(I3C_HDR_DDR))) {
+ struct i3c_xfer xfers_wr_cmd[] = {
+ {
+ .cmd = 0x3b,
+ .len = 2,
+ .data.out = &data_word,
+ }
+ };
+
+ 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_word = cpu_to_le16(MMC5633_HDR_CTRL0_MEAS_M << 8);
+
+ ret = i3c_device_do_xfers(data->i3cdev, xfers_wr_cmd, 1, I3C_HDR_DDR);
+ if (ret < 0)
+ return ret;
+
+ ret = read_poll_timeout(i3c_device_do_xfers, val,
+ val ||
+ (le16_to_cpu(status) & MMC5633_STATUS1_MEAS_M_DONE_BIT),
+ 10000, 10000 * 100, 0,
+ data->i3cdev, xfers_rd_sta_cmd, 1, I3C_HDR_DDR);
+
+ if (ret || val) {
+ dev_err(data->dev, "data not ready\n");
+ return -EIO;
+ }
+
+ 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);
+ if (ret < 0)
+ return ret;
+
+ return regmap_bulk_read(data->regmap, MMC5633_REG_XOUT_L, buf, sz);
+}
+
+static int mmc5633_get_raw(struct mmc5633_data *data, int index, unsigned char *buf, int *val)
+{
+ /*
+ * 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 = buf[2 * index];
+ *val <<= 8;
+
+ *val |= buf[2 * index + 1];
+ *val <<= 8;
+
+ *val |= buf[index + 6];
+
+ *val >>= 4;
+
+ return 0;
+}
+
+#define MMC5633_ALL_SIZE (3 * 3 + 1) /* each channel have 3 byte and TEMP */
+
+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;
+ int ret, i;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ scoped_guard(mutex, &data->mutex) {
+ ret = mmc5633_read_measurement(data, 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:
+ *val = 0;
+ *val2 = 62500;
+ return IIO_VAL_INT_PLUS_NANO;
+ 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 < 0 || 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 ret;
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info mmc5633_info = {
+ .read_raw = mmc5633_read_raw,
+ .write_raw = mmc5633_write_raw,
+ .attrs = &mmc5633_attribute_group,
+};
+
+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_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_FLAT,
+
+ .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,mmc5633", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, mmc5633_of_match);
+
+static const struct i2c_device_id mmc5633_i2c_id[] = {
+ { "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.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 4/4] iio: magnetometer: Add mmc5633 sensor
2025-09-24 14:30 ` [PATCH v2 4/4] iio: magnetometer: Add mmc5633 sensor Frank Li
@ 2025-09-24 21:22 ` Adrian Fluturel
2025-09-25 4:19 ` Frank Li
2025-09-27 18:58 ` Jonathan Cameron
1 sibling, 1 reply; 11+ messages in thread
From: Adrian Fluturel @ 2025-09-24 21:22 UTC (permalink / raw)
To: Frank Li; +Cc: Jonathan Cameron, David Lechner, linux-iio
On Wed, Sep 24, 2025 at 10:30:05AM -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.
>
> 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>
> ---
> change in v2
> - new patch
> ---
> drivers/iio/magnetometer/Kconfig | 12 +
> drivers/iio/magnetometer/Makefile | 1 +
> drivers/iio/magnetometer/mmc5633.c | 543 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 556 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..b1a6973ea175634bbc2247ff84488ea5393eba0e
> --- /dev/null
> +++ b/drivers/iio/magnetometer/mmc5633.c
> @@ -0,0 +1,543 @@
> +// 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>
> +
> +#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)
> +
> +#define MMC5633_CTRL1_BW_MASK (MMC5633_CTRL1_BW0_BIT | \
> + MMC5633_CTRL1_BW1_BIT)
> +
> +#define MMC5633_WAIT_CHARGE_PUMP 50000 /* us */
> +#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
> +
> +enum mmc5633_axis {
> + AXIS_X,
> + AXIS_Y,
> + AXIS_Z,
> +};
> +
> +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},
> + };
> +
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("1.2 2.0 3.5 6.6");
> +
> +#define MMC5633_CHANNEL(_axis) { \
> + .type = IIO_MAGN, \
> + .modified = 1, \
> + .channel2 = IIO_MOD_ ## _axis, \
> + .address = 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),
> +};
> +
> +static struct attribute *mmc5633_attributes[] = {
> + &iio_const_attr_sampling_frequency_available.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group mmc5633_attribute_group = {
> + .attrs = mmc5633_attributes,
> +};
> +
> +static int mmc5633_get_samp_freq_index(struct mmc5633_data *data,
> + int val, int val2)
> +{
> + int 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_hw_set(struct mmc5633_data *data, bool set)
> +{
> + u8 coil_bit;
> +
> + if (set)
> + coil_bit = MMC5633_CTRL0_SET;
> + else
> + coil_bit = MMC5633_CTRL0_RESET;
> +
> + return regmap_write(data->regmap, MMC5633_REG_CTRL0, coil_bit);
> +}
> +
> +static int mmc5633_init(struct mmc5633_data *data)
> +{
> + unsigned int reg_id, 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 = mmc5633_hw_set(data, true);
> + if (ret < 0)
> + return ret;
> +
> + usleep_range(MMC5633_WAIT_SET_RESET, MMC5633_WAIT_SET_RESET + 1);
> +
> + ret = mmc5633_hw_set(data, false);
> + if (ret < 0)
> + return ret;
> +
> + /* set default sampling frequency */
> + ret = regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
> + MMC5633_CTRL1_BW_MASK,
> + FIELD_PREP(MMC5633_CTRL1_BW_MASK, 0));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int mmc5633_take_measurement(struct mmc5633_data *data)
> +{
> + unsigned int reg_status;
> + int ret;
> +
> + ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_MEAS_M);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_read_poll_timeout(data->regmap, MMC5633_REG_STATUS1, reg_status,
> + reg_status & MMC5633_STATUS1_MEAS_M_DONE_BIT,
> + 10000, 10000 * 100);
> +
> + if (ret) {
> + dev_err(data->dev, "data not ready\n");
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int mmc5633_read_measurement(struct mmc5633_data *data, void *buf, size_t sz)
> +{
> + __le16 data_word;
> + __le16 status;
> + int ret, val;
> +
> + if (data->i3cdev &&
> + (i3c_device_get_supported_xfer_mode(data->i3cdev) & BIT(I3C_HDR_DDR))) {
> + struct i3c_xfer xfers_wr_cmd[] = {
> + {
> + .cmd = 0x3b,
> + .len = 2,
> + .data.out = &data_word,
> + }
> + };
> +
> + 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_word = cpu_to_le16(MMC5633_HDR_CTRL0_MEAS_M << 8);
> +
> + ret = i3c_device_do_xfers(data->i3cdev, xfers_wr_cmd, 1, I3C_HDR_DDR);
> + if (ret < 0)
> + return ret;
> +
> + ret = read_poll_timeout(i3c_device_do_xfers, val,
> + val ||
> + (le16_to_cpu(status) & MMC5633_STATUS1_MEAS_M_DONE_BIT),
> + 10000, 10000 * 100, 0,
> + data->i3cdev, xfers_rd_sta_cmd, 1, I3C_HDR_DDR);
> +
> + if (ret || val) {
> + dev_err(data->dev, "data not ready\n");
> + return -EIO;
> + }
> +
> + 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);
> + if (ret < 0)
> + return ret;
> +
> + return regmap_bulk_read(data->regmap, MMC5633_REG_XOUT_L, buf, sz);
> +}
> +
> +static int mmc5633_get_raw(struct mmc5633_data *data, int index, unsigned char *buf, int *val)
> +{
> + /*
> + * 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 = buf[2 * index];
> + *val <<= 8;
> +
> + *val |= buf[2 * index + 1];
> + *val <<= 8;
> +
> + *val |= buf[index + 6];
> +
> + *val >>= 4;
> +
> + return 0;
> +}
> +
> +#define MMC5633_ALL_SIZE (3 * 3 + 1) /* each channel have 3 byte and TEMP */
> +
> +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;
> + int ret, i;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + scoped_guard(mutex, &data->mutex) {
> + ret = mmc5633_read_measurement(data, 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:
> + *val = 0;
> + *val2 = 62500;
> + return IIO_VAL_INT_PLUS_NANO;
> + 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 < 0 || 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 ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info mmc5633_info = {
> + .read_raw = mmc5633_read_raw,
> + .write_raw = mmc5633_write_raw,
> + .attrs = &mmc5633_attribute_group,
> +};
> +
> +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_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_FLAT,
> +
> + .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,mmc5633", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, mmc5633_of_match);
> +
> +static const struct i2c_device_id mmc5633_i2c_id[] = {
> + { "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.1
>
Hello all,
I've been working on a driver for the MMC 5603NJ using I2C these last few days and it seems to be working. As it happens, I noticed your patch and looked over the datasheet and it seems they are very similar, but your sensor has some extra features. My driver also reads the temperature. I am not experienced enough to tell if they are compatible and if we could merge the drivers in some way. Is there a point to me sending my driver? What is the protocol for situations like these?
I would test your driver to see if it works on my sensor, but there are some features there that my board does not have, so in its current form, I don't think I can use it.
Thank you for your consideration,
- Adrian Fluturel
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 4/4] iio: magnetometer: Add mmc5633 sensor
2025-09-24 21:22 ` Adrian Fluturel
@ 2025-09-25 4:19 ` Frank Li
0 siblings, 0 replies; 11+ messages in thread
From: Frank Li @ 2025-09-25 4:19 UTC (permalink / raw)
To: Adrian Fluturel; +Cc: Jonathan Cameron, David Lechner, linux-iio
On Wed, Sep 24, 2025 at 11:22:15PM +0200, Adrian Fluturel wrote:
> On Wed, Sep 24, 2025 at 10:30:05AM -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.
> >
> > 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>
> > ---
> > change in v2
> > - new patch
> > ---
> > drivers/iio/magnetometer/Kconfig | 12 +
> > drivers/iio/magnetometer/Makefile | 1 +
> > drivers/iio/magnetometer/mmc5633.c | 543 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 556 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..b1a6973ea175634bbc2247ff84488ea5393eba0e
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/mmc5633.c
> > @@ -0,0 +1,543 @@
> > +// 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>
> > +
> > +#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)
> > +
> > +#define MMC5633_CTRL1_BW_MASK (MMC5633_CTRL1_BW0_BIT | \
> > + MMC5633_CTRL1_BW1_BIT)
> > +
> > +#define MMC5633_WAIT_CHARGE_PUMP 50000 /* us */
> > +#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
> > +
> > +enum mmc5633_axis {
> > + AXIS_X,
> > + AXIS_Y,
> > + AXIS_Z,
> > +};
> > +
> > +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},
> > + };
> > +
> > +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("1.2 2.0 3.5 6.6");
> > +
> > +#define MMC5633_CHANNEL(_axis) { \
> > + .type = IIO_MAGN, \
> > + .modified = 1, \
> > + .channel2 = IIO_MOD_ ## _axis, \
> > + .address = 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),
> > +};
> > +
> > +static struct attribute *mmc5633_attributes[] = {
> > + &iio_const_attr_sampling_frequency_available.dev_attr.attr,
> > + NULL
> > +};
> > +
> > +static const struct attribute_group mmc5633_attribute_group = {
> > + .attrs = mmc5633_attributes,
> > +};
> > +
> > +static int mmc5633_get_samp_freq_index(struct mmc5633_data *data,
> > + int val, int val2)
> > +{
> > + int 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_hw_set(struct mmc5633_data *data, bool set)
> > +{
> > + u8 coil_bit;
> > +
> > + if (set)
> > + coil_bit = MMC5633_CTRL0_SET;
> > + else
> > + coil_bit = MMC5633_CTRL0_RESET;
> > +
> > + return regmap_write(data->regmap, MMC5633_REG_CTRL0, coil_bit);
> > +}
> > +
> > +static int mmc5633_init(struct mmc5633_data *data)
> > +{
> > + unsigned int reg_id, 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 = mmc5633_hw_set(data, true);
> > + if (ret < 0)
> > + return ret;
> > +
> > + usleep_range(MMC5633_WAIT_SET_RESET, MMC5633_WAIT_SET_RESET + 1);
> > +
> > + ret = mmc5633_hw_set(data, false);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* set default sampling frequency */
> > + ret = regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
> > + MMC5633_CTRL1_BW_MASK,
> > + FIELD_PREP(MMC5633_CTRL1_BW_MASK, 0));
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int mmc5633_take_measurement(struct mmc5633_data *data)
> > +{
> > + unsigned int reg_status;
> > + int ret;
> > +
> > + ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_MEAS_M);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = regmap_read_poll_timeout(data->regmap, MMC5633_REG_STATUS1, reg_status,
> > + reg_status & MMC5633_STATUS1_MEAS_M_DONE_BIT,
> > + 10000, 10000 * 100);
> > +
> > + if (ret) {
> > + dev_err(data->dev, "data not ready\n");
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mmc5633_read_measurement(struct mmc5633_data *data, void *buf, size_t sz)
> > +{
> > + __le16 data_word;
> > + __le16 status;
> > + int ret, val;
> > +
> > + if (data->i3cdev &&
> > + (i3c_device_get_supported_xfer_mode(data->i3cdev) & BIT(I3C_HDR_DDR))) {
> > + struct i3c_xfer xfers_wr_cmd[] = {
> > + {
> > + .cmd = 0x3b,
> > + .len = 2,
> > + .data.out = &data_word,
> > + }
> > + };
> > +
> > + 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_word = cpu_to_le16(MMC5633_HDR_CTRL0_MEAS_M << 8);
> > +
> > + ret = i3c_device_do_xfers(data->i3cdev, xfers_wr_cmd, 1, I3C_HDR_DDR);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = read_poll_timeout(i3c_device_do_xfers, val,
> > + val ||
> > + (le16_to_cpu(status) & MMC5633_STATUS1_MEAS_M_DONE_BIT),
> > + 10000, 10000 * 100, 0,
> > + data->i3cdev, xfers_rd_sta_cmd, 1, I3C_HDR_DDR);
> > +
> > + if (ret || val) {
> > + dev_err(data->dev, "data not ready\n");
> > + return -EIO;
> > + }
> > +
> > + 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);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return regmap_bulk_read(data->regmap, MMC5633_REG_XOUT_L, buf, sz);
> > +}
> > +
> > +static int mmc5633_get_raw(struct mmc5633_data *data, int index, unsigned char *buf, int *val)
> > +{
> > + /*
> > + * 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 = buf[2 * index];
> > + *val <<= 8;
> > +
> > + *val |= buf[2 * index + 1];
> > + *val <<= 8;
> > +
> > + *val |= buf[index + 6];
> > +
> > + *val >>= 4;
> > +
> > + return 0;
> > +}
> > +
> > +#define MMC5633_ALL_SIZE (3 * 3 + 1) /* each channel have 3 byte and TEMP */
> > +
> > +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;
> > + int ret, i;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + scoped_guard(mutex, &data->mutex) {
> > + ret = mmc5633_read_measurement(data, 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:
> > + *val = 0;
> > + *val2 = 62500;
> > + return IIO_VAL_INT_PLUS_NANO;
> > + 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 < 0 || 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 ret;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct iio_info mmc5633_info = {
> > + .read_raw = mmc5633_read_raw,
> > + .write_raw = mmc5633_write_raw,
> > + .attrs = &mmc5633_attribute_group,
> > +};
> > +
> > +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_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_FLAT,
> > +
> > + .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,mmc5633", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, mmc5633_of_match);
> > +
> > +static const struct i2c_device_id mmc5633_i2c_id[] = {
> > + { "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.1
> >
>
> Hello all,
>
> I've been working on a driver for the MMC 5603NJ using I2C these last few days and it seems to be working. As it happens, I noticed your patch and looked over the datasheet and it seems they are very similar, but your sensor has some extra features. My driver also reads the temperature. I am not experienced enough to tell if they are compatible and if we could merge the drivers in some way. Is there a point to me sending my driver? What is the protocol for situations like these?
>
Please wrap your reply at 80 or 100 chars to read easily. If your driver
is good enough, you can post to public list, I can pickup and try to
merge it.
If not, you can send patch to me only.
Frank
> I would test your driver to see if it works on my sensor, but there are some features there that my board does not have, so in its current form, I don't think I can use it.
>
> Thank you for your consideration,
> - Adrian Fluturel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/4] iio: magnetometer: Add mmc5633 sensor
2025-09-24 14:30 ` [PATCH v2 4/4] iio: magnetometer: Add mmc5633 sensor Frank Li
2025-09-24 21:22 ` Adrian Fluturel
@ 2025-09-27 18:58 ` Jonathan Cameron
1 sibling, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2025-09-27 18:58 UTC (permalink / raw)
To: Frank Li
Cc: Alexandre Belloni, Miquel Raynal, David Lechner, Nuno Sá,
Andy Shevchenko, linux-i3c, linux-kernel, imx, linux-iio,
Carlos Song
On Wed, 24 Sep 2025 10:30:05 -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>
> ---
> change in v2
> - new patch
> ---
HI Frank,
Some minor comments inline,
Thanks,
Jonathan
> drivers/iio/magnetometer/Kconfig | 12 +
> drivers/iio/magnetometer/Makefile | 1 +
> drivers/iio/magnetometer/mmc5633.c | 543 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 556 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
> diff --git a/drivers/iio/magnetometer/mmc5633.c b/drivers/iio/magnetometer/mmc5633.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..b1a6973ea175634bbc2247ff84488ea5393eba0e
> --- /dev/null
> +++ b/drivers/iio/magnetometer/mmc5633.c
> @@ -0,0 +1,543 @@
> +// 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
> + *
Trivial but this blank line doesn't add anything, so drop it.
> + */
> +static const struct {
> + int val;
> + int val2;
> +} mmc5633_samp_freq[] = { {1, 200000},
> + {2, 0},
> + {3, 500000},
> + {6, 600000},
> + };
Generally prefer { 1, 20000 }
style for IIO and here I'd format this as:
static const struct {
int val;
int val2;
} mmc5633_samp_freq[] = {
{ 1, 200000 },
{ 2, 0 },
{ 3, 500000 },
{ 6, 600000 },
};
> +
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("1.2 2.0 3.5 6.6");
> +
> +static struct attribute *mmc5633_attributes[] = {
> + &iio_const_attr_sampling_frequency_available.dev_attr.attr,
Please use the read_avail() callback and the associated masks.
That makes it available for in kernel users and generally is preferred
for new drivers. One day I'll get rid of the defines you are using here
but it will take a while yet!
> + NULL
> +};
> +
> +static const struct attribute_group mmc5633_attribute_group = {
> + .attrs = mmc5633_attributes,
> +};
> +
> +static int mmc5633_hw_set(struct mmc5633_data *data, bool set)
> +{
> + u8 coil_bit;
> +
> + if (set)
> + coil_bit = MMC5633_CTRL0_SET;
> + else
> + coil_bit = MMC5633_CTRL0_RESET;
> +
> + return regmap_write(data->regmap, MMC5633_REG_CTRL0, coil_bit);
This helper doesn't provide all that much value. Maybe just
call the regmap_write() inline and let the value written make
it obvious which is set and reset?
> +}
> +
> +static int mmc5633_init(struct mmc5633_data *data)
> +{
> + unsigned int reg_id, 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
Make
> + * a SET/RESET sequence, the axis polarity being naturally
> + * aligned after RESET
RESET.
> + */
> + ret = mmc5633_hw_set(data, true);
> + if (ret < 0)
> + return ret;
> +
> + usleep_range(MMC5633_WAIT_SET_RESET, MMC5633_WAIT_SET_RESET + 1);
> +
> + ret = mmc5633_hw_set(data, false);
> + if (ret < 0)
> + return ret;
> +
> + /* set default sampling frequency */
> + ret = regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
> + MMC5633_CTRL1_BW_MASK,
> + FIELD_PREP(MMC5633_CTRL1_BW_MASK, 0));
> + if (ret < 0)
> + return ret;
> +
> + return 0;
return regmap_upate_bits()
Given regmap always returns negative for errors of 0 for success it
would be better to check if (ret) which then makes this sort of final
call look more obviously correct.
> +}
> +
> +static int mmc5633_take_measurement(struct mmc5633_data *data)
> +{
> + unsigned int reg_status;
> + int ret;
> +
> + ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_MEAS_M);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_read_poll_timeout(data->regmap, MMC5633_REG_STATUS1, reg_status,
> + reg_status & MMC5633_STATUS1_MEAS_M_DONE_BIT,
> + 10000, 10000 * 100);
> +
Drop the blank line here to keep the error check associated with the call
> + if (ret) {
> + dev_err(data->dev, "data not ready\n");
> + return -EIO;
Why not return ret
> + }
> +
> + return 0;
> +}
> +
> +static int mmc5633_read_measurement(struct mmc5633_data *data, void *buf, size_t sz)
> +{
> + __le16 data_word;
> + __le16 status;
Might as well combine on one line.
> + int ret, val;
> +
> + if (data->i3cdev &&
> + (i3c_device_get_supported_xfer_mode(data->i3cdev) & BIT(I3C_HDR_DDR))) {
I'd factor this lot out as a helper function to improve readability a little.
> + struct i3c_xfer xfers_wr_cmd[] = {
> + {
> + .cmd = 0x3b,
> + .len = 2,
> + .data.out = &data_word,
> + }
> + };
> +
> + 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_word = cpu_to_le16(MMC5633_HDR_CTRL0_MEAS_M << 8);
if you are shifting it by 8 bits and it's an 16 bit value, feels like it's actually not
and it's a pair of bytes. Perhaps just set the relevant byte in a u8 [2] would be clearer?
If this is how it's described on the datasheet I guess I don't mind that much.
> +
> + ret = i3c_device_do_xfers(data->i3cdev, xfers_wr_cmd, 1, I3C_HDR_DDR);
> + if (ret < 0)
> + return ret;
> +
> + ret = read_poll_timeout(i3c_device_do_xfers, val,
> + val ||
> + (le16_to_cpu(status) & MMC5633_STATUS1_MEAS_M_DONE_BIT),
> + 10000, 10000 * 100, 0,
> + data->i3cdev, xfers_rd_sta_cmd, 1, I3C_HDR_DDR);
> +
> + if (ret || val) {
> + dev_err(data->dev, "data not ready\n");
> + return -EIO;
Nice to return ret if it is set.
> + }
> +
> + 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);
> + if (ret < 0)
> + return ret;
> +
> + return regmap_bulk_read(data->regmap, MMC5633_REG_XOUT_L, buf, sz);
> +}
> +
> +static int mmc5633_get_raw(struct mmc5633_data *data, int index, unsigned char *buf, int *val)
> +{
> + /*
> + * 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 = buf[2 * index];
> + *val <<= 8;
> +
> + *val |= buf[2 * index + 1];
> + *val <<= 8;
First bit could be a get_unaligned_be16() << 8 or something like that.
> +
> + *val |= buf[index + 6];
> +
> + *val >>= 4;
> +
> + return 0;
> +}
> +
> +#define MMC5633_ALL_SIZE (3 * 3 + 1) /* each channel have 3 byte and TEMP */
> +
> +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;
> + int ret, i;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + scoped_guard(mutex, &data->mutex) {
> + ret = mmc5633_read_measurement(data, 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:
> + *val = 0;
> + *val2 = 62500;
> + return IIO_VAL_INT_PLUS_NANO;
> + 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 < 0 || i >= ARRAY_SIZE(mmc5633_samp_freq))
How does a FIELD_GET() give a negative?
> + return -EINVAL;
> +
> + *val = mmc5633_samp_freq[i].val;
> + *val2 = mmc5633_samp_freq[i].val2;
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + return -EINVAL;
> + }
> +}
>
> +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" },
> + {}
be consistent. I'd prefer
{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, mmc5633_i2c_id);
^ permalink raw reply [flat|nested] 11+ messages in thread