View Change

12 comments:

• Commit Message:

  • Patch Set #2, Line 10: APIS

    APIs

  • Patch Set #2, Line 17: Type-A dongle on Volteer

    no need to indent here

• File src/ec/google/chromeec/ec.h:

  • Patch Set #2, Line 30: /* Check for the current mux state in EC */

    I realize the comments in here right now are terrible, but it doesn't have to be that way! Can we improve these new ones? e.g., it looks like *flags is an `out` parameter, can we document that? and also that `port` is the port number from the EC's point of view?

  • Patch Set #2, Line 33:

     * USB2 and USB3 port numbers between EC and AP
     * is not one to one mapping, this function will return the
     * correct mapped AP port number in port_map.
    

    80 chars wide is OK here.

  • Patch Set #2, Line 36: */

    Same thing, more documentation please. *port_map is another `out` parameter, and `port` is from EC's point of view?

  • Patch Set #2, Line 39: int google_chromeec_usb_pd_control(int port, bool *ufp, bool *dbg_acc,

    What are the possible values of dp_mode?

• File src/ec/google/chromeec/ec.c:

  • Patch Set #2, Line 1446:

    	struct ec_response_usb_pd_control_v2 resp = {};
    	struct chromeec_command cmd = {
    		.cmd_code = EC_CMD_USB_PD_CONTROL,
    		.cmd_version = 2,
    		.cmd_data_in = &pd_control,
    		.cmd_size_in = sizeof(pd_control),
    		.cmd_data_out = &resp,
    		.cmd_size_out = sizeof(resp),
    		.cmd_dev_index = 0,
    	};
    
    

    Are we sure about this interface yet? The EC code looks like it's still has some TODOs, etc.

  • Patch Set #2, Line 1460: PD_CC_DFP_ATTACHED

    what about PD_CC_DFP_DEBUG_ACC ? That would still indicate it's UFP rather than DFP. Otherwise how do you tell the difference between PD_CC_DFP_DEBUG_ACC and PD_CC_UFP_DEBUG_ACC?

  • Patch Set #2, Line 1495:

     * Check for the current mux state in EC
     * Flags representing mux state can be
     * found in ec_commands.h
    

    80 chars wide

  • Patch Set #2, Line 1501:

    	if (port < 0)
    		return -1;
    

    this should go after the declarations

  • Patch Set #2, Line 1516: if (google_chromeec_command(&cmd) < 0)

    blank line after declarations

  • Patch Set #2, Line 1531:

    	uint8_t num_ports;
    	int ret;
    	struct chromeec_command cmd;
    	int i;
    
    	ret = google_chromeec_get_num_pd_ports(&num_ports);
    	if (ret < 0)
    		return -1;
    
    	for (i = 0; i < num_ports; i++) 
    

    Thanks for the refactor! But would you mind dumping this into a separate commit?

To view, visit change 42078. To unsubscribe, or for help writing mail filters, visit settings.