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
