Re: [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

My comments are longer than the patch... if you act on them, this is
Acked-by: Carl-Daniel Hailfinger
> 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