Hello,
I was looking at the code and ACPI for a number of different embedded controllers while adding support for a Lenovo mainboard. The mainboard in question has an EC nearly identical to the Twist S230U (Compal KB9012), so I planned to factor out the S230U's EC code/ACPI into its own chip.
While doing this, I noticed there were several duplicate functions across the `ec/` directory, so I'd like to potentially submit some patches to clean them up. However, I'm not sure of the best way to go about this.
The following is what I currently have in mind:
- Removing most individual `ec_write_cmd()` and similar functions andreplacing calls with their respective `ec/acpi/ec.c` library functions - Removing individual `kbc_` polling functions and sharing the ones defined in `drivers/pc80/keyboard.c` - Should public library functions for sending and receiving data from a PC80 keyboard controller be defined in the PC80 driver and exported in `include/pc80/keyboard.h`? - We already have `send_keyboard()` there, which sends a command and receives data - would it be good to add individual functions for those operations as well?
Any suggestions or recommendations would be appreciated. I looked for guidelines on suggested code organization in the docs/commit history/ mailing list, but came up mostly empty-handed.
Thanks!
If this could help you, coreboot-supported Lenovo G505S also has EC KB9012
чт, 7 апр. 2022 г. в 06:44, Abel Briggs abelbriggs1@hotmail.com:
Hello,
I was looking at the code and ACPI for a number of different embedded controllers while adding support for a Lenovo mainboard. The mainboard in question has an EC nearly identical to the Twist S230U (Compal KB9012), so I planned to factor out the S230U's EC code/ACPI into its own chip.
While doing this, I noticed there were several duplicate functions across the `ec/` directory, so I'd like to potentially submit some patches to clean them up. However, I'm not sure of the best way to go about this.
The following is what I currently have in mind:
- Removing most individual `ec_write_cmd()` and similar functions andreplacing calls with their respective `ec/acpi/ec.c` library functions
- Removing individual `kbc_` polling functions and sharing the ones defined in `drivers/pc80/keyboard.c`
- Should public library functions for sending and receiving data from a PC80 keyboard controller be defined in the PC80 driver and exported in `include/pc80/keyboard.h`?
- We already have `send_keyboard()` there, which sends a command and receives data - would it be good to add individual functions for those operations as well?
Any suggestions or recommendations would be appreciated. I looked for guidelines on suggested code organization in the docs/commit history/ mailing list, but came up mostly empty-handed.
Thanks!
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
Yes, I was looking at the G505S and its EC when considering these patches.
The situation with the EC in the G505S seems a bit odd. The EC is named `ene932`, but there are several commits in the codebase which reference the chip as a KB9012. The KB9012 datasheet, however, reference the ENE930 series chips as the predecessor chip to the KB9010 series.
The S230U has a KB9012 and the Compal firmware seems to have a similar `OperationRegion` layout to the firmware in the `ene932`, but the EC query values are completely different. The S230U basically defines its own EC in its mainboard tree and only uses the `ene932` code for its helper I/O space functions.
If anyone has a G505S or its vendor documentation to check that the chip on the board is really an ENE930 series, I think that would help clear this up.
This is something I was considering trying to refactor as well, by giving the S230U its own `ec/` common code. However, I would probably throw that patch in with my mainboard's patchset (Lenovo Edge E530), since otherwise the S230U would be the only user of that code.
________________________________ From: Ivan Ivanov qmastery16@gmail.com Sent: Friday, April 8, 2022, 4:38 AM To: Abel Briggs abelbriggs1@hotmail.com Cc: coreboot@coreboot.org coreboot@coreboot.org Subject: Re: [coreboot] Refactoring duplicate Embedded Controller code
If this could help you, coreboot-supported Lenovo G505S also has EC KB9012
чт, 7 апр. 2022 г. в 06:44, Abel Briggs abelbriggs1@hotmail.com:
Hello,
I was looking at the code and ACPI for a number of different embedded controllers while adding support for a Lenovo mainboard. The mainboard in question has an EC nearly identical to the Twist S230U (Compal KB9012), so I planned to factor out the S230U's EC code/ACPI into its own chip.
While doing this, I noticed there were several duplicate functions across the `ec/` directory, so I'd like to potentially submit some patches to clean them up. However, I'm not sure of the best way to go about this.
The following is what I currently have in mind:
- Removing most individual `ec_write_cmd()` and similar functions andreplacing calls with their respective `ec/acpi/ec.c` library functions
- Removing individual `kbc_` polling functions and sharing the ones defined in `drivers/pc80/keyboard.c`
- Should public library functions for sending and receiving data from a PC80 keyboard controller be defined in the PC80 driver and exported in `include/pc80/keyboard.h`?
- We already have `send_keyboard()` there, which sends a command and receives data - would it be good to add individual functions for those operations as well?
Any suggestions or recommendations would be appreciated. I looked for guidelines on suggested code organization in the docs/commit history/ mailing list, but came up mostly empty-handed.
Thanks!
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
-
Abel Briggs -
Ivan Ivanov