On Thu, Sep 14, 2023 at 12:16 AM Jiri Slaby <jirislaby@xxxxxxxxxx> wrote: > > On 13. 09. 23, 23:14, Max Filippov wrote: > > 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) > ... > > > +static void esp32s3_acm_put_char_sync(struct uart_port *port, unsigned char c) > > +{ > > + while (!esp32s3_acm_tx_fifo_free(port)) > > + cpu_relax(); > > No limits... Fixed. > > + esp32s3_acm_put_char(port, c); > > + esp32s3_acm_push(port); > > +} > > + > > +static void esp32s3_acm_transmit_buffer(struct uart_port *port) > > +{ > > tx helper. Ok. > > + struct circ_buf *xmit = &port->state->xmit; > > + u32 tx_fifo_used = esp32s3_acm_tx_fifo_cnt(port); > > + > > + if (esp32s3_acm_tx_fifo_free(port)) { > > + while (!uart_circ_empty(xmit) && tx_fifo_used < ESP32S3_ACM_TX_FIFO_SIZE) { > > + esp32s3_acm_put_char(port, xmit->buf[xmit->tail]); > > + xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > > + port->icount.tx++; > > + ++tx_fifo_used; > > + } > > + } > > + > > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > > + uart_write_wakeup(port); > > + > > + if (uart_circ_empty(xmit)) { > > + esp32s3_acm_stop_tx(port); > > + } else { > > + u32 int_ena; > > + > > + int_ena = esp32s3_acm_read(port, USB_SERIAL_JTAG_INT_ENA_REG); > > + esp32s3_acm_write(port, USB_SERIAL_JTAG_INT_ENA_REG, > > + int_ena | USB_SERIAL_JTAG_SERIAL_IN_EMPTY_INT_ENA_MASK); > > + } > > + > > + if (tx_fifo_used > 0 && tx_fifo_used < ESP32S3_ACM_TX_FIFO_SIZE) > > + esp32s3_acm_write(port, USB_SERIAL_JTAG_EP1_CONF_REG, > > + USB_SERIAL_JTAG_WR_DONE_MASK); > > +} > > > > +static irqreturn_t esp32s3_acm_int(int irq, void *dev_id) > > +{ > > + struct uart_port *port = dev_id; > > + u32 status; > > + > > + status = esp32s3_acm_read(port, USB_SERIAL_JTAG_INT_ST_REG); > > + esp32s3_acm_write(port, USB_SERIAL_JTAG_INT_CLR_REG, status); > > + > > + if (status & USB_SERIAL_JTAG_SERIAL_OUT_RECV_PKT_INT_ST_MASK) > > + esp32s3_acm_rxint(port); > > + if (status & USB_SERIAL_JTAG_SERIAL_IN_EMPTY_INT_ST_MASK) > > + esp32s3_acm_txint(port); > > + > > + return IRQ_HANDLED; > > IRQ_STATUS() Ok. > > +} > > > +static int esp32s3_acm_startup(struct uart_port *port) > > +{ > > + int ret = 0; > > + > > + esp32s3_acm_write(port, USB_SERIAL_JTAG_INT_ENA_REG, > > + USB_SERIAL_JTAG_SERIAL_OUT_RECV_PKT_INT_ENA_MASK); > > + ret = devm_request_irq(port->dev, port->irq, esp32s3_acm_int, 0, > > + DRIVER_NAME, port); > > + return ret; > > No need for ret. Or not, you don't handle the failure properly again > (disable ints). And the order appears to be switched too. Fixed. > > +static void > > +esp32s3_acm_console_write(struct console *co, const char *s, unsigned int count) > > +{ > > + struct uart_port *port = esp32s3_acm_ports[co->index]; > > + unsigned long flags; > > + int locked = 1; > > bool? ANd in the otrher driver too. Ok. > > + > > + if (port->sysrq) > > + locked = 0; > > + else if (oops_in_progress) > > + locked = spin_trylock_irqsave(&port->lock, flags); > > + else > > + spin_lock_irqsave(&port->lock, flags); > > + > > + esp32s3_acm_string_write(port, s, count); > > + > > + if (locked) > > + spin_unlock_irqrestore(&port->lock, flags); > > +} > > > > +#ifdef CONFIG_CONSOLE_POLL > > +static int esp32s3_acm_earlycon_read(struct console *con, char *s, unsigned int n) > > +{ > > + struct earlycon_device *dev = con->data; > > + int num_read = 0; > > num looks like should be unsigned? Ok. > > + > > + while (num_read < n) { > > + int c = esp32s3_acm_poll_get_char(&dev->port); > > + > > + if (c == NO_POLL_CHAR) > > + break; > > + s[num_read++] = c; > > + } > > + return num_read; > > +} > > +#endif > > > > +static int esp32s3_acm_probe(struct platform_device *pdev) > > +{ > > + struct device_node *np = pdev->dev.of_node; > > + struct uart_port *port; > > + struct resource *res; > > + int ret; > > + > > + port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL); > > + if (!port) > > + return -ENOMEM; > > + > > + ret = of_alias_get_id(np, "serial"); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "failed to get alias id, errno %d\n", ret); > > + return ret; > > + } > > + if (ret >= UART_NR) { > > + dev_err(&pdev->dev, "driver limited to %d serial ports\n", > > + UART_NR); > > + return -ENOMEM; > > + } > > + > > + port->line = ret; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!res) > > + return -ENODEV; > > + > > + port->mapbase = res->start; > > + port->membase = devm_ioremap_resource(&pdev->dev, res); > > + if (IS_ERR(port->membase)) > > + return PTR_ERR(port->membase); > > + > > + port->dev = &pdev->dev; > > + port->type = PORT_ESP32ACM; > > + port->iotype = UPIO_MEM; > > + port->irq = platform_get_irq(pdev, 0); > > + port->ops = &esp32s3_acm_pops; > > + port->flags = UPF_BOOT_AUTOCONF; > > + port->has_sysrq = 1; > > + port->fifosize = ESP32S3_ACM_TX_FIFO_SIZE; > > + > > + esp32s3_acm_ports[port->line] = port; > > + > > + platform_set_drvdata(pdev, port); > > + > > + ret = uart_add_one_port(&esp32s3_acm_reg, port); > > + return ret; > > return imm. Ok. -- Thanks. -- Max
