[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMo8BfJgpP-=tNEChcyR3z6i_QeJ9Ywq7EOjjC5i7Uq4OrgXNA@mail.gmail.com>
Date: Tue, 3 Oct 2023 12:46:46 -0700
From: Max Filippov <jcmvbkbc@...il.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
devicetree@...r.kernel.org, Jiri Slaby <jirislaby@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH v4 5/5] drivers/tty/serial: add ESP32S3 ACM device driver
On Tue, Oct 3, 2023 at 5:55 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> 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
Powered by blists - more mailing lists