Re: [PATCH] Kernel support for VESA Bios Extension

[Marco Gerards <metgerards@student.han.nl>]

Vesa Jääskeläinen <chaac@nic.fi> writes:

>>>I would like to hear comments about the patch. I tried to implement
>>>most commonly needed functions, so you can't do everything with
>>>this. As I didn't need to use every function in my frame buffer
>>>console test, some things might not work. But I tried to make my best
>>>to proof read every function.
>> Can you please make sure you are using the GCS for coding style?
>> Please have a look at the other sourcecode and the GCS (GNU Coding
>> Standards) first:
>> http://www.gnu.org/prep/standards/
>> It is important for us to have a single and consistent coding style.
>> If you have questions about either GRUB or the coding style you are of
>> course free to ask.
>
> Perhaps this is a bit closer to requirement?

It is, thanks.

> If there still are issues on formatting could you specify what issues
> you are thinking on.

Of course.

> +typedef grub_uint32_t 	grub_vbe_farptr_t;
> +typedef grub_uint32_t	grub_vbe_physptr_t;
> +typedef grub_uint32_t	grub_vbe_status_t;

Please use a single space before the type name.

> +struct grub_vbe_mode_info_block
> +{
> +  /* Mandory information for all VBE revisions */

After the comment add a `.' and two spaces instead of one.  This is
true for all comments.

> +  /*
> +   * Reserved field to make structure to be 256 bytes long, VESA BIOS 
> +   * Extension 3.0 Specification says to reserve 189 bytes here but 
> +   * that doesn't make structure to be 256 bytes. So additional one is 
> +   * added here.
> +   */

I personally prefer not using that many `*'s and believe it is not in
the GCS.  But I think Okuji uses this as well, but I don't.  Perhaps
we need our own guidelines for such things, I think consistency is
important.  Okuji, what you you think?

> +  grub_uint8_t reserved4[189+1];

Please put spaces around operators.  So this should be:

grub_uint8_t reserved4[189 + 1]

   * 31          24         19   16                 7           0
>   * ------------------------------------------------------------
>   * |             | |B| |A|       | |   |1|0|E|W|A|            |
> - * | BASE 31..24 |G|/|0|V| LIMIT |P|DPL|  TYPE   | BASE 23:16 |
> + * | BASE 31..24 |G|/|L|V| LIMIT |P|DPL|  TYPE   | BASE 23:16 |  4
>   * |             | |D| |L| 19..16| |   |1|1|C|R|A|            |
>   * ------------------------------------------------------------
>   * |                             |                            |
> - * |        BASE 15..0           |       LIMIT 15..0          |
> + * |        BASE 15..0           |       LIMIT 15..0          |  0
>   * |                             |                            |
>   * ------------------------------------------------------------
>   *

What is this?  Is this copied from (copyrighted) documentation?


To me it seemed line some lines were too long.  Please keep in mind it
should be all readable on a 80 columns width terminal according to the
GCS.

I will be gone this weekend.  After the weekend I will try to
understand the patch and proofread the assembler code.  But my x86
assembly is *very* rusty, so I hope someone else can review this patch
because I am afraid when it comes to assembler my opinion is not worth
a bit. ;)

Thanks,
Marco