[PATCH v3 0/3] platform/chrome: typec: Add port partner registration

[PATCH v3 0/3] platform/chrome: typec: Add port partner registration

[Prashant Malani]

The following series adds port-partner registration when
connects/disconnects are detected. In order to do that, we also register
a listener for the cros-usbpd-notifier, which will inform the driver of
EC PD events. While we are here, separate out the Type C port data
objects into a separate struct.

v2: https://lkml.org/lkml/2020/4/9/861
v1: https://lkml.org/lkml/2020/4/6/1155

Changes in v3:
- Fixed trivial Kconfig dependency ordering error.

Prashant Malani (3):
  platform/chrome: typec: Use notifier for updates
  platform/chrome: typec: Add struct for port data
  platform/chrome: typec: Register port partner

 drivers/platform/chrome/Kconfig         |   1 +
 drivers/platform/chrome/cros_ec_typec.c | 119 ++++++++++++++++++++----
 2 files changed, 103 insertions(+), 17 deletions(-)

-- 
2.26.0.110.g2183baf09c-goog

[PATCH v3 1/3] platform/chrome: typec: Use notifier for updates

[Prashant Malani]

Register a listener for the cros-usbpd-notifier, and update port state
when a notification comes in.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v3:
- Fixed ordering issue in Kconfig dependency.

Changes in v2:
- Added Kconfig dependency to CROS_USBPD_NOTIFY.
- Fixed spacing error.
- Removed superfluous devm_warn() call.

 drivers/platform/chrome/Kconfig         |  1 +
 drivers/platform/chrome/cros_ec_typec.c | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 03ea5129ed0c3..a484ab2c91ff0 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -217,6 +217,7 @@ config CROS_EC_SYSFS
 config CROS_EC_TYPEC
 	tristate "ChromeOS EC Type-C Connector Control"
 	depends on MFD_CROS_EC_DEV && TYPEC
+	depends on CROS_USBPD_NOTIFY
 	default MFD_CROS_EC_DEV
 	help
 	  If you say Y here, you get support for accessing Type C connector
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 874269c070739..d444dd7422a2a 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -11,6 +11,7 @@
 #include <linux/of.h>
 #include <linux/platform_data/cros_ec_commands.h>
 #include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_data/cros_usbpd_notify.h>
 #include <linux/platform_device.h>
 #include <linux/usb/typec.h>
 
@@ -26,6 +27,7 @@ struct cros_typec_data {
 	struct typec_port *ports[EC_USB_PD_MAX_PORTS];
 	/* Initial capabilities for each port. */
 	struct typec_capability *caps[EC_USB_PD_MAX_PORTS];
+	struct notifier_block nb;
 };
 
 static int cros_typec_parse_port_props(struct typec_capability *cap,
@@ -272,6 +274,22 @@ static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
 	return 0;
 }
 
+static int cros_ec_typec_event(struct notifier_block *nb,
+			       unsigned long host_event, void *_notify)
+{
+	struct cros_typec_data *typec = container_of(nb, struct cros_typec_data,
+						     nb);
+	int ret, i;
+
+	for (i = 0; i < typec->num_ports; i++) {
+		ret = cros_typec_port_update(typec, i);
+		if (ret < 0)
+			dev_warn(typec->dev, "Update failed for port: %d\n", i);
+	}
+
+	return NOTIFY_OK;
+}
+
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id cros_typec_acpi_id[] = {
 	{ "GOOG0014", 0 },
@@ -332,6 +350,11 @@ static int cros_typec_probe(struct platform_device *pdev)
 			goto unregister_ports;
 	}
 
+	typec->nb.notifier_call = cros_ec_typec_event;
+	ret = cros_usbpd_register_notify(&typec->nb);
+	if (ret < 0)
+		goto unregister_ports;
+
 	return 0;
 
 unregister_ports:
-- 
2.26.0.110.g2183baf09c-goog

[PATCH v3 2/3] platform/chrome: typec: Add struct for port data

[Prashant Malani]

Add a separate struct for storing port data, including Type C connector
class struct pointers and caps.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v3:
- No changes.

Changes in v2:
- Removed unrequired devm_kfree() call.

 drivers/platform/chrome/cros_ec_typec.c | 48 ++++++++++++++++---------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index d444dd7422a2a..56ded09a60ffb 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -17,6 +17,13 @@
 
 #define DRV_NAME "cros-ec-typec"
 
