* [PATCH v3 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
@ 2019-10-15 6:11 Akash Asthana
2019-10-15 18:58 ` 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
Move ISR registration from startup to probe function to avoid registering
it everytime when the port open is called for driver.
Signed-off-by: Akash Asthana <akashast@codeaurora.org>
---
Changes in v3:
- Address review comments on v2 patch.
- Using devm_kasprintf instead of scnprintf API.
drivers/tty/serial/qcom_geni_serial.c | 30 +++++++++++++++++++-----------
1 file changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 14c6306..12dc007 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -9,6 +9,7 @@
#include <linux/console.h>
#include <linux/io.h>
#include <linux/iopoll.h>
+#include <linux/irq.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
@@ -101,7 +102,7 @@
struct qcom_geni_serial_port {
struct uart_port uport;
struct geni_se se;
- char name[20];
+ const char *name;
u32 tx_fifo_depth;
u32 tx_fifo_width;
u32 rx_fifo_depth;
@@ -830,7 +831,7 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport)
if (uart_console(uport))
console_stop(uport->cons);
- free_irq(uport->irq, uport);
+ disable_irq(uport->irq);
spin_lock_irqsave(&uport->lock, flags);
qcom_geni_serial_stop_tx(uport);
qcom_geni_serial_stop_rx(uport);
@@ -890,21 +891,14 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
int ret;
struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
- scnprintf(port->name, sizeof(port->name),
- "qcom_serial_%s%d",
- (uart_console(uport) ? "console" : "uart"), uport->line);
-
if (!port->setup) {
ret = qcom_geni_serial_port_setup(uport);
if (ret)
return ret;
}
+ enable_irq(uport->irq);
- ret = request_irq(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;
+ return 0;
}
static unsigned long get_clk_cfg(unsigned long clk_freq)
@@ -1297,11 +1291,25 @@ 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;
+ port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
+ "qcom_geni_serial_%s%d",
+ uart_console(uport) ? "console" : "uart", uport->line);
+ if (!port->name)
+ return ERR_PTR(-ENOMEM);
+
irq = platform_get_irq(pdev, 0);
if (irq < 0)
return irq;
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;
+ }
+
uport->private_data = drv;
platform_set_drvdata(pdev, port);
port->handle_rx = console ? handle_rx_console : handle_rx_uart;
--
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 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
2019-10-15 6:11 [PATCH v3 1/2] tty: serial: qcom_geni_serial: IRQ cleanup Akash Asthana
@ 2019-10-15 18:58 ` Greg KH
2019-10-17 11:08 ` Akash Asthana
0 siblings, 1 reply; 3+ messages in thread
From: Greg KH @ 2019-10-15 18:58 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:03AM +0530, Akash Asthana wrote:
> Move ISR registration from startup to probe function to avoid registering
> it everytime when the port open is called for driver.
>
> Signed-off-by: Akash Asthana <akashast@codeaurora.org>
> ---
> Changes in v3:
> - Address review comments on v2 patch.
> - Using devm_kasprintf instead of scnprintf API.
>
> drivers/tty/serial/qcom_geni_serial.c | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 14c6306..12dc007 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -9,6 +9,7 @@
> #include <linux/console.h>
> #include <linux/io.h>
> #include <linux/iopoll.h>
> +#include <linux/irq.h>
> #include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> @@ -101,7 +102,7 @@
> struct qcom_geni_serial_port {
> struct uart_port uport;
> struct geni_se se;
> - char name[20];
> + const char *name;
> u32 tx_fifo_depth;
> u32 tx_fifo_width;
> u32 rx_fifo_depth;
> @@ -830,7 +831,7 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport)
> if (uart_console(uport))
> console_stop(uport->cons);
>
> - free_irq(uport->irq, uport);
> + disable_irq(uport->irq);
> spin_lock_irqsave(&uport->lock, flags);
> qcom_geni_serial_stop_tx(uport);
> qcom_geni_serial_stop_rx(uport);
> @@ -890,21 +891,14 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
> int ret;
> struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>
> - scnprintf(port->name, sizeof(port->name),
> - "qcom_serial_%s%d",
> - (uart_console(uport) ? "console" : "uart"), uport->line);
> -
> if (!port->setup) {
> ret = qcom_geni_serial_port_setup(uport);
> if (ret)
> return ret;
> }
> + enable_irq(uport->irq);
>
> - ret = request_irq(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;
> + return 0;
> }
>
> static unsigned long get_clk_cfg(unsigned long clk_freq)
> @@ -1297,11 +1291,25 @@ 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;
>
> + port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
> + "qcom_geni_serial_%s%d",
> + uart_console(uport) ? "console" : "uart", uport->line);
> + if (!port->name)
> + return ERR_PTR(-ENOMEM);
Why are you returning a pointer when the return type of this function is
int? Did the compiler not complain? Shouldn't this just be -ENOMEM?
> +
> irq = platform_get_irq(pdev, 0);
> if (irq < 0)
> return irq;
> 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);
Why print this out? Doesn't the function print an error if it fails?
> + return ret;
See, an int return value :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
2019-10-15 18:58 ` Greg KH
@ 2019-10-17 11:08 ` Akash Asthana
0 siblings, 0 replies; 3+ messages in thread
From: Akash Asthana @ 2019-10-17 11:08 UTC (permalink / raw)
To: Greg KH
Cc: linux-arm-msm, linux-serial, mgautam, bjorn.andersson, swboyd, msavaliy
On 10/16/2019 12:28 AM, Greg KH wrote:
> On Tue, Oct 15, 2019 at 11:41:03AM +0530, Akash Asthana wrote:
>> Move ISR registration from startup to probe function to avoid registering
>> it everytime when the port open is called for driver.
>>
>> Signed-off-by: Akash Asthana<akashast@codeaurora.org>
>> ---
>> Changes in v3:
>> - Address review comments on v2 patch.
>> - Using devm_kasprintf instead of scnprintf API.
>>
>> drivers/tty/serial/qcom_geni_serial.c | 30 +++++++++++++++++++-----------
>> 1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
>> index 14c6306..12dc007 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -9,6 +9,7 @@
>> #include <linux/console.h>
>> #include <linux/io.h>
>> #include <linux/iopoll.h>
>> +#include <linux/irq.h>
>> #include <linux/module.h>
>> #include <linux/of.h>
>> #include <linux/of_device.h>
>> @@ -101,7 +102,7 @@
>> struct qcom_geni_serial_port {
>> struct uart_port uport;
>> struct geni_se se;
>> - char name[20];
>> + const char *name;
>> u32 tx_fifo_depth;
>> u32 tx_fifo_width;
>> u32 rx_fifo_depth;
>> @@ -830,7 +831,7 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport)
>> if (uart_console(uport))
>> console_stop(uport->cons);
>>
>> - free_irq(uport->irq, uport);
>> + disable_irq(uport->irq);
>> spin_lock_irqsave(&uport->lock, flags);
>> qcom_geni_serial_stop_tx(uport);
>> qcom_geni_serial_stop_rx(uport);
>> @@ -890,21 +891,14 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
>> int ret;
>> struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
>>
>> - scnprintf(port->name, sizeof(port->name),
>> - "qcom_serial_%s%d",
>> - (uart_console(uport) ? "console" : "uart"), uport->line);
>> -
>> if (!port->setup) {
>> ret = qcom_geni_serial_port_setup(uport);
>> if (ret)
>> return ret;
>> }
>> + enable_irq(uport->irq);
>>
>> - ret = request_irq(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;
>> + return 0;
>> }
>>
>> static unsigned long get_clk_cfg(unsigned long clk_freq)
>> @@ -1297,11 +1291,25 @@ 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;
>>
>> + port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
>> + "qcom_geni_serial_%s%d",
>> + uart_console(uport) ? "console" : "uart", uport->line);
>> + if (!port->name)
>> + return ERR_PTR(-ENOMEM);
> Why are you returning a pointer when the return type of this function is
> int? Did the compiler not complain? Shouldn't this just be -ENOMEM?
Sorry about it! I will take care of this in future. I missed it due to
compiler setting, every warning was not treated as error.
>> +
>> irq = platform_get_irq(pdev, 0);
>> if (irq < 0)
>> return irq;
>> 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);
> Why print this out? Doesn't the function print an error if it fails?
The function doesn't print error for every failure paths.
>> + return ret;
> See, an int return value :)
>
> 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:08 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 1/2] tty: serial: qcom_geni_serial: IRQ cleanup Akash Asthana
2019-10-15 18:58 ` Greg KH
2019-10-17 11:08 ` 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).