Re: [PATCH] Fixed ieee1275 console

[Robert Millan]

On Wed, Oct 10, 2007 at 05:11:13PM +0200, Marcin Kurek wrote:
> Hell[o]
> 
> > Avoid the cosmetical changes! (there are more thorough the patch)
> 
> Arghh, I was sure I removed it :/ Anyway attached updated console patches.

Great, thank you.

> diff -urN grub2.org/term/ieee1275/ofconsole.c grub2/term/ieee1275/ofconsole.c
> --- grub2.org/term/ieee1275/ofconsole.c       2007-07-22 11:05:11.000000000 
> +0200
> +++ grub2/term/ieee1275/ofconsole.c   2007-10-10 16:03:34.136688149 +0200
> @@ -75,6 +75,7 @@
>  grub_ofconsole_putchar (grub_uint32_t c)
>  {
>    char chr = c;
> +  
>    if (c == '\n')

Caught you! ;-)

> diff -urN grub2.org/include/grub/ieee1275/ieee1275.h 
> grub2/include/grub/ieee1275/ieee1275.h
> --- grub2.org/include/grub/ieee1275/ieee1275.h        2007-10-04 
> 22:44:12.000000000 +0200
> +++ grub2/include/grub/ieee1275/ieee1275.h    2007-10-10 16:41:39.594688149 
> +0200
> @@ -82,6 +82,16 @@
>  
>    /* CodeGen firmware does not correctly implement "output-device output" */
>    GRUB_IEEE1275_FLAG_BROKEN_OUTPUT,
> +
> +  /* In non fb mode default number of console rows is 24, but in fact it's 
> 25 */
> +  GRUB_IEEE1275_FLAG_NOFB_ROWS25,
> +
> +  /* Old Pegaos firmware does not accept cls escape sequence */
> +  GRUB_IEEE1275_FLAG_NOCLS,
> +
> +  /* On CodeGen firmware, cp437 characters 0xc0 to 0xcb are reserved for the
> +     bplan logo */
> +  GRUB_IEEE1275_FLAG_BPLAN_LOGO,
>  };

I know it seems burdensome, but please can you split this in three patches, one
for each fix?  Then it's easier to review just one and say "this is good", and
also easier to figure out why every hunk was done (since one knows what we're
trying to archieve).
 
> +      /* It seems no cls escape is available then simulate it using \n flood 
> */
> +      int x = (grub_ofconsole_height * 2) - grub_curr_y;

Maybe this would be easier to understand as "(grub_ofconsole_height -
grub_curr_y) + grub_ofconsole_height".  What do others think? :-)

> +  /* Check do we are on serial or normal console */
> +  if(! grub_ieee1275_instance_to_package (stdout_ihandle, &stdout_phandle))
> +    {
> +      char type[16];
> +      char name[128];
> +      
> +      if(! grub_ieee1275_get_property (stdout_phandle, "device_type", &type,
> +                                       sizeof type, 0) &&
> +         ! grub_ieee1275_get_property (stdout_phandle, "name", &name,
> +                                       sizeof name, 0))
> +        {
> +          /*
> +           * In general type "serial" is used for console without
> +           * framebuffer support in recent firmware versions then
> +           * we need to check the name too to determine is it real or
> +           * serial console
> +           */
> +
> +          if (! grub_strcmp (type, "serial"))
> +            {
> +              /* If "name" is something else than "display" we assume serial 
> console */
> +              if(! grub_strcmp (name, "display"))
> +                  grub_ofconsole_serial = 0;
> +            }
> +          else
> +            { 
> +              grub_ofconsole_serial = 0;
> +
> +              /* Older versions use name/type set to "bootconsole" */
> +              if ( grub_strcmp (name, "bootconsole"))
> +                grub_ofconsole_fb = 1;
> +            }
> +        }
> +    }

Nice.  On which firmware variants did you try this?  I can try efika (stock
firmware) if you haven't yet.

In your code there's a condition in which _serial is set to 0 and _fb is
left unset (as 0).  Is this intended?  Sounds like a bug.

This could be avoided if you represent it as a multi-value variable, e.g.

enum
  {
    GRUB_OFCONSOLE_SERIAL,
    GRUB_OFCONSOLE_FB,
  };

grub_u8_t grub_ofconsole_backend = GRUB_OFCONSOLE_SERIAL;

[...]
if (foo)
  grub_ofconsole_backend = GRUB_OFCONSOLE_FB;

what do you think?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)