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

[Ward Vandewege]

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.

[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

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

[Ward Vandewege]

On Thu, Apr 10, 2008 at 03:44:35AM +0200, Carl-Daniel Hailfinger wrote:
> 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.

All errors are my own of course!
> > /* 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.

That doesn't work:

southbridge/amd/cs5536/cs5536.c:652: error: ‘for’ loop initial declaration
used outside C99 mode

Suggestions?
> > 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++) {

You're right of course, much better that way (minus the int declaration). My
line just happened to match the first entry but was plain wrong...

Thanks,
Ward.

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

[Carl-Daniel Hailfinger]

On 10.04.2008 04:03, Ward Vandewege wrote:
> On Thu, Apr 10, 2008 at 03:44:35AM +0200, Carl-Daniel Hailfinger wrote:
> > 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.
>
> All errors are my own of course!
> > > /* 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.
>
> That doesn't work:
>
> southbridge/amd/cs5536/cs5536.c:652: error: ‘for’ loop initial declaration
> used outside C99 mode
>
> Suggestions?

Use the -std=gnu99 gcc parameter. Anyway, usage of C99 features is
probably best left to another patch, so feel free to ignore that request
of mine for now.
> > > 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++) {
>
> You're right of course, much better that way (minus the int declaration). My
> line just happened to match the first entry but was plain wrong...

Fine. Now we just have to create a mergeable version of the dtc patch.

Regards,
Carl-Daniel

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

[Ward Vandewege]

Updated version of this patch, as per Carl-Daniel's comments and taking into
account the DTC changes have been merged.

Thanks,
Ward.

add unwanted_vpci parsing to cs5536 southbridge

[Jordan Crouse]

On 17/04/08 12:19 -0400, Ward Vandewege wrote:
> Updated version of this patch, as per Carl-Daniel's comments and taking into
> account the DTC changes have been merged.
>
> Thanks,
> Ward.
> Add unwanted_vpci parsing to AMD's cs5536 southbridge.
>
> Signed-off-by: Ward Vandewege

Acked-by: Jordan Crouse

With the fix mention on IRC to move the variable declaration to the 
top of the function.
> 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;
> for (i = 0; 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,8 @@
>     * probably not what you want. 
>     */
>    power_button = "0";
> /* vpci devices to be disabled */
> unwanted_vpci = < 0 >;
>
> };
>

add unwanted_vpci parsing to cs5536 southbridge

[Ward Vandewege]

On Thu, Apr 17, 2008 at 10:32:36AM -0600, Jordan Crouse wrote:
> On 17/04/08 12:19 -0400, Ward Vandewege wrote:
> > Updated version of this patch, as per Carl-Daniel's comments and taking into
> > account the DTC changes have been merged.
> >
> > Thanks,
> > Ward.
> > Add unwanted_vpci parsing to AMD's cs5536 southbridge.
> >
> > Signed-off-by: Ward Vandewege
>
> Acked-by: Jordan Crouse
>
> With the fix mention on IRC to move the variable declaration to the 
> top of the function.

r662, including that fix and some extra comments.

Thanks!
Ward.