| Message ID | 20220213222737.15709-2-LinoSanfilippo@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/9] serial: core: move RS485 configuration tasks from drivers into core | expand |
On 13. 02. 22, 23:27, Lino Sanfilippo wrote: > Several drivers that support setting the RS485 configuration via userspace > implement on or more of the following tasks: > > - in case of an invalid RTS configuration (both RTS after send and RTS on > send set or both unset) fall back to enable RTS on send and disable RTS > after send > > - nullify the padding field of the returned serial_rs485 struct > > - copy the configuration into the uart port struct > > - limit RTS delays to 100 ms > > Move these tasks into the serial core to make them generic and to provide > a consistent beheviour among all drivers. > > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> > --- > drivers/tty/serial/serial_core.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index 846192a7b4bf..3fab4070359c 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -1282,8 +1282,21 @@ static int uart_set_rs485_config(struct uart_port *port, > if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user))) > return -EFAULT; > > + /* pick sane settings if the user hasn't */ > + if (!(rs485.flags & SER_RS485_RTS_ON_SEND) == > + !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) { > + rs485.flags |= SER_RS485_RTS_ON_SEND; > + rs485.flags &= ~SER_RS485_RTS_AFTER_SEND; > + } > + /* clamp the delays to [0, 100ms] */ > + rs485.delay_rts_before_send = min(rs485.delay_rts_before_send, 100U); > + rs485.delay_rts_after_send = min(rs485.delay_rts_after_send, 100U); Why is this magic 100? Can we have that number somehow documented? You should define a macro for that anyway. > + memset(rs485.padding, 0, sizeof(rs485.padding)); What is this memset good for? > spin_lock_irqsave(&port->lock, flags); > ret = port->rs485_config(port, &rs485); > + if (!ret) > + port->rs485 = rs485; > spin_unlock_irqrestore(&port->lock, flags); > if (ret) > return ret; thanks,
On Sun, Feb 13, 2022 at 11:27:29PM +0100, Lino Sanfilippo wrote: > Several drivers that support setting the RS485 configuration via userspace > implement on or more of the following tasks: s/on/one/ > > - in case of an invalid RTS configuration (both RTS after send and RTS on > send set or both unset) fall back to enable RTS on send and disable RTS > after send > > - nullify the padding field of the returned serial_rs485 struct > > - copy the configuration into the uart port struct > > - limit RTS delays to 100 ms > > Move these tasks into the serial core to make them generic and to provide > a consistent beheviour among all drivers. s/beheviour/behaviour/ > Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> > --- > drivers/tty/serial/serial_core.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index 846192a7b4bf..3fab4070359c 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -1282,8 +1282,21 @@ static int uart_set_rs485_config(struct uart_port *port, > if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user))) > return -EFAULT; > > + /* pick sane settings if the user hasn't */ > + if (!(rs485.flags & SER_RS485_RTS_ON_SEND) == > + !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) { > + rs485.flags |= SER_RS485_RTS_ON_SEND; > + rs485.flags &= ~SER_RS485_RTS_AFTER_SEND; > + } > + /* clamp the delays to [0, 100ms] */ > + rs485.delay_rts_before_send = min(rs485.delay_rts_before_send, 100U); > + rs485.delay_rts_after_send = min(rs485.delay_rts_after_send, 100U); > + memset(rs485.padding, 0, sizeof(rs485.padding)); > + > spin_lock_irqsave(&port->lock, flags); > ret = port->rs485_config(port, &rs485); > + if (!ret) > + port->rs485 = rs485; I was only Cc:d for the imx patch (patch #7) and tried to verify the claim there that "the serial core already assigns the passed configuration to the uart port". That failed when I looked at my kernel tree. So it would be great, if you sent dependencies (or at least a cover letter) to all recipients of a given patch to ease review. Also I want to suggest to mention uart_set_rs485_config() in the commit log of the imx patch (and probably the others) to simplify verifying the claim there. Thanks Uwe
Hi, On 14.02.22 at 08:06, Uwe Kleine-König wrote: > On Sun, Feb 13, 2022 at 11:27:29PM +0100, Lino Sanfilippo wrote: >> Several drivers that support setting the RS485 configuration via userspace >> implement on or more of the following tasks: > > s/on/one/ Ok > >> >> - in case of an invalid RTS configuration (both RTS after send and RTS on >> send set or both unset) fall back to enable RTS on send and disable RTS >> after send >> >> - nullify the padding field of the returned serial_rs485 struct >> >> - copy the configuration into the uart port struct >> >> - limit RTS delays to 100 ms >> >> Move these tasks into the serial core to make them generic and to provide >> a consistent beheviour among all drivers. > > s/beheviour/behaviour/ > Ok >> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> >> --- >> drivers/tty/serial/serial_core.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c >> index 846192a7b4bf..3fab4070359c 100644 >> --- a/drivers/tty/serial/serial_core.c >> +++ b/drivers/tty/serial/serial_core.c >> @@ -1282,8 +1282,21 @@ static int uart_set_rs485_config(struct uart_port *port, >> if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user))) >> return -EFAULT; >> >> + /* pick sane settings if the user hasn't */ >> + if (!(rs485.flags & SER_RS485_RTS_ON_SEND) == >> + !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) { >> + rs485.flags |= SER_RS485_RTS_ON_SEND; >> + rs485.flags &= ~SER_RS485_RTS_AFTER_SEND; >> + } >> + /* clamp the delays to [0, 100ms] */ >> + rs485.delay_rts_before_send = min(rs485.delay_rts_before_send, 100U); >> + rs485.delay_rts_after_send = min(rs485.delay_rts_after_send, 100U); >> + memset(rs485.padding, 0, sizeof(rs485.padding)); >> + >> spin_lock_irqsave(&port->lock, flags); >> ret = port->rs485_config(port, &rs485); >> + if (!ret) >> + port->rs485 = rs485; > > I was only Cc:d for the imx patch (patch #7) and tried to verify the > claim there that "the serial core already assigns the passed > configuration to the uart port". That failed when I looked at my kernel > tree. > > So it would be great, if you sent dependencies (or at least a cover > letter) to all recipients of a given patch to ease review. Also I want > to suggest to mention uart_set_rs485_config() in the commit log of the > imx patch (and probably the others) to simplify verifying the claim > there. > Thanks for the review, I will correct the typos in the next version. I will also cc you directly for the next version if you dont mind. get_maintainers only spit out "Pengutronix Kernel Team" so I used that address for the whole series (including the cover letter). Regards, Lino
Hi, On 14.02.22 at 06:41, Jiri Slaby wrote: > On 13. 02. 22, 23:27, Lino Sanfilippo wrote: >> Several drivers that support setting the RS485 configuration via userspace >> implement on or more of the following tasks: >> >> - in case of an invalid RTS configuration (both RTS after send and RTS on >> send set or both unset) fall back to enable RTS on send and disable RTS >> after send >> >> - nullify the padding field of the returned serial_rs485 struct >> >> - copy the configuration into the uart port struct >> >> - limit RTS delays to 100 ms >> >> Move these tasks into the serial core to make them generic and to provide >> a consistent beheviour among all drivers. >> >> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> >> --- >> drivers/tty/serial/serial_core.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c >> index 846192a7b4bf..3fab4070359c 100644 >> --- a/drivers/tty/serial/serial_core.c >> +++ b/drivers/tty/serial/serial_core.c >> @@ -1282,8 +1282,21 @@ static int uart_set_rs485_config(struct uart_port *port, >> if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user))) >> return -EFAULT; >> + /* pick sane settings if the user hasn't */ >> + if (!(rs485.flags & SER_RS485_RTS_ON_SEND) == >> + !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) { >> + rs485.flags |= SER_RS485_RTS_ON_SEND; >> + rs485.flags &= ~SER_RS485_RTS_AFTER_SEND; >> + } >> + /* clamp the delays to [0, 100ms] */ >> + rs485.delay_rts_before_send = min(rs485.delay_rts_before_send, 100U); >> + rs485.delay_rts_after_send = min(rs485.delay_rts_after_send, 100U); > > Why is this magic 100? The only drivers that seem to care about a max value for the RTS delays use 100 ms (omap-serial, amba pl011, 8250) so I chose this to stay compatible with the current driver implementations. 100 ms also seems large enough to be used as a general max value. > Can we have that number somehow documented? You should define a macro for that anyway. Ok, I will do so. > >> + memset(rs485.padding, 0, sizeof(rs485.padding)); > > What is this memset good for? Drivers like max310x, amba-pl011, 8250_pci, 8250_fintek, 8250_lpc18xx seem to care about returning a serial_rs485 struct with cleared padding field to userspace. So they all clear that field on their own. Although not really necessary, to me this seems to be a good default behavior, so I added it to the serial core. > >> spin_lock_irqsave(&port->lock, flags); >> ret = port->rs485_config(port, &rs485); >> + if (!ret) >> + port->rs485 = rs485; >> spin_unlock_irqrestore(&port->lock, flags); >> if (ret) >> return ret; > > thanks, Thanks for the review! Regards, Lino
Hello Lino, On Mon, Feb 14, 2022 at 04:09:53PM +0100, Lino Sanfilippo wrote: > On 14.02.22 at 08:06, Uwe Kleine-König wrote: > > I was only Cc:d for the imx patch (patch #7) and tried to verify the > > claim there that "the serial core already assigns the passed > > configuration to the uart port". That failed when I looked at my kernel > > tree. > > > > So it would be great, if you sent dependencies (or at least a cover > > letter) to all recipients of a given patch to ease review. Also I want > > to suggest to mention uart_set_rs485_config() in the commit log of the > > imx patch (and probably the others) to simplify verifying the claim > > there. > > Thanks for the review, I will correct the typos in the next version. > I will also cc you directly for the next version if you dont mind. I don't mind. I get so many patches by mail, I'm good at ignoring them ;-) > get_maintainers only spit out "Pengutronix Kernel Team" so I used that > address for the whole series (including the cover letter). That's why I eventually found the whole series and could reply to patch #1. Best regards Uwe
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 846192a7b4bf..3fab4070359c 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -1282,8 +1282,21 @@ static int uart_set_rs485_config(struct uart_port *port, if (copy_from_user(&rs485, rs485_user, sizeof(*rs485_user))) return -EFAULT; + /* pick sane settings if the user hasn't */ + if (!(rs485.flags & SER_RS485_RTS_ON_SEND) == + !(rs485.flags & SER_RS485_RTS_AFTER_SEND)) { + rs485.flags |= SER_RS485_RTS_ON_SEND; + rs485.flags &= ~SER_RS485_RTS_AFTER_SEND; + } + /* clamp the delays to [0, 100ms] */ + rs485.delay_rts_before_send = min(rs485.delay_rts_before_send, 100U); + rs485.delay_rts_after_send = min(rs485.delay_rts_after_send, 100U); + memset(rs485.padding, 0, sizeof(rs485.padding)); + spin_lock_irqsave(&port->lock, flags); ret = port->rs485_config(port, &rs485); + if (!ret) + port->rs485 = rs485; spin_unlock_irqrestore(&port->lock, flags); if (ret) return ret;
Several drivers that support setting the RS485 configuration via userspace implement on or more of the following tasks: - in case of an invalid RTS configuration (both RTS after send and RTS on send set or both unset) fall back to enable RTS on send and disable RTS after send - nullify the padding field of the returned serial_rs485 struct - copy the configuration into the uart port struct - limit RTS delays to 100 ms Move these tasks into the serial core to make them generic and to provide a consistent beheviour among all drivers. Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de> --- drivers/tty/serial/serial_core.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) base-commit: ad30d108a5135af584ff47f5ff81be971b6c26f1