| Message ID | 1345582058-2291-4-git-send-email-linux@prisktech.co.nz |
|---|---|
| State | Superseded |
| Headers | show |
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 ?
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
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
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
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");
Signed-off-by: Tony Prisk <linux@prisktech.co.nz> --- drivers/tty/serial/vt8500_serial.c | 37 ++++++++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 4 deletions(-)