See patch.
Uwe.
* Uwe Hermann uwe@hermann-uwe.de [070331 00:11]:
Index: src/superio/nsc/pc87309/pc87309.h
--- src/superio/nsc/pc87309/pc87309.h (Revision 0) +++ src/superio/nsc/pc87309/pc87309.h (Revision 0) @@ -0,0 +1,30 @@
+#define PC87309_FDC 0x00 /* Floppy */ +#define PC87309_PP 0x01 /* Parallel port */ +#define PC87309_SP2 0x02 /* Com2 / IR */ +#define PC87309_SP1 0x03 /* Com1 */ +#define PC87309_PM 0x04 /* Power management */ +#define PC87309_KBCM 0x05 /* Mouse */ +#define PC87309_KBCK 0x06 /* Keyboard */
Since these defines are only ever used in one single file, it should not be an extra include file, but part of superio.c
In order to make the code more reusable I suggest also dropping the PC87309_ prefix
On Sat, Mar 31, 2007 at 01:13:46AM +0200, Stefan Reinauer wrote:
- Uwe Hermann uwe@hermann-uwe.de [070331 00:11]:
Index: src/superio/nsc/pc87309/pc87309.h
--- src/superio/nsc/pc87309/pc87309.h (Revision 0) +++ src/superio/nsc/pc87309/pc87309.h (Revision 0) @@ -0,0 +1,30 @@
+#define PC87309_FDC 0x00 /* Floppy */ +#define PC87309_PP 0x01 /* Parallel port */ +#define PC87309_SP2 0x02 /* Com2 / IR */ +#define PC87309_SP1 0x03 /* Com1 */ +#define PC87309_PM 0x04 /* Power management */ +#define PC87309_KBCM 0x05 /* Mouse */ +#define PC87309_KBCK 0x06 /* Keyboard */
Since these defines are only ever used in one single file, it should not be an extra include file, but part of superio.c
In order to make the code more reusable I suggest also dropping the PC87309_ prefix
I already dropped the prefix on almost all variables and function names. For the remaining ones I think it makes sense to keep them.
For example, PC87309_SP1 is used in the auto.c file of the respective board to enable the Super I/O and use COM1.
I don't know for sure whether it makes sense to have the defines in an extra file, but pretty much _all_ other Super I/Os have them in an extra file... So I think we should keep it that way for consistency reasons, or change the behavior for _all_ Super I/Os. Comments?
Uwe.
* Uwe Hermann uwe@hermann-uwe.de [070331 04:09]:
I already dropped the prefix on almost all variables and function names. For the remaining ones I think it makes sense to keep them.
For example, PC87309_SP1 is used in the auto.c file of the respective board to enable the Super I/O and use COM1.
Ah ok, this might be important on boards with more than one superio.
Or maybe we should find a unique abstraction for this?
I don't know for sure whether it makes sense to have the defines in an extra file, but pretty much _all_ other Super I/Os have them in an extra file... So I think we should keep it that way for consistency reasons, or change the behavior for _all_ Super I/Os. Comments?
Yes. I think all other SuperIOs are doing the wrong thing, too. An include is there to share information between files. If information is not shared, it should be local.
Anyways, I think we might have reasons to expect SuperIOs dying out from the next generation boards.
On Sat, Mar 31, 2007 at 08:20:01PM +0200, Stefan Reinauer wrote:
- Uwe Hermann uwe@hermann-uwe.de [070331 04:09]:
I already dropped the prefix on almost all variables and function names. For the remaining ones I think it makes sense to keep them.
For example, PC87309_SP1 is used in the auto.c file of the respective board to enable the Super I/O and use COM1.
Ah ok, this might be important on boards with more than one superio.
Or maybe we should find a unique abstraction for this?
On the long run (and for v3), maybe. But for now I'd like to get this committed and fix up _all_ Super I/Os at the same time later...
Yes. I think all other SuperIOs are doing the wrong thing, too. An include is there to share information between files. If information is not shared, it should be local.
AFAICS the include files _are_ used elsewhere in the code (for some Super I/Os, at least):
$ rgrep w83627hf.h * | grep -v .svn src/superio/winbond/w83627hf/superio.c:#include "w83627hf.h" src/superio/winbond/w83627hf/w83627hf_early_serial.c:#include "w83627hf.h" src/superio/winbond/w83627hf/w83627hf_early_init.c:#include "w83627hf.h" src/mainboard/dell/s1850/auto.c:#include "superio/winbond/w83627hf/w83627hf.h" src/mainboard/supermicro/x6dai_g/auto.c:#include "superio/winbond/w83627hf/w83627hf.h" src/mainboard/supermicro/x6dhe_g/auto.c:#include "superio/winbond/w83627hf/w83627hf.h" src/mainboard/supermicro/x6dhe_g2/auto.updated.c:#include "superio/winbond/w83627hf/w83627hf.h" src/mainboard/supermicro/x6dhr_ig/auto.c:#include "superio/winbond/w83627hf/w83627hf.h" src/mainboard/supermicro/x6dhr_ig2/auto.c:#include "superio/winbond/w83627hf/w83627hf.h"
Uwe.
* Uwe Hermann uwe@hermann-uwe.de [070331 20:53]:
On Sat, Mar 31, 2007 at 08:20:01PM +0200, Stefan Reinauer wrote:
- Uwe Hermann uwe@hermann-uwe.de [070331 04:09]:
I already dropped the prefix on almost all variables and function names. For the remaining ones I think it makes sense to keep them.
For example, PC87309_SP1 is used in the auto.c file of the respective board to enable the Super I/O and use COM1.
Ah ok, this might be important on boards with more than one superio.
Or maybe we should find a unique abstraction for this?
On the long run (and for v3), maybe. But for now I'd like to get this committed and fix up _all_ Super I/Os at the same time later...
oh of course.
Yes. I think all other SuperIOs are doing the wrong thing, too. An include is there to share information between files. If information is not shared, it should be local.
AFAICS the include files _are_ used elsewhere in the code (for some Super I/Os, at least):
$ rgrep w83627hf.h * | grep -v .svn src/superio/winbond/w83627hf/superio.c:#include "w83627hf.h" src/superio/winbond/w83627hf/w83627hf_early_serial.c:#include "w83627hf.h" src/superio/winbond/w83627hf/w83627hf_early_init.c:#include "w83627hf.h" src/mainboard/dell/s1850/auto.c:#include "superio/winbond/w83627hf/w83627hf.h" src/mainboard/supermicro/x6dai_g/auto.c:#include "superio/winbond/w83627hf/w83627hf.h" src/mainboard/supermicro/x6dhe_g/auto.c:#include "superio/winbond/w83627hf/w83627hf.h" src/mainboard/supermicro/x6dhe_g2/auto.updated.c:#include "superio/winbond/w83627hf/w83627hf.h" src/mainboard/supermicro/x6dhr_ig/auto.c:#include "superio/winbond/w83627hf/w83627hf.h" src/mainboard/supermicro/x6dhr_ig2/auto.c:#include "superio/winbond/w83627hf/w83627hf.h"
ah, ok then forget my comment. instead
Acked-by: Stefan Reinauer stepan@coresystems.de
On Sat, Mar 31, 2007 at 09:27:32PM +0200, Stefan Reinauer wrote:
Acked-by: Stefan Reinauer stepan@coresystems.de
Committed in r2574, thanks!
Uwe.
-
Stefan Reinauer -
Uwe Hermann