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

Andy Shevchenko
2025-10-23T08:23:46Z

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