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

Frank Li
2025-10-23T15:18:50Z

On Thu, Oct 23, 2025 at 11:23:39AM +0300, Andy Shevchenko wrote:
> 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;

Not exactly, priv_xfers is old API, which supported now. I plan remove it
after remove all from i3c master controller driver after this patch merged.

i2c_xfers: must be no NULL

priv_xfers and i3c_xfers, one of both should be no NULL.

i2c_xfer is NULL, should be return -EINVAL, but you logic may success if
i3c_xfers is not NULL.

>
>
> > 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.

This struct use only for i3c_device_do_xfers(), which pass i3c_xfer_mode
informaiton. argument 'mode' will distrigiush rnw/cmd.

i3c_xfer[] array don't allow switch transfer mode during whole i3c
transcation. So doesn't put mode in here.

>
> > 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?

git grep -r priv_xfer drivers/
drivers/base/regmap/regmap-i3c.c: struct i3c_priv_xfer xfers[] = {
drivers/base/regmap/regmap-i3c.c: return i3c_device_do_priv_xfers(i3c, xfers, 1);
drivers/base/regmap/regmap-i3c.c: struct i3c_priv_xfer xfers[2];
drivers/base/regmap/regmap-i3c.c: return i3c_device_do_priv_xfers(i3c, xfers, 2);
drivers/hwmon/lm75.c: struct i3c_priv_xfer xfers[] = {
drivers/hwmon/lm75.c: ret = i3c_device_do_priv_xfers(i3cdev, xfers, 2);
drivers/hwmon/lm75.c: struct i3c_priv_xfer xfers[] = {
drivers/hwmon/lm75.c: return i3c_device_do_priv_xfers(i3cdev, xfers, 1);
drivers/i3c/device.c:int i3c_device_do_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers,
drivers/i3c/master.c: if (!ops->priv_xfers && !ops->i3c_xfers)
drivers/i3c/master.c: if (!master->ops->priv_xfers)
drivers/i3c/master.c: return master->ops->priv_xfers(dev, xfers, nxfers);
drivers/i3c/master/dw-i3c-master.c:static int dw_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
drivers/i3c/master/dw-i3c-master.c: struct i3c_priv_xfer *i3c_xfers,
drivers/i3c/master/dw-i3c-master.c: .priv_xfers = dw_i3c_master_priv_xfers,
drivers/i3c/master/i3c-master-cdns.c:static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev,
drivers/i3c/master/i3c-master-cdns.c: struct i3c_priv_xfer *xfers,
drivers/i3c/master/i3c-master-cdns.c: .priv_xfers = cdns_i3c_master_priv_xfers,
drivers/i3c/master/mipi-i3c-hci/core.c:static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
drivers/i3c/master/mipi-i3c-hci/core.c: struct i3c_priv_xfer *i3c_xfers,
drivers/i3c/master/mipi-i3c-hci/core.c: .priv_xfers = i3c_hci_priv_xfers,
drivers/i3c/master/renesas-i3c.c:static int renesas_i3c_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *i3c_xfers,
drivers/i3c/master/renesas-i3c.c: .priv_xfers = renesas_i3c_priv_xfers,
drivers/i3c/master/svc-i3c-master.c: struct i3c_priv_xfer *xfer;
drivers/i3c/master/svc-i3c-master.c: * at svc_i3c_master_priv_xfers().
drivers/i3c/master/svc-i3c-master.c:static int svc_i3c_master_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers,
drivers/net/mctp/mctp-i3c.c: struct i3c_priv_xfer xfer = { .rnw = 1, .len = mi->mrl };
drivers/net/mctp/mctp-i3c.c: rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);
drivers/net/mctp/mctp-i3c.c: struct i3c_priv_xfer xfer = { .rnw = false };
drivers/net/mctp/mctp-i3c.c: rc = i3c_device_do_priv_xfers(mi->i3c, &xfer, 1);

After this patch merged, I can clean up it at difference subsytem. After
all cleanup done, we can safely remove this define.

Frank
>
> --
> With Best Regards,
> Andy Shevchenko
>
>