[coreboot] [PATCH] [v3] add unwanted_vpci parsing to cs5536 southbridge

[Carl-Daniel Hailfinger]

On 10.04.2008 03:06, Ward Vandewege wrote:
> This one is largely thanks to Marc. Mostly written during the summit in
> Denver. It depends on the dts patch I just sent.
>
> Thanks,
> Ward.
>
> Add unwanted_vpci parsing to AMD's cs5536 southbridge.
>
> Signed-off-by: Ward Vandewege <ward at gnu.org>
>   

My comments are longer than the patch... if you act on them, this is
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

> Index: southbridge/amd/cs5536/cs5536.c
> ===================================================================
> --- southbridge/amd/cs5536/cs5536.c	(revision 656)
> +++ southbridge/amd/cs5536/cs5536.c	(working copy)
> @@ -648,6 +648,12 @@
>  
>  	enable_USB_port4(sb);
>  
> +	/* disable unwanted virtual PCI devices */
> +	int i;
>   

Maybe move the int i; declaration inside the for statement? That makes
the scope more obvious and has the added benefit of possibly conserving
stack space (if other loops do the same). Yes, I know this is C99 only,
but we have other uses for C99 which enables some cleanups.

> +	for (i = 0; 0 != (char *)(sb->unwanted_vpci[i]); i = i+4) {
>   

Ugh. What exactly are you trying to do? This looks wrong on so many
levels. Wouldn't the following line be more correct?

for (int i = 0; sb->unwanted_vpci[i]; i++) {


> +		hide_vpci(sb->unwanted_vpci[i]);
> +	}
> +
>  	if (sb->enable_ide)
>  		ide_init(dev);
>  
> Index: southbridge/amd/cs5536/dts
> ===================================================================
> --- southbridge/amd/cs5536/dts	(revision 656)
> +++ southbridge/amd/cs5536/dts	(working copy)
> @@ -64,4 +64,6 @@
>  	 * probably not what you want. 
>  	 */
>  	power_button = "0";
> +
> +	unwanted_vpci = < 0 >;
>   

The dts stuff is pending review as per my other mail.
>  };
>   
>   

Regards,
Carl-Daniel