lkml.org 
[lkml]   [2022]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 1/2] usb: serial: add support for CH348
From


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

\
 
 \ /
  Last update: 2022-10-24 11:41    [W:1.741 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site