Messages in this thread | | | Date | Mon, 13 Mar 2023 09:49:26 +0200 | Subject | Re: [PATCH] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls | From | Jarkko Sonninen <> |
| |
On 3/13/23 09:00, Greg Kroah-Hartman wrote: > Nit, your "To:" line was corrupted :( > > On Mon, Mar 13, 2023 at 03:04:16AM +0200, Jarkko Sonninen wrote: >> Add support for RS-485 in Exar USB adapters. >> RS-485 mode is controlled by TIOCGRS485 and TIOCSRS485 ioctls. >> Gpio mode register is set to enable RS-485. >> >> Signed-off-by: Jarkko Sonninen <kasper@iki.fi> >> --- >> drivers/usb/serial/xr_serial.c | 72 +++++++++++++++++++++++++++++++++- >> 1 file changed, 71 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c >> index fdb0aae546c3..edbb6add087c 100644 >> --- a/drivers/usb/serial/xr_serial.c >> +++ b/drivers/usb/serial/xr_serial.c >> @@ -93,6 +93,7 @@ struct xr_txrx_clk_mask { >> #define XR_GPIO_MODE_SEL_DTR_DSR 0x2 >> #define XR_GPIO_MODE_SEL_RS485 0x3 >> #define XR_GPIO_MODE_SEL_RS485_ADDR 0x4 >> +#define XR_GPIO_MODE_RS485_TX_H 0x8 >> #define XR_GPIO_MODE_TX_TOGGLE 0x100 >> #define XR_GPIO_MODE_RX_TOGGLE 0x200 >> >> @@ -237,6 +238,8 @@ static const struct xr_type xr_types[] = { >> struct xr_data { >> const struct xr_type *type; >> u8 channel; /* zero-based index or interface number */ >> + struct serial_rs485 rs485; >> + spinlock_t lock; >> }; >> >> static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val) >> @@ -629,6 +632,7 @@ static void xr_set_flow_mode(struct tty_struct *tty, >> struct xr_data *data = usb_get_serial_port_data(port); >> const struct xr_type *type = data->type; >> u16 flow, gpio_mode; >> + unsigned long flags, rs485_flags; >> int ret; >> >> ret = xr_get_reg_uart(port, type->gpio_mode, &gpio_mode); >> @@ -645,9 +649,16 @@ static void xr_set_flow_mode(struct tty_struct *tty, >> /* Set GPIO mode for controlling the pins manually by default. */ >> gpio_mode &= ~XR_GPIO_MODE_SEL_MASK; >> >> + spin_lock_irqsave(&data->lock, flags); >> + rs485_flags = data->rs485.flags; >> + spin_unlock_irqrestore(&data->lock, flags); >> + if (rs485_flags & SER_RS485_ENABLED) >> + gpio_mode |= XR_GPIO_MODE_SEL_RS485 | XR_GPIO_MODE_RS485_TX_H; >> + else if (C_CRTSCTS(tty) && C_BAUD(tty) != B0) >> + gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS; >> + >> if (C_CRTSCTS(tty) && C_BAUD(tty) != B0) { >> dev_dbg(&port->dev, "Enabling hardware flow ctrl\n"); >> - gpio_mode |= XR_GPIO_MODE_SEL_RTS_CTS; >> flow = XR_UART_FLOW_MODE_HW; >> } else if (I_IXON(tty)) { >> u8 start_char = START_CHAR(tty); >> @@ -827,6 +838,64 @@ static void xr_set_termios(struct tty_struct *tty, >> xr_set_flow_mode(tty, port, old_termios); >> } >> >> +static int xr_get_rs485_config(struct tty_struct *tty, >> + unsigned int __user *argp) >> +{ >> + struct usb_serial_port *port = tty->driver_data; >> + struct xr_data *data = usb_get_serial_port_data(port); >> + unsigned long flags; >> + struct serial_rs485 rs485; >> + >> + spin_lock_irqsave(&data->lock, flags); >> + memcpy(&rs485, &data->rs485, sizeof(rs485)); > Why are you using the stack for this? Why not directly access the real > structure instead? And are you sure you need a lock? If so, why?
I suppose the lock is not really needed and I'll change this. I was unsure about it.
>> + spin_unlock_irqrestore(&data->lock, flags); >> + dev_dbg(tty->dev, "%s flags %02x\n", __func__, rs485.flags); > dev_dbg() provides __func__ for free, so you never need to provide it > again. Just use the +f flag when enabling it from userspace. > >> + >> + if (copy_to_user(argp, &rs485, sizeof(rs485))) >> + return -EFAULT; >> + >> + return 0; >> +} >> + >> +static int xr_set_rs485_config(struct tty_struct *tty, >> + unsigned long __user *argp) >> +{ >> + struct usb_serial_port *port = tty->driver_data; >> + struct xr_data *data = usb_get_serial_port_data(port); >> + struct serial_rs485 rs485; >> + unsigned long flags; >> + >> + if (copy_from_user(&rs485, argp, sizeof(rs485))) >> + return -EFAULT; >> + >> + dev_dbg(tty->dev, "%s flags %02x\n", __func__, rs485.flags); > Again, no need for __func__. > >> + rs485.flags &= SER_RS485_ENABLED; >> + spin_lock_irqsave(&data->lock, flags); >> + memcpy(&data->rs485, &rs485, sizeof(rs485)); >> + spin_unlock_irqrestore(&data->lock, flags); >> + xr_set_flow_mode(tty, port, 0); >> + >> + if (copy_to_user(argp, &data->rs485, sizeof(data->rs485))) >> + return -EFAULT; >> + >> + return 0; >> +} >> + >> + >> +static int xr_ioctl(struct tty_struct *tty, >> + unsigned int cmd, unsigned long arg) > Very odd indentation, please fix up. > >> +{ >> + void __user *argp = (void __user *)arg; >> + >> + switch (cmd) { >> + case TIOCGRS485: >> + return xr_get_rs485_config(tty, argp); >> + case TIOCSRS485: >> + return xr_set_rs485_config(tty, argp); >> + } >> + return -ENOIOCTLCMD; > Wrong ioctl return value :(
What is the correct ioctl error return value ? ENOIOCTLCMD was used in most places in usb serial as an error return.
> thanks, > > greg k-h
- Jarkko
| |