Re: [PATCH 1/2] i3c: Add basic HDR support

[Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>]

Hi Frank,

On 1/30/2025 1:35 AM, Frank Li wrote:
> I3C HDR requires enter/exit patterns during each I3C transfer. Add a new
> API i3c_device_do_priv_xfers_mode(). The existing
> i3c_device_do_priv_xfers() now calls i3c_device_do_priv_xfers_mode(I3C_SDR)
> to maintain backward compatibility.
> 
> Introduce a 'cmd' field in 'struct i3c_priv_xfer', using an anonymous union
> with 'rnw' since HDR mode relies on read/write commands instead of the
> 'rnw' bit in the address as in SDR mode.
> 
> Add a priv_xfers_mode callback for I3C master drivers. If priv_xfers_mode
> is not implemented, fallback to SDR mode using the existing priv_xfers
> callback.
> 
> 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.
> 
Is it wise to add two function inside main funcion and separate SDR vs 
HDR ? so we don't need to change current function arguments.
> 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].
> ---
>   drivers/i3c/device.c       | 19 +++++++++++++------
>   drivers/i3c/internals.h    |  2 +-
>   drivers/i3c/master.c       |  8 +++++++-
>   include/linux/i3c/device.h | 12 +++++++++++-
>   include/linux/i3c/master.h |  3 +++
>   5 files changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
> index e80e487569146..e3db3a6a9e4f6 100644
> --- a/drivers/i3c/device.c
> +++ b/drivers/i3c/device.c
> @@ -15,12 +15,13 @@
>   #include "internals.h"
>   
>   /**
> - * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a
> - *				specific device
> + * i3c_device_do_priv_xfers_mode() - do I3C SDR private 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.
>    *
> @@ -32,9 +33,9 @@
>    *		    driver needs to resend the '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_priv_xfers_mode(struct i3c_device *dev,
> +				  struct i3c_priv_xfer *xfers,
> +				  int nxfers, enum i3c_hdr_mode mode)
why can't we pass mode as another structure member inside struct 
i3c_priv_xfer ? Adding function agrument parameter impacts existing drivers.
>   {
>   	int ret, i;
>   
> @@ -47,11 +48,17 @@ 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_priv_xfers_locked(dev->desc, xfers, nxfers, mode);
>   	i3c_bus_normaluse_unlock(dev->bus);
>   
>   	return ret;
>   }
> +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers_mode);
> +
> +int i3c_device_do_priv_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers, int nxfers)
> +{
> +	return i3c_device_do_priv_xfers_mode(dev, xfers, nxfers, I3C_SDR);
> +}
>   EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);
>   
>   /**
> diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> index 433f6088b7cec..553edc9846ac0 100644
> --- a/drivers/i3c/internals.h
> +++ b/drivers/i3c/internals.h
> @@ -16,7 +16,7 @@ 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 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 d5dc4180afbcf..67aaba0a38db2 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2945,7 +2945,7 @@ 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 nxfers, enum i3c_hdr_mode mode)
>   {
>   	struct i3c_master_controller *master;
>   
> @@ -2956,9 +2956,15 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
>   	if (!master || !xfers)
>   		return -EINVAL;
>   
> +	if (master->ops->priv_xfers_mode)
> +		return master->ops->priv_xfers_mode(dev, xfers, nxfers, mode);
> +
>   	if (!master->ops->priv_xfers)
>   		return -ENOTSUPP;
>   
> +	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 b674f64d0822e..7ce70d0967e27 100644
> --- a/include/linux/i3c/device.h
> +++ b/include/linux/i3c/device.h
> @@ -40,11 +40,13 @@ 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,
> @@ -53,6 +55,7 @@ enum i3c_hdr_mode {
>   /**
>    * struct i3c_priv_xfer - I3C SDR private 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
> @@ -61,7 +64,10 @@ enum i3c_hdr_mode {
>    * @err: I3C error code
>    */
>   struct i3c_priv_xfer {
> -	u8 rnw;
> +	union {
> +		u8 rnw;
> +		u8 cmd;
> +	};
>   	u16 len;
>   	u16 actual_len;
>   	union {
> @@ -301,6 +307,10 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev,
>   			     struct i3c_priv_xfer *xfers,
>   			     int nxfers);
>   
> +int i3c_device_do_priv_xfers_mode(struct i3c_device *dev,
> +				  struct i3c_priv_xfer *xfers,
> +				  int nxfers, enum i3c_hdr_mode mode);
> +
>   int i3c_device_do_setdasa(struct i3c_device *dev);
>   
>   void i3c_device_get_info(const struct i3c_device *dev, struct i3c_device_info *info);
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 12d532b012c5a..352bd41139569 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -472,6 +472,9 @@ struct i3c_master_controller_ops {
>   	int (*priv_xfers)(struct i3c_dev_desc *dev,
>   			  struct i3c_priv_xfer *xfers,
>   			  int nxfers);
> +	int (*priv_xfers_mode)(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,
>