| Message ID | 20220703170039.2058202-2-LinoSanfilippo@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| 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;
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 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
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;