| Message ID | 1571119863-14105-1-git-send-email-akashast@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] tty: serial: qcom_geni_serial: IRQ cleanup | expand |
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
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
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;
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(-)