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

Frank Li
2023-10-23T16:41:52Z

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