linux-serial.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
@ 2019-11-05  9:57 Akash Asthana
  2019-11-05 17:17 ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Akash Asthana @ 2019-11-05  9:57 UTC (permalink / raw)
  To: gregkh; +Cc: linux-arm-msm, linux-serial, mgautam, swboyd, 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 v4:
 - As per Stephen's comment, move ISR registration(later in probe) after
   registering uart port with serial core.
 - As per Greg's comment, corrected returning of PTR value from integer type
   function(probe).

Changes in v3:
 - As per Stephen's comment, using devm_kasprintf instead of scnprintf API.

 drivers/tty/serial/qcom_geni_serial.c | 38 ++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 14c6306..634054a 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,6 +1291,12 @@ 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 -ENOMEM;
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;
@@ -1307,7 +1307,21 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
 	port->handle_rx = console ? handle_rx_console : handle_rx_uart;
 	if (!console)
 		device_create_file(uport->dev, &dev_attr_loopback);
-	return uart_add_one_port(drv, uport);
+
+	ret = uart_add_one_port(drv, uport);
+	if (ret)
+		return ret;
+
+	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);
+		uart_remove_one_port(drv, uport);
+		return ret;
+	}
+
+	return ret;
 }
 
 static int qcom_geni_serial_remove(struct platform_device *pdev)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
  2019-11-05  9:57 [PATCH v4 1/2] tty: serial: qcom_geni_serial: IRQ cleanup Akash Asthana
@ 2019-11-05 17:17 ` Greg KH
  2019-11-06  1:00   ` Stephen Boyd
  0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2019-11-05 17:17 UTC (permalink / raw)
  To: Akash Asthana; +Cc: linux-arm-msm, linux-serial, mgautam, swboyd

On Tue, Nov 05, 2019 at 03:27:15PM +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 v4:
>  - As per Stephen's comment, move ISR registration(later in probe) after
>    registering uart port with serial core.
>  - As per Greg's comment, corrected returning of PTR value from integer type
>    function(probe).
> 
> Changes in v3:
>  - As per Stephen's comment, using devm_kasprintf instead of scnprintf API.
> 
>  drivers/tty/serial/qcom_geni_serial.c | 38 ++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 14c6306..634054a 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,6 +1291,12 @@ 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 -ENOMEM;
> +
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
> @@ -1307,7 +1307,21 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  	port->handle_rx = console ? handle_rx_console : handle_rx_uart;
>  	if (!console)
>  		device_create_file(uport->dev, &dev_attr_loopback);
> -	return uart_add_one_port(drv, uport);
> +
> +	ret = uart_add_one_port(drv, uport);
> +	if (ret)
> +		return ret;

What is going to remove the sysfs file you just created above (in a racy
way, it's broken and needs to be fixed, but that's a different issue
here...)?


> +
> +	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);
> +		uart_remove_one_port(drv, uport);
> +		return ret;

Does this remove the sysfs file?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
  2019-11-05 17:17 ` Greg KH
@ 2019-11-06  1:00   ` Stephen Boyd
  2019-11-06 12:16     ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2019-11-06  1:00 UTC (permalink / raw)
  To: Akash Asthana, Greg KH; +Cc: linux-arm-msm, linux-serial, mgautam

Quoting Greg KH (2019-11-05 09:17:05)
> On Tue, Nov 05, 2019 at 03:27:15PM +0530, Akash Asthana wrote:
> > @@ -1307,7 +1307,21 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> >       port->handle_rx = console ? handle_rx_console : handle_rx_uart;
> >       if (!console)
> >               device_create_file(uport->dev, &dev_attr_loopback);
> > -     return uart_add_one_port(drv, uport);
> > +
> > +     ret = uart_add_one_port(drv, uport);
> > +     if (ret)
> > +             return ret;
> 
> What is going to remove the sysfs file you just created above (in a racy
> way, it's broken and needs to be fixed, but that's a different issue
> here...)?
> 
> 
> > +
> > +     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);
> > +             uart_remove_one_port(drv, uport);
> > +             return ret;
> 
> Does this remove the sysfs file?
> 

The loopback file isn't documented. It isn't removed when the driver is
removed either. Can we just remove the whole thing? It would be nicer if
that sort of thing was supported in the tty layer somehow. Is it?


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
  2019-11-06  1:00   ` Stephen Boyd
@ 2019-11-06 12:16     ` Greg KH
  2019-11-06 16:33       ` Stephen Boyd
  0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2019-11-06 12:16 UTC (permalink / raw)
  To: Stephen Boyd; +Cc: Akash Asthana, linux-arm-msm, linux-serial, mgautam

On Tue, Nov 05, 2019 at 05:00:12PM -0800, Stephen Boyd wrote:
> Quoting Greg KH (2019-11-05 09:17:05)
> > On Tue, Nov 05, 2019 at 03:27:15PM +0530, Akash Asthana wrote:
> > > @@ -1307,7 +1307,21 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> > >       port->handle_rx = console ? handle_rx_console : handle_rx_uart;
> > >       if (!console)
> > >               device_create_file(uport->dev, &dev_attr_loopback);
> > > -     return uart_add_one_port(drv, uport);
> > > +
> > > +     ret = uart_add_one_port(drv, uport);
> > > +     if (ret)
> > > +             return ret;
> > 
> > What is going to remove the sysfs file you just created above (in a racy
> > way, it's broken and needs to be fixed, but that's a different issue
> > here...)?
> > 
> > 
> > > +
> > > +     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);
> > > +             uart_remove_one_port(drv, uport);
> > > +             return ret;
> > 
> > Does this remove the sysfs file?
> > 
> 
> The loopback file isn't documented. It isn't removed when the driver is
> removed either. Can we just remove the whole thing? It would be nicer if
> that sort of thing was supported in the tty layer somehow. Is it?

I don't know what that file does, so yes, please delete it :)

And as for support in the tty layer itself, if you figure out what it
does, sure, we can add support if needed.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
  2019-11-06 12:16     ` Greg KH
@ 2019-11-06 16:33       ` Stephen Boyd
  2019-11-11  7:05         ` Akash Asthana
  0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2019-11-06 16:33 UTC (permalink / raw)
  To: Greg KH; +Cc: Akash Asthana, linux-arm-msm, linux-serial, mgautam

Quoting Greg KH (2019-11-06 04:16:00)
> On Tue, Nov 05, 2019 at 05:00:12PM -0800, Stephen Boyd wrote:
> > Quoting Greg KH (2019-11-05 09:17:05)
> > > On Tue, Nov 05, 2019 at 03:27:15PM +0530, Akash Asthana wrote:
> > > > @@ -1307,7 +1307,21 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
> > > >       port->handle_rx = console ? handle_rx_console : handle_rx_uart;
> > > >       if (!console)
> > > >               device_create_file(uport->dev, &dev_attr_loopback);
> > > > -     return uart_add_one_port(drv, uport);
> > > > +
> > > > +     ret = uart_add_one_port(drv, uport);
> > > > +     if (ret)
> > > > +             return ret;
> > > 
> > > What is going to remove the sysfs file you just created above (in a racy
> > > way, it's broken and needs to be fixed, but that's a different issue
> > > here...)?
> > > 
> > > 
> > > > +
> > > > +     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);
> > > > +             uart_remove_one_port(drv, uport);
> > > > +             return ret;
> > > 
> > > Does this remove the sysfs file?
> > > 
> > 
> > The loopback file isn't documented. It isn't removed when the driver is
> > removed either. Can we just remove the whole thing? It would be nicer if
> > that sort of thing was supported in the tty layer somehow. Is it?
> 
> I don't know what that file does, so yes, please delete it :)
> 
> And as for support in the tty layer itself, if you figure out what it
> does, sure, we can add support if needed.
> 

I think it may be supported in the tty layer with TIOCM_LOOP already.
Akash, can you confirm?


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] tty: serial: qcom_geni_serial: IRQ cleanup
  2019-11-06 16:33       ` Stephen Boyd
@ 2019-11-11  7:05         ` Akash Asthana
  0 siblings, 0 replies; 6+ messages in thread
From: Akash Asthana @ 2019-11-11  7:05 UTC (permalink / raw)
  To: Stephen Boyd, Greg KH; +Cc: linux-arm-msm, linux-serial, mgautam


On 11/6/2019 10:03 PM, Stephen Boyd wrote:
Thanks for reviewing.
> Quoting Greg KH (2019-11-06 04:16:00)
>> On Tue, Nov 05, 2019 at 05:00:12PM -0800, Stephen Boyd wrote:
>>> Quoting Greg KH (2019-11-05 09:17:05)
>>>> On Tue, Nov 05, 2019 at 03:27:15PM +0530, Akash Asthana wrote:
>>>>> @@ -1307,7 +1307,21 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>>>>>        port->handle_rx = console ? handle_rx_console : handle_rx_uart;
>>>>>        if (!console)
>>>>>                device_create_file(uport->dev, &dev_attr_loopback);
>>>>> -     return uart_add_one_port(drv, uport);
>>>>> +
>>>>> +     ret = uart_add_one_port(drv, uport);
>>>>> +     if (ret)
>>>>> +             return ret;
>>>> What is going to remove the sysfs file you just created above (in a racy
>>>> way, it's broken and needs to be fixed, but that's a different issue
>>>> here...)?
>>>>
>>>>
>>>>> +
>>>>> +     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);
>>>>> +             uart_remove_one_port(drv, uport);
>>>>> +             return ret;
>>>> Does this remove the sysfs file?
>>>>
>>> The loopback file isn't documented. It isn't removed when the driver is
>>> removed either. Can we just remove the whole thing? It would be nicer if
>>> that sort of thing was supported in the tty layer somehow. Is it?
>> I don't know what that file does, so yes, please delete it :)

This sysfs file was exposed to user to configure hardware to run in 
loopback mode (sorting TX and RX lines).

We use this mode to sanity test TX and RX path of the driver after 
making any code changes.

But we can remove this sysfs file and use TIOCM_LOOP to set HW in 
loopback mode as suggested by stephen.

>> And as for support in the tty layer itself, if you figure out what it
>> does, sure, we can add support if needed.
>>
> I think it may be supported in the tty layer with TIOCM_LOOP already.
> Akash, can you confirm?

Yeah, its already supported in tty layer. We can call TIOCMSET ioctl 
from user space test application with TIOCM_LOOP argument to configure 
our HW in loopback mode.

I will remove all the code related to loopback sysfs.

Thanks for this suggestion.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-11  7:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-05  9:57 [PATCH v4 1/2] tty: serial: qcom_geni_serial: IRQ cleanup Akash Asthana
2019-11-05 17:17 ` Greg KH
2019-11-06  1:00   ` Stephen Boyd
2019-11-06 12:16     ` Greg KH
2019-11-06 16:33       ` Stephen Boyd
2019-11-11  7:05         ` 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).