[PATCH v2 0/3] mfd/power: cros_ec: add support for USBPD charger driver.

[PATCH v2 0/3] mfd/power: cros_ec: add support for USBPD charger driver.

[Enric Balletbo i Serra]

Dear all,

This is a second version of a patched to add support for the ChromeOS EC
USBPD charger driver, the driver has been used on Chromebooks kernel for
long time and has been ported now to mainline. The patches were tested
successfully with a Samsung Chromebook Plus and an Pixel 2 Chromebook.

This version is a rework with respect the first version, some features
like the PD log have been removed and will be send as follow up patches
for further discuss.

The second patch of this series depends on these series that are queued
for 4.18:

- https://lkml.org/lkml/2018/4/23/602 ([PATCH v8 0/6] typec: tcpm: Add
  sink side support for PPS)

The third patch of this series depends on the following patch to apply
cleanly:

- 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,
 Enric

Changes in v2:
- [2/3] Add SPDX header, use devm_ variant and drop .owner
- [2/3] Removed the PD log functionality (will be send on a follow up patch)
- [2/3] Removed the extra custom sysfs attributes (will be send on a follow up patch)
- [3/3] Use ARRAY_SIZE instead of hardcoded 1.

Enric Balletbo i Serra (1):
  mfd: cros_ec_dev: Register cros_usbpd-charger driver as a subdevice.

Sameer Nanda (2):
  mfd: cros_ec: Add USBPD charger commands and struct definitions.
  power: supply: add cros-ec USBPD charger driver.

 drivers/mfd/cros_ec_dev.c                 |  16 +
 drivers/power/supply/Kconfig              |  10 +
 drivers/power/supply/Makefile             |   1 +
 drivers/power/supply/cros_usbpd-charger.c | 511 ++++++++++++++++++++++
 include/linux/mfd/cros_ec.h               |   2 +
 include/linux/mfd/cros_ec_commands.h      | 132 +++++-
 6 files changed, 668 insertions(+), 4 deletions(-)
 create mode 100644 drivers/power/supply/cros_usbpd-charger.c

-- 
2.17.0

[PATCH v2 1/3] mfd: cros_ec: Add USBPD charger commands and struct definitions.

[Enric Balletbo i Serra]

From: Sameer Nanda <snanda@chromium.org>

The USBPD charger driver gets information from the ChromeOS EC, this
patch adds the USBPD charger definitions needed by this driver.

Signed-off-by: Sameer Nanda <snanda@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

Changes in v2: None

 include/linux/mfd/cros_ec_commands.h | 132 ++++++++++++++++++++++++++-
 1 file changed, 128 insertions(+), 4 deletions(-)

diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index f2edd9969b40..94dcf331796c 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -2593,14 +2593,18 @@ struct ec_params_current_limit {
 } __packed;
 
 /*
- * Set maximum external power current.
+ * Set maximum external voltage / current.
  */
-#define EC_CMD_EXT_POWER_CURRENT_LIMIT 0xa2
+#define EC_CMD_EXTERNAL_POWER_LIMIT 0x00A2
 
