2020-11-06 01:30:11

by Prashant Malani

[permalink] [raw]
Subject: [PATCH 0/6] platform/chrome: cros_ec_typec: Add cable registration

The following series adds Type C cable registration to the cros-ec-typec
driver. The first few patches perform a few minor re-organizations to
prepare for the cable registration patch.

The last couple of CLs update the USB PD VDO header file to add a
captive cable connector for the Type C cable plug field, and then use
the added macro to add the corresponding field of the Type C cable
descriptor in the cros-ec-typec driver.

Prashant Malani (6):
platform/chrome: cros_ec_typec: Make disc_done flag partner-only
platform/chrome: cros_ec_typec: Factor out PD identity parsing
platform/chrome: cros_ec_typec: Rename discovery struct
platform/chrome: cros_ec_typec: Register cable
usb: pd: Add captive Type C cable type
platform/chrome: cros_ec_typec: Store cable plug type

drivers/platform/chrome/cros_ec_typec.c | 148 +++++++++++++++++++-----
include/linux/usb/pd_vdo.h | 4 +-
2 files changed, 123 insertions(+), 29 deletions(-)

--
2.29.1.341.ge80a0c044ae-goog


2020-11-06 01:31:10

by Prashant Malani

[permalink] [raw]
Subject: [PATCH 2/6] platform/chrome: cros_ec_typec: Factor out PD identity parsing

Factor out the PD identity parsing code into a separate function. This
way it can be re-used for Cable PD identity parsing in future patches.

No functional changes are introduced by this patch.

Signed-off-by: Prashant Malani <[email protected]>
---
drivers/platform/chrome/cros_ec_typec.c | 35 ++++++++++++++++---------
1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 801c3d2c1fbd..f6d3c37c2c27 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -657,6 +657,28 @@ static int cros_typec_register_altmodes(struct cros_typec_data *typec, int port_
return ret;
}

