Patchwork [PATCHv3,3/9] serial: vt8500: Add devicetree support for vt8500-serial

login
register
mail settings
Submitter Tony Prisk
Date Aug. 21, 2012, 8:47 p.m.
Message ID <1345582058-2291-4-git-send-email-linux@prisktech.co.nz>
Download mbox | patch
Permalink /patch/179163/
State New
Headers show

Comments

Tony Prisk - Aug. 21, 2012, 8:47 p.m.
Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
---
 drivers/tty/serial/vt8500_serial.c |   37 ++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)
Alan Cox - Aug. 21, 2012, 10:12 p.m.
On Wed, 22 Aug 2012 08:47:32 +1200
Tony Prisk <linux@prisktech.co.nz> wrote:

> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> ---
>  drivers/tty/serial/vt8500_serial.c |   37 ++++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)

Can we have a comment attached to a change this size. In particular one
describing why it gone from 4 to 6 ports, and why the port id twiddling.

Is there a reason you can't use the device tree port id ?

What are the regression risks for existing users expecting the pdev->id
binding ?
Tony Prisk - Aug. 22, 2012, 6:34 a.m.
On Tue, 2012-08-21 at 23:12 +0100, Alan Cox wrote:
> On Wed, 22 Aug 2012 08:47:32 +1200
> Tony Prisk <linux@prisktech.co.nz> wrote:
> 
> > Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> > ---
> >  drivers/tty/serial/vt8500_serial.c |   37 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> Can we have a comment attached to a change this size. In particular one
> describing why it gone from 4 to 6 ports, and why the port id twiddling.
> 
> Is there a reason you can't use the device tree port id ?
> 
> What are the regression risks for existing users expecting the pdev->id
> binding ?
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Sorry Alan,

The original patch was very simple, but I revisited it to fix other
issues and forgot to add the relevant comments.

Port size is changed to fix a problem - WM8505 actually had 6 uart's
defined in platform data but the vt8500_ports variable was only 4.

I have added devicetree port id support as well.

No regression risks as the entire platform is being converted to
devicetree and existing code dropped in this patchset.

Patchv4 3/9 to follow.

Regards

Tony Prisk
Arnd Bergmann - Aug. 22, 2012, 6:44 a.m.
On Wednesday 22 August 2012, Tony Prisk wrote:
> The original patch was very simple, but I revisited it to fix other
> issues and forgot to add the relevant comments.
> 
> Port size is changed to fix a problem - WM8505 actually had 6 uart's
> defined in platform data but the vt8500_ports variable was only 4.
> 
> I have added devicetree port id support as well.

If you do multiple things in one driver, you should normally send multiple
patches as well, each with a description why that change is done.
It may seem silly at first to send out a one-line patch next to a 100-line
patch for the same file, but those cases are actually the ones where it's
most important.

	Arnd
Rob Landley - Aug. 23, 2012, 9:29 p.m.
On 08/22/2012 01:44 AM, Arnd Bergmann wrote:
> On Wednesday 22 August 2012, Tony Prisk wrote:
>> The original patch was very simple, but I revisited it to fix other
>> issues and forgot to add the relevant comments.
>>
>> Port size is changed to fix a problem - WM8505 actually had 6 uart's
>> defined in platform data but the vt8500_ports variable was only 4.
>>
>> I have added devicetree port id support as well.
> 
> If you do multiple things in one driver, you should normally send multiple
> patches as well, each with a description why that change is done.
> It may seem silly at first to send out a one-line patch next to a 100-line
> patch for the same file, but those cases are actually the ones where it's
> most important.

Think of us poor git-bisect monkeys who have no idea why something broke
but can (purely mechanically) figure out which commit did it. If it's a
patch that does three unrelated things, we're kinda stuck.

Rob

Patch

