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

[PATCH 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.

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/cros_ec_typec.c | 121 ++++++++++++++++++++----
 1 file changed, 104 insertions(+), 17 deletions(-)

-- 
2.26.0.292.g33ef6b2f38-goog

[PATCH 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>
---
 drivers/platform/chrome/cros_ec_typec.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 874269c070739..cf7c2652a1d6d 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,13 @@ 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) {
+		dev_err(dev, "Failed to register notifier\n");
+		goto unregister_ports;
+	}
+
 	return 0;
 
 unregister_ports:
-- 
2.26.0.292.g33ef6b2f38-goog

[PATCH 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>
---
 drivers/platform/chrome/cros_ec_typec.c | 49 ++++++++++++++++---------
 1 file changed, 32 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index cf7c2652a1d6d..1955e1dfebc6d 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,26 @@ 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);
+		devm_kfree(typec->dev, typec->ports[i]);
+	}
+}
+
 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 +132,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 +156,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 +194,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 +211,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))
@@ -360,9 +377,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.292.g33ef6b2f38-goog

[PATCH 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>
---
 drivers/platform/chrome/cros_ec_typec.c | 47 +++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 1955e1dfebc6d..e7d4d6ccccca6 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. */
@@ -191,6 +194,29 @@ 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,
+	};
+
+	/*
+	 * 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)) {
+		port->partner = NULL;
+		return PTR_ERR(port->partner);
+	}
+
+	return 0;
+}
+
 static void cros_typec_set_port_params_v0(struct cros_typec_data *typec,
 		int port_num, struct ec_response_usb_pd_control *resp)
 {
@@ -213,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;
@@ -227,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.292.g33ef6b2f38-goog

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

[Enric Balletbo i Serra]

Hi Prashant,

Thank you for the patch.

On 7/4/20 3:09, 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>
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 874269c070739..cf7c2652a1d6d 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>

I think you need to add a kconfig dependency/selection in order to don't have
build problems.

>  #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);

nit: space between : and %d

> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
>  #ifdef CONFIG_ACPI
>  static const struct acpi_device_id cros_typec_acpi_id[] = {
>  	{ "GOOG0014", 0 },
> @@ -332,6 +350,13 @@ 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) {
> +		dev_err(dev, "Failed to register notifier\n");

Currently cros_usbpd_register_notfity calls blocking_notifier_chain_register
that always return zero. Is fine to check the error but the print is unneded. If
probe fails will report the error.

> +		goto unregister_ports;
> +	}
> +
>  	return 0;
>  
>  unregister_ports:
> 

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

[Enric Balletbo i Serra]

Hi Prashant,

Thank you for the patch.

On 7/4/20 3:09, 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>
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 49 ++++++++++++++++---------
>  1 file changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index cf7c2652a1d6d..1955e1dfebc6d 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,26 @@ 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);
> +		devm_kfree(typec->dev, typec->ports[i]);

This is not needed, the allocated memory is device managed, so will be freed on
removal or error.

> +	}
> +}
> +
>  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 +132,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 +156,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 +194,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 +211,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))
> @@ -360,9 +377,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;
>  }
>  
> 

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

[Enric Balletbo i Serra]

Hi Prashant,

Thank you for your patch.

On 7/4/20 3:09, 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>
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 47 +++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 1955e1dfebc6d..e7d4d6ccccca6 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. */
> @@ -191,6 +194,29 @@ 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,
> +	};
> +
> +	/*
> +	 * 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)) {
> +		port->partner = NULL;
> +		return PTR_ERR(port->partner);

This is always returning PTR_ERR(NULL) that yields 0, that's not what you want.
A static checker warning will be triggered.


> +	}
> +
> +	return 0;
> +}
> +
>  static void cros_typec_set_port_params_v0(struct cros_typec_data *typec,
>  		int port_num, struct ec_response_usb_pd_control *resp)
>  {
> @@ -213,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;
> @@ -227,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 1/3] platform/chrome: typec: Use notifier for updates

[Prashant Malani]

Hi Enric,

Thanks for the reviews, as always. Kindly see inline:

On Thu, Apr 09, 2020 at 11:01:42PM +0200, Enric Balletbo i Serra wrote:
> Hi Prashant,
> 
> Thank you for the patch.
> 
> On 7/4/20 3:09, 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>
> > ---
> >  drivers/platform/chrome/cros_ec_typec.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index 874269c070739..cf7c2652a1d6d 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>
> 
> I think you need to add a kconfig dependency/selection in order to don't have
> build problems.

Thanks, will make the change.
> 
> >  #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);
> 
> nit: space between : and %d
> 
Done.
> > +	}
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> >  #ifdef CONFIG_ACPI
> >  static const struct acpi_device_id cros_typec_acpi_id[] = {
> >  	{ "GOOG0014", 0 },
> > @@ -332,6 +350,13 @@ 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) {
> > +		dev_err(dev, "Failed to register notifier\n");
> 
> Currently cros_usbpd_register_notfity calls blocking_notifier_chain_register
> that always return zero. Is fine to check the error but the print is unneded. If
> probe fails will report the error.
> 
Done.

> > +		goto unregister_ports;
> > +	}
> > +
> >  	return 0;
> >  
> >  unregister_ports:
> > 

Best regards,

-Prashant

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

[Prashant Malani]

Thanks Enric,

On Thu, Apr 09, 2020 at 11:19:51PM +0200, Enric Balletbo i Serra wrote:
> Hi Prashant,
> 
> Thank you for the patch.
> 
> On 7/4/20 3:09, 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>
> > ---
> >  drivers/platform/chrome/cros_ec_typec.c | 49 ++++++++++++++++---------
> >  1 file changed, 32 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index cf7c2652a1d6d..1955e1dfebc6d 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,26 @@ 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);
> > +		devm_kfree(typec->dev, typec->ports[i]);
> 
> This is not needed, the allocated memory is device managed, so will be freed on
> removal or error.
> 
Got it. Will make the change.
> > +	}
> > +}
> > +
> >  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 +132,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 +156,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 +194,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 +211,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))
> > @@ -360,9 +377,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;
> >  }
> >  
> > 

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

[Prashant Malani]

Hi Enric,

Thanks as always. Response inline:

On Thu, Apr 09, 2020 at 11:40:39PM +0200, Enric Balletbo i Serra wrote:
> Hi Prashant,
> 
> Thank you for your patch.
> 
> On 7/4/20 3:09, 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>
> > ---
> >  drivers/platform/chrome/cros_ec_typec.c | 47 +++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index 1955e1dfebc6d..e7d4d6ccccca6 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. */
> > @@ -191,6 +194,29 @@ 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,
> > +	};
> > +
> > +	/*
> > +	 * 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)) {
> > +		port->partner = NULL;
> > +		return PTR_ERR(port->partner);
> 
> This is always returning PTR_ERR(NULL) that yields 0, that's not what you want.
> A static checker warning will be triggered.
> 
Got it. I will use a "ret" variable.
> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static void cros_typec_set_port_params_v0(struct cros_typec_data *typec,
> >  		int port_num, struct ec_response_usb_pd_control *resp)
> >  {
> > @@ -213,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;
> > @@ -227,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)
> >