Skip to contentSkip to conversation

serial: add drivers for the ESP32xx serial devices

PATCH Superseded Max Filippov 4 participants 18 messages Sep 28, 2023 – Oct 6, 2023

Subsystems (2)

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

v4 cover serial: add drivers for the ESP32xx serial devices

Hello,

this series adds drivers for the UART and ACM controllers found in the Espressif ESP32 and ESP32S3 SoCs.

Changes v3->v4:
- address review comments, listed in each patch
- add reviewed-by tags

Changes v2->v3:
- address review comments, listed in each patch

Changes v1->v2:
- address review comments, listed in each patch
- add cleanup for the uart_get_baud_rate function

Max Filippov (5):
serial: core: tidy invalid baudrate handling in uart_get_baud_rate
dt-bindings: serial: document esp32-uart drivers/tty/serial: add driver for the ESP32 UART
dt-bindings: serial: document esp32s3-acm drivers/tty/serial: add ESP32S3 ACM device driver

 .../bindings/serial/esp,esp32-acm.yaml        |  42 +
 .../bindings/serial/esp,esp32-uart.yaml       |  51 ++
 drivers/tty/serial/Kconfig                    |  27 +
 drivers/tty/serial/Makefile                   |   2 +
 drivers/tty/serial/esp32_acm.c                | 459 +++++++++++
 drivers/tty/serial/esp32_uart.c               | 741 ++++++++++++++++++
 drivers/tty/serial/serial_core.c              |   4 +-
 include/uapi/linux/serial_core.h              |   6 +
 8 files changed, 1329 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/serial/esp,esp32-acm.yaml
 create mode 100644 Documentation/devicetree/bindings/serial/esp,esp32-uart.yaml
 create mode 100644 drivers/tty/serial/esp32_acm.c
 create mode 100644 drivers/tty/serial/esp32_uart.c
Signature · 1 line
2.30.2
#1 lore raw

PATCH v4 1/5 serial: core: tidy invalid baudrate handling in uart_get_baud_rate

uart_get_baud_rate has input parameters 'min' and 'max' limiting the range of acceptable baud rates from the caller's perspective. If neither current or old termios structures have acceptable baud rate setting and 9600 is not in the min/max range either the function returns 0 and issues a warning.
However for a UART that does not support speed of 9600 baud this is expected behavior.
Clarify that 0 can be (and always could be) returned from the uart_get_baud_rate. Don't issue a warning in that case.
Move the warinng to the uart_get_divisor instead, which is often called with the uart_get_baud_rate return value.

---
Changes v3->v4:
- drop WARN_ON from uart_get_divisor()

 drivers/tty/serial/serial_core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/tty/serial/serial_core.c +1 -3
index 7bdc21d5e13b..3f130fe9f1a0 100644
@@ -431,7 +431,7 @@ EXPORT_SYMBOL(uart_update_timeout);
  * baud.
  *
  * If the new baud rate is invalid, try the @old termios setting. If it's still
- * invalid, we try 9600 baud.
+ * invalid, we try 9600 baud. If that is also invalid 0 is returned.
  *
  * The @termios structure is updated to reflect the baud rate we're actually
  * going to be using. Don't do this for the case where B0 is requested ("hang
@@ -515,8 +515,6 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
 							max - 1, max - 1);
 		}
 	}
-	/* Should never happen */
-	WARN_ON(1);
 	return 0;
 }
 EXPORT_SYMBOL(uart_get_baud_rate);
Signature · 1 line
2.30.2
#2 lore raw

Re: [PATCH v4 1/5] serial: core: tidy invalid baudrate handling in uart_get_baud_rate

Quoted 34 lines · Max Filippov

uart_get_baud_rate has input parameters 'min' and 'max' limiting the range of acceptable baud rates from the caller's perspective. If neither current or old termios structures have acceptable baud rate setting and 9600 is not in the min/max range either the function returns 0 and issues a warning.
However for a UART that does not support speed of 9600 baud this is expected behavior.
Clarify that 0 can be (and always could be) returned from the uart_get_baud_rate. Don't issue a warning in that case.
Move the warinng to the uart_get_divisor instead, which is often called with the uart_get_baud_rate return value.

---
Changes v3->v4:
- drop WARN_ON from uart_get_divisor()

 drivers/tty/serial/serial_core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/tty/serial/serial_core.c +1 -1
index 7bdc21d5e13b..3f130fe9f1a0 100644
@@ -431,7 +431,7 @@ EXPORT_SYMBOL(uart_update_timeout);
  * baud.
  *
  * If the new baud rate is invalid, try the @old termios setting. If it's still
- * invalid, we try 9600 baud.
+ * invalid, we try 9600 baud. If that is also invalid 0 is returned.
  *
  * The @termios structure is updated to reflect the baud rate we're actually
  * going to be using. Don't do this for the case where B0 is requested ("hang

drivers/tty/serial/serial_core.c uart_get_baud_rate(struct uart_port *port, struct ktermios *termios, · line 520

Quoted 8 lines
drivers/tty/serial/serial_core.c +0 -2
@@ -515,8 +515,6 @@ uart_get_baud_rate(struct uart_port *port, struct ktermios *termios,
 							max - 1, max - 1);
 		}
 	}
-	/* Should never happen */
-	WARN_ON(1);
 	return 0;
 }
 EXPORT_SYMBOL(uart_get_baud_rate);

While looking into this, I found this old commit:

commit 16ae2a877bf4179737921235e85ceffd7b79354f

Author: Alan Cox <[email protected]>
Date:   Mon Jan 4 16:26:21 2010 +0000

serial: Fix crash if the minimum rate of the device is > 9600 baud

In that situation if the old rate is invalid and the new rate is invalid and the chip cannot do 9600 baud we report zero, which makes all the drivers explode.

Instead force the rate based on min/max

But for some reason it does not work as advertized here? What is the exact cause for that?

Is something wrong with how min/max have that +1/-1 there or what?

Signature · 1 line
 i.
#3 lore raw

Re: [PATCH v4 1/5] serial: core: tidy invalid baudrate handling in uart_get_baud_rate

Quoted 8 lines · Ilpo Järvinen

While looking into this, I found this old commit:

commit 16ae2a877bf4179737921235e85ceffd7b79354f

Author: Alan Cox <[email protected]>
Date:   Mon Jan 4 16:26:21 2010 +0000

serial: Fix crash if the minimum rate of the device is > 9600 baud

Quoted 8 lines

In that situation if the old rate is invalid and the new rate is invalid and the chip cannot do 9600 baud we report zero, which makes all the drivers explode.

Instead force the rate based on min/max

But for some reason it does not work as advertized here? What is the exact cause for that?

In my case I see that tty_termios_encode_baud_rate() is called with ibaud == obaud == 9769, but it finds the closest rate 9600, which is within 2% of the actual minimum, but is outside the min/max range supported by the hardware.

Quoted 1 line

Is something wrong with how min/max have that +1/-1 there or what?

Signature · 2 lines
Thanks.
-- Max
#4 lore raw

Re: [PATCH v4 1/5] serial: core: tidy invalid baudrate handling in uart_get_baud_rate

In reply to Max Filippov
Quoted 11 lines · Max Filippov

uart_get_baud_rate has input parameters 'min' and 'max' limiting the range of acceptable baud rates from the caller's perspective. If neither current or old termios structures have acceptable baud rate setting and 9600 is not in the min/max range either the function returns 0 and issues a warning.
However for a UART that does not support speed of 9600 baud this is expected behavior.
Clarify that 0 can be (and always could be) returned from the uart_get_baud_rate. Don't issue a warning in that case.
Move the warinng to the uart_get_divisor instead, which is often called with the uart_get_baud_rate return value.

This doesn't match up with the patch contents anymore :(

#5 lore raw

PATCH v4 2/5 dt-bindings: serial: document esp32-uart

In reply to Max Filippov

Add documentation for the ESP32xx UART controllers.

---
Changes v2->v3:
- add a reference to serial.yaml

Changes v1->v2:
- drop '|' from description
- change 'compatible' property type to enum
- drop label from the example node
- fix example indentation

 .../bindings/serial/esp,esp32-uart.yaml       | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/esp,esp32-uart.yaml
Documentation/devicetree/bindings/serial/esp,esp32-uart.yaml new file +51 -0
new file mode 100644
index 000000000000..2a80ca997a0c
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serial/esp,esp32-uart.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ESP32xx UART controllers
+
+maintainers:
+  - Max Filippov <[email protected]>
+
+description:
+  ESP32 UART controller is a part of the ESP32 SoC.
+  ESP32S3 UART controller is a part of the ESP32S3 SoC.
+  Both SoCs are produced by Espressif Systems Co. Ltd.
+
+allOf:
+  - $ref: serial.yaml#
+
+properties:
+  compatible:
+    enum:
+      - esp,esp32-uart
+      - esp,esp32s3-uart
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    serial@60000000 {
+      compatible = "esp,esp32s3-uart";
+      reg = <0x60000000 0x80>;
+      interrupts = <27 1 0>;
+      clocks = <&serial_clk>;
+    };
Signature · 1 line
2.30.2
#6 lore raw

PATCH v4 3/5 drivers/tty/serial: add driver for the ESP32 UART

In reply to Max Filippov

Add driver for the UART controllers of the Espressif ESP32 and ESP32S3 SoCs. Hardware specification is available at the following URLs:

https://www.espressif.com/sites/default/files/documentation/esp32_technical_reference_manual_en.pdf
(Chapter 13 UART Controller)
https://www.espressif.com/sites/default/files/documentation/esp32-s3_technical_reference_manual_en.pdf
(Chapter 26 UART Controller)

