[PATCH v6 0/5] i3c: Add basic HDR mode support

[PATCH v6 0/5] i3c: Add basic HDR mode support

[Frank Li]

Add basic HDR mode support, only support private transfer, not support
CCC command.

Update i3c framework API to allow pass down mode and extend driver callback
function.

Implement HDR transfer in svc i3c master driver.

Simplifed HDR flow is (ref i3c spec line 5514) Figure 129

<--              SDR            ---> | <--- HDR
START 0x7E RnW(0) ACK CCC(ENTHDR0) T   HDR-CMD(00-7f write, 80--ff read)

                                    ----> |
HDR-DATA HDR-CRC HDR-RESTART .... HDR-EXIT

Note: HDR-CMD is 16bit data, which included 7bit slave address and 8bit
read/write command.

svc hardware can auto issue SDR part.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v6:
- remove acpi part
- collect Conor Dooley ack tags
- Link to v5: https://lore.kernel.org/r/20251007-i3c_ddr-v5-0-444184f7725e@nxp.com

Changes in v5:
- Just realized missed CC mail list devicetree@vger.kernel.org and resend
- Link to v4: https://lore.kernel.org/r/20251007-i3c_ddr-v4-0-3afea5105775@nxp.com

Changes in v4:
- use master's hdr_cap to check HDR cap.
- add mmc5603 support.
- Link to v3: https://lore.kernel.org/r/20250930-i3c_ddr-v3-0-b627dc2ef172@nxp.com

Changes in v3:
- Add new patch for change rnw to union for svc.
- Detial changes see each patch's change log.
- Link to v2: https://lore.kernel.org/r/20250924-i3c_ddr-v2-0-682a0eb32572@nxp.com

Changes in v2:
- Add sensor driver, which use HDR mode read/write data.
- change priv_xfer to i3c_xfer.
- Link to v1: https://lore.kernel.org/r/20250129-i3c_ddr-v1-0-028a7a5d4324@nxp.com

---
Frank Li (5):
      i3c: Add HDR API support
      i3c: master: svc: Replace bool rnw with union for HDR support
      i3c: master: svc: Add basic HDR mode support
      dt-bindings: trivial-devices: add MEMSIC 3-axis magnetometer
      iio: magnetometer: Add mmc5633 sensor

 .../devicetree/bindings/trivial-devices.yaml       |   4 +
 drivers/i3c/device.c                               |  27 +-
 drivers/i3c/internals.h                            |   6 +-
 drivers/i3c/master.c                               |  19 +-
 drivers/i3c/master/svc-i3c-master.c                |  96 +++-
 drivers/iio/magnetometer/Kconfig                   |  12 +
 drivers/iio/magnetometer/Makefile                  |   1 +
 drivers/iio/magnetometer/mmc5633.c                 | 571 +++++++++++++++++++++
 include/linux/i3c/device.h                         |  34 +-
 include/linux/i3c/master.h                         |   4 +
 10 files changed, 737 insertions(+), 37 deletions(-)
---
base-commit: 1e546ca7362028be9e96445645c95307f7092731
change-id: 20250129-i3c_ddr-b15488901eb8

Best regards,
--
Frank Li <Frank.Li@nxp.com>

[PATCH v6 1/5] i3c: Add HDR API support

