Re: [PATCH] tty: serial: qcom_geni_serial: Wakeup over UART RX

[Stephen Boyd <swboyd@chromium.org>]

Quoting Akash Asthana (2019-09-19 00:16:06)
> Add system wakeup capability over UART RX line for wakeup capable UART.
> When system is suspended, RX line act as an interrupt to wakeup system
> for any communication requests from peer.
> 
> Cleanup of IRQ registration, moving it to probe from startup function.

Probably should be a different patch than the one that adds wakeup irq
handling.

> 
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 73 +++++++++++++++++++++++++++++------
>  1 file changed, 62 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 35e5f9c..43d1da4 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -19,6 +19,8 @@
>  #include <linux/slab.h>
>  #include <linux/tty.h>
>  #include <linux/tty_flip.h>
> +#include <linux/pm_wakeirq.h>
> +#include <linux/irq.h>

Can you sort these includes alphabetically? Or at least attempt to place
this somewhere in there alphabetically?

>  
>  /* UART specific GENI registers */
>  #define SE_UART_LOOPBACK_CFG           0x22c
> @@ -98,6 +100,8 @@
>  #define CONSOLE_RX_BYTES_PW 4
>  #endif
>  
> +#define WAKEUP_EVENT_MSEC   2000

This is used one place. Just inline it and drop this define.

> +
>  struct qcom_geni_serial_port {
>         struct uart_port uport;
>         struct geni_se se;
> @@ -115,6 +119,7 @@ struct qcom_geni_serial_port {
>         bool brk;
>  
>         unsigned int tx_remaining;
> +       int wakeup_irq;
>  };
>  
>  static const struct uart_ops qcom_geni_console_pops;
> @@ -756,6 +761,15 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport, bool done,
>                 uart_write_wakeup(uport);
>  }
>  
> +static irqreturn_t qcom_geni_serial_wakeup_isr(int isr, void *dev)
> +{
> +       struct uart_port *uport = dev;
> +
> +       pm_wakeup_event(uport->dev, WAKEUP_EVENT_MSEC);
> +
> +       return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
>  {
>         u32 m_irq_en;
> @@ -1290,6 +1297,8 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>         port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
>         port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
>  
> +       scnprintf(port->name, sizeof(port->name), "qcom_geni_serial_%s%d",
> +               (uart_console(uport) ? "console" : "uart"), uport->line);

This looks like an unrelated change?

>         irq = platform_get_irq(pdev, 0);
>         if (irq < 0) {
>                 dev_err(&pdev->dev, "Failed to get IRQ %d\n", irq);
> @@ -1297,6 +1306,39 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>         }
>         uport->irq = irq;
>  
> +       irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
> +       ret = devm_request_irq(uport->dev, uport->irq, qcom_geni_serial_isr,
> +                               IRQF_TRIGGER_HIGH, port->name, uport);
> +       if (ret) {
> +               dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
> +               return ret;
> +       }
> +
> +       if (!console) {
> +               port->wakeup_irq = platform_get_irq(pdev, 1);

Can you use dev_pm_set_wake_irq() instead?

> +               if (port->wakeup_irq < 0) {
> +                       dev_err(&pdev->dev, "Failed to get wakeup IRQ %d\n",
> +                                                       port->wakeup_irq);
> +               } else {
> +                       dev_info(&pdev->dev, "wakeup_irq =%d\n",
> +                               port->wakeup_irq);

Please no dev_info() messages like this.

> +                       irq_set_status_flags(port->wakeup_irq, IRQ_NOAUTOEN);
> +                       ret = devm_request_irq(uport->dev, port->wakeup_irq,
> +                               qcom_geni_serial_wakeup_isr,
> +                               IRQF_TRIGGER_FALLING, "uart_wakeup", uport);
> +                       if (ret) {
> +                               dev_err(uport->dev, "Failed to register wakeup "
> +                                       "IRQ ret %d\n", ret);

Don't split format strings across many lines. The arguments can go to a
different line, but the string can be on a single line.

> +                               return ret;
> +                       }
> +
> +                       device_init_wakeup(&pdev->dev, true);
> +                       ret = enable_irq_wake(port->wakeup_irq);
> +                       if (unlikely(ret))
> +                               dev_err(uport->dev, "%s:Failed to set IRQ "
> +                                       "wake:%d\n", __func__, ret);
> +               }
> +       }
>         uport->private_data = drv;
>         platform_set_drvdata(pdev, port);
>         port->handle_rx = console ? handle_rx_console : handle_rx_uart;
> @@ -1311,6 +1353,7 @@ static int qcom_geni_serial_remove(struct platform_device *pdev)
>         struct uart_driver *drv = port->uport.private_data;
>  
>         uart_remove_one_port(drv, &port->uport);
> +
>         return 0;
>  }
>  

This hunk shouldn't be here. Please drop