diff --git a/drivers/tty/serial/vt8500_serial.c b/drivers/tty/serial/vt8500_serial.c
index 2be006f..72e32db 100644
--- a/drivers/tty/serial/vt8500_serial.c
+++ b/drivers/tty/serial/vt8500_serial.c
@@ -34,6 +34,7 @@ 
 #include <linux/slab.h>
 #include <linux/clk.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
 
 /*
  * UART Register offsets
@@ -76,6 +77,8 @@ 
 #define RX_FIFO_INTS	(RXFAF | RXFF | RXOVER | PER | FER | RXTOUT)
 #define TX_FIFO_INTS	(TXFAE | TXFE | TXUDR)
 
+#define VT8500_MAX_PORTS	6
+
 struct vt8500_port {
 	struct uart_port	uart;
 	char			name[16];
@@ -83,6 +86,13 @@  struct vt8500_port {
 	unsigned int		ier;
 };
 
+/*
+ * we use this variable to keep track of which ports
+ * have been allocated as we can't use pdev->id in
+ * devicetree
+ */
+static unsigned long vt8500_ports_in_use;
+
 static inline void vt8500_write(struct uart_port *port, unsigned int val,
 			     unsigned int off)
 {
@@ -431,7 +441,7 @@  static int vt8500_verify_port(struct uart_port *port,
 	return 0;
 }
 
-static struct vt8500_port *vt8500_uart_ports[4];
+static struct vt8500_port *vt8500_uart_ports[VT8500_MAX_PORTS];
 static struct uart_driver vt8500_uart_driver;
 
 #ifdef CONFIG_SERIAL_VT8500_CONSOLE
@@ -549,6 +559,7 @@  static int __devinit vt8500_serial_probe(struct platform_device *pdev)
 	struct vt8500_port *vt8500_port;
 	struct resource *mmres, *irqres;
 	int ret;
+	int port;
 
 	mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irqres = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
@@ -559,13 +570,25 @@  static int __devinit vt8500_serial_probe(struct platform_device *pdev)
 	if (!vt8500_port)
 		return -ENOMEM;
 
+	/* calculate the port id */
+	port = find_first_zero_bit(&vt8500_ports_in_use,
+				sizeof(vt8500_ports_in_use));
+	if (port > VT8500_MAX_PORTS)
+		return -ENODEV;
+
+	/* reserve the port id */
+	if (test_and_set_bit(port, &vt8500_ports_in_use)) {
+		/* port already in use - shouldn't really happen */
+		return -EBUSY;
+	}
+
 	vt8500_port->uart.type = PORT_VT8500;
 	vt8500_port->uart.iotype = UPIO_MEM;
 	vt8500_port->uart.mapbase = mmres->start;
 	vt8500_port->uart.irq = irqres->start;
 	vt8500_port->uart.fifosize = 16;
 	vt8500_port->uart.ops = &vt8500_uart_pops;
-	vt8500_port->uart.line = pdev->id;
+	vt8500_port->uart.line = port;
 	vt8500_port->uart.dev = &pdev->dev;
 	vt8500_port->uart.flags = UPF_IOREMAP | UPF_BOOT_AUTOCONF;
 	vt8500_port->uart.uartclk = 24000000;
@@ -579,7 +602,7 @@  static int __devinit vt8500_serial_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	vt8500_uart_ports[pdev->id] = vt8500_port;
+	vt8500_uart_ports[port] = vt8500_port;
 
 	uart_add_one_port(&vt8500_uart_driver, &vt8500_port->uart);
 
@@ -603,12 +626,18 @@  static int __devexit vt8500_serial_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id wmt_dt_ids[] = {
+	{ .compatible = "via,vt8500-uart", },
+	{}
+};
+
 static struct platform_driver vt8500_platform_driver = {
 	.probe  = vt8500_serial_probe,
 	.remove = __devexit_p(vt8500_serial_remove),
 	.driver = {
 		.name = "vt8500_serial",
 		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(wmt_dt_ids),
 	},
 };
 
@@ -642,4 +671,4 @@  module_exit(vt8500_serial_exit);
 
 MODULE_AUTHOR("Alexey Charkov <alchark@gmail.com>");
 MODULE_DESCRIPTION("Driver for vt8500 serial device");
-MODULE_LICENSE("GPL");
+MODULE_LICENSE("GPL v2");