Re: [patch] PPC cleanups

[Marco Gerards]

Hollis Blanchard <address@hidden> writes:

> This patch has a lot of long-overdue PPC cleanups, including build
> warnings. Since we all know the Changelog doesn't explain enough about a
> patch, a quick overview:
> - include/grub/powerpc/ieee1275/init.h was useless.
> - roundup() was unused.
> - rather than everyone accessing grub_ieee1275_flags directly, it's now
>   static and provides test/set accessors.
> - grub_ieee1275_realmode should have always been a flag instead of a
>   global; I don't know what I was thinking at the time.
> - ofdisk.h is a now real header (like biosdisk.h); no more
>   grub_ofdisk_fini/grub_ofdisk_init "missing prototype" warnings.
>
> Important new functionality:
> - actually boot a Linux kernel successfully (don't prematurely free memory)
> - properly handle partition numbering on briQ and Pegasos
>
> I have boot-tested this patch on briQ and G3, and would like to check it
> in soon.

The patch looks fine to me, although I have a few small comments, see
what I write below.  This evening I am going to test the patch to see
if it works for me.

> I also have my sights on include/grub/powerpc/ieee1275/biosdisk.h. Any
> suggestions on how to handle using util/i386/pc/biosdisk.c on PPC for
> grub-emu?

There was a discussion about this before, please read it for details I
might have forgotten.  But ATM the PC version works for the PPC as
well.  There is no platform specific code there yet and I want to keep
things shared as long as it is possible.  Doesn't it work for you or
are there any issues?  The biosdisk code is the code, together with
the i386 specific code, I am the least familiar with.

> 2005-04-21  Hollis Blanchard  <address@hidden>

>       * disk/powerpc/ieee1275/ofdisk.c: Include grub/machine/ofdisk.h. Don't
>       include grub/machine/init.h.

Two spaces are required here, please fix this in the other places in
the changelog entry/comments as well.

>       * include/grub/powerpc/ieee1275/ieee1275.h (grub_ieee1275_flags):
>       Remove prototype.
>       (grub_ieee1275_realmode): Likewise.
>       (grub_ieee1275_flag): New enum.

Can you give the enums a prefix?  For example
`GRUB_IEEE1275_NO_PARTITION_0' can be changed to
`GRUB_IEEE1275_FLAGS_NO_PARTITION_0'.

>       * kern/powerpc/ieee1275/init.c: Don't include grub/machine/init.h.
>       Include grub/machine/console.h. Include grub/machine/ofdisk.h.
>       (abort): Correct whitespace.

Huh?  What is this whitespace change?  And there is no need to mention
it in the changelog in case it is required.

--
Marco