Re: play.c

[Hollis Blanchard]

On Nov 5, 2005, at 8:17 AM, Vincent Pelletier wrote:
> Here is the play command, along with some songs.

Can you describe how the PIT is related to the speaker? I'm wondering 
if this code could mostly work on PPC platforms that have PC-like 
peripherals.

> --- /dev/null   1 Jan 1970 00:00:00 -0000
> +++ commands/i386/pc/play.c     5 Nov 2005 14:06:00 -0000
> @@ -0,0 +1,248 @@
...
> +/* Lots of this file is borowed from GNU/Hurd generic-speaker driver 
> */

"borrowed"

> +/* Read a byte from a port.  */
> +static inline unsigned char
> +inb (unsigned short port)
> +{
> +  unsigned char value;
> +
> +  asm volatile ("inb    %w1, %0" : "=a" (value) : "Nd" (port));
> +  asm volatile ("outb   %%al, $0x80" : : );
> +
> +  return value;
> +}

What is the purpose of outb to 0x80?

Please combine these into a single asm statement. The compiler can and 
will put code in between these statements...

> +/* Write a byte to a port.  */
> +static inline void
> +outb (unsigned short port, unsigned char value)
> +{
> +  asm volatile ("outb   %b0, %w1" : : "a" (value), "Nd" (port));
> +  asm volatile ("outb   %%al, $0x80" : : );
> +}

Again, combine the asm statements please.

If this code could be usable on other architectures, we will need to 
move inb/outb into architecture-specific code. That's probably not a 
bad idea anyways...

> +static void
> +beep_off (void)
> +{
> +  unsigned char status;
> +
> +  status = inb (SPEAKER) & ~(SPEAKER_TMR2 | SPEAKER_DATA);
> +  outb (SPEAKER, status);
> +}

A minor issue, but this code makes more sense to me:
        status = inb (SPEAKER);
        outb (SPEAKER, status & ~(SPEAKER_TMR2 | SPEAKER_DATA));

> +static grub_err_t
> +grub_cmd_play (struct grub_arg_list *state __attribute__ ((unused)),
> +             int argc, char **args)
> +{
> +  grub_file_t file;
> +  struct note buf;
> +  int tempo;
> +  unsigned int to;
> +
> +  if (argc != 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "file name required");
> +
> +  file = grub_file_open (args[0]);
> +  if (! file)
> +    return 0;
> +
> +  if (grub_file_read (file, (void *) &tempo, sizeof(tempo)) != 
> sizeof(tempo))
> +    return 0;

Shouldn't these error conditions return non-0 error codes?

> +  grub_printf ("Now playing %s...\n", args[0]);

Probably not needed.

> +  grub_dprintf ("play","tempo = %d\n", tempo);
> +
> +  while (grub_file_read (file, (void *) &buf, sizeof(struct note)) == 
> sizeof(struct note) &&
> +         buf.pitch != T_FINE &&
> +         grub_checkkey () == -1)
> +    {
> +
> +      grub_dprintf ("play","pitch = %d, duration = %d\n", buf.pitch, 
> buf.duration);
> +
> +      switch (buf.pitch)
> +        {
> +          case T_REST:
> +            beep_off ();
> +            break;
> +
> +          default:
> +            beep_on (buf.pitch);
> +            break;
> +        }
> +
> +      to = grub_get_rtc () + 120*buf.duration/tempo;
> +      while ((unsigned int) grub_get_rtc () <= to && grub_checkkey () 
> == -1)

I would add some parentheses. Also, I would prefer "grub_checkkey () < 
0", because on i386 that is a BIOS call, and who knows if all BIOS 
return exactly -1...

> +       ;
> +    }
> +
> +  beep_off ();
> +
> +  grub_printf ("Done\n");

Again, a little too verbose.

> +  grub_file_close (file);
> +
> +  while (grub_checkkey () != -1)

Same comment as above.

> +    grub_getkey ();
> +
> +  return 0;
> +}

Also, did you have some tool for creating those song files?

I would expect it's not entirely legal to ship versions of copyrighted 
songs.

-Hollis