+/* Per port data. */
+struct cros_typec_port {
+	struct typec_port *port;
+	/* Initial capabilities for the port. */
+	struct typec_capability caps;
+};
+
 /* Platform-specific data for the Chrome OS EC Type C controller. */
 struct cros_typec_data {
 	struct device *dev;
@@ -24,9 +31,7 @@ struct cros_typec_data {
 	int num_ports;
 	unsigned int cmd_ver;
 	/* Array of ports, indexed by port number. */
-	struct typec_port *ports[EC_USB_PD_MAX_PORTS];
-	/* Initial capabilities for each port. */
-	struct typec_capability *caps[EC_USB_PD_MAX_PORTS];
+	struct cros_typec_port *ports[EC_USB_PD_MAX_PORTS];
 	struct notifier_block nb;
 };
 
@@ -76,14 +81,25 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
 	return 0;
 }
 
+static void cros_unregister_ports(struct cros_typec_data *typec)
+{
+	int i;
+
+	for (i = 0; i < typec->num_ports; i++) {
+		if (!typec->ports[i])
+			continue;
+		typec_unregister_port(typec->ports[i]->port);
+	}
+}
+
 static int cros_typec_init_ports(struct cros_typec_data *typec)
 {
 	struct device *dev = typec->dev;
 	struct typec_capability *cap;
 	struct fwnode_handle *fwnode;
+	struct cros_typec_port *cros_port;
 	const char *port_prop;
 	int ret;
-	int i;
 	int nports;
 	u32 port_num = 0;
 
@@ -115,22 +131,23 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
 
 		dev_dbg(dev, "Registering port %d\n", port_num);
 
-		cap = devm_kzalloc(dev, sizeof(*cap), GFP_KERNEL);
-		if (!cap) {
+		cros_port = devm_kzalloc(dev, sizeof(*cros_port), GFP_KERNEL);
+		if (!cros_port) {
 			ret = -ENOMEM;
 			goto unregister_ports;
 		}
 
-		typec->caps[port_num] = cap;
+		typec->ports[port_num] = cros_port;
+		cap = &cros_port->caps;
 
 		ret = cros_typec_parse_port_props(cap, fwnode, dev);
 		if (ret < 0)
 			goto unregister_ports;
 
-		typec->ports[port_num] = typec_register_port(dev, cap);
-		if (IS_ERR(typec->ports[port_num])) {
+		cros_port->port = typec_register_port(dev, cap);
+		if (IS_ERR(cros_port->port)) {
 			dev_err(dev, "Failed to register port %d\n", port_num);
-			ret = PTR_ERR(typec->ports[port_num]);
+			ret = PTR_ERR(cros_port->port);
 			goto unregister_ports;
 		}
 	}
@@ -138,8 +155,7 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
 	return 0;
 
 unregister_ports:
-	for (i = 0; i < typec->num_ports; i++)
-		typec_unregister_port(typec->ports[i]);
+	cros_unregister_ports(typec);
 	return ret;
 }
 
@@ -177,7 +193,7 @@ static int cros_typec_ec_command(struct cros_typec_data *typec,
 static void cros_typec_set_port_params_v0(struct cros_typec_data *typec,
 		int port_num, struct ec_response_usb_pd_control *resp)
 {
-	struct typec_port *port = typec->ports[port_num];
+	struct typec_port *port = typec->ports[port_num]->port;
 	enum typec_orientation polarity;
 
 	if (!resp->enabled)
@@ -194,7 +210,7 @@ static void cros_typec_set_port_params_v0(struct cros_typec_data *typec,
 static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
 		int port_num, struct ec_response_usb_pd_control_v1 *resp)
 {
-	struct typec_port *port = typec->ports[port_num];
+	struct typec_port *port = typec->ports[port_num]->port;
 	enum typec_orientation polarity;
 
 	if (!(resp->enabled & PD_CTRL_RESP_ENABLED_CONNECTED))
@@ -358,9 +374,7 @@ static int cros_typec_probe(struct platform_device *pdev)
 	return 0;
 
 unregister_ports:
-	for (i = 0; i < typec->num_ports; i++)
-		if (typec->ports[i])
-			typec_unregister_port(typec->ports[i]);
+	cros_unregister_ports(typec);
 	return ret;
 }
 
-- 
2.26.0.110.g2183baf09c-goog

[PATCH v3 3/3] platform/chrome: typec: Register port partner

[Prashant Malani]

Register (and unregister) the port partner when a connect (and
disconnect) is detected.

Co-developed-by: Jon Flatley <jflat@chromium.org>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v3:
- No changes.

Changes in v2:
- Fixed error pointer return value.

 drivers/platform/chrome/cros_ec_typec.c | 48 +++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 56ded09a60ffb..304e0b20f279b 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -22,6 +22,9 @@ struct cros_typec_port {
 	struct typec_port *port;
 	/* Initial capabilities for the port. */
 	struct typec_capability caps;
+	struct typec_partner *partner;
+	/* Port partner PD identity info. */
+	struct usb_pd_identity p_identity;
 };
 
 /* Platform-specific data for the Chrome OS EC Type C controller. */
@@ -190,6 +193,30 @@ static int cros_typec_ec_command(struct cros_typec_data *typec,
 	return ret;
 }
 
+static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num,
+				  bool pd_en)
+{
+	struct cros_typec_port *port = typec->ports[port_num];
+	struct typec_partner_desc p_desc = {
+		.usb_pd = pd_en,
+	};
+	int ret = 0;
+
+	/*
+	 * Fill an initial PD identity, which will then be updated with info
+	 * from the EC.
+	 */
+	p_desc.identity = &port->p_identity;
+
+	port->partner = typec_register_partner(port->port, &p_desc);
+	if (IS_ERR_OR_NULL(port->partner)) {
+		ret = PTR_ERR(port->partner);
+		port->partner = NULL;
+	}
+
+	return ret;
+}
+
 static void cros_typec_set_port_params_v0(struct cros_typec_data *typec,
 		int port_num, struct ec_response_usb_pd_control *resp)
 {
@@ -212,6 +239,8 @@ static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
 {
 	struct typec_port *port = typec->ports[port_num]->port;
 	enum typec_orientation polarity;
+	bool pd_en;
+	int ret;
 
 	if (!(resp->enabled & PD_CTRL_RESP_ENABLED_CONNECTED))
 		polarity = TYPEC_ORIENTATION_NONE;
@@ -226,6 +255,25 @@ static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
 			TYPEC_SOURCE : TYPEC_SINK);
 	typec_set_vconn_role(port, resp->role & PD_CTRL_RESP_ROLE_VCONN ?
 			TYPEC_SOURCE : TYPEC_SINK);
+
+	/* Register/remove partners when a connect/disconnect occurs. */
+	if (resp->enabled & PD_CTRL_RESP_ENABLED_CONNECTED) {
+		if (typec->ports[port_num]->partner)
+			return;
+
+		pd_en = resp->enabled & PD_CTRL_RESP_ENABLED_PD_CAPABLE;
+		ret = cros_typec_add_partner(typec, port_num, pd_en);
+		if (!ret)
+			dev_warn(typec->dev,
+				 "Failed to register partner on port: %d\n",
+				 port_num);
+	} else {
+		if (!typec->ports[port_num]->partner)
+			return;
+
+		typec_unregister_partner(typec->ports[port_num]->partner);
+		typec->ports[port_num]->partner = NULL;
+	}
 }
 
 static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
-- 
2.26.0.110.g2183baf09c-goog

Re: [PATCH v3 0/3] platform/chrome: typec: Add port partner registration

[Enric Balletbo i Serra]

Hi Prashant,

Thanks for your patches.

On 10/4/20 2:23, Prashant Malani wrote:
> The following series adds port-partner registration when
> connects/disconnects are detected. In order to do that, we also register
> a listener for the cros-usbpd-notifier, which will inform the driver of
> EC PD events. While we are here, separate out the Type C port data
> objects into a separate struct.
> 
> v2: https://lkml.org/lkml/2020/4/9/861
> v1: https://lkml.org/lkml/2020/4/6/1155
> 
> Changes in v3:
> - Fixed trivial Kconfig dependency ordering error.
> 
> Prashant Malani (3):
>   platform/chrome: typec: Use notifier for updates
>   platform/chrome: typec: Add struct for port data
>   platform/chrome: typec: Register port partner
> 
>  drivers/platform/chrome/Kconfig         |   1 +
>  drivers/platform/chrome/cros_ec_typec.c | 119 ++++++++++++++++++++----
>  2 files changed, 103 insertions(+), 17 deletions(-)
> 

Would be nice have the Heikki review on those.

Thanks,
 Enric

Re: [PATCH v3 1/3] platform/chrome: typec: Use notifier for updates

[Heikki Krogerus]

On Thu, Apr 09, 2020 at 05:23:15PM -0700, Prashant Malani wrote:
> Register a listener for the cros-usbpd-notifier, and update port state
> when a notification comes in.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> 
> Changes in v3:
> - Fixed ordering issue in Kconfig dependency.
> 
> Changes in v2:
> - Added Kconfig dependency to CROS_USBPD_NOTIFY.
> - Fixed spacing error.
> - Removed superfluous devm_warn() call.
> 
>  drivers/platform/chrome/Kconfig         |  1 +
>  drivers/platform/chrome/cros_ec_typec.c | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 03ea5129ed0c3..a484ab2c91ff0 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -217,6 +217,7 @@ config CROS_EC_SYSFS
>  config CROS_EC_TYPEC
>  	tristate "ChromeOS EC Type-C Connector Control"
>  	depends on MFD_CROS_EC_DEV && TYPEC
> +	depends on CROS_USBPD_NOTIFY
>  	default MFD_CROS_EC_DEV
>  	help
>  	  If you say Y here, you get support for accessing Type C connector
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 874269c070739..d444dd7422a2a 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -11,6 +11,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_data/cros_ec_commands.h>
>  #include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_data/cros_usbpd_notify.h>
>  #include <linux/platform_device.h>
>  #include <linux/usb/typec.h>
>  
> @@ -26,6 +27,7 @@ struct cros_typec_data {
>  	struct typec_port *ports[EC_USB_PD_MAX_PORTS];
>  	/* Initial capabilities for each port. */
>  	struct typec_capability *caps[EC_USB_PD_MAX_PORTS];
> +	struct notifier_block nb;
>  };
>  
>  static int cros_typec_parse_port_props(struct typec_capability *cap,
> @@ -272,6 +274,22 @@ static int cros_typec_get_cmd_version(struct cros_typec_data *typec)
>  	return 0;
>  }
>  
> +static int cros_ec_typec_event(struct notifier_block *nb,
> +			       unsigned long host_event, void *_notify)
> +{
> +	struct cros_typec_data *typec = container_of(nb, struct cros_typec_data,
> +						     nb);
> +	int ret, i;
> +
> +	for (i = 0; i < typec->num_ports; i++) {
> +		ret = cros_typec_port_update(typec, i);
> +		if (ret < 0)
> +			dev_warn(typec->dev, "Update failed for port: %d\n", i);
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
>  #ifdef CONFIG_ACPI
>  static const struct acpi_device_id cros_typec_acpi_id[] = {
>  	{ "GOOG0014", 0 },
> @@ -332,6 +350,11 @@ static int cros_typec_probe(struct platform_device *pdev)
>  			goto unregister_ports;
>  	}
>  
> +	typec->nb.notifier_call = cros_ec_typec_event;
> +	ret = cros_usbpd_register_notify(&typec->nb);
> +	if (ret < 0)
> +		goto unregister_ports;
> +
>  	return 0;
>  
>  unregister_ports:
> -- 
> 2.26.0.110.g2183baf09c-goog

thanks,

-- 
heikki

Re: [PATCH v3 2/3] platform/chrome: typec: Add struct for port data

[Heikki Krogerus]

On Thu, Apr 09, 2020 at 05:23:16PM -0700, Prashant Malani wrote:
> Add a separate struct for storing port data, including Type C connector
> class struct pointers and caps.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> 
> Changes in v3:
> - No changes.
> 
> Changes in v2:
> - Removed unrequired devm_kfree() call.
> 
>  drivers/platform/chrome/cros_ec_typec.c | 48 ++++++++++++++++---------
>  1 file changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index d444dd7422a2a..56ded09a60ffb 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -17,6 +17,13 @@
>  
>  #define DRV_NAME "cros-ec-typec"
>  
> +/* Per port data. */
> +struct cros_typec_port {
> +	struct typec_port *port;
> +	/* Initial capabilities for the port. */
> +	struct typec_capability caps;
> +};
> +
>  /* Platform-specific data for the Chrome OS EC Type C controller. */
>  struct cros_typec_data {
>  	struct device *dev;
> @@ -24,9 +31,7 @@ struct cros_typec_data {
>  	int num_ports;
>  	unsigned int cmd_ver;
>  	/* Array of ports, indexed by port number. */
> -	struct typec_port *ports[EC_USB_PD_MAX_PORTS];
> -	/* Initial capabilities for each port. */
> -	struct typec_capability *caps[EC_USB_PD_MAX_PORTS];
> +	struct cros_typec_port *ports[EC_USB_PD_MAX_PORTS];
>  	struct notifier_block nb;
>  };
>  
> @@ -76,14 +81,25 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
>  	return 0;
>  }
>  
> +static void cros_unregister_ports(struct cros_typec_data *typec)
> +{
> +	int i;
> +
> +	for (i = 0; i < typec->num_ports; i++) {
> +		if (!typec->ports[i])
> +			continue;
> +		typec_unregister_port(typec->ports[i]->port);
> +	}
> +}
> +
>  static int cros_typec_init_ports(struct cros_typec_data *typec)
>  {
>  	struct device *dev = typec->dev;
>  	struct typec_capability *cap;
>  	struct fwnode_handle *fwnode;
> +	struct cros_typec_port *cros_port;
>  	const char *port_prop;
>  	int ret;
> -	int i;
>  	int nports;
>  	u32 port_num = 0;
>  
> @@ -115,22 +131,23 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
>  
>  		dev_dbg(dev, "Registering port %d\n", port_num);
>  
> -		cap = devm_kzalloc(dev, sizeof(*cap), GFP_KERNEL);
> -		if (!cap) {
> +		cros_port = devm_kzalloc(dev, sizeof(*cros_port), GFP_KERNEL);
> +		if (!cros_port) {
>  			ret = -ENOMEM;
>  			goto unregister_ports;
>  		}
>  
> -		typec->caps[port_num] = cap;
> +		typec->ports[port_num] = cros_port;
> +		cap = &cros_port->caps;
>  
>  		ret = cros_typec_parse_port_props(cap, fwnode, dev);
>  		if (ret < 0)
>  			goto unregister_ports;
>  
> -		typec->ports[port_num] = typec_register_port(dev, cap);
> -		if (IS_ERR(typec->ports[port_num])) {
> +		cros_port->port = typec_register_port(dev, cap);
> +		if (IS_ERR(cros_port->port)) {
>  			dev_err(dev, "Failed to register port %d\n", port_num);
> -			ret = PTR_ERR(typec->ports[port_num]);
> +			ret = PTR_ERR(cros_port->port);
>  			goto unregister_ports;
>  		}
>  	}
> @@ -138,8 +155,7 @@ static int cros_typec_init_ports(struct cros_typec_data *typec)
>  	return 0;
>  
>  unregister_ports:
> -	for (i = 0; i < typec->num_ports; i++)
> -		typec_unregister_port(typec->ports[i]);
> +	cros_unregister_ports(typec);
>  	return ret;
>  }
>  
> @@ -177,7 +193,7 @@ static int cros_typec_ec_command(struct cros_typec_data *typec,
>  static void cros_typec_set_port_params_v0(struct cros_typec_data *typec,
>  		int port_num, struct ec_response_usb_pd_control *resp)
>  {
> -	struct typec_port *port = typec->ports[port_num];
> +	struct typec_port *port = typec->ports[port_num]->port;
>  	enum typec_orientation polarity;
>  
>  	if (!resp->enabled)
> @@ -194,7 +210,7 @@ static void cros_typec_set_port_params_v0(struct cros_typec_data *typec,
>  static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
>  		int port_num, struct ec_response_usb_pd_control_v1 *resp)
>  {
> -	struct typec_port *port = typec->ports[port_num];
> +	struct typec_port *port = typec->ports[port_num]->port;
>  	enum typec_orientation polarity;
>  
>  	if (!(resp->enabled & PD_CTRL_RESP_ENABLED_CONNECTED))
> @@ -358,9 +374,7 @@ static int cros_typec_probe(struct platform_device *pdev)
>  	return 0;
>  
>  unregister_ports:
> -	for (i = 0; i < typec->num_ports; i++)
> -		if (typec->ports[i])
> -			typec_unregister_port(typec->ports[i]);
> +	cros_unregister_ports(typec);
>  	return ret;
>  }
>  
> -- 
> 2.26.0.110.g2183baf09c-goog

thanks,

-- 
heikki

Re: [PATCH v3 3/3] platform/chrome: typec: Register port partner

[Heikki Krogerus]

On Thu, Apr 09, 2020 at 05:23:17PM -0700, Prashant Malani wrote:
> Register (and unregister) the port partner when a connect (and
> disconnect) is detected.
> 
> Co-developed-by: Jon Flatley <jflat@chromium.org>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> 
> Changes in v3:
> - No changes.
> 
> Changes in v2:
> - Fixed error pointer return value.
> 
>  drivers/platform/chrome/cros_ec_typec.c | 48 +++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 56ded09a60ffb..304e0b20f279b 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -22,6 +22,9 @@ struct cros_typec_port {
>  	struct typec_port *port;
>  	/* Initial capabilities for the port. */
>  	struct typec_capability caps;
> +	struct typec_partner *partner;
> +	/* Port partner PD identity info. */
> +	struct usb_pd_identity p_identity;
>  };
>  
>  /* Platform-specific data for the Chrome OS EC Type C controller. */
> @@ -190,6 +193,30 @@ static int cros_typec_ec_command(struct cros_typec_data *typec,
>  	return ret;
>  }
>  
> +static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num,
> +				  bool pd_en)
> +{
> +	struct cros_typec_port *port = typec->ports[port_num];
> +	struct typec_partner_desc p_desc = {
> +		.usb_pd = pd_en,
> +	};
> +	int ret = 0;
> +
> +	/*
> +	 * Fill an initial PD identity, which will then be updated with info
> +	 * from the EC.
> +	 */
> +	p_desc.identity = &port->p_identity;
> +
> +	port->partner = typec_register_partner(port->port, &p_desc);
> +	if (IS_ERR_OR_NULL(port->partner)) {
> +		ret = PTR_ERR(port->partner);
> +		port->partner = NULL;
> +	}
> +
> +	return ret;
> +}
> +
>  static void cros_typec_set_port_params_v0(struct cros_typec_data *typec,
>  		int port_num, struct ec_response_usb_pd_control *resp)
>  {
> @@ -212,6 +239,8 @@ static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
>  {
>  	struct typec_port *port = typec->ports[port_num]->port;
>  	enum typec_orientation polarity;
> +	bool pd_en;
> +	int ret;
>  
>  	if (!(resp->enabled & PD_CTRL_RESP_ENABLED_CONNECTED))
>  		polarity = TYPEC_ORIENTATION_NONE;
> @@ -226,6 +255,25 @@ static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
>  			TYPEC_SOURCE : TYPEC_SINK);
>  	typec_set_vconn_role(port, resp->role & PD_CTRL_RESP_ROLE_VCONN ?
>  			TYPEC_SOURCE : TYPEC_SINK);
> +
> +	/* Register/remove partners when a connect/disconnect occurs. */
> +	if (resp->enabled & PD_CTRL_RESP_ENABLED_CONNECTED) {
> +		if (typec->ports[port_num]->partner)
> +			return;
> +
> +		pd_en = resp->enabled & PD_CTRL_RESP_ENABLED_PD_CAPABLE;
> +		ret = cros_typec_add_partner(typec, port_num, pd_en);
> +		if (!ret)
> +			dev_warn(typec->dev,
> +				 "Failed to register partner on port: %d\n",
> +				 port_num);
> +	} else {
> +		if (!typec->ports[port_num]->partner)
> +			return;
> +
> +		typec_unregister_partner(typec->ports[port_num]->partner);
> +		typec->ports[port_num]->partner = NULL;
> +	}
>  }
>  
>  static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
> -- 
> 2.26.0.110.g2183baf09c-goog

thanks,

-- 
heikki

Re: [PATCH v3 3/3] platform/chrome: typec: Register port partner

[Enric Balletbo i Serra]

Hi Prashant,

Thank you for your patch.

On 10/4/20 2:23, Prashant Malani wrote:
> Register (and unregister) the port partner when a connect (and
> disconnect) is detected.
> 
> Co-developed-by: Jon Flatley <jflat@chromium.org>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes in v3:
> - No changes.
> 
> Changes in v2:
> - Fixed error pointer return value.
> 
>  drivers/platform/chrome/cros_ec_typec.c | 48 +++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 56ded09a60ffb..304e0b20f279b 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -22,6 +22,9 @@ struct cros_typec_port {
>  	struct typec_port *port;
>  	/* Initial capabilities for the port. */
>  	struct typec_capability caps;
> +	struct typec_partner *partner;
> +	/* Port partner PD identity info. */
> +	struct usb_pd_identity p_identity;
>  };
>  
>  /* Platform-specific data for the Chrome OS EC Type C controller. */
> @@ -190,6 +193,30 @@ static int cros_typec_ec_command(struct cros_typec_data *typec,
>  	return ret;
>  }
>  
> +static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num,
> +				  bool pd_en)
> +{
> +	struct cros_typec_port *port = typec->ports[port_num];
> +	struct typec_partner_desc p_desc = {
> +		.usb_pd = pd_en,
> +	};
> +	int ret = 0;
> +
> +	/*
> +	 * Fill an initial PD identity, which will then be updated with info
> +	 * from the EC.
> +	 */
> +	p_desc.identity = &port->p_identity;
> +
> +	port->partner = typec_register_partner(port->port, &p_desc);
> +	if (IS_ERR_OR_NULL(port->partner)) {
> +		ret = PTR_ERR(port->partner);

If you're checking for IS_ERR_OR_NULL that means that port->partner can be NULL,
so PTR_ERR(NULL) is 0 returning something that you don't really want.

But looking at the typec_register_partner, NULL is not an option as returns a
handle to the partner on success or ERR_PTR on failure. So, the code should just do:

if (IS_ERR(port->partner))
    return PTR_ERR(port->partner);

And AFAICS you don't need to set port->partner to NULL.

Can you double check this?

Thanks,
 Enric

> +		port->partner = NULL;
> +	}
> +
> +	return ret;
> +}
> +
>  static void cros_typec_set_port_params_v0(struct cros_typec_data *typec,
>  		int port_num, struct ec_response_usb_pd_control *resp)
>  {
> @@ -212,6 +239,8 @@ static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
>  {
>  	struct typec_port *port = typec->ports[port_num]->port;
>  	enum typec_orientation polarity;
> +	bool pd_en;
> +	int ret;
>  
>  	if (!(resp->enabled & PD_CTRL_RESP_ENABLED_CONNECTED))
>  		polarity = TYPEC_ORIENTATION_NONE;
> @@ -226,6 +255,25 @@ static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
>  			TYPEC_SOURCE : TYPEC_SINK);
>  	typec_set_vconn_role(port, resp->role & PD_CTRL_RESP_ROLE_VCONN ?
>  			TYPEC_SOURCE : TYPEC_SINK);
> +
> +	/* Register/remove partners when a connect/disconnect occurs. */
> +	if (resp->enabled & PD_CTRL_RESP_ENABLED_CONNECTED) {
> +		if (typec->ports[port_num]->partner)
> +			return;
> +
> +		pd_en = resp->enabled & PD_CTRL_RESP_ENABLED_PD_CAPABLE;
> +		ret = cros_typec_add_partner(typec, port_num, pd_en);
> +		if (!ret)
> +			dev_warn(typec->dev,
> +				 "Failed to register partner on port: %d\n",
> +				 port_num);
> +	} else {
> +		if (!typec->ports[port_num]->partner)
> +			return;
> +
> +		typec_unregister_partner(typec->ports[port_num]->partner);
> +		typec->ports[port_num]->partner = NULL;
> +	}
>  }
>  
>  static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
> 

Re: [PATCH v3 3/3] platform/chrome: typec: Register port partner

[Prashant Malani]

Thanks for the review Enric,

On Tue, Apr 14, 2020 at 7:08 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Prashant,
>
> Thank you for your patch.
>
> On 10/4/20 2:23, Prashant Malani wrote:
> > Register (and unregister) the port partner when a connect (and
> > disconnect) is detected.
> >
> > Co-developed-by: Jon Flatley <jflat@chromium.org>
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes in v3:
> > - No changes.
> >
> > Changes in v2:
> > - Fixed error pointer return value.
> >
> >  drivers/platform/chrome/cros_ec_typec.c | 48 +++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index 56ded09a60ffb..304e0b20f279b 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -22,6 +22,9 @@ struct cros_typec_port {
> >       struct typec_port *port;
> >       /* Initial capabilities for the port. */
> >       struct typec_capability caps;
> > +     struct typec_partner *partner;
> > +     /* Port partner PD identity info. */
> > +     struct usb_pd_identity p_identity;
> >  };
> >
> >  /* Platform-specific data for the Chrome OS EC Type C controller. */
> > @@ -190,6 +193,30 @@ static int cros_typec_ec_command(struct cros_typec_data *typec,
> >       return ret;
> >  }
> >
> > +static int cros_typec_add_partner(struct cros_typec_data *typec, int port_num,
> > +                               bool pd_en)
> > +{
> > +     struct cros_typec_port *port = typec->ports[port_num];
> > +     struct typec_partner_desc p_desc = {
> > +             .usb_pd = pd_en,
> > +     };
> > +     int ret = 0;
> > +
> > +     /*
> > +      * Fill an initial PD identity, which will then be updated with info
> > +      * from the EC.
> > +      */
> > +     p_desc.identity = &port->p_identity;
> > +
> > +     port->partner = typec_register_partner(port->port, &p_desc);
> > +     if (IS_ERR_OR_NULL(port->partner)) {
> > +             ret = PTR_ERR(port->partner);
>
> If you're checking for IS_ERR_OR_NULL that means that port->partner can be NULL,
> so PTR_ERR(NULL) is 0 returning something that you don't really want.
>
> But looking at the typec_register_partner, NULL is not an option as returns a
> handle to the partner on success or ERR_PTR on failure. So, the code should just do:
>
> if (IS_ERR(port->partner))
>     return PTR_ERR(port->partner);
Will update the patch to do this on the next version, thanks.
>
> And AFAICS you don't need to set port->partner to NULL.
>
> Can you double check this?
I think we will need it, since we check for it in
cros_typec_set_port_params_v1() to avoid double registration of a
partner:

+       /* Register/remove partners when a connect/disconnect occurs. */
+       if (resp->enabled & PD_CTRL_RESP_ENABLED_CONNECTED) {
+               if (typec->ports[port_num]->partner)
+                       return;

So if it is not set to NULL on error earlier, the registration won't
take place here (in case another PD notification comes in while the
PD_CTRL_RESP_ENABLED_CONNECTED bit is still set). Kindly correct me if
this understanding is incorrect.

Best regards,

-Prashant

>
> Thanks,
>  Enric
>
> > +             port->partner = NULL;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  static void cros_typec_set_port_params_v0(struct cros_typec_data *typec,
> >               int port_num, struct ec_response_usb_pd_control *resp)
> >  {
> > @@ -212,6 +239,8 @@ static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
> >  {
> >       struct typec_port *port = typec->ports[port_num]->port;
> >       enum typec_orientation polarity;
> > +     bool pd_en;
> > +     int ret;
> >
> >       if (!(resp->enabled & PD_CTRL_RESP_ENABLED_CONNECTED))
> >               polarity = TYPEC_ORIENTATION_NONE;
> > @@ -226,6 +255,25 @@ static void cros_typec_set_port_params_v1(struct cros_typec_data *typec,
> >                       TYPEC_SOURCE : TYPEC_SINK);
> >       typec_set_vconn_role(port, resp->role & PD_CTRL_RESP_ROLE_VCONN ?
> >                       TYPEC_SOURCE : TYPEC_SINK);
> > +
> > +     /* Register/remove partners when a connect/disconnect occurs. */
> > +     if (resp->enabled & PD_CTRL_RESP_ENABLED_CONNECTED) {
> > +             if (typec->ports[port_num]->partner)
> > +                     return;
> > +
> > +             pd_en = resp->enabled & PD_CTRL_RESP_ENABLED_PD_CAPABLE;
> > +             ret = cros_typec_add_partner(typec, port_num, pd_en);
> > +             if (!ret)
> > +                     dev_warn(typec->dev,
> > +                              "Failed to register partner on port: %d\n",
> > +                              port_num);
> > +     } else {
> > +             if (!typec->ports[port_num]->partner)
> > +                     return;
> > +
> > +             typec_unregister_partner(typec->ports[port_num]->partner);
> > +             typec->ports[port_num]->partner = NULL;
> > +     }
> >  }
> >
> >  static int cros_typec_port_update(struct cros_typec_data *typec, int port_num)
> >