Message-ID:
 <SH0PR01MB084193AC3C4B8D961F228818F9E62@SH0PR01MB0841.CHNPR01.prod.partner.outlook.cn>
Date: Wed, 1 Oct 2025 08:18:57 +0000
From: Joshua Yeong <joshua.yeong@...rfivetech.com>
To: Frank Li <Frank.Li@....com>, Alexandre Belloni
	<alexandre.belloni@...tlin.com>, Miquel Raynal <miquel.raynal@...tlin.com>,
	Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>,
	Nuno Sá <nuno.sa@...log.com>, Andy Shevchenko
	<andy@...nel.org>
CC: "linux-i3c@...ts.infradead.org" <linux-i3c@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>, "linux-iio@...r.kernel.org"
	<linux-iio@...r.kernel.org>
Subject: RE: [PATCH v3 1/5] i3c: Add HDR API support

On Wednesday, October 1, 2025 3:34 AM, Frank Li <Frank.Li@....com> 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.
>
> Add i3c_device_do_xfers() with an HDR mode argument, while keeping
> i3c_device_do_priv_xfers() as a wrapper that calls i3c_device_do_xfers() with I3C_SDR for backward compatibility.
>
> Introduce a 'cmd' field in struct i3c_xfer as an anonymous union with 'rnw', since HDR mode uses read/write commands instead of the SDR address bit.
>
> Add .i3c_xfers() callback for master controllers. If not implemented, fall back to SDR with .priv_xfers(). The .priv_xfers() API can be removed once all controllers switch to .i3c_xfers().
>
> Add 'mode_mask' bitmask to advertise controller capability.
>
> Signed-off-by: Frank Li <Frank.Li@....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 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       | 22 ++++++++++++++++++----
>  include/linux/i3c/device.h | 28 ++++++++++++++++++++++------  include/linux/i3c/master.h |  6 ++++++
>  5 files changed, 69 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c index 2396545763ff853097d9f0173787e087f7a6e688..00706b47758bc164178a5578a018b36c5c433f5f 100644
> --- a/drivers/i3c/device.c
> +++ b/drivers/i3c/device.c
> @@ -15,12 +15,12 @@
>  #include "internals.h"
>
>  /**
> - * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a
> - *				specific device
> + * i3c_device_do_xfers() - do I3C transfers directed to a specific 
> + device
>   *
>   * @dev: device with which the transfers should be done
>   * @xfers: array of transfers
>   * @nxfers: number of transfers
> + * @mode: transfer mode
>   *
>   * Initiate one or several private SDR transfers with @dev.
>   *
> @@ -33,9 +33,8 @@
>   *   'xfers' some time later. See I3C spec ver 1.1.1 09-Jun-2021. Section:
>   *   5.1.2.2.3.
>   */
> -int i3c_device_do_priv_xfers(struct i3c_device *dev,
> -			     struct i3c_priv_xfer *xfers,
> -			     int nxfers)
> +int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers,
> +			int nxfers, enum i3c_hdr_mode mode)
>  {
>  	int ret, i;
>
> @@ -48,12 +47,12 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev,
>  	}
>
>  	i3c_bus_normaluse_lock(dev->bus);
> -	ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
> +	ret = i3c_dev_do_xfers_locked(dev->desc, xfers, nxfers, mode);
>  	i3c_bus_normaluse_unlock(dev->bus);
>
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);
> +EXPORT_SYMBOL_GPL(i3c_device_do_xfers);
>
>  /**
>   * i3c_device_do_setdasa() - do I3C dynamic address assignement with @@ -260,6 +259,20 @@ i3c_device_match_id(struct i3c_device *i3cdev,  }  EXPORT_SYMBOL_GPL(i3c_device_match_id);
>
> +/**
> + * i3c_device_get_supported_xfer_mode - Returns the supported transfer mode by
> + *					connected master controller.
> + * @dev: I3C device
> + *
> + * Return: a bit mask, which supported transfer mode, bit position is defined at
> + *	   enum i3c_hdr_mode
> + */
> +u32 i3c_device_get_supported_xfer_mode(struct i3c_device *dev) {
> +	return i3c_dev_get_master(dev->desc)->mode_mask;
> +}
> +EXPORT_SYMBOL_GPL(i3c_device_get_supported_xfer_mode);
> +
>  /**
>   * i3c_driver_register_with_owner() - register an I3C device driver
>   *
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h index 0d857cc68cc5d473db733b12ffcec0c1c28d9def..2adba9136f3d147b82c58bd9b491d6d1bc6bfdf7 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -15,9 +15,9 @@ void i3c_bus_normaluse_lock(struct i3c_bus *bus);  void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
>
>  int i3c_dev_setdasa_locked(struct i3c_dev_desc *dev); -int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
> -				 struct i3c_priv_xfer *xfers,
> -				 int nxfers);
> +int i3c_dev_do_xfers_locked(struct i3c_dev_desc *dev,
> +			    struct i3c_xfer *xfers,
> +			    int nxfers, enum i3c_hdr_mode mode);
>  int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev);  int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);  int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev, diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index 2ef898a8fd8065032b68c97c52dcb12e771525a4..1ba21fd737a31386704e47afb3026c4fc8fc7305 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2749,10 +2749,13 @@ EXPORT_SYMBOL_GPL(i3c_generic_ibi_recycle_slot);
>
>  static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)  {
> -	if (!ops || !ops->bus_init || !ops->priv_xfers ||
> +	if (!ops || !ops->bus_init ||
>  	    !ops->send_ccc_cmd || !ops->do_daa || !ops->i2c_xfers)
>  		return -EINVAL;
>
> +	if (!ops->priv_xfers && !ops->i3c_xfers)
> +		return -EINVAL;
> +
>  	if (ops->request_ibi &&
>  	    (!ops->enable_ibi || !ops->disable_ibi || !ops->free_ibi ||
>  	     !ops->recycle_ibi_slot))
> @@ -2808,6 +2811,9 @@ int i3c_master_register(struct i3c_master_controller *master,
>  	master->dev.release = i3c_masterdev_release;
>  	master->ops = ops;
>  	master->secondary = secondary;
> +	/* Spec require must support SDR mode */
> +	master->mode_mask |= BIT(I3C_SDR);
> +
>  	INIT_LIST_HEAD(&master->boardinfo.i2c);
>  	INIT_LIST_HEAD(&master->boardinfo.i3c);
>
> @@ -2942,9 +2948,8 @@ int i3c_dev_setdasa_locked(struct i3c_dev_desc *dev)
>  						dev->boardinfo->init_dyn_addr);
>  }
>
> -int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
> -				 struct i3c_priv_xfer *xfers,
> -				 int nxfers)
> +int i3c_dev_do_xfers_locked(struct i3c_dev_desc *dev, struct i3c_xfer *xfers,
> +			    int nxfers, enum i3c_hdr_mode mode)
>  {
>  	struct i3c_master_controller *master;
>
> @@ -2955,9 +2960,18 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
>  	if (!master || !xfers)
>  		return -EINVAL;
>
> +	if (!(master->mode_mask & BIT(mode)))
> +		return -EOPNOTSUPP;
> +
> +	if (master->ops->i3c_xfers)
> +		return master->ops->i3c_xfers(dev, xfers, nxfers, mode);
> +
>  	if (!master->ops->priv_xfers)
>  		return -EOPNOTSUPP;
>
> +	if (mode != I3C_SDR)
> +		return -EINVAL;
> +
>  	return master->ops->priv_xfers(dev, xfers, nxfers);  }
>
> diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h index 7f136de4b73ef839fb4a1837a87b1aebbddbfe93..be7d9e4c98e09ec29357d19dc73d1f050d7bde1e 100644
> --- a/include/linux/i3c/device.h
> +++ b/include/linux/i3c/device.h
> @@ -40,19 +40,22 @@ enum i3c_error_code {
>
>  /**
>   * enum i3c_hdr_mode - HDR mode ids
> + * @I3C_SDR: SDR mode (NOT HDR mode)
>   * @I3C_HDR_DDR: DDR mode
>   * @I3C_HDR_TSP: TSP mode
>   * @I3C_HDR_TSL: TSL mode
>   */
>  enum i3c_hdr_mode {
> +	I3C_SDR,
>  	I3C_HDR_DDR,
>  	I3C_HDR_TSP,
>  	I3C_HDR_TSL,
>  };

Hi Frank,

Perhaps hdr_mode here no longer be meaningful if SDR were to mix in.
This would more like xfer_mode?
 
>
>  /**
> - * struct i3c_priv_xfer - I3C SDR private transfer
> + * struct i3c_xfer - I3C data transfer
>   * @rnw: encodes the transfer direction. true for a read, false for a write
> + * @cmd: Read/Write command in HDR mode, read: 0x80 - 0xff, write: 0x00 
> + - 0x7f
>   * @len: transfer length in bytes of the transfer
>   * @actual_len: actual length in bytes are transferred by the controller
>   * @data: input/output buffer
> @@ -60,8 +63,11 @@ enum i3c_hdr_mode {
>   * @data.out: output buffer. Must point to a DMA-able buffer
>   * @err: I3C error code
>   */
> -struct i3c_priv_xfer {
> -	u8 rnw;
> +struct i3c_xfer {
> +	union {
> +		u8 rnw;
> +		u8 cmd;
> +	};
>  	u16 len;
>  	u16 actual_len;
>  	union {
> @@ -71,6 +77,9 @@ struct i3c_priv_xfer {
>  	enum i3c_error_code err;
>  };
>
> +/* keep back compatible */
> +#define i3c_priv_xfer i3c_xfer
> +
>  /**
>   * enum i3c_dcr - I3C DCR values
>   * @I3C_DCR_GENERIC_DEVICE: generic I3C device @@ -297,9 +306,15 @@ static __always_inline void i3c_i2c_driver_unregister(struct i3c_driver *i3cdrv,
>  		      i3c_i2c_driver_unregister,	\
>  		      __i2cdrv)
>
> -int i3c_device_do_priv_xfers(struct i3c_device *dev,
> -			     struct i3c_priv_xfer *xfers,
> -			     int nxfers);
> +int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_xfer *xfers,
> +			int nxfers, enum i3c_hdr_mode mode);
> +
> +static inline int i3c_device_do_priv_xfers(struct i3c_device *dev,
> +					   struct i3c_priv_xfer *xfers,
> +					   int nxfers)
> +{
> +	return i3c_device_do_xfers(dev, xfers, nxfers, I3C_SDR); }
>
>  int i3c_device_do_setdasa(struct i3c_device *dev);
>
> @@ -341,5 +356,6 @@ int i3c_device_request_ibi(struct i3c_device *dev,  void i3c_device_free_ibi(struct i3c_device *dev);  int i3c_device_enable_ibi(struct i3c_device *dev);  int i3c_device_disable_ibi(struct i3c_device *dev);
> +u32 i3c_device_get_supported_xfer_mode(struct i3c_device *dev);
>
>  #endif /* I3C_DEV_H */
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h index 043f5c7ff398ff631f1eea6acfc54a2e871016d8..de180511dc65e260885099913141eef8d0768f5e 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_hdr_mode mode);
>  	int (*attach_i2c_dev)(struct i2c_dev_desc *dev);
>  	void (*detach_i2c_dev)(struct i2c_dev_desc *dev);
>  	int (*i2c_xfers)(struct i2c_dev_desc *dev, @@ -505,6 +509,7 @@ struct i3c_master_controller_ops {
>   * @secondary: true if the master is a secondary master
>   * @init_done: true when the bus initialization is done
>   * @hotjoin: true if the master support hotjoin
> + * @mode_mask: bit mask for supported transfer mode
>   * @boardinfo.i3c: list of I3C  boardinfo objects
>   * @boardinfo.i2c: list of I2C boardinfo objects
>   * @boardinfo: board-level information attached to devices connected on the bus @@ -528,6 +533,7 @@ struct i3c_master_controller {
>  	unsigned int secondary : 1;
>  	unsigned int init_done : 1;
>  	unsigned int hotjoin: 1;
> +	unsigned int mode_mask;
>  	struct {
>  		struct list_head i3c;
>  		struct list_head i2c;
>
> --
> 2.34.1
>
>
> -- 
> linux-i3c mailing list
> linux-i3c@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

Powered by blists - more mailing lists