Message-ID: <aPq/6/+63sHuq/qy@lizhi-Precision-Tower-5810>
Date: Thu, 23 Oct 2025 19:53:15 -0400
From: Frank Li <Frank.li@....com>
To: Andy Shevchenko <andriy.shevchenko@...el.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 09:22:55PM +0300, Andy Shevchenko wrote:
> 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.

Sorry, I agree, It should assigned value to it. Actually, I have not
realized it tights to Hardware bits at beginning.

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

Yes!

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

There also are other cleanup works. The key point is that everyone agree my
HDR solution. Cleanup these is not big deal. I am not sure how to avoid
build broken at difference subsystem.

After this patch merge, cleanup will be easier and safer.

Frank

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

Powered by blists - more mailing lists