[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.

Rename enum i3c_hdr_mode to i3c_xfer_mode. Previous definition need match
CCC GET_CAP1 bit position. Use 31 as SDR transfer mode.

Add i3c_device_do_xfers() with an xfer 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 v4
- Rename enum i3c_hdr_mode to i3c_xfer_mode.

change in v3
- Add Depreciated comment for priv_xfers.

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       | 19 +++++++++++++++----
 include/linux/i3c/device.h | 34 ++++++++++++++++++++++++++--------
 include/linux/i3c/master.h |  4 ++++
 5 files changed, 68 insertions(+), 22 deletions(-)

diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
index 2396545763ff853097d9f0173787e087f7a6e688..e6add862645196ad41d0c91d3d7103c877a1ef5a 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_xfer_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)->this->info.hdr_cap | BIT(I3C_SDR);
+}
+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..f8d68b9d6474cbc56640a643db3c2c4cd95dd26b 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_xfer_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 8efec085d396cf0c7dd9ee35b636eea9664c634f..b36d4ffadeff20641a1eeb84b702178f05eb891c 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))
@@ -2942,9 +2945,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_xfer_mode mode)
 {
 	struct i3c_master_controller *master;
 
@@ -2955,9 +2957,18 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
 	if (!master || !xfers)
 		return -EINVAL;
 
+	if (mode != I3C_SDR && !(master->this->info.hdr_cap & 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..563e63f6dd99a95d66bd80aff0b760c231b487a9 100644
--- a/include/linux/i3c/device.h
+++ b/include/linux/i3c/device.h
@@ -39,20 +39,25 @@ enum i3c_error_code {
 };
 
 /**
- * enum i3c_hdr_mode - HDR mode ids
+ * enum i3c_xfer_mode - I3C xfer mode ids
  * @I3C_HDR_DDR: DDR mode
  * @I3C_HDR_TSP: TSP mode
  * @I3C_HDR_TSL: TSL mode
+ * @I3C_SDR: SDR mode (NOT HDR mode)
  */
-enum i3c_hdr_mode {
+enum i3c_xfer_mode {
+	/* The below 3 value (I3C_HDR*) must match GETCAP1 Byte bit position */
 	I3C_HDR_DDR,
 	I3C_HDR_TSP,
 	I3C_HDR_TSL,
+	/* Use for default SDR transfer mode */
+	I3C_SDR = 0x31,
 };
 
 /**
- * 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 +65,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 +79,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 +308,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_xfer_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 +358,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..ef1363122066215983d37c8e3ce062f3eefe48ae 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -474,9 +474,13 @@ struct i3c_master_controller_ops {
 				 const struct i3c_ccc_cmd *cmd);
 	int (*send_ccc_cmd)(struct i3c_master_controller *master,
 			    struct i3c_ccc_cmd *cmd);
+	/* Depreciated, please use i3c_xfers() */
 	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_xfer_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,

-- 
2.34.1

[PATCH v6 2/5] i3c: master: svc: Replace bool rnw with union for HDR support

[Frank Li]

Replace the bool rnw field with a union in preparation for adding HDR
support. HDR uses a cmd field instead of the rnw bit to indicate read or
write direction.

Add helper function svc_cmd_is_read() to check transfer direction.

Add a local variable 'rnw' in svc_i3c_master_priv_xfers() to avoid
repeatedly accessing xfers[i].rnw.

No functional change.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 701ae165b25b7991360f3a862b34cc1870a9a2ba..956172dc9d5f1f54d76b4c2917f2d9cf3bd21a85 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -165,7 +165,11 @@
 
 struct svc_i3c_cmd {
 	u8 addr;
-	bool rnw;
+	union {
+		bool rnw;
+		u8 cmd;
+		u32 rnw_cmd;
+	};
 	u8 *in;
 	const void *out;
 	unsigned int len;
@@ -383,6 +387,11 @@ svc_i3c_master_dev_from_addr(struct svc_i3c_master *master,
 	return master->descs[i];
 }
 
+static bool svc_cmd_is_read(u32 rnw_cmd, u32 type)
+{
+	return rnw_cmd;
+}
+
 static void svc_i3c_master_emit_stop(struct svc_i3c_master *master)
 {
 	writel(SVC_I3C_MCTRL_REQUEST_STOP, master->regs + SVC_I3C_MCTRL);
@@ -1272,10 +1281,11 @@ 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,
+			       u32 rnw_cmd, unsigned int xfer_type, u8 addr,
 			       u8 *in, const u8 *out, unsigned int xfer_len,
 			       unsigned int *actual_len, bool continued, bool repeat_start)
 {
+	bool rnw = svc_cmd_is_read(rnw_cmd, xfer_type);
 	int retry = repeat_start ? 1 : 2;
 	u32 reg;
 	int ret;
@@ -1463,7 +1473,7 @@ static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
 	for (i = 0; i < xfer->ncmds; i++) {
 		struct svc_i3c_cmd *cmd = &xfer->cmds[i];
 
-		ret = svc_i3c_master_xfer(master, cmd->rnw, xfer->type,
+		ret = svc_i3c_master_xfer(master, cmd->rnw_cmd, xfer->type,
 					  cmd->addr, cmd->in, cmd->out,
 					  cmd->len, &cmd->actual_len,
 					  cmd->continued, i > 0);
@@ -1656,14 +1666,15 @@ static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
 
 	for (i = 0; i < nxfers; i++) {
 		struct svc_i3c_cmd *cmd = &xfer->cmds[i];
+		bool rnw = xfers[i].rnw;
 
 		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->in = rnw ? xfers[i].data.in : NULL;
+		cmd->out = rnw ? NULL : xfers[i].data.out;
 		cmd->len = xfers[i].len;
-		cmd->actual_len = xfers[i].rnw ? xfers[i].len : 0;
+		cmd->actual_len = rnw ? xfers[i].len : 0;
 		cmd->continued = (i + 1) < nxfers;
 	}
 

-- 
2.34.1

[PATCH v6 3/5] i3c: master: svc: Add basic HDR mode support

[Frank Li]

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 v4
- use hdr_cap.

change in v3
- rename to svc_cmd_is_read()
- rename to i3c_mode_to_svc_type()
- use local varible bool rnw to reduce change

change in v2
- support HDR DDR write
- rdterm unit is byte, not words (RM is wrong).
---
 drivers/i3c/master/svc-i3c-master.c | 77 +++++++++++++++++++++++++++++++------
 1 file changed, 66 insertions(+), 11 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 956172dc9d5f1f54d76b4c2917f2d9cf3bd21a85..f25c9ed561290d2a33046855b84702eb310590b0 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
@@ -389,7 +392,17 @@ svc_i3c_master_dev_from_addr(struct svc_i3c_master *master,
 
 static bool svc_cmd_is_read(u32 rnw_cmd, u32 type)
 {
-	return rnw_cmd;
+	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)
@@ -780,6 +793,8 @@ static int svc_i3c_master_bus_init(struct i3c_master_controller *m)
 
 	info.dyn_addr = ret;
 
+	info.hdr_cap = I3C_CCC_HDR_MODE(I3C_HDR_DDR);
+
 	writel(SVC_MDYNADDR_VALID | SVC_MDYNADDR_ADDR(info.dyn_addr),
 	       master->regs + SVC_I3C_MDYNADDR);
 
@@ -1293,6 +1308,16 @@ 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_cmd, master->regs + SVC_I3C_MWDATAB);
+		if (!rnw) {
+			/* 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 |
@@ -1386,7 +1411,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 
 	if (rnw)
 		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;
@@ -1399,10 +1424,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,
@@ -1412,7 +1446,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);
 
@@ -1459,6 +1497,11 @@ static void svc_i3c_master_dequeue_xfer(struct svc_i3c_master *master,
 	spin_unlock_irqrestore(&master->xferqueue.lock, flags);
 }
 
+static int i3c_mode_to_svc_type(enum i3c_xfer_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;
@@ -1648,9 +1691,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_xfer_mode mode)
 {
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct svc_i3c_master *master = to_svc_i3c_master(m);
@@ -1658,19 +1700,32 @@ 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 using DDR
+		 * mode. First entry is cmd in FIFO, so actual available FIFO
+		 * for data is SVC_I3C_FIFO_SIZE - 2 since DDR only supports
+		 * 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 = i3c_mode_to_svc_type(mode);
 
 	for (i = 0; i < nxfers; i++) {
+		u32 rnw_cmd = (mode == I3C_SDR) ? xfers[i].rnw : xfers[i].cmd;
+		bool rnw = svc_cmd_is_read(rnw_cmd, xfer->type);
 		struct svc_i3c_cmd *cmd = &xfer->cmds[i];
-		bool rnw = xfers[i].rnw;
 
 		cmd->xfer = &xfers[i];
 		cmd->addr = master->addrs[data->index];
-		cmd->rnw = rnw;
+		cmd->rnw_cmd = rnw_cmd;
 		cmd->in = rnw ? xfers[i].data.in : NULL;
 		cmd->out = rnw ? NULL : xfers[i].data.out;
 		cmd->len = xfers[i].len;
@@ -1869,7 +1924,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,

-- 
2.34.1

[PATCH v6 4/5] dt-bindings: trivial-devices: add MEMSIC 3-axis magnetometer

[Frank Li]

Add compatible string 'memsic,mmc5603' and 'memsic,mmc5633' for
MEMSIC 3-axis magnetometer.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v6:
- add Conor Dooley ack tag.

Changes in v5
- none

Changes in v4
- add memsic,mmc5603

Changes from v1 .. v3
- None
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 7609acaa752d5c1c89a26bb007fa38357dee1a28..72786eebfbd63beffd2a09fc20c7aedbe9e96a8e 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -225,6 +225,10 @@ properties:
           - meas,tsys01
             # MEMSIC magnetometer
           - memsic,mmc35240
+            # MEMSIC 3-axis magnetometer
+          - memsic,mmc5603
+            # MEMSIC 3-axis magnetometer (Support I3C HDR)
+          - memsic,mmc5633
             # MEMSIC 3-axis accelerometer
           - memsic,mxc4005
             # MEMSIC 2-axis 8-bit digital accelerometer

-- 
2.34.1

[PATCH v6 5/5] iio: magnetometer: Add mmc5633 sensor

[Frank Li]

Add mmc5633 sensor basic support.
- Support read 20 bits X/Y/Z magnetic.
- Support I3C HDR mode to send start measurememt command.
- Support I3C HDR mode to read all sensors data by one command.

Co-developed-by: Carlos Song <carlos.song@nxp.com>
Signed-off-by: Carlos Song <carlos.song@nxp.com>
Co-developed-by: Adrian Fluturel <fluturel.adrian@gmail.com>
Signed-off-by: Adrian Fluturel <fluturel.adrian@gmail.com>
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change in v6:
- remove acpi part
- return 0 for success path at mmc5633_write_raw

Change in V4
- use { 1, 2000 }
- Add _US for timeout
- Use GEN_MASK for MMC5633_CTRL1_BW_MASK
- Use { } for terminator.
- remove !!
- fix mix tab and space
- add mmc5603 (merge https://lore.kernel.org/all/20251003000731.22927-1-fluturel.adrian@gmail.com/)
- add tempature measure support

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

change in v2
- new patch
---
 drivers/iio/magnetometer/Kconfig   |  12 +
 drivers/iio/magnetometer/Makefile  |   1 +
 drivers/iio/magnetometer/mmc5633.c | 571 +++++++++++++++++++++++++++++++++++++
 3 files changed, 584 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..c862a85a06c1786108cc16d31c93a12997df5578
--- /dev/null
+++ b/drivers/iio/magnetometer/mmc5633.c
@@ -0,0 +1,571 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MMC5633 - MEMSIC 3-axis Magnetic Sensor
+ *
+ * Copyright (c) 2015, Intel Corporation.
+ * Copyright (c) 2025, NXP
+ *
+ * IIO driver for MMC5633, base on mmc35240.c
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/i3c/device.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/init.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/unaligned.h>
+
+#define MMC5633_REG_XOUT_L	0x00
+#define MMC5633_REG_XOUT_H	0x01
+#define MMC5633_REG_YOUT_L	0x02
+#define MMC5633_REG_YOUT_H	0x03
+#define MMC5633_REG_ZOUT_L	0x04
+#define MMC5633_REG_ZOUT_H	0x05
+#define MMC5633_REG_XOUT_2	0x06
+#define MMC5633_REG_YOUT_2	0x07
+#define MMC5633_REG_ZOUT_2	0x08
+#define MMC5633_REG_TOUT	0x09
+
+#define MMC5633_REG_STATUS1	0x18
+#define MMC5633_REG_STATUS0	0x19
+#define MMC5633_REG_CTRL0	0x1b
+#define MMC5633_REG_CTRL1	0x1c
+#define MMC5633_REG_CTRL2	0x1d
+
+#define MMC5633_REG_ID		0x39
+
+#define MMC5633_STATUS1_MEAS_T_DONE_BIT	BIT(7)
+#define MMC5633_STATUS1_MEAS_M_DONE_BIT	BIT(6)
+
+#define MMC5633_CTRL0_CMM_FREQ_EN	BIT(7)
+#define MMC5633_CTRL0_AUTO_ST_EN	BIT(6)
+#define MMC5633_CTRL0_AUTO_SR_EN	BIT(5)
+#define MMC5633_CTRL0_RESET		BIT(4)
+#define MMC5633_CTRL0_SET		BIT(3)
+#define MMC5633_CTRL0_MEAS_T		BIT(1)
+#define MMC5633_CTRL0_MEAS_M		BIT(0)
+
+#define MMC5633_CTRL1_BW_MASK		GENMASK(1, 0)
+
+#define MMC5633_WAIT_SET_RESET_US	1000
+
+#define MMC5633_HDR_CTRL0_MEAS_M	0x01
+#define MMC5633_HDR_CTRL0_MEAS_T	0x03
+#define MMC5633_HDR_CTRL0_SET		0X05
+#define MMC5633_HDR_CTRL0_RESET		0x07
+
+enum mmc5633_axis {
+	MMC5633_AXIS_X,
+	MMC5633_AXIS_Y,
+	MMC5633_AXIS_Z,
+	MMC5633_TEMPERATURE,
+};
+
+struct mmc5633_data {
+	struct device *dev;
+	struct i3c_device *i3cdev;
+	struct mutex mutex; /* protect to finish one whole measurement */
+	struct regmap *regmap;
+};
+
+static const struct {
+	int val;
+	int val2;
+} mmc5633_samp_freq[] = {
+	{ 1, 200000 },
+	{ 2, 0 },
+	{ 3, 500000 },
+	{ 6, 600000 },
+};
+
+#define MMC5633_CHANNEL(_axis) { \
+	.type = IIO_MAGN, \
+	.modified = 1, \
+	.channel2 = IIO_MOD_ ## _axis, \
+	.address = MMC5633_AXIS_ ## _axis, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+				    BIT(IIO_CHAN_INFO_SCALE), \
+}
+
+static const struct iio_chan_spec mmc5633_channels[] = {
+	MMC5633_CHANNEL(X),
+	MMC5633_CHANNEL(Y),
+	MMC5633_CHANNEL(Z),
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_OFFSET),
+		.address = MMC5633_TEMPERATURE,
+	},
+};
+
+static int mmc5633_get_samp_freq_index(struct mmc5633_data *data,
+				       int val, int val2)
+{
+	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_init(struct mmc5633_data *data)
+{
+	unsigned int reg_id, ret;
+
+	ret = regmap_read(data->regmap, MMC5633_REG_ID, &reg_id);
+	if (ret < 0)
+		return dev_err_probe(data->dev, ret,
+				     "Error reading product id\n");
+
+	/*
+	 * Make sure we restore sensor characteristics, by doing
+	 * a SET/RESET sequence, the axis polarity being naturally
+	 * aligned after RESET.
+	 */
+	ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_SET);
+	if (ret < 0)
+		return ret;
+
+	fsleep(MMC5633_WAIT_SET_RESET_US);
+
+	ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_RESET);
+	if (ret < 0)
+		return ret;
+
+	/* set default sampling frequency */
+	return regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
+				  MMC5633_CTRL1_BW_MASK,
+				  FIELD_PREP(MMC5633_CTRL1_BW_MASK, 0));
+}
+
+static int mmc5633_take_measurement(struct mmc5633_data *data, int address)
+{
+	unsigned int reg_status;
+	int ret;
+	int val;
+
+	val = (address == MMC5633_TEMPERATURE) ? MMC5633_CTRL0_MEAS_T : MMC5633_CTRL0_MEAS_M;
+	ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, val);
+	if (ret < 0)
+		return ret;
+
+	val = (address == MMC5633_TEMPERATURE) ?
+	      MMC5633_STATUS1_MEAS_T_DONE_BIT : MMC5633_STATUS1_MEAS_M_DONE_BIT;
+	ret = regmap_read_poll_timeout(data->regmap, MMC5633_REG_STATUS1, reg_status,
+				       reg_status & val, 10000, 10000 * 100);
+	if (ret) {
+		dev_err(data->dev, "data not ready\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static bool mmc5633_is_support_hdr(struct mmc5633_data *data)
+{
+	if (!data->i3cdev)
+		return false;
+
+	return i3c_device_get_supported_xfer_mode(data->i3cdev) & BIT(I3C_HDR_DDR);
+}
+
+static int mmc5633_read_measurement(struct mmc5633_data *data, int address, void *buf, size_t sz)
+{
+	u8 data_cmd[2], status[2];
+	int ret, val, ready;
+
+	if (mmc5633_is_support_hdr(data)) {
+		struct i3c_xfer xfers_wr_cmd[] = {
+			{
+				.cmd = 0x3b,
+				.len = 2,
+				.data.out = data_cmd,
+			}
+		};
+
+		struct i3c_xfer xfers_rd_sta_cmd[] = {
+			{
+				.cmd = 0x23 | BIT(7), /* RDSTA CMD */
+				.len = 2,
+				.data.in = status,
+			},
+		};
+
+		struct i3c_xfer xfers_rd_data_cmd[] = {
+			{
+				.cmd = 0x22 | BIT(7), /* RDLONG CMD */
+				.len = sz,
+				.data.in = buf,
+			},
+		};
+
+		data_cmd[0] = 0;
+		data_cmd[1] = (address == MMC5633_TEMPERATURE) ?
+			      MMC5633_HDR_CTRL0_MEAS_T : MMC5633_HDR_CTRL0_MEAS_M;
+
+		ret = i3c_device_do_xfers(data->i3cdev, xfers_wr_cmd, 1, I3C_HDR_DDR);
+		if (ret < 0)
+			return ret;
+
+		ready = (address == MMC5633_TEMPERATURE) ?
+			MMC5633_STATUS1_MEAS_T_DONE_BIT : MMC5633_STATUS1_MEAS_M_DONE_BIT;
+		ret = read_poll_timeout(i3c_device_do_xfers, val,
+					val ||
+					status[0] & ready,
+					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 ret ? ret : -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, address);
+	if (ret < 0)
+		return ret;
+
+	if (address == MMC5633_TEMPERATURE)
+		/*
+		 * Put tempeature to last byte of buff to align HDR case.
+		 * I3C will early terminate data read if previous data is not
+		 * available.
+		 */
+		return regmap_bulk_read(data->regmap, MMC5633_REG_TOUT, buf + sz - 1, 1);
+
+	return regmap_bulk_read(data->regmap, MMC5633_REG_XOUT_L, buf, sz);
+}
+
+#define MMC5633_ALL_SIZE (3 * 3 + 1) /* each channel have 3 byte and TEMP */
+
+static int mmc5633_get_raw(struct mmc5633_data *data, int index, unsigned char *buf, int *val)
+{
+	if (index == MMC5633_TEMPERATURE) {
+		*val = buf[MMC5633_ALL_SIZE - 1];
+		return 0;
+	}
+	/*
+	 * X[19..12] X[11..4] Y[19..12] Y[11..4] Z[19..12] Z[11..4] X[3..0] Y[3..0] Z[3..0]
+	 */
+	*val = get_unaligned_be16(buf + 2 * index) << 4;
+	*val |= buf[index + 6] >> 4;
+
+	return 0;
+}
+
+static int mmc5633_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long mask)
+{
+	struct mmc5633_data *data = iio_priv(indio_dev);
+	char buf[MMC5633_ALL_SIZE];
+	unsigned int reg;
+	int ret, i;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		scoped_guard(mutex, &data->mutex) {
+			ret = mmc5633_read_measurement(data, chan->address, buf, MMC5633_ALL_SIZE);
+			if (ret < 0)
+				return ret;
+		}
+
+		ret = mmc5633_get_raw(data, chan->address, buf, val);
+		if (ret < 0)
+			return ret;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_MAGN) {
+			*val = 0;
+			*val2 = 62500;
+		} else {
+			*val = 0;
+			*val2 = 800000000; /* 0.8C */
+		}
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_OFFSET:
+		if (chan->type == IIO_TEMP) {
+			*val = -75;
+			return IIO_VAL_INT;
+		}
+		return -EINVAL;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		scoped_guard(mutex, &data->mutex) {
+			ret = regmap_read(data->regmap, MMC5633_REG_CTRL1, &reg);
+			if (ret < 0)
+				return ret;
+		}
+
+		i = FIELD_GET(MMC5633_CTRL1_BW_MASK, reg);
+		if (i >= ARRAY_SIZE(mmc5633_samp_freq))
+			return -EINVAL;
+
+		*val = mmc5633_samp_freq[i].val;
+		*val2 = mmc5633_samp_freq[i].val2;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mmc5633_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	struct mmc5633_data *data = iio_priv(indio_dev);
+	int i, ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		i = mmc5633_get_samp_freq_index(data, val, val2);
+		if (i < 0)
+			return -EINVAL;
+
+		scoped_guard(mutex, &data->mutex) {
+			ret = regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
+						 MMC5633_CTRL1_BW_MASK,
+						 FIELD_PREP(MMC5633_CTRL1_BW_MASK, i));
+			if (ret)
+				return ret;
+		};
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mmc5633_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals,
+			      int *type,
+			      int *length,
+			      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*vals = (const int *)mmc5633_samp_freq;
+		*length = ARRAY_SIZE(mmc5633_samp_freq) * 2;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info mmc5633_info = {
+	.read_raw	= mmc5633_read_raw,
+	.write_raw	= mmc5633_write_raw,
+	.read_avail	= mmc5633_read_avail,
+};
+
+static bool mmc5633_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MMC5633_REG_CTRL0:
+	case MMC5633_REG_CTRL1:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool mmc5633_is_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MMC5633_REG_XOUT_L:
+	case MMC5633_REG_XOUT_H:
+	case MMC5633_REG_YOUT_L:
+	case MMC5633_REG_YOUT_H:
+	case MMC5633_REG_ZOUT_L:
+	case MMC5633_REG_ZOUT_H:
+	case MMC5633_REG_XOUT_2:
+	case MMC5633_REG_YOUT_2:
+	case MMC5633_REG_ZOUT_2:
+	case MMC5633_REG_TOUT:
+	case MMC5633_REG_STATUS1:
+	case MMC5633_REG_ID:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool mmc5633_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MMC5633_REG_CTRL0:
+	case MMC5633_REG_CTRL1:
+		return false;
+	default:
+		return true;
+	}
+}
+
+static const struct reg_default mmc5633_reg_defaults[] = {
+	{ MMC5633_REG_CTRL0,  0x00 },
+	{ MMC5633_REG_CTRL1,  0x00 },
+};
+
+static const struct regmap_config mmc5633_regmap_config = {
+	.name = "mmc5633_regmap",
+
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = MMC5633_REG_ID,
+	.cache_type = REGCACHE_MAPLE,
+
+	.writeable_reg = mmc5633_is_writeable_reg,
+	.readable_reg = mmc5633_is_readable_reg,
+	.volatile_reg = mmc5633_is_volatile_reg,
+
+	.reg_defaults = mmc5633_reg_defaults,
+	.num_reg_defaults = ARRAY_SIZE(mmc5633_reg_defaults),
+};
+
+static int mmc5633_common_probe(struct device *dev, struct regmap *regmap,
+				char *name, struct i3c_device *i3cdev)
+{
+	struct mmc5633_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, indio_dev);
+
+	data = iio_priv(indio_dev);
+
+	data->regmap = regmap;
+	data->i3cdev = i3cdev;
+	data->dev = dev;
+
+	ret = devm_mutex_init(dev, &data->mutex);
+	if (ret)
+		return ret;
+
+	indio_dev->info = &mmc5633_info;
+	indio_dev->name = name;
+	indio_dev->channels = mmc5633_channels;
+	indio_dev->num_channels = ARRAY_SIZE(mmc5633_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = mmc5633_init(data);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "mmc5633 chip init failed\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static int mmc5633_suspend(struct device *dev)
+{
+	struct mmc5633_data *data = iio_priv(dev_get_drvdata(dev));
+
+	regcache_cache_only(data->regmap, true);
+
+	return 0;
+}
+
+static int mmc5633_resume(struct device *dev)
+{
+	struct mmc5633_data *data = iio_priv(dev_get_drvdata(dev));
+	int ret;
+
+	regcache_mark_dirty(data->regmap);
+	ret = regcache_sync_region(data->regmap, MMC5633_REG_CTRL0,
+				   MMC5633_REG_CTRL1);
+	if (ret < 0)
+		dev_err(dev, "Failed to restore control registers\n");
+
+	regcache_cache_only(data->regmap, false);
+
+	return 0;
+}
+
+static int mmc5633_i2c_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init_i2c(client, &mmc5633_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap), "regmap init failed");
+
+	return mmc5633_common_probe(dev, regmap, client->name, NULL);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(mmc5633_pm_ops, mmc5633_suspend,
+				mmc5633_resume);
+
+static const struct of_device_id mmc5633_of_match[] = {
+	{ .compatible = "memsic,mmc5603", },
+	{ .compatible = "memsic,mmc5633", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mmc5633_of_match);
+
+static const struct i2c_device_id mmc5633_i2c_id[] = {
+	{ "mmc5603" },
+	{ "mmc5633" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mmc5633_i2c_id);
+
+static struct i2c_driver mmc5633_i2c_driver = {
+	.driver = {
+		.name = "mmc5633_i2c",
+		.of_match_table = mmc5633_of_match,
+		.pm = pm_sleep_ptr(&mmc5633_pm_ops),
+	},
+	.probe = mmc5633_i2c_probe,
+	.id_table =  mmc5633_i2c_id,
+};
+
+static const struct i3c_device_id mmc5633_i3c_ids[] = {
+	I3C_DEVICE(0x0251, 0x0000, NULL),
+	{ }
+};
+MODULE_DEVICE_TABLE(i3c, mmc5633_i3c_ids);
+
+static int mmc5633_i3c_probe(struct i3c_device *i3cdev)
+{
+	struct device *dev = i3cdev_to_dev(i3cdev);
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init_i3c(i3cdev, &mmc5633_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to register i3c regmap\n");
+
+	return mmc5633_common_probe(dev, regmap, "mmc5633_i3c", i3cdev);
+}
+
+static struct i3c_driver mmc5633_i3c_driver = {
+	.driver = {
+		.name = "mmc5633_i3c",
+	},
+	.probe = mmc5633_i3c_probe,
+	.id_table = mmc5633_i3c_ids,
+};
+
+module_i3c_i2c_driver(mmc5633_i3c_driver, &mmc5633_i2c_driver)
+
+MODULE_AUTHOR("Frank Li <Frank.li@nxp.com>");
+MODULE_DESCRIPTION("MEMSIC MMC5633 magnetic sensor driver");
+MODULE_LICENSE("GPL");

-- 
2.34.1

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

[Jonathan Cameron]

On Tue, 14 Oct 2025 12:40:04 -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>
> Co-developed-by: Adrian Fluturel <fluturel.adrian@gmail.com>
> Signed-off-by: Adrian Fluturel <fluturel.adrian@gmail.com>
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
LGTM.

So my ideal case (assuming i3c parts are good to go) is an immutable
branch in an appropriate tree that gets merged into both i3c and IIO trees
and has first 4 patches.  I'll then put the driver on top in the IIO tree.

I can spin one if that is useful.

Thanks,

Jonathan

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

[Jonathan Cameron]

On Sat, 18 Oct 2025 17:52:32 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 14 Oct 2025 12:40:04 -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>
> > Co-developed-by: Adrian Fluturel <fluturel.adrian@gmail.com>
> > Signed-off-by: Adrian Fluturel <fluturel.adrian@gmail.com>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>  
> LGTM.
> 
> So my ideal case (assuming i3c parts are good to go) is an immutable
> branch in an appropriate tree that gets merged into both i3c and IIO trees
> and has first 4 patches.  I'll then put the driver on top in the IIO tree.
I can't count. First 3 patches.  Binding and driver would go on top in the IIO tree.

> 
> I can spin one if that is useful.
> 
> Thanks,
> 
> Jonathan

Re: [PATCH v6 1/5] i3c: Add HDR API support

[Andy Shevchenko]

On Tue, Oct 14, 2025 at 12:40:00PM -0400, Frank Li wrote:
> 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.
> 
> Rename enum i3c_hdr_mode to i3c_xfer_mode. Previous definition need match
> CCC GET_CAP1 bit position. Use 31 as SDR transfer mode.
> 
> Add i3c_device_do_xfers() with an xfer 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.

...

>  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;

I would find the logically coupled proto-based xfers:

	if (!ops->i2c_xfers && !ops->i3c_xfers)
		return -EINVAL;


>  	if (ops->request_ibi &&
>  	    (!ops->enable_ibi || !ops->disable_ibi || !ops->free_ibi ||
>  	     !ops->recycle_ibi_slot))

>  }

...

> -enum i3c_hdr_mode {
> +enum i3c_xfer_mode {
> +	/* The below 3 value (I3C_HDR*) must match GETCAP1 Byte bit position */
>  	I3C_HDR_DDR,
>  	I3C_HDR_TSP,
>  	I3C_HDR_TSL,
> +	/* Use for default SDR transfer mode */
> +	I3C_SDR = 0x31,

Why has this a specific value, while the rest have not? If it's HW mandated,
the all of them has to be assigned properly to avoid potential bugs.

>  };

...

>  /**
> - * 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

>   * @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;
> +	};

What field is used to distinguish the union member in current use?
In another word, union must be accompanied with a selector.

>  	u16 len;
>  	u16 actual_len;
>  	union {

>  	enum i3c_error_code err;
>  };

...

> +/* keep back compatible */
> +#define i3c_priv_xfer i3c_xfer

How many of the current users do this? Can't we just rename treewide?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 2/5] i3c: master: svc: Replace bool rnw with union for HDR support

[Andy Shevchenko]

On Tue, Oct 14, 2025 at 12:40:01PM -0400, Frank Li wrote:
> Replace the bool rnw field with a union in preparation for adding HDR
> support. HDR uses a cmd field instead of the rnw bit to indicate read or
> write direction.
> 
> Add helper function svc_cmd_is_read() to check transfer direction.
> 
> Add a local variable 'rnw' in svc_i3c_master_priv_xfers() to avoid
> repeatedly accessing xfers[i].rnw.
> 
> No functional change.

...

>  struct svc_i3c_cmd {
>  	u8 addr;
> -	bool rnw;
> +	union {
> +		bool rnw;
> +		u8 cmd;
> +		u32 rnw_cmd;
> +	};

Same Q, what is the selector?

>  	u8 *in;
>  	const void *out;
>  	unsigned int len;

>  }

> +static bool svc_cmd_is_read(u32 rnw_cmd, u32 type)
> +{
> +	return rnw_cmd;
> +}

Useless?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 3/5] i3c: master: svc: Add basic HDR mode support

[Andy Shevchenko]

On Tue, Oct 14, 2025 at 12:40:02PM -0400, Frank Li wrote:
> 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.

...

> +static void svc_i3c_master_emit_force_exit(struct svc_i3c_master *master)
> +{
> +	u32 reg = 0;

Useless.

> +	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);

No error checks? Why is it okay?
Why is the first parameter 0 while it's not an _atomic() call?

> +	udelay(1);

No explanations given. Also is it really need to be atomic? If not, use
fsleep() and it will choose the best suitable API under the hood.

>  }

...

> +	if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR) {
> +		/* DDR command need prefill into FIFO */
> +		writel(rnw_cmd, master->regs + SVC_I3C_MWDATAB);
> +		if (!rnw) {
> +			/* write data also need prefill into FIFO */
> +			ret = svc_i3c_master_write(master, out, xfer_len);
> +		if (ret)
> +			goto emit_stop;
> +		}

The indentation here is a mess.

> +	}

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Oct 14, 2025 at 12:40:04PM -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.

...

> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/i3c/device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/init.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/unaligned.h>

Follow IWYU principle, many are missing.

...

> +#define MMC5633_STATUS1_MEAS_T_DONE_BIT	BIT(7)
> +#define MMC5633_STATUS1_MEAS_M_DONE_BIT	BIT(6)

+ bits.h

...

> +#define MMC5633_WAIT_SET_RESET_US	1000

Perhaps (1 * MSEC_PER_USEC) ?

...

> +#define MMC5633_HDR_CTRL0_MEAS_M	0x01
> +#define MMC5633_HDR_CTRL0_MEAS_T	0x03
> +#define MMC5633_HDR_CTRL0_SET		0X05

Keep the style of the values consistent.

> +#define MMC5633_HDR_CTRL0_RESET		0x07

...

> +struct mmc5633_data {
> +	struct device *dev;
> +	struct i3c_device *i3cdev;
> +	struct mutex mutex; /* protect to finish one whole measurement */

+ mutex.h

> +	struct regmap *regmap;
> +};

...

> +static int mmc5633_get_samp_freq_index(struct mmc5633_data *data,
> +				       int val, int val2)
> +{
> +	int i;

Why signed?

> +	for (i = 0; i < ARRAY_SIZE(mmc5633_samp_freq); i++)

+ array_size.h

> +		if (mmc5633_samp_freq[i].val == val &&
> +		    mmc5633_samp_freq[i].val2 == val2)
> +			return i;
> +	return -EINVAL;
> +}

...

> +static int mmc5633_init(struct mmc5633_data *data)
> +{
> +	unsigned int reg_id, ret;

Have you ever compiled this?
Why ret is unsigned?

> +	ret = regmap_read(data->regmap, MMC5633_REG_ID, &reg_id);
> +	if (ret < 0)
> +		return dev_err_probe(data->dev, ret,
> +				     "Error reading product id\n");

+ dev_printk.h

> +	/*
> +	 * Make sure we restore sensor characteristics, by doing
> +	 * a SET/RESET sequence, the axis polarity being naturally
> +	 * aligned after RESET.
> +	 */
> +	ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_SET);
> +	if (ret < 0)
> +		return ret;

> +	fsleep(MMC5633_WAIT_SET_RESET_US);

Do you have a reference to a datasheet? Perhaps add a small comment with it.

> +	ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, MMC5633_CTRL0_RESET);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* set default sampling frequency */
> +	return regmap_update_bits(data->regmap, MMC5633_REG_CTRL1,
> +				  MMC5633_CTRL1_BW_MASK,
> +				  FIELD_PREP(MMC5633_CTRL1_BW_MASK, 0));
> +}

...

> +static int mmc5633_take_measurement(struct mmc5633_data *data, int address)
> +{
> +	unsigned int reg_status;
> +	int ret;
> +	int val;
> +
> +	val = (address == MMC5633_TEMPERATURE) ? MMC5633_CTRL0_MEAS_T : MMC5633_CTRL0_MEAS_M;
> +	ret = regmap_write(data->regmap, MMC5633_REG_CTRL0, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = (address == MMC5633_TEMPERATURE) ?
> +	      MMC5633_STATUS1_MEAS_T_DONE_BIT : MMC5633_STATUS1_MEAS_M_DONE_BIT;
> +	ret = regmap_read_poll_timeout(data->regmap, MMC5633_REG_STATUS1, reg_status,
> +				       reg_status & val, 10000, 10000 * 100);

10 * MSEC_PER_USEC, 100 * 10 * MSEC_PER_USEC

> +	if (ret) {
> +		dev_err(data->dev, "data not ready\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}

...

> +static int mmc5633_read_measurement(struct mmc5633_data *data, int address, void *buf, size_t sz)
> +{
> +	u8 data_cmd[2], status[2];
> +	int ret, val, ready;
> +
> +	if (mmc5633_is_support_hdr(data)) {
> +		struct i3c_xfer xfers_wr_cmd[] = {
> +			{
> +				.cmd = 0x3b,
> +				.len = 2,
> +				.data.out = data_cmd,
> +			}
> +		};

> +

Redundant blank line.

> +		struct i3c_xfer xfers_rd_sta_cmd[] = {
> +			{
> +				.cmd = 0x23 | BIT(7), /* RDSTA CMD */
> +				.len = 2,
> +				.data.in = status,
> +			},
> +		};
> +

Ditto.

> +		struct i3c_xfer xfers_rd_data_cmd[] = {
> +			{
> +				.cmd = 0x22 | BIT(7), /* RDLONG CMD */
> +				.len = sz,
> +				.data.in = buf,
> +			},
> +		};
> +
> +		data_cmd[0] = 0;
> +		data_cmd[1] = (address == MMC5633_TEMPERATURE) ?
> +			      MMC5633_HDR_CTRL0_MEAS_T : MMC5633_HDR_CTRL0_MEAS_M;
> +
> +		ret = i3c_device_do_xfers(data->i3cdev, xfers_wr_cmd, 1, I3C_HDR_DDR);
> +		if (ret < 0)
> +			return ret;
> +
> +		ready = (address == MMC5633_TEMPERATURE) ?
> +			MMC5633_STATUS1_MEAS_T_DONE_BIT : MMC5633_STATUS1_MEAS_M_DONE_BIT;
> +		ret = read_poll_timeout(i3c_device_do_xfers, val,
> +					val ||
> +					status[0] & ready,
> +					10000, 10000 * 100, 0,

Use MSEC_PER_USEC as per above.

> +					data->i3cdev, xfers_rd_sta_cmd, 1, I3C_HDR_DDR);

> +

Redundant blank line, and instead...

> +		if (ret || val) {
> +			dev_err(data->dev, "data not ready\n");
> +			return ret ? ret : -EIO;
> +		}

...split this conditional to the linear ones.

> +		return i3c_device_do_xfers(data->i3cdev, xfers_rd_data_cmd, 1, I3C_HDR_DDR);
> +	}
> +
> +	/* Fallback to use SDR/I2C mode */
> +	ret = mmc5633_take_measurement(data, address);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (address == MMC5633_TEMPERATURE)
> +		/*
> +		 * Put tempeature to last byte of buff to align HDR case.
> +		 * I3C will early terminate data read if previous data is not
> +		 * available.
> +		 */
> +		return regmap_bulk_read(data->regmap, MMC5633_REG_TOUT, buf + sz - 1, 1);


> +	return regmap_bulk_read(data->regmap, MMC5633_REG_XOUT_L, buf, sz);
> +}

...

> +#define MMC5633_ALL_SIZE (3 * 3 + 1) /* each channel have 3 byte and TEMP */

have --> has and put comment on top of the definition.

...

> +	char buf[MMC5633_ALL_SIZE];
> +	unsigned int reg;
> +	int ret, i;

Why is 'i' signed?

> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		scoped_guard(mutex, &data->mutex) {

+ cleanup.h

> +			ret = mmc5633_read_measurement(data, chan->address, buf, MMC5633_ALL_SIZE);
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		ret = mmc5633_get_raw(data, chan->address, buf, val);
> +		if (ret < 0)
> +			return ret;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_MAGN) {
> +			*val = 0;
> +			*val2 = 62500;
> +		} else {
> +			*val = 0;
> +			*val2 = 800000000; /* 0.8C */
> +		}
> +		return IIO_VAL_INT_PLUS_NANO;
> +	case IIO_CHAN_INFO_OFFSET:
> +		if (chan->type == IIO_TEMP) {
> +			*val = -75;
> +			return IIO_VAL_INT;
> +		}
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		scoped_guard(mutex, &data->mutex) {
> +			ret = regmap_read(data->regmap, MMC5633_REG_CTRL1, &reg);
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		i = FIELD_GET(MMC5633_CTRL1_BW_MASK, reg);

+ bitfield.h

> +		if (i >= ARRAY_SIZE(mmc5633_samp_freq))
> +			return -EINVAL;
> +
> +		*val = mmc5633_samp_freq[i].val;
> +		*val2 = mmc5633_samp_freq[i].val2;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}

...

> +static 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;
> +	}
> +}

+ types.h

...

> +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;

+ errno.h

> +	dev_set_drvdata(dev, indio_dev);

+ device.h

> +	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 DEFINE_SIMPLE_DEV_PM_OPS(mmc5633_pm_ops, mmc5633_suspend,
> +				mmc5633_resume);

One line (despite of being longer than 80).

...

> +static const struct of_device_id mmc5633_of_match[] = {
> +	{ .compatible = "memsic,mmc5603", },
> +	{ .compatible = "memsic,mmc5633", },

Remove inner commas.

> +	{ }
> +};

...

> +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))