---
Changes v2->v3:
- rework brk handling in esp32_uart_rxint
- only increment port->icount.rx in case insert_flip_char() is called
- use HZ instead of msecs_to_jiffies(1000) in esp32_uart_put_char_sync
- add early return to esp32_uart_transmit_buffer
- use request_irq/free_irq instead of devm_* versions
- add blank lines before certain return statements

Changes v1->v2:
- redefine register fields using BIT and GENMASK
- drop _MASK suffix from register field definitions
- drop *_SHIFT definitions where possible
- drop unused rxfifo_full_thrhd_mask and txfifo_empty_thrhd_mask
- split register reads/writes and bitwise operations into multiple lines
- use u8 instead of unsigned char in internal functions
- add timeout to esp32_uart_put_char_sync
- use uart_port_tx_limited in esp32_uart_transmit_buffer
- use IRQ_RETVAL in esp32_uart_int
- disable clock in esp32_uart_startup in case devm_request_irq fails
- rearrange devm_request_irq with enabling IRQs in the UART registers
- drop empty esp32_uart_release_port and esp32_uart_request_port
- simplify esp32_uart_tx_empty
- mask out unsupported CMSPAR flag in termios->c_cflag in esp32_uart_set_termios
- invoke uart_update_timeout in esp32_uart_set_termios
- drop MODULE_DESCRIPTION
- rearrange esp32_uart_set_baud: return bool indicating whether baud rate was set or not, use it in the esp32_uart_set_termios to set the default 115200
- turn 'locked' into bool in esp32_uart_console_write
- turn num_read into unsigned int in esp32_uart_earlycon_read

 drivers/tty/serial/Kconfig       |  13 +
 drivers/tty/serial/Makefile      |   1 +
 drivers/tty/serial/esp32_uart.c  | 741 +++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |   3 +
 4 files changed, 758 insertions(+)
 create mode 100644 drivers/tty/serial/esp32_uart.c
drivers/tty/serial/Kconfig +13 -0
index bdc568a4ab66..d9ca6b268f01 100644
@@ -1578,6 +1578,19 @@ config SERIAL_NUVOTON_MA35D1_CONSOLE
 	  but you can alter that using a kernel command line option such as
 	  "console=ttyNVTx".
 
+config SERIAL_ESP32
+	tristate "Espressif ESP32 UART support"
+	depends on XTENSA_PLATFORM_ESP32 || (COMPILE_TEST && OF)
+	select SERIAL_CORE
+	select SERIAL_CORE_CONSOLE
+	select SERIAL_EARLYCON
+	help
+	  Driver for the UART controllers of the Espressif ESP32xx SoCs.
+	  When earlycon option is enabled the following kernel command line
+	  snippets may be used:
+	    earlycon=esp32s3uart,mmio32,0x60000000,115200n8,40000000
+	    earlycon=esp32uart,mmio32,0x3ff40000,115200n8
+
 endmenu
 
 config SERIAL_MCTRL_GPIO
drivers/tty/serial/Makefile +1 -0
index 138abbc89738..7b73137df7f3 100644
@@ -88,6 +88,7 @@ obj-$(CONFIG_SERIAL_MILBEAUT_USIO) += milbeaut_usio.o
 obj-$(CONFIG_SERIAL_SIFIVE)	+= sifive.o
 obj-$(CONFIG_SERIAL_LITEUART) += liteuart.o
 obj-$(CONFIG_SERIAL_SUNPLUS)	+= sunplus-uart.o
+obj-$(CONFIG_SERIAL_ESP32)	+= esp32_uart.o
 
 # GPIOLIB helpers for modem control lines
 obj-$(CONFIG_SERIAL_MCTRL_GPIO)	+= serial_mctrl_gpio.o
drivers/tty/serial/esp32_uart.c new file +741 -0

741 lines not on this page · raw message

include/uapi/linux/serial_core.h +3 -0
index add349889d0a..ff076d6be159 100644
@@ -245,4 +245,7 @@
 /* Sunplus UART */
 #define PORT_SUNPLUS	123
 
+/* Espressif ESP32 UART */
+#define PORT_ESP32UART	124
+
 #endif /* _UAPILINUX_SERIAL_CORE_H */
Signature · 1 line
2.30.2
#7 lore raw

Re: [PATCH v4 3/5] drivers/tty/serial: add driver for the ESP32 UART

Quoted 91 lines · Max Filippov

Add driver for the UART controllers of the Espressif ESP32 and ESP32S3 SoCs. Hardware specification is available at the following URLs:

https://www.espressif.com/sites/default/files/documentation/esp32_technical_reference_manual_en.pdf
(Chapter 13 UART Controller)
https://www.espressif.com/sites/default/files/documentation/esp32-s3_technical_reference_manual_en.pdf
(Chapter 26 UART Controller)

