| Message ID | 20220703170039.2058202-1-LinoSanfilippo@gmx.de |
|---|---|
| Headers | show |
| Series | Fixes and cleanup for RS485 | expand |
On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote: > > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > In uart_get_rs485_mode() only try to get a termination GPIO if RS485 bus > termination is supported by the driver. This prevents from allocating > and holding a GPIO descriptor for the drivers lifetimg that will never be lifetiming > used. ... > port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term", > GPIOD_OUT_LOW); > + > + if (port->rs485_term_gpio && This check is incorrect. Either you need to move that after error checking (that's what I personally prefer), or use !IS_ERR_OR_NULL(). > + !(port->rs485_supported->flags & SER_RS485_TERMINATE_BUS)) { > + dev_warn(port->dev, > + "%s (%d): RS485 termination gpio not supported by driver\n", > + port->name, port->line); > + devm_gpiod_put(dev, port->rs485_term_gpio); > + port->rs485_term_gpio = NULL; > + } > + > if (IS_ERR(port->rs485_term_gpio)) { > ret = PTR_ERR(port->rs485_term_gpio); > port->rs485_term_gpio = NULL;
On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote: > > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > In serial8250_em485_config() the termination GPIO is set with the uart_port > spinlock held. This is an issue if setting the GPIO line can sleep (e.g. > since the concerning GPIO expander is connected via SPI or I2C). > > Fix this by setting the termination line outside of the uart_port spinlock > in the serial core and using gpiod_set_value_cansleep() which instead of > gpiod_set_value() allows to sleep. allows it to > Beside fixing the termination GPIO line setting for the 8250 driver this > change also makes setting the termination GPIO generic for all UART > drivers. ... > +static void uart_set_rs485_termination(struct uart_port *port, > + const struct serial_rs485 *rs485) > +{ > + if (!port->rs485_term_gpio This duplicates the check the GPIO library does. Drop it. > || !(rs485->flags & SER_RS485_ENABLED)) > + return; > + > + gpiod_set_value_cansleep(port->rs485_term_gpio, > + !!(rs485->flags & SER_RS485_TERMINATE_BUS)); > +}
On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote: > > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > When setting the RS485 configuration from userspace via TIOCSRS485 the > delays are clamped to 100ms. Make this consistent with the values passed > in by means of device tree parameters. I'm not sure I got it right. Is the values from DT now clampet as well as user space does or other way around? In either way the commit message misses the explanation why it's not a problem if user previously passed bigger values either via user space or via DT, because it's an ABI change, right?
On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote: > > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > Without an RTS GPIO RS485 is not possible so disable the support > regardless of whether RS485 is enabled at boottime or not. Also remove the boot time > now superfluous check for the RTS GPIO in ar933x_config_rs485(). > > Fixes: e849145e1fdd ("serial: ar933x: Fill in rs485_supported") Is it an independent fix? If so, it should be the first patch in the series, otherwise if it's dependent on something from previous patches you need to mark all of them as a fix.
Hi, On 03.07.22 20:27, Andy Shevchenko wrote: > On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote: >> >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> >> In uart_get_rs485_mode() only try to get a termination GPIO if RS485 bus >> termination is supported by the driver. This prevents from allocating >> and holding a GPIO descriptor for the drivers lifetimg that will never be > > lifetiming > >> used. > > ... > >> port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term", >> GPIOD_OUT_LOW); >> + >> + if (port->rs485_term_gpio && > > This check is incorrect. Either you need to move that after error > checking (that's what I personally prefer), or use !IS_ERR_OR_NULL(). > Right, a stupid mistake. I will fix this, thanks! Regards, Lino
On 03.07.22 20:34, Andy Shevchenko wrote: > On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote: >> >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> >> When setting the RS485 configuration from userspace via TIOCSRS485 the >> delays are clamped to 100ms. Make this consistent with the values passed >> in by means of device tree parameters. > > I'm not sure I got it right. Is the values from DT now clampet as well > as user space does or other way around? In either way the commit > message misses the explanation why it's not a problem if user > previously passed bigger values either via user space or via DT, > because it's an ABI change, right? > Values are now clamped to 100 ms if set by userspace via ioctl and not clamped at all if set by DT. I will improve the commit message to make this more clear. Thanks, Lino
On 03.07.22 20:39, Andy Shevchenko wrote: > On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote: >> >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> >> Without an RTS GPIO RS485 is not possible so disable the support >> regardless of whether RS485 is enabled at boottime or not. Also remove the > > boot time > >> now superfluous check for the RTS GPIO in ar933x_config_rs485(). >> >> Fixes: e849145e1fdd ("serial: ar933x: Fill in rs485_supported") > > Is it an independent fix? If so, it should be the first patch in the > series, otherwise if it's dependent on something from previous patches > you need to mark all of them as a fix. > The fix is independent, patch 8 depends on the fix however. I was not aware of this fixes-first rule for series with patches that are independent from each other. I will change the order accordingly in the next version of the series. Thanks, Lino
On 03.07.22 20:31, Andy Shevchenko wrote: > On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote: >> >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> >> In serial8250_em485_config() the termination GPIO is set with the uart_port >> spinlock held. This is an issue if setting the GPIO line can sleep (e.g. >> since the concerning GPIO expander is connected via SPI or I2C). >> >> Fix this by setting the termination line outside of the uart_port spinlock >> in the serial core and using gpiod_set_value_cansleep() which instead of >> gpiod_set_value() allows to sleep. > > allows it to > Ok. >> Beside fixing the termination GPIO line setting for the 8250 driver this >> change also makes setting the termination GPIO generic for all UART >> drivers. > > ... > >> +static void uart_set_rs485_termination(struct uart_port *port, >> + const struct serial_rs485 *rs485) >> +{ > >> + if (!port->rs485_term_gpio > > This duplicates the check the GPIO library does. Drop it. > Ok. >> || !(rs485->flags & SER_RS485_ENABLED)) >> + return; >> + >> + gpiod_set_value_cansleep(port->rs485_term_gpio, >> + !!(rs485->flags & SER_RS485_TERMINATE_BUS)); >> +} > Thanks for the review! Regards, Lino
On Sun, 3 Jul 2022, Lino Sanfilippo wrote: > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > In serial8250_em485_config() the termination GPIO is set with the uart_port > spinlock held. This is an issue if setting the GPIO line can sleep (e.g. > since the concerning GPIO expander is connected via SPI or I2C). > > Fix this by setting the termination line outside of the uart_port spinlock > in the serial core and using gpiod_set_value_cansleep() which instead of > gpiod_set_value() allows to sleep. > > Beside fixing the termination GPIO line setting for the 8250 driver this > change also makes setting the termination GPIO generic for all UART > drivers. > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > --- > drivers/tty/serial/8250/8250_port.c | 3 --- > drivers/tty/serial/serial_core.c | 12 ++++++++++++ > 2 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index ed2a606f2da7..72252d956f17 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -676,9 +676,6 @@ int serial8250_em485_config(struct uart_port *port, struct ktermios *termios, > rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; > } > > - gpiod_set_value(port->rs485_term_gpio, > - rs485->flags & SER_RS485_TERMINATE_BUS); > - I sent a series to make .rs485_supported per uart_port and properly set SER_RS485_TERMINATE_BUS according to DT config. With that series added first, SER_RS485_TERMINATE_BUS should also be removed from serial8250_em485_supported so that serial core can properly manage it all.
On Mon, 4 Jul 2022, Lino Sanfilippo wrote: > On 03.07.22 20:39, Andy Shevchenko wrote: > > On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <LinoSanfilippo@gmx.de> wrote: > >> > >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > >> > >> Without an RTS GPIO RS485 is not possible so disable the support > >> regardless of whether RS485 is enabled at boottime or not. Also remove the > > > > boot time > > > >> now superfluous check for the RTS GPIO in ar933x_config_rs485(). > >> > >> Fixes: e849145e1fdd ("serial: ar933x: Fill in rs485_supported") > > > > Is it an independent fix? If so, it should be the first patch in the > > series, otherwise if it's dependent on something from previous patches > > you need to mark all of them as a fix. > > > > The fix is independent, patch 8 depends on the fix however. I was not > aware of this fixes-first rule for series with patches that are independent > from each other. I will change the order accordingly in the next version of the series. While at it, you could separate just the fix to own patch and the ->rs485_config() cleanup to own patch (or move it all to patch 8). Not that this fix is expected to go anywhere else besides tty-next.
On Sun, 3 Jul 2022, Lino Sanfilippo wrote: > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > In uart_get_rs485_mode() only try to get a termination GPIO if RS485 bus > termination is supported by the driver. This prevents from allocating > and holding a GPIO descriptor for the drivers lifetimg that will never be > used. > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > --- > > NOTE: > This patch follows the design decision that "rs485_supported" is > set by the driver at initialization and cannot be modified > afterwards. However the better approach would be to let the serial > core modify the termination GPIO support setting based on the > existence of a termination GPIO. If "rs485_supported" is not a > read-only value any more in future the logic implemented in this > patch should be adjusted accordingly. > > drivers/tty/serial/serial_core.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index 85ef7ef00b82..3768663dfa4d 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -3404,6 +3404,16 @@ int uart_get_rs485_mode(struct uart_port *port) > */ > port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term", > GPIOD_OUT_LOW); > + > + if (port->rs485_term_gpio && > + !(port->rs485_supported->flags & SER_RS485_TERMINATE_BUS)) { > + dev_warn(port->dev, > + "%s (%d): RS485 termination gpio not supported by driver\n", > + port->name, port->line); > + devm_gpiod_put(dev, port->rs485_term_gpio); > + port->rs485_term_gpio = NULL; > + } > + > if (IS_ERR(port->rs485_term_gpio)) { > ret = PTR_ERR(port->rs485_term_gpio); > port->rs485_term_gpio = NULL; I sent a series to embed supported_rs485 to uart_port and manage SER_RS485_TERMINATE_BUS properly so I think this won't be necessary with that?
On 04.07.22 11:55, Ilpo Järvinen wrote: > On Sun, 3 Jul 2022, Lino Sanfilippo wrote: > >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> >> In uart_get_rs485_mode() only try to get a termination GPIO if RS485 bus >> termination is supported by the driver. This prevents from allocating >> and holding a GPIO descriptor for the drivers lifetimg that will never be >> used. >> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> --- >> >> NOTE: >> This patch follows the design decision that "rs485_supported" is >> set by the driver at initialization and cannot be modified >> afterwards. However the better approach would be to let the serial >> core modify the termination GPIO support setting based on the >> existence of a termination GPIO. If "rs485_supported" is not a >> read-only value any more in future the logic implemented in this >> patch should be adjusted accordingly. >> >> drivers/tty/serial/serial_core.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c >> index 85ef7ef00b82..3768663dfa4d 100644 >> --- a/drivers/tty/serial/serial_core.c >> +++ b/drivers/tty/serial/serial_core.c >> @@ -3404,6 +3404,16 @@ int uart_get_rs485_mode(struct uart_port *port) >> */ >> port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term", >> GPIOD_OUT_LOW); >> + >> + if (port->rs485_term_gpio && >> + !(port->rs485_supported->flags & SER_RS485_TERMINATE_BUS)) { >> + dev_warn(port->dev, >> + "%s (%d): RS485 termination gpio not supported by driver\n", >> + port->name, port->line); >> + devm_gpiod_put(dev, port->rs485_term_gpio); >> + port->rs485_term_gpio = NULL; >> + } >> + >> if (IS_ERR(port->rs485_term_gpio)) { >> ret = PTR_ERR(port->rs485_term_gpio); >> port->rs485_term_gpio = NULL; > > I sent a series to embed supported_rs485 to uart_port and manage > SER_RS485_TERMINATE_BUS properly so I think this won't be necessary > with that? > > This is why I wrote the "NOTE" above. But yes, this patch is not needed any more. I will drop it in the next version. Regards, Lino
On 04.07.22 11:51, Ilpo Järvinen wrote: > On Sun, 3 Jul 2022, Lino Sanfilippo wrote: > >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> >> In serial8250_em485_config() the termination GPIO is set with the uart_port >> spinlock held. This is an issue if setting the GPIO line can sleep (e.g. >> since the concerning GPIO expander is connected via SPI or I2C). >> >> Fix this by setting the termination line outside of the uart_port spinlock >> in the serial core and using gpiod_set_value_cansleep() which instead of >> gpiod_set_value() allows to sleep. >> >> Beside fixing the termination GPIO line setting for the 8250 driver this >> change also makes setting the termination GPIO generic for all UART >> drivers. >> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> --- >> drivers/tty/serial/8250/8250_port.c | 3 --- >> drivers/tty/serial/serial_core.c | 12 ++++++++++++ >> 2 files changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c >> index ed2a606f2da7..72252d956f17 100644 >> --- a/drivers/tty/serial/8250/8250_port.c >> +++ b/drivers/tty/serial/8250/8250_port.c >> @@ -676,9 +676,6 @@ int serial8250_em485_config(struct uart_port *port, struct ktermios *termios, >> rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; >> } >> >> - gpiod_set_value(port->rs485_term_gpio, >> - rs485->flags & SER_RS485_TERMINATE_BUS); >> - > > I sent a series to make .rs485_supported per uart_port and properly set > SER_RS485_TERMINATE_BUS according to DT config. With that series added > first, SER_RS485_TERMINATE_BUS should also be removed from > serial8250_em485_supported so that serial core can properly manage > it all. > Ok, I will rebase the next version of my patches on your series then. Regards, Lino
From: Lino Sanfilippo <l.sanfilippo@kunbus.com> The following series includes cleanup and fixes around RS485 in the serial core and uart drivers: Patch 1: Only request the rs485 termination gpio if it is supported. NOTE: This patch follows the design decision that "rs485_supported" is set by the driver at initialization and cannot be modified afterwards. However the better approach would be to let the serial core modify the termination GPIO support setting based on the existence of a termination GPIO. If "rs485_supported" is not a read-only value any more in future the logic implemented in this patch should be adjusted accordingly. Patch 2: Set the rs485 termination GPIO in the serial core. This is needed since if the gpio is only accessible in sleepable context. It also is a further step to make the RS485 handling more generic. Patch 3: Move sanitizing of RS485 delays into an own function. This is in preparation of patch 4. Patch 4: Sanitize RS485 delays read from device tree. Patch 5: Correct RS485 delays in binding documentation. Patch 6: Remove redundant code in 8250_dwlib. Patch 7: Fix check for RS485 support. Patch 8: Remove redundant code in ar933x. Patch 9: Remove redundant code in 8250-lpc18xx. Changes in v2: - print a warning if termination GPIO is specified in DT/ACPI but is not supported by driver - fixed commit message for devtree documentation (as suggested by Andy) - fixed code comment - added patch 7 Lino Sanfilippo (9): serial: core: only get RS485 termination GPIO if supported serial: core, 8250: set RS485 termination gpio in serial core serial: core: move sanitizing of RS485 delays into own function serial: core: sanitize RS485 delays read from device tree dt_bindings: rs485: Correct delay values serial: 8250_dwlib: remove redundant sanity check for RS485 flags serial: ar933x: Fix check for RS485 support serial: ar933x: Remove redundant assignment in rs485_config serial: 8250: lpc18xx: Remove redundant sanity check for RS485 flags .../devicetree/bindings/serial/rs485.yaml | 4 +- drivers/tty/serial/8250/8250_dwlib.c | 10 +-- drivers/tty/serial/8250/8250_lpc18xx.c | 6 +- drivers/tty/serial/8250/8250_port.c | 3 - drivers/tty/serial/ar933x_uart.c | 18 ++--- drivers/tty/serial/serial_core.c | 70 +++++++++++++------ 6 files changed, 60 insertions(+), 51 deletions(-) base-commit: 7349660438603ed19282e75949561406531785a5