On Tue, Oct 3, 2023 at 5:55 AM Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > > On Thu, Sep 28, 2023 at 08:16:31AM -0700, Max Filippov wrote: > > Add driver for the ACM controller of the Espressif ESP32S3 Soc. > > Odd extra space :( Ok. > > 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. > > 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? > 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. > > + > > +#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. > > +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. > > --- 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. -- Thanks. -- Max
