Re: [PATCH v11 1/6] i3c: Add HDR API support

[Andrew Jeffery <andrew@codeconstruct.com.au>]

Hi Frank,

On Thu, 2025-11-06 at 12:36 -0500, 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.
> 
> 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 v9
> - fix typo Deprecated
> - remove reduntant master->ops->priv_xfers check.
> 
> change in v8
> - new API use i3c_xfer instead of i3c_priv_xfer.
> 
> change in v7
> - explicit set enum I3C_HDR_* to value, which spec required.
> - add comments about check priv_xfers and i3c_xfers
> 
> change in v5-v6
> - none
> 
> change in v4
> - Rename enum i3c_hdr_mode to i3c_xfer_mode.
> 
> change in v3
> - Add Deprecated 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 | 40 +++++++++++++++++++++++++++++-----------
>  include/linux/i3c/master.h |  4 ++++
>  5 files changed, 70 insertions(+), 26 deletions(-)
> 

*snip*

>  
> diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
> index 7f136de4b73ef839fb4a1837a87b1aebbddbfe93..7f7738041f3809e538816e94f90b99e58eb806f9 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 {
> -	I3C_HDR_DDR,
> -	I3C_HDR_TSP,
> -	I3C_HDR_TSL,
> +enum i3c_xfer_mode {
> +	/* The below 3 value (I3C_HDR*) must match GETCAP1 Byte bit position */
> +	I3C_HDR_DDR = 0,
> +	I3C_HDR_TSP = 1,
> +	I3C_HDR_TSL = 2,
> +	/* Use for default SDR transfer mode */
> +	I3C_SDR = 0x31,

0x31 is 49 - is that really what you intend here? For instance,
building this patch for ARM32 produces:

   In file included from ../include/linux/bits.h:5,
                    from ../include/linux/ratelimit_types.h:5,
                    from ../include/linux/printk.h:9,
                    from ../include/asm-generic/bug.h:31,
                    from ../arch/arm/include/asm/bug.h:60,
                    from ../include/linux/bug.h:5,
                    from ../drivers/i3c/device.c:9:
   ../drivers/i3c/device.c: In function ‘i3c_device_get_supported_xfer_mode’:
   ../include/vdso/bits.h:7:40: warning: left shift count >= width of type [-Wshift-count-overflow]
       7 | #define BIT(nr)                 (UL(1) << (nr))
         |                                        ^~
   ../drivers/i3c/device.c:272:68: note: in expansion of macro ‘BIT’
     272 |         return i3c_dev_get_master(dev->desc)->this->info.hdr_cap | BIT(I3C_SDR);
         |                                                                    ^~~

Should this be decimal 31, rather than hex 31?

Andrew