Re: [PATCH v4 5/5] drivers/tty/serial: add ESP32S3 ACM device driver

[Max Filippov]

On Thu, Oct 5, 2023 at 11:57 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, Oct 03, 2023 at 12:46:46PM -0700, Max Filippov wrote:
> > > > Hardware specification is available at the following URL:
> > > >
> > > >   https://www.espressif.com/sites/default/files/documentation/esp32-s3_technical_reference_manual_en.pdf
> > > >   (Chapter 33 USB Serial/JTAG Controller)
> > >
> > > I don't understand this driver, "ACM" is a USB host <-> gadget protocol,
> > > why do you need a platform serial driver for this?
> >
> > The USB part of this piece of hardware is fixed and not controllable, so
> > all we have is a very limited UART interface. But to the outside world
> > it's a USB device with the CDC-ACM interface.
>
> Where is the "outside world" here?  The other end of the tty connection?

Yes.

> So this is a "ACM gadget"?  If so, please try to use that term as that's
> what we use in the kernel to keep things straight.

Ok.

> > > > diff --git a/drivers/tty/serial/esp32_acm.c b/drivers/tty/serial/esp32_acm.c
> > > > new file mode 100644
> > > > index 000000000000..f02abd2ac65e
> > > > --- /dev/null
> > > > +++ b/drivers/tty/serial/esp32_acm.c
> > > > @@ -0,0 +1,459 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > >
> > > Why "or later"?  I have to ask, sorry.
> >
> > I don't really have a preference here. Is there a reason to choose
> > GPL-2.0 only for a new code?
>
> It's your call, you need to pick that, but I can provide recommendations
> if you want :)

Please do?

> > > And no copyright information?  That's fine, but be sure your company's
> > > lawyers are ok with it...
> >
> > There's no company behind this, just myself.
>
> Great, it's your copyright, be proud, put it on there!

If I don't have to I'd rather not. This is just a piece of meaningless noise.

> > > > +#define DEV_NAME     "ttyACM"
> > >
> > > There is already a ttyACM driver in the kernel, will this conflict with
> > > that one?  And are you using the same major/minor numbers for it as
> > > well?  If so, how is this going to work?
> >
> > I'll rename it to ttyS. I see that it coexists with the other driver that calls
> > its devices ttyS just fine.
>
> Great.  But if you are going to be like a ACM gadget, use ttyGS like
> that driver does?

Ok.

> > > > --- a/include/uapi/linux/serial_core.h
> > > > +++ b/include/uapi/linux/serial_core.h
> > > > @@ -248,4 +248,7 @@
> > > >  /* Espressif ESP32 UART */
> > > >  #define PORT_ESP32UART       124
> > > >
> > > > +/* Espressif ESP32 ACM */
> > > > +#define PORT_ESP32ACM        125
> > >
> > > Why are these defines needed?  What in userspace is going to require
> > > them?  If nothing, please do not add them.
> >
> > I don't understand what the alternatives are. The comment for the
> > uart_ops::config_port() callback says that port->type should be set
> > to the type of the port found, and I see that almost every serial driver
> > defines a unique PORT_* for that.
>
> Yes, but not all do.  If you don't need to do anything special, it can
> just claim to be a normal device, we've had threads about this on the
> list before.  If you don't need to determine in userspace from the tty
> connection what device it is, just use the default one instead.

Ok, it looks like having

#define PORT_ESP32ACM (-1)

in the driver source should be ok? I've tested that it works.

-- 
Thanks.
-- Max