Hi Sekhar,

Thanks for the review.

On Thu, Jun 06, 2013 at 16:14:19, Nori, Sekhar wrote:

On 6/6/2013 4:02 PM, Sekhar Nori wrote:

quoted

Hi Prakash,

It appears that this patch was not tested thoroughly. See below:

On 5/28/2013 1:58 PM, Manjunathappa, Prakash wrote:

quoted

For modules having single clock, clk_get should be done with dev_id.
But current davinci implementation handles multiple instances
of the UART devices with single platform_device_register. Hence clk_get
is based on con_id rather than dev_id, this is not correct. Do
platform_device_register for each instance and clk_get on dev_id.

Signed-off-by: Manjunathappa, Prakash <redacted>
---

quoted

-static struct plat_serial8250_port da8xx_serial_pdata[] = {
+static struct plat_serial8250_port da8xx_serial0_pdata[] = {
 	{
 		.mapbase	= DA8XX_UART0_BASE,
 		.irq		= IRQ_DA8XX_UARTINT0,

@@ -75,7 +75,9 @@ static struct plat_serial8250_port da8xx_serial_pdata[] = {
 					UPF_IOREMAP,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
-	},
+	}
+};
+static struct plat_serial8250_port da8xx_serial1_pdata[] = {
 	{
 		.mapbase	= DA8XX_UART1_BASE,
 		.irq		= IRQ_DA8XX_UARTINT1,

@@ -83,7 +85,9 @@ static struct plat_serial8250_port da8xx_serial_pdata[] = {
 					UPF_IOREMAP,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
-	},
+	}
+};
+static struct plat_serial8250_port da8xx_serial2_pdata[] = {
 	{
 		.mapbase	= DA8XX_UART2_BASE,
 		.irq		= IRQ_DA8XX_UARTINT2,

@@ -91,18 +95,31 @@ static struct plat_serial8250_port da8xx_serial_pdata[] = {
 					UPF_IOREMAP,
 		.iotype		= UPIO_MEM,
 		.regshift	= 2,
-	},
-	{
-		.flags	= 0,
-	},
+	}

8250_core.c relies on sentinel value with p->flags = 0 to terminate
looking for more ports. You don't have sentinel values in any of pdata
the arrays you introduced so the code basically goes looking into areas
of memory not its own. This caused bunch of "cannot register port"
errors for me but I can easily imagine more serious errors.

Will fix this.

Similarly, you check for platform_data being NULL to go over the list of
serial devices in the code in serial.c you introduced, but don't have a
sentinel when you define the serial device array.

I have fixed this and above, posted patch here:
http://davinci-linux-open-source.1494791.n2.nabble.com/PATCH-v3-0-5-ARM-davinci-fix-UART-clock-enabling-tt7583378.html

Thanks,
Prakash