-struct ec_params_ext_power_current_limit {
-	uint32_t limit; /* in mA */
+/* Command v0 is used only on Spring and is obsolete + unsupported */
+struct ec_params_external_power_limit_v1 {
+	uint16_t current_lim; /* in mA, or EC_POWER_LIMIT_NONE to clear limit */
+	uint16_t voltage_lim; /* in mV, or EC_POWER_LIMIT_NONE to clear limit */
 } __packed;
 
+#define EC_POWER_LIMIT_NONE 0xffff
+
 /* Inform the EC when entering a sleep state */
 #define EC_CMD_HOST_SLEEP_EVENT 0xa9
 
@@ -2974,6 +2978,12 @@ enum usb_chg_type {
 	USB_CHG_TYPE_VBUS,
 	USB_CHG_TYPE_UNKNOWN,
 };
+enum usb_power_roles {
+	USB_PD_PORT_POWER_DISCONNECTED,
+	USB_PD_PORT_POWER_SOURCE,
+	USB_PD_PORT_POWER_SINK,
+	USB_PD_PORT_POWER_SINK_NOT_CHARGING,
+};
 
 struct usb_chg_measures {
 	uint16_t voltage_max;
@@ -2991,6 +3001,120 @@ struct ec_response_usb_pd_power_info {
 	uint32_t max_power;
 } __packed;
 
+struct ec_params_usb_pd_info_request {
+	uint8_t port;
+} __packed;
+
+/* Read USB-PD Device discovery info */
+#define EC_CMD_USB_PD_DISCOVERY 0x0113
+struct ec_params_usb_pd_discovery_entry {
+	uint16_t vid;  /* USB-IF VID */
+	uint16_t pid;  /* USB-IF PID */
+	uint8_t ptype; /* product type (hub,periph,cable,ama) */
+} __packed;
+
+/* Override default charge behavior */
+#define EC_CMD_PD_CHARGE_PORT_OVERRIDE 0x0114
+
+/* Negative port parameters have special meaning */
+enum usb_pd_override_ports {
+	OVERRIDE_DONT_CHARGE = -2,
+	OVERRIDE_OFF = -1,
+	/* [0, CONFIG_USB_PD_PORT_COUNT): Port# */
+};
+
+struct ec_params_charge_port_override {
+	int16_t override_port; /* Override port# */
+} __packed;
+
+/* Read (and delete) one entry of PD event log */
+#define EC_CMD_PD_GET_LOG_ENTRY 0x0115
+
+struct ec_response_pd_log {
+	uint32_t timestamp; /* relative timestamp in milliseconds */
+	uint8_t type;       /* event type : see PD_EVENT_xx below */
+	uint8_t size_port;  /* [7:5] port number [4:0] payload size in bytes */
+	uint16_t data;      /* type-defined data payload */
+	uint8_t payload[0]; /* optional additional data payload: 0..16 bytes */
+} __packed;
+
+/* The timestamp is the microsecond counter shifted to get about a ms. */
+#define PD_LOG_TIMESTAMP_SHIFT 10 /* 1 LSB = 1024us */
+
+#define PD_LOG_SIZE_MASK  0x1f
+#define PD_LOG_PORT_MASK  0xe0
+#define PD_LOG_PORT_SHIFT    5
+#define PD_LOG_PORT_SIZE(port, size) (((port) << PD_LOG_PORT_SHIFT) | \
+				      ((size) & PD_LOG_SIZE_MASK))
+#define PD_LOG_PORT(size_port) ((size_port) >> PD_LOG_PORT_SHIFT)
+#define PD_LOG_SIZE(size_port) ((size_port) & PD_LOG_SIZE_MASK)
+
+/* PD event log : entry types */
+/* PD MCU events */
+#define PD_EVENT_MCU_BASE       0x00
+#define PD_EVENT_MCU_CHARGE             (PD_EVENT_MCU_BASE+0)
+#define PD_EVENT_MCU_CONNECT            (PD_EVENT_MCU_BASE+1)
+/* Reserved for custom board event */
+#define PD_EVENT_MCU_BOARD_CUSTOM       (PD_EVENT_MCU_BASE+2)
+/* PD generic accessory events */
+#define PD_EVENT_ACC_BASE       0x20
+#define PD_EVENT_ACC_RW_FAIL   (PD_EVENT_ACC_BASE+0)
+#define PD_EVENT_ACC_RW_ERASE  (PD_EVENT_ACC_BASE+1)
+/* PD power supply events */
+#define PD_EVENT_PS_BASE        0x40
+#define PD_EVENT_PS_FAULT      (PD_EVENT_PS_BASE+0)
+/* PD video dongles events */
+#define PD_EVENT_VIDEO_BASE     0x60
+#define PD_EVENT_VIDEO_DP_MODE (PD_EVENT_VIDEO_BASE+0)
+#define PD_EVENT_VIDEO_CODEC   (PD_EVENT_VIDEO_BASE+1)
+/* Returned in the "type" field, when there is no entry available */
+#define PD_EVENT_NO_ENTRY       0xff
+
+/*
+ * PD_EVENT_MCU_CHARGE event definition :
+ * the payload is "struct usb_chg_measures"
+ * the data field contains the port state flags as defined below :
+ */
+/* Port partner is a dual role device */
+#define CHARGE_FLAGS_DUAL_ROLE         (1 << 15)
+/* Port is the pending override port */
+#define CHARGE_FLAGS_DELAYED_OVERRIDE  (1 << 14)
+/* Port is the override port */
+#define CHARGE_FLAGS_OVERRIDE          (1 << 13)
+/* Charger type */
+#define CHARGE_FLAGS_TYPE_SHIFT               3
+#define CHARGE_FLAGS_TYPE_MASK       (0xf << CHARGE_FLAGS_TYPE_SHIFT)
+/* Power delivery role */
+#define CHARGE_FLAGS_ROLE_MASK         (7 <<  0)
+
+/*
+ * PD_EVENT_PS_FAULT data field flags definition :
+ */
+#define PS_FAULT_OCP                          1
+#define PS_FAULT_FAST_OCP                     2
+#define PS_FAULT_OVP                          3
+#define PS_FAULT_DISCH                        4
+
+/*
+ * PD_EVENT_VIDEO_CODEC payload is "struct mcdp_info".
+ */
+struct mcdp_version {
+	uint8_t major;
+	uint8_t minor;
+	uint16_t build;
+} __packed;
+
+struct mcdp_info {
+	uint8_t family[2];
+	uint8_t chipid[2];
+	struct mcdp_version irom;
+	struct mcdp_version fw;
+} __packed;
+
+/* struct mcdp_info field decoding */
+#define MCDP_CHIPID(chipid) ((chipid[0] << 8) | chipid[1])
+#define MCDP_FAMILY(family) ((family[0] << 8) | family[1])
+
 /* Get info about USB-C SS muxes */
 #define EC_CMD_USB_PD_MUX_INFO 0x11a
 
-- 
2.17.0

[PATCH v2 2/3] power: supply: add cros-ec USBPD charger driver.

[Enric Balletbo i Serra]

From: Sameer Nanda <snanda@chromium.org>

This driver gets various bits of information about what is connected to
USB PD ports from the EC and converts that into power_supply properties.

Signed-off-by: Sameer Nanda <snanda@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

Changes in v2:
- [2/3] Add SPDX header, use devm_ variant and drop .owner
- [2/3] Removed the PD log functionality (will be send on a follow up patch)
- [2/3] Removed the extra custom sysfs attributes (will be send on a follow up patch)

 drivers/power/supply/Kconfig              |  10 +
 drivers/power/supply/Makefile             |   1 +
 drivers/power/supply/cros_usbpd-charger.c | 511 ++++++++++++++++++++++
 include/linux/mfd/cros_ec.h               |   2 +
 4 files changed, 524 insertions(+)
 create mode 100644 drivers/power/supply/cros_usbpd-charger.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 428b426842f4..046eb23ba6f0 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -624,4 +624,14 @@ config CHARGER_RT9455
 	help
 	  Say Y to enable support for Richtek RT9455 battery charger.
 
+config CHARGER_CROS_USBPD
+	tristate "ChromeOS EC based USBPD charger"
+	depends on MFD_CROS_EC
+	default n
+	help
+	  Say Y here to enable ChromeOS EC based USBPD charger
+	  driver. This driver gets various bits of information about
+	  what is connected to USB PD ports from the EC and converts
+	  that into power_supply properties.
+
 endif # POWER_SUPPLY
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index e83aa843bcc6..907e26f824b2 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -83,3 +83,4 @@ obj-$(CONFIG_CHARGER_TPS65090)	+= tps65090-charger.o
 obj-$(CONFIG_CHARGER_TPS65217)	+= tps65217_charger.o
 obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
 obj-$(CONFIG_AXP288_CHARGER)	+= axp288_charger.o
+obj-$(CONFIG_CHARGER_CROS_USBPD)	+= cros_usbpd-charger.o
diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
new file mode 100644
index 000000000000..c57d1c41828c
--- /dev/null
+++ b/drivers/power/supply/cros_usbpd-charger.c
@@ -0,0 +1,511 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Power supply driver for ChromeOS EC based USB PD Charger.
+ *
+ * Copyright (c) 2014 - 2018 Google, Inc
+ */
+
+#include <linux/module.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+#include <linux/platform_device.h>
+#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_CACHE_UPDATE_DELAY		msecs_to_jiffies(500)
+#define CHARGER_MANUFACTURER_MODEL_LENGTH	32
+
+#define DRV_NAME "cros-usbpd-charger"
+
+struct port_data {
+	int port_number;
+	char name[CHARGER_DIR_NAME_LENGTH];
+	char manufacturer[CHARGER_MANUFACTURER_MODEL_LENGTH];
+	char model_name[CHARGER_MANUFACTURER_MODEL_LENGTH];
+	struct power_supply *psy;
+	struct power_supply_desc psy_desc;
+	int psy_usb_type;
+	int psy_online;
+	int psy_status;
+	int psy_current_max;
+	int psy_voltage_max_design;
+	int psy_voltage_now;
+	int psy_power_max;
+	struct charger_data *charger;
+	unsigned long last_update;
+};
+
+struct charger_data {
+	struct device *dev;
+	struct cros_ec_dev *ec_dev;
+	struct cros_ec_device *ec_device;
+	int num_charger_ports;
+	int num_registered_psy;
+	struct port_data *ports[EC_USB_PD_MAX_PORTS];
+	struct notifier_block notifier;
+};
+
+static enum power_supply_property cros_usbpd_charger_props[] = {
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_CURRENT_MAX,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_MANUFACTURER,
+};
+
+static int cros_usbpd_charger_ec_command(struct charger_data *charger,
+					 unsigned int version,
+					 unsigned int command,
+					 void *outdata,
+					 unsigned int outsize,
+					 void *indata,
+					 unsigned int insize)
+{
+	struct cros_ec_dev *ec_dev = charger->ec_dev;
+	struct cros_ec_command *msg;
+	int ret;
+
+	msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->version = version;
+	msg->command = ec_dev->cmd_offset + command;
+	msg->outsize = outsize;
+	msg->insize = insize;
+
+	if (outsize)
+		memcpy(msg->data, outdata, outsize);
+
+	ret = cros_ec_cmd_xfer_status(charger->ec_device, msg);
+	if (ret >= 0 && insize)
+		memcpy(indata, msg->data, insize);
+
+	kfree(msg);
+	return ret;
+}
+
+static int cros_usbpd_charger_get_num_ports(struct charger_data *charger)
+{
+	struct ec_response_usb_pd_ports resp;
+	int ret;
+
+	ret = cros_usbpd_charger_ec_command(charger, 0, EC_CMD_USB_PD_PORTS,
+					    NULL, 0, &resp, sizeof(resp));
+	if (ret < 0) {
+		dev_err(charger->dev,
+			"Unable to get the number or ports (err:0x%x)\n", ret);
+		return ret;
+	}
+
+	return resp.num_ports;
+}
+
+static int cros_usbpd_charger_get_discovery_info(struct port_data *port)
+{
+	struct charger_data *charger = port->charger;
+	struct ec_params_usb_pd_discovery_entry resp;
+	struct ec_params_usb_pd_info_request req;
+	int ret;
+
+	req.port = port->port_number;
+
+	ret = cros_usbpd_charger_ec_command(charger, 0,
+					    EC_CMD_USB_PD_DISCOVERY,
+					    &req, sizeof(req),
+					    &resp, sizeof(resp));
+	if (ret < 0) {
+		dev_err(charger->dev,
+			"Unable to query discovery info (err:0x%x)\n", ret);
+		return ret;
+	}
+
+	dev_dbg(charger->dev, "Port %d: VID = 0x%x, PID=0x%x, PTYPE=0x%x\n",
+		port->port_number, resp.vid, resp.pid, resp.ptype);
+
+	snprintf(port->manufacturer, sizeof(port->manufacturer), "%x",
+		 resp.vid);
+	snprintf(port->model_name, sizeof(port->model_name), "%x", resp.pid);
+
+	return 0;
+}
+
+static int cros_usbpd_charger_get_power_info(struct port_data *port)
+{
+	struct charger_data *charger = port->charger;
+	struct ec_response_usb_pd_power_info resp;
+	struct ec_params_usb_pd_power_info req;
+	int last_psy_status, last_psy_usb_type;
+	struct device *dev = charger->dev;
+	int ret;
+
+	req.port = port->port_number;
+	ret = cros_usbpd_charger_ec_command(charger, 0,
+					    EC_CMD_USB_PD_POWER_INFO,
+					    &req, sizeof(req),
+					    &resp, sizeof(resp));
+	if (ret < 0) {
+		dev_err(dev, "Unable to query PD power info (err:0x%x)\n", ret);
+		return ret;
+	}
+
+	last_psy_status = port->psy_status;
+	last_psy_usb_type = port->psy_usb_type;
+
+	switch (resp.role) {
+	case USB_PD_PORT_POWER_DISCONNECTED:
+		port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		port->psy_online = 0;
+		break;
+	case USB_PD_PORT_POWER_SOURCE:
+		port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		port->psy_online = 0;
+		break;
+	case USB_PD_PORT_POWER_SINK:
+		port->psy_status = POWER_SUPPLY_STATUS_CHARGING;
+		port->psy_online = 1;
+		break;
+	case USB_PD_PORT_POWER_SINK_NOT_CHARGING:
+		port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		port->psy_online = 1;
+		break;
+	default:
+		dev_err(dev, "Unknown role %d\n", resp.role);
+		break;
+	}
+
+	port->psy_voltage_max_design = resp.meas.voltage_max;
+	port->psy_voltage_now = resp.meas.voltage_now;
+	port->psy_current_max = resp.meas.current_max;
+	port->psy_power_max = resp.max_power;
+
+	switch (resp.type) {
+	case USB_CHG_TYPE_BC12_SDP:
+	case USB_CHG_TYPE_VBUS:
+		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_SDP;
+		break;
+	case USB_CHG_TYPE_NONE:
+		/*
+		 * For dual-role devices when we are a source, the firmware
+		 * reports the type as NONE. Report such chargers as type
+		 * USB_PD_DRP.
+		 */
+		if (resp.role == USB_PD_PORT_POWER_SOURCE && resp.dualrole)
+			port->psy_usb_type = POWER_SUPPLY_USB_TYPE_PD_DRP;
+		else
+			port->psy_usb_type = POWER_SUPPLY_USB_TYPE_SDP;
+		break;
+	case USB_CHG_TYPE_OTHER:
+	case USB_CHG_TYPE_PROPRIETARY:
+		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_APPLE_BRICK_ID;
+		break;
+	case USB_CHG_TYPE_C:
+		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_C;
+		break;
+	case USB_CHG_TYPE_BC12_DCP:
+		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_DCP;
+		break;
+	case USB_CHG_TYPE_BC12_CDP:
+		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_CDP;
+		break;
+	case USB_CHG_TYPE_PD:
+		if (resp.dualrole)
+			port->psy_usb_type = POWER_SUPPLY_USB_TYPE_PD_DRP;
+		else
+			port->psy_usb_type = POWER_SUPPLY_USB_TYPE_PD;
+		break;
+	case USB_CHG_TYPE_UNKNOWN:
+		/*
+		 * While the EC is trying to determine the type of charger that
+		 * has been plugged in, it will report the charger type as
+		 * unknown. Additionally since the power capabilities are
+		 * unknown, report the max current and voltage as zero.
+		 */
+		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_UNKNOWN;
+		port->psy_voltage_max_design = 0;
+		port->psy_current_max = 0;
+		break;
+	default:
+		dev_err(dev, "Port %d: default case!\n", port->port_number);
+		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_SDP;
+	}
+
+	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",
+		port->port_number, resp.type, resp.meas.voltage_max,
+		resp.meas.voltage_now, resp.meas.current_max,
+		resp.meas.current_lim, resp.max_power);
+
+	/*
+	 * If power supply type or status changed, explicitly call
+	 * power_supply_changed. This results in udev event getting generated
+	 * and allows user mode apps to react quicker instead of waiting for
+	 * their next poll of power supply status.
+	 */
+	if (last_psy_usb_type != port->psy_usb_type ||
+	    last_psy_status != port->psy_status)
+		power_supply_changed(port->psy);
+
+	return 0;
+}
+
+static int cros_usbpd_charger_get_port_status(struct port_data *port,
+					      bool ratelimit)
+{
+	int ret;
+
+	if (ratelimit &&
+	    time_is_after_jiffies(port->last_update +
+				  CHARGER_CACHE_UPDATE_DELAY))
+		return 0;
+
+	ret = cros_usbpd_charger_get_power_info(port);
+	if (ret < 0)
+		return ret;
+
+	ret = cros_usbpd_charger_get_discovery_info(port);
+	port->last_update = jiffies;
+
+	return ret;
+}
+
+static void cros_usbpd_charger_power_changed(struct power_supply *psy)
+{
+	struct port_data *port = power_supply_get_drvdata(psy);
+	struct charger_data *charger = port->charger;
+	int i;
+
+	for (i = 0; i < charger->num_registered_psy; i++)
+		cros_usbpd_charger_get_port_status(charger->ports[i], false);
+}
+
+static int cros_usbpd_charger_get_prop(struct power_supply *psy,
+				       enum power_supply_property psp,
+				       union power_supply_propval *val)
+{
+	struct port_data *port = power_supply_get_drvdata(psy);
+	struct charger_data *charger = port->charger;
+	struct cros_ec_device *ec_device = charger->ec_device;
+	struct device *dev = charger->dev;
+	int ret;
+
+	/* Only refresh ec_port_status for dynamic properties */
+	switch (psp) {
+	case POWER_SUPPLY_PROP_ONLINE:
+		/*
+		 * If mkbp_event_supported, then we can be assured that
+		 * the driver's state for the online property is consistent
+		 * with the hardware. However, if we aren't event driven,
+		 * the optimization before to skip an ec_port_status get
+		 * and only returned cached values of the online property will
+		 * cause a delay in detecting a cable attach until one of the
+		 * other properties are read.
+		 *
+		 * Allow an ec_port_status refresh for online property check
+		 * if we're not already online to check for plug events if
+		 * not mkbp_event_supported.
+		 */
+		if (ec_device->mkbp_event_supported || port->psy_online)
+			break;
+	case POWER_SUPPLY_PROP_CURRENT_MAX:
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		ret = cros_usbpd_charger_get_port_status(port, true);
+		if (ret < 0) {
+			dev_err(dev, "Failed to get port status (err:0x%x)\n",
+				ret);
+			return -EINVAL;
+		}
+		break;
+	default:
+		break;
+	}
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = port->psy_online;
+		break;
+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = port->psy_status;
+		break;
+	case POWER_SUPPLY_PROP_CURRENT_MAX:
+		val->intval = port->psy_current_max * 1000;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+		val->intval = port->psy_voltage_max_design * 1000;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = port->psy_voltage_now * 1000;
+		break;
+	case POWER_SUPPLY_PROP_MODEL_NAME:
+		val->strval = port->model_name;
+		break;
+	case POWER_SUPPLY_PROP_MANUFACTURER:
+		val->strval = port->manufacturer;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int cros_usbpd_charger_ec_event(struct notifier_block *nb,
+				       unsigned long queued_during_suspend,
+				       void *_notify)
+{
+	struct cros_ec_device *ec_device;
+	struct charger_data *charger;
+	struct device *dev;
+	u32 host_event;
+
+	charger = container_of(nb, struct charger_data, notifier);
+	ec_device = charger->ec_device;
+	dev = charger->dev;
+
+	host_event = cros_ec_get_host_event(ec_device);
+	if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
+		cros_usbpd_charger_power_changed(charger->ports[0]->psy);
+		return NOTIFY_OK;
+	} else {
+		return NOTIFY_DONE;
+	}
+}
+
+static int cros_usbpd_charger_probe(struct platform_device *pd)
+{
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
+	struct cros_ec_device *ec_device = ec_dev->ec_dev;
+	struct power_supply_desc *psy_desc;
+	struct device *dev = &pd->dev;
+	struct charger_data *charger;
+	struct power_supply *psy;
+	struct port_data *port;
+	int ret = -EINVAL;
+	int i;
+
+	charger = devm_kzalloc(dev, sizeof(struct charger_data),
+			       GFP_KERNEL);
+	if (!charger)
+		return -ENOMEM;
+
+	charger->dev = dev;
+	charger->ec_dev = ec_dev;
+	charger->ec_device = ec_device;
+
+	platform_set_drvdata(pd, charger);
+
+	charger->num_charger_ports = cros_usbpd_charger_get_num_ports(charger);
+	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.
+		 */
+		dev_info(dev, "No charging ports found\n");
+		ret = -ENODEV;
+		goto fail_nowarn;
+	}
+
+	for (i = 0; i < charger->num_charger_ports; i++) {
+		struct power_supply_config psy_cfg = {};
+
+		port = devm_kzalloc(dev, sizeof(struct port_data), GFP_KERNEL);
+		if (!port) {
+			ret = -ENOMEM;
+			goto fail;
+		}
+
+		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_cfg.drv_data = port;
+
+		psy = devm_power_supply_register_no_ws(dev, psy_desc,
+						       &psy_cfg);
+		if (IS_ERR(psy)) {
+			dev_err(dev, "Failed to register power supply\n");
+			continue;
+		}
+		port->psy = psy;
+
+		charger->ports[charger->num_registered_psy++] = port;
+		ec_device->charger = psy;
+	}
+
+	if (!charger->num_registered_psy) {
+		ret = -ENODEV;
+		dev_err(dev, "No power supplies registered\n");
+		goto fail;
+	}
+
+	if (ec_device->mkbp_event_supported) {
+		/* Get PD events from the EC */
+		charger->notifier.notifier_call = cros_usbpd_charger_ec_event;
+		ret = blocking_notifier_chain_register(
+						&ec_device->event_notifier,
+						&charger->notifier);
+		if (ret < 0)
+			dev_warn(dev, "failed to register notifier\n");
+	}
+
+	return 0;
+
+fail:
+	WARN(1, "%s: Failing probe (err:0x%x)\n", dev_name(dev), ret);
+
+fail_nowarn:
+	dev_info(dev, "Failing probe (err:0x%x)\n", ret);
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int cros_usbpd_charger_resume(struct device *dev)
+{
+	struct charger_data *charger = dev_get_drvdata(dev);
+	int i;
+
+	if (!charger)
+		return 0;
+
+	for (i = 0; i < charger->num_registered_psy; i++) {
+		power_supply_changed(charger->ports[i]->psy);
+		charger->ports[i]->last_update =
+				jiffies - CHARGER_CACHE_UPDATE_DELAY;
+	}
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(cros_usbpd_charger_pm_ops, NULL,
+			 cros_usbpd_charger_resume);
+
+static struct platform_driver cros_usbpd_charger_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.pm = &cros_usbpd_charger_pm_ops,
+	},
+	.probe = cros_usbpd_charger_probe
+};
+
+module_platform_driver(cros_usbpd_charger_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ChromeOS EC USBPD charger");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index f36125ee9245..a1eb39344231 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -19,6 +19,7 @@
 #include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/notifier.h>
+#include <linux/power_supply.h>
 #include <linux/mfd/cros_ec_commands.h>
 #include <linux/mutex.h>
 
@@ -143,6 +144,7 @@ struct cros_ec_device {
 			struct cros_ec_command *msg);
 	int (*pkt_xfer)(struct cros_ec_device *ec,
 			struct cros_ec_command *msg);
+	struct power_supply *charger;
 	struct mutex lock;
 	bool mkbp_event_supported;
 	struct blocking_notifier_head event_notifier;
-- 
2.17.0

[PATCH v2 3/3] mfd: cros_ec_dev: Register cros_usbpd-charger driver as a subdevice.

[Enric Balletbo i Serra]

Check whether this EC instance has USBPD host command support and
instatiate the cros_usbpd-charger driver as a subdevice in such case.

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

Changes in v2:
- [3/3] Use ARRAY_SIZE instead of hardcoded 1.

 drivers/mfd/cros_ec_dev.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 1d6dc5c7a19d..d06870da3a2e 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -387,6 +387,10 @@ static const struct mfd_cell cros_ec_rtc_cells[] = {
 	{ .name = "cros-ec-rtc" }
 };
 
+static const struct mfd_cell cros_usbpd_charger_cells[] = {
+	{ .name = "cros-usbpd-charger" }
+};
+
 static int ec_device_probe(struct platform_device *pdev)
 {
 	int retval = -ENOMEM;
@@ -438,6 +442,18 @@ static int ec_device_probe(struct platform_device *pdev)
 				retval);
 	}
 
+	/* Check whether this EC instance has the PD charge manager */
+	if (cros_ec_check_features(ec, EC_FEATURE_USB_PD)) {
+		retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
+					 cros_usbpd_charger_cells,
+					 ARRAY_SIZE(cros_usbpd_charger_cells),
+					 NULL, 0, NULL);
+		if (retval)
+			dev_err(ec->dev,
+				"failed to add cros-usbpd-charger device: %d\n",
+				retval);
+	}
+
 	/* Take control of the lightbar from the EC. */
 	lb_manual_suspend_ctrl(ec, 1);
 
-- 
2.17.0

Re: [PATCH v2 2/3] power: supply: add cros-ec USBPD charger driver.

[Miguel Ojeda]

On Mon, Apr 30, 2018 at 3:14 PM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> From: Sameer Nanda <snanda@chromium.org>
>
> This driver gets various bits of information about what is connected to
> USB PD ports from the EC and converts that into power_supply properties.
>
> Signed-off-by: Sameer Nanda <snanda@chromium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>
> Changes in v2:
> - [2/3] Add SPDX header, use devm_ variant and drop .owner
> - [2/3] Removed the PD log functionality (will be send on a follow up patch)
> - [2/3] Removed the extra custom sysfs attributes (will be send on a follow up patch)
>
>  drivers/power/supply/Kconfig              |  10 +
>  drivers/power/supply/Makefile             |   1 +
>  drivers/power/supply/cros_usbpd-charger.c | 511 ++++++++++++++++++++++
>  include/linux/mfd/cros_ec.h               |   2 +
>  4 files changed, 524 insertions(+)
>  create mode 100644 drivers/power/supply/cros_usbpd-charger.c
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 428b426842f4..046eb23ba6f0 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -624,4 +624,14 @@ config CHARGER_RT9455
>         help
>           Say Y to enable support for Richtek RT9455 battery charger.
>
> +config CHARGER_CROS_USBPD
> +       tristate "ChromeOS EC based USBPD charger"
> +       depends on MFD_CROS_EC
> +       default n
> +       help
> +         Say Y here to enable ChromeOS EC based USBPD charger
> +         driver. This driver gets various bits of information about
> +         what is connected to USB PD ports from the EC and converts
> +         that into power_supply properties.
> +
>  endif # POWER_SUPPLY
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index e83aa843bcc6..907e26f824b2 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -83,3 +83,4 @@ obj-$(CONFIG_CHARGER_TPS65090)        += tps65090-charger.o
>  obj-$(CONFIG_CHARGER_TPS65217) += tps65217_charger.o
>  obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
>  obj-$(CONFIG_AXP288_CHARGER)   += axp288_charger.o
> +obj-$(CONFIG_CHARGER_CROS_USBPD)       += cros_usbpd-charger.o
> diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
> new file mode 100644
> index 000000000000..c57d1c41828c
> --- /dev/null
> +++ b/drivers/power/supply/cros_usbpd-charger.c
> @@ -0,0 +1,511 @@
> +// SPDX-License-Identifier: GPL-2.0

If this is GPL-2.0...

> +/*
> + * Power supply driver for ChromeOS EC based USB PD Charger.
> + *
> + * Copyright (c) 2014 - 2018 Google, Inc
> + */
> +
> +
> +module_platform_driver(cros_usbpd_charger_driver);
> +
> +MODULE_LICENSE("GPL");

...then this should be "GPL v2":
https://elixir.bootlin.com/linux/v4.17-rc3/source/include/linux/module.h#L175

Cheers,
Miguel

Re: [PATCH v2 2/3] power: supply: add cros-ec USBPD charger driver.

[Enric Balletbo Serra]

Hi Miguel,

2018-04-30 15:20 GMT+02:00 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>:
> On Mon, Apr 30, 2018 at 3:14 PM, Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>> From: Sameer Nanda <snanda@chromium.org>
>>
>> This driver gets various bits of information about what is connected to
>> USB PD ports from the EC and converts that into power_supply properties.
>>
>> Signed-off-by: Sameer Nanda <snanda@chromium.org>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>> Changes in v2:
>> - [2/3] Add SPDX header, use devm_ variant and drop .owner
>> - [2/3] Removed the PD log functionality (will be send on a follow up patch)
>> - [2/3] Removed the extra custom sysfs attributes (will be send on a follow up patch)
>>
>>  drivers/power/supply/Kconfig              |  10 +
>>  drivers/power/supply/Makefile             |   1 +
>>  drivers/power/supply/cros_usbpd-charger.c | 511 ++++++++++++++++++++++
>>  include/linux/mfd/cros_ec.h               |   2 +
>>  4 files changed, 524 insertions(+)
>>  create mode 100644 drivers/power/supply/cros_usbpd-charger.c
>>
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index 428b426842f4..046eb23ba6f0 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -624,4 +624,14 @@ config CHARGER_RT9455
>>         help
>>           Say Y to enable support for Richtek RT9455 battery charger.
>>
>> +config CHARGER_CROS_USBPD
>> +       tristate "ChromeOS EC based USBPD charger"
>> +       depends on MFD_CROS_EC
>> +       default n
>> +       help
>> +         Say Y here to enable ChromeOS EC based USBPD charger
>> +         driver. This driver gets various bits of information about
>> +         what is connected to USB PD ports from the EC and converts
>> +         that into power_supply properties.
>> +
>>  endif # POWER_SUPPLY
>> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
>> index e83aa843bcc6..907e26f824b2 100644
>> --- a/drivers/power/supply/Makefile
>> +++ b/drivers/power/supply/Makefile
>> @@ -83,3 +83,4 @@ obj-$(CONFIG_CHARGER_TPS65090)        += tps65090-charger.o
>>  obj-$(CONFIG_CHARGER_TPS65217) += tps65217_charger.o
>>  obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
>>  obj-$(CONFIG_AXP288_CHARGER)   += axp288_charger.o
>> +obj-$(CONFIG_CHARGER_CROS_USBPD)       += cros_usbpd-charger.o
>> diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
>> new file mode 100644
>> index 000000000000..c57d1c41828c
>> --- /dev/null
>> +++ b/drivers/power/supply/cros_usbpd-charger.c
>> @@ -0,0 +1,511 @@
>> +// SPDX-License-Identifier: GPL-2.0
>
> If this is GPL-2.0...
>

Good catch, I think that this should be

SPDX-License-Identifier: GPL-2.0+

I will change in next version when I get more review.

>> +/*
>> + * Power supply driver for ChromeOS EC based USB PD Charger.
>> + *
>> + * Copyright (c) 2014 - 2018 Google, Inc
>> + */
>> +
>> +
>> +module_platform_driver(cros_usbpd_charger_driver);
>> +
>> +MODULE_LICENSE("GPL");
>
> ...then this should be "GPL v2":
> https://elixir.bootlin.com/linux/v4.17-rc3/source/include/linux/module.h#L175
>
> Cheers,
> Miguel

Thanks,
Enric

Re: [PATCH v2 1/3] mfd: cros_ec: Add USBPD charger commands and struct definitions.

[Lee Jones]

On Mon, 30 Apr 2018, Enric Balletbo i Serra wrote:

> From: Sameer Nanda <snanda@chromium.org>
> 
> The USBPD charger driver gets information from the ChromeOS EC, this
> patch adds the USBPD charger definitions needed by this driver.
> 
> Signed-off-by: Sameer Nanda <snanda@chromium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
> Changes in v2: None
> 
>  include/linux/mfd/cros_ec_commands.h | 132 ++++++++++++++++++++++++++-
>  1 file changed, 128 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> index f2edd9969b40..94dcf331796c 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h

[...]

> +struct ec_response_pd_log {
> +	uint32_t timestamp; /* relative timestamp in milliseconds */
> +	uint8_t type;       /* event type : see PD_EVENT_xx below */
> +	uint8_t size_port;  /* [7:5] port number [4:0] payload size in bytes */
> +	uint16_t data;      /* type-defined data payload */
> +	uint8_t payload[0]; /* optional additional data payload: 0..16 bytes */
> +} __packed;

I think this whole file should be converted to Kerneldoc format.

[...]

> +/*
> + * PD_EVENT_MCU_CHARGE event definition :
> + * the payload is "struct usb_chg_measures"
> + * the data field contains the port state flags as defined below :
> + */
> +/* Port partner is a dual role device */
> +#define CHARGE_FLAGS_DUAL_ROLE         (1 << 15)

BIT()?

> +/* Port is the pending override port */
> +#define CHARGE_FLAGS_DELAYED_OVERRIDE  (1 << 14)
> +/* Port is the override port */
> +#define CHARGE_FLAGS_OVERRIDE          (1 << 13)

[...]

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

Re: [PATCH v2 2/3] power: supply: add cros-ec USBPD charger driver.

[Lee Jones]

On Mon, 30 Apr 2018, Enric Balletbo i Serra wrote:

> From: Sameer Nanda <snanda@chromium.org>
> 
> This driver gets various bits of information about what is connected to
> USB PD ports from the EC and converts that into power_supply properties.
> 
> Signed-off-by: Sameer Nanda <snanda@chromium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
> Changes in v2:
> - [2/3] Add SPDX header, use devm_ variant and drop .owner
> - [2/3] Removed the PD log functionality (will be send on a follow up patch)
> - [2/3] Removed the extra custom sysfs attributes (will be send on a follow up patch)
> 
>  drivers/power/supply/Kconfig              |  10 +
>  drivers/power/supply/Makefile             |   1 +
>  drivers/power/supply/cros_usbpd-charger.c | 511 ++++++++++++++++++++++

>  include/linux/mfd/cros_ec.h               |   2 +

Acked-by: Lee Jones <lee.jones@linaro.org>

>  4 files changed, 524 insertions(+)
>  create mode 100644 drivers/power/supply/cros_usbpd-charger.c

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

Re: [PATCH v2 3/3] mfd: cros_ec_dev: Register cros_usbpd-charger driver as a subdevice.

[Lee Jones]

On Mon, 30 Apr 2018, Enric Balletbo i Serra wrote:

> Check whether this EC instance has USBPD host command support and
> instatiate the cros_usbpd-charger driver as a subdevice in such case.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
> Changes in v2:
> - [3/3] Use ARRAY_SIZE instead of hardcoded 1.
> 
>  drivers/mfd/cros_ec_dev.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)

Patch looks good to me.

Does it depend on any of the other patches in the series?

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 v2 2/3] power: supply: add cros-ec USBPD charger driver.

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 19924 bytes --]

Hi Enric,

On Mon, Apr 30, 2018 at 03:14:02PM +0200, Enric Balletbo i Serra wrote:
> From: Sameer Nanda <snanda@chromium.org>
> 
> This driver gets various bits of information about what is connected to
> USB PD ports from the EC and converts that into power_supply properties.
> 
> Signed-off-by: Sameer Nanda <snanda@chromium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
> Changes in v2:
> - [2/3] Add SPDX header, use devm_ variant and drop .owner
> - [2/3] Removed the PD log functionality (will be send on a follow up patch)
> - [2/3] Removed the extra custom sysfs attributes (will be send on a follow up patch)

Thanks, it makes reviewing the patchset easier.

