[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID:
<SH0PR01MB08416A34379DDE9F7762415BF9E62@SH0PR01MB0841.CHNPR01.prod.partner.outlook.cn>
Date: Wed, 1 Oct 2025 09:07:49 +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 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;
We might actually not need to have a separate field for mode_mask here.
dev->info.hdr_cap should already contains information whether particular HDR is supported.
We should check both controller and device support HDR before triggering any command.
> +
> + 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,
> };
>
> /**
> - * 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