* Re: [PATCH] tty: serial: qcom_geni_serial: Wakeup over UART RX
[not found] ` <5d83c725.1c69fb81.9e57a.5569@mx.google.com>
@ 2019-10-10 8:44 ` akashast
0 siblings, 0 replies; only message in thread
From: akashast @ 2019-10-10 8:44 UTC (permalink / raw)
To: Stephen Boyd
Cc: agross, gregkh, linux-arm-msm, linux-serial, mgautam, jslaby,
bjorn.andersson, linux-arm-msm-owner
On 2019-09-19 23:51, Stephen Boyd wrote:
> 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.
>
Sure, will do it.
>>
>> 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?
>
ok
>>
>> /* 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.
>
ok
>> +
>> 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?
>
We moved this change to probe from startup as part of IRQ cleanup. We
are initializing port->name variable here that is used in
devm_request_irq call.
>> 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?
>
okay, we are using this API this will take care of
enable_irq_wake/disable_irq_wake during suspend and resume but still, we
need to enable/disable wakeup irq in suspend and resume call.
>> + 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.
>
ok
>> + 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.
>
ok
>> + 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
ok
^ permalink raw reply [flat|nested] only message in thread