[LinuxBIOS] [PATCH] v3: 8254_pit.patch

[Marc Jones]

I think that I put the 8254_pit.h in the correct place.
LinuxBIOSv3\include\arch\x86\8254_pit.h

Marc

[Uwe Hermann]

On Tue, Jul 03, 2007 at 10:53:23AM -0600, Marc Jones wrote:
> Index: LinuxBIOSv3/arch/x86/geodelx/geodelx.c
>
> --- LinuxBIOSv3.orig/arch/x86/geodelx/geodelx.c	2007-07-02 10:23:52.000000000 -0600
> +++ LinuxBIOSv3/arch/x86/geodelx/geodelx.c	2007-07-03 09:37:11.000000000 -0600
> @@ -30,6 +30,7 @@
>  #include <io.h>
>  #include <amd_geodelx.h>
>  #include <spd.h>
> +#include <8254_pit.h>
>
> /* all these functions used to be in a lot of fiddly little files.  To
> make it easier to find functions, we are merging them here. This
>
> @@ -41,8 +42,6 @@
>
> /**
> Starts Timer 1 for port 61 use.
> 0x43 is PIT command/control.
> 0x41 is PIT counter 1.
> The command 0x56 means write counter 1 lower 8 bits in next IO,
> set the counter mode to square wave generator (count down to 0
>
> @@ -58,8 +57,8 @@
>    */
>  void start_timer1(void)
>  {
> outb(0x56, 0x43);
> outb(0x12, 0x41);
> outb(0x56, I82C54_CONTROL_WORD_REGISTER);
> outb(0x12, I82C54_COUNTER1);

Yep, but the 0x56 and 0x12 should be #defines (or have comments), too.
> }
>
> /** 
> Index: LinuxBIOSv3/arch/x86/speaker.c
> ===================================================================
> --- LinuxBIOSv3.orig/arch/x86/speaker.c	2007-07-02 10:23:37.000000000 -0600
> +++ LinuxBIOSv3/arch/x86/speaker.c	2007-07-03 09:34:08.000000000 -0600
> @@ -2,6 +2,7 @@
> This file is part of the LinuxBIOS project.
> Copyright (C) 2007 Uwe Hermann
> Copyright (C) 2007 Advanced Micro Devices, Inc.

Nope, please drop that this time. There's no new code, just shuffling of
existing code in this patch.
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License as published by
>
> @@ -18,23 +19,9 @@
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
>
> */
>
> -/*
> Datasheet:
> Name: 82C54 CHMOS Programmable Interval Timer
> PDF: http://www.intel.com/design/archives/periphrl/docs/23124406.pdf
> Order number: 231244-006
> */

Please let's keep this here _and_ in 8254_pit.h, it's useful in both
files, IMO.
> #include <io.h>
>  #include <lib.h>
>
> -#define I82C54_CONTROL_WORD_REGISTER	0x43	/* Write-only. */
>
> -#define I82C54_COUNTER0			0x40
> -#define I82C54_COUNTER1			0x41
> -#define I82C54_COUNTER2			0x42
>
> -#define PC_SPEAKER_PORT			0x61

The PC_SPEAKER_PORT belongs in this file, I think. It's not PIT related,
but speaker related.
> +#include <8254_pit.h>
>
> /**
> Use the PC speaker to create a tone/sound of the specified frequency.
>
> Index: LinuxBIOSv3/include/arch/x86/8254_pit.h

Hm, maybe we should make this include/arch/x86/legacy.h and stick other
#defines in there, too. We should avoid too many small almost-empty
files and there will be other "legacy" devices which need #defines.
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ LinuxBIOSv3/include/arch/x86/8254_pit.h	2007-07-03 09:34:04.000000000 -0600
> @@ -0,0 +1,36 @@
> Copyright (C) 2007 Advanced Micro Devices, Inc.

See above, drop this please.

Otherwise patch is ok.

Uwe.

[Marc Jones]

Updated the patch based on comments. Some comments inline below.

Uwe Hermann wrote:
> On Tue, Jul 03, 2007 at 10:53:23AM -0600, Marc Jones wrote:
> > Index: LinuxBIOSv3/arch/x86/geodelx/geodelx.c
> >
> > --- LinuxBIOSv3.orig/arch/x86/geodelx/geodelx.c	2007-07-02 10:23:52.000000000 -0600
> > +++ LinuxBIOSv3/arch/x86/geodelx/geodelx.c	2007-07-03 09:37:11.000000000 -0600
> > @@ -30,6 +30,7 @@
> >  #include <io.h>
> >  #include <amd_geodelx.h>
> >  #include <spd.h>
> > +#include <8254_pit.h>
> >
> > /* all these functions used to be in a lot of fiddly little files.  To
> > make it easier to find functions, we are merging them here. This
> >
> > @@ -41,8 +42,6 @@
> >
> > /**
> > Starts Timer 1 for port 61 use.
> > 0x43 is PIT command/control.
> > 0x41 is PIT counter 1.
> > The command 0x56 means write counter 1 lower 8 bits in next IO,
> > set the counter mode to square wave generator (count down to 0
> >
> > @@ -58,8 +57,8 @@
> >    */
> >  void start_timer1(void)
> >  {
> > outb(0x56, 0x43);
> > outb(0x12, 0x41);
> > outb(0x56, I82C54_CONTROL_WORD_REGISTER);
> > outb(0x12, I82C54_COUNTER1);
>
> Yep, but the 0x56 and 0x12 should be #defines (or have comments), too.

The 0x56 and 0x12 are well commented in the function header.
> > }
> >
> > /** 
> > Index: LinuxBIOSv3/arch/x86/speaker.c
> > ===================================================================
> > --- LinuxBIOSv3.orig/arch/x86/speaker.c	2007-07-02 10:23:37.000000000 -0600
> > +++ LinuxBIOSv3/arch/x86/speaker.c	2007-07-03 09:34:08.000000000 -0600
> > @@ -2,6 +2,7 @@
> > This file is part of the LinuxBIOS project.
> > Copyright (C) 2007 Uwe Hermann
> > Copyright (C) 2007 Advanced Micro Devices, Inc.
>
> Nope, please drop that this time. There's no new code, just shuffling of
> existing code in this patch.

ok
> > This program is free software; you can redistribute it and/or modify
> > it under the terms of the GNU General Public License as published by
> >
> > @@ -18,23 +19,9 @@
> > Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
> >
> > */
> >
> > -/*
> > Datasheet:
> > Name: 82C54 CHMOS Programmable Interval Timer
> > PDF: http://www.intel.com/design/archives/periphrl/docs/23124406.pdf
> > Order number: 231244-006
> > */
>
> Please let's keep this here _and_ in 8254_pit.h, it's useful in both
> files, IMO.

ok
> > #include <io.h>
> >  #include <lib.h>
> >
> > -#define I82C54_CONTROL_WORD_REGISTER	0x43	/* Write-only. */
> >
> > -#define I82C54_COUNTER0			0x40
> > -#define I82C54_COUNTER1			0x41
> > -#define I82C54_COUNTER2			0x42
> >
> > -#define PC_SPEAKER_PORT			0x61
>
> The PC_SPEAKER_PORT belongs in this file, I think. It's not PIT related,
> but speaker related.

good point
> > +#include <8254_pit.h>
> >
> > /**
> > Use the PC speaker to create a tone/sound of the specified frequency.
> >
> > Index: LinuxBIOSv3/include/arch/x86/8254_pit.h
>
> Hm, maybe we should make this include/arch/x86/legacy.h and stick other
> #defines in there, too. We should avoid too many small almost-empty
> files and there will be other "legacy" devices which need #defines.

I started on this path but since there wasn't a legacy.h i figured 
everything was getting it's own headers. There is a legacy.h now.
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ LinuxBIOSv3/include/arch/x86/8254_pit.h	2007-07-03 09:34:04.000000000 -0600
> > @@ -0,0 +1,36 @@
> > Copyright (C) 2007 Advanced Micro Devices, Inc.
>
> See above, drop this please.

ok
> Otherwise patch is ok.
>
> Uwe.

Marc

[Uwe Hermann]

On Tue, Jul 03, 2007 at 06:19:48PM -0600, Marc Jones wrote:
> > Yep, but the 0x56 and 0x12 should be #defines (or have comments), too.
>
> The 0x56 and 0x12 are well commented in the function header.

Ah, yes, I should have looked a bit closer.
> Moved some generic 8254 PIT #defines out of the speaker code to legacy.h.
> Use legacy.h PIT defines in Geode code.
>
> Signed-off-by: Marc Jones

Looks good to me.

Acked-by: Uwe Hermann

Uwe.