+ err.h

> +		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,
> +};

> +

Redundant blank line.

> +module_i3c_i2c_driver(mmc5633_i3c_driver, &mmc5633_i2c_driver)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/5] i3c: Add HDR API support

[Frank Li]

On Thu, Oct 23, 2025 at 11:23:39AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 14, 2025 at 12:40:00PM -0400, Frank Li wrote:
> > 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.
> >
> > Rename enum i3c_hdr_mode to i3c_xfer_mode. Previous definition need match
> > CCC GET_CAP1 bit position. Use 31 as SDR transfer mode.
> >
> > Add i3c_device_do_xfers() with an xfer 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.
>
> ...
>
> >  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;
>
> I would find the logically coupled proto-based xfers:
>
> 	if (!ops->i2c_xfers && !ops->i3c_xfers)
> 		return -EINVAL;

Not exactly, priv_xfers is old API, which supported now. I plan remove it
after remove all from i3c master controller driver after this patch merged.

i2c_xfers: must be no NULL

priv_xfers and i3c_xfers, one of both should be no NULL.

i2c_xfer is NULL, should be return -EINVAL, but you logic may success if
i3c_xfers is not NULL.

>
>
> >  	if (ops->request_ibi &&
> >  	    (!ops->enable_ibi || !ops->disable_ibi || !ops->free_ibi ||
> >  	     !ops->recycle_ibi_slot))
>
> >  }
>
> ...
>
> > -enum i3c_hdr_mode {
> > +enum i3c_xfer_mode {
> > +	/* The below 3 value (I3C_HDR*) must match GETCAP1 Byte bit position */
> >  	I3C_HDR_DDR,
> >  	I3C_HDR_TSP,
> >  	I3C_HDR_TSL,
> > +	/* Use for default SDR transfer mode */
> > +	I3C_SDR = 0x31,
>
> Why has this a specific value, while the rest have not? If it's HW mandated,
> the all of them has to be assigned properly to avoid potential bugs.
>
> >  };
>
> ...
>
> >  /**
> > - * 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
>
> >   * @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;
> > +	};
>
> What field is used to distinguish the union member in current use?
> In another word, union must be accompanied with a selector.

This struct use only for i3c_device_do_xfers(), which pass i3c_xfer_mode
informaiton. argument 'mode' will distrigiush rnw/cmd.

i3c_xfer[] array don't allow switch transfer mode during whole i3c
transcation. So doesn't put mode in here.

>
> >  	u16 len;
> >  	u16 actual_len;
> >  	union {
>
> >  	enum i3c_error_code err;
> >  };
>
> ...
>
> > +/* keep back compatible */
> > +#define i3c_priv_xfer i3c_xfer
>
> How many of the current users do this? Can't we just rename treewide?

git grep -r priv_xfer drivers/
drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[] = {
drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 1);
drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[2];
drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 2);
drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
drivers/hwmon/lm75.c:   ret = i3c_device_do_priv_xfers(i3cdev, xfers, 2);
drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
drivers/hwmon/lm75.c:   return i3c_device_do_priv_xfers(i3cdev, xfers, 1);
drivers/i3c/device.c:int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers,
drivers/i3c/master.c:   if (!ops->priv_xfers && !ops->i3c_xfers)
drivers/i3c/master.c:   if (!master->ops->priv_xfers)
drivers/i3c/master.c:   return master->ops->priv_xfers(dev, xfers, nxfers);
drivers/i3c/master/dw-i3c-master.c:static int dw_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
drivers/i3c/master/dw-i3c-master.c:                                 struct i3c_priv_xfer *i3c_xfers,
drivers/i3c/master/dw-i3c-master.c:     .priv_xfers = dw_i3c_master_priv_xfers,
drivers/i3c/master/i3c-master-cdns.c:static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
drivers/i3c/master/i3c-master-cdns.c:                                 struct i3c_priv_xfer *xfers,
drivers/i3c/master/i3c-master-cdns.c:   .priv_xfers = cdns_i3c_master_priv_xfers,
drivers/i3c/master/mipi-i3c-hci/core.c:static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
drivers/i3c/master/mipi-i3c-hci/core.c:                       struct i3c_priv_xfer *i3c_xfers,
drivers/i3c/master/mipi-i3c-hci/core.c: .priv_xfers             = i3c_hci_priv_xfers,
drivers/i3c/master/renesas-i3c.c:static int renesas_i3c_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *i3c_xfers,
drivers/i3c/master/renesas-i3c.c:       .priv_xfers = renesas_i3c_priv_xfers,
drivers/i3c/master/svc-i3c-master.c:    struct i3c_priv_xfer *xfer;
drivers/i3c/master/svc-i3c-master.c:     * at svc_i3c_master_priv_xfers().
drivers/i3c/master/svc-i3c-master.c:static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = 1, .len = mi->mrl };
drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = false };
drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);