+/*
+ * Parse the PD identity data from the EC PD discovery responses and copy that to the supplied
+ * PD identity struct.
+ */
+static void cros_typec_parse_pd_identity(struct usb_pd_identity *id,
+ struct ec_response_typec_discovery *disc)
+{
+ int i;
+
+ /* First, update the PD identity VDOs for the partner. */
+ if (disc->identity_count > 0)
+ id->id_header = disc->discovery_vdo[0];
+ if (disc->identity_count > 1)
+ id->cert_stat = disc->discovery_vdo[1];
+ if (disc->identity_count > 2)
+ id->product = disc->discovery_vdo[2];
+
+ /* Copy the remaining identity VDOs till a maximum of 6. */
+ for (i = 3; i < disc->identity_count && i < VDO_MAX_OBJECTS; i++)
+ id->vdo[i - 3] = disc->discovery_vdo[i];
+}
+
static int cros_typec_handle_sop_disc(struct cros_typec_data *typec, int port_num)
{
struct cros_typec_port *port = typec->ports[port_num];
@@ -666,7 +688,6 @@ static int cros_typec_handle_sop_disc(struct cros_typec_data *typec, int port_nu
.partner_type = TYPEC_PARTNER_SOP,
};
int ret = 0;
- int i;

if (!port->partner) {
dev_err(typec->dev,
@@ -684,17 +705,7 @@ static int cros_typec_handle_sop_disc(struct cros_typec_data *typec, int port_nu
goto disc_exit;
}

- /* First, update the PD identity VDOs for the partner. */
- if (sop_disc->identity_count > 0)
- port->p_identity.id_header = sop_disc->discovery_vdo[0];
- if (sop_disc->identity_count > 1)
- port->p_identity.cert_stat = sop_disc->discovery_vdo[1];
- if (sop_disc->identity_count > 2)
- port->p_identity.product = sop_disc->discovery_vdo[2];
-
- /* Copy the remaining identity VDOs till a maximum of 6. */
- for (i = 3; i < sop_disc->identity_count && i < VDO_MAX_OBJECTS; i++)
- port->p_identity.vdo[i - 3] = sop_disc->discovery_vdo[i];
+ cros_typec_parse_pd_identity(&port->p_identity, sop_disc);

ret = typec_partner_set_identity(port->partner);
if (ret < 0) {
--
2.29.1.341.ge80a0c044ae-goog

2020-11-06 01:31:41

by Prashant Malani

[permalink] [raw]
Subject: [PATCH 4/6] platform/chrome: cros_ec_typec: Register cable

When the Chrome Embedded Controller notifies the driver that SOP'
discovery is complete, retrieve the PD discovery data and register a
cable object with the Type C connector class framework.

Cc: Heikki Krogerus <[email protected]>
Signed-off-by: Prashant Malani <[email protected]>
---
drivers/platform/chrome/cros_ec_typec.c | 67 +++++++++++++++++++++++++
1 file changed, 67 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 3c8ff07c8803..5e7f0b4ebbec 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -44,8 +44,11 @@ struct cros_typec_port {
/* Initial capabilities for the port. */
struct typec_capability caps;
struct typec_partner *partner;
+ struct typec_cable *cable;
/* Port partner PD identity info. */
struct usb_pd_identity p_identity;
+ /* Port cable PD identity info. */
+ struct usb_pd_identity c_identity;
struct typec_switch *ori_sw;
struct typec_mux *mux;
struct usb_role_switch *role_sw;
@@ -59,6 +62,7 @@ struct cros_typec_port {

/* Flag indicating that PD partner discovery data parsing is completed. */
bool sop_disc_done;
+ bool sop_prime_disc_done;
struct ec_response_typec_discovery *disc_data;
struct list_head partner_mode_list;
};
@@ -213,6 +217,17 @@ static void cros_typec_remove_partner(struct cros_typec_data *typec,
port->sop_disc_done = false;
}

+static void cros_typec_remove_cable(struct cros_typec_data *typec,
+ int port_num)
+{
+ struct cros_typec_port *port = typec->ports[port_num];
+
+ typec_unregister_cable(port->cable);
+ port->cable = NULL;
+ memset(&port->c_identity, 0, sizeof(port->c_identity));
+ port->sop_prime_disc_done = false;
+}
+
static void cros_unregister_ports(struct cros_typec_data *typec)
{
int i;
@@ -224,6 +239,9 @@ static void cros_unregister_ports(struct cros_typec_data *typec)
if (typec->ports[i]->partner)
cros_typec_remove_partner(typec, i);

+ if (typec->ports[i]->cable)
+ cros_typec_remove_cable(typec, i);
+
usb_role_switch_put(typec->ports[i]->role_sw);
typec_switch_put(typec->ports[i]->ori_sw);
typec_mux_put(typec->ports[i]->mux);
@@ -600,6 +618,9 @@ static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
if (!typec->ports[port_num]->partner)
return;
cros_typec_remove_partner(typec, port_num);
+
+ if (typec->ports[port_num]->cable)
+ cros_typec_remove_cable(typec, port_num);
}
}

@@ -679,6 +700,43 @@ static void cros_typec_parse_pd_identity(struct usb_pd_identity *id,
id->vdo[i - 3] = disc->discovery_vdo[i];
}

+static int cros_typec_handle_sop_prime_disc(struct cros_typec_data *typec, int port_num)
+{
+ struct cros_typec_port *port = typec->ports[port_num];
+ struct ec_response_typec_discovery *disc = port->disc_data;
+ struct typec_cable_desc desc = {};
+ struct ec_params_typec_discovery req = {
+ .port = port_num,
+ .partner_type = TYPEC_PARTNER_SOP_PRIME,
+ };
+ int ret = 0;
+
+ memset(disc, 0, EC_PROTO2_MAX_RESPONSE_SIZE);
+ ret = cros_typec_ec_command(typec, 0, EC_CMD_TYPEC_DISCOVERY, &req, sizeof(req),
+ disc, EC_PROTO2_MAX_RESPONSE_SIZE);
+ if (ret < 0) {
+ dev_err(typec->dev, "Failed to get SOP' discovery data for port: %d\n", port_num);
+ goto sop_prime_disc_exit;
+ }
+
+ /* Parse the PD identity data, even if only 0s were returned. */
+ cros_typec_parse_pd_identity(&port->c_identity, disc);
+
+ if (disc->identity_count != 0)
+ desc.active = PD_IDH_PTYPE(port->c_identity.id_header) == IDH_PTYPE_ACABLE;
+
+ desc.identity = &port->c_identity;
+
+ port->cable = typec_register_cable(port->port, &desc);
+ if (IS_ERR(port->cable)) {
+ ret = PTR_ERR(port->cable);
+ port->cable = NULL;
+ }
+
+sop_prime_disc_exit:
+ return ret;
+}
+
static int cros_typec_handle_sop_disc(struct cros_typec_data *typec, int port_num)
{
struct cros_typec_port *port = typec->ports[port_num];
@@ -746,6 +804,15 @@ static void cros_typec_handle_status(struct cros_typec_data *typec, int port_num
else
typec->ports[port_num]->sop_disc_done = true;
}
+
+ if (resp.events & PD_STATUS_EVENT_SOP_PRIME_DISC_DONE &&
+ !typec->ports[port_num]->sop_prime_disc_done) {
+ ret = cros_typec_handle_sop_prime_disc(typec, port_num);
+ if (ret < 0)
+ dev_err(typec->dev, "Couldn't parse SOP' Disc data, port: %d\n", port_num);
+ else
+ typec->ports[port_num]->sop_prime_disc_done = true;
+ }
}

static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
--
2.29.1.341.ge80a0c044ae-goog

2020-11-06 01:32:24

by Prashant Malani

[permalink] [raw]
Subject: [PATCH 5/6] usb: pd: Add captive Type C cable type

The USB Power Delivery Specification R3.0 adds a captive cable type
to the "USB Type-C plug to USB Type-C/Captive" field (Bits 19-18,
Passive/Active Cable VDO, Table 6-38 & 6-39).

Add the corresponding definition to the Cable VDO header. Also add a
helper macro to get the Type C cable connector type, when provided
the cable VDO.

Cc: Heikki Krogerus <[email protected]>
Signed-off-by: Prashant Malani <[email protected]>
---
include/linux/usb/pd_vdo.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/usb/pd_vdo.h b/include/linux/usb/pd_vdo.h
index 68bdc4e2f5a9..8c5cb5830754 100644
--- a/include/linux/usb/pd_vdo.h
+++ b/include/linux/usb/pd_vdo.h
@@ -177,7 +177,7 @@
* <31:28> :: Cable HW version
* <27:24> :: Cable FW version
* <23:20> :: Reserved, Shall be set to zero
- * <19:18> :: type-C to Type-A/B/C (00b == A, 01 == B, 10 == C)
+ * <19:18> :: type-C to Type-A/B/C/Captive (00b == A, 01 == B, 10 == C, 11 == Captive)
* <17> :: Type-C to Plug/Receptacle (0b == plug, 1b == receptacle)
* <16:13> :: cable latency (0001 == <10ns(~1m length))
* <12:11> :: cable termination type (11b == both ends active VCONN req)
@@ -193,6 +193,7 @@
#define CABLE_ATYPE 0
#define CABLE_BTYPE 1
#define CABLE_CTYPE 2
+#define CABLE_CAPTIVE 3
#define CABLE_PLUG 0
#define CABLE_RECEPTACLE 1
#define CABLE_CURR_1A5 0
@@ -208,6 +209,7 @@
| (tx1d) << 10 | (tx2d) << 9 | (rx1d) << 8 | (rx2d) << 7 \
| ((cur) & 0x3) << 5 | (vps) << 4 | (sopp) << 3 \
| ((usbss) & 0x7))
+#define VDO_TYPEC_CABLE_TYPE(vdo) (((vdo) >> 18) & 0x3)

/*
* AMA VDO
--
2.29.1.341.ge80a0c044ae-goog

2020-11-06 01:32:43

by Prashant Malani

[permalink] [raw]
Subject: [PATCH 1/6] platform/chrome: cros_ec_typec: Make disc_done flag partner-only

Change the disc_done flag, which indicates whether PD discovery is
complete, to sop_disc_done instead, since we will process SOP and SOP'
discovery data separately.

Signed-off-by: Prashant Malani <[email protected]>
---
drivers/platform/chrome/cros_ec_typec.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index ce031a10eb1b..801c3d2c1fbd 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -57,8 +57,8 @@ struct cros_typec_port {
/* Port alt modes. */
struct typec_altmode p_altmode[CROS_EC_ALTMODE_MAX];

- /* Flag indicating that PD discovery data parsing is completed. */
- bool disc_done;
+ /* Flag indicating that PD partner discovery data parsing is completed. */
+ bool sop_disc_done;
struct ec_response_typec_discovery *sop_disc;
struct list_head partner_mode_list;
};
@@ -210,7 +210,7 @@ static void cros_typec_remove_partner(struct cros_typec_data *typec,
typec_unregister_partner(port->partner);
port->partner = NULL;
memset(&port->p_identity, 0, sizeof(port->p_identity));
- port->disc_done = false;
+ port->sop_disc_done = false;
}

static void cros_unregister_ports(struct cros_typec_data *typec)
@@ -727,18 +727,13 @@ static void cros_typec_handle_status(struct cros_typec_data *typec, int port_num
return;
}

- if (typec->ports[port_num]->disc_done)
- return;
-
/* Handle any events appropriately. */
- if (resp.events & PD_STATUS_EVENT_SOP_DISC_DONE) {
+ if (resp.events & PD_STATUS_EVENT_SOP_DISC_DONE && !typec->ports[port_num]->sop_disc_done) {
ret = cros_typec_handle_sop_disc(typec, port_num);
- if (ret < 0) {
+ if (ret < 0)
dev_err(typec->dev, "Couldn't parse SOP Disc data, port: %d\n", port_num);
- return;
- }
-
- typec->ports[port_num]->disc_done = true;
+ else
+ typec->ports[port_num]->sop_disc_done = true;
}
}

--
2.29.1.341.ge80a0c044ae-goog

2020-11-06 01:33:09

by Prashant Malani

[permalink] [raw]
Subject: [PATCH 6/6] platform/chrome: cros_ec_typec: Store cable plug type

Use the PD VDO Type C cable plug type macro to retrieve and store the
cable plug type in the cable descriptor.

Cc: Heikki Krogerus <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Prashant Malani <[email protected]>
---
drivers/platform/chrome/cros_ec_typec.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 5e7f0b4ebbec..0a2a8b0f8115 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -709,6 +709,7 @@ static int cros_typec_handle_sop_prime_disc(struct cros_typec_data *typec, int p
.port = port_num,
.partner_type = TYPEC_PARTNER_SOP_PRIME,
};
+ uint32_t cable_plug_type;
int ret = 0;

memset(disc, 0, EC_PROTO2_MAX_RESPONSE_SIZE);
@@ -722,8 +723,26 @@ static int cros_typec_handle_sop_prime_disc(struct cros_typec_data *typec, int p
/* Parse the PD identity data, even if only 0s were returned. */
cros_typec_parse_pd_identity(&port->c_identity, disc);

- if (disc->identity_count != 0)
+ if (disc->identity_count != 0) {
+ cable_plug_type = VDO_TYPEC_CABLE_TYPE(port->c_identity.vdo[0]);
+ switch (cable_plug_type) {
+ case CABLE_ATYPE:
+ desc.type = USB_PLUG_TYPE_A;
+ break;
+ case CABLE_BTYPE:
+ desc.type = USB_PLUG_TYPE_B;
+ break;
+ case CABLE_CTYPE:
+ desc.type = USB_PLUG_TYPE_C;
+ break;
+ case CABLE_CAPTIVE:
+ desc.type = USB_PLUG_CAPTIVE;
+ break;
+ default:
+ desc.type = USB_PLUG_NONE;
+ }
desc.active = PD_IDH_PTYPE(port->c_identity.id_header) == IDH_PTYPE_ACABLE;
+ }

desc.identity = &port->c_identity;

--
2.29.1.341.ge80a0c044ae-goog

2020-11-06 01:33:30

by Prashant Malani

[permalink] [raw]
Subject: [PATCH 3/6] platform/chrome: cros_ec_typec: Rename discovery struct

Rename the sop_disc data struct which is used to store PD discovery data
to the more generic name of disc_data. It can then be re-used to store
and process cable discovery data.

Signed-off-by: Prashant Malani <[email protected]>
---
drivers/platform/chrome/cros_ec_typec.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index f6d3c37c2c27..3c8ff07c8803 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -59,7 +59,7 @@ struct cros_typec_port {

/* Flag indicating that PD partner discovery data parsing is completed. */
bool sop_disc_done;
- struct ec_response_typec_discovery *sop_disc;
+ struct ec_response_typec_discovery *disc_data;
struct list_head partner_mode_list;
};

@@ -323,8 +323,8 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)

cros_typec_register_port_altmodes(typec, port_num);

- cros_port->sop_disc = devm_kzalloc(dev, EC_PROTO2_MAX_RESPONSE_SIZE, GFP_KERNEL);
- if (!cros_port->sop_disc) {
+ cros_port->disc_data = devm_kzalloc(dev, EC_PROTO2_MAX_RESPONSE_SIZE, GFP_KERNEL);
+ if (!cros_port->disc_data) {
ret = -ENOMEM;
goto unregister_ports;
}
@@ -617,7 +617,7 @@ static int cros_typec_get_mux_info(struct cros_typec_data *typec, int port_num,
static int cros_typec_register_altmodes(struct cros_typec_data *typec, int port_num)
{
struct cros_typec_port *port = typec->ports[port_num];
- struct ec_response_typec_discovery *sop_disc = port->sop_disc;
+ struct ec_response_typec_discovery *sop_disc = port->disc_data;
struct cros_typec_altmode_node *node;
struct typec_altmode_desc desc;
struct typec_altmode *amode;
@@ -682,7 +682,7 @@ static void cros_typec_parse_pd_identity(struct usb_pd_identity *id,
static int cros_typec_handle_sop_disc(struct cros_typec_data *typec, int port_num)
{
struct cros_typec_port *port = typec->ports[port_num];
- struct ec_response_typec_discovery *sop_disc = port->sop_disc;
+ struct ec_response_typec_discovery *sop_disc = port->disc_data;
struct ec_params_typec_discovery req = {
.port = port_num,
.partner_type = TYPEC_PARTNER_SOP,
--
2.29.1.341.ge80a0c044ae-goog

2020-11-06 07:24:17

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 6/6] platform/chrome: cros_ec_typec: Store cable plug type

On Thu, Nov 05, 2020 at 05:28:03PM -0800, Prashant Malani wrote:
> Use the PD VDO Type C cable plug type macro to retrieve and store the
> cable plug type in the cable descriptor.
>
> Cc: Heikki Krogerus <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Prashant Malani <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_typec.c | 21 ++++++++++++++++++++-
> 1 file changed, 20 insertions(+), 1 deletion(-)

Where are the first 5 patches in this series?


>
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 5e7f0b4ebbec..0a2a8b0f8115 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -709,6 +709,7 @@ static int cros_typec_handle_sop_prime_disc(struct cros_typec_data *typec, int p
> .port = port_num,
> .partner_type = TYPEC_PARTNER_SOP_PRIME,
> };
> + uint32_t cable_plug_type;

u32 please, this isn't userspace code :)

> int ret = 0;
>
> memset(disc, 0, EC_PROTO2_MAX_RESPONSE_SIZE);
> @@ -722,8 +723,26 @@ static int cros_typec_handle_sop_prime_disc(struct cros_typec_data *typec, int p
> /* Parse the PD identity data, even if only 0s were returned. */
> cros_typec_parse_pd_identity(&port->c_identity, disc);
>
> - if (disc->identity_count != 0)
> + if (disc->identity_count != 0) {
> + cable_plug_type = VDO_TYPEC_CABLE_TYPE(port->c_identity.vdo[0]);
> + switch (cable_plug_type) {
> + case CABLE_ATYPE:
> + desc.type = USB_PLUG_TYPE_A;
> + break;
> + case CABLE_BTYPE:
> + desc.type = USB_PLUG_TYPE_B;
> + break;
> + case CABLE_CTYPE:
> + desc.type = USB_PLUG_TYPE_C;
> + break;
> + case CABLE_CAPTIVE:
> + desc.type = USB_PLUG_CAPTIVE;
> + break;
> + default:
> + desc.type = USB_PLUG_NONE;
> + }
> desc.active = PD_IDH_PTYPE(port->c_identity.id_header) == IDH_PTYPE_ACABLE;
> + }

So you save it but what happens with the value?

confused,

greg k-h

2020-11-06 09:03:31

by Prashant Malani

[permalink] [raw]
Subject: Re: [PATCH 6/6] platform/chrome: cros_ec_typec: Store cable plug type

Hi Greg,

Thanks for looking at the patch.

On Fri, Nov 06, 2020 at 08:20:59AM +0100, Greg Kroah-Hartman wrote:
> On Thu, Nov 05, 2020 at 05:28:03PM -0800, Prashant Malani wrote:
> > Use the PD VDO Type C cable plug type macro to retrieve and store the
> > cable plug type in the cable descriptor.
> >
> > Cc: Heikki Krogerus <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Signed-off-by: Prashant Malani <[email protected]>
> > ---
> > drivers/platform/chrome/cros_ec_typec.c | 21 ++++++++++++++++++++-
> > 1 file changed, 20 insertions(+), 1 deletion(-)
>
> Where are the first 5 patches in this series?

I believe I'd cc-ed you (and linux-usb) on the following patches in the
series(modified git_cc[1] which adds individual patch get_maintainers.pl to the cover
letter, patch 1 and patch 2):
cover letter:
https://lore.kernel.org/lkml/[email protected]/
patch 1/6 (cros-ec-typec cleanup):
https://lore.kernel.org/lkml/[email protected]/
patch 2/6 (cros-ec-typec cleanup):
https://lore.kernel.org/lkml/[email protected]/
patch 5/6 (PD VDO header patch):
https://lore.kernel.org/lkml/[email protected]/
patch 6/6 (this patch):
https://lore.kernel.org/lkml/[email protected]/

Did you not receive these?
>
>
> >
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index 5e7f0b4ebbec..0a2a8b0f8115 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -709,6 +709,7 @@ static int cros_typec_handle_sop_prime_disc(struct cros_typec_data *typec, int p
> > .port = port_num,
> > .partner_type = TYPEC_PARTNER_SOP_PRIME,
> > };
> > + uint32_t cable_plug_type;
>
> u32 please, this isn't userspace code :)

Will fix this in the next version.
>
> > int ret = 0;
> >
> > memset(disc, 0, EC_PROTO2_MAX_RESPONSE_SIZE);
> > @@ -722,8 +723,26 @@ static int cros_typec_handle_sop_prime_disc(struct cros_typec_data *typec, int p
> > /* Parse the PD identity data, even if only 0s were returned. */
> > cros_typec_parse_pd_identity(&port->c_identity, disc);
> >
> > - if (disc->identity_count != 0)
> > + if (disc->identity_count != 0) {
> > + cable_plug_type = VDO_TYPEC_CABLE_TYPE(port->c_identity.vdo[0]);
> > + switch (cable_plug_type) {
> > + case CABLE_ATYPE:
> > + desc.type = USB_PLUG_TYPE_A;
> > + break;
> > + case CABLE_BTYPE:
> > + desc.type = USB_PLUG_TYPE_B;
> > + break;
> > + case CABLE_CTYPE:
> > + desc.type = USB_PLUG_TYPE_C;
> > + break;
> > + case CABLE_CAPTIVE:
> > + desc.type = USB_PLUG_CAPTIVE;
> > + break;
> > + default:
> > + desc.type = USB_PLUG_NONE;
> > + }
> > desc.active = PD_IDH_PTYPE(port->c_identity.id_header) == IDH_PTYPE_ACABLE;
> > + }
>
> So you save it but what happens with the value?

The type C connector class framework exposes it via syfs to user-space when we
register the cable via typec_register_cable() in patch 4/6 [2].

I'll go ahead and CC
you and linux-usb on the entire series for the next version.

[1]: https://lwn.net/Articles/585782/
[2]:
https://lore.kernel.org/lkml/[email protected]/

Best regards,

2020-11-06 09:36:11

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 6/6] platform/chrome: cros_ec_typec: Store cable plug type

On Fri, Nov 06, 2020 at 12:59:07AM -0800, Prashant Malani wrote:
> Hi Greg,
>
> Thanks for looking at the patch.
>
> On Fri, Nov 06, 2020 at 08:20:59AM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Nov 05, 2020 at 05:28:03PM -0800, Prashant Malani wrote:
> > > Use the PD VDO Type C cable plug type macro to retrieve and store the
> > > cable plug type in the cable descriptor.
> > >
> > > Cc: Heikki Krogerus <[email protected]>
> > > Cc: Greg Kroah-Hartman <[email protected]>
> > > Signed-off-by: Prashant Malani <[email protected]>
> > > ---
> > > drivers/platform/chrome/cros_ec_typec.c | 21 ++++++++++++++++++++-
> > > 1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > Where are the first 5 patches in this series?
>
> I believe I'd cc-ed you (and linux-usb) on the following patches in the
> series(modified git_cc[1] which adds individual patch get_maintainers.pl to the cover
> letter, patch 1 and patch 2):
> cover letter:
> https://lore.kernel.org/lkml/[email protected]/
> patch 1/6 (cros-ec-typec cleanup):
> https://lore.kernel.org/lkml/[email protected]/
> patch 2/6 (cros-ec-typec cleanup):
> https://lore.kernel.org/lkml/[email protected]/
> patch 5/6 (PD VDO header patch):
> https://lore.kernel.org/lkml/[email protected]/
> patch 6/6 (this patch):
> https://lore.kernel.org/lkml/[email protected]/
>
> Did you not receive these?

Ah, I got 1, 2, and 5, and now 6. That's confusing, think about if you
were to receive such a series, what would you think to do with it?

> > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > > index 5e7f0b4ebbec..0a2a8b0f8115 100644
> > > --- a/drivers/platform/chrome/cros_ec_typec.c
> > > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > > @@ -709,6 +709,7 @@ static int cros_typec_handle_sop_prime_disc(struct cros_typec_data *typec, int p
> > > .port = port_num,
> > > .partner_type = TYPEC_PARTNER_SOP_PRIME,
> > > };
> > > + uint32_t cable_plug_type;
> >
> > u32 please, this isn't userspace code :)
>
> Will fix this in the next version.
> >
> > > int ret = 0;
> > >
> > > memset(disc, 0, EC_PROTO2_MAX_RESPONSE_SIZE);
> > > @@ -722,8 +723,26 @@ static int cros_typec_handle_sop_prime_disc(struct cros_typec_data *typec, int p
> > > /* Parse the PD identity data, even if only 0s were returned. */
> > > cros_typec_parse_pd_identity(&port->c_identity, disc);
> > >
> > > - if (disc->identity_count != 0)
> > > + if (disc->identity_count != 0) {
> > > + cable_plug_type = VDO_TYPEC_CABLE_TYPE(port->c_identity.vdo[0]);
> > > + switch (cable_plug_type) {
> > > + case CABLE_ATYPE:
> > > + desc.type = USB_PLUG_TYPE_A;
> > > + break;
> > > + case CABLE_BTYPE:
> > > + desc.type = USB_PLUG_TYPE_B;
> > > + break;
> > > + case CABLE_CTYPE:
> > > + desc.type = USB_PLUG_TYPE_C;
> > > + break;
> > > + case CABLE_CAPTIVE:
> > > + desc.type = USB_PLUG_CAPTIVE;
> > > + break;
> > > + default:
> > > + desc.type = USB_PLUG_NONE;
> > > + }
> > > desc.active = PD_IDH_PTYPE(port->c_identity.id_header) == IDH_PTYPE_ACABLE;
> > > + }
> >
> > So you save it but what happens with the value?
>
> The type C connector class framework exposes it via syfs to user-space when we
> register the cable via typec_register_cable() in patch 4/6 [2].

So you added a new sysfs file and api without updating
Documentation/ABI/? That's not good :(

> I'll go ahead and CC
> you and linux-usb on the entire series for the next version.

Please do, thanks!

greg k-h

2020-11-06 09:49:25

by Prashant Malani

[permalink] [raw]
Subject: Re: [PATCH 6/6] platform/chrome: cros_ec_typec: Store cable plug type

Hi Greg,

On Fri, Nov 06, 2020 at 10:33:02AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Nov 06, 2020 at 12:59:07AM -0800, Prashant Malani wrote:
> > Hi Greg,
> >
> > Did you not receive these?
>
> Ah, I got 1, 2, and 5, and now 6. That's confusing, think about if you
> were to receive such a series, what would you think to do with it?
>

Yeah, I agree it looks confusing. Sorry about that.

> > > So you save it but what happens with the value?
> >
> > The type C connector class framework exposes it via syfs to user-space when we
> > register the cable via typec_register_cable() in patch 4/6 [2].
>
> So you added a new sysfs file and api without updating
> Documentation/ABI/? That's not good :(

This is a pre-existing API[1] and sysfs file[2] so we are using those
and not adding anything new.

[1]:
https://www.kernel.org/doc/html/latest/driver-api/usb/typec.html#c.typec_register_cable
[2]: https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-typec
(see /sys/class/<port>-cable/plug_type)


Best regards,