Message-ID: <aTDiakSN34TCIskS@lizhi-Precision-Tower-5810>
Date: Wed, 3 Dec 2025 20:22:50 -0500
From: Frank Li <Frank.li@....com>
To: Andrew Jeffery <andrew@...econstruct.com.au>
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, linux@...ck-us.net
Subject: Re: [PATCH v11 1/6] i3c: Add HDR API support

On Thu, Dec 04, 2025 at 10:47:57AM +1030, Andrew Jeffery wrote:
> Hi Frank,
>
> On Thu, 2025-11-06 at 12:36 -0500, 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@....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 v9
> > - fix typo Deprecated
> > - remove reduntant master->ops->priv_xfers check.
> >
> > change in v8
> > - new API use i3c_xfer instead of i3c_priv_xfer.
> >
> > 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 Deprecated 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       | 19 ++++++++++++++-----
> >  include/linux/i3c/device.h | 40 +++++++++++++++++++++++++++++-----------
> >  include/linux/i3c/master.h |  4 ++++
> >  5 files changed, 70 insertions(+), 26 deletions(-)
> >
>
> *snip*
>
> >  
> > diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
> > index 7f136de4b73ef839fb4a1837a87b1aebbddbfe93..7f7738041f3809e538816e94f90b99e58eb806f9 100644
> > --- a/include/linux/i3c/device.h
> > +++ b/include/linux/i3c/device.h
> > @@ -39,20 +39,25 @@ enum i3c_error_code {
> >  };
> >  
> >  /**
> > - * enum i3c_hdr_mode - HDR mode ids
> > + * enum i3c_xfer_mode - I3C xfer mode ids
> >   * @I3C_HDR_DDR: DDR mode
> >   * @I3C_HDR_TSP: TSP mode
> >   * @I3C_HDR_TSL: TSL mode
> > + * @I3C_SDR: SDR mode (NOT HDR mode)
> >   */
> > -enum i3c_hdr_mode {
> > -	I3C_HDR_DDR,
> > -	I3C_HDR_TSP,
> > -	I3C_HDR_TSL,
> > +enum i3c_xfer_mode {
> > +	/* The below 3 value (I3C_HDR*) must match GETCAP1 Byte bit position */
> > +	I3C_HDR_DDR = 0,
> > +	I3C_HDR_TSP = 1,
> > +	I3C_HDR_TSL = 2,
> > +	/* Use for default SDR transfer mode */
> > +	I3C_SDR = 0x31,
>
> 0x31 is 49 - is that really what you intend here? For instance,
> building this patch for ARM32 produces:
>
>    In file included from ../include/linux/bits.h:5,
>                     from ../include/linux/ratelimit_types.h:5,
>                     from ../include/linux/printk.h:9,
>                     from ../include/asm-generic/bug.h:31,
>                     from ../arch/arm/include/asm/bug.h:60,
>                     from ../include/linux/bug.h:5,
>                     from ../drivers/i3c/device.c:9:
>    ../drivers/i3c/device.c: In function ‘i3c_device_get_supported_xfer_mode’:
>    ../include/vdso/bits.h:7:40: warning: left shift count >= width of type [-Wshift-count-overflow]
>        7 | #define BIT(nr)                 (UL(1) << (nr))
>          |                                        ^~
>    ../drivers/i3c/device.c:272:68: note: in expansion of macro ‘BIT’
>      272 |         return i3c_dev_get_master(dev->desc)->this->info.hdr_cap | BIT(I3C_SDR);
>          |                                                                    ^~~
>
> Should this be decimal 31, rather than hex 31?

Yes, fixed patch

https://lore.kernel.org/linux-i3c/aS8YKfhPAxvpj6xy@lizhi-Precision-Tower-5810/T/#m7367e8845c31b924672445dcf639eea39112aac0

Frank

>
> Andrew
>
> --
> linux-i3c mailing list
> linux-i3c@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

Powered by blists - more mailing lists