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

[Akash Asthana <akashast@codeaurora.org>]

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