Fixes/cleanup for vt8500 serial driver
Hi Greg,
This is a series of fixes/cleanup for the vt8500 serial/uart driver.
I'm not sure how you want to handle these.
Arguably #1, #2 and #3 are bug fixes which could go into 3.8. None of them cause immediate problems, but potentially could.
#4 is definately a cleanup for 3.9.
Let me know if you would rather I split them up and how.
Regards
Tony P
The following changes since commit 9931faca02c604c22335f5a935a501bb2ace6e20:
Linux 3.8-rc3 (2013-01-09 18:59:55 -0800)
are available in the git repository at:
git://server.prisktech.co.nz/git/linuxwmt.git tags/vt8500-serial-fixes
for you to fetch changes up to 55df4e19676ca7674bcdc5831036ed3f708050e8:
serial: tty: Cleanup code using devm_ function (2013-01-14 05:39:17 +1300)
----------------------------------------------------------------
Series of fixes/cleanups for vt8500 serial/UART driver.
----------------------------------------------------------------
Tony Prisk (4):
serial: vt8500: Fix range-checking on vt8500_uart_ports
serial: vt8500: ioremap'd resource is never freed
serial: vt8500: UART uses gated clock rather than 24Mhz reference
serial: tty: Cleanup code using devm_ function
arch/arm/boot/dts/vt8500.dtsi | 40 +++++++++++++++++++++--- arch/arm/boot/dts/wm8505.dtsi | 60 ++++++++++++++++++++++++++++++++---- arch/arm/boot/dts/wm8650.dtsi | 20 ++++++++++-- drivers/tty/serial/vt8500_serial.c | 38 +++++++++++------------ 4 files changed, 126 insertions(+), 32 deletions(-)
PATCH 1/4 serial: vt8500: Fix range-checking on vt8500_uart_ports
Fix two instances where the index to vt8500_uart_ports is tested against > VT8500_MAX_PORTS. Correct usage should be >= VT8500_MAX_PORTS.
--- drivers/tty/serial/vt8500_serial.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/vt8500_serial.c +2 -2
@@ -569,7 +569,7 @@ static int vt8500_serial_probe(struct platform_device *pdev)if (np) port =of_alias_get_id (np,"serial" ); -if (port >VT8500_MAX_PORTS ) +if (port >=VT8500_MAX_PORTS ) port =-1 ;else port =-1 ;
@@ -580,7 +580,7 @@ static int vt8500_serial_probe(struct platform_device *pdev) sizeof(vt8500_ports_in_use)); } -if (port >VT8500_MAX_PORTS ) +if (port >=VT8500_MAX_PORTS )return -ENODEV ;/* reserve the port id */
Signature · 1 line
1.7.9.5
PATCH 2/4 serial: vt8500: ioremap'd resource is never freed
In reply to Tony PriskMemory mapped via ioremap call is never released. Rather than add an iounmap call, change allocation function to devm_request_and_ioremap.
Also, change the error on failure for this call to -EBUSY rather than -ENOMEM.
--- drivers/tty/serial/vt8500_serial.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/tty/serial/vt8500_serial.c +2 -2
@@ -615,9 +615,9 @@ static int vt8500_serial_probe(struct platform_device *pdev)snprintf (vt8500_port->name,sizeof (vt8500_port->name),"VT8500 UART%d" , pdev->id); - vt8500_port->uart.membase =ioremap (mmres->start,resource_size (mmres)); + vt8500_port->uart.membase =devm_request_and_ioremap (&pdev->dev, mmres);if (!vt8500_port->uart.membase) { - ret = -ENOMEM ; + ret = -EBUSY ; goto err; }
Signature · 1 line
1.7.9.5
Re: [PATCH 2/4] serial: vt8500: ioremap'd resource is never freed
Hello.
Quoted 5 lines · Tony Prisk
Memory mapped via ioremap call is never released. Rather than add an iounmap call, change allocation function to
devm_request_and_ioremap.Also, change the error on failure for this call to
-EBUSYrather than-ENOMEM.
Not really well justified. See below.
Quoted 10 lines
Signed-off-by Tony Prisk <[email protected]>--- drivers/tty/serial/vt8500_serial.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/tty/serial/vt8500_serial.c +0
-0
drivers/tty/serial/vt8500_serial.c static int vt8500_serial_probe(struct platform_device *pdev) · line 620
Quoted 8 lines
drivers/tty/serial/vt8500_serial.c +2
-2@@ -615,9 +615,9 @@ static int vt8500_serial_probe(struct platform_device *pdev)snprintf (vt8500_port->name,sizeof (vt8500_port->name),"VT8500 UART%d" , pdev->id); - vt8500_port->uart.membase =ioremap (mmres->start,resource_size (mmres)); + vt8500_port->uart.membase =devm_request_and_ioremap (&pdev->dev, mmres);if (!vt8500_port->uart.membase) { - ret = -ENOMEM ; + ret = -EBUSY ;
According to the comment to devm_request_and_ioremap(), the recommended
return code on its failure -EADDRNOTAVAIL.
WBR, Sergei
PATCH 3/4 serial: vt8500: UART uses gated clock rather than 24Mhz reference
In reply to Tony PriskUART modules on Wondermedia SoCs are connected via a gated clock source, rather than directly to the 24Mhz reference clock. While uboot enables UART0 for debugging, other UART ports are unavailable until the clock is enabled.
This patch checks that a valid clock is actually passed from devicetree, enables the clock in probe. This change removes the fallback when a clock was not specified as it doesn't apply any longer (and would only work if the UART clock was already enabled).
DTSI files are updated for VT8500, WM8505 and WM8650.
--- arch/arm/boot/dts/vt8500.dtsi | 40 +++++++++++++++++++++--- arch/arm/boot/dts/wm8505.dtsi | 60 ++++++++++++++++++++++++++++++++---- arch/arm/boot/dts/wm8650.dtsi | 20 ++++++++++-- drivers/tty/serial/vt8500_serial.c | 17 ++++++---- 4 files changed, 119 insertions(+), 18 deletions(-)
arch/arm/boot/dts/vt8500.dtsi +36 -4
@@ -45,6 +45,38 @@ compatible = "fixed-clock"; clock-frequency = <24000000>; }; + + clkuart0: uart0 { + #clock-cells = <0>; + compatible = "via,vt8500-device-clock"; + clocks = <&ref24>; + enable-reg = <0x250>; + enable-bit = <1>; + }; + + clkuart1: uart1 { + #clock-cells = <0>; + compatible = "via,vt8500-device-clock"; + clocks = <&ref24>; + enable-reg = <0x250>; + enable-bit = <2>; + }; + + clkuart2: uart2 { + #clock-cells = <0>; + compatible = "via,vt8500-device-clock"; + clocks = <&ref24>; + enable-reg = <0x250>; + enable-bit = <3>; + }; + + clkuart3: uart3 { + #clock-cells = <0>; + compatible = "via,vt8500-device-clock"; + clocks = <&ref24>; + enable-reg = <0x250>; + enable-bit = <4>; + }; }; };
@@ -83,28 +115,28 @@ compatible = "via,vt8500-uart"; reg = <0xd8200000 0x1040>; interrupts = <32>; - clocks = <&ref24>; + clocks = <&clkuart0>; }; uart@d82b0000 { compatible = "via,vt8500-uart"; reg = <0xd82b0000 0x1040>; interrupts = <33>; - clocks = <&ref24>; + clocks = <&clkuart1>; }; uart@d8210000 { compatible = "via,vt8500-uart"; reg = <0xd8210000 0x1040>; interrupts = <47>; - clocks = <&ref24>; + clocks = <&clkuart2>; }; uart@d82c0000 { compatible = "via,vt8500-uart"; reg = <0xd82c0000 0x1040>; interrupts = <50>; - clocks = <&ref24>; + clocks = <&clkuart3>; }; rtc@d8100000 {
arch/arm/boot/dts/wm8505.dtsi +54 -6
102 lines not on this page · raw message
arch/arm/boot/dts/wm8650.dtsi +18 -2
@@ -75,6 +75,22 @@ reg = <0x204>; }; + clkuart0: uart0 { + #clock-cells = <0>; + compatible = "via,vt8500-device-clock"; + clocks = <&ref24>; + enable-reg = <0x250>; + enable-bit = <1>; + }; + + clkuart1: uart1 { + #clock-cells = <0>; + compatible = "via,vt8500-device-clock"; + clocks = <&ref24>; + enable-reg = <0x250>; + enable-bit = <2>; + }; + arm: arm { #clock-cells = <0>; compatible = "via,vt8500-device-clock";
@@ -128,14 +144,14 @@ compatible = "via,vt8500-uart"; reg = <0xd8200000 0x1040>; interrupts = <32>; - clocks = <&ref24>; + clocks = <&clkuart0>; }; uart@d82b0000 { compatible = "via,vt8500-uart"; reg = <0xd82b0000 0x1040>; interrupts = <33>; - clocks = <&ref24>; + clocks = <&clkuart1>; }; rtc@d8100000 {
drivers/tty/serial/vt8500_serial.c +11 -6
@@ -604,12 +604,16 @@ static int vt8500_serial_probe(struct platform_device *pdev) vt8500_port->uart.flags =UPF_IOREMAP |UPF_BOOT_AUTOCONF ; vt8500_port->clk =of_clk_get (pdev->dev.of_node,0 ); -if (vt8500_port->clk) { - vt8500_port->uart.uartclk =clk_get_rate (vt8500_port->clk); - }else { -/* use the default of 24Mhz if not specified and warn */ -pr_warn ("%s: serial clock source not specified\n" , __func__); - vt8500_port->uart.uartclk =24000000 ; +if (IS_ERR (vt8500_port->clk)) { +dev_err (&pdev->dev,"failed to get clock\n" ); + ret = -EINVAL ; + goto err; + } + + ret =clk_prepare_enable (vt8500_port->clk); +if (ret) { +dev_err (&pdev->dev,"failed to enable clock\n" ); + goto err; } snprintf(vt8500_port->name, sizeof(vt8500_port->name),
@@ -639,6 +643,7 @@ static int vt8500_serial_remove(struct platform_device *pdev)struct vt8500_port *vt8500_port =platform_get_drvdata (pdev);platform_set_drvdata (pdev,NULL ); +clk_disable_unprepare (vt8500_port->clk);uart_remove_one_port (&vt8500_uart_driver, &vt8500_port->uart);kfree (vt8500_port);
Signature · 1 line
1.7.9.5
Re: [PATCH 3/4] serial: vt8500: UART uses gated clock rather than 24Mhz reference
Quoted 6 lines · Tony Prisk
drivers/tty/serial/vt8500_serial.c +0
-6-if (vt8500_port->clk) { - vt8500_port->uart.uartclk =clk_get_rate (vt8500_port->clk); - }else { -/* use the default of 24Mhz if not specified and warn */ -pr_warn ("%s: serial clock source not specified\n" , __func__); - vt8500_port->uart.uartclk =24000000 ;
drivers/tty/serial/vt8500_serial.c static int vt8500_serial_probe(struct platform_device *pdev) · line 616
Quoted 10 lines
drivers/tty/serial/vt8500_serial.c +10
-0+if (IS_ERR (vt8500_port->clk)) { +dev_err (&pdev->dev,"failed to get clock\n" ); + ret = -EINVAL ; + goto err; + } + + ret =clk_prepare_enable (vt8500_port->clk); +if (ret) { +dev_err (&pdev->dev,"failed to enable clock\n" ); + goto err;
Looks like you get rid of the initialization of vt8500_port->uart.uartclk here. Was that intentional?
PATCH 4/4 serial: tty: Cleanup code using devm_ function
In reply to Tony PriskConvert the last memory allocation (vt8500_port) to use devm_kzalloc and remove the fail path cleanup code from vt8500_serial_probe.
Reorder iomem mapping above clk_enable to simplify fail code. The clock is only enabled if all other resources are available.
--- drivers/tty/serial/vt8500_serial.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)
drivers/tty/serial/vt8500_serial.c +8 -15
@@ -589,7 +589,8 @@ static int vt8500_serial_probe(struct platform_device *pdev)return -EBUSY ; } - vt8500_port =kzalloc (sizeof (struct vt8500_port ),GFP_KERNEL ); + vt8500_port =devm_kzalloc (&pdev->dev,sizeof (struct vt8500_port ), +GFP_KERNEL );if (!vt8500_port)return -ENOMEM ;
@@ -603,28 +604,25 @@ static int vt8500_serial_probe(struct platform_device *pdev) vt8500_port->uart.dev = &pdev->dev; vt8500_port->uart.flags =UPF_IOREMAP |UPF_BOOT_AUTOCONF ; + vt8500_port->uart.membase =devm_request_and_ioremap (&pdev->dev, mmres); +if (!vt8500_port->uart.membase) +return -EBUSY ; + vt8500_port->clk =of_clk_get (pdev->dev.of_node,0 );if (IS_ERR (vt8500_port->clk)) {dev_err (&pdev->dev,"failed to get clock\n" ); - ret = -EINVAL ; - goto err; +return -EINVAL ; } ret =clk_prepare_enable (vt8500_port->clk);if (ret) {dev_err (&pdev->dev,"failed to enable clock\n" ); - goto err; +return ret; }snprintf (vt8500_port->name,sizeof (vt8500_port->name),"VT8500 UART%d" , pdev->id); - vt8500_port->uart.membase =devm_request_and_ioremap (&pdev->dev, mmres); -if (!vt8500_port->uart.membase) { - ret = -EBUSY ; - goto err; - } - vt8500_uart_ports[port] = vt8500_port;uart_add_one_port (&vt8500_uart_driver, &vt8500_port->uart);
@@ -632,10 +630,6 @@ static int vt8500_serial_probe(struct platform_device *pdev)platform_set_drvdata (pdev, vt8500_port);return 0 ; - -err: -kfree (vt8500_port); -return ret; }static int vt8500_serial_remove (struct platform_device *pdev)
@@ -645,7 +639,6 @@ static int vt8500_serial_remove(struct platform_device *pdev)platform_set_drvdata (pdev,NULL );clk_disable_unprepare (vt8500_port->clk);uart_remove_one_port (&vt8500_uart_driver, &vt8500_port->uart); -kfree (vt8500_port);return 0 ; }
Signature · 1 line
1.7.9.5