From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1DtBzI-0002Wm-AB for mharc-grub-devel@gnu.org; Thu, 14 Jul 2005 18:15:24 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1DtBzE-0002VC-FV for grub-devel@gnu.org; Thu, 14 Jul 2005 18:15:20 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1DtBzC-0002U0-Hb for grub-devel@gnu.org; Thu, 14 Jul 2005 18:15:19 -0400 Received: from [199.232.76.173] (helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1DtBub-0008HP-Gx for grub-devel@gnu.org; Thu, 14 Jul 2005 18:10:33 -0400 Received: from [145.74.66.11] (helo=mail-cn.han.nl) by monty-python.gnu.org with esmtp (Exim 4.34) id 1DtBec-00052w-7C for grub-devel@gnu.org; Thu, 14 Jul 2005 17:54:02 -0400 Received: from vscan-cn.han.nl (venus.han.nl [145.74.65.6]) by mail-cn.han.nl (Postfix) with ESMTP id 2FFA78B6C for ; Thu, 14 Jul 2005 23:45:36 +0200 (CEST) Received: from mail-cn.han.nl ([145.74.66.11]) by vscan-cn.han.nl (venus.han.nl [145.74.65.6]) (amavisd-new, port 10024) with ESMTP id 05326-03 for ; Thu, 14 Jul 2005 22:52:16 +0200 (CEST) Received: from mail1.han.nl (mail1.han.nl [145.74.103.11]) by mail-cn.han.nl (Postfix) with ESMTP id 89BAD8ADE for ; Thu, 14 Jul 2005 23:45:33 +0200 (CEST) Received: from localhost.localdomain (mgerards.xs4all.nl [82.92.27.129]) by mail1.han.nl (Postfix) with ESMTP id 49E14C046 for ; Thu, 14 Jul 2005 23:45:33 +0200 (CEST) Mail-Copies-To: metgerards@student.han.nl To: The development of GRUB 2 References: <42D6B0CB.9070208@nic.fi> <87hdex6wju.fsf@student.han.nl> <42D6CCE5.5010807@nic.fi> From: Marco Gerards Date: Thu, 14 Jul 2005 23:45:36 +0200 In-Reply-To: <42D6CCE5.5010807@nic.fi> ( =?iso-8859-1?q?Vesa_J=E4=E4skel=E4inen's_message_of?= "Thu, 14 Jul 2005 23:36:53 +0300") Message-ID: <87hdex5af3.fsf@student.han.nl> User-Agent: Gnus/5.1007 (Gnus v5.10.7) Emacs/21.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable X-Virus-Scanned: by amavisd-new (2.2.0) at vscan-cn.han.nl Subject: Re: [PATCH] Kernel support for VESA Bios Extension X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Jul 2005 22:15:20 -0000 Vesa J=E4=E4skel=E4inen 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=20 > + * Extension 3.0 Specification says to reserve 189 bytes here but=20 > + * that doesn't make structure to be 256 bytes. So additional one is=20 > + * 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