| Message ID | 20230314070002.1008959-1-kasper@iki.fi (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] USB: serial: xr: Add TIOCGRS485 and TIOCSRS485 ioctls | expand |
On Tue, Mar 14, 2023 at 09:00:01AM +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> > --- > > In this version only rs485.flags are stored to state. > There is no locking as only one bit of the flags is used. > ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code. And the difference between previous versions? Take a look at the documentation for how to better describe version differences please. > > drivers/usb/serial/xr_serial.c | 62 +++++++++++++++++++++++++++++++++- > 1 file changed, 61 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c > index fdb0aae546c3..7b542ccb6596 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,7 @@ static const struct xr_type xr_types[] = { > struct xr_data { > const struct xr_type *type; > u8 channel; /* zero-based index or interface number */ > + u32 rs485_flags; Nit, you might want to move this up above channel as you now have a hole in this structure. Not like it's that big of a deal so if you don't have to respin this no need to change. > }; > > static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val) > @@ -645,9 +647,13 @@ 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; > > + if (data->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 +833,59 @@ 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); > + struct serial_rs485 rs485; > + > + dev_dbg(tty->dev, "Flags %02x\n", data->rs485_flags); > + memset(&rs485, 0, sizeof(rs485)); > + rs485.flags = data->rs485_flags; > + 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; > + > + if (copy_from_user(&rs485, argp, sizeof(rs485))) > + return -EFAULT; > + > + dev_dbg(tty->dev, "Flags %02x\n", rs485.flags); > + data->rs485_flags = rs485.flags & SER_RS485_ENABLED; > + xr_set_flow_mode(tty, port, (const struct ktermios *)0); > + > + // Only the enable flag is implemented All the other comments in this file use /* */ so you should be consistent. > + memset(&rs485, 0, sizeof(rs485)); But you just copied this from userspace, why zero it out again? Is that to be expected (I really don't remember, sorry). Anyway, just minor comments, I'll let others review it as well. thanks, greg k-h
On 3/14/23 09:37, Greg Kroah-Hartman wrote: > On Tue, Mar 14, 2023 at 09:00:01AM +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> >> --- >> >> In this version only rs485.flags are stored to state. >> There is no locking as only one bit of the flags is used. >> ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code. > And the difference between previous versions? Take a look at the > documentation for how to better describe version differences please. I'll try to be more precise in the future. >> + memset(&rs485, 0, sizeof(rs485)); > But you just copied this from userspace, why zero it out again? Is that > to be expected (I really don't remember, sorry). serial.h says /* * Serial interface for controlling RS485 settings on chips with suitable * support. Set with TIOCSRS485 and get with TIOCGRS485 if supported by your * platform. The set function returns the new state, with any unsupported bits * reverted appropriately. */ This may mean only the flags, but I decided to zero the other unsupported fields as well. > Anyway, just minor comments, I'll let others review it as well. > > thanks, > > greg k-h - Jarkko
On Tue, Mar 14, 2023 at 09:00:01AM +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. Which register you use is an implementation details which is not really needed in the commit message. Please say something about how the hardware works and try to describe what you are implementing here and perhaps something about what is left unsupported (e.g. the fixed rts polarity). > Signed-off-by: Jarkko Sonninen <kasper@iki.fi> > --- > > In this version only rs485.flags are stored to state. > There is no locking as only one bit of the flags is used. > ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code. > > drivers/usb/serial/xr_serial.c | 62 +++++++++++++++++++++++++++++++++- > 1 file changed, 61 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c > index fdb0aae546c3..7b542ccb6596 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,7 @@ static const struct xr_type xr_types[] = { > struct xr_data { > const struct xr_type *type; > u8 channel; /* zero-based index or interface number */ > + u32 rs485_flags; > }; > > static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val) > @@ -645,9 +647,13 @@ 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; > > + if (data->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; The logic here is unnecessarily convoluted here and you also should not set hardware flow control mode if rs485 mode is enabled. Perhaps you can add a local boolean flag to hold the rs485 state and test it before the current if-else construct. Then you only enable hw-flow when rs485 mode is disabled while stile allowing sw-flow to be set (hopefully that's a legal combination, please do try to verify that). It also looks like you have inverted the RS485 polarity by using XR_GPIO_MODE_RS485_TX_H (more on that below). > } else if (I_IXON(tty)) { > u8 start_char = START_CHAR(tty); > @@ -827,6 +833,59 @@ 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) argp points to struct serial_rs485 to use that as the type rather than pointer to unsigned int. > +{ > + struct usb_serial_port *port = tty->driver_data; > + struct xr_data *data = usb_get_serial_port_data(port); > + struct serial_rs485 rs485; > + > + dev_dbg(tty->dev, "Flags %02x\n", data->rs485_flags); This is not a very informative message. Please add back the function prefix so that is also distinguishable from the dev_dbg() in the other rs485 helper and use the following format: "%s - flags = 0x%02x\n", __func__, ... And add a new line here. > + memset(&rs485, 0, sizeof(rs485)); > + rs485.flags = data->rs485_flags; > + 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) Use a pointer to the struct here too. > +{ > + struct usb_serial_port *port = tty->driver_data; > + struct xr_data *data = usb_get_serial_port_data(port); > + struct serial_rs485 rs485; > + > + if (copy_from_user(&rs485, argp, sizeof(rs485))) > + return -EFAULT; > + > + dev_dbg(tty->dev, "Flags %02x\n", rs485.flags); Please update the format string as mentioned above. Add a newline here. > + data->rs485_flags = rs485.flags & SER_RS485_ENABLED; > + xr_set_flow_mode(tty, port, (const struct ktermios *)0); This function accesses tty->termios so you can not call it here without any locking as it can change underneath you and nothing currently prevents set_termios() from calling the same function in parallel. If you take a write lock on the termios rw sempahore you can use it also to protect the rs485 data instead of relying on implicit atomicity rules. And perhaps you should just copy the entire rs485 struct from the start as these devices supports further features which someone may want to implement support for later (e.g. delay after send and 9th bit addressing). You should just use NULL for the third (old_termios) argument. > + > + // Only the enable flag is implemented No c99 comments, please. > + memset(&rs485, 0, sizeof(rs485)); > + rs485.flags = data->rs485_flags; This does not look correct given that you set the RS485 TX polarity so that RTS is high (logic disable) during TX above. You need to at least make sure that both the SER_RS485_RTS_ON_SEND and SER_RS485_RTS_AFTER_SEND bits match the polarity setting. But perhaps you could consider implementing support for configuring the polarity from the start. > + if (copy_to_user(argp, &rs485, sizeof(rs485))) > + return -EFAULT; > + > + return 0; > +} > + > + > +static int xr_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg) > +{ > + 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; > +} > + > static int xr_open(struct tty_struct *tty, struct usb_serial_port *port) > { > int ret; > @@ -1010,6 +1069,7 @@ static struct usb_serial_driver xr_device = { > .set_termios = xr_set_termios, > .tiocmget = xr_tiocmget, > .tiocmset = xr_tiocmset, > + .ioctl = xr_ioctl, > .dtr_rts = xr_dtr_rts > }; Johan
On Tue, Mar 14, 2023 at 08:37:30AM +0100, Greg Kroah-Hartman wrote: > On Tue, Mar 14, 2023 at 09:00:01AM +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> > > --- > > @@ -237,6 +238,7 @@ static const struct xr_type xr_types[] = { > > struct xr_data { > > const struct xr_type *type; > > u8 channel; /* zero-based index or interface number */ > > + u32 rs485_flags; > > Nit, you might want to move this up above channel as you now have a hole > in this structure. Not like it's that big of a deal so if you don't > have to respin this no need to change. Generally, it's better to keep related fields together than worry about holes so the above is just fine. Johan
On 4/13/23 11:53, Johan Hovold wrote: > On Tue, Mar 14, 2023 at 09:00:01AM +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. > Which register you use is an implementation details which is not really > needed in the commit message. > > Please say something about how the hardware works and try to describe > what you are implementing here and perhaps something about what is left > unsupported (e.g. the fixed rts polarity). Add support for RS-485 in Exar USB adapters. RS-485 mode is controlled by TIOCGRS485 and TIOCSRS485 ioctls. SER_RS485_ENABLED and SER_RS485_RTS_ON_SEND flags are implemented. There is only one polarity control in Exar for both RTS_ON_SEND and RTS_AFTER_SEND. RS-485 delays and addressing modes are not implemented. ok ? >> Signed-off-by: Jarkko Sonninen <kasper@iki.fi> >> --- >> >> In this version only rs485.flags are stored to state. >> There is no locking as only one bit of the flags is used. >> ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code. >> >> drivers/usb/serial/xr_serial.c | 62 +++++++++++++++++++++++++++++++++- >> 1 file changed, 61 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c >> index fdb0aae546c3..7b542ccb6596 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,7 @@ static const struct xr_type xr_types[] = { >> struct xr_data { >> const struct xr_type *type; >> u8 channel; /* zero-based index or interface number */ >> + u32 rs485_flags; >> }; >> >> static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val) >> @@ -645,9 +647,13 @@ 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; >> >> + if (data->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; > The logic here is unnecessarily convoluted here and you also should not > set hardware flow control mode if rs485 mode is enabled. > > Perhaps you can add a local boolean flag to hold the rs485 state and > test it before the current if-else construct. Then you only enable > hw-flow when rs485 mode is disabled while stile allowing sw-flow to be > set (hopefully that's a legal combination, please do try to verify > that). I'll implement SER_RS485_RTS_ON_SEND and set XR_GPIO_MODE_RS485_TX_H according to it. I tested sw flow and it works. > It also looks like you have inverted the RS485 polarity by using > XR_GPIO_MODE_RS485_TX_H (more on that below). > >> } else if (I_IXON(tty)) { >> u8 start_char = START_CHAR(tty); >> @@ -827,6 +833,59 @@ 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) > argp points to struct serial_rs485 to use that as the type rather than > pointer to unsigned int. > >> +{ >> + struct usb_serial_port *port = tty->driver_data; >> + struct xr_data *data = usb_get_serial_port_data(port); >> + struct serial_rs485 rs485; >> + >> + dev_dbg(tty->dev, "Flags %02x\n", data->rs485_flags); > This is not a very informative message. Please add back the function > prefix so that is also distinguishable from the dev_dbg() in the other > rs485 helper and use the following format: > > "%s - flags = 0x%02x\n", __func__, ... > > And add a new line here. I'll remove this debug and add one to xr_set_flow_mode dev_dbg(&port->dev, "Enabling RS-485\n"); >> + memset(&rs485, 0, sizeof(rs485)); >> + rs485.flags = data->rs485_flags; >> + 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) > Use a pointer to the struct here too. > >> +{ >> + struct usb_serial_port *port = tty->driver_data; >> + struct xr_data *data = usb_get_serial_port_data(port); >> + struct serial_rs485 rs485; >> + >> + if (copy_from_user(&rs485, argp, sizeof(rs485))) >> + return -EFAULT; >> + >> + dev_dbg(tty->dev, "Flags %02x\n", rs485.flags); > Please update the format string as mentioned above. > > Add a newline here. I'll remove this debug. >> + data->rs485_flags = rs485.flags & SER_RS485_ENABLED; >> + xr_set_flow_mode(tty, port, (const struct ktermios *)0); > This function accesses tty->termios so you can not call it here without > any locking as it can change underneath you and nothing currently > prevents set_termios() from calling the same function in parallel. > > If you take a write lock on the termios rw sempahore you can use it also > to protect the rs485 data instead of relying on implicit atomicity > rules. > > And perhaps you should just copy the entire rs485 struct from the start > as these devices supports further features which someone may want to > implement support for later (e.g. delay after send and 9th bit > addressing). > > You should just use NULL for the third (old_termios) argument. > >> + >> + // Only the enable flag is implemented > No c99 comments, please. > >> + memset(&rs485, 0, sizeof(rs485)); >> + rs485.flags = data->rs485_flags; > This does not look correct given that you set the RS485 TX polarity so > that RTS is high (logic disable) during TX above. > > You need to at least make sure that both the SER_RS485_RTS_ON_SEND and > SER_RS485_RTS_AFTER_SEND bits match the polarity setting. But perhaps > you could consider implementing support for configuring the polarity > from the start. What should happen if user sets SER_RS485_RTS_ON_SEND=1 and SER_RS485_RTS_AFTER_SEND=0 or vice versa ? There is only one bit for polarity control in the Exar register. I am thinking of using only SER_RS485_RTS_ON_SEND to control the polarity and setting _RTS_AFTER_SEND to the same value. >> + if (copy_to_user(argp, &rs485, sizeof(rs485))) >> + return -EFAULT; >> + >> + return 0; >> +} >> + >> + >> +static int xr_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg) >> +{ >> + 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; >> +} >> + >> static int xr_open(struct tty_struct *tty, struct usb_serial_port *port) >> { >> int ret; >> @@ -1010,6 +1069,7 @@ static struct usb_serial_driver xr_device = { >> .set_termios = xr_set_termios, >> .tiocmget = xr_tiocmget, >> .tiocmset = xr_tiocmset, >> + .ioctl = xr_ioctl, >> .dtr_rts = xr_dtr_rts >> }; > Johan - Jarkko
On Sun, Apr 16, 2023 at 11:40:00AM +0300, Jarkko Sonninen wrote: > On 4/13/23 11:53, Johan Hovold wrote: > > On Tue, Mar 14, 2023 at 09:00:01AM +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. > > Which register you use is an implementation details which is not really > > needed in the commit message. > > > > Please say something about how the hardware works and try to describe > > what you are implementing here and perhaps something about what is left > > unsupported (e.g. the fixed rts polarity). > > Add support for RS-485 in Exar USB adapters. > RS-485 mode is controlled by TIOCGRS485 and TIOCSRS485 ioctls. > SER_RS485_ENABLED and SER_RS485_RTS_ON_SEND flags are implemented. > There is only one polarity control in Exar for both RTS_ON_SEND and > RTS_AFTER_SEND. > RS-485 delays and addressing modes are not implemented. > > ok ? Sure, something like that, but perhaps you can amend the bit about RTS_AFTER_SEND (see below). It would be even better if you could try to rephrase this so that it reads a little easier. Something along the lines of Exar devices like <model> can control an RS485 tranceiver by automatically asserting the RTS#/RS485 pin before sending data and deasserting it when the last stop bit has been transmitted. The polarity of the RST#/RS485 signal is configurable and the hardware also supports features <XYZ>... Add support for enabling and disabling RS-485 mode and configuring the signal polarity using the TIOCGRS485 and TIOCSRS485 ioctls. Support for <XYZ> is left unimplemented for now. > >> Signed-off-by: Jarkko Sonninen <kasper@iki.fi> > >> --- > >> > >> In this version only rs485.flags are stored to state. > >> There is no locking as only one bit of the flags is used. > >> ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code. > >> > >> drivers/usb/serial/xr_serial.c | 62 +++++++++++++++++++++++++++++++++- > >> 1 file changed, 61 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c > >> index fdb0aae546c3..7b542ccb6596 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,7 @@ static const struct xr_type xr_types[] = { > >> struct xr_data { > >> const struct xr_type *type; > >> u8 channel; /* zero-based index or interface number */ > >> + u32 rs485_flags; > >> }; > >> > >> static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val) > >> @@ -645,9 +647,13 @@ 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; > >> > >> + if (data->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; > > The logic here is unnecessarily convoluted here and you also should not > > set hardware flow control mode if rs485 mode is enabled. > > > > Perhaps you can add a local boolean flag to hold the rs485 state and > > test it before the current if-else construct. Then you only enable > > hw-flow when rs485 mode is disabled while stile allowing sw-flow to be > > set (hopefully that's a legal combination, please do try to verify > > that). > > I'll implement SER_RS485_RTS_ON_SEND and set XR_GPIO_MODE_RS485_TX_H > according to it. > > I tested sw flow and it works. Thanks for checking. > > It also looks like you have inverted the RS485 polarity by using > > XR_GPIO_MODE_RS485_TX_H (more on that below). > > > >> } else if (I_IXON(tty)) { > >> u8 start_char = START_CHAR(tty); > >> +static int xr_set_rs485_config(struct tty_struct *tty, > >> + unsigned long __user *argp) > > Use a pointer to the struct here too. > > > >> +{ > >> + struct usb_serial_port *port = tty->driver_data; > >> + struct xr_data *data = usb_get_serial_port_data(port); > >> + struct serial_rs485 rs485; > >> + > >> + if (copy_from_user(&rs485, argp, sizeof(rs485))) > >> + return -EFAULT; > >> + > >> + dev_dbg(tty->dev, "Flags %02x\n", rs485.flags); > > Please update the format string as mentioned above. > > > > Add a newline here. > > I'll remove this debug. > > >> + data->rs485_flags = rs485.flags & SER_RS485_ENABLED; > >> + xr_set_flow_mode(tty, port, (const struct ktermios *)0); > > This function accesses tty->termios so you can not call it here without > > any locking as it can change underneath you and nothing currently > > prevents set_termios() from calling the same function in parallel. > > > > If you take a write lock on the termios rw sempahore you can use it also > > to protect the rs485 data instead of relying on implicit atomicity > > rules. > > > > And perhaps you should just copy the entire rs485 struct from the start > > as these devices supports further features which someone may want to > > implement support for later (e.g. delay after send and 9th bit > > addressing). > > > > You should just use NULL for the third (old_termios) argument. > > > >> + > >> + // Only the enable flag is implemented > > No c99 comments, please. > > > >> + memset(&rs485, 0, sizeof(rs485)); > >> + rs485.flags = data->rs485_flags; > > This does not look correct given that you set the RS485 TX polarity so > > that RTS is high (logic disable) during TX above. > > > > You need to at least make sure that both the SER_RS485_RTS_ON_SEND and > > SER_RS485_RTS_AFTER_SEND bits match the polarity setting. But perhaps > > you could consider implementing support for configuring the polarity > > from the start. > > What should happen if user sets SER_RS485_RTS_ON_SEND=1 and > SER_RS485_RTS_AFTER_SEND=0 or vice versa ? > > There is only one bit for polarity control in the Exar register. > > I am thinking of using only SER_RS485_RTS_ON_SEND to control the > polarity and setting _RTS_AFTER_SEND to the same value. Yeah, this is perhaps not the best designed interface we have... The way I interpret those flag, they should generally be each others negation as presumably most hardware controlled rs485 implementations works as the Exar devices do (i.e. they have one polarity flag). Settings both to the same value doesn't seem to make much sense so not sure why it was designed this way. But here you should just set SER_RS485_RTS_AFTER_SEND to !SER_RS485_RTS_ON_SEND. > >> + if (copy_to_user(argp, &rs485, sizeof(rs485))) > >> + return -EFAULT; > >> + > >> + return 0; > >> +} > >> + > >> + You can drop the second newline here (and elsewhere if you added more of these). > >> +static int xr_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg) > >> +{ > >> + 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; > >> +} Johan
diff --git a/drivers/usb/serial/xr_serial.c b/drivers/usb/serial/xr_serial.c index fdb0aae546c3..7b542ccb6596 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,7 @@ static const struct xr_type xr_types[] = { struct xr_data { const struct xr_type *type; u8 channel; /* zero-based index or interface number */ + u32 rs485_flags; }; static int xr_set_reg(struct usb_serial_port *port, u8 channel, u16 reg, u16 val) @@ -645,9 +647,13 @@ 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; + if (data->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 +833,59 @@ 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); + struct serial_rs485 rs485; + + dev_dbg(tty->dev, "Flags %02x\n", data->rs485_flags); + memset(&rs485, 0, sizeof(rs485)); + rs485.flags = data->rs485_flags; + 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; + + if (copy_from_user(&rs485, argp, sizeof(rs485))) + return -EFAULT; + + dev_dbg(tty->dev, "Flags %02x\n", rs485.flags); + data->rs485_flags = rs485.flags & SER_RS485_ENABLED; + xr_set_flow_mode(tty, port, (const struct ktermios *)0); + + // Only the enable flag is implemented + memset(&rs485, 0, sizeof(rs485)); + rs485.flags = data->rs485_flags; + if (copy_to_user(argp, &rs485, sizeof(rs485))) + return -EFAULT; + + return 0; +} + + +static int xr_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg) +{ + 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; +} + static int xr_open(struct tty_struct *tty, struct usb_serial_port *port) { int ret; @@ -1010,6 +1069,7 @@ static struct usb_serial_driver xr_device = { .set_termios = xr_set_termios, .tiocmget = xr_tiocmget, .tiocmset = xr_tiocmset, + .ioctl = xr_ioctl, .dtr_rts = xr_dtr_rts };
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> --- In this version only rs485.flags are stored to state. There is no locking as only one bit of the flags is used. ioctl returns -ENOIOCTLCMD as the actual error handling is in tty code. drivers/usb/serial/xr_serial.c | 62 +++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-)