* [PATCH v3 2/2] tty: serial: qcom_geni_serial: Wakeup over UART RX
@ 2019-10-15 6:11 Akash Asthana
2019-10-15 18:59 ` Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Akash Asthana @ 2019-10-15 6:11 UTC (permalink / raw)
To: gregkh
Cc: linux-arm-msm, linux-serial, mgautam, bjorn.andersson, swboyd,
msavaliy, Akash Asthana
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@codeaurora.org>
---
Changes in V3:
- Address review comments on v2 patch.
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);
+ } 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);
+ return ret;
+ }
+
+ device_init_wakeup(&pdev->dev, true);
+ ret = dev_pm_set_wake_irq(&pdev->dev, 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;
@@ -1332,7 +1366,12 @@ static int __maybe_unused qcom_geni_serial_sys_suspend(struct device *dev)
struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
struct uart_port *uport = &port->uport;
- return uart_suspend_port(uport->private_data, uport);
+ uart_suspend_port(uport->private_data, uport);
+
+ if (port->wakeup_irq > 0)
+ enable_irq(port->wakeup_irq);
+
+ return 0;
}
static int __maybe_unused qcom_geni_serial_sys_resume(struct device *dev)
@@ -1340,6 +1379,9 @@ static int __maybe_unused qcom_geni_serial_sys_resume(struct device *dev)
struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
struct uart_port *uport = &port->uport;
+ if (port->wakeup_irq > 0)
+ disable_irq(port->wakeup_irq);
+
return uart_resume_port(uport->private_data, uport);
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v3 2/2] tty: serial: qcom_geni_serial: Wakeup over UART RX
2019-10-15 6:11 [PATCH v3 2/2] tty: serial: qcom_geni_serial: Wakeup over UART RX Akash Asthana
@ 2019-10-15 18:59 ` Greg KH
2019-10-17 11:09 ` Akash Asthana
0 siblings, 1 reply; 3+ messages in thread
From: Greg KH @ 2019-10-15 18:59 UTC (permalink / raw)
To: Akash Asthana
Cc: linux-arm-msm, linux-serial, mgautam, bjorn.andersson, swboyd, msavaliy
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@codeaurora.org>
> ---
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v3 2/2] tty: serial: qcom_geni_serial: Wakeup over UART RX
2019-10-15 18:59 ` Greg KH
@ 2019-10-17 11:09 ` Akash Asthana
0 siblings, 0 replies; 3+ messages in thread
From: Akash Asthana @ 2019-10-17 11:09 UTC (permalink / raw)
To: Greg KH
Cc: linux-arm-msm, linux-serial, mgautam, bjorn.andersson, swboyd, msavaliy
On 10/16/2019 12:29 AM, Greg KH wrote:
> 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@codeaurora.org>
>> ---
>> Changes in V3:
>> - Address review comments on v2 patch.
> That's horribly vague, please be specific as to what you changed.
Ok, I will take care of this in future.
>
>> 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?
I will remove this.
>> + } 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.
devm_request_irq function doesn't print error for every failure paths.
>
>> + 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.
Ok, I will remove it.
>> + 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.
I will remove this.
> Consistancy is key.
>
> thanks,
>
> greg k-h
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-10-17 11:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-15 6:11 [PATCH v3 2/2] tty: serial: qcom_geni_serial: Wakeup over UART RX Akash Asthana
2019-10-15 18:59 ` Greg KH
2019-10-17 11:09 ` Akash Asthana
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).