mbox series

[0/5] rs485 properties: platform agnosticism + RTS polarity

Message ID cover.1510134353.git.lukas@wunner.de
Headers show
Series rs485 properties: platform agnosticism + RTS polarity | expand

Message

Lukas Wunner Nov. 8, 2017, 10:32 a.m. UTC 
Rework the common helper function for retrieving rs485 properties
to be platform agnostic (patch [1/5]) and to allow specifying the
RTS polarity (patch [2/5] and [3/5]).

Amend the fsl_lpuart and imx drivers to take advantage of this
and fix a bunch of bugs while at it (patch [4/5] and [5/5]).

Note that Michal Oleszczyk posted a patch yesterday which documents
the "rs485-rts-active-high" property supported by omap-serial in a
way which suggests that *all* drivers default to active low, which
is not the case (all other drivers default to active high).
I think the present series is a better approach, but let's have a
technical discussion about that.

I posted an earlier version of patch [1/5] a while back but withdrew
it because I wanted to change uart_get_rs485_mode() to take a
struct dev and a struct serial_rs485, instead of a struct uart_port
(which seemed too fragile on second thought).  That change has been
made now.

Thanks,

Lukas


Lukas Wunner (5):
  serial: Make retrieval of rs485 properties platform-agnostic
  dt-bindings: serial: Add common rs485 binding for RTS polarity
  serial: core: Support common rs485 binding for RTS polarity
  serial: fsl_lpuart: Support common rs485 binding for RTS polarity
  serial: imx: Support common rs485 binding for RTS polarity

 .../devicetree/bindings/serial/fsl-imx-uart.txt    |  3 ++-
 .../devicetree/bindings/serial/fsl-lpuart.txt      |  3 ++-
 .../devicetree/bindings/serial/omap_serial.txt     |  1 +
 Documentation/devicetree/bindings/serial/rs485.txt |  1 +
 drivers/tty/serial/atmel_serial.c                  |  2 +-
 drivers/tty/serial/fsl_lpuart.c                    | 15 ++++----------
 drivers/tty/serial/imx.c                           | 11 ++++++++---
 drivers/tty/serial/omap-serial.c                   | 11 +++++++----
 drivers/tty/serial/serial_core.c                   | 23 +++++++++++++++-------
 include/linux/serial_core.h                        |  3 +--
 10 files changed, 43 insertions(+), 30 deletions(-)

Comments

Uwe Kleine-König Nov. 8, 2017, 11:13 a.m. UTC | #1 
On Wed, Nov 08, 2017 at 11:32:34AM +0100, Lukas Wunner wrote:
> When a driver invokes the uart_get_rs485_mode() helper, set the RTS
> polarity to active high by default unless the newly introduced
> "rs485-rts-active-low" property was specified.
> 
> omap-serial historically defaults to active low and supports an
> "rs485-rts-active-high" property to inverse the polarity.  Retain that
> behavior for compatibility.
> 
> Cc: Mark Jackson <mpfj@newflow.co.uk>
> Cc: Michał Oleszczyk <oleszczyk.m@gmail.com>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/tty/serial/omap-serial.c | 7 +++++--
>  drivers/tty/serial/serial_core.c | 8 ++++++++
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index e6aadb6d02e5..7ef36c0af825 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -1617,10 +1617,13 @@ static int serial_omap_probe_rs485(struct uart_omap_port *up,
>  
>  	uart_get_rs485_mode(up->dev, rs485conf);
>  
> -	if (of_property_read_bool(np, "rs485-rts-active-high"))
> +	if (of_property_read_bool(np, "rs485-rts-active-high")) {
>  		rs485conf->flags |= SER_RS485_RTS_ON_SEND;
> -	else
> +		rs485conf->flags &= ~SER_RS485_RTS_AFTER_SEND;
> +	} else {
> +		rs485conf->flags &= ~SER_RS485_RTS_ON_SEND;
>  		rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
> +	}
>  
>  	/* check for tx enable gpio */
>  	up->rts_gpio = of_get_named_gpio_flags(np, "rts-gpio", 0, &flags);
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 548bb80223c1..64e15507a9bd 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3048,6 +3048,14 @@ void uart_get_rs485_mode(struct device *dev, struct serial_rs485 *rs485conf)
>  		rs485conf->delay_rts_after_send = 0;
>  	}
>  
> +	if (device_property_read_bool(dev, "rs485-rts-active-low")) {
> +		rs485conf->flags &= ~SER_RS485_RTS_ON_SEND;
> +		rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
> +	} else {
> +		rs485conf->flags |= SER_RS485_RTS_ON_SEND;
> +		rs485conf->flags &= ~SER_RS485_RTS_AFTER_SEND;
> +	}
> +

I wonder if it would be easier to understand when making that:

-	rs485conf->flags &= ~(SER_RS485_RX_DURING_TX | SER_RS485_ENABLED);
+	rs485conf->flags &= ~(SER_RS485_RX_DURING_TX | SER_RS485_RTS_AFTER_SEND | SER_RS485_ENABLED);
+	rs485conf->flags |= SER_RS485_RTS_ON_SEND;
...
+	if (device_property_read_bool(dev, "rs485-rts-active-low")) {
+		rs485conf->flags &= ~SER_RS485_RTS_ON_SEND;
+		rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
+	}

(together with the then needed comment updates) which would highlight
that SER_RS485_RTS_ON_SEND is the default.

imx.c has unconditional

	sport->port.rs485.flags |= SER_RS485_RTS_ON_SEND;

that needs to be dropped in this patch.

Best regards
Uwe
Lukas Wunner Nov. 8, 2017, 5:33 p.m. UTC | #2 
On Wed, Nov 08, 2017 at 12:13:55PM +0100, Uwe Kleine-König wrote:
> On Wed, Nov 08, 2017 at 11:32:34AM +0100, Lukas Wunner wrote:
> > When a driver invokes the uart_get_rs485_mode() helper, set the RTS
> > polarity to active high by default unless the newly introduced
> > "rs485-rts-active-low" property was specified.

Thanks a lot Uwe for your review comments, those are good points,
d'accord on all of them.  I've already incorporated all suggested
changes into my local repo but will wait a few days before respinning
to see if there are further comments.

Best,

Lukas
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html