Re: [PATCH v2 5/5] tty/serial/8250: use mctrl_gpio helpers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 22, 2016 at 2:12 PM,  <yegorslists@xxxxxxxxxxxxxx> wrote:
> From: Yegor Yefremov <yegorslists@xxxxxxxxxxxxxx>
>
> This patch permits the usage fo GPIOs to control the CTS/RTS/DTR/DSR/DCD/RI
> signals.
>
> Signed-off-by: Yegor Yefremov <yegorslists@xxxxxxxxxxxxxx>
> ---
> hanges:
>         v2: - use newly introduced serial8250_in_MCR/serial8250_out_MCR
>             - don't use autoRTS/CTS, if RTS pin is made via GPIO
>
>  Documentation/devicetree/bindings/serial/8250.txt | 19 ++++++++++
>  drivers/tty/serial/8250/8250.h                    | 43 ++++++++++++++++++++++-
>  drivers/tty/serial/8250/8250_core.c               |  4 +++
>  drivers/tty/serial/8250/8250_omap.c               | 31 +++++++++-------
>  drivers/tty/serial/8250/8250_port.c               |  7 +++-
>  drivers/tty/serial/8250/Kconfig                   |  1 +
>  include/linux/serial_8250.h                       |  1 +
>  7 files changed, 91 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
> index 936ab5b..f5561ac 100644
> --- a/Documentation/devicetree/bindings/serial/8250.txt
> +++ b/Documentation/devicetree/bindings/serial/8250.txt
> @@ -42,6 +42,9 @@ Optional properties:
>  - auto-flow-control: one way to enable automatic flow control support. The
>    driver is allowed to detect support for the capability even without this
>    property.
> +- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
> +  line respectively. It will use specified GPIO instead of the peripheral
> +  function pin for the UART feature. If unsure, don't specify this property.
>
>  Note:
>  * fsl,ns16550:
> @@ -63,3 +66,19 @@ Example:
>                 interrupts = <10>;
>                 reg-shift = <2>;
>         };
> +
> +Example for OMAP UART using GPIO-based modem control signals:
> +
> +       uart4: serial@49042000 {
> +               compatible = "ti,omap3-uart";
> +               reg = <0x49042000 0x400>;
> +               interrupts = <80>;
> +               ti,hwmods = "uart4";
> +               clock-frequency = <48000000>;
> +               cts-gpios = <&gpio3 5 GPIO_ACTIVE_LOW>;
> +               rts-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
> +               dtr-gpios = <&gpio1 12 GPIO_ACTIVE_LOW>;
> +               dsr-gpios = <&gpio1 13 GPIO_ACTIVE_LOW>;
> +               dcd-gpios = <&gpio1 14 GPIO_ACTIVE_LOW>;
> +               rng-gpios = <&gpio1 15 GPIO_ACTIVE_LOW>;
> +       };
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index 73bf57d..8e92baf 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -15,6 +15,8 @@
>  #include <linux/serial_reg.h>
>  #include <linux/dmaengine.h>
>
> +#include "../serial_mctrl_gpio.h"
> +
>  struct uart_8250_dma {
>         int (*tx_dma)(struct uart_8250_port *p);
>         int (*rx_dma)(struct uart_8250_port *p, unsigned int iir);
> @@ -134,12 +136,51 @@ void serial8250_em485_destroy(struct uart_8250_port *p);
>
>  static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
>  {
> +       int mctrl_gpio = 0;
> +
>         serial_out(up, UART_MCR, value);
> +
> +       if (value & UART_MCR_RTS)
> +               mctrl_gpio |= TIOCM_RTS;
> +       if (value & UART_MCR_DTR)
> +               mctrl_gpio |= TIOCM_DTR;
> +       if (value & UART_MCR_OUT1)
> +               mctrl_gpio |= TIOCM_OUT1;
> +       if (value & UART_MCR_OUT2)
> +               mctrl_gpio |= TIOCM_OUT2;
> +
> +       mctrl_gpio_set(up->gpios, mctrl_gpio);

I've got a kernel crash from kernel robot. If we use UART before
general initialization (earlyprintk), then any call to mctrl API would
result in NULL pointer dereference. One solution would be to check, if
gpios IS_ERR_OR_NULL(). See below:

--- a/drivers/tty/serial/serial_mctrl_gpio.c
+++ b/drivers/tty/serial/serial_mctrl_gpio.c
@@ -54,6 +54,9 @@ void mctrl_gpio_set(struct mctrl_gpios *gpios,
unsigned int mctrl)
        int value_array[UART_GPIO_MAX];
        unsigned int count = 0;

+       if (IS_ERR_OR_NULL(gpios))
+               return;
+
        for (i = 0; i < UART_GPIO_MAX; i++)
                if (gpios->gpio[i] && mctrl_gpios_desc[i].dir_out) {
                        desc_array[count] = gpios->gpio[i];

What do you think?

>  }
>
>  static inline int serial8250_in_MCR(struct uart_8250_port *up)
>  {
> -       return serial_in(up, UART_MCR);
> +       int mctrl, mctrl_gpio = 0;
> +
> +       mctrl = serial_in(up, UART_MCR);
> +
> +       mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, &mctrl_gpio);
> +
> +       if (mctrl_gpio & TIOCM_RTS)
> +               mctrl |= UART_MCR_RTS;
> +       else
> +               mctrl &= ~UART_MCR_RTS;
> +
> +       if (mctrl_gpio & TIOCM_DTR)
> +               mctrl |= UART_MCR_DTR;
> +       else
> +               mctrl &= ~UART_MCR_DTR;
> +
> +       if (mctrl_gpio & TIOCM_OUT1)
> +               mctrl |= UART_MCR_OUT1;
> +       else
> +               mctrl &= ~UART_MCR_OUT1;
> +
> +       if (mctrl_gpio & TIOCM_OUT2)
> +               mctrl |= UART_MCR_OUT2;
> +       else
> +               mctrl &= ~UART_MCR_OUT2;

Should we perhaps check, if particular signal is really implemented as GPIO?

> +       return mctrl;
>  }
>
>  #if defined(__alpha__) && !defined(CONFIG_PCI)
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 2f4f5ee..bfde589 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1010,6 +1010,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>                 if (up->port.flags & UPF_FIXED_TYPE)
>                         uart->port.type = up->port.type;
>
> +               uart->gpios = mctrl_gpio_init(&uart->port, 0);
> +               if (IS_ERR(uart->gpios))
> +                       dev_dbg(uart->port.dev, "Failed to init mctrl_gpio\n");
> +
>                 serial8250_set_defaults(uart);
>
>                 /* Possibly override default I/O functions.  */
> diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
> index d8bd8ce..d4e7663 100644
> --- a/drivers/tty/serial/8250/8250_omap.c
> +++ b/drivers/tty/serial/8250/8250_omap.c
> @@ -128,18 +128,21 @@ static void omap8250_set_mctrl(struct uart_port *port, unsigned int mctrl)
>
>         serial8250_do_set_mctrl(port, mctrl);
>
> -       /*
> -        * Turn off autoRTS if RTS is lowered and restore autoRTS setting
> -        * if RTS is raised
> -        */
> -       lcr = serial_in(up, UART_LCR);
> -       serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
> -       if ((mctrl & TIOCM_RTS) && (port->status & UPSTAT_AUTORTS))
> -               priv->efr |= UART_EFR_RTS;
> -       else
> -               priv->efr &= ~UART_EFR_RTS;
> -       serial_out(up, UART_EFR, priv->efr);
> -       serial_out(up, UART_LCR, lcr);
> +       if (IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(up->gpios,
> +                                               UART_GPIO_RTS))) {
> +               /*
> +                * Turn off autoRTS if RTS is lowered and restore autoRTS
> +                * setting if RTS is raised
> +                */
> +               lcr = serial_in(up, UART_LCR);
> +               serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
> +               if ((mctrl & TIOCM_RTS) && (port->status & UPSTAT_AUTORTS))
> +                       priv->efr |= UART_EFR_RTS;
> +               else
> +                       priv->efr &= ~UART_EFR_RTS;
> +               serial_out(up, UART_EFR, priv->efr);
> +               serial_out(up, UART_LCR, lcr);
> +       }
>  }
>
>  /*
> @@ -440,7 +443,9 @@ static void omap_8250_set_termios(struct uart_port *port,
>         priv->efr = 0;
>         up->port.status &= ~(UPSTAT_AUTOCTS | UPSTAT_AUTORTS | UPSTAT_AUTOXOFF);
>
> -       if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW) {
> +       if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW
> +               && IS_ERR_OR_NULL(mctrl_gpio_to_gpiod(up->gpios,
> +                                                       UART_GPIO_RTS))) {
>                 /* Enable AUTOCTS (autoRTS is enabled when RTS is raised) */
>                 up->port.status |= UPSTAT_AUTOCTS | UPSTAT_AUTORTS;
>                 priv->efr |= UART_EFR_CTS;
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 094763b..bdf2dc4 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1611,6 +1611,8 @@ static void serial8250_disable_ms(struct uart_port *port)
>         if (up->bugs & UART_BUG_NOMSR)
>                 return;
>
> +       mctrl_gpio_disable_ms(up->gpios);
> +
>         up->ier &= ~UART_IER_MSI;
>         serial_port_out(port, UART_IER, up->ier);
>  }
> @@ -1623,6 +1625,8 @@ static void serial8250_enable_ms(struct uart_port *port)
>         if (up->bugs & UART_BUG_NOMSR)
>                 return;
>
> +       mctrl_gpio_enable_ms(up->gpios);
> +
>         up->ier |= UART_IER_MSI;
>
>         serial8250_rpm_get(up);
> @@ -1900,7 +1904,8 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port)
>                 ret |= TIOCM_DSR;
>         if (status & UART_MSR_CTS)
>                 ret |= TIOCM_CTS;
> -       return ret;
> +
> +       return mctrl_gpio_get(up->gpios, &ret);
>  }
>
>  void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 64742a0..74cf227 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -6,6 +6,7 @@
>  config SERIAL_8250
>         tristate "8250/16550 and compatible serial support"
>         select SERIAL_CORE
> +       select SERIAL_MCTRL_GPIO if GPIOLIB
>         ---help---
>           This selects whether you want to include the driver for the standard
>           serial ports.  The standard answer is Y.  People who might say N
> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index 4348797..3148d6d 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -110,6 +110,7 @@ struct uart_8250_port {
>                                                  *   if no_console_suspend
>                                                  */
>         unsigned char           probe;
> +       struct mctrl_gpios      *gpios;
>  #define UART_PROBE_RSA (1 << 0)
>
>         /*
> --
> 2.1.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux