[PATCH 0/3] power: supply: cros: add support for dedicated port and expose connected ports

[PATCH 0/3] power: supply: cros: add support for dedicated port and expose connected ports

[Fabien Parent]

Dear all,

This patch series adds support for an optional dedicated port
to the ChromeOS power supply driver and adds a new property that expose
when a power supply is connected. The series was tested on ChromeOS "Fizz"
hardware.

This patch series depends on the following patch serie which adds
the ChromeOS power supply driver:
 * https://lkml.org/lkml/2018/5/2/585 [PATCH v4 0/3] mfd/power: cros_ec: add
   support for USBPD charger driver

The ChromeOS power supply driver also depends on the following patches to be
applied:
 * https://lkml.org/lkml/2018/4/23/602 ([PATCH v8 0/6] typec: tcpm: Add
   sink side support for PPS)
 * https://lkml.org/lkml/2018/4/18/229 ([RESEND PATCH v5 4/7] mfd:
   cros_ec_dev: Register cros-ec-rtc driver as a subdevice.)

Best Regards,
Fabien

Fabien Parent (3):
  mfd: cros: add charger port count command definition
  power: supply: cros: add support for dedicated port
  power: supply: cros: add property to detect connected ports

 drivers/power/supply/cros_usbpd-charger.c | 129 +++++++++++++++++++---
 include/linux/mfd/cros_ec_commands.h      |  10 ++
 2 files changed, 124 insertions(+), 15 deletions(-)

-- 
2.17.0

[PATCH 1/3] mfd: cros: add charger port count command definition

[Fabien Parent]

A new more command has been added to the ChromeOS embedded controller
that allows to get the number of charger port count. Unlike
EC_CMD_USB_PD_PORTS, this new command also includes the dedicated
port if present.

This command will be used to expose the dedicated charger port
in the ChromeOS charger driver.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
---
 include/linux/mfd/cros_ec_commands.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index 0d926492ac3a..e3187f8bdb7e 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -3005,6 +3005,16 @@ struct ec_params_usb_pd_info_request {
 	uint8_t port;
 } __packed;
 
+/*
+ * This command will return the number of USB PD charge port + the number
+ * of dedicated port present.
+ * EC_CMD_USB_PD_PORTS does NOT include the dedicated ports
+ */
+#define EC_CMD_CHARGE_PORT_COUNT 0x0105
+struct ec_response_charge_port_count {
+	uint8_t port_count;
+} __packed;
+
 /* Read USB-PD Device discovery info */
 #define EC_CMD_USB_PD_DISCOVERY 0x0113
 struct ec_params_usb_pd_discovery_entry {
-- 
2.17.0

[PATCH 2/3] power: supply: cros: add support for dedicated port

[Fabien Parent]

ChromeOS devices can have one optional dedicated port.
The Dedicated port is unique and similar to the USB PD ports
except that it doesn't support as many properties.

The presence of a dedicated port is determined from whether the
EC's charger port count is equal to 'number of USB PD port' + 1.
The dedicated port ID is always the last valid port ID.

This commit keeps compatibility with Embedded Controllers that do not
support the new EC_CMD_CHARGE_PORT_COUNT command by setting
the number of charger port to be equal to the number of USB PD port
when this command fails.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
---
 drivers/power/supply/cros_usbpd-charger.c | 115 +++++++++++++++++++---
 1 file changed, 101 insertions(+), 14 deletions(-)

diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
index 3a0c96fd1bc1..808688a6586c 100644
--- a/drivers/power/supply/cros_usbpd-charger.c
+++ b/drivers/power/supply/cros_usbpd-charger.c
@@ -12,8 +12,12 @@
 #include <linux/power_supply.h>
 #include <linux/slab.h>
 
-#define CHARGER_DIR_NAME			"CROS_USBPD_CHARGER%d"
-#define CHARGER_DIR_NAME_LENGTH			sizeof(CHARGER_DIR_NAME)
+#define CHARGER_USBPD_DIR_NAME			"CROS_USBPD_CHARGER%d"
+#define CHARGER_DEDICATED_DIR_NAME		"CROS_DEDICATED_CHARGER"
+#define CHARGER_DIR_NAME_LENGTH		(sizeof(CHARGER_USBPD_DIR_NAME) >= \
+					 sizeof(CHARGER_DEDICATED_DIR_NAME) ? \
+					 sizeof(CHARGER_USBPD_DIR_NAME) : \
+					 sizeof(CHARGER_DEDICATED_DIR_NAME))
 #define CHARGER_CACHE_UPDATE_DELAY		msecs_to_jiffies(500)
 #define CHARGER_MANUFACTURER_MODEL_LENGTH	32
 
@@ -42,6 +46,7 @@ struct charger_data {
 	struct cros_ec_dev *ec_dev;
 	struct cros_ec_device *ec_device;
 	int num_charger_ports;
+	int num_usbpd_ports;
 	int num_registered_psy;
 	struct port_data *ports[EC_USB_PD_MAX_PORTS];
 	struct notifier_block notifier;
@@ -58,6 +63,12 @@ static enum power_supply_property cros_usbpd_charger_props[] = {
 	POWER_SUPPLY_PROP_USB_TYPE
 };
 
+static enum power_supply_property cros_usbpd_dedicated_charger_props[] = {
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+};
+
 static enum power_supply_usb_type cros_usbpd_charger_usb_types[] = {
 	POWER_SUPPLY_USB_TYPE_UNKNOWN,
 	POWER_SUPPLY_USB_TYPE_SDP,
@@ -69,6 +80,11 @@ static enum power_supply_usb_type cros_usbpd_charger_usb_types[] = {
 	POWER_SUPPLY_USB_TYPE_APPLE_BRICK_ID
 };
 
+static bool cros_usbpd_charger_port_is_dedicated(struct port_data *port)
+{
+	return port->port_number >= port->charger->num_usbpd_ports;
+}
+
 static int cros_usbpd_charger_ec_command(struct charger_data *charger,
 					 unsigned int version,
 					 unsigned int command,
@@ -102,6 +118,23 @@ static int cros_usbpd_charger_ec_command(struct charger_data *charger,
 }
 
 static int cros_usbpd_charger_get_num_ports(struct charger_data *charger)
+{
+	struct ec_response_charge_port_count resp;
+	int ret;
+
+	ret = cros_usbpd_charger_ec_command(charger, 0,
+					    EC_CMD_CHARGE_PORT_COUNT,
+					    NULL, 0, &resp, sizeof(resp));
+	if (ret < 0) {
+		dev_err(charger->dev,
+			"Unable to get the number of ports (err:0x%x)\n", ret);
+		return ret;
+	}
+
+	return resp.port_count;
+}
+
+static int cros_usbpd_charger_get_usbpd_num_ports(struct charger_data *charger)
 {
 	struct ec_response_usb_pd_ports resp;
 	int ret;
@@ -246,7 +279,10 @@ static int cros_usbpd_charger_get_power_info(struct port_data *port)
 		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_SDP;
 	}
 
-	port->psy_desc.type = POWER_SUPPLY_TYPE_USB;
+	if (cros_usbpd_charger_port_is_dedicated(port))
+		port->psy_desc.type = POWER_SUPPLY_TYPE_MAINS;
+	else
+		port->psy_desc.type = POWER_SUPPLY_TYPE_USB;
 
 	dev_dbg(dev,
 		"Port %d: type=%d vmax=%d vnow=%d cmax=%d clim=%d pmax=%d\n",
@@ -281,7 +317,8 @@ static int cros_usbpd_charger_get_port_status(struct port_data *port,
 	if (ret < 0)
 		return ret;
 
-	ret = cros_usbpd_charger_get_discovery_info(port);
+	if (!cros_usbpd_charger_port_is_dedicated(port))
+		ret = cros_usbpd_charger_get_discovery_info(port);
 	port->last_update = jiffies;
 
 	return ret;
@@ -425,17 +462,56 @@ static int cros_usbpd_charger_probe(struct platform_device *pd)
 
 	platform_set_drvdata(pd, charger);
 
+	/*
+	 * We need to know the number of USB PD ports in order to know whether
+	 * there is a dedicated port. The dedicated port will always be
+	 * after the USB PD ports, and there should be only one.
+	 */
+	charger->num_usbpd_ports =
+		cros_usbpd_charger_get_usbpd_num_ports(charger);
+	if (charger->num_usbpd_ports <= 0) {
+		/*
+		 * This can happen on a system that doesn't support USB PD.
+		 * Log a message, but no need to warn.
+		 */
+		dev_info(dev, "No USB PD charging ports found\n");
+	}
+
 	charger->num_charger_ports = cros_usbpd_charger_get_num_ports(charger);
-	if (charger->num_charger_ports <= 0) {
+	if (charger->num_charger_ports < 0) {
 		/*
 		 * This can happen on a system that doesn't support USB PD.
 		 * Log a message, but no need to warn.
+		 * Older ECs do not support the above command, in that case
+		 * let's set up the number of charger ports equal to the number
+		 * of USB PD ports
+		 */
+		dev_info(dev, "Could not get charger port count\n");
+		charger->num_charger_ports = charger->num_usbpd_ports;
+	}
+
+	if (charger->num_charger_ports <= 0) {
+		/*
+		 * This can happen on a system that doesn't support USB PD and
+		 * doesn't have a dedicated port.
+		 * Log a message, but no need to warn.
 		 */
 		dev_info(dev, "No charging ports found\n");
 		ret = -ENODEV;
 		goto fail_nowarn;
 	}
 
+	/*
+	 * Sanity checks on the number of ports:
+	 *  there should be at most 1 dedicated port
+	 */
+	if (charger->num_charger_ports < charger->num_usbpd_ports ||
+	    charger->num_charger_ports > (charger->num_usbpd_ports + 1)) {
+		dev_err(dev, "Unexpected number of charge port count\n");
+		ret = -EPROTO;
+		goto fail_nowarn;
+	}
+
 	for (i = 0; i < charger->num_charger_ports; i++) {
 		struct power_supply_config psy_cfg = {};
 
@@ -447,22 +523,33 @@ static int cros_usbpd_charger_probe(struct platform_device *pd)
 
 		port->charger = charger;
 		port->port_number = i;
-		sprintf(port->name, CHARGER_DIR_NAME, i);
 
 		psy_desc = &port->psy_desc;
-		psy_desc->name = port->name;
-		psy_desc->type = POWER_SUPPLY_TYPE_USB;
 		psy_desc->get_property = cros_usbpd_charger_get_prop;
 		psy_desc->external_power_changed =
 					cros_usbpd_charger_power_changed;
-		psy_desc->properties = cros_usbpd_charger_props;
-		psy_desc->num_properties =
-					ARRAY_SIZE(cros_usbpd_charger_props);
-		psy_desc->usb_types = cros_usbpd_charger_usb_types;
-		psy_desc->num_usb_types =
-				ARRAY_SIZE(cros_usbpd_charger_usb_types);
 		psy_cfg.drv_data = port;
 
+		if (cros_usbpd_charger_port_is_dedicated(port)) {
+			sprintf(port->name, CHARGER_DEDICATED_DIR_NAME);
+			psy_desc->type = POWER_SUPPLY_TYPE_MAINS;
+			psy_desc->properties =
+				cros_usbpd_dedicated_charger_props;
+			psy_desc->num_properties =
+				ARRAY_SIZE(cros_usbpd_dedicated_charger_props);
+		} else {
+			sprintf(port->name, CHARGER_USBPD_DIR_NAME, i);
+			psy_desc->type = POWER_SUPPLY_TYPE_USB;
+			psy_desc->properties = cros_usbpd_charger_props;
+			psy_desc->num_properties =
+				ARRAY_SIZE(cros_usbpd_charger_props);
+			psy_desc->usb_types = cros_usbpd_charger_usb_types;
+			psy_desc->num_usb_types =
+				ARRAY_SIZE(cros_usbpd_charger_usb_types);
+		}
+
+		psy_desc->name = port->name;
+
 		psy = devm_power_supply_register_no_ws(dev, psy_desc,
 						       &psy_cfg);
 		if (IS_ERR(psy)) {
-- 
2.17.0

[PATCH 3/3] power: supply: cros: add property to detect connected ports

[Fabien Parent]

When a port is connected but acting as a source, its 'online' and
'status' properties are identical to a port that is not connected. This
makes it tedious for userspace to know for sure whether a port is
connected or not.

This commit adds a new property 'present' to reflect whether a port
is connected or not.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
---
 drivers/power/supply/cros_usbpd-charger.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
index 808688a6586c..d44ab35670ab 100644
--- a/drivers/power/supply/cros_usbpd-charger.c
+++ b/drivers/power/supply/cros_usbpd-charger.c
@@ -32,6 +32,7 @@ struct port_data {
 	struct power_supply_desc psy_desc;
 	int psy_usb_type;
 	int psy_online;
+	int psy_present;
 	int psy_status;
 	int psy_current_max;
 	int psy_voltage_max_design;
@@ -54,6 +55,7 @@ struct charger_data {
 
 static enum power_supply_property cros_usbpd_charger_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_CURRENT_MAX,
 	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
@@ -65,6 +67,7 @@ static enum power_supply_property cros_usbpd_charger_props[] = {
 
 static enum power_supply_property cros_usbpd_dedicated_charger_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
 };
@@ -205,18 +208,22 @@ static int cros_usbpd_charger_get_power_info(struct port_data *port)
 	case USB_PD_PORT_POWER_DISCONNECTED:
 		port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
 		port->psy_online = 0;
+		port->psy_present = 0;
 		break;
 	case USB_PD_PORT_POWER_SOURCE:
 		port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
 		port->psy_online = 0;
+		port->psy_present = 1;
 		break;
 	case USB_PD_PORT_POWER_SINK:
 		port->psy_status = POWER_SUPPLY_STATUS_CHARGING;
 		port->psy_online = 1;
+		port->psy_present = 1;
 		break;
 	case USB_PD_PORT_POWER_SINK_NOT_CHARGING:
 		port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
 		port->psy_online = 1;
+		port->psy_present = 1;
 		break;
 	default:
 		dev_err(dev, "Unknown role %d\n", resp.role);
@@ -362,6 +369,7 @@ static int cros_usbpd_charger_get_prop(struct power_supply *psy,
 		 */
 		if (ec_device->mkbp_event_supported || port->psy_online)
 			break;
+	case POWER_SUPPLY_PROP_PRESENT:
 	case POWER_SUPPLY_PROP_CURRENT_MAX:
 	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
@@ -380,6 +388,9 @@ static int cros_usbpd_charger_get_prop(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_ONLINE:
 		val->intval = port->psy_online;
 		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = port->psy_present;
+		break;
 	case POWER_SUPPLY_PROP_STATUS:
 		val->intval = port->psy_status;
 		break;
-- 
2.17.0

Re: [PATCH 3/3] power: supply: cros: add property to detect connected ports

[Enric Balletbo Serra]

Hi Fabien,

Many thanks for the patch.

2018-05-30 5:17 GMT+02:00 Fabien Parent <fparent@baylibre.com>:
> When a port is connected but acting as a source, its 'online' and
> 'status' properties are identical to a port that is not connected. This
> makes it tedious for userspace to know for sure whether a port is
> connected or not.
>
> This commit adds a new property 'present' to reflect whether a port
> is connected or not.
>

Actually, there is an attempt to align the usage of the different
properties between drivers [1]. According to current documentation,
the present property is used to report whether a battery is present or
not in the system. So, I am not sure if using the present property is
the proper way to do it. Maybe Sebastian can give us some clues or
preferences?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/tree/Documentation/ABI/testing/sysfs-class-power?h=for-next&id=91dabc54073324006d5eaba483679c47b6eb93a8#n159

Best regards,
  Enric

> Signed-off-by: Fabien Parent <fparent@baylibre.com>
> ---
>  drivers/power/supply/cros_usbpd-charger.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
> index 808688a6586c..d44ab35670ab 100644
> --- a/drivers/power/supply/cros_usbpd-charger.c
> +++ b/drivers/power/supply/cros_usbpd-charger.c
> @@ -32,6 +32,7 @@ struct port_data {
>         struct power_supply_desc psy_desc;
>         int psy_usb_type;
>         int psy_online;
> +       int psy_present;
>         int psy_status;
>         int psy_current_max;
>         int psy_voltage_max_design;
> @@ -54,6 +55,7 @@ struct charger_data {
>
>  static enum power_supply_property cros_usbpd_charger_props[] = {
>         POWER_SUPPLY_PROP_ONLINE,
> +       POWER_SUPPLY_PROP_PRESENT,
>         POWER_SUPPLY_PROP_STATUS,
>         POWER_SUPPLY_PROP_CURRENT_MAX,
>         POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> @@ -65,6 +67,7 @@ static enum power_supply_property cros_usbpd_charger_props[] = {
>
>  static enum power_supply_property cros_usbpd_dedicated_charger_props[] = {
>         POWER_SUPPLY_PROP_ONLINE,
> +       POWER_SUPPLY_PROP_PRESENT,
>         POWER_SUPPLY_PROP_STATUS,
>         POWER_SUPPLY_PROP_VOLTAGE_NOW,
>  };
> @@ -205,18 +208,22 @@ static int cros_usbpd_charger_get_power_info(struct port_data *port)
>         case USB_PD_PORT_POWER_DISCONNECTED:
>                 port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
>                 port->psy_online = 0;
> +               port->psy_present = 0;
>                 break;
>         case USB_PD_PORT_POWER_SOURCE:
>                 port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
>                 port->psy_online = 0;
> +               port->psy_present = 1;
>                 break;
>         case USB_PD_PORT_POWER_SINK:
>                 port->psy_status = POWER_SUPPLY_STATUS_CHARGING;
>                 port->psy_online = 1;
> +               port->psy_present = 1;
>                 break;
>         case USB_PD_PORT_POWER_SINK_NOT_CHARGING:
>                 port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
>                 port->psy_online = 1;
> +               port->psy_present = 1;
>                 break;
>         default:
>                 dev_err(dev, "Unknown role %d\n", resp.role);
> @@ -362,6 +369,7 @@ static int cros_usbpd_charger_get_prop(struct power_supply *psy,
>                  */
>                 if (ec_device->mkbp_event_supported || port->psy_online)
>                         break;
> +       case POWER_SUPPLY_PROP_PRESENT:
>         case POWER_SUPPLY_PROP_CURRENT_MAX:
>         case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
>         case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> @@ -380,6 +388,9 @@ static int cros_usbpd_charger_get_prop(struct power_supply *psy,
>         case POWER_SUPPLY_PROP_ONLINE:
>                 val->intval = port->psy_online;
>                 break;
> +       case POWER_SUPPLY_PROP_PRESENT:
> +               val->intval = port->psy_present;
> +               break;
>         case POWER_SUPPLY_PROP_STATUS:
>                 val->intval = port->psy_status;
>                 break;
> --
> 2.17.0
>

Re: [PATCH 1/3] mfd: cros: add charger port count command definition

[Enric Balletbo Serra]

2018-05-30 5:17 GMT+02:00 Fabien Parent <fparent@baylibre.com>:
> A new more command has been added to the ChromeOS embedded controller
> that allows to get the number of charger port count. Unlike
> EC_CMD_USB_PD_PORTS, this new command also includes the dedicated
> port if present.
>
> This command will be used to expose the dedicated charger port
> in the ChromeOS charger driver.
>
> Signed-off-by: Fabien Parent <fparent@baylibre.com>
> ---
>  include/linux/mfd/cros_ec_commands.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> index 0d926492ac3a..e3187f8bdb7e 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -3005,6 +3005,16 @@ struct ec_params_usb_pd_info_request {
>         uint8_t port;
>  } __packed;
>
> +/*
> + * This command will return the number of USB PD charge port + the number
> + * of dedicated port present.
> + * EC_CMD_USB_PD_PORTS does NOT include the dedicated ports
> + */
> +#define EC_CMD_CHARGE_PORT_COUNT 0x0105
> +struct ec_response_charge_port_count {
> +       uint8_t port_count;
> +} __packed;
> +
>  /* Read USB-PD Device discovery info */
>  #define EC_CMD_USB_PD_DISCOVERY 0x0113
>  struct ec_params_usb_pd_discovery_entry {
> --
> 2.17.0
>

The patch looks good to me, ideally, will be good if you can add the
kernel-doc documentation for the struct, in any case

Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Thanks,
  Enric

Re: [PATCH 1/3] mfd: cros: add charger port count command definition

[Lee Jones]

On Tue, 29 May 2018, Fabien Parent wrote:

> A new more command has been added to the ChromeOS embedded controller
> that allows to get the number of charger port count. Unlike
> EC_CMD_USB_PD_PORTS, this new command also includes the dedicated
> port if present.
> 
> This command will be used to expose the dedicated charger port
> in the ChromeOS charger driver.
> 
> Signed-off-by: Fabien Parent <fparent@baylibre.com>
> ---
>  include/linux/mfd/cros_ec_commands.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Does not want to apply.  I didn't investigate why.

Please rebase and resend?
-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/3] mfd: cros: add charger port count command definition

[Fabien Parent]

On Mon, Jun 4, 2018 at 1:59 AM, Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 29 May 2018, Fabien Parent wrote:
>
>> A new more command has been added to the ChromeOS embedded controller
>> that allows to get the number of charger port count. Unlike
>> EC_CMD_USB_PD_PORTS, this new command also includes the dedicated
>> port if present.
>>
>> This command will be used to expose the dedicated charger port
>> in the ChromeOS charger driver.
>>
>> Signed-off-by: Fabien Parent <fparent@baylibre.com>
>> ---
>>  include/linux/mfd/cros_ec_commands.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>
> Does not want to apply.  I didn't investigate why.
>
> Please rebase and resend?

Sorry, I forgot to add you as "to:" to the patch series.
This patch applies on top of https://lkml.org/lkml/2018/5/2/590
([PATCH v4 1/3] mfd: cros_ec: Add USBPD charger commands and struct
definitions)

> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 0/3] power: supply: cros: add support for dedicated port and expose connected ports

[Fabien Parent]

+ Lee Jones for the mfd patch.

On Tue, May 29, 2018 at 8:17 PM, Fabien Parent <fparent@baylibre.com> wrote:
> Dear all,
>
> This patch series adds support for an optional dedicated port
> to the ChromeOS power supply driver and adds a new property that expose
> when a power supply is connected. The series was tested on ChromeOS "Fizz"
> hardware.
>
> This patch series depends on the following patch serie which adds
> the ChromeOS power supply driver:
>  * https://lkml.org/lkml/2018/5/2/585 [PATCH v4 0/3] mfd/power: cros_ec: add
>    support for USBPD charger driver
>
> The ChromeOS power supply driver also depends on the following patches to be
> applied:
>  * https://lkml.org/lkml/2018/4/23/602 ([PATCH v8 0/6] typec: tcpm: Add
>    sink side support for PPS)
>  * https://lkml.org/lkml/2018/4/18/229 ([RESEND PATCH v5 4/7] mfd:
>    cros_ec_dev: Register cros-ec-rtc driver as a subdevice.)
>
> Best Regards,
> Fabien
>
> Fabien Parent (3):
>   mfd: cros: add charger port count command definition
>   power: supply: cros: add support for dedicated port
>   power: supply: cros: add property to detect connected ports
>
>  drivers/power/supply/cros_usbpd-charger.c | 129 +++++++++++++++++++---
>  include/linux/mfd/cros_ec_commands.h      |  10 ++
>  2 files changed, 124 insertions(+), 15 deletions(-)
>
> --
> 2.17.0
>

Re: [PATCH 1/3] mfd: cros: add charger port count command definition

[Lee Jones]

On Mon, 04 Jun 2018, Fabien Parent wrote:

> On Mon, Jun 4, 2018 at 1:59 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Tue, 29 May 2018, Fabien Parent wrote:
> >
> >> A new more command has been added to the ChromeOS embedded controller
> >> that allows to get the number of charger port count. Unlike
> >> EC_CMD_USB_PD_PORTS, this new command also includes the dedicated
> >> port if present.
> >>
> >> This command will be used to expose the dedicated charger port
> >> in the ChromeOS charger driver.
> >>
> >> Signed-off-by: Fabien Parent <fparent@baylibre.com>
> >> ---
> >>  include/linux/mfd/cros_ec_commands.h | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >
> > Does not want to apply.  I didn't investigate why.
> >
> > Please rebase and resend?
> 
> Sorry, I forgot to add you as "to:" to the patch series.
> This patch applies on top of https://lkml.org/lkml/2018/5/2/590
> ([PATCH v4 1/3] mfd: cros_ec: Add USBPD charger commands and struct
> definitions)

Okay.  Please re-submit after that set has landed then.

Please, also add my Ack to the patch:

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/3] power: supply: cros: add support for dedicated port

[Fabien Parent]

Hi,

Making sure this patch and the next one [1] are not being forgotten.

[1] https://patchwork.kernel.org/patch/10437565/

On Wed, May 30, 2018 at 5:17 AM, Fabien Parent <fparent@baylibre.com> wrote:
> ChromeOS devices can have one optional dedicated port.
> The Dedicated port is unique and similar to the USB PD ports
> except that it doesn't support as many properties.
>
> The presence of a dedicated port is determined from whether the
> EC's charger port count is equal to 'number of USB PD port' + 1.
> The dedicated port ID is always the last valid port ID.
>
> This commit keeps compatibility with Embedded Controllers that do not
> support the new EC_CMD_CHARGE_PORT_COUNT command by setting
> the number of charger port to be equal to the number of USB PD port
> when this command fails.
>
> Signed-off-by: Fabien Parent <fparent@baylibre.com>
> ---
>  drivers/power/supply/cros_usbpd-charger.c | 115 +++++++++++++++++++---
>  1 file changed, 101 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
> index 3a0c96fd1bc1..808688a6586c 100644
> --- a/drivers/power/supply/cros_usbpd-charger.c
> +++ b/drivers/power/supply/cros_usbpd-charger.c
> @@ -12,8 +12,12 @@
>  #include <linux/power_supply.h>
>  #include <linux/slab.h>
>
> -#define CHARGER_DIR_NAME                       "CROS_USBPD_CHARGER%d"
> -#define CHARGER_DIR_NAME_LENGTH                        sizeof(CHARGER_DIR_NAME)
> +#define CHARGER_USBPD_DIR_NAME                 "CROS_USBPD_CHARGER%d"
> +#define CHARGER_DEDICATED_DIR_NAME             "CROS_DEDICATED_CHARGER"
> +#define CHARGER_DIR_NAME_LENGTH                (sizeof(CHARGER_USBPD_DIR_NAME) >= \
> +                                        sizeof(CHARGER_DEDICATED_DIR_NAME) ? \
> +                                        sizeof(CHARGER_USBPD_DIR_NAME) : \
> +                                        sizeof(CHARGER_DEDICATED_DIR_NAME))
>  #define CHARGER_CACHE_UPDATE_DELAY             msecs_to_jiffies(500)
>  #define CHARGER_MANUFACTURER_MODEL_LENGTH      32
>
> @@ -42,6 +46,7 @@ struct charger_data {
>         struct cros_ec_dev *ec_dev;
>         struct cros_ec_device *ec_device;
>         int num_charger_ports;
> +       int num_usbpd_ports;
>         int num_registered_psy;
>         struct port_data *ports[EC_USB_PD_MAX_PORTS];
>         struct notifier_block notifier;
> @@ -58,6 +63,12 @@ static enum power_supply_property cros_usbpd_charger_props[] = {
>         POWER_SUPPLY_PROP_USB_TYPE
>  };
>
> +static enum power_supply_property cros_usbpd_dedicated_charger_props[] = {
> +       POWER_SUPPLY_PROP_ONLINE,
> +       POWER_SUPPLY_PROP_STATUS,
> +       POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +};
> +
>  static enum power_supply_usb_type cros_usbpd_charger_usb_types[] = {
>         POWER_SUPPLY_USB_TYPE_UNKNOWN,
>         POWER_SUPPLY_USB_TYPE_SDP,
> @@ -69,6 +80,11 @@ static enum power_supply_usb_type cros_usbpd_charger_usb_types[] = {
>         POWER_SUPPLY_USB_TYPE_APPLE_BRICK_ID
>  };
>
> +static bool cros_usbpd_charger_port_is_dedicated(struct port_data *port)
> +{
> +       return port->port_number >= port->charger->num_usbpd_ports;
> +}
> +
>  static int cros_usbpd_charger_ec_command(struct charger_data *charger,
>                                          unsigned int version,
>                                          unsigned int command,
> @@ -102,6 +118,23 @@ static int cros_usbpd_charger_ec_command(struct charger_data *charger,
>  }
>
>  static int cros_usbpd_charger_get_num_ports(struct charger_data *charger)
> +{
> +       struct ec_response_charge_port_count resp;
> +       int ret;
> +
> +       ret = cros_usbpd_charger_ec_command(charger, 0,
> +                                           EC_CMD_CHARGE_PORT_COUNT,
> +                                           NULL, 0, &resp, sizeof(resp));
> +       if (ret < 0) {
> +               dev_err(charger->dev,
> +                       "Unable to get the number of ports (err:0x%x)\n", ret);
> +               return ret;
> +       }
> +
> +       return resp.port_count;
> +}
> +
> +static int cros_usbpd_charger_get_usbpd_num_ports(struct charger_data *charger)
>  {
>         struct ec_response_usb_pd_ports resp;
>         int ret;
> @@ -246,7 +279,10 @@ static int cros_usbpd_charger_get_power_info(struct port_data *port)
>                 port->psy_usb_type = POWER_SUPPLY_USB_TYPE_SDP;
>         }
>
> -       port->psy_desc.type = POWER_SUPPLY_TYPE_USB;
> +       if (cros_usbpd_charger_port_is_dedicated(port))
> +               port->psy_desc.type = POWER_SUPPLY_TYPE_MAINS;
> +       else
> +               port->psy_desc.type = POWER_SUPPLY_TYPE_USB;
>
>         dev_dbg(dev,
>                 "Port %d: type=%d vmax=%d vnow=%d cmax=%d clim=%d pmax=%d\n",
> @@ -281,7 +317,8 @@ static int cros_usbpd_charger_get_port_status(struct port_data *port,
>         if (ret < 0)
>                 return ret;
>
> -       ret = cros_usbpd_charger_get_discovery_info(port);
> +       if (!cros_usbpd_charger_port_is_dedicated(port))
> +               ret = cros_usbpd_charger_get_discovery_info(port);
>         port->last_update = jiffies;
>
>         return ret;
> @@ -425,17 +462,56 @@ static int cros_usbpd_charger_probe(struct platform_device *pd)
>
>         platform_set_drvdata(pd, charger);
>
> +       /*
> +        * We need to know the number of USB PD ports in order to know whether
> +        * there is a dedicated port. The dedicated port will always be
> +        * after the USB PD ports, and there should be only one.
> +        */
> +       charger->num_usbpd_ports =
> +               cros_usbpd_charger_get_usbpd_num_ports(charger);
> +       if (charger->num_usbpd_ports <= 0) {
> +               /*
> +                * This can happen on a system that doesn't support USB PD.
> +                * Log a message, but no need to warn.
> +                */
> +               dev_info(dev, "No USB PD charging ports found\n");
> +       }
> +
>         charger->num_charger_ports = cros_usbpd_charger_get_num_ports(charger);
> -       if (charger->num_charger_ports <= 0) {
> +       if (charger->num_charger_ports < 0) {
>                 /*
>                  * This can happen on a system that doesn't support USB PD.
>                  * Log a message, but no need to warn.
> +                * Older ECs do not support the above command, in that case
> +                * let's set up the number of charger ports equal to the number
> +                * of USB PD ports
> +                */
> +               dev_info(dev, "Could not get charger port count\n");
> +               charger->num_charger_ports = charger->num_usbpd_ports;
> +       }
> +
> +       if (charger->num_charger_ports <= 0) {
> +               /*
> +                * This can happen on a system that doesn't support USB PD and
> +                * doesn't have a dedicated port.
> +                * Log a message, but no need to warn.
>                  */
>                 dev_info(dev, "No charging ports found\n");
>                 ret = -ENODEV;
>                 goto fail_nowarn;
>         }
>
> +       /*
> +        * Sanity checks on the number of ports:
> +        *  there should be at most 1 dedicated port
> +        */
> +       if (charger->num_charger_ports < charger->num_usbpd_ports ||
> +           charger->num_charger_ports > (charger->num_usbpd_ports + 1)) {
> +               dev_err(dev, "Unexpected number of charge port count\n");
> +               ret = -EPROTO;
> +               goto fail_nowarn;
> +       }
> +
>         for (i = 0; i < charger->num_charger_ports; i++) {
>                 struct power_supply_config psy_cfg = {};
>
> @@ -447,22 +523,33 @@ static int cros_usbpd_charger_probe(struct platform_device *pd)
>
>                 port->charger = charger;
>                 port->port_number = i;
> -               sprintf(port->name, CHARGER_DIR_NAME, i);
>
>                 psy_desc = &port->psy_desc;
> -               psy_desc->name = port->name;
> -               psy_desc->type = POWER_SUPPLY_TYPE_USB;
>                 psy_desc->get_property = cros_usbpd_charger_get_prop;
>                 psy_desc->external_power_changed =
>                                         cros_usbpd_charger_power_changed;
> -               psy_desc->properties = cros_usbpd_charger_props;
> -               psy_desc->num_properties =
> -                                       ARRAY_SIZE(cros_usbpd_charger_props);
> -               psy_desc->usb_types = cros_usbpd_charger_usb_types;
> -               psy_desc->num_usb_types =
> -                               ARRAY_SIZE(cros_usbpd_charger_usb_types);
>                 psy_cfg.drv_data = port;
>
> +               if (cros_usbpd_charger_port_is_dedicated(port)) {
> +                       sprintf(port->name, CHARGER_DEDICATED_DIR_NAME);
> +                       psy_desc->type = POWER_SUPPLY_TYPE_MAINS;
> +                       psy_desc->properties =
> +                               cros_usbpd_dedicated_charger_props;
> +                       psy_desc->num_properties =
> +                               ARRAY_SIZE(cros_usbpd_dedicated_charger_props);
> +               } else {
> +                       sprintf(port->name, CHARGER_USBPD_DIR_NAME, i);
> +                       psy_desc->type = POWER_SUPPLY_TYPE_USB;
> +                       psy_desc->properties = cros_usbpd_charger_props;
> +                       psy_desc->num_properties =
> +                               ARRAY_SIZE(cros_usbpd_charger_props);
> +                       psy_desc->usb_types = cros_usbpd_charger_usb_types;
> +                       psy_desc->num_usb_types =
> +                               ARRAY_SIZE(cros_usbpd_charger_usb_types);
> +               }
> +
> +               psy_desc->name = port->name;
> +
>                 psy = devm_power_supply_register_no_ws(dev, psy_desc,
>                                                        &psy_cfg);
>                 if (IS_ERR(psy)) {
> --
> 2.17.0
>