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

[Greg Kroah-Hartman]

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

> > > 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 :)

> > 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!

> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/bits.h>
> > > +#include <linux/console.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/io.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/serial_core.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/tty_flip.h>
> > > +#include <asm/serial.h>
> > > +
> > > +#define DRIVER_NAME  "esp32s3-acm"
> > > +#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?

> > > +static void esp32s3_acm_set_mctrl(struct uart_port *port, unsigned int mctrl)
> > > +{
> > > +}
> >
> > Do you have to have empty functions for callbacks that do nothing?
> 
> The serial core has unconditional calls to these callbacks.

Ah, good catch, maybe we should fix up the serial core.

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

thanks,

greg k-h