---
Changes v2->v3:
- rework brk handling in esp32_uart_rxint
- only increment port->icount.rx in case insert_flip_char() is called
- use HZ instead of msecs_to_jiffies(1000) in esp32_uart_put_char_sync
- add early return to esp32_uart_transmit_buffer
- use request_irq/free_irq instead of devm_* versions
- add blank lines before certain return statements

Changes v1->v2:
- redefine register fields using BIT and GENMASK
- drop _MASK suffix from register field definitions
- drop *_SHIFT definitions where possible
- drop unused rxfifo_full_thrhd_mask and txfifo_empty_thrhd_mask
- split register reads/writes and bitwise operations into multiple lines
- use u8 instead of unsigned char in internal functions
- add timeout to esp32_uart_put_char_sync
- use uart_port_tx_limited in esp32_uart_transmit_buffer
- use IRQ_RETVAL in esp32_uart_int
- disable clock in esp32_uart_startup in case devm_request_irq fails
- rearrange devm_request_irq with enabling IRQs in the UART registers
- drop empty esp32_uart_release_port and esp32_uart_request_port
- simplify esp32_uart_tx_empty
- mask out unsupported CMSPAR flag in termios->c_cflag in esp32_uart_set_termios
- invoke uart_update_timeout in esp32_uart_set_termios
- drop MODULE_DESCRIPTION
- rearrange esp32_uart_set_baud: return bool indicating whether baud rate was set or not, use it in the esp32_uart_set_termios to set the default 115200
- turn 'locked' into bool in esp32_uart_console_write
- turn num_read into unsigned int in esp32_uart_earlycon_read

 drivers/tty/serial/Kconfig       |  13 +
 drivers/tty/serial/Makefile      |   1 +
 drivers/tty/serial/esp32_uart.c  | 741 +++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |   3 +
 4 files changed, 758 insertions(+)
 create mode 100644 drivers/tty/serial/esp32_uart.c
drivers/tty/serial/Kconfig +13 -0
index bdc568a4ab66..d9ca6b268f01 100644
@@ -1578,6 +1578,19 @@ config SERIAL_NUVOTON_MA35D1_CONSOLE
 	  but you can alter that using a kernel command line option such as
 	  "console=ttyNVTx".
 
+config SERIAL_ESP32
+	tristate "Espressif ESP32 UART support"
+	depends on XTENSA_PLATFORM_ESP32 || (COMPILE_TEST && OF)
+	select SERIAL_CORE
+	select SERIAL_CORE_CONSOLE
+	select SERIAL_EARLYCON
+	help
+	  Driver for the UART controllers of the Espressif ESP32xx SoCs.
+	  When earlycon option is enabled the following kernel command line
+	  snippets may be used:
+	    earlycon=esp32s3uart,mmio32,0x60000000,115200n8,40000000
+	    earlycon=esp32uart,mmio32,0x3ff40000,115200n8
+
 endmenu
 
 config SERIAL_MCTRL_GPIO
drivers/tty/serial/Makefile +1 -0
index 138abbc89738..7b73137df7f3 100644
@@ -88,6 +88,7 @@ obj-$(CONFIG_SERIAL_MILBEAUT_USIO) += milbeaut_usio.o
 obj-$(CONFIG_SERIAL_SIFIVE)	+= sifive.o
 obj-$(CONFIG_SERIAL_LITEUART) += liteuart.o
 obj-$(CONFIG_SERIAL_SUNPLUS)	+= sunplus-uart.o
+obj-$(CONFIG_SERIAL_ESP32)	+= esp32_uart.o
 
 # GPIOLIB helpers for modem control lines
 obj-$(CONFIG_SERIAL_MCTRL_GPIO)	+= serial_mctrl_gpio.o
drivers/tty/serial/esp32_uart.c new file +0 -0
new file mode 100644
index 000000000000..bb5471e8b6b6

drivers/tty/serial/esp32_uart.c line 1

Quoted 1 line
drivers/tty/serial/esp32_uart.c new file +1 -0
@@ -0,0 +1,741 @@
+// SPDX-License-Identifier: GPL-2.0-or-later

Same license and copyright question as I had on the ACM patch.

thanks,

greg k-h

#8 lore raw

PATCH v4 4/5 dt-bindings: serial: document esp32s3-acm

In reply to Max Filippov

Add documentation for the ESP32S3 USB CDC-ACM controller.

---
Changes v2->v3:
- add a reference to serial.yaml

Changes v1->v2:
- fix description
- rename node in the example
- fix example indentation

 .../bindings/serial/esp,esp32-acm.yaml        | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/esp,esp32-acm.yaml
Documentation/devicetree/bindings/serial/esp,esp32-acm.yaml new file +42 -0
new file mode 100644
index 000000000000..e385ca9d9e2b
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serial/esp,esp32-acm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ESP32S3 ACM controller
+
+maintainers:
+  - Max Filippov <[email protected]>
+
+description:
+  Fixed function USB CDC-ACM device controller of the Espressif ESP32S3 SoC.
+
+allOf:
+  - $ref: serial.yaml#
+
+properties:
+  compatible:
+    const: esp,esp32s3-acm
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    serial@60038000 {
+      compatible = "esp,esp32s3-acm";
+      reg = <0x60038000 0x1000>;
+      interrupts = <96 3 0>;
+    };
Signature · 1 line
2.30.2
#9 lore raw

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