>  drivers/power/supply/Kconfig              |  10 +
>  drivers/power/supply/Makefile             |   1 +
>  drivers/power/supply/cros_usbpd-charger.c | 511 ++++++++++++++++++++++
>  include/linux/mfd/cros_ec.h               |   2 +
>  4 files changed, 524 insertions(+)
>  create mode 100644 drivers/power/supply/cros_usbpd-charger.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 428b426842f4..046eb23ba6f0 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -624,4 +624,14 @@ config CHARGER_RT9455
>  	help
>  	  Say Y to enable support for Richtek RT9455 battery charger.
>  
> +config CHARGER_CROS_USBPD
> +	tristate "ChromeOS EC based USBPD charger"
> +	depends on MFD_CROS_EC
> +	default n
> +	help
> +	  Say Y here to enable ChromeOS EC based USBPD charger
> +	  driver. This driver gets various bits of information about
> +	  what is connected to USB PD ports from the EC and converts
> +	  that into power_supply properties.
> +
>  endif # POWER_SUPPLY
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index e83aa843bcc6..907e26f824b2 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -83,3 +83,4 @@ obj-$(CONFIG_CHARGER_TPS65090)	+= tps65090-charger.o
>  obj-$(CONFIG_CHARGER_TPS65217)	+= tps65217_charger.o
>  obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
>  obj-$(CONFIG_AXP288_CHARGER)	+= axp288_charger.o
> +obj-$(CONFIG_CHARGER_CROS_USBPD)	+= cros_usbpd-charger.o
> diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
> new file mode 100644
> index 000000000000..c57d1c41828c
> --- /dev/null
> +++ b/drivers/power/supply/cros_usbpd-charger.c
> @@ -0,0 +1,511 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Power supply driver for ChromeOS EC based USB PD Charger.
> + *
> + * Copyright (c) 2014 - 2018 Google, Inc
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/platform_device.h>
> +#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_CACHE_UPDATE_DELAY		msecs_to_jiffies(500)
> +#define CHARGER_MANUFACTURER_MODEL_LENGTH	32
> +
> +#define DRV_NAME "cros-usbpd-charger"
> +
> +struct port_data {
> +	int port_number;
> +	char name[CHARGER_DIR_NAME_LENGTH];
> +	char manufacturer[CHARGER_MANUFACTURER_MODEL_LENGTH];
> +	char model_name[CHARGER_MANUFACTURER_MODEL_LENGTH];
> +	struct power_supply *psy;
> +	struct power_supply_desc psy_desc;
> +	int psy_usb_type;
> +	int psy_online;
> +	int psy_status;
> +	int psy_current_max;
> +	int psy_voltage_max_design;
> +	int psy_voltage_now;
> +	int psy_power_max;
> +	struct charger_data *charger;
> +	unsigned long last_update;
> +};
> +
> +struct charger_data {
> +	struct device *dev;
> +	struct cros_ec_dev *ec_dev;
> +	struct cros_ec_device *ec_device;
> +	int num_charger_ports;
> +	int num_registered_psy;
> +	struct port_data *ports[EC_USB_PD_MAX_PORTS];
> +	struct notifier_block notifier;
> +};
> +
> +static enum power_supply_property cros_usbpd_charger_props[] = {
> +	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_CURRENT_MAX,
> +	POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_MODEL_NAME,
> +	POWER_SUPPLY_PROP_MANUFACTURER,
> +};
> +
> +static int cros_usbpd_charger_ec_command(struct charger_data *charger,
> +					 unsigned int version,
> +					 unsigned int command,
> +					 void *outdata,
> +					 unsigned int outsize,
> +					 void *indata,
> +					 unsigned int insize)
> +{
> +	struct cros_ec_dev *ec_dev = charger->ec_dev;
> +	struct cros_ec_command *msg;
> +	int ret;
> +
> +	msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	msg->version = version;
> +	msg->command = ec_dev->cmd_offset + command;
> +	msg->outsize = outsize;
> +	msg->insize = insize;
> +
> +	if (outsize)
> +		memcpy(msg->data, outdata, outsize);
> +
> +	ret = cros_ec_cmd_xfer_status(charger->ec_device, msg);
> +	if (ret >= 0 && insize)
> +		memcpy(indata, msg->data, insize);
> +
> +	kfree(msg);
> +	return ret;
> +}
> +
> +static int cros_usbpd_charger_get_num_ports(struct charger_data *charger)
> +{
> +	struct ec_response_usb_pd_ports resp;
> +	int ret;
> +
> +	ret = cros_usbpd_charger_ec_command(charger, 0, EC_CMD_USB_PD_PORTS,
> +					    NULL, 0, &resp, sizeof(resp));
> +	if (ret < 0) {
> +		dev_err(charger->dev,
> +			"Unable to get the number or ports (err:0x%x)\n", ret);
> +		return ret;
> +	}
> +
> +	return resp.num_ports;
> +}
> +
> +static int cros_usbpd_charger_get_discovery_info(struct port_data *port)
> +{
> +	struct charger_data *charger = port->charger;
> +	struct ec_params_usb_pd_discovery_entry resp;
> +	struct ec_params_usb_pd_info_request req;
> +	int ret;
> +
> +	req.port = port->port_number;
> +
> +	ret = cros_usbpd_charger_ec_command(charger, 0,
> +					    EC_CMD_USB_PD_DISCOVERY,
> +					    &req, sizeof(req),
> +					    &resp, sizeof(resp));
> +	if (ret < 0) {
> +		dev_err(charger->dev,
> +			"Unable to query discovery info (err:0x%x)\n", ret);
> +		return ret;
> +	}
> +
> +	dev_dbg(charger->dev, "Port %d: VID = 0x%x, PID=0x%x, PTYPE=0x%x\n",
> +		port->port_number, resp.vid, resp.pid, resp.ptype);
> +
> +	snprintf(port->manufacturer, sizeof(port->manufacturer), "%x",
> +		 resp.vid);
> +	snprintf(port->model_name, sizeof(port->model_name), "%x", resp.pid);
> +
> +	return 0;
> +}
> +
> +static int cros_usbpd_charger_get_power_info(struct port_data *port)
> +{
> +	struct charger_data *charger = port->charger;
> +	struct ec_response_usb_pd_power_info resp;
> +	struct ec_params_usb_pd_power_info req;
> +	int last_psy_status, last_psy_usb_type;
> +	struct device *dev = charger->dev;
> +	int ret;
> +
> +	req.port = port->port_number;
> +	ret = cros_usbpd_charger_ec_command(charger, 0,
> +					    EC_CMD_USB_PD_POWER_INFO,
> +					    &req, sizeof(req),
> +					    &resp, sizeof(resp));
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to query PD power info (err:0x%x)\n", ret);
> +		return ret;
> +	}
> +
> +	last_psy_status = port->psy_status;
> +	last_psy_usb_type = port->psy_usb_type;
> +
> +	switch (resp.role) {
> +	case USB_PD_PORT_POWER_DISCONNECTED:
> +		port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		port->psy_online = 0;
> +		break;
> +	case USB_PD_PORT_POWER_SOURCE:
> +		port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		port->psy_online = 0;
> +		break;
> +	case USB_PD_PORT_POWER_SINK:
> +		port->psy_status = POWER_SUPPLY_STATUS_CHARGING;
> +		port->psy_online = 1;
> +		break;
> +	case USB_PD_PORT_POWER_SINK_NOT_CHARGING:
> +		port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +		port->psy_online = 1;
> +		break;
> +	default:
> +		dev_err(dev, "Unknown role %d\n", resp.role);
> +		break;
> +	}
> +
> +	port->psy_voltage_max_design = resp.meas.voltage_max;
> +	port->psy_voltage_now = resp.meas.voltage_now;
> +	port->psy_current_max = resp.meas.current_max;
> +	port->psy_power_max = resp.max_power;
> +
> +	switch (resp.type) {
> +	case USB_CHG_TYPE_BC12_SDP:
> +	case USB_CHG_TYPE_VBUS:
> +		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_SDP;
> +		break;
> +	case USB_CHG_TYPE_NONE:
> +		/*
> +		 * For dual-role devices when we are a source, the firmware
> +		 * reports the type as NONE. Report such chargers as type
> +		 * USB_PD_DRP.
> +		 */
> +		if (resp.role == USB_PD_PORT_POWER_SOURCE && resp.dualrole)
> +			port->psy_usb_type = POWER_SUPPLY_USB_TYPE_PD_DRP;
> +		else
> +			port->psy_usb_type = POWER_SUPPLY_USB_TYPE_SDP;
> +		break;
> +	case USB_CHG_TYPE_OTHER:
> +	case USB_CHG_TYPE_PROPRIETARY:
> +		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_APPLE_BRICK_ID;
> +		break;
> +	case USB_CHG_TYPE_C:
> +		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_C;
> +		break;
> +	case USB_CHG_TYPE_BC12_DCP:
> +		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_DCP;
> +		break;
> +	case USB_CHG_TYPE_BC12_CDP:
> +		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_CDP;
> +		break;
> +	case USB_CHG_TYPE_PD:
> +		if (resp.dualrole)
> +			port->psy_usb_type = POWER_SUPPLY_USB_TYPE_PD_DRP;
> +		else
> +			port->psy_usb_type = POWER_SUPPLY_USB_TYPE_PD;
> +		break;
> +	case USB_CHG_TYPE_UNKNOWN:
> +		/*
> +		 * While the EC is trying to determine the type of charger that
> +		 * has been plugged in, it will report the charger type as
> +		 * unknown. Additionally since the power capabilities are
> +		 * unknown, report the max current and voltage as zero.
> +		 */
> +		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_UNKNOWN;
> +		port->psy_voltage_max_design = 0;
> +		port->psy_current_max = 0;
> +		break;
> +	default:
> +		dev_err(dev, "Port %d: default case!\n", port->port_number);
> +		port->psy_usb_type = POWER_SUPPLY_USB_TYPE_SDP;
> +	}
> +
> +	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",
> +		port->port_number, resp.type, resp.meas.voltage_max,
> +		resp.meas.voltage_now, resp.meas.current_max,
> +		resp.meas.current_lim, resp.max_power);
> +
> +	/*
> +	 * If power supply type or status changed, explicitly call
> +	 * power_supply_changed. This results in udev event getting generated
> +	 * and allows user mode apps to react quicker instead of waiting for
> +	 * their next poll of power supply status.
> +	 */
> +	if (last_psy_usb_type != port->psy_usb_type ||
> +	    last_psy_status != port->psy_status)
> +		power_supply_changed(port->psy);
> +
> +	return 0;
> +}
> +
> +static int cros_usbpd_charger_get_port_status(struct port_data *port,
> +					      bool ratelimit)
> +{
> +	int ret;
> +
> +	if (ratelimit &&
> +	    time_is_after_jiffies(port->last_update +
> +				  CHARGER_CACHE_UPDATE_DELAY))
> +		return 0;
> +
> +	ret = cros_usbpd_charger_get_power_info(port);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = cros_usbpd_charger_get_discovery_info(port);
> +	port->last_update = jiffies;
> +
> +	return ret;
> +}
> +
> +static void cros_usbpd_charger_power_changed(struct power_supply *psy)
> +{
> +	struct port_data *port = power_supply_get_drvdata(psy);
> +	struct charger_data *charger = port->charger;
> +	int i;
> +
> +	for (i = 0; i < charger->num_registered_psy; i++)
> +		cros_usbpd_charger_get_port_status(charger->ports[i], false);
> +}
> +
> +static int cros_usbpd_charger_get_prop(struct power_supply *psy,
> +				       enum power_supply_property psp,
> +				       union power_supply_propval *val)
> +{
> +	struct port_data *port = power_supply_get_drvdata(psy);
> +	struct charger_data *charger = port->charger;
> +	struct cros_ec_device *ec_device = charger->ec_device;
> +	struct device *dev = charger->dev;
> +	int ret;
> +
> +	/* Only refresh ec_port_status for dynamic properties */
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		/*
> +		 * If mkbp_event_supported, then we can be assured that
> +		 * the driver's state for the online property is consistent
> +		 * with the hardware. However, if we aren't event driven,
> +		 * the optimization before to skip an ec_port_status get
> +		 * and only returned cached values of the online property will
> +		 * cause a delay in detecting a cable attach until one of the
> +		 * other properties are read.
> +		 *
> +		 * Allow an ec_port_status refresh for online property check
> +		 * if we're not already online to check for plug events if
> +		 * not mkbp_event_supported.
> +		 */
> +		if (ec_device->mkbp_event_supported || port->psy_online)
> +			break;
> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
> +	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		ret = cros_usbpd_charger_get_port_status(port, true);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to get port status (err:0x%x)\n",
> +				ret);
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		val->intval = port->psy_online;
> +		break;
> +	case POWER_SUPPLY_PROP_STATUS:
> +		val->intval = port->psy_status;
> +		break;
> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
> +		val->intval = port->psy_current_max * 1000;
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> +		val->intval = port->psy_voltage_max_design * 1000;
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		val->intval = port->psy_voltage_now * 1000;
> +		break;
> +	case POWER_SUPPLY_PROP_MODEL_NAME:
> +		val->strval = port->model_name;
> +		break;
> +	case POWER_SUPPLY_PROP_MANUFACTURER:
> +		val->strval = port->manufacturer;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

