Message-ID: <aPpyf1xPmU_koEXH@smile.fi.intel.com>
Date: Thu, 23 Oct 2025 21:22:55 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Frank Li <Frank.li@....com>
Cc: 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>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-i3c@...ts.infradead.org,
	linux-kernel@...r.kernel.org, imx@...ts.linux.dev,
	linux-iio@...r.kernel.org, joshua.yeong@...rfivetech.com,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v6 1/5] i3c: Add HDR API support

On Thu, Oct 23, 2025 at 11:18:37AM -0400, Frank Li wrote:
> 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:

...

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

You are right. I misread && as ||. Can you add a summary of the above in the
comment above the conditionals or kernel-doc of this function (if present)?

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

Are you agree on this or disagree or...?
If you agree, don't leave the unneeded context in the reply.
Otherwise, please express your objections.

...

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

Then why that mode field is not present here?

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

I presume that this is the answer to my above Q? If so, I think this
dislayering is not okay, because the struct effectively lost the crucial piece
of information (for example, if you need to trace the contents of it, the mode
also needs to be provided again, and so on). I think the data type must have
this mode field as well (as long as union is in use).

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

`git grep -lw ...` is a better approach :-)

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

I counted 9. I think it's not a big deal to convert all of them at once without
leaving an intermediate state. But this is a call for the I³C subsystem maintainer.

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists