Messages in this thread | | | Date | Mon, 24 Oct 2022 11:40:16 +0200 | Subject | Re: [PATCH v2 1/2] usb: serial: add support for CH348 | From | Oliver Neukum <> |
| |
On 21.10.22 15:37, Corentin Labbe wrote: > The CH348 is an USB octo port serial adapter. > This patch adds support for it. >
Hi,
comments inline.
> +struct ch348_rxbuf { > + u8 port; > + u8 length; > + u8 data[]; > +} __packed; > + > +#define CH348_RX_PORT_CHUNK_LENGTH 32 > +#define CH348_RX_PORT_MAX_LENGTH 30 > + > +struct ch348_txbuf { > + u8 port; > + __le16 length; > + u8 data[]; > +} __packed; > +
You know how long data will be. Why leave it unspecified?
> +static void ch348_process_read_urb(struct urb *urb) > +{ > + struct usb_serial_port *port = urb->context; > + struct ch348 *ch348 = usb_get_serial_data(port->serial); > + u8 *buffer = urb->transfer_buffer, *end; > + unsigned int portnum, usblen; > + struct ch348_rxbuf *rxb; > + > + if (!urb->actual_length) {
That check needs to be for < 2 or you can process garbage.
> + dev_warn(&port->dev, "%s:%d empty rx buffer\n", __func__, __LINE__); > + return; > + } > + > + end = buffer + urb->actual_length; > + > + for (; buffer < end; buffer += CH348_RX_PORT_CHUNK_LENGTH) { > + rxb = (struct ch348_rxbuf *)buffer; > + portnum = rxb->port; > + if (portnum >= CH348_MAXPORT) { > + dev_warn(&port->dev, "%s:%d invalid port %d\n", > + __func__, __LINE__, portnum); > + break; > + } > + > + usblen = rxb->length; > + if (usblen > 30) {
You have defined a nummerical constant for that. Use it.
> + dev_warn(&port->dev, "%s:%d invalid length %d for port %d\n", > + __func__, __LINE__, usblen, portnum); > + break; > + } > + > + port = ch348->ttyport[portnum].port; > + tty_insert_flip_string(&port->port, rxb->data, usblen); > + tty_flip_buffer_push(&port->port); > + port->icount.rx += usblen; > + usb_serial_debug_data(&port->dev, __func__, usblen, rxb->data); > + } > +} > + > +static int ch348_prepare_write_buffer(struct usb_serial_port *port, void *dest, size_t size) > +{ > + struct ch348_txbuf *rxt = dest; > + const size_t txhdrsize = offsetof(struct ch348_txbuf, data);
What is that? This is a constant.
> + int count; > + > + count = kfifo_out_locked(&port->write_fifo, rxt->data, > + size - txhdrsize, &port->lock); > + > + rxt->port = port->port_number; > + rxt->length = cpu_to_le16(count); > + > + return count + txhdrsize; > +}
> +static void ch348_set_termios(struct tty_struct *tty, struct usb_serial_port *port, > + const struct ktermios *termios_old) > +{ > + struct ch348 *ch348 = usb_get_serial_data(port->serial); > + int portnum = port->port_number; > + struct ktermios *termios = &tty->termios; > + int ret, sent; > + __le32 dwDTERate;
OK it is LE.
> + u8 bCharFormat; > + struct ch348_initbuf *buffer; > + > + if (termios_old && !tty_termios_hw_change(&tty->termios, termios_old)) > + return; > + > + buffer = kzalloc(sizeof(*buffer), GFP_KERNEL); > + if (!buffer) { > + if (termios_old) > + tty->termios = *termios_old; > + return; > + } > + > + dwDTERate = tty_get_baud_rate(tty);
This is speed_t, in other words unsigned int without specified endianness.
> + /* test show no success on low baud and datasheet said it is not supported */ > + if (dwDTERate < 1200) > + dwDTERate = DEFAULT_BAUD_RATE; > + /* datasheet said it is not supported */ > + if (dwDTERate > 6000000) > + dwDTERate = 6000000;
You are comparing to constants in native endianness.
> + > + bCharFormat = termios->c_cflag & CSTOPB ? 2 : 1; > + > + buffer->bParityType = termios->c_cflag & PARENB ? > + (termios->c_cflag & PARODD ? 1 : 2) + > + (termios->c_cflag & CMSPAR ? 2 : 0) : 0; > + > + switch (termios->c_cflag & CSIZE) { > + case CS5: > + buffer->bDataBits = 5; > + break; > + case CS6: > + buffer->bDataBits = 6; > + break; > + case CS7: > + buffer->bDataBits = 7; > + break; > + case CS8: > + default: > + buffer->bDataBits = 8; > + break; > + } > + buffer->cmd = CMD_WB_E | (portnum & 0x0F); > + buffer->reg = R_INIT; > + buffer->port = portnum; > + buffer->dwDTERate = cpu_to_be32(le32_to_cpu(dwDTERate));
So it is native, not LE?
[..]
> + > +static int ch348_fixup_port_bulk_in(struct ch348 *ch348, struct usb_serial_port *port) > +{ > + int i; > + > + /* Already Initialized */ > + if (port->bulk_in_size) {
BTW, shouldn't these be unsigned int? [..]
Regards Oliver
| |