Re: [coreboot] [PATCH] Add new ec subdir for Embedded Controllers

[Stefan Reinauer]

* Sven Schnelle  [110117 21:46]:
> Index: src/ec/acpi/ec.h
>
> --- src/ec/acpi/ec.h	(revision 0)
> +++ src/ec/acpi/ec.h	(working copy)
> @@ -17,9 +17,11 @@
> Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
>
> */
>
> -#ifndef _MAINBOARD_EC_H
> -#define _MAINBOARD_EC_H
> +#ifndef _EC_ACPI_H
> +#define _EC_ACPI_H
>
> +#include "chip.h"

Is chip.h needed ?
> #define EC_DATA	0x62
>  #define EC_SC	0x66
> @@ -28,6 +28,7 @@
>    select SOUTHBRIDGE_TI_PCIXX12
>    select SUPERIO_SMSC_FDC37N972
>    select SUPERIO_SMSC_SIO10N268
> select EC_ACPI
> select BOARD_HAS_FADT
> select HAVE_ACPI_TABLES
> select HAVE_PIRQ_TABLE

Maybe we want to add support for explicit EC types and reference those
here? And then have the acpi code shared between all those ECs.
The Roda RK886EX has it's EC code spread out into src/superio/renesas,
too.
> Index: src/mainboard/getac/p470/mainboard_smi.c
>
> --- src/mainboard/getac/p470/mainboard_smi.c	(revision 6260)
> +++ src/mainboard/getac/p470/mainboard_smi.c	(working copy)
> @@ -26,7 +26,8 @@
>  #include "southbridge/intel/i82801gx/i82801gx.h"
>  #include "southbridge/intel/i82801gx/nvs.h"
>  #include "northbridge/intel/i945/udelay.c"
> -#include "ec.c"
> +#include <ec/acpi/ec.h>

ec/acpi.h?
> +#include "ec_oem.c"
>
> #define MAX_LCD_BRIGHTNESS 0xd8
> Index: src/mainboard/getac/p470/Makefile.inc
>
> --- src/mainboard/getac/p470/Makefile.inc	(revision 6260)
> +++ src/mainboard/getac/p470/Makefile.inc	(working copy)
> @@ -21,3 +21,4 @@
>  ramstage-$(CONFIG_HAVE_ACPI_SLIC) += acpi_slic.c
>
> smm-$(CONFIG_HAVE_SMI_HANDLER) += mainboard_smi.c
> +smm-$(CONFIG_HAVE_SMI_HANDLER) += ../../../ec/acpi/ec.c

shouldn't the EC makefile in src/ec/acpi say

smm-$(CONFIG_HAVE_SMI_HANDLER) += ec.c instead?
> Index: src/mainboard/getac/p470/devicetree.cb
>
> --- src/mainboard/getac/p470/devicetree.cb	(revision 6260)
> +++ src/mainboard/getac/p470/devicetree.cb	(working copy)
> @@ -133,6 +133,12 @@
>    				end
>    				device pnp 4e.b off		# HWM
>    				end
> 			chip ec/acpi
> 				device pnp ff.1 on
> 					io 0x60 = 0x62
> 					io 0x62 = 0x66
> 				end
> 			end

what's the device pnp and io 0x60/io 0x62 used for? this does not look
too good. I would wipe it.

Stefan