After this patch merged, I can clean up it at difference subsytem. After
all cleanup done, we can safely remove this define.

Frank
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v6 2/5] i3c: master: svc: Replace bool rnw with union for HDR support

[Frank Li]

On Thu, Oct 23, 2025 at 11:26:37AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 14, 2025 at 12:40:01PM -0400, Frank Li wrote:
> > Replace the bool rnw field with a union in preparation for adding HDR
> > support. HDR uses a cmd field instead of the rnw bit to indicate read or
> > write direction.
> >
> > Add helper function svc_cmd_is_read() to check transfer direction.
> >
> > Add a local variable 'rnw' in svc_i3c_master_priv_xfers() to avoid
> > repeatedly accessing xfers[i].rnw.
> >
> > No functional change.
>
> ...
>
> >  struct svc_i3c_cmd {
> >  	u8 addr;
> > -	bool rnw;
> > +	union {
> > +		bool rnw;
> > +		u8 cmd;
> > +		u32 rnw_cmd;
> > +	};
>
> Same Q, what is the selector?

Choose by transfer mode in callbeck i3c_xfers(..., mode).

>
> >  	u8 *in;
> >  	const void *out;
> >  	unsigned int len;
>
> >  }
>
> > +static bool svc_cmd_is_read(u32 rnw_cmd, u32 type)
> > +{
> > +	return rnw_cmd;
> > +}
>
> Useless?

Just prepare for HDR support.  HDR mode is difference method to check read
or write.

Frank
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v6 3/5] i3c: master: svc: Add basic HDR mode support

[Frank Li]

On Thu, Oct 23, 2025 at 11:29:53AM +0300, Andy Shevchenko wrote:
> On Tue, Oct 14, 2025 at 12:40:02PM -0400, Frank Li wrote:
> > 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.
>
> ...
>
> > +static void svc_i3c_master_emit_force_exit(struct svc_i3c_master *master)
> > +{
> > +	u32 reg = 0;
>
> Useless.
>
> > +	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);
>
> No error checks? Why is it okay?
> Why is the first parameter 0 while it's not an _atomic() call?
>
> > +	udelay(1);
>
> No explanations given. Also is it really need to be atomic? If not, use
> fsleep() and it will choose the best suitable API under the hood.

It is in atomic context. I will add comments.

Frank

>
> >  }
>
> ...
>
> > +	if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR) {
> > +		/* DDR command need prefill into FIFO */
> > +		writel(rnw_cmd, master->regs + SVC_I3C_MWDATAB);
> > +		if (!rnw) {
> > +			/* write data also need prefill into FIFO */
> > +			ret = svc_i3c_master_write(master, out, xfer_len);
> > +		if (ret)
> > +			goto emit_stop;
> > +		}
>
> The indentation here is a mess.
>
> > +	}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v6 1/5] i3c: Add HDR API support

[Andy Shevchenko]

On Thu, Oct 23, 2025 at 11:18:37AM -0400, Frank Li wrote:
> On Thu, Oct 23, 2025 at 11:23:39AM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 14, 2025 at 12:40:00PM -0400, Frank Li wrote:

...

> > >  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;
> >
> > I would find the logically coupled proto-based xfers:
> >
> > 	if (!ops->i2c_xfers && !ops->i3c_xfers)
> > 		return -EINVAL;
> 
> Not exactly, priv_xfers is old API, which supported now. I plan remove it
> after remove all from i3c master controller driver after this patch merged.
> 
> i2c_xfers: must be no NULL
> 
> priv_xfers and i3c_xfers, one of both should be no NULL.
> 
> i2c_xfer is NULL, should be return -EINVAL, but you logic may success if
> i3c_xfers is not NULL.

You are right. I misread && as ||. Can you add a summary of the above in the
comment above the conditionals or kernel-doc of this function (if present)?

> > >  	if (ops->request_ibi &&
> > >  	    (!ops->enable_ibi || !ops->disable_ibi || !ops->free_ibi ||
> > >  	     !ops->recycle_ibi_slot))
> >
> > >  }

...

> > > -enum i3c_hdr_mode {
> > > +enum i3c_xfer_mode {
> > > +	/* The below 3 value (I3C_HDR*) must match GETCAP1 Byte bit position */
> > >  	I3C_HDR_DDR,
> > >  	I3C_HDR_TSP,
> > >  	I3C_HDR_TSL,
> > > +	/* Use for default SDR transfer mode */
> > > +	I3C_SDR = 0x31,
> >
> > Why has this a specific value, while the rest have not? If it's HW mandated,
> > the all of them has to be assigned properly to avoid potential bugs.
> >
> > >  };

Are you agree on this or disagree or...?
If you agree, don't leave the unneeded context in the reply.
Otherwise, please express your objections.

...

> > >  /**
> > > - * 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
> >
> > >   * @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;
> > > +	};
> >
> > What field is used to distinguish the union member in current use?
> > In another word, union must be accompanied with a selector.
> 
> This struct use only for i3c_device_do_xfers(), which pass i3c_xfer_mode
> informaiton. argument 'mode' will distrigiush rnw/cmd.

Then why that mode field is not present here?

> i3c_xfer[] array don't allow switch transfer mode during whole i3c
> transcation. So doesn't put mode in here.

I presume that this is the answer to my above Q? If so, I think this
dislayering is not okay, because the struct effectively lost the crucial piece
of information (for example, if you need to trace the contents of it, the mode
also needs to be provided again, and so on). I think the data type must have
this mode field as well (as long as union is in use).

> > >  	u16 len;
> > >  	u16 actual_len;
> > >  	union {
> >
> > >  	enum i3c_error_code err;
> > >  };

...

> > > +/* keep back compatible */
> > > +#define i3c_priv_xfer i3c_xfer
> >
> > How many of the current users do this? Can't we just rename treewide?
> 
> git grep -r priv_xfer drivers/

`git grep -lw ...` is a better approach :-)

> drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[] = {
> drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 1);
> drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[2];
> drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 2);
> drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> drivers/hwmon/lm75.c:   ret = i3c_device_do_priv_xfers(i3cdev, xfers, 2);
> drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> drivers/hwmon/lm75.c:   return i3c_device_do_priv_xfers(i3cdev, xfers, 1);
> drivers/i3c/device.c:int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers,
> drivers/i3c/master.c:   if (!ops->priv_xfers && !ops->i3c_xfers)
> drivers/i3c/master.c:   if (!master->ops->priv_xfers)
> drivers/i3c/master.c:   return master->ops->priv_xfers(dev, xfers, nxfers);
> drivers/i3c/master/dw-i3c-master.c:static int dw_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> drivers/i3c/master/dw-i3c-master.c:                                 struct i3c_priv_xfer *i3c_xfers,
> drivers/i3c/master/dw-i3c-master.c:     .priv_xfers = dw_i3c_master_priv_xfers,
> drivers/i3c/master/i3c-master-cdns.c:static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> drivers/i3c/master/i3c-master-cdns.c:                                 struct i3c_priv_xfer *xfers,
> drivers/i3c/master/i3c-master-cdns.c:   .priv_xfers = cdns_i3c_master_priv_xfers,
> drivers/i3c/master/mipi-i3c-hci/core.c:static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
> drivers/i3c/master/mipi-i3c-hci/core.c:                       struct i3c_priv_xfer *i3c_xfers,
> drivers/i3c/master/mipi-i3c-hci/core.c: .priv_xfers             = i3c_hci_priv_xfers,
> drivers/i3c/master/renesas-i3c.c:static int renesas_i3c_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *i3c_xfers,
> drivers/i3c/master/renesas-i3c.c:       .priv_xfers = renesas_i3c_priv_xfers,
> drivers/i3c/master/svc-i3c-master.c:    struct i3c_priv_xfer *xfer;
> drivers/i3c/master/svc-i3c-master.c:     * at svc_i3c_master_priv_xfers().
> drivers/i3c/master/svc-i3c-master.c:static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
> drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = 1, .len = mi->mrl };
> drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = false };
> drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> 
> After this patch merged, I can clean up it at difference subsytem. After
> all cleanup done, we can safely remove this define.

I counted 9. I think it's not a big deal to convert all of them at once without
leaving an intermediate state. But this is a call for the I³C subsystem maintainer.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 3/5] i3c: master: svc: Add basic HDR mode support

[Andy Shevchenko]

On Thu, Oct 23, 2025 at 11:24:02AM -0400, Frank Li wrote:
> On Thu, Oct 23, 2025 at 11:29:53AM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 14, 2025 at 12:40:02PM -0400, Frank Li wrote:

...

> > > +	readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> > > +			   SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
> >
> > No error checks? Why is it okay?
> > Why is the first parameter 0 while it's not an _atomic() call?
> >
> > > +	udelay(1);
> >
> > No explanations given. Also is it really need to be atomic? If not, use
> > fsleep() and it will choose the best suitable API under the hood.
> 
> It is in atomic context. I will add comments.

Not only, the call that's used in the code from iopoll.h is wrong in such a
case. Haven't you tested this with debug atomic context?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 3/5] i3c: master: svc: Add basic HDR mode support

[Frank Li]

On Thu, Oct 23, 2025 at 09:25:00PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 23, 2025 at 11:24:02AM -0400, Frank Li wrote:
> > On Thu, Oct 23, 2025 at 11:29:53AM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 14, 2025 at 12:40:02PM -0400, Frank Li wrote:
>
> ...
>
> > > > +	readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> > > > +			   SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000);
> > >
> > > No error checks? Why is it okay?
> > > Why is the first parameter 0 while it's not an _atomic() call?
> > >
> > > > +	udelay(1);
> > >
> > > No explanations given. Also is it really need to be atomic? If not, use
> > > fsleep() and it will choose the best suitable API under the hood.
> >
> > It is in atomic context. I will add comments.
>
> Not only, the call that's used in the code from iopoll.h is wrong in such a
> case. Haven't you tested this with debug atomic context?

not enable debug atomic, but I think it should be
readl_poll_timeout_atomic ().

Frank
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v6 1/5] i3c: Add HDR API support

[Frank Li]

On Thu, Oct 23, 2025 at 09:22:55PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 23, 2025 at 11:18:37AM -0400, Frank Li wrote:
> > On Thu, Oct 23, 2025 at 11:23:39AM +0300, Andy Shevchenko wrote:
> > > On Tue, Oct 14, 2025 at 12:40:00PM -0400, Frank Li wrote:
>
> ...
>
> > > >  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;
> > >
> > > I would find the logically coupled proto-based xfers:
> > >
> > > 	if (!ops->i2c_xfers && !ops->i3c_xfers)
> > > 		return -EINVAL;
> >
> > Not exactly, priv_xfers is old API, which supported now. I plan remove it
> > after remove all from i3c master controller driver after this patch merged.
> >
> > i2c_xfers: must be no NULL
> >
> > priv_xfers and i3c_xfers, one of both should be no NULL.
> >
> > i2c_xfer is NULL, should be return -EINVAL, but you logic may success if
> > i3c_xfers is not NULL.
>
> You are right. I misread && as ||. Can you add a summary of the above in the
> comment above the conditionals or kernel-doc of this function (if present)?
>
> > > >  	if (ops->request_ibi &&
> > > >  	    (!ops->enable_ibi || !ops->disable_ibi || !ops->free_ibi ||
> > > >  	     !ops->recycle_ibi_slot))
> > >
> > > >  }
>
> ...
>
> > > > -enum i3c_hdr_mode {
> > > > +enum i3c_xfer_mode {
> > > > +	/* The below 3 value (I3C_HDR*) must match GETCAP1 Byte bit position */
> > > >  	I3C_HDR_DDR,
> > > >  	I3C_HDR_TSP,
> > > >  	I3C_HDR_TSL,
> > > > +	/* Use for default SDR transfer mode */
> > > > +	I3C_SDR = 0x31,
> > >
> > > Why has this a specific value, while the rest have not? If it's HW mandated,
> > > the all of them has to be assigned properly to avoid potential bugs.
> > >
> > > >  };
>
> Are you agree on this or disagree or...?
> If you agree, don't leave the unneeded context in the reply.
> Otherwise, please express your objections.

Sorry, I agree, It should assigned value to it. Actually, I have not
realized it tights to Hardware bits at beginning.

>
> ...
>
> > > >  /**
> > > > - * 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
> > >
> > > >   * @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;
> > > > +	};
> > >
> > > What field is used to distinguish the union member in current use?
> > > In another word, union must be accompanied with a selector.
> >
> > This struct use only for i3c_device_do_xfers(), which pass i3c_xfer_mode
> > informaiton. argument 'mode' will distrigiush rnw/cmd.
>
> Then why that mode field is not present here?

Yes!

>
> > i3c_xfer[] array don't allow switch transfer mode during whole i3c
> > transcation. So doesn't put mode in here.
>
> I presume that this is the answer to my above Q? If so, I think this
> dislayering is not okay, because the struct effectively lost the crucial piece
> of information (for example, if you need to trace the contents of it, the mode
> also needs to be provided again, and so on). I think the data type must have
> this mode field as well (as long as union is in use).
>
> > > >  	u16 len;
> > > >  	u16 actual_len;
> > > >  	union {
> > >
> > > >  	enum i3c_error_code err;
> > > >  };
>
> ...
>
> > > > +/* keep back compatible */
> > > > +#define i3c_priv_xfer i3c_xfer
> > >
> > > How many of the current users do this? Can't we just rename treewide?
> >
> > git grep -r priv_xfer drivers/
>
> `git grep -lw ...` is a better approach :-)
>
> > drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[] = {
> > drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 1);
> > drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[2];
> > drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 2);
> > drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> > drivers/hwmon/lm75.c:   ret = i3c_device_do_priv_xfers(i3cdev, xfers, 2);
> > drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> > drivers/hwmon/lm75.c:   return i3c_device_do_priv_xfers(i3cdev, xfers, 1);
> > drivers/i3c/device.c:int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers,
> > drivers/i3c/master.c:   if (!ops->priv_xfers && !ops->i3c_xfers)
> > drivers/i3c/master.c:   if (!master->ops->priv_xfers)
> > drivers/i3c/master.c:   return master->ops->priv_xfers(dev, xfers, nxfers);
> > drivers/i3c/master/dw-i3c-master.c:static int dw_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > drivers/i3c/master/dw-i3c-master.c:                                 struct i3c_priv_xfer *i3c_xfers,
> > drivers/i3c/master/dw-i3c-master.c:     .priv_xfers = dw_i3c_master_priv_xfers,
> > drivers/i3c/master/i3c-master-cdns.c:static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > drivers/i3c/master/i3c-master-cdns.c:                                 struct i3c_priv_xfer *xfers,
> > drivers/i3c/master/i3c-master-cdns.c:   .priv_xfers = cdns_i3c_master_priv_xfers,
> > drivers/i3c/master/mipi-i3c-hci/core.c:static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
> > drivers/i3c/master/mipi-i3c-hci/core.c:                       struct i3c_priv_xfer *i3c_xfers,
> > drivers/i3c/master/mipi-i3c-hci/core.c: .priv_xfers             = i3c_hci_priv_xfers,
> > drivers/i3c/master/renesas-i3c.c:static int renesas_i3c_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *i3c_xfers,
> > drivers/i3c/master/renesas-i3c.c:       .priv_xfers = renesas_i3c_priv_xfers,
> > drivers/i3c/master/svc-i3c-master.c:    struct i3c_priv_xfer *xfer;
> > drivers/i3c/master/svc-i3c-master.c:     * at svc_i3c_master_priv_xfers().
> > drivers/i3c/master/svc-i3c-master.c:static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
> > drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = 1, .len = mi->mrl };
> > drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> > drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = false };
> > drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> >
> > After this patch merged, I can clean up it at difference subsytem. After
> > all cleanup done, we can safely remove this define.
>
> I counted 9. I think it's not a big deal to convert all of them at once without
> leaving an intermediate state. But this is a call for the I³C subsystem maintaiiner.

There also are other cleanup works. The key point is that everyone agree my
HDR solution. Cleanup these is not big deal. I am not sure how to avoid
build broken at difference subsystem.

After this patch merge, cleanup will be easier and safer.

Frank

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v6 1/5] i3c: Add HDR API support

[Andy Shevchenko]

On Thu, Oct 23, 2025 at 07:53:15PM -0400, Frank Li wrote:
> On Thu, Oct 23, 2025 at 09:22:55PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 23, 2025 at 11:18:37AM -0400, Frank Li wrote:
> > > On Thu, Oct 23, 2025 at 11:23:39AM +0300, Andy Shevchenko wrote:
> > > > On Tue, Oct 14, 2025 at 12:40:00PM -0400, Frank Li wrote:

...

