[coreboot] New patch to review for coreboot: 0f25d3c Add support for Google ChromeEC

[Peter Stuge]

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