drivers/tty/serial/serial_core.c | 11 ++++++++++- include/uapi/linux/serial.h | 19 +++++++++++-------- 2 files changed, 21 insertions(+), 9 deletions(-)
Add "SER_RS485_MODE_RS422" flag to struct serial_rs485, so that serial
port can switch interface into RS422 if supported by using ioctl command
"TIOCSRS485".
By treating RS422 as a mode of RS485, which means while enabling RS422
there are two flags need to be set (SER_RS485_ENABLED and
SER_RS485_MODE_RS422), it would make things much easier. For example
some places that checks for "SER_RS485_ENABLED" won't need to be rewritten.
There are only two things need to be noticed:
- While enabling RS422, other RS485 flags should not be set.
- RS422 doesn't need to deal with termination, so while disabling RS485
or enabling RS422, uart_set_rs485_termination() shall return.
Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
---
Changes from v3 to v4:
- Include 'linux/const.h' header in '/include/uapi/linux/serial.h'
- Replace BIT() with _BITUL() which defined in
'/include/uapi/linux/const.h'
Changes from v2 to v3:
- Remove "SER_RS422_ENABLED" flag from legacy flags.
- Revise "SER_RS422_ENABLED" into "SER_RS485_MODE_RS422".
- Remove the code which checks the conflicts between SER_RS485_ENABLED
and SER_RS422_ENABLED.
- Add return check in uart_set_rs485_termination().
Changes from v1 to v2:
- Revise the logic that checks whether RS422/RS485 are enabled
simultaneously.
v3: https://lore.kernel.org/all/20231108060719.11775-1-crescentcy.hsieh@moxa.com/
v2: https://lore.kernel.org/all/20231101064404.45711-1-crescentcy.hsieh@moxa.com/
v1: https://lore.kernel.org/all/20231030053632.5109-1-crescentcy.hsieh@moxa.com/
---
drivers/tty/serial/serial_core.c | 11 ++++++++++-
include/uapi/linux/serial.h | 19 +++++++++++--------
2 files changed, 21 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 831d03361..777f091a4 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1376,6 +1376,13 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
return;
}
+ /* Clear other RS485 flags and return if enabling RS422 */
+ if (rs485->flags & SER_RS485_MODE_RS422) {
+ memset(rs485, 0, sizeof(*rs485));
+ rs485->flags |= (SER_RS485_ENABLED | SER_RS485_MODE_RS422);
+ return;
+ }
+
/* Pick sane settings if the user hasn't */
if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
@@ -1400,7 +1407,9 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
static void uart_set_rs485_termination(struct uart_port *port,
const struct serial_rs485 *rs485)
{
- if (!(rs485->flags & SER_RS485_ENABLED))
+ /* Return while disabling RS485 or enabling RS422 */
+ if (!(rs485->flags & SER_RS485_ENABLED) ||
+ (rs485->flags & SER_RS485_ENABLED && rs485->flags & SER_RS485_MODE_RS422))
return;
gpiod_set_value_cansleep(port->rs485_term_gpio,
diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
index 53bc1af67..9086367db 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -11,6 +11,7 @@
#ifndef _UAPI_LINUX_SERIAL_H
#define _UAPI_LINUX_SERIAL_H
+#include <linux/const.h>
#include <linux/types.h>
#include <linux/tty_flags.h>
@@ -137,17 +138,19 @@ struct serial_icounter_struct {
* * %SER_RS485_ADDRB - Enable RS485 addressing mode.
* * %SER_RS485_ADDR_RECV - Receive address filter (enables @addr_recv). Requires %SER_RS485_ADDRB.
* * %SER_RS485_ADDR_DEST - Destination address (enables @addr_dest). Requires %SER_RS485_ADDRB.
+ * * %SER_RS485_MODE_RS422 - Enable RS422. Requires %SER_RS485_ENABLED.
*/
struct serial_rs485 {
__u32 flags;
-#define SER_RS485_ENABLED (1 << 0)
-#define SER_RS485_RTS_ON_SEND (1 << 1)
-#define SER_RS485_RTS_AFTER_SEND (1 << 2)
-#define SER_RS485_RX_DURING_TX (1 << 4)
-#define SER_RS485_TERMINATE_BUS (1 << 5)
-#define SER_RS485_ADDRB (1 << 6)
-#define SER_RS485_ADDR_RECV (1 << 7)
-#define SER_RS485_ADDR_DEST (1 << 8)
+#define SER_RS485_ENABLED _BITUL(0)
+#define SER_RS485_RTS_ON_SEND _BITUL(1)
+#define SER_RS485_RTS_AFTER_SEND _BITUL(2)
+#define SER_RS485_RX_DURING_TX _BITUL(3)
+#define SER_RS485_TERMINATE_BUS _BITUL(4)
+#define SER_RS485_ADDRB _BITUL(5)
+#define SER_RS485_ADDR_RECV _BITUL(6)
+#define SER_RS485_ADDR_DEST _BITUL(7)
+#define SER_RS485_MODE_RS422 _BITUL(8)
__u32 delay_rts_before_send;
__u32 delay_rts_after_send;
--
2.34.1Hi,
On 11/13/2023 3:41 AM, Crescent CY Hsieh wrote:
> Add "SER_RS485_MODE_RS422" flag to struct serial_rs485, so that serial
> port can switch interface into RS422 if supported by using ioctl command
> "TIOCSRS485".
>
> By treating RS422 as a mode of RS485, which means while enabling RS422
> there are two flags need to be set (SER_RS485_ENABLED and
> SER_RS485_MODE_RS422), it would make things much easier. For example
> some places that checks for "SER_RS485_ENABLED" won't need to be rewritten.
>
> There are only two things need to be noticed:
>
> - While enabling RS422, other RS485 flags should not be set.
> - RS422 doesn't need to deal with termination, so while disabling RS485
> or enabling RS422, uart_set_rs485_termination() shall return.
>
> Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
> > [...]
> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> index 53bc1af67..9086367db 100644
> --- a/include/uapi/linux/serial.h
> +++ b/include/uapi/linux/serial.h
> @@ -11,6 +11,7 @@
> #ifndef _UAPI_LINUX_SERIAL_H
> #define _UAPI_LINUX_SERIAL_H
>
> +#include <linux/const.h>
> #include <linux/types.h>
>
> #include <linux/tty_flags.h>
> @@ -137,17 +138,19 @@ struct serial_icounter_struct {
> * * %SER_RS485_ADDRB - Enable RS485 addressing mode.
> * * %SER_RS485_ADDR_RECV - Receive address filter (enables @addr_recv). Requires %SER_RS485_ADDRB.
> * * %SER_RS485_ADDR_DEST - Destination address (enables @addr_dest). Requires %SER_RS485_ADDRB.
> + * * %SER_RS485_MODE_RS422 - Enable RS422. Requires %SER_RS485_ENABLED.
> */
Documentation/driver-api/serial/serial-rs485.rst could also use an update,
since it doesn't mention your new flag at all.
The documentation as it is also doesn't give a very good idea of what flags
userspace might need to set for RS-232 vs RS-422 vs RS-485 (2- or 4-wire).
If I compare this to your original patch set [1] for your hardware, then
your proposed flag would be used in the following ways, correct?
RS-232: rs485->flags = 0
RS-422: rs485->flags = SER_RS485_ENABLED|SER_RS485_MODE_RS422
RS-485 (2-wire half-duplex): rs485->flags = SER_RS485_ENABLED
RS-485 (4-wire full-duplex): rs485->flags = SER_RS485_ENABLED|SER_RS485_RX_DURING_TX
In iot2040_rs485_config in 8250_exar.c [2] we already seem to have:
RS-232: rs485->flags = 0
RS-422: rs485->flags = SER_RS485_ENABLED|SER_RS485_RX_DURING_TX
RS-485 (2-wire half-duplex?): rs485->flags = SER_RS485_ENABLED
This would seem to create an inconsistency in this API.
I've also been trying to get a driver for NI's serial hardware upstream [3]; we
have "RS-485" products that can do both RS-422/RS-485, and also have use of
functionality to toggle between the two modes, so-- whichever way this flag
goes-- I'd like to be consistent with how other drivers do it.
[1] https://lore.kernel.org/linux-serial/20231018091739.10125-7-crescentcy.hsieh@moxa.com/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/8250/8250_exar.c?h=v6.6#n459
[3] https://lore.kernel.org/linux-serial/20231023210458.447779-3-brenda.streiff@ni.com/
Hi, On 15.11.23 03:50, Brenda Streiff wrote: > > Documentation/driver-api/serial/serial-rs485.rst could also use an update, > since it doesn't mention your new flag at all. > > The documentation as it is also doesn't give a very good idea of what flags > userspace might need to set for RS-232 vs RS-422 vs RS-485 (2- or 4-wire). > > If I compare this to your original patch set [1] for your hardware, then > your proposed flag would be used in the following ways, correct? > > RS-232: rs485->flags = 0 > RS-422: rs485->flags = SER_RS485_ENABLED|SER_RS485_MODE_RS422 > RS-485 (2-wire half-duplex): rs485->flags = SER_RS485_ENABLED > RS-485 (4-wire full-duplex): rs485->flags = SER_RS485_ENABLED|SER_RS485_RX_DURING_TX > > In iot2040_rs485_config in 8250_exar.c [2] we already seem to have: > RS-232: rs485->flags = 0 > RS-422: rs485->flags = SER_RS485_ENABLED|SER_RS485_RX_DURING_TX > RS-485 (2-wire half-duplex?): rs485->flags = SER_RS485_ENABLED > > This would seem to create an inconsistency in this API. > We can adjust 8250_exar later to also honor SER_RS485_MODE_RS422. But yes, we have to also keep the current logic (i.e. set the RS422 mode if SER_RS485_ENABLED|SER_RS485_RX_DURING_TX is set) for backward compatibility. Regards, Lino
On Tue, Nov 14, 2023 at 08:50:42PM -0600, Brenda Streiff wrote: > If I compare this to your original patch set [1] for your hardware, then > your proposed flag would be used in the following ways, correct? > > RS-232: rs485->flags = 0 > RS-422: rs485->flags = SER_RS485_ENABLED|SER_RS485_MODE_RS422 > RS-485 (2-wire half-duplex): rs485->flags = SER_RS485_ENABLED > RS-485 (4-wire full-duplex): rs485->flags = SER_RS485_ENABLED|SER_RS485_RX_DURING_TX > > In iot2040_rs485_config in 8250_exar.c [2] we already seem to have: > RS-232: rs485->flags = 0 > RS-422: rs485->flags = SER_RS485_ENABLED|SER_RS485_RX_DURING_TX > RS-485 (2-wire half-duplex?): rs485->flags = SER_RS485_ENABLED > > This would seem to create an inconsistency in this API. I've checked the patch series of iot2040_rs485_config() about RS422 [1], it seems to be reasonable back then so no one dicussed about that. > I've also been trying to get a driver for NI's serial hardware upstream [3]; we > have "RS-485" products that can do both RS-422/RS-485, and also have use of > functionality to toggle between the two modes, so-- whichever way this flag > goes-- I'd like to be consistent with how other drivers do it. > > [1] https://lore.kernel.org/linux-serial/20231018091739.10125-7-crescentcy.hsieh@moxa.com/ > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/8250/8250_exar.c?h=v6.6#n459 > [3] https://lore.kernel.org/linux-serial/20231023210458.447779-3-brenda.streiff@ni.com/ Originally, I was trying to add a new flag "SER_RS422_ENABLED" to represent RS422, but Jiri replied "Hopefully not" [2]. So I used an unused flag to represent RS422 as a workaround solution, then Jiri realized what was I trying to do and recommeded to add RS422 flag [3]. Then, when I was adding a new flag for RS422, Lino suggested to see RS422 as a mode of RS485 [4]. Anyway, IMO, it's more reasonable to add a flag for representing RS422, as for the naming, structure or future extention, it would require more discussion. [1] https://lore.kernel.org/all/?q=IOT2040_UART_MODE_RS422 [2] https://lore.kernel.org/linux-serial/92aed0d9-791f-4708-8a73-4c78457a710e@kernel.org/ [3] https://lore.kernel.org/linux-serial/3332a86c-1a1d-4b78-bbfa-8ac3e2e642a1@kernel.org/ [4] https://lore.kernel.org/all/3fc18b13-fef0-439e-abf0-1fe4e46b224a@gmx.de/ --- Sincerely, Crescent CY Hsieh
Hi,
On 13.11.23 10:41, Crescent CY Hsieh wrote:
> Add "SER_RS485_MODE_RS422" flag to struct serial_rs485, so that serial
> port can switch interface into RS422 if supported by using ioctl command
> "TIOCSRS485".
>
> By treating RS422 as a mode of RS485, which means while enabling RS422
> there are two flags need to be set (SER_RS485_ENABLED and
> SER_RS485_MODE_RS422), it would make things much easier. For example
> some places that checks for "SER_RS485_ENABLED" won't need to be rewritten.
>
> There are only two things need to be noticed:
>
> - While enabling RS422, other RS485 flags should not be set.
> - RS422 doesn't need to deal with termination, so while disabling RS485
> or enabling RS422, uart_set_rs485_termination() shall return.
Commit messages should use the imperative form.
>
> Signed-off-by: Crescent CY Hsieh <crescentcy.hsieh@moxa.com>
>
> ---
> Changes from v3 to v4:
> - Include 'linux/const.h' header in '/include/uapi/linux/serial.h'
> - Replace BIT() with _BITUL() which defined in
> '/include/uapi/linux/const.h'
>
> Changes from v2 to v3:
> - Remove "SER_RS422_ENABLED" flag from legacy flags.
> - Revise "SER_RS422_ENABLED" into "SER_RS485_MODE_RS422".
> - Remove the code which checks the conflicts between SER_RS485_ENABLED
> and SER_RS422_ENABLED.
> - Add return check in uart_set_rs485_termination().
>
> Changes from v1 to v2:
> - Revise the logic that checks whether RS422/RS485 are enabled
> simultaneously.
>
> v3: https://lore.kernel.org/all/20231108060719.11775-1-crescentcy.hsieh@moxa.com/
> v2: https://lore.kernel.org/all/20231101064404.45711-1-crescentcy.hsieh@moxa.com/
> v1: https://lore.kernel.org/all/20231030053632.5109-1-crescentcy.hsieh@moxa.com/
>
> ---
> drivers/tty/serial/serial_core.c | 11 ++++++++++-
> include/uapi/linux/serial.h | 19 +++++++++++--------
> 2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 831d03361..777f091a4 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1376,6 +1376,13 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
> return;
> }
>
> + /* Clear other RS485 flags and return if enabling RS422 */
> + if (rs485->flags & SER_RS485_MODE_RS422) {
> + memset(rs485, 0, sizeof(*rs485));
> + rs485->flags |= (SER_RS485_ENABLED | SER_RS485_MODE_RS422)> + return;
> + }
> +
> /* Pick sane settings if the user hasn't */
> if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
> !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
> @@ -1400,7 +1407,9 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
> static void uart_set_rs485_termination(struct uart_port *port,
> const struct serial_rs485 *rs485)
> {
> - if (!(rs485->flags & SER_RS485_ENABLED))
> + /* Return while disabling RS485 or enabling RS422 */
> + if (!(rs485->flags & SER_RS485_ENABLED) ||
> + (rs485->flags & SER_RS485_ENABLED && rs485->flags & SER_RS485_MODE_RS422))
Is this check needed at all? If no termination GPIO is specified, gpiod_set_value_cansleep()
does nothing. If a termination GPIO is specified it will be deasserted in case of RS422
(since for the RS422 case the SER_RS485_TERMINATE_BUS flag is deleted in
uart_sanitize_serial_rs485()). This seems like a good behaviour to me, so IMO the above check
is not needed.
>
> gpiod_set_value_cansleep(port->rs485_term_gpio,
> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> index 53bc1af67..9086367db 100644
> --- a/include/uapi/linux/serial.h
> +++ b/include/uapi/linux/serial.h
> @@ -11,6 +11,7 @@
> #ifndef _UAPI_LINUX_SERIAL_H
> #define _UAPI_LINUX_SERIAL_H
>
> +#include <linux/const.h>
> #include <linux/types.h>
>
> #include <linux/tty_flags.h>
> @@ -137,17 +138,19 @@ struct serial_icounter_struct {
> * * %SER_RS485_ADDRB - Enable RS485 addressing mode.
> * * %SER_RS485_ADDR_RECV - Receive address filter (enables @addr_recv). Requires %SER_RS485_ADDRB.
> * * %SER_RS485_ADDR_DEST - Destination address (enables @addr_dest). Requires %SER_RS485_ADDRB.
> + * * %SER_RS485_MODE_RS422 - Enable RS422. Requires %SER_RS485_ENABLED.
All of the above flags are only effective if RS485_ENABLED is set, so no need
to mention this explicitly for the MODE_RS422 flag.
> */
> struct serial_rs485 {
> __u32 flags;
> -#define SER_RS485_ENABLED (1 << 0)
> -#define SER_RS485_RTS_ON_SEND (1 << 1)
> -#define SER_RS485_RTS_AFTER_SEND (1 << 2)
> -#define SER_RS485_RX_DURING_TX (1 << 4)
> -#define SER_RS485_TERMINATE_BUS (1 << 5)
> -#define SER_RS485_ADDRB (1 << 6)
> -#define SER_RS485_ADDR_RECV (1 << 7)
> -#define SER_RS485_ADDR_DEST (1 << 8)
> +#define SER_RS485_ENABLED _BITUL(0)
> +#define SER_RS485_RTS_ON_SEND _BITUL(1)
> +#define SER_RS485_RTS_AFTER_SEND _BITUL(2)
> +#define SER_RS485_RX_DURING_TX _BITUL(3)
> +#define SER_RS485_TERMINATE_BUS _BITUL(4)
> +#define SER_RS485_ADDRB _BITUL(5)
> +#define SER_RS485_ADDR_RECV _BITUL(6)
> +#define SER_RS485_ADDR_DEST _BITUL(7)
> +#define SER_RS485_MODE_RS422 _BITUL(8)
>
> __u32 delay_rts_before_send;
> __u32 delay_rts_after_send;
Regards,
Lino
© 2016 - 2025 Red Hat, Inc.