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@xxxxxx>
> ---
> 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?
> + 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 :(
thanks,
greg k-h
