Re: [PATCH v2 1/1] tty: i3c: add TTY over I3C master support

From: Frank Li <Frank.li@nxp.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: alexandre.belloni@bootlin.com, conor.culhane@silvaco.com,
	imx@lists.linux.dev, jirislaby@kernel.org, joe@perches.com,
	linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-serial@vger.kernel.org, miquel.raynal@bootlin.com
Subject: Re: [PATCH v2 1/1] tty: i3c: add TTY over I3C master support
Date: Mon, 23 Oct 2023 12:26:42 -0400	[thread overview]
Message-ID: <ZTaewidgtcDaBega@lizhi-Precision-Tower-5810> (raw)
In-Reply-To: <2023102105-outfit-legroom-1633@gregkh>

On Sat, Oct 21, 2023 at 07:02:40PM +0200, Greg KH wrote:
> Note, your subject line needs to change.
> 
> On Fri, Oct 20, 2023 at 12:00:27PM -0400, Frank Li wrote:
> > In typical embedded Linux systems, UART consoles require at least two pins,
> > TX and RX. In scenarios where I2C/I3C devices like sensors or PMICs are
> > present, we can save these two pins by using this driver. Pins is crucial
> 
> "Pins are crucial"
> 
> > resources, especially in small chip packages.
> > 
> > This introduces support for using the I3C bus to transfer console tty data,
> > effectively replacing the need for dedicated UART pins. This not only
> > conserves valuable pin resources but also facilitates testing of I3C's
> > advanced features, including early termination, in-band interrupt (IBI)
> > support, and the creation of more complex data patterns. Additionally,
> > it aids in identifying and addressing issues within the I3C controller
> > driver.
> 
> But where is the serial data ending up at?  Not a normal uart, what is
> on the other end?  And do line settings mean anything here?

Currently, it use slave i3c code. 
https://lore.kernel.org/imx/20231018215809.3477437-1-Frank.Li@nxp.com/T/#t

idealy build an i3c->usb dongle to bride it to usb acm. 

> 
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > 
> > Notes:
> >     This patch depend on
> >     https://lore.kernel.org/imx/20231018205929.3435110-1-Frank.Li@nxp.com/T/#t
> 
> Let's wait for those to be accepted first, right?

Yes.

> 
> > +static DEFINE_IDR(i3c_tty_minors);
> > +static DEFINE_MUTEX(i3c_tty_minors_lock);
> 
> I thought idr didn't need a mutex anymore, are you sure this is still
> needed?
> 
> > +static struct tty_driver *i3c_tty_driver;
> > +
> > +#define I3C_TTY_MINORS		256
> 
> Do you really need 256 minors?

Any resource concern about it. Maybe 32/64 is enough. I refer from USB tty
driver.

> 
> > +#define I3C_TTY_TRANS_SIZE	16
> > +#define I3C_TTY_RX_STOP		0
> > +#define I3C_TTY_RETRY		20
> > +#define I3C_TTY_YIELD_US	100
> > +
> > +struct ttyi3c_port {
> > +	struct tty_port port;
> > +	int minor;
> > +	spinlock_t xlock; /* protect xmit */
> > +	char tx_buff[I3C_TTY_TRANS_SIZE];
> > +	char rx_buff[I3C_TTY_TRANS_SIZE];
> > +	struct i3c_device *i3cdev;
> > +	struct work_struct txwork;
> > +	struct work_struct rxwork;
> > +	struct completion txcomplete;
> > +	unsigned long status;
> > +	int buf_overrun;
> 
> You set buf_overrun but never do anything with it.  Why care about it?
> 
> Other than these minor things, looks sane, nice work.
> 
> thanks,
> 
> greg k-h