Missing POWER_SUPPLY_PROP_USB_TYPE, don't throw away your work from
cros_usbpd_charger_get_power_info() :)

> +
> +	return 0;
> +}
> +
> +static int cros_usbpd_charger_ec_event(struct notifier_block *nb,
> +				       unsigned long queued_during_suspend,
> +				       void *_notify)
> +{
> +	struct cros_ec_device *ec_device;
> +	struct charger_data *charger;
> +	struct device *dev;
> +	u32 host_event;
> +
> +	charger = container_of(nb, struct charger_data, notifier);
> +	ec_device = charger->ec_device;
> +	dev = charger->dev;
> +
> +	host_event = cros_ec_get_host_event(ec_device);
> +	if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
> +		cros_usbpd_charger_power_changed(charger->ports[0]->psy);
> +		return NOTIFY_OK;
> +	} else {
> +		return NOTIFY_DONE;
> +	}
> +}
> +
> +static int cros_usbpd_charger_probe(struct platform_device *pd)
> +{
> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
> +	struct cros_ec_device *ec_device = ec_dev->ec_dev;
> +	struct power_supply_desc *psy_desc;
> +	struct device *dev = &pd->dev;
> +	struct charger_data *charger;
> +	struct power_supply *psy;
> +	struct port_data *port;
> +	int ret = -EINVAL;
> +	int i;
> +
> +	charger = devm_kzalloc(dev, sizeof(struct charger_data),
> +			       GFP_KERNEL);
> +	if (!charger)
> +		return -ENOMEM;
> +
> +	charger->dev = dev;
> +	charger->ec_dev = ec_dev;
> +	charger->ec_device = ec_device;
> +
> +	platform_set_drvdata(pd, charger);
> +
> +	charger->num_charger_ports = cros_usbpd_charger_get_num_ports(charger);
> +	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.
> +		 */
> +		dev_info(dev, "No charging ports found\n");
> +		ret = -ENODEV;
> +		goto fail_nowarn;
> +	}
> +
> +	for (i = 0; i < charger->num_charger_ports; i++) {
> +		struct power_supply_config psy_cfg = {};
> +
> +		port = devm_kzalloc(dev, sizeof(struct port_data), GFP_KERNEL);
> +		if (!port) {
> +			ret = -ENOMEM;
> +			goto fail;
> +		}
> +
> +		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);

You also need to add psy_desc->usb_types and psy_desc->num_usb_types
for POWER_SUPPLY_PROP_USB_TYPE. It should contains a list of
supported USB types.

> +		psy_cfg.drv_data = port;
> +
> +		psy = devm_power_supply_register_no_ws(dev, psy_desc,
> +						       &psy_cfg);
> +		if (IS_ERR(psy)) {
> +			dev_err(dev, "Failed to register power supply\n");
> +			continue;
> +		}
> +		port->psy = psy;
> +
> +		charger->ports[charger->num_registered_psy++] = port;
> +		ec_device->charger = psy;
> +	}
> +
> +	if (!charger->num_registered_psy) {
> +		ret = -ENODEV;
> +		dev_err(dev, "No power supplies registered\n");
> +		goto fail;
> +	}
> +
> +	if (ec_device->mkbp_event_supported) {
> +		/* Get PD events from the EC */
> +		charger->notifier.notifier_call = cros_usbpd_charger_ec_event;
> +		ret = blocking_notifier_chain_register(
> +						&ec_device->event_notifier,
> +						&charger->notifier);
> +		if (ret < 0)
> +			dev_warn(dev, "failed to register notifier\n");

I don't see this being unregistered on removal? I suggest (untested):

ret = devm_add_action_or_reset(dev,
    cros_usbpd_charger_unregister_notifier, charger);
if (ret < 0)
    return ret;

static void cros_usbpd_charger_unregister_notifier(void *data)
{
    struct charger_data *charger = data;
    struct cros_ec_device *ec_device = charger->ec_device;

    blocking_notifier_chain_unregister(&ec_device->event_notifier,
                                       &charger->notifier);
}

> +	}
> +
> +	return 0;
> +
> +fail:
> +	WARN(1, "%s: Failing probe (err:0x%x)\n", dev_name(dev), ret);
> +
> +fail_nowarn:
> +	dev_info(dev, "Failing probe (err:0x%x)\n", ret);
> +	return ret;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int cros_usbpd_charger_resume(struct device *dev)
> +{
> +	struct charger_data *charger = dev_get_drvdata(dev);
> +	int i;
> +
> +	if (!charger)
> +		return 0;
> +
> +	for (i = 0; i < charger->num_registered_psy; i++) {
> +		power_supply_changed(charger->ports[i]->psy);
> +		charger->ports[i]->last_update =
> +				jiffies - CHARGER_CACHE_UPDATE_DELAY;
> +	}
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(cros_usbpd_charger_pm_ops, NULL,
> +			 cros_usbpd_charger_resume);
> +
> +static struct platform_driver cros_usbpd_charger_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.pm = &cros_usbpd_charger_pm_ops,
> +	},
> +	.probe = cros_usbpd_charger_probe
> +};
> +
> +module_platform_driver(cros_usbpd_charger_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("ChromeOS EC USBPD charger");
> +MODULE_ALIAS("platform:" DRV_NAME);
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index f36125ee9245..a1eb39344231 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -19,6 +19,7 @@
>  #include <linux/cdev.h>
>  #include <linux/device.h>
>  #include <linux/notifier.h>
> +#include <linux/power_supply.h>
>  #include <linux/mfd/cros_ec_commands.h>
>  #include <linux/mutex.h>
>  
> @@ -143,6 +144,7 @@ struct cros_ec_device {
>  			struct cros_ec_command *msg);
>  	int (*pkt_xfer)(struct cros_ec_device *ec,
>  			struct cros_ec_command *msg);
> +	struct power_supply *charger;
>  	struct mutex lock;
>  	bool mkbp_event_supported;
>  	struct blocking_notifier_head event_notifier;
> -- 

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/3] power: supply: add cros-ec USBPD charger driver.

