lkml.org 
[lkml]   [2022]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/8] serial: core: only get RS485 termination gpio if supported
    On Sat, 25 Jun 2022, Lukas Wunner wrote:

    > On Wed, Jun 22, 2022 at 05:46:52PM +0200, Lino Sanfilippo wrote:
    > > In uart_get_rs485_mode() only try to get a termination GPIO if RS485 bus
    > > termination is supported by the driver.
    > [...]
    > > --- a/drivers/tty/serial/serial_core.c
    > > +++ b/drivers/tty/serial/serial_core.c
    > > @@ -3384,17 +3384,20 @@ int uart_get_rs485_mode(struct uart_port *port)
    > > rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
    > > }
    > >
    > > - /*
    > > - * Disabling termination by default is the safe choice: Else if many
    > > - * bus participants enable it, no communication is possible at all.
    > > - * Works fine for short cables and users may enable for longer cables.
    > > - */
    > > - port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term",
    > > - GPIOD_OUT_LOW);
    > > - if (IS_ERR(port->rs485_term_gpio)) {
    > > - ret = PTR_ERR(port->rs485_term_gpio);
    > > - port->rs485_term_gpio = NULL;
    > > - return dev_err_probe(dev, ret, "Cannot get rs485-term-gpios\n");
    > > + if (port->rs485_supported->flags & SER_RS485_TERMINATE_BUS) {
    >
    > So I think linux-next commit be2e2cb1d281 ("serial: Sanitize rs485_struct")
    > contains a mistake in that it forces drivers to set SER_RS485_TERMINATE_BUS
    > in their rs485_supported->flags to allow enabling bus termination.
    >
    > That's wrong because *every* rs485-capable driver can enable bus
    > termination if a GPIO has been defined for that in the DT.

    Do you mean every em485 using driver? Otherwise I don't see this "forces
    drivers to set" happening anywhere in the code?

    You're partially right because there are other bugs in this area such
    as the one you propose a fix below. While I was making the sanitization
    series, I entirely missed some parts related to termination because
    SER_RS485_TERMINATE_BUS is seemingly not set/handled correctly by the
    core.

    Another thing that looks a bug is that on subsequent call to TIOCSRS485,
    w/o SER_RS485_TERMINATE_BUS nothing happens (for non-em485 driver, that
    is)? It seems to be taken care by 2/8 of this series though, I think. But
    it should be properly marked as Fixes: ... in that case although nobody
    has complained about it so likely not a huge issue to anyone.

    > In fact, another commit which was applied as part of the same series,
    > ebe2cf736a04 ("serial: pl011: Fill in rs485_supported") does not set
    > SER_RS485_TERMINATE_BUS in amba-pl011.c's flags and thus forbids the
    > driver from enabling bus termination, even though we know there are
    > products out there which support bus termination on the pl011 through
    > a GPIO (Revolution Pi RevPi Compact, Revpi Flat).
    >
    > I think what you want to do is amend uart_get_rs485_mode() to set
    > SER_RS485_TERMINATE_BUS in port->rs485_supported_flags if a GPIO
    > was found in the DT. Instead of the change proposed above.

    That seems appropriate (and is a fix).

    What makes it a bit complicated though is that it's a pointer currently
    and what it points to is shared per driver (besides being const):
    const struct serial_rs485 *rs485_supported;
    While it could be embedded into uart_port, there's the .padding which we
    might not want to bloat uart_port with. Perhaps create non-uapi struct
    kserial_rs485 w/o .padding and add static_assert()s to ensure the
    layout is identical to serial_rs485?


    --
    i.

    \
     
     \ /
      Last update: 2022-06-27 11:06    [W:2.544 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site