Re: [PATCH v2 1/5] ARM: davinci: uart: move to devid based clk_get

[Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>]

On 6/6/2013 4:02 PM, Sekhar Nori wrote:
> Hi Prakash,
> 
> It appears that this patch was not tested thoroughly. See below:
> 
> On 5/28/2013 1:58 PM, Manjunathappa, Prakash wrote:
>> 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 <prakash.pm-l0cyMroinI0@public.gmane.org>
>> ---
> 
>> -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.

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.

Thanks,
Sekhar