In reply to Max Filippov

Add driver for the ACM controller of the Espressif ESP32S3 Soc. 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)

---
Changes v2->v3:
- use HZ instead of msecs_to_jiffies(1000) in esp32_acm_put_char_sync
- add early return to esp32_acm_transmit_buffer
- use request_irq/free_irq instead of devm_* versions

Changes v1->v2:
- redefine register fields using BIT and GENMASK
- drop _MASK suffix from register field definitions
- drop *_SHIFT definitions where possible
- split register reads/writes and bitwise operations into multiple lines
- use u8 instead of unsigned char in internal functions
- add timeout to esp32_acm_put_char_sync
- use uart_port_tx_limited in esp32_acm_transmit_buffer
- use IRQ_RETVAL in esp32_acm_int
- drop esp32s3_acm_console_putchar and esp32s3_acm_earlycon_putchar
- turn num_read into unsigned int in esp32_acm_earlycon_read
- drop MODULE_DESCRIPTION

 drivers/tty/serial/Kconfig       |  14 +
 drivers/tty/serial/Makefile      |   1 +
 drivers/tty/serial/esp32_acm.c   | 459 +++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |   3 +
 4 files changed, 477 insertions(+)
 create mode 100644 drivers/tty/serial/esp32_acm.c
drivers/tty/serial/Kconfig +14 -0
index d9ca6b268f01..85807db8f7ce 100644
@@ -1591,6 +1591,20 @@ config SERIAL_ESP32
 	    earlycon=esp32s3uart,mmio32,0x60000000,115200n8,40000000
 	    earlycon=esp32uart,mmio32,0x3ff40000,115200n8
 
+config SERIAL_ESP32_ACM
+	tristate "Espressif ESP32 USB ACM support"
+	depends on XTENSA_PLATFORM_ESP32 || (COMPILE_TEST && OF)
+	select SERIAL_CORE
+	select SERIAL_CORE_CONSOLE
+	select SERIAL_EARLYCON
+	help
+	  Driver for the CDC ACM controllers of the Espressif ESP32S3 SoCs
+	  that share separate USB controller with the JTAG adapter.
+	  The device name used for this controller is ttyACM.
+	  When earlycon option is enabled the following kernel command line
+	  snippet may be used:
+	    earlycon=esp32s3acm,mmio32,0x60038000
+
 endmenu
 
 config SERIAL_MCTRL_GPIO
drivers/tty/serial/Makefile +1 -0
index 7b73137df7f3..970a292ca418 100644
@@ -89,6 +89,7 @@ obj-$(CONFIG_SERIAL_SIFIVE)	+= sifive.o
 obj-$(CONFIG_SERIAL_LITEUART) += liteuart.o
 obj-$(CONFIG_SERIAL_SUNPLUS)	+= sunplus-uart.o
 obj-$(CONFIG_SERIAL_ESP32)	+= esp32_uart.o
+obj-$(CONFIG_SERIAL_ESP32_ACM)	+= esp32_acm.o
 
 # GPIOLIB helpers for modem control lines
 obj-$(CONFIG_SERIAL_MCTRL_GPIO)	+= serial_mctrl_gpio.o
drivers/tty/serial/esp32_acm.c new file +459 -0

459 lines not on this page · raw message

include/uapi/linux/serial_core.h +3 -0
index ff076d6be159..1045bf096837 100644
@@ -248,4 +248,7 @@
 /* Espressif ESP32 UART */
 #define PORT_ESP32UART	124
 
+/* Espressif ESP32 ACM */
+#define PORT_ESP32ACM	125
+
 #endif /* _UAPILINUX_SERIAL_CORE_H */
Signature · 1 line
2.30.2
#10 lore raw

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

Quoted 1 line · Max Filippov

Add driver for the ACM controller of the Espressif ESP32S3 Soc.

