Re: [PATCH] platform/chrome: Adding support for visibility of alt modes under their respective 'active' attributes
From: Prashant Malani
Date: Mon Aug 08 2022 - 16:46:36 EST
Hi Rajat,
On Mon, Aug 8, 2022 at 11:32 AM Khandelwal, Rajat
<rajat.khandelwal@xxxxxxxxx> wrote:
>
> Please consider the replied thread for review.
>
> -----Original Message-----
> From: Khandelwal, Rajat <rajat.khandelwal@xxxxxxxxx>
> Sent: Tuesday, August 9, 2022 3:19 AM
> To: pmalani@xxxxxxxxxxxx; bleung@xxxxxxxxxxxx; groeck@xxxxxxxxxxxx
> Cc: chrome-platform@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Khandelwal, Rajat <rajat.khandelwal@xxxxxxxxx>; Rao, Abhijeet <abhijeet.rao@xxxxxxxxx>
> Subject: [PATCH] Adding support for visibility of alt modes under their respective 'active' attributes
>
> In the current implementation of cros-ec-typec driver, 'active'
> attribute of type-C class doesn't reflect the current status of active
> mode. It's hardwired to 'no'. This patch adds the functionality to
> userspace.
> After this patch:
> localhost /sys/bus/typec/devices # cat port2-partner.1/active
> yes
> This was just an example of DP alt mode reflecting it's active status as
> 'yes', not 'no'
> Same goes for TBT alt mode.
>
> Let me know what you think of this as the userspace attribute currently
> has no significance.
For this reason alone (there is no real userspace use for this
attribute), I don't think this patch
is required. We're in the process of adding support for kernel
alternate mode drivers; that
will ensure this property is updated correctly.
Let's not add temporary solutions which we then have to carry for
years (if it can be avoided).
>
> Signed-off-by: Rajat Khandelwal <rajat.khandelwal@xxxxxxxxx>
> ---
> drivers/platform/chrome/cros_ec_typec.c | 52 ++++++++++++++++++-------
> 1 file changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 7cb2e35c4ded..0de221ea22db 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -32,6 +32,12 @@ enum {
> CROS_EC_ALTMODE_MAX,
> };
>
> +/* Supported alt modes and their SVIDs */
> +enum {
> + CROS_EC_ALTMODE_DP_SVID = 0xff01,
> + CROS_EC_ALTMODE_TBT_SVID = 0x8087,
> +};
> +
> /* Container for altmode pointer nodes. */
> struct cros_typec_altmode_node {
> struct typec_altmode *amode;
> @@ -514,25 +520,14 @@ static int cros_typec_enable_usb4(struct cros_typec_data *typec,
> }
>
> static int cros_typec_configure_mux(struct cros_typec_data *typec, int port_num,
> - struct ec_response_usb_pd_control_v2 *pd_ctrl)
> + struct ec_response_usb_pd_control_v2 *pd_ctrl,
> + struct ec_response_usb_pd_mux_info resp)
> {
> struct cros_typec_port *port = typec->ports[port_num];
> - struct ec_response_usb_pd_mux_info resp;
> - struct ec_params_usb_pd_mux_info req = {
> - .port = port_num,
> - };
> struct ec_params_usb_pd_mux_ack mux_ack;
> enum typec_orientation orientation;
> int ret;
>
> - ret = cros_ec_command(typec->ec, 0, EC_CMD_USB_PD_MUX_INFO,
> - &req, sizeof(req), &resp, sizeof(resp));
> - if (ret < 0) {
> - dev_warn(typec->dev, "Failed to get mux info for port: %d, err = %d\n",
> - port_num, ret);
> - return ret;
> - }
> -
We shouldn't be using MUX_INFO command outside of the configure_mux() command,
or for anything other than configuration of muxes.
> /* No change needs to be made, let's exit early. */
> if (port->mux_flags == resp.flags && port->role == pd_ctrl->role)
> return 0;
> @@ -945,8 +940,14 @@ static void cros_typec_handle_status(struct cros_typec_data *typec, int port_num
>
> static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
> {
> + struct cros_typec_port *port = typec->ports[port_num];
> struct ec_params_usb_pd_control req;
> + struct ec_params_usb_pd_mux_info req_pd_mux = {
> + .port = port_num,
> + };
> struct ec_response_usb_pd_control_v2 resp;
> + struct ec_response_usb_pd_mux_info pd_mux;
> + struct cros_typec_altmode_node *node, *n;
> int ret;
>
> if (port_num < 0 || port_num >= typec->num_ports) {
> @@ -966,8 +967,16 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
> if (ret < 0)
> return ret;
>
> + ret = cros_ec_command(typec->ec, 0, EC_CMD_USB_PD_MUX_INFO,
> + &req_pd_mux, sizeof(req_pd_mux), &pd_mux, sizeof(pd_mux));
> + if (ret < 0) {
> + dev_warn(typec->dev, "Failed to get mux info for port: %d, err = %d\n",
> + port_num, ret);
> + return ret;
> + }
> +
> /* Update the switches if they exist, according to requested state */
> - ret = cros_typec_configure_mux(typec, port_num, &resp);
> + ret = cros_typec_configure_mux(typec, port_num, &resp, pd_mux);
> if (ret)
> dev_warn(typec->dev, "Configure muxes failed, err = %d\n", ret);
>
> @@ -986,6 +995,21 @@ static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
> if (typec->typec_cmd_supported)
> cros_typec_handle_status(typec, port_num);
>
> + list_for_each_entry_safe(node, n, &port->partner_mode_list, list) {
> + if (node->amode->svid == CROS_EC_ALTMODE_DP_SVID &&
> + !(pd_mux.flags & USB_PD_MUX_DP_ENABLED))
> + typec_altmode_update_active(node->amode, false);
> + else if (node->amode->svid == CROS_EC_ALTMODE_TBT_SVID &&
> + !(pd_mux.flags & USB_PD_MUX_TBT_COMPAT_ENABLED))
> + typec_altmode_update_active(node->amode, false);
> + else if (node->amode->svid == CROS_EC_ALTMODE_DP_SVID &&
> + (pd_mux.flags & USB_PD_MUX_DP_ENABLED))
> + typec_altmode_update_active(node->amode, true);
> + else if (node->amode->svid == CROS_EC_ALTMODE_TBT_SVID &&
> + (pd_mux.flags & USB_PD_MUX_TBT_COMPAT_ENABLED))
> + typec_altmode_update_active(node->amode, true);
> + }
> +
> return 0;
> }
>
> --
> 2.17.1
>