[coreboot] New patch to review for coreboot: 0f25d3c Add support for Google ChromeEC
Peter Stuge
peter at stuge.se
Fri Feb 22 23:36:29 CET 2013
Stefan Reinauer wrote:
> +++ b/src/ec/google/chromeec/ec.c
..
> +/* an internal API to send a command to the EC and wait for response. */
> +struct chromeec_command {
> + u8 cmd_code; /* command code in, status out */
> + u8 cmd_version; /* command version */
> + const void* cmd_data_in; /* command data, if any */
> + void* cmd_data_out; /* command response, if any */
> + u16 cmd_size_in; /* size of command data */
> + u16 cmd_size_out; /* expected size of command response in,
> + * actual received size out */
> +};
No need for __packed since the struct only goes to lower layer code
which packs the actual command?
> +int google_chromeec_kbbacklight(int percent)
> +{
> + struct chromeec_command cec_cmd;
> + struct ec_params_pwm_set_keyboard_backlight cmd_backlight;
> + struct ec_response_pwm_get_keyboard_backlight rsp_backlight;
> + /* if they were dumb, help them out */
> + percent = percent % 101;
Please:
percent = MAX(percent, 0);
percent = MIN(percent, 100);
The first one can of course be skipped if the parameter is unsigned.
> +void google_chromeec_post(u8 postcode)
> +{
> + /* backlight is a percent. postcode is a u8.
> + * Convert the u8 to %.
> + */
> + postcode = (postcode/4) + (postcode/8);
> + google_chromeec_kbbacklight(postcode);
> +}
Cute.
> +u32 google_chromeec_get_events_b(void)
> +{
> + return google_chromeec_get_mask(EC_CMD_HOST_EVENT_GET_B);
> +}
What's "b" ? That's seriously undescriptive. From ACPI?
> +static
> +int google_chromeec_hello(void)
> +{
> + struct chromeec_command cec_cmd;
> + struct ec_params_hello cmd_hello;
> + struct ec_response_hello rsp_hello;
> + cmd_hello.in_data = 0x10203040;
> + cec_cmd.cmd_code = EC_CMD_HELLO;
> + cec_cmd.cmd_version = 0;
> + cec_cmd.cmd_data_in = &cmd_hello.in_data;
> + cec_cmd.cmd_data_out = &rsp_hello.out_data;
> + cec_cmd.cmd_size_in = sizeof(cmd_hello.in_data);
> + cec_cmd.cmd_size_out = sizeof(rsp_hello.out_data);
Statically initializing the struct would look a little nicer:
struct chromeec_command cec_cmd = {
.cmd_code = EC_CMD_HELLO,
.cmd_version = 0,
...
};
> +static void google_chromeec_init(device_t dev)
> +{
..
> + if (cec_cmd.cmd_code ||
> + (recovery_mode_enabled() &&
> + (lpcv_cmd.current_image != EC_IMAGE_RO))) {
> + struct ec_params_reboot_ec reboot_ec;
> + /* Reboot the EC and make it come back in RO mode */
> + reboot_ec.cmd = EC_REBOOT_COLD;
> + reboot_ec.flags = 0;
> + cec_cmd.cmd_code = EC_CMD_REBOOT_EC;
> + cec_cmd.cmd_version = 0;
> + cec_cmd.cmd_data_in = &reboot_ec;
> + cec_cmd.cmd_size_in = sizeof(reboot_ec);
> + cec_cmd.cmd_size_out = 0; /* ignore response, if any */
> + printk(BIOS_DEBUG, "Rebooting with EC in RO mode:\n");
> + google_chromeec_command(&cec_cmd);
> + udelay(1000);
> + hard_reset();
> + hlt();
> + }
> +
> +}
How about die() at the end instead of hlt()? I believe hlt returns.
All in all, lovely work! I guess you had lots of fun while debugging.
//Peter
More information about the coreboot
mailing list