Odd extra space :(

Quoted 4 lines

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?

Quoted 71 lines

---
Changes v2->v3:
- use HZ instead of msecs_to_jiffies(1000) in esp32_acm_put_char_sync
- add early return to esp32_acm_transmit_buffer
- use request_irq/free_irq instead of devm_* versions

Changes v1->v2:
- redefine register fields using BIT and GENMASK
- drop _MASK suffix from register field definitions
- drop *_SHIFT definitions where possible
- split register reads/writes and bitwise operations into multiple lines
- use u8 instead of unsigned char in internal functions
- add timeout to esp32_acm_put_char_sync
- use uart_port_tx_limited in esp32_acm_transmit_buffer
- use IRQ_RETVAL in esp32_acm_int
- drop esp32s3_acm_console_putchar and esp32s3_acm_earlycon_putchar
- turn num_read into unsigned int in esp32_acm_earlycon_read
- drop MODULE_DESCRIPTION

 drivers/tty/serial/Kconfig       |  14 +
 drivers/tty/serial/Makefile      |   1 +
 drivers/tty/serial/esp32_acm.c   | 459 +++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |   3 +
 4 files changed, 477 insertions(+)
 create mode 100644 drivers/tty/serial/esp32_acm.c
drivers/tty/serial/Kconfig +14 -0
index d9ca6b268f01..85807db8f7ce 100644
@@ -1591,6 +1591,20 @@ config SERIAL_ESP32
 	    earlycon=esp32s3uart,mmio32,0x60000000,115200n8,40000000
 	    earlycon=esp32uart,mmio32,0x3ff40000,115200n8
 
+config SERIAL_ESP32_ACM
+	tristate "Espressif ESP32 USB ACM support"
+	depends on XTENSA_PLATFORM_ESP32 || (COMPILE_TEST && OF)
+	select SERIAL_CORE
+	select SERIAL_CORE_CONSOLE
+	select SERIAL_EARLYCON
+	help
+	  Driver for the CDC ACM controllers of the Espressif ESP32S3 SoCs
+	  that share separate USB controller with the JTAG adapter.
+	  The device name used for this controller is ttyACM.
+	  When earlycon option is enabled the following kernel command line
+	  snippet may be used:
+	    earlycon=esp32s3acm,mmio32,0x60038000
+
 endmenu
 
 config SERIAL_MCTRL_GPIO
drivers/tty/serial/Makefile +1 -0
index 7b73137df7f3..970a292ca418 100644
@@ -89,6 +89,7 @@ obj-$(CONFIG_SERIAL_SIFIVE)	+= sifive.o
 obj-$(CONFIG_SERIAL_LITEUART) += liteuart.o
 obj-$(CONFIG_SERIAL_SUNPLUS)	+= sunplus-uart.o
 obj-$(CONFIG_SERIAL_ESP32)	+= esp32_uart.o
+obj-$(CONFIG_SERIAL_ESP32_ACM)	+= esp32_acm.o
 
 # GPIOLIB helpers for modem control lines
 obj-$(CONFIG_SERIAL_MCTRL_GPIO)	+= serial_mctrl_gpio.o
drivers/tty/serial/esp32_acm.c new file +0 -0
new file mode 100644
index 000000000000..f02abd2ac65e

drivers/tty/serial/esp32_acm.c line 1

Quoted 1 line
drivers/tty/serial/esp32_acm.c new file +1 -0
@@ -0,0 +1,459 @@
+// SPDX-License-Identifier: GPL-2.0-or-later

Why "or later"? I have to ask, sorry.

And no copyright information? That's fine, but be sure your company's lawyers are ok with it...

Quoted 7 lines
drivers/tty/serial/esp32_acm.c new file +7 -0
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/console.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/irq.h>

drivers/tty/serial/esp32_acm.c line 18

Quoted 10 lines
drivers/tty/serial/esp32_acm.c new file +10 -0
+#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?

Quoted 59 lines
drivers/tty/serial/esp32_acm.c new file +59 -0
+#define UART_NR		4
+
+#define ESP32S3_ACM_TX_FIFO_SIZE	64
+
+#define USB_SERIAL_JTAG_EP1_REG		0x00
+#define USB_SERIAL_JTAG_EP1_CONF_REG	0x04
+#define USB_SERIAL_JTAG_WR_DONE				BIT(0)
+#define USB_SERIAL_JTAG_SERIAL_IN_EP_DATA_FREE		BIT(1)
+#define USB_SERIAL_JTAG_INT_ST_REG	0x0c
+#define USB_SERIAL_JTAG_SERIAL_OUT_RECV_PKT_INT_ST	BIT(2)
+#define USB_SERIAL_JTAG_SERIAL_IN_EMPTY_INT_ST		BIT(3)
+#define USB_SERIAL_JTAG_INT_ENA_REG	0x10
+#define USB_SERIAL_JTAG_SERIAL_OUT_RECV_PKT_INT_ENA	BIT(2)
+#define USB_SERIAL_JTAG_SERIAL_IN_EMPTY_INT_ENA		BIT(3)
+#define USB_SERIAL_JTAG_INT_CLR_REG	0x14
+#define USB_SERIAL_JTAG_IN_EP1_ST_REG	0x2c
+#define USB_SERIAL_JTAG_IN_EP1_WR_ADDR			GENMASK(8, 2)
+#define USB_SERIAL_JTAG_OUT_EP1_ST_REG	0x3c
+#define USB_SERIAL_JTAG_OUT_EP1_REC_DATA_CNT		GENMASK(22, 16)
+
+static const struct of_device_id esp32s3_acm_dt_ids[] = {
+	{
+		.compatible = "esp,esp32s3-acm",
+	}, { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, esp32s3_acm_dt_ids);
+
+static struct uart_port *esp32s3_acm_ports[UART_NR];
+
+static void esp32s3_acm_write(struct uart_port *port, unsigned long reg, u32 v)
+{
+	writel(v, port->membase + reg);
+}
+
+static u32 esp32s3_acm_read(struct uart_port *port, unsigned long reg)
+{
+	return readl(port->membase + reg);
+}
+
+static u32 esp32s3_acm_tx_fifo_free(struct uart_port *port)
+{
+	u32 status = esp32s3_acm_read(port, USB_SERIAL_JTAG_EP1_CONF_REG);
+
+	return status & USB_SERIAL_JTAG_SERIAL_IN_EP_DATA_FREE;
+}
+
+static u32 esp32s3_acm_tx_fifo_cnt(struct uart_port *port)
+{
+	u32 status = esp32s3_acm_read(port, USB_SERIAL_JTAG_IN_EP1_ST_REG);
+
+	return FIELD_GET(USB_SERIAL_JTAG_IN_EP1_WR_ADDR, status);
+}
+
+static u32 esp32s3_acm_rx_fifo_cnt(struct uart_port *port)
+{
+	u32 status = esp32s3_acm_read(port, USB_SERIAL_JTAG_OUT_EP1_ST_REG);
+
+	return FIELD_GET(USB_SERIAL_JTAG_OUT_EP1_REC_DATA_CNT, status);
+}

drivers/tty/serial/esp32_acm.c line 87

Quoted 10 lines
drivers/tty/serial/esp32_acm.c new file +10 -0
+
+/* return TIOCSER_TEMT when transmitter is not busy */
+static unsigned int esp32s3_acm_tx_empty(struct uart_port *port)
+{
+	return esp32s3_acm_tx_fifo_cnt(port) == 0 ? TIOCSER_TEMT : 0;
+}
+
+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?

include/uapi/linux/serial_core.h line 252

Quoted 8 lines
include/uapi/linux/serial_core.h +2 -0
@@ -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.

thanks,

greg k-h

#11 lore raw

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

Quoted 5 lines · Greg Kroah-Hartman
Quoted 1 line · Max Filippov

Add driver for the ACM controller of the Espressif ESP32S3 Soc.

Odd extra space :(

Ok.

Quoted 7 lines
Quoted 4 lines

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.

Quoted 9 lines

drivers/tty/serial/esp32_acm.c line 1

Quoted 7 lines
drivers/tty/serial/esp32_acm.c new file +1 -0
new file mode 100644
index 000000000000..f02abd2ac65e
@@ -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?

Quoted 2 lines

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.

Quoted 13 lines
Quoted 13 lines
drivers/tty/serial/esp32_acm.c new file +13 -0
+
+#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>
Quoted 8 lines

drivers/tty/serial/esp32_acm.c line 18

Quoted 4 lines
drivers/tty/serial/esp32_acm.c new file +4 -0
+#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.

Quoted 5 lines

drivers/tty/serial/esp32_acm.c line 87

Quoted 3 lines
drivers/tty/serial/esp32_acm.c new file +3 -0
+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.

Quoted 11 lines

include/uapi/linux/serial_core.h line 252

Quoted 8 lines
include/uapi/linux/serial_core.h +2 -0
@@ -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.

Signature · 2 lines
Thanks.
-- Max
#12 lore raw

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

Quoted 11 lines · Max Filippov
Quoted 7 lines
Quoted 4 lines

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.

Quoted 10 lines
Quoted 9 lines

drivers/tty/serial/esp32_acm.c line 1

Quoted 7 lines
drivers/tty/serial/esp32_acm.c new file +1 -0
new file mode 100644
index 000000000000..f02abd2ac65e
@@ -0,0 +1,459 @@
+// SPDX-License-Identifier: GPL-2.0-or-later

Why "or later"? I have to ask, sorry.

Quoted 2 lines

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

Quoted 4 lines
Quoted 2 lines

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!

Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
drivers/tty/serial/esp32_acm.c new file +13 -0
+
+#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>
Quoted 8 lines

drivers/tty/serial/esp32_acm.c line 18

Quoted 4 lines
drivers/tty/serial/esp32_acm.c new file +4 -0
+#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?

Quoted 2 lines

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?

Quoted 7 lines
Quoted 5 lines

drivers/tty/serial/esp32_acm.c line 87

Quoted 3 lines
drivers/tty/serial/esp32_acm.c new file +3 -0
+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.

Quoted 12 lines
Quoted 11 lines

include/uapi/linux/serial_core.h line 252

Quoted 8 lines
include/uapi/linux/serial_core.h +2 -0
@@ -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.

Quoted 4 lines

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

#13 lore raw

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

Quoted 6 lines · Greg Kroah-Hartman
Quoted 5 lines · Max Filippov
Quoted 5 lines
Quoted 4 lines

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)

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 10 lines
Quoted 10 lines
Quoted 9 lines

drivers/tty/serial/esp32_acm.c line 1

Quoted 7 lines
drivers/tty/serial/esp32_acm.c new file +1 -0
new file mode 100644
index 000000000000..f02abd2ac65e
@@ -0,0 +1,459 @@
+// SPDX-License-Identifier: GPL-2.0-or-later

Why "or later"? I have to ask, sorry.

Quoted 5 lines
Quoted 2 lines

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?

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

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.

Quoted 11 lines
Quoted 8 lines
Quoted 5 lines

drivers/tty/serial/esp32_acm.c line 18

Quoted 1 line
drivers/tty/serial/esp32_acm.c new file +1 -0
+#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.

Quoted 17 lines
Quoted 12 lines
Quoted 11 lines

include/uapi/linux/serial_core.h line 252

Quoted 8 lines
include/uapi/linux/serial_core.h +2 -0
@@ -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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Signature · 2 lines
Thanks.
-- Max
#14 lore raw

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

Quoted 8 lines · Max Filippov

On Thu, Oct 5, 2023 at 11:57 AM Greg Kroah-Hartman

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines

drivers/tty/serial/esp32_acm.c line 1

Quoted 4 lines
drivers/tty/serial/esp32_acm.c new file +1 -0
@@ -0,0 +1,459 @@
+// SPDX-License-Identifier: GPL-2.0-or-later

Why "or later"? I have to ask, sorry.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

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?

If you only want your code being used in Linux, then stick with "GPL-2.0". If you want your code to be able to be copied into other GPLv3 licensed code bodies (like Hurd and others), then license it as "GPL-2.0-or-later". Your call.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

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.

You already own the copyright by virtue of creating the code (you can't give it away), so might as well put your mark on it to make it more noticable. But it's not required, if you don't want to, no one can force you, again, your call.

Quoted 22 lines
Quoted 17 lines
Quoted 12 lines
Quoted 11 lines

include/uapi/linux/serial_core.h line 252

Quoted 8 lines
include/uapi/linux/serial_core.h +2 -0
@@ -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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 5 lines

Ok, it looks like having

#define PORT_ESP32ACM (-1)

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

Hah, I like that hack. But why not just use PORT_UNKNOWN?

thanks,

greg k-h

#15 lore raw

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

Quoted 4 lines · Greg Kroah-Hartman

If you only want your code being used in Linux, then stick with "GPL-2.0". If you want your code to be able to be copied into other GPLv3 licensed code bodies (like Hurd and others), then license it as "GPL-2.0-or-later". Your call.

Thanks. From that point of view I prefer GPL-2.0-or-later.

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines

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.

Quoted 7 lines
Quoted 5 lines

Ok, it looks like having

#define PORT_ESP32ACM (-1)

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

Hah, I like that hack. But why not just use PORT_UNKNOWN?

A lot of functionality doesn't work with PORT_UNKNOWN, e.g. console or modem control.
I've got the idea of using -1 from this email:
https://lore.kernel.org/linux-serial/[email protected]/

Signature · 2 lines
Thanks.
-- Max
#16 lore raw

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

Quoted 10 lines · Max Filippov

On Fri, Oct 6, 2023 at 2:34 AM Greg Kroah-Hartman

Quoted 9 lines
Quoted 9 lines
Quoted 4 lines

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)

Quoted 8 lines
Quoted 3 lines
Quoted 1 line

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

Hah, I like that hack. But why not just use PORT_UNKNOWN?

A lot of functionality doesn't work with PORT_UNKNOWN, e.g. console or modem control.
I've got the idea of using -1 from this email:
https://lore.kernel.org/linux-serial/[email protected]/

Ok, we should encode this as a "real" number, "PORT_ANY" and set it to -1 and let all new devices use it.

thanks,

greg k-h

#17 lore raw

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

In reply to Max Filippov
Quoted 33 lines · Max Filippov
Quoted 5 lines · Greg Kroah-Hartman
Quoted 1 line · Max Filippov

Add driver for the ACM controller of the Espressif ESP32S3 Soc.

Odd extra space :(

Ok.

Quoted 7 lines
Quoted 4 lines

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.

Quoted 9 lines

drivers/tty/serial/esp32_acm.c line 1

Quoted 7 lines
drivers/tty/serial/esp32_acm.c new file +1 -0
new file mode 100644
index 000000000000..f02abd2ac65e
@@ -0,0 +1,459 @@
+// SPDX-License-Identifier: GPL-2.0-or-later

Why "or later"? I have to ask, sorry.

Quoted 2 lines

I don't really have a preference here. Is there a reason to choose GPL-2.0 only for a new code?

Yes, that's the default license for the kernel. So you should have a reason for picking something else. Like maybe you copied all this from somewhere else and that was the license.

Rob