Please consider the replied thread for review.
-----Original Message-----
From: Khandelwal, Rajat <[email protected]>
Sent: Tuesday, August 9, 2022 3:19 AM
To: [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; Khandelwal, Rajat <[email protected]>; Rao, Abhijeet <[email protected]>
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.
Signed-off-by: Rajat Khandelwal <[email protected]>
---
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;
- }
-
/* 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
Hi Rajat,
On Mon, Aug 8, 2022 at 11:32 AM Khandelwal, Rajat
<[email protected]> wrote:
>
> Please consider the replied thread for review.
>
> -----Original Message-----
> From: Khandelwal, Rajat <[email protected]>
> Sent: Tuesday, August 9, 2022 3:19 AM
> To: [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; Khandelwal, Rajat <[email protected]>; Rao, Abhijeet <[email protected]>
> 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 <[email protected]>
> ---
> 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
>