| Message ID | 20201116201150.2919178-1-pmalani@chromium.org |
|---|---|
| Headers | show |
| Series | chrome/platform: cros_ec_typec: Register cables, partner altmodes and plug altmodes | expand |
On Mon, Nov 16, 2020 at 12:11:58PM -0800, Prashant Malani wrote: > Modify the altmode registration (and unregistration) code so that it > can be used by both partners and plugs. > > Then, add code to register plug altmodes using the newly parameterized > function. Also set the number of alternate modes for the plug using the > associated Type C connector class function > typec_plug_set_num_altmodes(). > > Signed-off-by: Prashant Malani <pmalani@chromium.org> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > > Changes in v3: > - Re-arranged patch order and combined it with related series of > patches. > > No version v2. > > drivers/platform/chrome/cros_ec_typec.c | 50 ++++++++++++++++++++----- > 1 file changed, 40 insertions(+), 10 deletions(-) > > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c > index d2e154ae2362..65c5d0090ccd 100644 > --- a/drivers/platform/chrome/cros_ec_typec.c > +++ b/drivers/platform/chrome/cros_ec_typec.c > @@ -67,6 +67,7 @@ struct cros_typec_port { > bool sop_prime_disc_done; > struct ec_response_typec_discovery *disc_data; > struct list_head partner_mode_list; > + struct list_head plug_mode_list; > }; > > /* Platform-specific data for the Chrome OS EC Type C controller. */ > @@ -186,12 +187,15 @@ static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num, > return ret; > } > > -static void cros_typec_unregister_altmodes(struct cros_typec_data *typec, int port_num) > +static void cros_typec_unregister_altmodes(struct cros_typec_data *typec, int port_num, > + bool is_partner) > { > struct cros_typec_port *port = typec->ports[port_num]; > struct cros_typec_altmode_node *node, *tmp; > + struct list_head *head; > > - list_for_each_entry_safe(node, tmp, &port->partner_mode_list, list) { > + head = is_partner ? &port->partner_mode_list : &port->plug_mode_list; > + list_for_each_entry_safe(node, tmp, head, list) { > list_del(&node->list); > typec_unregister_altmode(node->amode); > devm_kfree(typec->dev, node); > @@ -203,7 +207,7 @@ static void cros_typec_remove_partner(struct cros_typec_data *typec, > { > struct cros_typec_port *port = typec->ports[port_num]; > > - cros_typec_unregister_altmodes(typec, port_num); > + cros_typec_unregister_altmodes(typec, port_num, true); > > port->state.alt = NULL; > port->state.mode = TYPEC_STATE_USB; > @@ -224,6 +228,8 @@ static void cros_typec_remove_cable(struct cros_typec_data *typec, > { > struct cros_typec_port *port = typec->ports[port_num]; > > + cros_typec_unregister_altmodes(typec, port_num, false); > + > typec_unregister_plug(port->plug); > port->plug = NULL; > typec_unregister_cable(port->cable); > @@ -352,6 +358,7 @@ static int cros_typec_init_ports(struct cros_typec_data *typec) > } > > INIT_LIST_HEAD(&cros_port->partner_mode_list); > + INIT_LIST_HEAD(&cros_port->plug_mode_list); > } > > return 0; > @@ -639,7 +646,11 @@ static int cros_typec_get_mux_info(struct cros_typec_data *typec, int port_num, > sizeof(req), resp, sizeof(*resp)); > } > > -static int cros_typec_register_altmodes(struct cros_typec_data *typec, int port_num) > +/* > + * Helper function to register partner/plug altmodes. > + */ > +static int cros_typec_register_altmodes(struct cros_typec_data *typec, int port_num, > + bool is_partner) > { > struct cros_typec_port *port = typec->ports[port_num]; > struct ec_response_typec_discovery *sop_disc = port->disc_data; > @@ -657,7 +668,11 @@ static int cros_typec_register_altmodes(struct cros_typec_data *typec, int port_ > desc.mode = j; > desc.vdo = sop_disc->svids[i].mode_vdo[j]; > > - amode = typec_partner_register_altmode(port->partner, &desc); > + if (is_partner) > + amode = typec_partner_register_altmode(port->partner, &desc); > + else > + amode = typec_plug_register_altmode(port->plug, &desc); > + > if (IS_ERR(amode)) { > ret = PTR_ERR(amode); > goto err_cleanup; > @@ -672,21 +687,30 @@ static int cros_typec_register_altmodes(struct cros_typec_data *typec, int port_ > } > > node->amode = amode; > - list_add_tail(&node->list, &port->partner_mode_list); > + > + if (is_partner) > + list_add_tail(&node->list, &port->partner_mode_list); > + else > + list_add_tail(&node->list, &port->plug_mode_list); > num_altmodes++; > } > } > > - ret = typec_partner_set_num_altmodes(port->partner, num_altmodes); > + if (is_partner) > + ret = typec_partner_set_num_altmodes(port->partner, num_altmodes); > + else > + ret = typec_plug_set_num_altmodes(port->plug, num_altmodes); > + > if (ret < 0) { > - dev_err(typec->dev, "Unable to set partner num_altmodes for port: %d\n", port_num); > + dev_err(typec->dev, "Unable to set %s num_altmodes for port: %d\n", > + is_partner ? "partner" : "plug", port_num); > goto err_cleanup; > } > > return 0; > > err_cleanup: > - cros_typec_unregister_altmodes(typec, port_num); > + cros_typec_unregister_altmodes(typec, port_num, is_partner); > return ret; > } > > @@ -774,6 +798,12 @@ static int cros_typec_handle_sop_prime_disc(struct cros_typec_data *typec, int p > goto sop_prime_disc_exit; > } > > + ret = cros_typec_register_altmodes(typec, port_num, false); > + if (ret < 0) { > + dev_err(typec->dev, "Failed to register plug altmodes, port: %d\n", port_num); > + goto sop_prime_disc_exit; > + } > + > return 0; > > sop_prime_disc_exit: > @@ -815,7 +845,7 @@ static int cros_typec_handle_sop_disc(struct cros_typec_data *typec, int port_nu > goto disc_exit; > } > > - ret = cros_typec_register_altmodes(typec, port_num); > + ret = cros_typec_register_altmodes(typec, port_num, true); > if (ret < 0) { > dev_err(typec->dev, "Failed to register partner altmodes, port: %d\n", port_num); > goto disc_exit; > -- > 2.29.2.299.gdc1121823c-goog thanks, -- heikki
Hi Heikki, On Tue, Nov 17, 2020 at 02:41:43PM +0200, Heikki Krogerus wrote: > On Mon, Nov 16, 2020 at 12:11:42PM -0800, Prashant Malani wrote: > > Add a field to the typec_plug struct to record the number of available > > altmodes as well as the corresponding sysfs attribute to expose this to > > userspace. > > > > This allows userspace to determine whether there are any > > remaining alternate modes left to be registered by the kernel driver. It > > can begin executing any policy state machine after all available > > alternate modes have been registered with the connector class framework. > > > > This value is set to "-1" initially, signifying that a valid number of > > alternate modes haven't been set for the plug. The sysfs file remains > > hidden as long as the attribute value is -1. > > Why couldn't we just keep it hidden for as long as the number of > alt modes is 0? If you already explained that, then I apologise, I've > forgotten. > No worries :) Succinctly, because 0 is a valid value for "number of altmodes supported". If we keep the attribute hidden for 0, then there won't be a way for userspace to determine that PD discovery is done and we don't expect any more cable plug altmodes to be registered by the kernel Type C port driver (it can determine this by comparing "number_of_altmodes" against the actual number of alt modes registered by the Type C port driver). If we keep "number_of_altmodes" hidden even for 0, the userspace cannot differentiate between "this cable doesn't support any altmodes" and "it does altmodes, but the PD stack hasn't completed PD Discovery including DiscoverIdentity yet". For reference, here is the initial patch and mini-discussion around it back in July for port-partner altmodes [1] (I've followed a similar logic here). Hope this helps the rationale a bit more. Best regards, -Prashant [1]: https://lore.kernel.org/linux-usb/20200701082230.GF856968@kuha.fi.intel.com/
On Mon, Nov 16, 2020 at 12:11:36PM -0800, Prashant Malani wrote: > This patch series adds support for the following bits of functionality, > parsing USB Type C Power Delivery information from the Chrome Embedded Controller > and using the Type C connector class: > - Register cable objects (including plug type). > - Register "number of altmodes" attribute for partners. > - Register altmodes and "number of altmodes" attribute for cable plugs. > > The functionality was earlier part of multiple series ([1], [2], [3]), but > I've combined it into 1 series and re-ordered the patches to hopefully make > it easier to peruse. I've maintained the patch Acked-by/Reviewed-by tags where > they were received. > > Patches 1/11, 2/11, 3/11 introduce the changes needed in the USB subsystem (PD VDO > header update, sysfs attribute additions) and hence the first three patches > can go through Greg's tree. I've taken the first 2 patches in my usb tree now, waiting for Heikki's response on patch 3 before I touch that. thanks, greg k-h
On Tue, Nov 17, 2020 at 09:40:16AM -0800, Prashant Malani wrote: > Hi Heikki, > > On Tue, Nov 17, 2020 at 02:41:43PM +0200, Heikki Krogerus wrote: > > On Mon, Nov 16, 2020 at 12:11:42PM -0800, Prashant Malani wrote: > > > Add a field to the typec_plug struct to record the number of available > > > altmodes as well as the corresponding sysfs attribute to expose this to > > > userspace. > > > > > > This allows userspace to determine whether there are any > > > remaining alternate modes left to be registered by the kernel driver. It > > > can begin executing any policy state machine after all available > > > alternate modes have been registered with the connector class framework. > > > > > > This value is set to "-1" initially, signifying that a valid number of > > > alternate modes haven't been set for the plug. The sysfs file remains > > > hidden as long as the attribute value is -1. > > > > Why couldn't we just keep it hidden for as long as the number of > > alt modes is 0? If you already explained that, then I apologise, I've > > forgotten. > > > > No worries :) > > Succinctly, because 0 is a valid value for "number of altmodes > supported". > > If we keep the attribute hidden for 0, then there won't > be a way for userspace to determine that PD discovery is done and we > don't expect any more cable plug altmodes to be registered by the kernel > Type C port driver (it can determine this by comparing > "number_of_altmodes" against the actual number of alt modes registered > by the Type C port driver). > > If we keep "number_of_altmodes" hidden even for 0, the userspace cannot > differentiate between "this cable doesn't support any altmodes" and > "it does altmodes, but the PD stack hasn't completed PD Discovery > including DiscoverIdentity yet". > > For reference, here is the initial patch and mini-discussion around it > back in July for port-partner altmodes [1] (I've followed a similar > logic here). > > Hope this helps the rationale a bit more. Got it. Thanks for the explanation (again :-). Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Best regards, > > -Prashant > > [1]: > https://lore.kernel.org/linux-usb/20200701082230.GF856968@kuha.fi.intel.com/ thanks, -- heikki
On Wed, Nov 18, 2020 at 12:59:59PM +0100, Greg KH wrote: > On Mon, Nov 16, 2020 at 12:11:36PM -0800, Prashant Malani wrote: > > This patch series adds support for the following bits of functionality, > > parsing USB Type C Power Delivery information from the Chrome Embedded Controller > > and using the Type C connector class: > > - Register cable objects (including plug type). > > - Register "number of altmodes" attribute for partners. > > - Register altmodes and "number of altmodes" attribute for cable plugs. > > > > The functionality was earlier part of multiple series ([1], [2], [3]), but > > I've combined it into 1 series and re-ordered the patches to hopefully make > > it easier to peruse. I've maintained the patch Acked-by/Reviewed-by tags where > > they were received. > > > > Patches 1/11, 2/11, 3/11 introduce the changes needed in the USB subsystem (PD VDO > > header update, sysfs attribute additions) and hence the first three patches > > can go through Greg's tree. > > I've taken the first 2 patches in my usb tree now, waiting for Heikki's > response on patch 3 before I touch that. Ok, now taken patch 3 too. I can take the rest in my usb-next tree if the platform people don't object as well, but would need an ack for that. thanks, greg k-h
Hi Greg, On Wed, Nov 18, 2020 at 01:16:49PM +0100, Greg KH wrote: > On Wed, Nov 18, 2020 at 12:59:59PM +0100, Greg KH wrote: > > On Mon, Nov 16, 2020 at 12:11:36PM -0800, Prashant Malani wrote: > > > This patch series adds support for the following bits of functionality, > > > parsing USB Type C Power Delivery information from the Chrome Embedded Controller > > > and using the Type C connector class: > > > - Register cable objects (including plug type). > > > - Register "number of altmodes" attribute for partners. > > > - Register altmodes and "number of altmodes" attribute for cable plugs. > > > > > > The functionality was earlier part of multiple series ([1], [2], [3]), but > > > I've combined it into 1 series and re-ordered the patches to hopefully make > > > it easier to peruse. I've maintained the patch Acked-by/Reviewed-by tags where > > > they were received. > > > > > > Patches 1/11, 2/11, 3/11 introduce the changes needed in the USB subsystem (PD VDO > > > header update, sysfs attribute additions) and hence the first three patches > > > can go through Greg's tree. > > > > I've taken the first 2 patches in my usb tree now, waiting for Heikki's > > response on patch 3 before I touch that. > > Ok, now taken patch 3 too. > Thanks! > I can take the rest in my usb-next tree if the platform people don't > object as well, but would need an ack for that. Will defer to Enric on this, but Patch 4/11 and onwards have a dependency on some patches which are already in the chrome-platform tree [1], so they may have to go through there. [1] https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git/log/?h=for-next Best regards, -Prashant
Hi Prashant, On Mon, 16 Nov 2020 12:11:36 -0800, Prashant Malani wrote: > This patch series adds support for the following bits of functionality, > parsing USB Type C Power Delivery information from the Chrome Embedded Controller > and using the Type C connector class: > - Register cable objects (including plug type). > - Register "number of altmodes" attribute for partners. > - Register altmodes and "number of altmodes" attribute for cable plugs. > > [...] Applied 4 through 11 of this series, staged for chrome-platform-5.12, thanks! [04/11] platform/chrome: cros_ec_typec: Make disc_done flag partner-only (no commit info) [05/11] platform/chrome: cros_ec_typec: Factor out PD identity parsing (no commit info) [06/11] platform/chrome: cros_ec_typec: Rename discovery struct (no commit info) [07/11] platform/chrome: cros_ec_typec: Register cable (no commit info) [08/11] platform/chrome: cros_ec_typec: Store cable plug type (no commit info) [09/11] platform/chrome: cros_ec_typec: Set partner num_altmodes (no commit info) [10/11] platform/chrome: cros_ec_typec: Register SOP' cable plug (no commit info) [11/11] platform/chrome: cros_ec_typec: Register plug altmodes (no commit info) Best regards, -- Benson Leung Staff Software Engineer Chrome OS Kernel Google Inc. bleung@google.com Chromium OS Project bleung@chromium.org
This patch series adds support for the following bits of functionality, parsing USB Type C Power Delivery information from the Chrome Embedded Controller and using the Type C connector class: - Register cable objects (including plug type). - Register "number of altmodes" attribute for partners. - Register altmodes and "number of altmodes" attribute for cable plugs. The functionality was earlier part of multiple series ([1], [2], [3]), but I've combined it into 1 series and re-ordered the patches to hopefully make it easier to peruse. I've maintained the patch Acked-by/Reviewed-by tags where they were received. Patches 1/11, 2/11, 3/11 introduce the changes needed in the USB subsystem (PD VDO header update, sysfs attribute additions) and hence the first three patches can go through Greg's tree. The others are users of the newly introduced USB changes and can go through the chrome-platform tree. Of course, the above is only a suggestion, so I'd be happy to follow another means of integrating the patches if available. The series is based on the following git branch and commit Branch: chrome-platform for-next [4] Commit: de0f49487db3 ("platform/chrome: cros_ec_typec: Register partner altmodes") For reference, the patches in this series which are yet to be reviewed are Patch 3/11, Patch 10/11 and Patch 11/11. Version history: - No v2 or v1, as mentioned earlier these patches were uploaded as separate series [1], [2] and [3] but have now been coalesced. [1]: https://lore.kernel.org/lkml/20201106184104.939284-1-pmalani@chromium.org/ [2]: https://lore.kernel.org/lkml/20201110061535.2163599-1-pmalani@chromium.org/ [3]: https://lore.kernel.org/linux-usb/20201112012329.1364975-1-pmalani@chromium.org/ [4]: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git/log/?h=for-next Prashant Malani (11): usb: pd: Add captive Type C cable type usb: typec: Add number of altmodes partner attr usb: typec: Add plug num_altmodes sysfs attr 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 platform/chrome: cros_ec_typec: Store cable plug type platform/chrome: cros_ec_typec: Set partner num_altmodes platform/chrome: cros_ec_typec: Register SOP' cable plug platform/chrome: cros_ec_typec: Register plug altmodes Documentation/ABI/testing/sysfs-class-typec | 17 ++ drivers/platform/chrome/cros_ec_typec.c | 219 ++++++++++++++++---- drivers/usb/typec/class.c | 139 ++++++++++++- include/linux/usb/pd_vdo.h | 4 +- include/linux/usb/typec.h | 2 + 5 files changed, 343 insertions(+), 38 deletions(-)