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@xxxxxxxxxxxxxx>
> ---
> 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