> > > > > +/* keep back compatible */
> > > > > +#define i3c_priv_xfer i3c_xfer
> > > >
> > > > How many of the current users do this? Can't we just rename treewide?
> > >
> > > git grep -r priv_xfer drivers/
> >
> > `git grep -lw ...` is a better approach :-)
> >
> > > drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[] = {
> > > drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 1);
> > > drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[2];
> > > drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 2);
> > > drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> > > drivers/hwmon/lm75.c:   ret = i3c_device_do_priv_xfers(i3cdev, xfers, 2);
> > > drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> > > drivers/hwmon/lm75.c:   return i3c_device_do_priv_xfers(i3cdev, xfers, 1);
> > > drivers/i3c/device.c:int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers,
> > > drivers/i3c/master.c:   if (!ops->priv_xfers && !ops->i3c_xfers)
> > > drivers/i3c/master.c:   if (!master->ops->priv_xfers)
> > > drivers/i3c/master.c:   return master->ops->priv_xfers(dev, xfers, nxfers);
> > > drivers/i3c/master/dw-i3c-master.c:static int dw_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > > drivers/i3c/master/dw-i3c-master.c:                                 struct i3c_priv_xfer *i3c_xfers,
> > > drivers/i3c/master/dw-i3c-master.c:     .priv_xfers = dw_i3c_master_priv_xfers,
> > > drivers/i3c/master/i3c-master-cdns.c:static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > > drivers/i3c/master/i3c-master-cdns.c:                                 struct i3c_priv_xfer *xfers,
> > > drivers/i3c/master/i3c-master-cdns.c:   .priv_xfers = cdns_i3c_master_priv_xfers,
> > > drivers/i3c/master/mipi-i3c-hci/core.c:static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
> > > drivers/i3c/master/mipi-i3c-hci/core.c:                       struct i3c_priv_xfer *i3c_xfers,
> > > drivers/i3c/master/mipi-i3c-hci/core.c: .priv_xfers             = i3c_hci_priv_xfers,
> > > drivers/i3c/master/renesas-i3c.c:static int renesas_i3c_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *i3c_xfers,
> > > drivers/i3c/master/renesas-i3c.c:       .priv_xfers = renesas_i3c_priv_xfers,
> > > drivers/i3c/master/svc-i3c-master.c:    struct i3c_priv_xfer *xfer;
> > > drivers/i3c/master/svc-i3c-master.c:     * at svc_i3c_master_priv_xfers().
> > > drivers/i3c/master/svc-i3c-master.c:static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
> > > drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = 1, .len = mi->mrl };
> > > drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> > > drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = false };
> > > drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> > >
> > > After this patch merged, I can clean up it at difference subsytem. After
> > > all cleanup done, we can safely remove this define.
> >
> > I counted 9. I think it's not a big deal to convert all of them at once without
> > leaving an intermediate state. But this is a call for the I³C subsystem maintaiiner.
> 
> There also are other cleanup works. The key point is that everyone agree my
> HDR solution. Cleanup these is not big deal. I am not sure how to avoid
> build broken at difference subsystem.
> 
> After this patch merge, cleanup will be easier and safer.

Then leave that renaming to the cleanup series. No need to use a define, just
use the old function name.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/5] i3c: Add HDR API support

[Frank Li]

On Fri, Oct 24, 2025 at 09:13:27AM +0300, Andy Shevchenko wrote:
> On Thu, Oct 23, 2025 at 07:53:15PM -0400, Frank Li wrote:
> > On Thu, Oct 23, 2025 at 09:22:55PM +0300, Andy Shevchenko wrote:
> > > On Thu, Oct 23, 2025 at 11:18:37AM -0400, Frank Li wrote:
> > > > On Thu, Oct 23, 2025 at 11:23:39AM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Oct 14, 2025 at 12:40:00PM -0400, Frank Li wrote:
>
> ...
>
> > > > > > +/* keep back compatible */
> > > > > > +#define i3c_priv_xfer i3c_xfer
> > > > >
> > > > > How many of the current users do this? Can't we just rename treewide?
> > > >
> > > > git grep -r priv_xfer drivers/
> > >
> > > `git grep -lw ...` is a better approach :-)
> > >
> > > > drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[] = {
> > > > drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 1);
> > > > drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[2];
> > > > drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 2);
> > > > drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> > > > drivers/hwmon/lm75.c:   ret = i3c_device_do_priv_xfers(i3cdev, xfers, 2);
> > > > drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> > > > drivers/hwmon/lm75.c:   return i3c_device_do_priv_xfers(i3cdev, xfers, 1);
> > > > drivers/i3c/device.c:int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers,
> > > > drivers/i3c/master.c:   if (!ops->priv_xfers && !ops->i3c_xfers)
> > > > drivers/i3c/master.c:   if (!master->ops->priv_xfers)
> > > > drivers/i3c/master.c:   return master->ops->priv_xfers(dev, xfers, nxfers);
> > > > drivers/i3c/master/dw-i3c-master.c:static int dw_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > > > drivers/i3c/master/dw-i3c-master.c:                                 struct i3c_priv_xfer *i3c_xfers,
> > > > drivers/i3c/master/dw-i3c-master.c:     .priv_xfers = dw_i3c_master_priv_xfers,
> > > > drivers/i3c/master/i3c-master-cdns.c:static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > > > drivers/i3c/master/i3c-master-cdns.c:                                 struct i3c_priv_xfer *xfers,
> > > > drivers/i3c/master/i3c-master-cdns.c:   .priv_xfers = cdns_i3c_master_priv_xfers,
> > > > drivers/i3c/master/mipi-i3c-hci/core.c:static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
> > > > drivers/i3c/master/mipi-i3c-hci/core.c:                       struct i3c_priv_xfer *i3c_xfers,
> > > > drivers/i3c/master/mipi-i3c-hci/core.c: .priv_xfers             = i3c_hci_priv_xfers,
> > > > drivers/i3c/master/renesas-i3c.c:static int renesas_i3c_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *i3c_xfers,
> > > > drivers/i3c/master/renesas-i3c.c:       .priv_xfers = renesas_i3c_priv_xfers,
> > > > drivers/i3c/master/svc-i3c-master.c:    struct i3c_priv_xfer *xfer;
> > > > drivers/i3c/master/svc-i3c-master.c:     * at svc_i3c_master_priv_xfers().
> > > > drivers/i3c/master/svc-i3c-master.c:static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
> > > > drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = 1, .len = mi->mrl };
> > > > drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> > > > drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = false };
> > > > drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> > > >
> > > > After this patch merged, I can clean up it at difference subsytem. After
> > > > all cleanup done, we can safely remove this define.
> > >
> > > I counted 9. I think it's not a big deal to convert all of them at once without
> > > leaving an intermediate state. But this is a call for the I³C subsystem maintaiiner.
> >
> > There also are other cleanup works. The key point is that everyone agree my
> > HDR solution. Cleanup these is not big deal. I am not sure how to avoid
> > build broken at difference subsystem.
> >
> > After this patch merge, cleanup will be easier and safer.
>
> Then leave that renaming to the cleanup series. No need to use a define, just
> use the old function name.

Using old function name for HDR will be very strange and conflict with
spec's name convention.

The term 'private' transfer in i3c spec is specific for SDR transfer. It
is neccessary steps to complete whole naming switches.

Frank

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v6 1/5] i3c: Add HDR API support

[Andy Shevchenko]

On Fri, Oct 24, 2025 at 10:03:22AM -0400, Frank Li wrote:
> On Fri, Oct 24, 2025 at 09:13:27AM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 23, 2025 at 07:53:15PM -0400, Frank Li wrote:
> > > On Thu, Oct 23, 2025 at 09:22:55PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Oct 23, 2025 at 11:18:37AM -0400, Frank Li wrote:
> > > > > On Thu, Oct 23, 2025 at 11:23:39AM +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Oct 14, 2025 at 12:40:00PM -0400, Frank Li wrote:

...

> > > > > > > +/* keep back compatible */
> > > > > > > +#define i3c_priv_xfer i3c_xfer
> > > > > >
> > > > > > How many of the current users do this? Can't we just rename treewide?
> > > > >
> > > > > git grep -r priv_xfer drivers/
> > > >
> > > > `git grep -lw ...` is a better approach :-)
> > > >
> > > > > drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[] = {
> > > > > drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 1);
> > > > > drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[2];
> > > > > drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 2);
> > > > > drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> > > > > drivers/hwmon/lm75.c:   ret = i3c_device_do_priv_xfers(i3cdev, xfers, 2);
> > > > > drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> > > > > drivers/hwmon/lm75.c:   return i3c_device_do_priv_xfers(i3cdev, xfers, 1);
> > > > > drivers/i3c/device.c:int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers,
> > > > > drivers/i3c/master.c:   if (!ops->priv_xfers && !ops->i3c_xfers)
> > > > > drivers/i3c/master.c:   if (!master->ops->priv_xfers)
> > > > > drivers/i3c/master.c:   return master->ops->priv_xfers(dev, xfers, nxfers);
> > > > > drivers/i3c/master/dw-i3c-master.c:static int dw_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > > > > drivers/i3c/master/dw-i3c-master.c:                                 struct i3c_priv_xfer *i3c_xfers,
> > > > > drivers/i3c/master/dw-i3c-master.c:     .priv_xfers = dw_i3c_master_priv_xfers,
> > > > > drivers/i3c/master/i3c-master-cdns.c:static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > > > > drivers/i3c/master/i3c-master-cdns.c:                                 struct i3c_priv_xfer *xfers,
> > > > > drivers/i3c/master/i3c-master-cdns.c:   .priv_xfers = cdns_i3c_master_priv_xfers,
> > > > > drivers/i3c/master/mipi-i3c-hci/core.c:static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
> > > > > drivers/i3c/master/mipi-i3c-hci/core.c:                       struct i3c_priv_xfer *i3c_xfers,
> > > > > drivers/i3c/master/mipi-i3c-hci/core.c: .priv_xfers             = i3c_hci_priv_xfers,
> > > > > drivers/i3c/master/renesas-i3c.c:static int renesas_i3c_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *i3c_xfers,
> > > > > drivers/i3c/master/renesas-i3c.c:       .priv_xfers = renesas_i3c_priv_xfers,
> > > > > drivers/i3c/master/svc-i3c-master.c:    struct i3c_priv_xfer *xfer;
> > > > > drivers/i3c/master/svc-i3c-master.c:     * at svc_i3c_master_priv_xfers().
> > > > > drivers/i3c/master/svc-i3c-master.c:static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
> > > > > drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = 1, .len = mi->mrl };
> > > > > drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> > > > > drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = false };
> > > > > drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> > > > >
> > > > > After this patch merged, I can clean up it at difference subsytem. After
> > > > > all cleanup done, we can safely remove this define.
> > > >
> > > > I counted 9. I think it's not a big deal to convert all of them at once without
> > > > leaving an intermediate state. But this is a call for the I³C subsystem maintaiiner.
> > >
> > > There also are other cleanup works. The key point is that everyone agree my
> > > HDR solution. Cleanup these is not big deal. I am not sure how to avoid
> > > build broken at difference subsystem.
> > >
> > > After this patch merge, cleanup will be easier and safer.
> >
> > Then leave that renaming to the cleanup series. No need to use a define, just
> > use the old function name.
> 
> Using old function name for HDR will be very strange and conflict with
> spec's name convention.
> 
> The term 'private' transfer in i3c spec is specific for SDR transfer. It
> is neccessary steps to complete whole naming switches.

Right, but this out of scope OR a prerequisite to this series. My point that
these two shouldn't be mixed and one left half-baked.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/5] i3c: Add HDR API support

[Frank Li]

On Fri, Oct 24, 2025 at 07:03:41PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 24, 2025 at 10:03:22AM -0400, Frank Li wrote:
> > On Fri, Oct 24, 2025 at 09:13:27AM +0300, Andy Shevchenko wrote:
> > > On Thu, Oct 23, 2025 at 07:53:15PM -0400, Frank Li wrote:
> > > > On Thu, Oct 23, 2025 at 09:22:55PM +0300, Andy Shevchenko wrote:
> > > > > On Thu, Oct 23, 2025 at 11:18:37AM -0400, Frank Li wrote:
> > > > > > On Thu, Oct 23, 2025 at 11:23:39AM +0300, Andy Shevchenko wrote:
> > > > > > > On Tue, Oct 14, 2025 at 12:40:00PM -0400, Frank Li wrote:
>
> ...
>
> > > > > > > > +/* keep back compatible */
> > > > > > > > +#define i3c_priv_xfer i3c_xfer
> > > > > > >
> > > > > > > How many of the current users do this? Can't we just rename treewide?
> > > > > >
> > > > > > git grep -r priv_xfer drivers/
> > > > >
> > > > > `git grep -lw ...` is a better approach :-)
> > > > >
> > > > > > drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[] = {
> > > > > > drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 1);
> > > > > > drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[2];
> > > > > > drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 2);
> > > > > > drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> > > > > > drivers/hwmon/lm75.c:   ret = i3c_device_do_priv_xfers(i3cdev, xfers, 2);
> > > > > > drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> > > > > > drivers/hwmon/lm75.c:   return i3c_device_do_priv_xfers(i3cdev, xfers, 1);
> > > > > > drivers/i3c/device.c:int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers,
> > > > > > drivers/i3c/master.c:   if (!ops->priv_xfers && !ops->i3c_xfers)
> > > > > > drivers/i3c/master.c:   if (!master->ops->priv_xfers)
> > > > > > drivers/i3c/master.c:   return master->ops->priv_xfers(dev, xfers, nxfers);
> > > > > > drivers/i3c/master/dw-i3c-master.c:static int dw_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > > > > > drivers/i3c/master/dw-i3c-master.c:                                 struct i3c_priv_xfer *i3c_xfers,
> > > > > > drivers/i3c/master/dw-i3c-master.c:     .priv_xfers = dw_i3c_master_priv_xfers,
> > > > > > drivers/i3c/master/i3c-master-cdns.c:static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > > > > > drivers/i3c/master/i3c-master-cdns.c:                                 struct i3c_priv_xfer *xfers,
> > > > > > drivers/i3c/master/i3c-master-cdns.c:   .priv_xfers = cdns_i3c_master_priv_xfers,
> > > > > > drivers/i3c/master/mipi-i3c-hci/core.c:static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
> > > > > > drivers/i3c/master/mipi-i3c-hci/core.c:                       struct i3c_priv_xfer *i3c_xfers,
> > > > > > drivers/i3c/master/mipi-i3c-hci/core.c: .priv_xfers             = i3c_hci_priv_xfers,
> > > > > > drivers/i3c/master/renesas-i3c.c:static int renesas_i3c_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *i3c_xfers,
> > > > > > drivers/i3c/master/renesas-i3c.c:       .priv_xfers = renesas_i3c_priv_xfers,
> > > > > > drivers/i3c/master/svc-i3c-master.c:    struct i3c_priv_xfer *xfer;
> > > > > > drivers/i3c/master/svc-i3c-master.c:     * at svc_i3c_master_priv_xfers().
> > > > > > drivers/i3c/master/svc-i3c-master.c:static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
> > > > > > drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = 1, .len = mi->mrl };
> > > > > > drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> > > > > > drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = false };
> > > > > > drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> > > > > >
> > > > > > After this patch merged, I can clean up it at difference subsytem. After
> > > > > > all cleanup done, we can safely remove this define.
> > > > >
> > > > > I counted 9. I think it's not a big deal to convert all of them at once without
> > > > > leaving an intermediate state. But this is a call for the I³C subsystem maintaiiner.
> > > >
> > > > There also are other cleanup works. The key point is that everyone agree my
> > > > HDR solution. Cleanup these is not big deal. I am not sure how to avoid
> > > > build broken at difference subsystem.
> > > >
> > > > After this patch merge, cleanup will be easier and safer.
> > >
> > > Then leave that renaming to the cleanup series. No need to use a define, just
> > > use the old function name.
> >
> > Using old function name for HDR will be very strange and conflict with
> > spec's name convention.
> >
> > The term 'private' transfer in i3c spec is specific for SDR transfer. It
> > is neccessary steps to complete whole naming switches.
>
> Right, but this out of scope OR a prerequisite to this series. My point that
> these two shouldn't be mixed and one left half-baked.

It doesn't make sense that the new iio driver still use old interface, then
replace new one later.

Is it okay I create new patch serial, which switch to new interface, but
it's depend on this one. Let each maintainers decide how/when merge it?

Frank

>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH v6 1/5] i3c: Add HDR API support

[Alexandre Belloni]

On 24/10/2025 12:29:28-0400, Frank Li wrote:
> On Fri, Oct 24, 2025 at 07:03:41PM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 24, 2025 at 10:03:22AM -0400, Frank Li wrote:
> > > On Fri, Oct 24, 2025 at 09:13:27AM +0300, Andy Shevchenko wrote:
> > > > On Thu, Oct 23, 2025 at 07:53:15PM -0400, Frank Li wrote:
> > > > > On Thu, Oct 23, 2025 at 09:22:55PM +0300, Andy Shevchenko wrote:
> > > > > > On Thu, Oct 23, 2025 at 11:18:37AM -0400, Frank Li wrote:
> > > > > > > On Thu, Oct 23, 2025 at 11:23:39AM +0300, Andy Shevchenko wrote:
> > > > > > > > On Tue, Oct 14, 2025 at 12:40:00PM -0400, Frank Li wrote:
> >
> > ...
> >
> > > > > > > > > +/* keep back compatible */
> > > > > > > > > +#define i3c_priv_xfer i3c_xfer
> > > > > > > >
> > > > > > > > How many of the current users do this? Can't we just rename treewide?
> > > > > > >
> > > > > > > git grep -r priv_xfer drivers/
> > > > > >
> > > > > > `git grep -lw ...` is a better approach :-)
> > > > > >
> > > > > > > drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[] = {
> > > > > > > drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 1);
> > > > > > > drivers/base/regmap/regmap-i3c.c:       struct i3c_priv_xfer xfers[2];
> > > > > > > drivers/base/regmap/regmap-i3c.c:       return i3c_device_do_priv_xfers(i3c, xfers, 2);
> > > > > > > drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> > > > > > > drivers/hwmon/lm75.c:   ret = i3c_device_do_priv_xfers(i3cdev, xfers, 2);
> > > > > > > drivers/hwmon/lm75.c:   struct i3c_priv_xfer xfers[] = {
> > > > > > > drivers/hwmon/lm75.c:   return i3c_device_do_priv_xfers(i3cdev, xfers, 1);
> > > > > > > drivers/i3c/device.c:int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers,
> > > > > > > drivers/i3c/master.c:   if (!ops->priv_xfers && !ops->i3c_xfers)
> > > > > > > drivers/i3c/master.c:   if (!master->ops->priv_xfers)
> > > > > > > drivers/i3c/master.c:   return master->ops->priv_xfers(dev, xfers, nxfers);
> > > > > > > drivers/i3c/master/dw-i3c-master.c:static int dw_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > > > > > > drivers/i3c/master/dw-i3c-master.c:                                 struct i3c_priv_xfer *i3c_xfers,
> > > > > > > drivers/i3c/master/dw-i3c-master.c:     .priv_xfers = dw_i3c_master_priv_xfers,
> > > > > > > drivers/i3c/master/i3c-master-cdns.c:static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
> > > > > > > drivers/i3c/master/i3c-master-cdns.c:                                 struct i3c_priv_xfer *xfers,
> > > > > > > drivers/i3c/master/i3c-master-cdns.c:   .priv_xfers = cdns_i3c_master_priv_xfers,
> > > > > > > drivers/i3c/master/mipi-i3c-hci/core.c:static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
> > > > > > > drivers/i3c/master/mipi-i3c-hci/core.c:                       struct i3c_priv_xfer *i3c_xfers,
> > > > > > > drivers/i3c/master/mipi-i3c-hci/core.c: .priv_xfers             = i3c_hci_priv_xfers,
> > > > > > > drivers/i3c/master/renesas-i3c.c:static int renesas_i3c_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *i3c_xfers,
> > > > > > > drivers/i3c/master/renesas-i3c.c:       .priv_xfers = renesas_i3c_priv_xfers,
> > > > > > > drivers/i3c/master/svc-i3c-master.c:    struct i3c_priv_xfer *xfer;
> > > > > > > drivers/i3c/master/svc-i3c-master.c:     * at svc_i3c_master_priv_xfers().
> > > > > > > drivers/i3c/master/svc-i3c-master.c:static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
> > > > > > > drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = 1, .len = mi->mrl };
> > > > > > > drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> > > > > > > drivers/net/mctp/mctp-i3c.c:    struct i3c_priv_xfer xfer = { .rnw = false };
> > > > > > > drivers/net/mctp/mctp-i3c.c:    rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
> > > > > > >
> > > > > > > After this patch merged, I can clean up it at difference subsytem. After
> > > > > > > all cleanup done, we can safely remove this define.
> > > > > >
> > > > > > I counted 9. I think it's not a big deal to convert all of them at once without
> > > > > > leaving an intermediate state. But this is a call for the I³C subsystem maintaiiner.
> > > > >
> > > > > There also are other cleanup works. The key point is that everyone agree my
> > > > > HDR solution. Cleanup these is not big deal. I am not sure how to avoid
> > > > > build broken at difference subsystem.
> > > > >
> > > > > After this patch merge, cleanup will be easier and safer.
> > > >
> > > > Then leave that renaming to the cleanup series. No need to use a define, just
> > > > use the old function name.
> > >
> > > Using old function name for HDR will be very strange and conflict with
> > > spec's name convention.
> > >
> > > The term 'private' transfer in i3c spec is specific for SDR transfer. It
> > > is neccessary steps to complete whole naming switches.
> >
> > Right, but this out of scope OR a prerequisite to this series. My point that
> > these two shouldn't be mixed and one left half-baked.
> 
> It doesn't make sense that the new iio driver still use old interface, then
> replace new one later.
> 
> Is it okay I create new patch serial, which switch to new interface, but
> it's depend on this one. Let each maintainers decide how/when merge it?
> 

I'm fine with the define as I believe you are going to send the series
switching to the new name quickly enough.
I'll provide an immutable branch so the various subsystem maintainers
can apply that on their own.

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com