[Lee Jones]

On Tue, 01 May 2018, Sebastian Reichel wrote:

> Hi Enric,
> 
> On Mon, Apr 30, 2018 at 03:14:02PM +0200, Enric Balletbo i Serra wrote:
> > From: Sameer Nanda <snanda@chromium.org>
> > 
> > This driver gets various bits of information about what is connected to
> > USB PD ports from the EC and converts that into power_supply properties.
> > 
> > Signed-off-by: Sameer Nanda <snanda@chromium.org>
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > ---
> > 
> > Changes in v2:
> > - [2/3] Add SPDX header, use devm_ variant and drop .owner
> > - [2/3] Removed the PD log functionality (will be send on a follow up patch)
> > - [2/3] Removed the extra custom sysfs attributes (will be send on a follow up patch)
> 
> Thanks, it makes reviewing the patchset easier.
> 
> >  drivers/power/supply/Kconfig              |  10 +
> >  drivers/power/supply/Makefile             |   1 +
> >  drivers/power/supply/cros_usbpd-charger.c | 511 ++++++++++++++++++++++
> >  include/linux/mfd/cros_ec.h               |   2 +
> >  4 files changed, 524 insertions(+)
> >  create mode 100644 drivers/power/supply/cros_usbpd-charger.c

[...]

Sebastian,

When you review a patch, would you mind chopping/snipping unnecessary
text please?  The chunks in-between the ones you reply to.

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

Re: [PATCH v2 1/3] mfd: cros_ec: Add USBPD charger commands and struct definitions.

[Enric Balletbo i Serra]

Hi Lee,

Thanks for the reviews.

On 01/05/18 10:29, Lee Jones wrote:
> On Mon, 30 Apr 2018, Enric Balletbo i Serra wrote:
> 
>> From: Sameer Nanda <snanda@chromium.org>
>>
>> The USBPD charger driver gets information from the ChromeOS EC, this
>> patch adds the USBPD charger definitions needed by this driver.
>>
>> Signed-off-by: Sameer Nanda <snanda@chromium.org>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>> Changes in v2: None
>>
>>  include/linux/mfd/cros_ec_commands.h | 132 ++++++++++++++++++++++++++-
>>  1 file changed, 128 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
>> index f2edd9969b40..94dcf331796c 100644
>> --- a/include/linux/mfd/cros_ec_commands.h
>> +++ b/include/linux/mfd/cros_ec_commands.h
> 
> [...]
> 
>> +struct ec_response_pd_log {
>> +	uint32_t timestamp; /* relative timestamp in milliseconds */
>> +	uint8_t type;       /* event type : see PD_EVENT_xx below */
>> +	uint8_t size_port;  /* [7:5] port number [4:0] payload size in bytes */
>> +	uint16_t data;      /* type-defined data payload */
>> +	uint8_t payload[0]; /* optional additional data payload: 0..16 bytes */
>> +} __packed;
> 
> I think this whole file should be converted to Kerneldoc format.
> 

Ok, is something I can do, let me do this in a separate patchset though.

> [...]
> 
>> +/*
>> + * PD_EVENT_MCU_CHARGE event definition :
>> + * the payload is "struct usb_chg_measures"
>> + * the data field contains the port state flags as defined below :
>> + */
>> +/* Port partner is a dual role device */
>> +#define CHARGE_FLAGS_DUAL_ROLE         (1 << 15)
> 
> BIT()?
> 

Changed in next version, there are also other places (not in this patch) where
the BIT() macro is not used. I'll send separate patchset to change it and to
convert to kerneldoc format.

>> +/* Port is the pending override port */
>> +#define CHARGE_FLAGS_DELAYED_OVERRIDE  (1 << 14)
>> +/* Port is the override port */
>> +#define CHARGE_FLAGS_OVERRIDE          (1 << 13)
> 
> [...]
> 

Re: [PATCH v2 3/3] mfd: cros_ec_dev: Register cros_usbpd-charger driver as a subdevice.

[Enric Balletbo i Serra]

Hi Lee,

On 01/05/18 10:32, Lee Jones wrote:
> On Mon, 30 Apr 2018, Enric Balletbo i Serra wrote:
> 
>> Check whether this EC instance has USBPD host command support and
>> instatiate the cros_usbpd-charger driver as a subdevice in such case.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>> Changes in v2:
>> - [3/3] Use ARRAY_SIZE instead of hardcoded 1.
>>
>>  drivers/mfd/cros_ec_dev.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
> 
> Patch looks good to me.
> 
> Does it depend on any of the other patches in the series?
> 

No, it only depends on [1] to apply cleanly (mentioned in the cover letter). I
think that [1] is close to be picked. If this is not the case I can rebase
without this dependency.

[1] https://lkml.org/lkml/2018/4/18/229

Best regards,
 Enric

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

Re: [PATCH v2 1/3] mfd: cros_ec: Add USBPD charger commands and struct definitions.

[Lee Jones]

> >> From: Sameer Nanda <snanda@chromium.org>
> >>
> >> The USBPD charger driver gets information from the ChromeOS EC, this
> >> patch adds the USBPD charger definitions needed by this driver.
> >>
> >> Signed-off-by: Sameer Nanda <snanda@chromium.org>
> >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> ---
> >>
> >> Changes in v2: None
> >>
> >>  include/linux/mfd/cros_ec_commands.h | 132 ++++++++++++++++++++++++++-
> >>  1 file changed, 128 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> >> index f2edd9969b40..94dcf331796c 100644
> >> --- a/include/linux/mfd/cros_ec_commands.h
> >> +++ b/include/linux/mfd/cros_ec_commands.h
> > 
> > [...]
> > 
> >> +struct ec_response_pd_log {
> >> +	uint32_t timestamp; /* relative timestamp in milliseconds */
> >> +	uint8_t type;       /* event type : see PD_EVENT_xx below */
> >> +	uint8_t size_port;  /* [7:5] port number [4:0] payload size in bytes */
> >> +	uint16_t data;      /* type-defined data payload */
> >> +	uint8_t payload[0]; /* optional additional data payload: 0..16 bytes */
> >> +} __packed;
> > 
> > I think this whole file should be converted to Kerneldoc format.
> 
> Ok, is something I can do, let me do this in a separate patchset though.

Yes, that's fine.

> >> +/*
> >> + * PD_EVENT_MCU_CHARGE event definition :
> >> + * the payload is "struct usb_chg_measures"
> >> + * the data field contains the port state flags as defined below :
> >> + */
> >> +/* Port partner is a dual role device */
> >> +#define CHARGE_FLAGS_DUAL_ROLE         (1 << 15)
> > 
> > BIT()?
> > 
> 
> Changed in next version, there are also other places (not in this patch) where
> the BIT() macro is not used. I'll send separate patchset to change it and to
> convert to kerneldoc format.

Also fine.

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