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

Guenter Roeck
2025-10-28T15:30:33Z

On Mon, Oct 27, 2025 at 04:08:29PM -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.
>
> 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 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 Depreciated comment for priv_xfers.

Deprecated

>
> 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.
> ---
...
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2821,10 +2821,14 @@ 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;
>
> + /* Must provide one of priv_xfers (SDR only) or i3c_xfers (all modes) */
> + 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))
> @@ -3014,9 +3018,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;
>
> @@ -3027,9 +3030,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;

i3c_master_check_ops() already ensures that either i3c_xfers or priv_xfers
is not NULL. The above check is therefore unnecessary.

That is not a new issue, though. Maybe there was/is a reason for the
additional check, but I don't see it.

>
> + if (mode != I3C_SDR)
> + return -EINVAL;
> +
> return master->ops->priv_xfers(dev, xfers, nxfers);
> }
>
...
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index c52a82dd79a63436c1de6a01c11df9e295c1660e..7e22f9d2a2ca3f4ab808db50f809efd192c795cd 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() */

Deprecated