On Tue, Oct 15, 2019 at 11:41:42AM +0530, Akash Asthana wrote:
> 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.
>
> Signed-off-by: Akash Asthana <akashast@xxxxxxxxxxxxxx>
> ---
> Changes in V3:
> - Address review comments on v2 patch.
That's horribly vague, please be specific as to what you changed.
>
> drivers/tty/serial/qcom_geni_serial.c | 44 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 12dc007..bc828e49 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -14,6 +14,7 @@
> #include <linux/of.h>
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> +#include <linux/pm_wakeirq.h>
> #include <linux/qcom-geni-se.h>
> #include <linux/serial.h>
> #include <linux/serial_core.h>
> @@ -116,6 +117,7 @@ struct qcom_geni_serial_port {
> bool brk;
>
> unsigned int tx_remaining;
> + int wakeup_irq;
> };
>
> static const struct uart_ops qcom_geni_console_pops;
> @@ -755,6 +757,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, 2000);
> +
> + return IRQ_HANDLED;
> +}
> +
> static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
> {
> u32 m_irq_en;
> @@ -1310,6 +1321,29 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> return ret;
> }
>
> + if (!console) {
> + port->wakeup_irq = platform_get_irq_optional(pdev, 1);
> + if (port->wakeup_irq < 0) {
> + dev_err(&pdev->dev, "Failed to get wakeup IRQ %d\n",
> + port->wakeup_irq);
Is this error message needed?
> + } else {
> + 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);
Same here.
> + return ret;
> + }
> +
> + device_init_wakeup(&pdev->dev, true);
> + ret = dev_pm_set_wake_irq(&pdev->dev, port->wakeup_irq);
> + if (unlikely(ret))
ONLY ever use likely/unlikely if you can benchmark the difference.
Otherwise don't use it, as you will get it wrong and cpus and compliers
know how to do this much better than we do.
> + dev_err(uport->dev, "%s:Failed to set IRQ wake:%d\n",
> + __func__, ret);
Again, is this needed? And why the function name if it is, the other
error messages you created didn't have that in it.
Consistancy is key.
thanks,
greg k-h
