lkml.org 
[lkml]   [2022]   [Dec]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] hid-ft260: add UART support.
On Sat, Oct 22, 2022 at 11:19:20AM +1300, Daniel Beer wrote:
> Based on an earlier patch submitted by Christina Quast:
>
> https://patches.linaro.org/project/linux-serial/patch/20220928192421.11908-1-contact@christina-quast.de/

Please link to lore.kernel.org, we have no idea what will happen over
time to other domains/links.

> Simplified and reworked to use the UART API rather than the TTY layer
> directly. Transmit, receive and baud rate changes are supported.

Why use the uart layer? Did you just change how the existing driver
works?

> +struct ft260_input_report {
> + u8 report; /* FT260_I2C_REPORT or FT260_UART_REPORT */
> u8 length; /* data payload length */
> - u8 data[2]; /* data payload */
> + u8 data[0]; /* data payload */

Please do not use [0], use [], people are working to replace all [0]
instances in the kernel.

> +struct ft260_configure_uart_request {
> + u8 report; /* FT260_SYSTEM_SETTINGS */
> + u8 request; /* FT260_SET_UART_CONFIG */
> + u8 flow_ctrl; /* 0: OFF, 1: RTS_CTS, 2: DTR_DSR */
> + /* 3: XON_XOFF, 4: No flow ctrl */
> + __le32 baudrate; /* little endian, 9600 = 0x2580, 19200 = 0x4B00 */

The data structure in the device really looks like this? Unaligned
accesses are odd.

> +static void ft260_uart_set_termios(struct uart_port *port,
> + struct ktermios *termios,
> + const struct ktermios *old_termios)
> +{
> + struct ft260_device *dev = container_of(port, struct ft260_device, port);
> + struct hid_device *hdev = dev->hdev;
> + unsigned int baud;
> + struct ft260_configure_uart_request req;
> + int ret;
> +
> + ft260_dbg("%s uart\n", __func__);

Please just use ftrace, no need for any of these "I am here!" lines.

Also dev_dbg() functions already have __func__ in them, no need to ever
add them again.

> --- a/include/uapi/linux/major.h
> +++ b/include/uapi/linux/major.h
> @@ -175,4 +175,6 @@
> #define BLOCK_EXT_MAJOR 259
> #define SCSI_OSD_MAJOR 260 /* open-osd's OSD scsi device */
>
> +#define FT260_MAJOR 261

A whole new major for just a single tty port? Please no, use dynamic
majors if you have to, or better yet, tie into the usb-serial
implementation (this is a USB device, right?) and then you don't have to
mess with this at all.

> +
> #endif
> diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> index 3ba34d8378bd..d9a7025f467e 100644
> --- a/include/uapi/linux/serial_core.h
> +++ b/include/uapi/linux/serial_core.h
> @@ -276,4 +276,7 @@
> /* Sunplus UART */
> #define PORT_SUNPLUS 123
>
> +/* FT260 HID UART */
> +#define PORT_FT260 124

Why is this required? What userspace code needs this new id? I want to
remove all of these ids, not add new ones.

thanks,

greg k-h

\
 
 \ /
  Last update: 2022-12-04 09:19    [W:0.072 / U:0.660 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site