[PATCH v8 0/7] EC-based USB Power Delivery support for Chrome machines

[PATCH v8 0/7] EC-based USB Power Delivery support for Chrome machines

[Tomeu Vizoso]

Hi,

this series contains a driver that exposes a power_supply to userspace
representing a port that support USB PD charging.

Allows userspace to display to the user if the machine is charging and
on which port, and if another device is being charged by a port.

This series may be best integrated through the MFD tree, for which we'll
need ACKs from Olof (for the changes to platform/chrome), from Dmitry
(for changes to cros_ec_keyb.c) and from Sebastian Reichel for the
new driver.

Patches 1 and 2 need to be taken together because in the first the MFD
driver handles the EC interrupt and only in the second patch the
keyboard driver stops handling it and using the notifier instead.

Thanks,

Tomeu

Changes in v8:
- Split out changes to drivers/input/keyboard
- Improved error messages
- Fixed kerneldoc comments
- Don't call cros_ec_cmd_xfer from declaration block
- Remove unnecessary variable host_event
- Don't add stuff to cros_ec_commands.h not needed in this series

Changes in v7:
- Rebased onto 4.6-rc1, with no conflicts.

Changes in v6:
- Return 0 if the EC doesn't support MKBP, as expected by callers.

Changes in v5:
- Check explicitly for !EC_RES_SUCCESS as suggested by Benson Leung.
- Fix type of variable passed to do_div.

Changes in v4:
- Calculate correctly the size of the payloads in
  cros_ec_get_host_command_version_mask.
- Declare size parameters in ec_command as size_t

Changes in v3:
- Remove duplicated prototype of cros_ec_get_host_event.
- Use do_div so it builds on 32bit (suggested by 0-day kbuild bot).
- Remove sysfs attributes ext_current_lim and ext_voltage_lim because
  I don't know yet where they should be placed (and don't have HW to
  test them).
- Remove superfluous pre-allocated buffers ec_inbuf and ec_outbuf.
- Lots of misc comments from Stephen Boyd were addressed.
- Unregister notification listener in .remove callback.
- Only register the PD charger device if there are any PD ports in this
  EC.
- Dropped patch using EC_CMD_GET_FEATURES to decide whether to create a
  charger device because we are now only creating one if a EC has any PD
  ports.
- Dropped patch adding power_supply types because it has been merged
  already.

Changes in v2:
- Allocate enough for the structs in
  cros_ec_get_host_command_version_mask,
  not their pointers.
- Allocate msg in the stack in get_next_event and
  get_keyboard_state_event, as suggested by Gwendal.
- Split out changes to cros_ec_commands.h and the helpers added to
  mfd/cros_ec.h from the patch that adds the charger driver, as
  suggested by Lee.
- Actually call get_ec_num_ports.
- Move cros_ec_usb_pd_charger_register into cros_ec_dev.c.

Sameer Nanda (1):
  power: cros_usbpd-charger: Add EC-based USB PD charger driver

Tomeu Vizoso (4):
  mfd: cros_ec: Add cros_ec_cmd_xfer_status helper
  mfd: cros_ec: Add cros_ec_get_host_event
  mfd: cros_ec: Add more definitions for PD commands
  platform/chrome: Register USB PD charger device

Vic Yang (2):
  mfd: cros_ec: Add MKBP event support
  Input: cros_ec_keyb - Stop handling interrupts directly

 drivers/input/keyboard/cros_ec_keyb.c   | 135 ++----
 drivers/mfd/cros_ec.c                   |  58 ++-
 drivers/platform/chrome/cros_ec_dev.c   |  44 ++
 drivers/platform/chrome/cros_ec_proto.c | 127 ++++++
 drivers/power/Kconfig                   |  10 +
 drivers/power/Makefile                  |   1 +
 drivers/power/cros_usbpd-charger.c      | 780 ++++++++++++++++++++++++++++++++
 include/linux/mfd/cros_ec.h             |  45 ++
 include/linux/mfd/cros_ec_commands.h    | 216 +++++++++
 9 files changed, 1309 insertions(+), 107 deletions(-)
 create mode 100644 drivers/power/cros_usbpd-charger.c

-- 
2.5.5

[PATCH v8 1/7] mfd: cros_ec: Add MKBP event support

[Tomeu Vizoso]

From: Vic Yang <victoryang@google.com>

Newer revisions of the ChromeOS EC add more events besides the keyboard
ones. So handle interrupts in the MFD driver and let consumers register
for notifications for the events they might care.

To keep backward compatibility, if the EC doesn't support MKBP event, we
fall back to the old MKBP key matrix host command.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Randall Spangler <rspangler@chromium.org>
Cc: Vincent Palatin <vpalatin@chromium.org>
Cc: Benson Leung <bleung@chromium.org>

---

Changes in v8:
- Split out changes to drivers/input/keyboard
- Improved error messages
- Fixed kerneldoc comments

Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4:
- Calculate correctly the size of the payloads in
  cros_ec_get_host_command_version_mask.

Changes in v3:
- Remove duplicated prototype of cros_ec_get_host_event.

Changes in v2:
- Allocate enough for the structs in
  cros_ec_get_host_command_version_mask,
  not their pointers.
- Allocate msg in the stack in get_next_event and
  get_keyboard_state_event, as suggested by Gwendal.

 drivers/mfd/cros_ec.c                   | 58 +++++++++++++++++++--
 drivers/platform/chrome/cros_ec_proto.c | 92 +++++++++++++++++++++++++++++++++
 include/linux/mfd/cros_ec.h             | 18 +++++++
 include/linux/mfd/cros_ec_commands.h    | 34 ++++++++++++
 4 files changed, 199 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
index 0eee63542038..abd83424b498 100644
--- a/drivers/mfd/cros_ec.c
+++ b/drivers/mfd/cros_ec.c
@@ -23,6 +23,7 @@
 #include <linux/module.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/cros_ec.h>
+#include <asm/unaligned.h>
 
 #define CROS_EC_DEV_EC_INDEX 0
 #define CROS_EC_DEV_PD_INDEX 1
@@ -49,11 +50,28 @@ static const struct mfd_cell ec_pd_cell = {
 	.pdata_size = sizeof(pd_p),
 };
 
+static irqreturn_t ec_irq_thread(int irq, void *data)
+{
+	struct cros_ec_device *ec_dev = data;
+	int ret;
+
+	if (device_may_wakeup(ec_dev->dev))
+		pm_wakeup_event(ec_dev->dev, 0);
+
+	ret = cros_ec_get_next_event(ec_dev);
+	if (ret > 0)
+		blocking_notifier_call_chain(&ec_dev->event_notifier,
+					     0, ec_dev);
+	return IRQ_HANDLED;
+}
+
 int cros_ec_register(struct cros_ec_device *ec_dev)
 {
 	struct device *dev = ec_dev->dev;
 	int err = 0;
 
+	BLOCKING_INIT_NOTIFIER_HEAD(&ec_dev->event_notifier);
+
 	ec_dev->max_request = sizeof(struct ec_params_hello);
 	ec_dev->max_response = sizeof(struct ec_response_get_protocol_info);
 	ec_dev->max_passthru = 0;
@@ -70,13 +88,24 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 
 	cros_ec_query_all(ec_dev);
 
+	if (ec_dev->irq) {
+		err = request_threaded_irq(ec_dev->irq, NULL, ec_irq_thread,
+					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+					   "chromeos-ec", ec_dev);
+		if (err) {
+			dev_err(dev, "Failed to request IRQ %d: %d",
+				ec_dev->irq, err);
+			return err;
+		}
+	}
+
 	err = mfd_add_devices(ec_dev->dev, PLATFORM_DEVID_AUTO, &ec_cell, 1,
 			      NULL, ec_dev->irq, NULL);
 	if (err) {
 		dev_err(dev,
 			"Failed to register Embedded Controller subdevice %d\n",
 			err);
-		return err;
+		goto fail_mfd;
 	}
 
 	if (ec_dev->max_passthru) {
@@ -94,7 +123,7 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 			dev_err(dev,
 				"Failed to register Power Delivery subdevice %d\n",
 				err);
-			return err;
+			goto fail_mfd;
 		}
 	}
 
@@ -103,13 +132,18 @@ int cros_ec_register(struct cros_ec_device *ec_dev)
 		if (err) {
 			mfd_remove_devices(dev);
 			dev_err(dev, "Failed to register sub-devices\n");
-			return err;
+			goto fail_mfd;
 		}
 	}
 
 	dev_info(dev, "Chrome EC device registered\n");
 
 	return 0;
+
+fail_mfd:
+	if (ec_dev->irq)
+		free_irq(ec_dev->irq, ec_dev);
+	return err;
 }
 EXPORT_SYMBOL(cros_ec_register);
 
@@ -136,13 +170,31 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
 }
 EXPORT_SYMBOL(cros_ec_suspend);
 
+static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
+{
+	while (cros_ec_get_next_event(ec_dev) > 0)
+		blocking_notifier_call_chain(&ec_dev->event_notifier,
+					     1, ec_dev);
+}
+
 int cros_ec_resume(struct cros_ec_device *ec_dev)
 {
 	enable_irq(ec_dev->irq);
 
+	/*
+	 * In some cases, we need to distinguish between events that occur
+	 * during suspend if the EC is not a wake source. For example,
+	 * keypresses during suspend should be discarded if it does not wake
+	 * the system.
+	 *
+	 * If the EC is not a wake source, drain the event queue and mark them
+	 * as "queued during suspend".
+	 */
 	if (ec_dev->wake_enabled) {
 		disable_irq_wake(ec_dev->irq);
 		ec_dev->wake_enabled = 0;
+	} else {
+		cros_ec_drain_events(ec_dev);
 	}
 
 	return 0;
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 990308ca384f..c792e116e621 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -19,6 +19,7 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <asm/unaligned.h>
 
 #define EC_COMMAND_RETRIES	50
 
@@ -234,11 +235,44 @@ static int cros_ec_host_command_proto_query_v2(struct cros_ec_device *ec_dev)
 	return ret;
 }
 
+static int cros_ec_get_host_command_version_mask(struct cros_ec_device *ec_dev,
+	u16 cmd, u32 *mask)
+{
+	struct ec_params_get_cmd_versions *pver;
+	struct ec_response_get_cmd_versions *rver;
+	struct cros_ec_command *msg;
+	int ret;
+
+	msg = kmalloc(sizeof(*msg) + max(sizeof(*rver), sizeof(*pver)),
+		      GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->version = 0;
+	msg->command = EC_CMD_GET_CMD_VERSIONS;
+	msg->insize = sizeof(*rver);
+	msg->outsize = sizeof(*pver);
+
+	pver = (struct ec_params_get_cmd_versions *)msg->data;
+	pver->cmd = cmd;
+
+	ret = cros_ec_cmd_xfer(ec_dev, msg);
+	if (ret > 0) {
+		rver = (struct ec_response_get_cmd_versions *)msg->data;
+		*mask = rver->version_mask;
+	}
+
+	kfree(msg);
+
+	return ret;
+}
+
 int cros_ec_query_all(struct cros_ec_device *ec_dev)
 {
 	struct device *dev = ec_dev->dev;
 	struct cros_ec_command *proto_msg;
 	struct ec_response_get_protocol_info *proto_info;
+	u32 ver_mask = 0;
 	int ret;
 
 	proto_msg = kzalloc(sizeof(*proto_msg) + sizeof(*proto_info),
@@ -328,6 +362,15 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
 		goto exit;
 	}
 
+	/* Probe if MKBP event is supported */
+	ret = cros_ec_get_host_command_version_mask(ec_dev,
+						    EC_CMD_GET_NEXT_EVENT,
+						    &ver_mask);
+	if (ret < 0 || ver_mask == 0)
+		ec_dev->mkbp_event_supported = 0;
+	else
+		ec_dev->mkbp_event_supported = 1;
+
 exit:
 	kfree(proto_msg);
 	return ret;
@@ -380,3 +423,52 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 	return ret;
 }
 EXPORT_SYMBOL(cros_ec_cmd_xfer);
+
+static int get_next_event(struct cros_ec_device *ec_dev)
+{
+	u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
+	struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
+	int ret;
+
+	msg->version = 0;
+	msg->command = EC_CMD_GET_NEXT_EVENT;
+	msg->insize = sizeof(ec_dev->event_data);
+	msg->outsize = 0;
+
+	ret = cros_ec_cmd_xfer(ec_dev, msg);
+	if (ret > 0) {
+		ec_dev->event_size = ret - 1;
+		memcpy(&ec_dev->event_data, msg->data,
+		       sizeof(ec_dev->event_data));
+	}
+
+	return ret;
+}
+
+static int get_keyboard_state_event(struct cros_ec_device *ec_dev)
+{
+	u8 buffer[sizeof(struct cros_ec_command) +
+		  sizeof(ec_dev->event_data.data)];
+	struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
+
+	msg->version = 0;
+	msg->command = EC_CMD_MKBP_STATE;
+	msg->insize = sizeof(ec_dev->event_data.data);
+	msg->outsize = 0;
+
+	ec_dev->event_size = cros_ec_cmd_xfer(ec_dev, msg);
+	ec_dev->event_data.event_type = EC_MKBP_EVENT_KEY_MATRIX;
+	memcpy(&ec_dev->event_data.data, msg->data,
+	       sizeof(ec_dev->event_data.data));
+
+	return ec_dev->event_size;
+}
+
+int cros_ec_get_next_event(struct cros_ec_device *ec_dev)
+{
+	if (ec_dev->mkbp_event_supported)
+		return get_next_event(ec_dev);
+	else
+		return get_keyboard_state_event(ec_dev);
+}
+EXPORT_SYMBOL(cros_ec_get_next_event);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index a677c2bd485c..15252702aca5 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -107,6 +107,10 @@ struct cros_ec_command {
  *     should check msg.result for the EC's result code.
  * @pkt_xfer: send packet to EC and get response
  * @lock: one transaction at a time
+ * @mkbp_event_supported: true if this EC supports the MKBP event protocol.
+ * @event_notifier: interrupt event notifier for transport devices.
+ * @event_data: raw payload transferred with the MKBP event.
+ * @event_size: size in bytes of the event data.
  */
 struct cros_ec_device {
 
@@ -135,6 +139,11 @@ struct cros_ec_device {
 	int (*pkt_xfer)(struct cros_ec_device *ec,
 			struct cros_ec_command *msg);
 	struct mutex lock;
+	bool mkbp_event_supported;
+	struct blocking_notifier_head event_notifier;
+
+	struct ec_response_get_next_event event_data;
+	int event_size;
 };
 
 /* struct cros_ec_platform - ChromeOS EC platform information
@@ -252,6 +261,15 @@ int cros_ec_register(struct cros_ec_device *ec_dev);
  */
 int cros_ec_query_all(struct cros_ec_device *ec_dev);
 
+/**
+ * cros_ec_get_next_event -  Fetch next event from the ChromeOS EC
+ *
+ * @ec_dev: Device to fetch event from
+ *
+ * Returns: 0 on success, Linux error number on failure
+ */
+int cros_ec_get_next_event(struct cros_ec_device *ec_dev);
+
 /* sysfs stuff */
 extern struct attribute_group cros_ec_attr_group;
 extern struct attribute_group cros_ec_lightbar_attr_group;
diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index 13b630c10d4c..54b0812601f8 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -1762,6 +1762,40 @@ struct ec_result_keyscan_seq_ctrl {
 	};
 } __packed;
 
+/*
+ * Command for retrieving the next pending MKBP event from the EC device
+ *
+ * The device replies with UNAVAILABLE if there aren't any pending events.
+ */
+#define EC_CMD_GET_NEXT_EVENT 0x67
+
+enum ec_mkbp_event {
+	/* Keyboard matrix changed. The event data is the new matrix state. */
+	EC_MKBP_EVENT_KEY_MATRIX = 0,
+
+	/* New host event. The event data is 4 bytes of host event flags. */
+	EC_MKBP_EVENT_HOST_EVENT = 1,
+
+	/* New Sensor FIFO data. The event data is fifo_info structure. */
+	EC_MKBP_EVENT_SENSOR_FIFO = 2,
+
+	/* Number of MKBP events */
+	EC_MKBP_EVENT_COUNT,
+};
+
+union ec_response_get_next_data {
+	uint8_t   key_matrix[13];
+
+	/* Unaligned */
+	uint32_t  host_event;
+} __packed;
+
+struct ec_response_get_next_event {
+	uint8_t event_type;
+	/* Followed by event data if any */
+	union ec_response_get_next_data data;
+} __packed;
+
 /*****************************************************************************/
 /* Temperature sensor commands */
 
-- 
2.5.5

[PATCH v8 2/7] Input: cros_ec_keyb - Stop handling interrupts directly

[Tomeu Vizoso]

From: Vic Yang <victoryang@google.com>

Because events other that keyboard ones will be handled by now on by
other drivers, stop directly handling interrupts and instead listen to
the new notifier in the MFD driver.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/input/keyboard/cros_ec_keyb.c | 135 ++++++++--------------------------
 1 file changed, 31 insertions(+), 104 deletions(-)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index b01966dc7eb3..b891503143dc 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -27,6 +27,7 @@
 #include <linux/input.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
+#include <linux/notifier.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/input/matrix_keypad.h>
@@ -44,6 +45,7 @@
  * @dev: Device pointer
  * @idev: Input device
  * @ec: Top level ChromeOS device to use to talk to EC
+ * @notifier: interrupt event notifier for transport devices
  */
 struct cros_ec_keyb {
 	unsigned int rows;
@@ -57,6 +59,7 @@ struct cros_ec_keyb {
 	struct device *dev;
 	struct input_dev *idev;
 	struct cros_ec_device *ec;
+	struct notifier_block notifier;
 };
 
 
@@ -146,67 +149,44 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
 	input_sync(ckdev->idev);
 }
 
-static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
-{
-	int ret = 0;
-	struct cros_ec_command *msg;
-
-	msg = kmalloc(sizeof(*msg) + ckdev->cols, GFP_KERNEL);
-	if (!msg)
-		return -ENOMEM;
-
-	msg->version = 0;
-	msg->command = EC_CMD_MKBP_STATE;
-	msg->insize = ckdev->cols;
-	msg->outsize = 0;
-
-	ret = cros_ec_cmd_xfer(ckdev->ec, msg);
-	if (ret < 0) {
-		dev_err(ckdev->dev, "Error transferring EC message %d\n", ret);
-		goto exit;
-	}
-
-	memcpy(kb_state, msg->data, ckdev->cols);
-exit:
-	kfree(msg);
-	return ret;
-}
-
-static irqreturn_t cros_ec_keyb_irq(int irq, void *data)
+static int cros_ec_keyb_open(struct input_dev *dev)
 {
-	struct cros_ec_keyb *ckdev = data;
-	struct cros_ec_device *ec = ckdev->ec;
-	int ret;
-	uint8_t kb_state[ckdev->cols];
-
-	if (device_may_wakeup(ec->dev))
-		pm_wakeup_event(ec->dev, 0);
-
-	ret = cros_ec_keyb_get_state(ckdev, kb_state);
-	if (ret >= 0)
-		cros_ec_keyb_process(ckdev, kb_state, ret);
-	else
-		dev_err(ec->dev, "failed to get keyboard state: %d\n", ret);
+	struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
 
-	return IRQ_HANDLED;
+	return blocking_notifier_chain_register(&ckdev->ec->event_notifier,
+						&ckdev->notifier);
 }
 
-static int cros_ec_keyb_open(struct input_dev *dev)
+static void cros_ec_keyb_close(struct input_dev *dev)
 {
 	struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
-	struct cros_ec_device *ec = ckdev->ec;
 
-	return request_threaded_irq(ec->irq, NULL, cros_ec_keyb_irq,
-					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
-					"cros_ec_keyb", ckdev);
+	blocking_notifier_chain_unregister(&ckdev->ec->event_notifier,
+					   &ckdev->notifier);
 }
 
-static void cros_ec_keyb_close(struct input_dev *dev)
+static int cros_ec_keyb_work(struct notifier_block *nb,
+			     unsigned long queued_during_suspend, void *_notify)
 {
-	struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
-	struct cros_ec_device *ec = ckdev->ec;
+	struct cros_ec_keyb *ckdev = container_of(nb, struct cros_ec_keyb,
+						  notifier);
 
-	free_irq(ec->irq, ckdev);
+	if (ckdev->ec->event_data.event_type != EC_MKBP_EVENT_KEY_MATRIX)
+		return NOTIFY_DONE;
+	/*
+	 * If EC is not the wake source, discard key state changes during
+	 * suspend.
+	 */
+	if (queued_during_suspend)
+		return NOTIFY_OK;
+	if (ckdev->ec->event_size != ckdev->cols) {
+		dev_err(ckdev->dev,
+			"Discarded incomplete key matrix event.\n");
+		return NOTIFY_OK;
+	}
+	cros_ec_keyb_process(ckdev, ckdev->ec->event_data.data.key_matrix,
+			     ckdev->ec->event_size);
+	return NOTIFY_OK;
 }
 
 /*
@@ -266,12 +246,8 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
 	if (!idev)
 		return -ENOMEM;
 
-	if (!ec->irq) {
-		dev_err(dev, "no EC IRQ specified\n");
-		return -EINVAL;
-	}
-
 	ckdev->ec = ec;
+	ckdev->notifier.notifier_call = cros_ec_keyb_work;
 	ckdev->dev = dev;
 	dev_set_drvdata(&pdev->dev, ckdev);
 
@@ -312,54 +288,6 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-/* Clear any keys in the buffer */
-static void cros_ec_keyb_clear_keyboard(struct cros_ec_keyb *ckdev)
-{
-	uint8_t old_state[ckdev->cols];
-	uint8_t new_state[ckdev->cols];
-	unsigned long duration;
-	int i, ret;
-
-	/*
-	 * Keep reading until we see that the scan state does not change.
-	 * That indicates that we are done.
-	 *
-	 * Assume that the EC keyscan buffer is at most 32 deep.
-	 */
-	duration = jiffies;
-	ret = cros_ec_keyb_get_state(ckdev, new_state);
-	for (i = 1; !ret && i < 32; i++) {
-		memcpy(old_state, new_state, sizeof(old_state));
-		ret = cros_ec_keyb_get_state(ckdev, new_state);
-		if (0 == memcmp(old_state, new_state, sizeof(old_state)))
-			break;
-	}
-	duration = jiffies - duration;
-	dev_info(ckdev->dev, "Discarded %d keyscan(s) in %dus\n", i,
-		jiffies_to_usecs(duration));
-}
-
-static int cros_ec_keyb_resume(struct device *dev)
-{
-	struct cros_ec_keyb *ckdev = dev_get_drvdata(dev);
-
-	/*
-	 * When the EC is not a wake source, then it could not have caused the
-	 * resume, so we clear the EC's key scan buffer. If the EC was a
-	 * wake source (e.g. the lid is open and the user might press a key to
-	 * wake) then the key scan buffer should be preserved.
-	 */
-	if (!ckdev->ec->was_wake_device)
-		cros_ec_keyb_clear_keyboard(ckdev);
-
-	return 0;
-}
-
-#endif
-
-static SIMPLE_DEV_PM_OPS(cros_ec_keyb_pm_ops, NULL, cros_ec_keyb_resume);
-
 #ifdef CONFIG_OF
 static const struct of_device_id cros_ec_keyb_of_match[] = {
 	{ .compatible = "google,cros-ec-keyb" },
@@ -373,7 +301,6 @@ static struct platform_driver cros_ec_keyb_driver = {
 	.driver = {
 		.name = "cros-ec-keyb",
 		.of_match_table = of_match_ptr(cros_ec_keyb_of_match),
-		.pm	= &cros_ec_keyb_pm_ops,
 	},
 };
 
-- 
2.5.5

Re: [PATCH v8 2/7] Input: cros_ec_keyb - Stop handling interrupts directly

[Dmitry Torokhov]

On Tue, Apr 12, 2016 at 02:32:25PM +0200, Tomeu Vizoso wrote:
> From: Vic Yang <victoryang@google.com>
> 
> Because events other that keyboard ones will be handled by now on by
> other drivers, stop directly handling interrupts and instead listen to
> the new notifier in the MFD driver.
> 

Hmm, where did Vic's sign-off go?

> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Please merge through whatever tree takes the rest of the patches.

> ---
> 
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/input/keyboard/cros_ec_keyb.c | 135 ++++++++--------------------------
>  1 file changed, 31 insertions(+), 104 deletions(-)
> 
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index b01966dc7eb3..b891503143dc 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -27,6 +27,7 @@
>  #include <linux/input.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> +#include <linux/notifier.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/input/matrix_keypad.h>
> @@ -44,6 +45,7 @@
>   * @dev: Device pointer
>   * @idev: Input device
>   * @ec: Top level ChromeOS device to use to talk to EC
> + * @notifier: interrupt event notifier for transport devices
>   */
>  struct cros_ec_keyb {
>  	unsigned int rows;
> @@ -57,6 +59,7 @@ struct cros_ec_keyb {
>  	struct device *dev;
>  	struct input_dev *idev;
>  	struct cros_ec_device *ec;
> +	struct notifier_block notifier;
>  };
>  
>  
> @@ -146,67 +149,44 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
>  	input_sync(ckdev->idev);
>  }
>  
> -static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
> -{
> -	int ret = 0;
> -	struct cros_ec_command *msg;
> -
> -	msg = kmalloc(sizeof(*msg) + ckdev->cols, GFP_KERNEL);
> -	if (!msg)
> -		return -ENOMEM;
> -
> -	msg->version = 0;
> -	msg->command = EC_CMD_MKBP_STATE;
> -	msg->insize = ckdev->cols;
> -	msg->outsize = 0;
> -
> -	ret = cros_ec_cmd_xfer(ckdev->ec, msg);
> -	if (ret < 0) {
> -		dev_err(ckdev->dev, "Error transferring EC message %d\n", ret);
> -		goto exit;
> -	}
> -
> -	memcpy(kb_state, msg->data, ckdev->cols);
> -exit:
> -	kfree(msg);
> -	return ret;
> -}
> -
> -static irqreturn_t cros_ec_keyb_irq(int irq, void *data)
> +static int cros_ec_keyb_open(struct input_dev *dev)
>  {
> -	struct cros_ec_keyb *ckdev = data;
> -	struct cros_ec_device *ec = ckdev->ec;
> -	int ret;
> -	uint8_t kb_state[ckdev->cols];
> -
> -	if (device_may_wakeup(ec->dev))
> -		pm_wakeup_event(ec->dev, 0);
> -
> -	ret = cros_ec_keyb_get_state(ckdev, kb_state);
> -	if (ret >= 0)
> -		cros_ec_keyb_process(ckdev, kb_state, ret);
> -	else
> -		dev_err(ec->dev, "failed to get keyboard state: %d\n", ret);
> +	struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
>  
> -	return IRQ_HANDLED;
> +	return blocking_notifier_chain_register(&ckdev->ec->event_notifier,
> +						&ckdev->notifier);
>  }
>  
> -static int cros_ec_keyb_open(struct input_dev *dev)
> +static void cros_ec_keyb_close(struct input_dev *dev)
>  {
>  	struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
> -	struct cros_ec_device *ec = ckdev->ec;
>  
> -	return request_threaded_irq(ec->irq, NULL, cros_ec_keyb_irq,
> -					IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> -					"cros_ec_keyb", ckdev);
> +	blocking_notifier_chain_unregister(&ckdev->ec->event_notifier,
> +					   &ckdev->notifier);
>  }
>  
> -static void cros_ec_keyb_close(struct input_dev *dev)
> +static int cros_ec_keyb_work(struct notifier_block *nb,
> +			     unsigned long queued_during_suspend, void *_notify)
>  {
> -	struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
> -	struct cros_ec_device *ec = ckdev->ec;
> +	struct cros_ec_keyb *ckdev = container_of(nb, struct cros_ec_keyb,
> +						  notifier);
>  
> -	free_irq(ec->irq, ckdev);
> +	if (ckdev->ec->event_data.event_type != EC_MKBP_EVENT_KEY_MATRIX)
> +		return NOTIFY_DONE;
> +	/*
> +	 * If EC is not the wake source, discard key state changes during
> +	 * suspend.
> +	 */
> +	if (queued_during_suspend)
> +		return NOTIFY_OK;
> +	if (ckdev->ec->event_size != ckdev->cols) {
> +		dev_err(ckdev->dev,
> +			"Discarded incomplete key matrix event.\n");
> +		return NOTIFY_OK;
> +	}
> +	cros_ec_keyb_process(ckdev, ckdev->ec->event_data.data.key_matrix,
> +			     ckdev->ec->event_size);
> +	return NOTIFY_OK;
>  }
>  
>  /*
> @@ -266,12 +246,8 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
>  	if (!idev)
>  		return -ENOMEM;
>  
> -	if (!ec->irq) {
> -		dev_err(dev, "no EC IRQ specified\n");
> -		return -EINVAL;
> -	}
> -
>  	ckdev->ec = ec;
> +	ckdev->notifier.notifier_call = cros_ec_keyb_work;
>  	ckdev->dev = dev;
>  	dev_set_drvdata(&pdev->dev, ckdev);
>  
> @@ -312,54 +288,6 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
> -/* Clear any keys in the buffer */
> -static void cros_ec_keyb_clear_keyboard(struct cros_ec_keyb *ckdev)
> -{
> -	uint8_t old_state[ckdev->cols];
> -	uint8_t new_state[ckdev->cols];
> -	unsigned long duration;
> -	int i, ret;
> -
> -	/*
> -	 * Keep reading until we see that the scan state does not change.
> -	 * That indicates that we are done.
> -	 *
> -	 * Assume that the EC keyscan buffer is at most 32 deep.
> -	 */
> -	duration = jiffies;
> -	ret = cros_ec_keyb_get_state(ckdev, new_state);
> -	for (i = 1; !ret && i < 32; i++) {
> -		memcpy(old_state, new_state, sizeof(old_state));
> -		ret = cros_ec_keyb_get_state(ckdev, new_state);
> -		if (0 == memcmp(old_state, new_state, sizeof(old_state)))
> -			break;
> -	}
> -	duration = jiffies - duration;
> -	dev_info(ckdev->dev, "Discarded %d keyscan(s) in %dus\n", i,
> -		jiffies_to_usecs(duration));
> -}
> -
> -static int cros_ec_keyb_resume(struct device *dev)
> -{
> -	struct cros_ec_keyb *ckdev = dev_get_drvdata(dev);
> -
> -	/*
> -	 * When the EC is not a wake source, then it could not have caused the
> -	 * resume, so we clear the EC's key scan buffer. If the EC was a
> -	 * wake source (e.g. the lid is open and the user might press a key to
> -	 * wake) then the key scan buffer should be preserved.
> -	 */
> -	if (!ckdev->ec->was_wake_device)
> -		cros_ec_keyb_clear_keyboard(ckdev);
> -
> -	return 0;
> -}
> -
> -#endif
> -
> -static SIMPLE_DEV_PM_OPS(cros_ec_keyb_pm_ops, NULL, cros_ec_keyb_resume);
> -
>  #ifdef CONFIG_OF
>  static const struct of_device_id cros_ec_keyb_of_match[] = {
>  	{ .compatible = "google,cros-ec-keyb" },
> @@ -373,7 +301,6 @@ static struct platform_driver cros_ec_keyb_driver = {
>  	.driver = {
>  		.name = "cros-ec-keyb",
>  		.of_match_table = of_match_ptr(cros_ec_keyb_of_match),
> -		.pm	= &cros_ec_keyb_pm_ops,
>  	},
>  };
>  
> -- 
> 2.5.5
> 

-- 
Dmitry

Re: [PATCH v8 2/7] Input: cros_ec_keyb - Stop handling interrupts directly

[Tomeu Vizoso]

On 25 April 2016 at 23:17, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Tue, Apr 12, 2016 at 02:32:25PM +0200, Tomeu Vizoso wrote:
>> From: Vic Yang <victoryang@google.com>
>>
>> Because events other that keyboard ones will be handled by now on by
>> other drivers, stop directly handling interrupts and instead listen to
>> the new notifier in the MFD driver.
>>
>
> Hmm, where did Vic's sign-off go?

Lee Jones asked to remove them in a previous version as he considers
them superfluous. My understanding is that as I'm the first to submit
them to mainline, the chain starts with me (I certify the b section of
http://developercertificate.org/).

>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Please merge through whatever tree takes the rest of the patches.

Thank you,

Tomeu

>> ---
>>
>> Changes in v8: None
>> Changes in v7: None
>> Changes in v6: None
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>  drivers/input/keyboard/cros_ec_keyb.c | 135 ++++++++--------------------------
>>  1 file changed, 31 insertions(+), 104 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
>> index b01966dc7eb3..b891503143dc 100644
>> --- a/drivers/input/keyboard/cros_ec_keyb.c
>> +++ b/drivers/input/keyboard/cros_ec_keyb.c
>> @@ -27,6 +27,7 @@
>>  #include <linux/input.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/kernel.h>
>> +#include <linux/notifier.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/slab.h>
>>  #include <linux/input/matrix_keypad.h>
>> @@ -44,6 +45,7 @@
>>   * @dev: Device pointer
>>   * @idev: Input device
>>   * @ec: Top level ChromeOS device to use to talk to EC
>> + * @notifier: interrupt event notifier for transport devices
>>   */
>>  struct cros_ec_keyb {
>>       unsigned int rows;
>> @@ -57,6 +59,7 @@ struct cros_ec_keyb {
>>       struct device *dev;
>>       struct input_dev *idev;
>>       struct cros_ec_device *ec;
>> +     struct notifier_block notifier;
>>  };
>>
>>
>> @@ -146,67 +149,44 @@ static void cros_ec_keyb_process(struct cros_ec_keyb *ckdev,
>>       input_sync(ckdev->idev);
>>  }
>>
>> -static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
>> -{
>> -     int ret = 0;
>> -     struct cros_ec_command *msg;
>> -
>> -     msg = kmalloc(sizeof(*msg) + ckdev->cols, GFP_KERNEL);
>> -     if (!msg)
>> -             return -ENOMEM;
>> -
>> -     msg->version = 0;
>> -     msg->command = EC_CMD_MKBP_STATE;
>> -     msg->insize = ckdev->cols;
>> -     msg->outsize = 0;
>> -
>> -     ret = cros_ec_cmd_xfer(ckdev->ec, msg);
>> -     if (ret < 0) {
>> -             dev_err(ckdev->dev, "Error transferring EC message %d\n", ret);
>> -             goto exit;
>> -     }
>> -
>> -     memcpy(kb_state, msg->data, ckdev->cols);
>> -exit:
>> -     kfree(msg);
>> -     return ret;
>> -}
>> -
>> -static irqreturn_t cros_ec_keyb_irq(int irq, void *data)
>> +static int cros_ec_keyb_open(struct input_dev *dev)
>>  {
>> -     struct cros_ec_keyb *ckdev = data;
>> -     struct cros_ec_device *ec = ckdev->ec;
>> -     int ret;
>> -     uint8_t kb_state[ckdev->cols];
>> -
>> -     if (device_may_wakeup(ec->dev))
>> -             pm_wakeup_event(ec->dev, 0);
>> -
>> -     ret = cros_ec_keyb_get_state(ckdev, kb_state);
>> -     if (ret >= 0)
>> -             cros_ec_keyb_process(ckdev, kb_state, ret);
>> -     else
>> -             dev_err(ec->dev, "failed to get keyboard state: %d\n", ret);
>> +     struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
>>
>> -     return IRQ_HANDLED;
>> +     return blocking_notifier_chain_register(&ckdev->ec->event_notifier,
>> +                                             &ckdev->notifier);
>>  }
>>
>> -static int cros_ec_keyb_open(struct input_dev *dev)
>> +static void cros_ec_keyb_close(struct input_dev *dev)
>>  {
>>       struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
>> -     struct cros_ec_device *ec = ckdev->ec;
>>
>> -     return request_threaded_irq(ec->irq, NULL, cros_ec_keyb_irq,
>> -                                     IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> -                                     "cros_ec_keyb", ckdev);
>> +     blocking_notifier_chain_unregister(&ckdev->ec->event_notifier,
>> +                                        &ckdev->notifier);
>>  }
>>
>> -static void cros_ec_keyb_close(struct input_dev *dev)
>> +static int cros_ec_keyb_work(struct notifier_block *nb,
>> +                          unsigned long queued_during_suspend, void *_notify)
>>  {
>> -     struct cros_ec_keyb *ckdev = input_get_drvdata(dev);
>> -     struct cros_ec_device *ec = ckdev->ec;
>> +     struct cros_ec_keyb *ckdev = container_of(nb, struct cros_ec_keyb,
>> +                                               notifier);
>>
>> -     free_irq(ec->irq, ckdev);
>> +     if (ckdev->ec->event_data.event_type != EC_MKBP_EVENT_KEY_MATRIX)
>> +             return NOTIFY_DONE;
>> +     /*
>> +      * If EC is not the wake source, discard key state changes during
>> +      * suspend.
>> +      */
>> +     if (queued_during_suspend)
>> +             return NOTIFY_OK;
>> +     if (ckdev->ec->event_size != ckdev->cols) {
>> +             dev_err(ckdev->dev,
>> +                     "Discarded incomplete key matrix event.\n");
>> +             return NOTIFY_OK;
>> +     }
>> +     cros_ec_keyb_process(ckdev, ckdev->ec->event_data.data.key_matrix,
>> +                          ckdev->ec->event_size);
>> +     return NOTIFY_OK;
>>  }
>>
>>  /*
>> @@ -266,12 +246,8 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
>>       if (!idev)
>>               return -ENOMEM;
>>
>> -     if (!ec->irq) {
>> -             dev_err(dev, "no EC IRQ specified\n");
>> -             return -EINVAL;
>> -     }
>> -
>>       ckdev->ec = ec;
>> +     ckdev->notifier.notifier_call = cros_ec_keyb_work;
>>       ckdev->dev = dev;
>>       dev_set_drvdata(&pdev->dev, ckdev);
>>
>> @@ -312,54 +288,6 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
>>       return 0;
>>  }
>>
>> -#ifdef CONFIG_PM_SLEEP
>> -/* Clear any keys in the buffer */
>> -static void cros_ec_keyb_clear_keyboard(struct cros_ec_keyb *ckdev)
>> -{
>> -     uint8_t old_state[ckdev->cols];
>> -     uint8_t new_state[ckdev->cols];
>> -     unsigned long duration;
>> -     int i, ret;
>> -
>> -     /*
>> -      * Keep reading until we see that the scan state does not change.
>> -      * That indicates that we are done.
>> -      *
>> -      * Assume that the EC keyscan buffer is at most 32 deep.
>> -      */
>> -     duration = jiffies;
>> -     ret = cros_ec_keyb_get_state(ckdev, new_state);
>> -     for (i = 1; !ret && i < 32; i++) {
>> -             memcpy(old_state, new_state, sizeof(old_state));
>> -             ret = cros_ec_keyb_get_state(ckdev, new_state);
>> -             if (0 == memcmp(old_state, new_state, sizeof(old_state)))
>> -                     break;
>> -     }
>> -     duration = jiffies - duration;
>> -     dev_info(ckdev->dev, "Discarded %d keyscan(s) in %dus\n", i,
>> -             jiffies_to_usecs(duration));
>> -}
>> -
>> -static int cros_ec_keyb_resume(struct device *dev)
>> -{
>> -     struct cros_ec_keyb *ckdev = dev_get_drvdata(dev);
>> -
>> -     /*
>> -      * When the EC is not a wake source, then it could not have caused the
>> -      * resume, so we clear the EC's key scan buffer. If the EC was a
>> -      * wake source (e.g. the lid is open and the user might press a key to
>> -      * wake) then the key scan buffer should be preserved.
>> -      */
>> -     if (!ckdev->ec->was_wake_device)
>> -             cros_ec_keyb_clear_keyboard(ckdev);
>> -
>> -     return 0;
>> -}
>> -
>> -#endif
>> -
>> -static SIMPLE_DEV_PM_OPS(cros_ec_keyb_pm_ops, NULL, cros_ec_keyb_resume);
>> -
>>  #ifdef CONFIG_OF
>>  static const struct of_device_id cros_ec_keyb_of_match[] = {
>>       { .compatible = "google,cros-ec-keyb" },
>> @@ -373,7 +301,6 @@ static struct platform_driver cros_ec_keyb_driver = {
>>       .driver = {
>>               .name = "cros-ec-keyb",
>>               .of_match_table = of_match_ptr(cros_ec_keyb_of_match),
>> -             .pm     = &cros_ec_keyb_pm_ops,
>>       },
>>  };
>>
>> --
>> 2.5.5
>>
>
> --
> Dmitry

Re: [PATCH v8 2/7] Input: cros_ec_keyb - Stop handling interrupts directly

[Lee Jones]

On Tue, 26 Apr 2016, Tomeu Vizoso wrote:

> On 25 April 2016 at 23:17, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > On Tue, Apr 12, 2016 at 02:32:25PM +0200, Tomeu Vizoso wrote:
> >> From: Vic Yang <victoryang@google.com>
> >>
> >> Because events other that keyboard ones will be handled by now on by
> >> other drivers, stop directly handling interrupts and instead listen to
> >> the new notifier in the MFD driver.
> >>
> >
> > Hmm, where did Vic's sign-off go?
> 
> Lee Jones asked to remove them in a previous version as he considers
> them superfluous. My understanding is that as I'm the first to submit
> them to mainline, the chain starts with me (I certify the b section of
> http://developercertificate.org/).

Hmm... It seems what I said has been misconstrued a little.  You
*should* remove SoBs from people who were *only* part of the
submission path.  However, you should *not* remove SoBs from patch
*authors*.  Since Vic is the author (or at least one of them), their
SoB should remain.

Apologies if that was not clear.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v8 2/7] Input: cros_ec_keyb - Stop handling interrupts directly

[Lee Jones]

On Tue, 26 Apr 2016, Tomeu Vizoso wrote:

> On 25 April 2016 at 23:17, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > On Tue, Apr 12, 2016 at 02:32:25PM +0200, Tomeu Vizoso wrote:
> >> From: Vic Yang <victoryang@google.com>
> >>
> >> Because events other that keyboard ones will be handled by now on by
> >> other drivers, stop directly handling interrupts and instead listen to
> >> the new notifier in the MFD driver.
> >>
> >
> > Hmm, where did Vic's sign-off go?
> 
> Lee Jones asked to remove them in a previous version as he considers
> them superfluous. My understanding is that as I'm the first to submit
> them to mainline, the chain starts with me (I certify the b section of
> http://developercertificate.org/).

Hmm... It seems what I said has been misconstrued a little.  You
*should* remove SoBs from people who were *only* part of the
submission path.  However, you should *not* remove SoBs from patch
*authors*.  Since Vic is the author (or at least one of them), their
SoB should remain.

Apologies if that was not clear.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 2/7] Input: cros_ec_keyb - Stop handling interrupts directly

[Tomeu Vizoso]

On 26 April 2016 at 08:57, Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 26 Apr 2016, Tomeu Vizoso wrote:
>
>> On 25 April 2016 at 23:17, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> > On Tue, Apr 12, 2016 at 02:32:25PM +0200, Tomeu Vizoso wrote:
>> >> From: Vic Yang <victoryang@google.com>
>> >>
>> >> Because events other that keyboard ones will be handled by now on by
>> >> other drivers, stop directly handling interrupts and instead listen to
>> >> the new notifier in the MFD driver.
>> >>
>> >
>> > Hmm, where did Vic's sign-off go?
>>
>> Lee Jones asked to remove them in a previous version as he considers
>> them superfluous. My understanding is that as I'm the first to submit
>> them to mainline, the chain starts with me (I certify the b section of
>> http://developercertificate.org/).
>
> Hmm... It seems what I said has been misconstrued a little.  You
> *should* remove SoBs from people who were *only* part of the
> submission path.  However, you should *not* remove SoBs from patch
> *authors*.  Since Vic is the author (or at least one of them), their
> SoB should remain.
>
> Apologies if that was not clear.

I see now, will fix the tags in the next revision.

Thanks,

Tomeu

Re: [PATCH v8 2/7] Input: cros_ec_keyb - Stop handling interrupts directly

[Enric Balletbo Serra]

Hi Tomeu,

2016-04-26 9:06 GMT+02:00 Tomeu Vizoso <tomeu.vizoso@collabora.com>:
> On 26 April 2016 at 08:57, Lee Jones <lee.jones@linaro.org> wrote:
>> On Tue, 26 Apr 2016, Tomeu Vizoso wrote:
>>
>>> On 25 April 2016 at 23:17, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>> > On Tue, Apr 12, 2016 at 02:32:25PM +0200, Tomeu Vizoso wrote:
>>> >> From: Vic Yang <victoryang@google.com>
>>> >>
>>> >> Because events other that keyboard ones will be handled by now on by
>>> >> other drivers, stop directly handling interrupts and instead listen to
>>> >> the new notifier in the MFD driver.
>>> >>
>>> >
>>> > Hmm, where did Vic's sign-off go?
>>>
>>> Lee Jones asked to remove them in a previous version as he considers
>>> them superfluous. My understanding is that as I'm the first to submit
>>> them to mainline, the chain starts with me (I certify the b section of
>>> http://developercertificate.org/).
>>
>> Hmm... It seems what I said has been misconstrued a little.  You
>> *should* remove SoBs from people who were *only* part of the
>> submission path.  However, you should *not* remove SoBs from patch
>> *authors*.  Since Vic is the author (or at least one of them), their
>> SoB should remain.
>>
>> Apologies if that was not clear.
>
> I see now, will fix the tags in the next revision.
>

With your permission I'll fix this and send a new patch series with
only the patch that adds the MKBP event support and this patch. These
two patches has sense by itself and are only a dependency of cros-ec
USB PD driver and other drivers, so I think makes sense send within a
separate series to increase the possibility to get merged and don't
block other drivers that depends on these.

Thanks,

Enric


> Thanks,
>
> Tomeu

Re: [PATCH v8 2/7] Input: cros_ec_keyb - Stop handling interrupts directly

[Tomeu Vizoso]

On 1 July 2016 at 10:49, Enric Balletbo Serra <eballetbo@gmail.com> wrote:
> Hi Tomeu,
>
> 2016-04-26 9:06 GMT+02:00 Tomeu Vizoso <tomeu.vizoso@collabora.com>:
>> On 26 April 2016 at 08:57, Lee Jones <lee.jones@linaro.org> wrote:
>>> On Tue, 26 Apr 2016, Tomeu Vizoso wrote:
>>>
>>>> On 25 April 2016 at 23:17, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>>> > On Tue, Apr 12, 2016 at 02:32:25PM +0200, Tomeu Vizoso wrote:
>>>> >> From: Vic Yang <victoryang@google.com>
>>>> >>
>>>> >> Because events other that keyboard ones will be handled by now on by
>>>> >> other drivers, stop directly handling interrupts and instead listen to
>>>> >> the new notifier in the MFD driver.
>>>> >>
>>>> >
>>>> > Hmm, where did Vic's sign-off go?
>>>>
>>>> Lee Jones asked to remove them in a previous version as he considers
>>>> them superfluous. My understanding is that as I'm the first to submit
>>>> them to mainline, the chain starts with me (I certify the b section of
>>>> http://developercertificate.org/).
>>>
>>> Hmm... It seems what I said has been misconstrued a little.  You
>>> *should* remove SoBs from people who were *only* part of the
>>> submission path.  However, you should *not* remove SoBs from patch
>>> *authors*.  Since Vic is the author (or at least one of them), their
>>> SoB should remain.
>>>
>>> Apologies if that was not clear.
>>
>> I see now, will fix the tags in the next revision.
>>
>
> With your permission I'll fix this and send a new patch series with
> only the patch that adds the MKBP event support and this patch. These
> two patches has sense by itself and are only a dependency of cros-ec
> USB PD driver and other drivers, so I think makes sense send within a
> separate series to increase the possibility to get merged and don't
> block other drivers that depends on these.

Sounds great, MKBP event support is indeed a dependency for the
upcoming features that we want to upstream.

Thanks,

Tomeu

> Thanks,
>
> Enric
>
>
>> Thanks,
>>
>> Tomeu

[PATCH v8 3/7] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Tomeu Vizoso]

So that callers of cros_ec_cmd_xfer don't have to repeat boilerplate
code when checking for errors from the EC side.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Benson Leung <bleung@chromium.org>
---

Changes in v8:
- Don't call cros_ec_cmd_xfer from declaration block

Changes in v7: None
Changes in v6: None
Changes in v5:
- Check explicitly for !EC_RES_SUCCESS as suggested by Benson Leung.

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/platform/chrome/cros_ec_proto.c | 15 +++++++++++++++
 include/linux/mfd/cros_ec.h             | 18 ++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index c792e116e621..f9677747cb3e 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -472,3 +472,18 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev)
 		return get_keyboard_state_event(ec_dev);
 }
 EXPORT_SYMBOL(cros_ec_get_next_event);
+
+int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
+			    struct cros_ec_command *msg)
+{
+	int ret;
+
+	ret = cros_ec_cmd_xfer(ec_dev, msg);
+	if (ret < 0)
+		dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
+	else if (msg->result != EC_RES_SUCCESS)
+		return -EECRESULT - msg->result;
+
+	return ret;
+}
+EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 15252702aca5..5d965aff5065 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -40,6 +40,9 @@
 #define EC_MAX_REQUEST_OVERHEAD		1
 #define EC_MAX_RESPONSE_OVERHEAD	2
 
+/* ec_command return value for non-success result from EC */
+#define EECRESULT 1000
+
 /*
  * Command interface between EC and AP, for LPC, I2C and SPI interfaces.
  */
@@ -233,6 +236,21 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 		     struct cros_ec_command *msg);
 
 /**
+ * cros_ec_cmd_xfer_status - Send a command to the ChromeOS EC
+ *
+ * This function is identical to cros_ec_cmd_xfer, except it returns succes
+ * status only if both the command was transmitted successfully and the EC
+ * replied with success status. It's not necessary to check msg->result when
+ * using this function.
+ *
+ * @ec_dev: EC device
+ * @msg: Message to write
+ * @return: Num. of bytes transferred on success, <0 on failure
+ */
+int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
+			    struct cros_ec_command *msg);
+
+/**
  * cros_ec_remove - Remove a ChromeOS EC
  *
  * Call this to deregister a ChromeOS EC, then clean up any private data.
-- 
2.5.5

[PATCH v8 4/7] mfd: cros_ec: Add cros_ec_get_host_event

[Tomeu Vizoso]

This function returns the code for the host event that triggered the
interrupt that is being currently handled.

Is to be used by observers of the event_notifier in the EC device.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v8:
- Remove unnecessary variable host_event

Changes in v7: None
Changes in v6:
- Return 0 if the EC doesn't support MKBP, as expected by callers.

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/platform/chrome/cros_ec_proto.c | 20 ++++++++++++++++++++
 include/linux/mfd/cros_ec.h             |  9 +++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index f9677747cb3e..da4188eecea4 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -487,3 +487,23 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
 	return ret;
 }
 EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
+
+u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev)
+{
+	if (!ec_dev->mkbp_event_supported) {
+		dev_warn(ec_dev->dev,
+			 "This EC does not support EC_MKBP_EVENT_HOST_EVENT");
+		return 0;
+	}
+
+	if (ec_dev->event_data.event_type != EC_MKBP_EVENT_HOST_EVENT)
+		return 0;
+
+	if (ec_dev->event_size != sizeof(ec_dev->event_data.data.host_event)) {
+		dev_warn(ec_dev->dev, "Invalid host event size\n");
+		return 0;
+	}
+
+	return get_unaligned_le32(&ec_dev->event_data.data.host_event);
+}
+EXPORT_SYMBOL(cros_ec_get_host_event);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 5d965aff5065..a39ed030d34f 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -288,6 +288,15 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev);
  */
 int cros_ec_get_next_event(struct cros_ec_device *ec_dev);
 
+/**
+ * cros_ec_get_host_event - Return a mask of event set by the EC.
+ *
+ * @ec_dev: Device to fetch event from
+ *
+ * Returns: event mask of the event that caused the last interrupt to be raised
+ */
+uint32_t cros_ec_get_host_event(struct cros_ec_device *ec_dev);
+
 /* sysfs stuff */
 extern struct attribute_group cros_ec_attr_group;
 extern struct attribute_group cros_ec_lightbar_attr_group;
-- 
2.5.5

[PATCH v8 5/7] mfd: cros_ec: Add more definitions for PD commands

[Tomeu Vizoso]

Copy a few structs and commands from the EC firmware headers so we can
communicate with the EC regarding PD functionality.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v8:
- Don't add stuff to cros_ec_commands.h not needed in this series

Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 include/linux/mfd/cros_ec_commands.h | 182 +++++++++++++++++++++++++++++++++++
 1 file changed, 182 insertions(+)

diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index 54b0812601f8..2a923a37dccb 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -286,6 +286,9 @@ enum host_event_code {
 	/* Hang detect logic detected a hang and warm rebooted the AP */
 	EC_HOST_EVENT_HANG_REBOOT = 21,
 
+	/* PD MCU triggering host event */
+	EC_HOST_EVENT_PD_MCU = 22,
+
 	/*
 	 * The high bit of the event mask is not used as a host event code.  If
 	 * it reads back as set, then the entire event mask should be
@@ -2576,6 +2579,185 @@ struct ec_params_usb_pd_control {
 	uint8_t mux;
 } __packed;
 
+#define EC_CMD_USB_PD_PORTS 0x102
+
+struct ec_response_usb_pd_ports {
+	uint8_t num_ports;
+} __packed;
+
+#define EC_CMD_USB_PD_POWER_INFO 0x103
+
+#define PD_POWER_CHARGING_PORT 0xff
+struct ec_params_usb_pd_power_info {
+	uint8_t port;
+} __packed;
+
+enum usb_chg_type {
+	USB_CHG_TYPE_NONE,
+	USB_CHG_TYPE_PD,
+	USB_CHG_TYPE_C,
+	USB_CHG_TYPE_PROPRIETARY,
+	USB_CHG_TYPE_BC12_DCP,
+	USB_CHG_TYPE_BC12_CDP,
+	USB_CHG_TYPE_BC12_SDP,
+	USB_CHG_TYPE_OTHER,
+	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;
+	uint16_t voltage_now;
+	uint16_t current_max;
+	uint16_t current_lim;
+} __packed;
+
+struct ec_response_usb_pd_power_info {
+	uint8_t role;
+	uint8_t type;
+	uint8_t dualrole;
+	uint8_t reserved1;
+	struct usb_chg_measures meas;
+	uint32_t max_power;
+} __packed;
+
+/* Read USB-PD Accessory info */
+#define EC_CMD_USB_PD_DEV_INFO 0x112
+
+struct ec_params_usb_pd_info_request {
+	uint8_t port;
+} __packed;
+
+/* Read USB-PD Device discovery info */
+#define EC_CMD_USB_PD_DISCOVERY 0x113
+
+/**
+ * struct ec_params_usb_pd_discovery_entry - Params for EC_CMD_USB_PD_DISCOVERY
+ *
+ * @vid: USB-IF VID
+ * @pid: USB-IF PID
+ * @ptype: product type (hub, periph, cable, ama)
+ */
+struct ec_params_usb_pd_discovery_entry {
+	uint16_t vid;
+	uint16_t pid;
+	uint8_t ptype;
+} __packed;
+
+/* Override default charge behavior */
+#define EC_CMD_PD_CHARGE_PORT_OVERRIDE 0x114
+
+/* 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 - Params for CHARGE_PORT_OVERRIDE cmd
+ *
+ * @override_port: Override port number
+ */
+struct ec_params_charge_port_override {
+	int16_t override_port;
+} __packed;
+
+/* Read (and delete) one entry of PD event log */
+#define EC_CMD_PD_GET_LOG_ENTRY 0x115
+
+/**
+ * struct ec_response_pd_log - Response for EC_CMD_PD_GET_LOG_ENTRY
+ *
+ * @timestamp_ms: relative timestamp
+ * @type: event type, see PD_EVENT_xx below
+ * @size_port: [7:5] port number [4:0] payload size in bytes
+ * @data: type-defined data payload
+ * @payload: optional additional data payload: 0..16 bytes
+ */
+struct ec_response_pd_log {
+	uint32_t timestamp_ms;
+	uint8_t type;
+	uint8_t size_port;
+	uint16_t data;
+	uint8_t payload[0];
+} __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)
+
+struct mcdp_version {
+	uint8_t major;
+	uint8_t minor;
+	uint16_t build;
+} __packed;
+
+/**
+ * struct mcdp_info - payload of PD_EVENT_VIDEO_CODEC
+ */
+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])
+
 /*****************************************************************************/
 /*
  * Passthru commands
-- 
2.5.5

[PATCH v8 6/7] power: cros_usbpd-charger: Add EC-based USB PD charger driver

[Tomeu Vizoso]

From: Sameer Nanda <snanda@chromium.org>

This driver exposes the charger functionality in the PD EC to userspace.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Sameer Nanda <snanda@chromium.org>
Cc: Benson Leung <bleung@chromium.org>
Cc: Shawn Nematbakhsh <shawnn@chromium.org>
---

Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5:
- Fix type of variable passed to do_div.

Changes in v4:
- Declare size parameters in ec_command as size_t

Changes in v3:
- Use do_div so it builds on 32bit (suggested by 0-day kbuild bot).
- Remove sysfs attributes ext_current_lim and ext_voltage_lim because
  I don't know yet where they should be placed (and don't have HW to
  test them).
- Remove superfluous pre-allocated buffers ec_inbuf and ec_outbuf.
- Lots of misc comments from Stephen Boyd were addressed.
- Unregister notification listener in .remove callback.

Changes in v2:
- Split out changes to cros_ec_commands.h and the helpers added to
  mfd/cros_ec.h from the patch that adds the charger driver, as
  suggested by Lee.
- Actually call get_ec_num_ports.

 drivers/power/Kconfig              |  10 +
 drivers/power/Makefile             |   1 +
 drivers/power/cros_usbpd-charger.c | 780 +++++++++++++++++++++++++++++++++++++
 3 files changed, 791 insertions(+)
 create mode 100644 drivers/power/cros_usbpd-charger.c

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
index 421770ddafa3..d096e76ed822 100644
--- a/drivers/power/Kconfig
+++ b/drivers/power/Kconfig
@@ -509,6 +509,16 @@ config AXP20X_POWER
 	  This driver provides support for the power supply features of
 	  AXP20x PMIC.
 
+config CHARGER_CROS_USB_PD
+	tristate "Chrome OS EC based USB PD charger"
+	depends on MFD_CROS_EC
+	default y
+	help
+	  Say Y here to enable Chrome OS EC based USB PD 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
 
 source "drivers/power/reset/Kconfig"
diff --git a/drivers/power/Makefile b/drivers/power/Makefile
index e46b75d448a5..c9eca1f9bfb0 100644
--- a/drivers/power/Makefile
+++ b/drivers/power/Makefile
@@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65217)	+= tps65217_charger.o
 obj-$(CONFIG_POWER_RESET)	+= reset/
 obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
 obj-$(CONFIG_AXP288_CHARGER)	+= axp288_charger.o
+obj-$(CONFIG_CHARGER_CROS_USB_PD)	+= cros_usbpd-charger.o
diff --git a/drivers/power/cros_usbpd-charger.c b/drivers/power/cros_usbpd-charger.c
new file mode 100644
index 000000000000..e7366660c093
--- /dev/null
+++ b/drivers/power/cros_usbpd-charger.c
@@ -0,0 +1,780 @@
+/*
+ * Power supply driver for ChromeOS EC based USB PD Charger.
+ *
+ * Copyright (c) 2014 Google, Inc
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/ktime.h>
+#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/rtc.h>
+#include <linux/slab.h>
+
+#define CROS_USB_PD_MAX_PORTS		8
+#define CROS_USB_PD_MAX_LOG_ENTRIES	30
+
+#define CROS_USB_PD_LOG_UPDATE_DELAY msecs_to_jiffies(60000)
+#define CROS_USB_PD_CACHE_UPDATE_DELAY msecs_to_jiffies(500)
+
+/* Buffer + macro for building PDLOG string */
+#define BUF_SIZE 80
+#define APPEND_STRING(buf, len, str, ...) ((len) += \
+	snprintf((buf) + (len), max(BUF_SIZE - (len), 0), (str), ##__VA_ARGS__))
+
+#define CHARGER_DIR_NAME		"CROS_USB_PD_CHARGER%d"
+#define CHARGER_DIR_NAME_LENGTH		sizeof(CHARGER_DIR_NAME)
+
+#define MANUFACTURER_MODEL_LENGTH	32
+
+struct port_data {
+	int port_number;
+	char name[CHARGER_DIR_NAME_LENGTH];
+	char manufacturer[MANUFACTURER_MODEL_LENGTH];
+	char model_name[MANUFACTURER_MODEL_LENGTH];
+	struct power_supply *psy;
+	struct power_supply_desc psy_desc;
+	int psy_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[CROS_USB_PD_MAX_PORTS];
+	struct delayed_work log_work;
+	struct workqueue_struct *log_workqueue;
+	struct notifier_block notifier;
+	bool suspended;
+};
+
+static enum power_supply_property cros_usb_pd_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_CHARGE_CONTROL_LIMIT_MAX,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_MANUFACTURER,
+};
+
+static int ec_command(struct cros_ec_dev *ec_dev, int version, int command,
+		      uint8_t *outdata, size_t outsize, uint8_t *indata,
+		      size_t insize)
+{
+	struct cros_ec_device *ec_device = ec_dev->ec_dev;
+	struct cros_ec_command *msg;
+	int ret;
+
+	msg = kmalloc(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;
+
+	memcpy(msg->data, outdata, outsize);
+	ret = cros_ec_cmd_xfer_status(ec_device, msg);
+	memcpy(indata, msg->data, insize);
+
+	kfree(msg);
+
+	return ret;
+}
+
+static int set_ec_usb_pd_override_ports(struct charger_data *charger,
+					int port_num)
+{
+	struct device *dev = charger->dev;
+	struct ec_params_charge_port_override req;
+	int ret;
+
+	req.override_port = port_num;
+
+	ret = ec_command(charger->ec_dev, 0, EC_CMD_PD_CHARGE_PORT_OVERRIDE,
+			 (uint8_t *)&req, sizeof(req),
+			 NULL, 0);
+	if (ret < 0) {
+		dev_err(dev, "Port Override command returned 0x%x\n", ret);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int get_ec_num_ports(struct charger_data *charger, int *num_ports)
+{
+	struct device *dev = charger->dev;
+	struct ec_response_usb_pd_ports resp;
+	int ret;
+
+	*num_ports = 0;
+	ret = ec_command(charger->ec_dev, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
+			 (uint8_t *)&resp, sizeof(resp));
+	if (ret < 0) {
+		dev_err(dev, "Unable to query PD ports (err:0x%x)\n", ret);
+		return ret;
+	}
+	*num_ports = resp.num_ports;
+	dev_dbg(dev, "Num ports = %d\n", *num_ports);
+
+	return 0;
+}
+
+static int get_ec_usb_pd_discovery_info(struct port_data *port)
+{
+	struct charger_data *charger = port->charger;
+	struct device *dev = charger->dev;
+	struct ec_params_usb_pd_info_request req;
+	struct ec_params_usb_pd_discovery_entry resp;
+	int ret;
+
+	req.port = port->port_number;
+
+	ret = ec_command(charger->ec_dev, 0, EC_CMD_USB_PD_DISCOVERY,
+			 (uint8_t *)&req, sizeof(req),
+			 (uint8_t *)&resp, sizeof(resp));
+	if (ret < 0) {
+		dev_err(dev, "Unable to query Discovery info (err:0x%x)\n",
+			 ret);
+		return -EINVAL;
+	}
+
+	dev_dbg(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, MANUFACTURER_MODEL_LENGTH, "%x", resp.vid);
+	snprintf(port->model_name, MANUFACTURER_MODEL_LENGTH, "%x", resp.pid);
+
+	return 0;
+}
+
+static int get_ec_usb_pd_power_info(struct port_data *port)
+{
+	struct charger_data *charger = port->charger;
+	struct device *dev = charger->dev;
+	struct ec_params_usb_pd_power_info req;
+	struct ec_response_usb_pd_power_info resp;
+	char role_str[80];
+	int ret, last_psy_status, last_psy_type;
+
+	req.port = port->port_number;
+	ret = ec_command(charger->ec_dev, 0, EC_CMD_USB_PD_POWER_INFO,
+			 (uint8_t *)&req, sizeof(req),
+			 (uint8_t *)&resp, sizeof(resp));
+	if (ret < 0) {
+		dev_err(dev, "Unable to query PD power info (err:0x%x)\n", ret);
+		return -EINVAL;
+	}
+
+	last_psy_status = port->psy_status;
+	last_psy_type = port->psy_type;
+
+	switch (resp.role) {
+	case USB_PD_PORT_POWER_DISCONNECTED:
+		snprintf(role_str, sizeof(role_str), "%s", "DISCONNECTED");
+		port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		port->psy_online = 0;
+		break;
+	case USB_PD_PORT_POWER_SOURCE:
+		snprintf(role_str, sizeof(role_str), "%s", "SOURCE");
+		port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		port->psy_online = 0;
+		break;
+	case USB_PD_PORT_POWER_SINK:
+		snprintf(role_str, sizeof(role_str), "%s", "SINK");
+		port->psy_status = POWER_SUPPLY_STATUS_CHARGING;
+		port->psy_online = 1;
+		break;
+	case USB_PD_PORT_POWER_SINK_NOT_CHARGING:
+		snprintf(role_str, sizeof(role_str), "%s", "NOT CHARGING");
+		port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		port->psy_online = 1;
+		break;
+	default:
+		snprintf(role_str, sizeof(role_str), "%s", "UNKNOWN");
+		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_type = POWER_SUPPLY_TYPE_USB;
+		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_type = POWER_SUPPLY_TYPE_USB_PD_DRP;
+		else
+			port->psy_type = POWER_SUPPLY_TYPE_USB;
+		break;
+	case USB_CHG_TYPE_PROPRIETARY:
+		port->psy_type = POWER_SUPPLY_TYPE_MAINS;
+		break;
+	case USB_CHG_TYPE_C:
+		port->psy_type = POWER_SUPPLY_TYPE_USB_TYPE_C;
+		break;
+	case USB_CHG_TYPE_BC12_DCP:
+		port->psy_type = POWER_SUPPLY_TYPE_USB_DCP;
+		break;
+	case USB_CHG_TYPE_BC12_CDP:
+		port->psy_type = POWER_SUPPLY_TYPE_USB_CDP;
+		break;
+	case USB_CHG_TYPE_PD:
+		if (resp.dualrole)
+			port->psy_type = POWER_SUPPLY_TYPE_USB_PD_DRP;
+		else
+			port->psy_type = POWER_SUPPLY_TYPE_USB_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. Treat this case as a dedicated charger since we
+		 * don't know any better just yet. Additionally since the power
+		 * capabilities are unknown, report the max current and voltage
+		 * as zero.
+		 */
+		port->psy_type = POWER_SUPPLY_TYPE_MAINS;
+		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_type = POWER_SUPPLY_TYPE_USB;
+	}
+
+	port->psy_desc.type = port->psy_type;
+
+	dev_dbg(dev,
+		"Port %d: %s type=%d=vmax=%d vnow=%d cmax=%d clim=%d pmax=%d\n",
+		port->port_number, role_str, 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_type != port->psy_type ||
+	    last_psy_status != port->psy_status) {
+		dev_dbg(dev, "Port %d: Calling power_supply_changed\n",
+			port->port_number);
+		power_supply_changed(port->psy);
+	}
+
+	return 0;
+}
+
+static int get_ec_port_status(struct port_data *port, bool ratelimit)
+{
+	int ret;
+
+	if (ratelimit &&
+	    time_is_after_jiffies(port->last_update +
+				  CROS_USB_PD_CACHE_UPDATE_DELAY))
+		return 0;
+
+	ret = get_ec_usb_pd_power_info(port);
+	if (ret < 0)
+		return ret;
+
+	ret = get_ec_usb_pd_discovery_info(port);
+	port->last_update = jiffies;
+
+	return ret;
+}
+
+static void cros_usb_pd_charger_power_changed(struct power_supply *psy)
+{
+	struct port_data *port = power_supply_get_drvdata(psy);
+	struct charger_data *charger = port->charger;
+	struct device *dev = charger->dev;
+	int i;
+
+	dev_dbg(dev, "cros_usb_pd_charger_power_changed\n");
+	for (i = 0; i < charger->num_registered_psy; i++)
+		get_ec_port_status(charger->ports[i], false);
+}
+
+static int cros_usb_pd_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 device *dev = charger->dev;
+	int ret;
+
+
+	/* Only refresh ec_port_status for dynamic properties */
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CURRENT_MAX:
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
+		ret = get_ec_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_CHARGE_CONTROL_LIMIT_MAX:
+		/* TODO: send a TBD host command to the EC. */
+		val->intval = 0;
+		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_usb_pd_charger_set_prop(struct power_supply *psy,
+				    enum power_supply_property psp,
+				    const union power_supply_propval *val)
+{
+	struct port_data *port = power_supply_get_drvdata(psy);
+	struct charger_data *charger = port->charger;
+	int port_number;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
+		/*
+		 * A value of -1 implies switching to battery as the power
+		 * source. Any other value implies using this port as the
+		 * power source.
+		 */
+		port_number = val->intval;
+		if (port_number != -1)
+			port_number = port->port_number;
+		return set_ec_usb_pd_override_ports(charger, port_number);
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int cros_usb_pd_charger_is_writeable(struct power_supply *psy,
+		enum power_supply_property psp)
+{
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
+		ret = 1;
+		break;
+	default:
+		ret = 0;
+	}
+
+	return ret;
+}
+
+static void cros_usb_pd_print_log_entry(struct ec_response_pd_log *r,
+					ktime_t tstamp)
+{
+	static const char * const fault_names[] = {
+		"---", "OCP", "fast OCP", "OVP", "Discharge"
+	};
+	static const char * const role_names[] = {
+		"Disconnected", "SRC", "SNK", "SNK (not charging)"
+	};
+	static const char * const chg_type_names[] = {
+		"None", "PD", "Type-C", "Proprietary",
+		"DCP", "CDP", "SDP", "Other", "VBUS"
+	};
+	int i;
+	int role_idx, type_idx;
+	const char *fault, *role, *chg_type;
+	struct usb_chg_measures *meas;
+	struct mcdp_info *minfo;
+	struct rtc_time rt;
+	u64 msecs;
+	int len = 0;
+	char buf[BUF_SIZE + 1];
+
+	/* the timestamp is the number of 1024th of seconds in the past */
+	tstamp = ktime_sub_us(tstamp,
+		 (uint64_t)r->timestamp_ms << PD_LOG_TIMESTAMP_SHIFT);
+	rt = rtc_ktime_to_tm(tstamp);
+
+	switch (r->type) {
+	case PD_EVENT_MCU_CHARGE:
+		if (r->data & CHARGE_FLAGS_OVERRIDE)
+			APPEND_STRING(buf, len, "override ");
+		if (r->data & CHARGE_FLAGS_DELAYED_OVERRIDE)
+			APPEND_STRING(buf, len, "pending_override ");
+		role_idx = r->data & CHARGE_FLAGS_ROLE_MASK;
+		role = role_idx < ARRAY_SIZE(role_names) ?
+			role_names[role_idx] : "Unknown";
+		type_idx = (r->data & CHARGE_FLAGS_TYPE_MASK)
+			 >> CHARGE_FLAGS_TYPE_SHIFT;
+		chg_type = type_idx < ARRAY_SIZE(chg_type_names) ?
+			chg_type_names[type_idx] : "???";
+
+		if ((role_idx == USB_PD_PORT_POWER_DISCONNECTED) ||
+		    (role_idx == USB_PD_PORT_POWER_SOURCE)) {
+			APPEND_STRING(buf, len, "%s", role);
+			break;
+		}
+
+		meas = (struct usb_chg_measures *)r->payload;
+		APPEND_STRING(buf, len, "%s %s %s %dmV max %dmV / %dmA", role,
+			r->data & CHARGE_FLAGS_DUAL_ROLE ? "DRP" : "Charger",
+			chg_type,
+			meas->voltage_now,
+			meas->voltage_max,
+			meas->current_max);
+		break;
+	case PD_EVENT_ACC_RW_FAIL:
+		APPEND_STRING(buf, len, "RW signature check failed");
+		break;
+	case PD_EVENT_PS_FAULT:
+		fault = r->data < ARRAY_SIZE(fault_names) ? fault_names[r->data]
+							  : "???";
+		APPEND_STRING(buf, len, "Power supply fault: %s", fault);
+		break;
+	case PD_EVENT_VIDEO_DP_MODE:
+		APPEND_STRING(buf, len, "DP mode %sabled",
+			      (r->data == 1) ? "en" : "dis");
+		break;
+	case PD_EVENT_VIDEO_CODEC:
+		minfo = (struct mcdp_info *)r->payload;
+		APPEND_STRING(buf, len,
+			      "HDMI info: family:%04x chipid:%04x irom:%d.%d.%d fw:%d.%d.%d",
+			      MCDP_FAMILY(minfo->family),
+			      MCDP_CHIPID(minfo->chipid),
+			      minfo->irom.major, minfo->irom.minor,
+			      minfo->irom.build, minfo->fw.major,
+			      minfo->fw.minor, minfo->fw.build);
+		break;
+	default:
+		APPEND_STRING(buf, len,
+			"Event %02x (%04x) [", r->type, r->data);
+		for (i = 0; i < PD_LOG_SIZE(r->size_port); i++)
+			APPEND_STRING(buf, len, "%02x ", r->payload[i]);
+		APPEND_STRING(buf, len, "]");
+		break;
+	}
+
+	msecs = ktime_to_ms(tstamp);
+	do_div(msecs, MSEC_PER_SEC);
+	pr_info("PDLOG %d/%02d/%02d %02d:%02d:%02d.%03llu P%d %s\n",
+		rt.tm_year + 1900, rt.tm_mon + 1, rt.tm_mday,
+		rt.tm_hour, rt.tm_min, rt.tm_sec, msecs,
+		PD_LOG_PORT(r->size_port), buf);
+}
+
+static void cros_usb_pd_log_check(struct work_struct *work)
+{
+	struct charger_data *charger = container_of(to_delayed_work(work),
+		struct charger_data, log_work);
+	struct device *dev = charger->dev;
+	union {
+		struct ec_response_pd_log r;
+		uint32_t words[8]; /* space for the payload */
+	} u;
+	int ret;
+	int entries = 0;
+	ktime_t now;
+
+	if (charger->suspended) {
+		dev_dbg(dev, "Suspended...bailing out\n");
+		return;
+	}
+
+	while (entries++ < CROS_USB_PD_MAX_LOG_ENTRIES) {
+		ret = ec_command(charger->ec_dev, 0, EC_CMD_PD_GET_LOG_ENTRY,
+				 NULL, 0, (uint8_t *)&u, sizeof(u));
+		now = ktime_get_real();
+		if (ret < 0) {
+			dev_dbg(dev, "Cannot get PD log %d\n", ret);
+			break;
+		}
+		if (u.r.type == PD_EVENT_NO_ENTRY)
+			break;
+
+		cros_usb_pd_print_log_entry(&u.r, now);
+	}
+
+	queue_delayed_work(charger->log_workqueue, &charger->log_work,
+		CROS_USB_PD_LOG_UPDATE_DELAY);
+}
+
+static int cros_usb_pd_ec_event(struct notifier_block *nb,
+	unsigned long queued_during_suspend, void *_notify)
+{
+	struct charger_data *charger;
+	struct device *dev;
+	struct cros_ec_device *ec_device;
+	u32 host_event;
+
+	charger = container_of(nb, struct charger_data, notifier);
+	dev = charger->dev;
+	ec_device = charger->ec_device;
+
+	host_event = cros_ec_get_host_event(ec_device);
+	if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
+		cros_usb_pd_charger_power_changed(charger->ports[0]->psy);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static char *charger_supplied_to[] = {"cros-usb_pd-charger"};
+
+static int cros_usb_pd_charger_probe(struct platform_device *pd)
+{
+	struct device *dev = &pd->dev;
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
+	struct cros_ec_device *ec_device;
+	struct charger_data *charger;
+	struct port_data *port;
+	struct power_supply_desc *psy_desc;
+	struct power_supply_config psy_cfg = {};
+	int i;
+	int ret = -EINVAL;
+
+	dev_dbg(dev, "cros_usb_pd_charger_probe\n");
+	if (!ec_dev) {
+		WARN(1, "%s: No EC dev found\n", dev_name(dev));
+		return -EINVAL;
+	}
+
+	ec_device = ec_dev->ec_dev;
+	if (!ec_device) {
+		WARN(1, "%s: No EC device found\n", dev_name(dev));
+		return -EINVAL;
+	}
+
+	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);
+
+	if ((get_ec_num_ports(charger, &charger->num_charger_ports) < 0) ||
+	    !charger->num_charger_ports) {
+		dev_err(dev, "No charging ports found\n");
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	for (i = 0; i < charger->num_charger_ports; i++) {
+		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_usb_pd_charger_get_prop;
+		psy_desc->set_property = cros_usb_pd_charger_set_prop;
+		psy_desc->property_is_writeable =
+			cros_usb_pd_charger_is_writeable;
+		psy_desc->external_power_changed =
+			cros_usb_pd_charger_power_changed;
+		psy_desc->properties = cros_usb_pd_charger_props;
+		psy_desc->num_properties = ARRAY_SIZE(
+			cros_usb_pd_charger_props);
+
+		psy_cfg.drv_data = port;
+		psy_cfg.supplied_to = charger_supplied_to;
+		psy_cfg.num_supplicants = ARRAY_SIZE(charger_supplied_to);
+
+		port->psy = power_supply_register_no_ws(dev, psy_desc,
+							&psy_cfg);
+		if (IS_ERR(port->psy)) {
+			dev_err(dev, "Failed to register power supply: %ld\n",
+				PTR_ERR(port->psy));
+			continue;
+		}
+
+		charger->ports[charger->num_registered_psy++] = port;
+	}
+
+	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_usb_pd_ec_event;
+		ret = blocking_notifier_chain_register(
+			&ec_device->event_notifier, &charger->notifier);
+		if (ret < 0)
+			dev_warn(dev, "failed to register notifier\n");
+	}
+
+	/* Retrieve PD event logs periodically */
+	INIT_DELAYED_WORK(&charger->log_work, cros_usb_pd_log_check);
+	charger->log_workqueue =
+		create_singlethread_workqueue("cros_usb_pd_log");
+	queue_delayed_work(charger->log_workqueue, &charger->log_work,
+		CROS_USB_PD_LOG_UPDATE_DELAY);
+
+	return 0;
+
+fail:
+	for (i = 0; i < charger->num_registered_psy; i++) {
+		port = charger->ports[i];
+		power_supply_unregister(port->psy);
+		devm_kfree(dev, port);
+	}
+	platform_set_drvdata(pd, NULL);
+
+	return ret;
+}
+
+static int cros_usb_pd_charger_remove(struct platform_device *pd)
+{
+	struct charger_data *charger = platform_get_drvdata(pd);
+	struct port_data *port;
+	int i;
+
+	if (charger->notifier.notifier_call)
+		blocking_notifier_chain_unregister(
+			&charger->ec_device->event_notifier,
+			&charger->notifier);
+
+	for (i = 0; i < charger->num_registered_psy; i++) {
+		port = charger->ports[i];
+		power_supply_unregister(port->psy);
+	}
+	flush_delayed_work(&charger->log_work);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int cros_usb_pd_charger_resume(struct device *dev)
+{
+	struct charger_data *charger = dev_get_drvdata(dev);
+	int i;
+
+	if (!charger)
+		return 0;
+
+	charger->suspended = false;
+
+	dev_dbg(dev, "cros_usb_pd_charger_resume: updating power supplies\n");
+	for (i = 0; i < charger->num_registered_psy; i++) {
+		power_supply_changed(charger->ports[i]->psy);
+		charger->ports[i]->last_update =
+				jiffies - CROS_USB_PD_CACHE_UPDATE_DELAY;
+	}
+	queue_delayed_work(charger->log_workqueue, &charger->log_work,
+		CROS_USB_PD_LOG_UPDATE_DELAY);
+
+	return 0;
+}
+
+static int cros_usb_pd_charger_suspend(struct device *dev)
+{
+	struct charger_data *charger = dev_get_drvdata(dev);
+
+	charger->suspended = true;
+
+	flush_delayed_work(&charger->log_work);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(cros_usb_pd_charger_pm_ops,
+	cros_usb_pd_charger_suspend, cros_usb_pd_charger_resume);
+
+static struct platform_driver cros_usb_pd_charger_driver = {
+	.driver = {
+		.name = "cros-usb-pd-charger",
+		.pm = &cros_usb_pd_charger_pm_ops,
+	},
+	.probe = cros_usb_pd_charger_probe,
+	.remove = cros_usb_pd_charger_remove,
+};
+
+module_platform_driver(cros_usb_pd_charger_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Chrome USB PD charger");
+MODULE_ALIAS("power_supply:cros-usb-pd-charger");
-- 
2.5.5

Re: [PATCH v8 6/7] power: cros_usbpd-charger: Add EC-based USB PD charger driver

[Tomeu Vizoso]

Hi Sebastian,

is there any chance that you could review this patch?

Thanks,

Tomeu

On 12 April 2016 at 14:32, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> From: Sameer Nanda <snanda@chromium.org>
>
> This driver exposes the charger functionality in the PD EC to userspace.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Sameer Nanda <snanda@chromium.org>
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Shawn Nematbakhsh <shawnn@chromium.org>
> ---
>
> Changes in v8: None
> Changes in v7: None
> Changes in v6: None
> Changes in v5:
> - Fix type of variable passed to do_div.
>
> Changes in v4:
> - Declare size parameters in ec_command as size_t
>
> Changes in v3:
> - Use do_div so it builds on 32bit (suggested by 0-day kbuild bot).
> - Remove sysfs attributes ext_current_lim and ext_voltage_lim because
>   I don't know yet where they should be placed (and don't have HW to
>   test them).
> - Remove superfluous pre-allocated buffers ec_inbuf and ec_outbuf.
> - Lots of misc comments from Stephen Boyd were addressed.
> - Unregister notification listener in .remove callback.
>
> Changes in v2:
> - Split out changes to cros_ec_commands.h and the helpers added to
>   mfd/cros_ec.h from the patch that adds the charger driver, as
>   suggested by Lee.
> - Actually call get_ec_num_ports.
>
>  drivers/power/Kconfig              |  10 +
>  drivers/power/Makefile             |   1 +
>  drivers/power/cros_usbpd-charger.c | 780 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 791 insertions(+)
>  create mode 100644 drivers/power/cros_usbpd-charger.c
>
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index 421770ddafa3..d096e76ed822 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -509,6 +509,16 @@ config AXP20X_POWER
>           This driver provides support for the power supply features of
>           AXP20x PMIC.
>
> +config CHARGER_CROS_USB_PD
> +       tristate "Chrome OS EC based USB PD charger"
> +       depends on MFD_CROS_EC
> +       default y
> +       help
> +         Say Y here to enable Chrome OS EC based USB PD 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
>
>  source "drivers/power/reset/Kconfig"
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index e46b75d448a5..c9eca1f9bfb0 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65217)        += tps65217_charger.o
>  obj-$(CONFIG_POWER_RESET)      += reset/
>  obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
>  obj-$(CONFIG_AXP288_CHARGER)   += axp288_charger.o
> +obj-$(CONFIG_CHARGER_CROS_USB_PD)      += cros_usbpd-charger.o
> diff --git a/drivers/power/cros_usbpd-charger.c b/drivers/power/cros_usbpd-charger.c
> new file mode 100644
> index 000000000000..e7366660c093
> --- /dev/null
> +++ b/drivers/power/cros_usbpd-charger.c
> @@ -0,0 +1,780 @@
> +/*
> + * Power supply driver for ChromeOS EC based USB PD Charger.
> + *
> + * Copyright (c) 2014 Google, Inc
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/ktime.h>
> +#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/rtc.h>
> +#include <linux/slab.h>
> +
> +#define CROS_USB_PD_MAX_PORTS          8
> +#define CROS_USB_PD_MAX_LOG_ENTRIES    30
> +
> +#define CROS_USB_PD_LOG_UPDATE_DELAY msecs_to_jiffies(60000)
> +#define CROS_USB_PD_CACHE_UPDATE_DELAY msecs_to_jiffies(500)
> +
> +/* Buffer + macro for building PDLOG string */
> +#define BUF_SIZE 80
> +#define APPEND_STRING(buf, len, str, ...) ((len) += \
> +       snprintf((buf) + (len), max(BUF_SIZE - (len), 0), (str), ##__VA_ARGS__))
> +
> +#define CHARGER_DIR_NAME               "CROS_USB_PD_CHARGER%d"
> +#define CHARGER_DIR_NAME_LENGTH                sizeof(CHARGER_DIR_NAME)
> +
> +#define MANUFACTURER_MODEL_LENGTH      32
> +
> +struct port_data {
> +       int port_number;
> +       char name[CHARGER_DIR_NAME_LENGTH];
> +       char manufacturer[MANUFACTURER_MODEL_LENGTH];
> +       char model_name[MANUFACTURER_MODEL_LENGTH];
> +       struct power_supply *psy;
> +       struct power_supply_desc psy_desc;
> +       int psy_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[CROS_USB_PD_MAX_PORTS];
> +       struct delayed_work log_work;
> +       struct workqueue_struct *log_workqueue;
> +       struct notifier_block notifier;
> +       bool suspended;
> +};
> +
> +static enum power_supply_property cros_usb_pd_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_CHARGE_CONTROL_LIMIT_MAX,
> +       POWER_SUPPLY_PROP_MODEL_NAME,
> +       POWER_SUPPLY_PROP_MANUFACTURER,
> +};
> +
> +static int ec_command(struct cros_ec_dev *ec_dev, int version, int command,
> +                     uint8_t *outdata, size_t outsize, uint8_t *indata,
> +                     size_t insize)
> +{
> +       struct cros_ec_device *ec_device = ec_dev->ec_dev;
> +       struct cros_ec_command *msg;
> +       int ret;
> +
> +       msg = kmalloc(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;
> +
> +       memcpy(msg->data, outdata, outsize);
> +       ret = cros_ec_cmd_xfer_status(ec_device, msg);
> +       memcpy(indata, msg->data, insize);
> +
> +       kfree(msg);
> +
> +       return ret;
> +}
> +
> +static int set_ec_usb_pd_override_ports(struct charger_data *charger,
> +                                       int port_num)
> +{
> +       struct device *dev = charger->dev;
> +       struct ec_params_charge_port_override req;
> +       int ret;
> +
> +       req.override_port = port_num;
> +
> +       ret = ec_command(charger->ec_dev, 0, EC_CMD_PD_CHARGE_PORT_OVERRIDE,
> +                        (uint8_t *)&req, sizeof(req),
> +                        NULL, 0);
> +       if (ret < 0) {
> +               dev_err(dev, "Port Override command returned 0x%x\n", ret);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int get_ec_num_ports(struct charger_data *charger, int *num_ports)
> +{
> +       struct device *dev = charger->dev;
> +       struct ec_response_usb_pd_ports resp;
> +       int ret;
> +
> +       *num_ports = 0;
> +       ret = ec_command(charger->ec_dev, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
> +                        (uint8_t *)&resp, sizeof(resp));
> +       if (ret < 0) {
> +               dev_err(dev, "Unable to query PD ports (err:0x%x)\n", ret);
> +               return ret;
> +       }
> +       *num_ports = resp.num_ports;
> +       dev_dbg(dev, "Num ports = %d\n", *num_ports);
> +
> +       return 0;
> +}
> +
> +static int get_ec_usb_pd_discovery_info(struct port_data *port)
> +{
> +       struct charger_data *charger = port->charger;
> +       struct device *dev = charger->dev;
> +       struct ec_params_usb_pd_info_request req;
> +       struct ec_params_usb_pd_discovery_entry resp;
> +       int ret;
> +
> +       req.port = port->port_number;
> +
> +       ret = ec_command(charger->ec_dev, 0, EC_CMD_USB_PD_DISCOVERY,
> +                        (uint8_t *)&req, sizeof(req),
> +                        (uint8_t *)&resp, sizeof(resp));
> +       if (ret < 0) {
> +               dev_err(dev, "Unable to query Discovery info (err:0x%x)\n",
> +                        ret);
> +               return -EINVAL;
> +       }
> +
> +       dev_dbg(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, MANUFACTURER_MODEL_LENGTH, "%x", resp.vid);
> +       snprintf(port->model_name, MANUFACTURER_MODEL_LENGTH, "%x", resp.pid);
> +
> +       return 0;
> +}
> +
> +static int get_ec_usb_pd_power_info(struct port_data *port)
> +{
> +       struct charger_data *charger = port->charger;
> +       struct device *dev = charger->dev;
> +       struct ec_params_usb_pd_power_info req;
> +       struct ec_response_usb_pd_power_info resp;
> +       char role_str[80];
> +       int ret, last_psy_status, last_psy_type;
> +
> +       req.port = port->port_number;
> +       ret = ec_command(charger->ec_dev, 0, EC_CMD_USB_PD_POWER_INFO,
> +                        (uint8_t *)&req, sizeof(req),
> +                        (uint8_t *)&resp, sizeof(resp));
> +       if (ret < 0) {
> +               dev_err(dev, "Unable to query PD power info (err:0x%x)\n", ret);
> +               return -EINVAL;
> +       }
> +
> +       last_psy_status = port->psy_status;
> +       last_psy_type = port->psy_type;
> +
> +       switch (resp.role) {
> +       case USB_PD_PORT_POWER_DISCONNECTED:
> +               snprintf(role_str, sizeof(role_str), "%s", "DISCONNECTED");
> +               port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +               port->psy_online = 0;
> +               break;
> +       case USB_PD_PORT_POWER_SOURCE:
> +               snprintf(role_str, sizeof(role_str), "%s", "SOURCE");
> +               port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +               port->psy_online = 0;
> +               break;
> +       case USB_PD_PORT_POWER_SINK:
> +               snprintf(role_str, sizeof(role_str), "%s", "SINK");
> +               port->psy_status = POWER_SUPPLY_STATUS_CHARGING;
> +               port->psy_online = 1;
> +               break;
> +       case USB_PD_PORT_POWER_SINK_NOT_CHARGING:
> +               snprintf(role_str, sizeof(role_str), "%s", "NOT CHARGING");
> +               port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> +               port->psy_online = 1;
> +               break;
> +       default:
> +               snprintf(role_str, sizeof(role_str), "%s", "UNKNOWN");
> +               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_type = POWER_SUPPLY_TYPE_USB;
> +               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_type = POWER_SUPPLY_TYPE_USB_PD_DRP;
> +               else
> +                       port->psy_type = POWER_SUPPLY_TYPE_USB;
> +               break;
> +       case USB_CHG_TYPE_PROPRIETARY:
> +               port->psy_type = POWER_SUPPLY_TYPE_MAINS;
> +               break;
> +       case USB_CHG_TYPE_C:
> +               port->psy_type = POWER_SUPPLY_TYPE_USB_TYPE_C;
> +               break;
> +       case USB_CHG_TYPE_BC12_DCP:
> +               port->psy_type = POWER_SUPPLY_TYPE_USB_DCP;
> +               break;
> +       case USB_CHG_TYPE_BC12_CDP:
> +               port->psy_type = POWER_SUPPLY_TYPE_USB_CDP;
> +               break;
> +       case USB_CHG_TYPE_PD:
> +               if (resp.dualrole)
> +                       port->psy_type = POWER_SUPPLY_TYPE_USB_PD_DRP;
> +               else
> +                       port->psy_type = POWER_SUPPLY_TYPE_USB_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. Treat this case as a dedicated charger since we
> +                * don't know any better just yet. Additionally since the power
> +                * capabilities are unknown, report the max current and voltage
> +                * as zero.
> +                */
> +               port->psy_type = POWER_SUPPLY_TYPE_MAINS;
> +               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_type = POWER_SUPPLY_TYPE_USB;
> +       }
> +
> +       port->psy_desc.type = port->psy_type;
> +
> +       dev_dbg(dev,
> +               "Port %d: %s type=%d=vmax=%d vnow=%d cmax=%d clim=%d pmax=%d\n",
> +               port->port_number, role_str, 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_type != port->psy_type ||
> +           last_psy_status != port->psy_status) {
> +               dev_dbg(dev, "Port %d: Calling power_supply_changed\n",
> +                       port->port_number);
> +               power_supply_changed(port->psy);
> +       }
> +
> +       return 0;
> +}
> +
> +static int get_ec_port_status(struct port_data *port, bool ratelimit)
> +{
> +       int ret;
> +
> +       if (ratelimit &&
> +           time_is_after_jiffies(port->last_update +
> +                                 CROS_USB_PD_CACHE_UPDATE_DELAY))
> +               return 0;
> +
> +       ret = get_ec_usb_pd_power_info(port);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = get_ec_usb_pd_discovery_info(port);
> +       port->last_update = jiffies;
> +
> +       return ret;
> +}
> +
> +static void cros_usb_pd_charger_power_changed(struct power_supply *psy)
> +{
> +       struct port_data *port = power_supply_get_drvdata(psy);
> +       struct charger_data *charger = port->charger;
> +       struct device *dev = charger->dev;
> +       int i;
> +
> +       dev_dbg(dev, "cros_usb_pd_charger_power_changed\n");
> +       for (i = 0; i < charger->num_registered_psy; i++)
> +               get_ec_port_status(charger->ports[i], false);
> +}
> +
> +static int cros_usb_pd_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 device *dev = charger->dev;
> +       int ret;
> +
> +
> +       /* Only refresh ec_port_status for dynamic properties */
> +       switch (psp) {
> +       case POWER_SUPPLY_PROP_CURRENT_MAX:
> +       case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> +       case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +       case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
> +               ret = get_ec_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_CHARGE_CONTROL_LIMIT_MAX:
> +               /* TODO: send a TBD host command to the EC. */
> +               val->intval = 0;
> +               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_usb_pd_charger_set_prop(struct power_supply *psy,
> +                                   enum power_supply_property psp,
> +                                   const union power_supply_propval *val)
> +{
> +       struct port_data *port = power_supply_get_drvdata(psy);
> +       struct charger_data *charger = port->charger;
> +       int port_number;
> +
> +       switch (psp) {
> +       case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
> +               /*
> +                * A value of -1 implies switching to battery as the power
> +                * source. Any other value implies using this port as the
> +                * power source.
> +                */
> +               port_number = val->intval;
> +               if (port_number != -1)
> +                       port_number = port->port_number;
> +               return set_ec_usb_pd_override_ports(charger, port_number);
> +       default:
> +               return -EINVAL;
> +       }
> +       return 0;
> +}
> +
> +static int cros_usb_pd_charger_is_writeable(struct power_supply *psy,
> +               enum power_supply_property psp)
> +{
> +       int ret;
> +
> +       switch (psp) {
> +       case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
> +               ret = 1;
> +               break;
> +       default:
> +               ret = 0;
> +       }
> +
> +       return ret;
> +}
> +
> +static void cros_usb_pd_print_log_entry(struct ec_response_pd_log *r,
> +                                       ktime_t tstamp)
> +{
> +       static const char * const fault_names[] = {
> +               "---", "OCP", "fast OCP", "OVP", "Discharge"
> +       };
> +       static const char * const role_names[] = {
> +               "Disconnected", "SRC", "SNK", "SNK (not charging)"
> +       };
> +       static const char * const chg_type_names[] = {
> +               "None", "PD", "Type-C", "Proprietary",
> +               "DCP", "CDP", "SDP", "Other", "VBUS"
> +       };
> +       int i;
> +       int role_idx, type_idx;
> +       const char *fault, *role, *chg_type;
> +       struct usb_chg_measures *meas;
> +       struct mcdp_info *minfo;
> +       struct rtc_time rt;
> +       u64 msecs;
> +       int len = 0;
> +       char buf[BUF_SIZE + 1];
> +
> +       /* the timestamp is the number of 1024th of seconds in the past */
> +       tstamp = ktime_sub_us(tstamp,
> +                (uint64_t)r->timestamp_ms << PD_LOG_TIMESTAMP_SHIFT);
> +       rt = rtc_ktime_to_tm(tstamp);
> +
> +       switch (r->type) {
> +       case PD_EVENT_MCU_CHARGE:
> +               if (r->data & CHARGE_FLAGS_OVERRIDE)
> +                       APPEND_STRING(buf, len, "override ");
> +               if (r->data & CHARGE_FLAGS_DELAYED_OVERRIDE)
> +                       APPEND_STRING(buf, len, "pending_override ");
> +               role_idx = r->data & CHARGE_FLAGS_ROLE_MASK;
> +               role = role_idx < ARRAY_SIZE(role_names) ?
> +                       role_names[role_idx] : "Unknown";
> +               type_idx = (r->data & CHARGE_FLAGS_TYPE_MASK)
> +                        >> CHARGE_FLAGS_TYPE_SHIFT;
> +               chg_type = type_idx < ARRAY_SIZE(chg_type_names) ?
> +                       chg_type_names[type_idx] : "???";
> +
> +               if ((role_idx == USB_PD_PORT_POWER_DISCONNECTED) ||
> +                   (role_idx == USB_PD_PORT_POWER_SOURCE)) {
> +                       APPEND_STRING(buf, len, "%s", role);
> +                       break;
> +               }
> +
> +               meas = (struct usb_chg_measures *)r->payload;
> +               APPEND_STRING(buf, len, "%s %s %s %dmV max %dmV / %dmA", role,
> +                       r->data & CHARGE_FLAGS_DUAL_ROLE ? "DRP" : "Charger",
> +                       chg_type,
> +                       meas->voltage_now,
> +                       meas->voltage_max,
> +                       meas->current_max);
> +               break;
> +       case PD_EVENT_ACC_RW_FAIL:
> +               APPEND_STRING(buf, len, "RW signature check failed");
> +               break;
> +       case PD_EVENT_PS_FAULT:
> +               fault = r->data < ARRAY_SIZE(fault_names) ? fault_names[r->data]
> +                                                         : "???";
> +               APPEND_STRING(buf, len, "Power supply fault: %s", fault);
> +               break;
> +       case PD_EVENT_VIDEO_DP_MODE:
> +               APPEND_STRING(buf, len, "DP mode %sabled",
> +                             (r->data == 1) ? "en" : "dis");
> +               break;
> +       case PD_EVENT_VIDEO_CODEC:
> +               minfo = (struct mcdp_info *)r->payload;
> +               APPEND_STRING(buf, len,
> +                             "HDMI info: family:%04x chipid:%04x irom:%d.%d.%d fw:%d.%d.%d",
> +                             MCDP_FAMILY(minfo->family),
> +                             MCDP_CHIPID(minfo->chipid),
> +                             minfo->irom.major, minfo->irom.minor,
> +                             minfo->irom.build, minfo->fw.major,
> +                             minfo->fw.minor, minfo->fw.build);
> +               break;
> +       default:
> +               APPEND_STRING(buf, len,
> +                       "Event %02x (%04x) [", r->type, r->data);
> +               for (i = 0; i < PD_LOG_SIZE(r->size_port); i++)
> +                       APPEND_STRING(buf, len, "%02x ", r->payload[i]);
> +               APPEND_STRING(buf, len, "]");
> +               break;
> +       }
> +
> +       msecs = ktime_to_ms(tstamp);
> +       do_div(msecs, MSEC_PER_SEC);
> +       pr_info("PDLOG %d/%02d/%02d %02d:%02d:%02d.%03llu P%d %s\n",
> +               rt.tm_year + 1900, rt.tm_mon + 1, rt.tm_mday,
> +               rt.tm_hour, rt.tm_min, rt.tm_sec, msecs,
> +               PD_LOG_PORT(r->size_port), buf);
> +}
> +
> +static void cros_usb_pd_log_check(struct work_struct *work)
> +{
> +       struct charger_data *charger = container_of(to_delayed_work(work),
> +               struct charger_data, log_work);
> +       struct device *dev = charger->dev;
> +       union {
> +               struct ec_response_pd_log r;
> +               uint32_t words[8]; /* space for the payload */
> +       } u;
> +       int ret;
> +       int entries = 0;
> +       ktime_t now;
> +
> +       if (charger->suspended) {
> +               dev_dbg(dev, "Suspended...bailing out\n");
> +               return;
> +       }
> +
> +       while (entries++ < CROS_USB_PD_MAX_LOG_ENTRIES) {
> +               ret = ec_command(charger->ec_dev, 0, EC_CMD_PD_GET_LOG_ENTRY,
> +                                NULL, 0, (uint8_t *)&u, sizeof(u));
> +               now = ktime_get_real();
> +               if (ret < 0) {
> +                       dev_dbg(dev, "Cannot get PD log %d\n", ret);
> +                       break;
> +               }
> +               if (u.r.type == PD_EVENT_NO_ENTRY)
> +                       break;
> +
> +               cros_usb_pd_print_log_entry(&u.r, now);
> +       }
> +
> +       queue_delayed_work(charger->log_workqueue, &charger->log_work,
> +               CROS_USB_PD_LOG_UPDATE_DELAY);
> +}
> +
> +static int cros_usb_pd_ec_event(struct notifier_block *nb,
> +       unsigned long queued_during_suspend, void *_notify)
> +{
> +       struct charger_data *charger;
> +       struct device *dev;
> +       struct cros_ec_device *ec_device;
> +       u32 host_event;
> +
> +       charger = container_of(nb, struct charger_data, notifier);
> +       dev = charger->dev;
> +       ec_device = charger->ec_device;
> +
> +       host_event = cros_ec_get_host_event(ec_device);
> +       if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
> +               cros_usb_pd_charger_power_changed(charger->ports[0]->psy);
> +               return NOTIFY_OK;
> +       }
> +
> +       return NOTIFY_DONE;
> +}
> +
> +static char *charger_supplied_to[] = {"cros-usb_pd-charger"};
> +
> +static int cros_usb_pd_charger_probe(struct platform_device *pd)
> +{
> +       struct device *dev = &pd->dev;
> +       struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
> +       struct cros_ec_device *ec_device;
> +       struct charger_data *charger;
> +       struct port_data *port;
> +       struct power_supply_desc *psy_desc;
> +       struct power_supply_config psy_cfg = {};
> +       int i;
> +       int ret = -EINVAL;
> +
> +       dev_dbg(dev, "cros_usb_pd_charger_probe\n");
> +       if (!ec_dev) {
> +               WARN(1, "%s: No EC dev found\n", dev_name(dev));
> +               return -EINVAL;
> +       }
> +
> +       ec_device = ec_dev->ec_dev;
> +       if (!ec_device) {
> +               WARN(1, "%s: No EC device found\n", dev_name(dev));
> +               return -EINVAL;
> +       }
> +
> +       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);
> +
> +       if ((get_ec_num_ports(charger, &charger->num_charger_ports) < 0) ||
> +           !charger->num_charger_ports) {
> +               dev_err(dev, "No charging ports found\n");
> +               ret = -ENODEV;
> +               goto fail;
> +       }
> +
> +       for (i = 0; i < charger->num_charger_ports; i++) {
> +               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_usb_pd_charger_get_prop;
> +               psy_desc->set_property = cros_usb_pd_charger_set_prop;
> +               psy_desc->property_is_writeable =
> +                       cros_usb_pd_charger_is_writeable;
> +               psy_desc->external_power_changed =
> +                       cros_usb_pd_charger_power_changed;
> +               psy_desc->properties = cros_usb_pd_charger_props;
> +               psy_desc->num_properties = ARRAY_SIZE(
> +                       cros_usb_pd_charger_props);
> +
> +               psy_cfg.drv_data = port;
> +               psy_cfg.supplied_to = charger_supplied_to;
> +               psy_cfg.num_supplicants = ARRAY_SIZE(charger_supplied_to);
> +
> +               port->psy = power_supply_register_no_ws(dev, psy_desc,
> +                                                       &psy_cfg);
> +               if (IS_ERR(port->psy)) {
> +                       dev_err(dev, "Failed to register power supply: %ld\n",
> +                               PTR_ERR(port->psy));
> +                       continue;
> +               }
> +
> +               charger->ports[charger->num_registered_psy++] = port;
> +       }
> +
> +       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_usb_pd_ec_event;
> +               ret = blocking_notifier_chain_register(
> +                       &ec_device->event_notifier, &charger->notifier);
> +               if (ret < 0)
> +                       dev_warn(dev, "failed to register notifier\n");
> +       }
> +
> +       /* Retrieve PD event logs periodically */
> +       INIT_DELAYED_WORK(&charger->log_work, cros_usb_pd_log_check);
> +       charger->log_workqueue =
> +               create_singlethread_workqueue("cros_usb_pd_log");
> +       queue_delayed_work(charger->log_workqueue, &charger->log_work,
> +               CROS_USB_PD_LOG_UPDATE_DELAY);
> +
> +       return 0;
> +
> +fail:
> +       for (i = 0; i < charger->num_registered_psy; i++) {
> +               port = charger->ports[i];
> +               power_supply_unregister(port->psy);
> +               devm_kfree(dev, port);
> +       }
> +       platform_set_drvdata(pd, NULL);
> +
> +       return ret;
> +}
> +
> +static int cros_usb_pd_charger_remove(struct platform_device *pd)
> +{
> +       struct charger_data *charger = platform_get_drvdata(pd);
> +       struct port_data *port;
> +       int i;
> +
> +       if (charger->notifier.notifier_call)
> +               blocking_notifier_chain_unregister(
> +                       &charger->ec_device->event_notifier,
> +                       &charger->notifier);
> +
> +       for (i = 0; i < charger->num_registered_psy; i++) {
> +               port = charger->ports[i];
> +               power_supply_unregister(port->psy);
> +       }
> +       flush_delayed_work(&charger->log_work);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int cros_usb_pd_charger_resume(struct device *dev)
> +{
> +       struct charger_data *charger = dev_get_drvdata(dev);
> +       int i;
> +
> +       if (!charger)
> +               return 0;
> +
> +       charger->suspended = false;
> +
> +       dev_dbg(dev, "cros_usb_pd_charger_resume: updating power supplies\n");
> +       for (i = 0; i < charger->num_registered_psy; i++) {
> +               power_supply_changed(charger->ports[i]->psy);
> +               charger->ports[i]->last_update =
> +                               jiffies - CROS_USB_PD_CACHE_UPDATE_DELAY;
> +       }
> +       queue_delayed_work(charger->log_workqueue, &charger->log_work,
> +               CROS_USB_PD_LOG_UPDATE_DELAY);
> +
> +       return 0;
> +}
> +
> +static int cros_usb_pd_charger_suspend(struct device *dev)
> +{
> +       struct charger_data *charger = dev_get_drvdata(dev);
> +
> +       charger->suspended = true;
> +
> +       flush_delayed_work(&charger->log_work);
> +
> +       return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(cros_usb_pd_charger_pm_ops,
> +       cros_usb_pd_charger_suspend, cros_usb_pd_charger_resume);
> +
> +static struct platform_driver cros_usb_pd_charger_driver = {
> +       .driver = {
> +               .name = "cros-usb-pd-charger",
> +               .pm = &cros_usb_pd_charger_pm_ops,
> +       },
> +       .probe = cros_usb_pd_charger_probe,
> +       .remove = cros_usb_pd_charger_remove,
> +};
> +
> +module_platform_driver(cros_usb_pd_charger_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Chrome USB PD charger");
> +MODULE_ALIAS("power_supply:cros-usb-pd-charger");
> --
> 2.5.5
>

Re: [PATCH v8 6/7] power: cros_usbpd-charger: Add EC-based USB PD charger driver

[Enric Balletbo i Serra]

Hi Tomeu,

Thanks for the patch, looks good, a few comments below.

On 20/04/16 09:42, Tomeu Vizoso wrote:
> Hi Sebastian,
>
> is there any chance that you could review this patch?
>
> Thanks,
>
> Tomeu
>
> On 12 April 2016 at 14:32, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> From: Sameer Nanda <snanda@chromium.org>
>>
>> This driver exposes the charger functionality in the PD EC to userspace.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> Cc: Sameer Nanda <snanda@chromium.org>
>> Cc: Benson Leung <bleung@chromium.org>
>> Cc: Shawn Nematbakhsh <shawnn@chromium.org>
>> ---
>>
>> Changes in v8: None
>> Changes in v7: None
>> Changes in v6: None
>> Changes in v5:
>> - Fix type of variable passed to do_div.
>>
>> Changes in v4:
>> - Declare size parameters in ec_command as size_t
>>
>> Changes in v3:
>> - Use do_div so it builds on 32bit (suggested by 0-day kbuild bot).
>> - Remove sysfs attributes ext_current_lim and ext_voltage_lim because
>>    I don't know yet where they should be placed (and don't have HW to
>>    test them).
>> - Remove superfluous pre-allocated buffers ec_inbuf and ec_outbuf.
>> - Lots of misc comments from Stephen Boyd were addressed.
>> - Unregister notification listener in .remove callback.
>>
>> Changes in v2:
>> - Split out changes to cros_ec_commands.h and the helpers added to
>>    mfd/cros_ec.h from the patch that adds the charger driver, as
>>    suggested by Lee.
>> - Actually call get_ec_num_ports.
>>
>>   drivers/power/Kconfig              |  10 +
>>   drivers/power/Makefile             |   1 +
>>   drivers/power/cros_usbpd-charger.c | 780 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 791 insertions(+)
>>   create mode 100644 drivers/power/cros_usbpd-charger.c
>>
>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
>> index 421770ddafa3..d096e76ed822 100644
>> --- a/drivers/power/Kconfig
>> +++ b/drivers/power/Kconfig
>> @@ -509,6 +509,16 @@ config AXP20X_POWER
>>            This driver provides support for the power supply features of
>>            AXP20x PMIC.
>>
>> +config CHARGER_CROS_USB_PD
>> +       tristate "Chrome OS EC based USB PD charger"
>> +       depends on MFD_CROS_EC
>> +       default y

Guess you should not set default to yes here.

>> +       help
>> +         Say Y here to enable Chrome OS EC based USB PD 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
>>
>>   source "drivers/power/reset/Kconfig"
>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>> index e46b75d448a5..c9eca1f9bfb0 100644
>> --- a/drivers/power/Makefile
>> +++ b/drivers/power/Makefile
>> @@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65217)        += tps65217_charger.o
>>   obj-$(CONFIG_POWER_RESET)      += reset/
>>   obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
>>   obj-$(CONFIG_AXP288_CHARGER)   += axp288_charger.o
>> +obj-$(CONFIG_CHARGER_CROS_USB_PD)      += cros_usbpd-charger.o
>> diff --git a/drivers/power/cros_usbpd-charger.c b/drivers/power/cros_usbpd-charger.c
>> new file mode 100644
>> index 000000000000..e7366660c093
>> --- /dev/null
>> +++ b/drivers/power/cros_usbpd-charger.c
>> @@ -0,0 +1,780 @@
>> +/*
>> + * Power supply driver for ChromeOS EC based USB PD Charger.
>> + *
>> + * Copyright (c) 2014 Google, Inc
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/ktime.h>
>> +#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/rtc.h>
>> +#include <linux/slab.h>
>> +
>> +#define CROS_USB_PD_MAX_PORTS          8
>> +#define CROS_USB_PD_MAX_LOG_ENTRIES    30
>> +
>> +#define CROS_USB_PD_LOG_UPDATE_DELAY msecs_to_jiffies(60000)
>> +#define CROS_USB_PD_CACHE_UPDATE_DELAY msecs_to_jiffies(500)
>> +
>> +/* Buffer + macro for building PDLOG string */
>> +#define BUF_SIZE 80
>> +#define APPEND_STRING(buf, len, str, ...) ((len) += \
>> +       snprintf((buf) + (len), max(BUF_SIZE - (len), 0), (str), ##__VA_ARGS__))
>> +
>> +#define CHARGER_DIR_NAME               "CROS_USB_PD_CHARGER%d"
>> +#define CHARGER_DIR_NAME_LENGTH                sizeof(CHARGER_DIR_NAME)
>> +
>> +#define MANUFACTURER_MODEL_LENGTH      32
>> +
>> +struct port_data {
>> +       int port_number;
>> +       char name[CHARGER_DIR_NAME_LENGTH];
>> +       char manufacturer[MANUFACTURER_MODEL_LENGTH];
>> +       char model_name[MANUFACTURER_MODEL_LENGTH];
>> +       struct power_supply *psy;
>> +       struct power_supply_desc psy_desc;
>> +       int psy_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[CROS_USB_PD_MAX_PORTS];
>> +       struct delayed_work log_work;
>> +       struct workqueue_struct *log_workqueue;
>> +       struct notifier_block notifier;
>> +       bool suspended;
>> +};
>> +
>> +static enum power_supply_property cros_usb_pd_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_CHARGE_CONTROL_LIMIT_MAX,
>> +       POWER_SUPPLY_PROP_MODEL_NAME,
>> +       POWER_SUPPLY_PROP_MANUFACTURER,
>> +};
>> +
>> +static int ec_command(struct cros_ec_dev *ec_dev, int version, int command,
>> +                     uint8_t *outdata, size_t outsize, uint8_t *indata,
>> +                     size_t insize)
>> +{
>> +       struct cros_ec_device *ec_device = ec_dev->ec_dev;
>> +       struct cros_ec_command *msg;
>> +       int ret;
>> +
>> +       msg = kmalloc(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;
>> +
>> +       memcpy(msg->data, outdata, outsize);
>> +       ret = cros_ec_cmd_xfer_status(ec_device, msg);
>> +       memcpy(indata, msg->data, insize);
>> +
>> +       kfree(msg);
>> +
>> +       return ret;
>> +}
>> +
>> +static int set_ec_usb_pd_override_ports(struct charger_data *charger,
>> +                                       int port_num)
>> +{
>> +       struct device *dev = charger->dev;
>> +       struct ec_params_charge_port_override req;
>> +       int ret;
>> +
>> +       req.override_port = port_num;
>> +
>> +       ret = ec_command(charger->ec_dev, 0, EC_CMD_PD_CHARGE_PORT_OVERRIDE,
>> +                        (uint8_t *)&req, sizeof(req),
>> +                        NULL, 0);
>> +       if (ret < 0) {
>> +               dev_err(dev, "Port Override command returned 0x%x\n", ret);
>> +               return -EINVAL;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int get_ec_num_ports(struct charger_data *charger, int *num_ports)
>> +{
>> +       struct device *dev = charger->dev;
>> +       struct ec_response_usb_pd_ports resp;
>> +       int ret;
>> +
>> +       *num_ports = 0;
>> +       ret = ec_command(charger->ec_dev, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
>> +                        (uint8_t *)&resp, sizeof(resp));
>> +       if (ret < 0) {
>> +               dev_err(dev, "Unable to query PD ports (err:0x%x)\n", ret);
>> +               return ret;

Generally you return -EINVAL instead of ret when ec_command fails, any 
reason why here is different?

>> +       }
>> +       *num_ports = resp.num_ports;
>> +       dev_dbg(dev, "Num ports = %d\n", *num_ports);
>> +
>> +       return 0;
>> +}
>> +
>> +static int get_ec_usb_pd_discovery_info(struct port_data *port)
>> +{
>> +       struct charger_data *charger = port->charger;
>> +       struct device *dev = charger->dev;
>> +       struct ec_params_usb_pd_info_request req;
>> +       struct ec_params_usb_pd_discovery_entry resp;
>> +       int ret;
>> +
>> +       req.port = port->port_number;
>> +
>> +       ret = ec_command(charger->ec_dev, 0, EC_CMD_USB_PD_DISCOVERY,
>> +                        (uint8_t *)&req, sizeof(req),
>> +                        (uint8_t *)&resp, sizeof(resp));
>> +       if (ret < 0) {
>> +               dev_err(dev, "Unable to query Discovery info (err:0x%x)\n",
>> +                        ret);
>> +               return -EINVAL;
>> +       }
>> +
>> +       dev_dbg(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, MANUFACTURER_MODEL_LENGTH, "%x", resp.vid);

This looks a vendor id code, generally I saw this propietry show the 
manufacturer name.

>> +       snprintf(port->model_name, MANUFACTURER_MODEL_LENGTH, "%x", resp.pid);
>> +
>> +       return 0;
>> +}
>> +
>> +static int get_ec_usb_pd_power_info(struct port_data *port)
>> +{
>> +       struct charger_data *charger = port->charger;
>> +       struct device *dev = charger->dev;
>> +       struct ec_params_usb_pd_power_info req;
>> +       struct ec_response_usb_pd_power_info resp;
>> +       char role_str[80];
>> +       int ret, last_psy_status, last_psy_type;
>> +
>> +       req.port = port->port_number;
>> +       ret = ec_command(charger->ec_dev, 0, EC_CMD_USB_PD_POWER_INFO,
>> +                        (uint8_t *)&req, sizeof(req),
>> +                        (uint8_t *)&resp, sizeof(resp));
>> +       if (ret < 0) {
>> +               dev_err(dev, "Unable to query PD power info (err:0x%x)\n", ret);
>> +               return -EINVAL;
>> +       }
>> +
>> +       last_psy_status = port->psy_status;
>> +       last_psy_type = port->psy_type;
>> +
>> +       switch (resp.role) {
>> +       case USB_PD_PORT_POWER_DISCONNECTED:
>> +               snprintf(role_str, sizeof(role_str), "%s", "DISCONNECTED");
>> +               port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
>> +               port->psy_online = 0;
>> +               break;
>> +       case USB_PD_PORT_POWER_SOURCE:
>> +               snprintf(role_str, sizeof(role_str), "%s", "SOURCE");
>> +               port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
>> +               port->psy_online = 0;
>> +               break;
>> +       case USB_PD_PORT_POWER_SINK:
>> +               snprintf(role_str, sizeof(role_str), "%s", "SINK");
>> +               port->psy_status = POWER_SUPPLY_STATUS_CHARGING;
>> +               port->psy_online = 1;
>> +               break;
>> +       case USB_PD_PORT_POWER_SINK_NOT_CHARGING:
>> +               snprintf(role_str, sizeof(role_str), "%s", "NOT CHARGING");
>> +               port->psy_status = POWER_SUPPLY_STATUS_NOT_CHARGING;
>> +               port->psy_online = 1;
>> +               break;
>> +       default:
>> +               snprintf(role_str, sizeof(role_str), "%s", "UNKNOWN");
>> +               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_type = POWER_SUPPLY_TYPE_USB;
>> +               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_type = POWER_SUPPLY_TYPE_USB_PD_DRP;
>> +               else
>> +                       port->psy_type = POWER_SUPPLY_TYPE_USB;
>> +               break;
>> +       case USB_CHG_TYPE_PROPRIETARY:
>> +               port->psy_type = POWER_SUPPLY_TYPE_MAINS;
>> +               break;
>> +       case USB_CHG_TYPE_C:
>> +               port->psy_type = POWER_SUPPLY_TYPE_USB_TYPE_C;
>> +               break;
>> +       case USB_CHG_TYPE_BC12_DCP:
>> +               port->psy_type = POWER_SUPPLY_TYPE_USB_DCP;
>> +               break;
>> +       case USB_CHG_TYPE_BC12_CDP:
>> +               port->psy_type = POWER_SUPPLY_TYPE_USB_CDP;
>> +               break;
>> +       case USB_CHG_TYPE_PD:
>> +               if (resp.dualrole)
>> +                       port->psy_type = POWER_SUPPLY_TYPE_USB_PD_DRP;
>> +               else
>> +                       port->psy_type = POWER_SUPPLY_TYPE_USB_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. Treat this case as a dedicated charger since we
>> +                * don't know any better just yet. Additionally since the power
>> +                * capabilities are unknown, report the max current and voltage
>> +                * as zero.
>> +                */
>> +               port->psy_type = POWER_SUPPLY_TYPE_MAINS;
>> +               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_type = POWER_SUPPLY_TYPE_USB;
>> +       }
>> +
>> +       port->psy_desc.type = port->psy_type;
>> +
>> +       dev_dbg(dev,
>> +               "Port %d: %s type=%d=vmax=%d vnow=%d cmax=%d clim=%d pmax=%d\n",
>> +               port->port_number, role_str, 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_type != port->psy_type ||
>> +           last_psy_status != port->psy_status) {
>> +               dev_dbg(dev, "Port %d: Calling power_supply_changed\n",
>> +                       port->port_number);
>> +               power_supply_changed(port->psy);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int get_ec_port_status(struct port_data *port, bool ratelimit)
>> +{
>> +       int ret;
>> +
>> +       if (ratelimit &&
>> +           time_is_after_jiffies(port->last_update +
>> +                                 CROS_USB_PD_CACHE_UPDATE_DELAY))
>> +               return 0;
>> +
>> +       ret = get_ec_usb_pd_power_info(port);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ret = get_ec_usb_pd_discovery_info(port);
>> +       port->last_update = jiffies;
>> +
>> +       return ret;
>> +}
>> +
>> +static void cros_usb_pd_charger_power_changed(struct power_supply *psy)
>> +{
>> +       struct port_data *port = power_supply_get_drvdata(psy);
>> +       struct charger_data *charger = port->charger;
>> +       struct device *dev = charger->dev;
>> +       int i;
>> +
>> +       dev_dbg(dev, "cros_usb_pd_charger_power_changed\n");

nit: In my opinion this debug message is not needed and doesn't add any 
information. It only shows that the function is called. You can usethe 
kernel tracing tools for that.

>> +       for (i = 0; i < charger->num_registered_psy; i++)
>> +               get_ec_port_status(charger->ports[i], false);
>> +}
>> +
>> +static int cros_usb_pd_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 device *dev = charger->dev;
>> +       int ret;
>> +
>> +
>> +       /* Only refresh ec_port_status for dynamic properties */
>> +       switch (psp) {
>> +       case POWER_SUPPLY_PROP_CURRENT_MAX:
>> +       case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
>> +       case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> +       case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
>> +               ret = get_ec_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_CHARGE_CONTROL_LIMIT_MAX:
>> +               /* TODO: send a TBD host command to the EC. */
>> +               val->intval = 0;

If I'm not mistaken the function below sets the control limit max value, 
but here you always return a 0. This is confusing for me. Seems that 
this is not supported yet? So maybe is better remove the code related to 
this or store the value set in a variable meanwhile you're not able to 
ask the value to the EC.

>> +               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_usb_pd_charger_set_prop(struct power_supply *psy,
>> +                                   enum power_supply_property psp,
>> +                                   const union power_supply_propval *val)
>> +{
>> +       struct port_data *port = power_supply_get_drvdata(psy);
>> +       struct charger_data *charger = port->charger;
>> +       int port_number;
>> +
>> +       switch (psp) {
>> +       case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
>> +               /*
>> +                * A value of -1 implies switching to battery as the power
>> +                * source. Any other value implies using this port as the
>> +                * power source.
>> +                */
>> +               port_number = val->intval;
>> +               if (port_number != -1)
>> +                       port_number = port->port_number;
>> +               return set_ec_usb_pd_override_ports(charger, port_number);
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +       return 0;
>> +}
>> +
>> +static int cros_usb_pd_charger_is_writeable(struct power_supply *psy,
>> +               enum power_supply_property psp)
>> +{
>> +       int ret;
>> +
>> +       switch (psp) {
>> +       case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
>> +               ret = 1;
>> +               break;
>> +       default:
>> +               ret = 0;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static void cros_usb_pd_print_log_entry(struct ec_response_pd_log *r,
>> +                                       ktime_t tstamp)
>> +{
>> +       static const char * const fault_names[] = {
>> +               "---", "OCP", "fast OCP", "OVP", "Discharge"
>> +       };
>> +       static const char * const role_names[] = {
>> +               "Disconnected", "SRC", "SNK", "SNK (not charging)"
>> +       };
>> +       static const char * const chg_type_names[] = {
>> +               "None", "PD", "Type-C", "Proprietary",
>> +               "DCP", "CDP", "SDP", "Other", "VBUS"
>> +       };
>> +       int i;
>> +       int role_idx, type_idx;
>> +       const char *fault, *role, *chg_type;
>> +       struct usb_chg_measures *meas;
>> +       struct mcdp_info *minfo;
>> +       struct rtc_time rt;
>> +       u64 msecs;
>> +       int len = 0;
>> +       char buf[BUF_SIZE + 1];
>> +
>> +       /* the timestamp is the number of 1024th of seconds in the past */
>> +       tstamp = ktime_sub_us(tstamp,
>> +                (uint64_t)r->timestamp_ms << PD_LOG_TIMESTAMP_SHIFT);
>> +       rt = rtc_ktime_to_tm(tstamp);
>> +
>> +       switch (r->type) {
>> +       case PD_EVENT_MCU_CHARGE:
>> +               if (r->data & CHARGE_FLAGS_OVERRIDE)
>> +                       APPEND_STRING(buf, len, "override ");
>> +               if (r->data & CHARGE_FLAGS_DELAYED_OVERRIDE)
>> +                       APPEND_STRING(buf, len, "pending_override ");
>> +               role_idx = r->data & CHARGE_FLAGS_ROLE_MASK;
>> +               role = role_idx < ARRAY_SIZE(role_names) ?
>> +                       role_names[role_idx] : "Unknown";
>> +               type_idx = (r->data & CHARGE_FLAGS_TYPE_MASK)
>> +                        >> CHARGE_FLAGS_TYPE_SHIFT;
>> +               chg_type = type_idx < ARRAY_SIZE(chg_type_names) ?
>> +                       chg_type_names[type_idx] : "???";
>> +
>> +               if ((role_idx == USB_PD_PORT_POWER_DISCONNECTED) ||
>> +                   (role_idx == USB_PD_PORT_POWER_SOURCE)) {
>> +                       APPEND_STRING(buf, len, "%s", role);
>> +                       break;
>> +               }
>> +
>> +               meas = (struct usb_chg_measures *)r->payload;
>> +               APPEND_STRING(buf, len, "%s %s %s %dmV max %dmV / %dmA", role,
>> +                       r->data & CHARGE_FLAGS_DUAL_ROLE ? "DRP" : "Charger",
>> +                       chg_type,
>> +                       meas->voltage_now,
>> +                       meas->voltage_max,
>> +                       meas->current_max);
>> +               break;
>> +       case PD_EVENT_ACC_RW_FAIL:
>> +               APPEND_STRING(buf, len, "RW signature check failed");
>> +               break;
>> +       case PD_EVENT_PS_FAULT:
>> +               fault = r->data < ARRAY_SIZE(fault_names) ? fault_names[r->data]
>> +                                                         : "???";
>> +               APPEND_STRING(buf, len, "Power supply fault: %s", fault);
>> +               break;
>> +       case PD_EVENT_VIDEO_DP_MODE:
>> +               APPEND_STRING(buf, len, "DP mode %sabled",
>> +                             (r->data == 1) ? "en" : "dis");
>> +               break;
>> +       case PD_EVENT_VIDEO_CODEC:
>> +               minfo = (struct mcdp_info *)r->payload;
>> +               APPEND_STRING(buf, len,
>> +                             "HDMI info: family:%04x chipid:%04x irom:%d.%d.%d fw:%d.%d.%d",
>> +                             MCDP_FAMILY(minfo->family),
>> +                             MCDP_CHIPID(minfo->chipid),
>> +                             minfo->irom.major, minfo->irom.minor,
>> +                             minfo->irom.build, minfo->fw.major,
>> +                             minfo->fw.minor, minfo->fw.build);
>> +               break;
>> +       default:
>> +               APPEND_STRING(buf, len,
>> +                       "Event %02x (%04x) [", r->type, r->data);
>> +               for (i = 0; i < PD_LOG_SIZE(r->size_port); i++)
>> +                       APPEND_STRING(buf, len, "%02x ", r->payload[i]);
>> +               APPEND_STRING(buf, len, "]");
>> +               break;
>> +       }
>> +
>> +       msecs = ktime_to_ms(tstamp);
>> +       do_div(msecs, MSEC_PER_SEC);
>> +       pr_info("PDLOG %d/%02d/%02d %02d:%02d:%02d.%03llu P%d %s\n",
>> +               rt.tm_year + 1900, rt.tm_mon + 1, rt.tm_mday,
>> +               rt.tm_hour, rt.tm_min, rt.tm_sec, msecs,
>> +               PD_LOG_PORT(r->size_port), buf);

nit: Maybe is better use a debug print level here. How often is this called?


>> +}
>> +
>> +static void cros_usb_pd_log_check(struct work_struct *work)
>> +{
>> +       struct charger_data *charger = container_of(to_delayed_work(work),
>> +               struct charger_data, log_work);
>> +       struct device *dev = charger->dev;
>> +       union {
>> +               struct ec_response_pd_log r;
>> +               uint32_t words[8]; /* space for the payload */
>> +       } u;
>> +       int ret;
>> +       int entries = 0;
>> +       ktime_t now;
>> +
>> +       if (charger->suspended) {
>> +               dev_dbg(dev, "Suspended...bailing out\n");
>> +               return;
>> +       }
>> +
>> +       while (entries++ < CROS_USB_PD_MAX_LOG_ENTRIES) {
>> +               ret = ec_command(charger->ec_dev, 0, EC_CMD_PD_GET_LOG_ENTRY,
>> +                                NULL, 0, (uint8_t *)&u, sizeof(u));
>> +               now = ktime_get_real();
>> +               if (ret < 0) {
>> +                       dev_dbg(dev, "Cannot get PD log %d\n", ret);
>> +                       break;
>> +               }
>> +               if (u.r.type == PD_EVENT_NO_ENTRY)
>> +                       break;
>> +
>> +               cros_usb_pd_print_log_entry(&u.r, now);
>> +       }
>> +
>> +       queue_delayed_work(charger->log_workqueue, &charger->log_work,
>> +               CROS_USB_PD_LOG_UPDATE_DELAY);
>> +}
>> +
>> +static int cros_usb_pd_ec_event(struct notifier_block *nb,
>> +       unsigned long queued_during_suspend, void *_notify)
>> +{
>> +       struct charger_data *charger;
>> +       struct device *dev;
>> +       struct cros_ec_device *ec_device;
>> +       u32 host_event;
>> +
>> +       charger = container_of(nb, struct charger_data, notifier);
>> +       dev = charger->dev;
>> +       ec_device = charger->ec_device;
>> +
>> +       host_event = cros_ec_get_host_event(ec_device);
>> +       if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
>> +               cros_usb_pd_charger_power_changed(charger->ports[0]->psy);
>> +               return NOTIFY_OK;
>> +       }
>> +
>> +       return NOTIFY_DONE;
>> +}
>> +
>> +static char *charger_supplied_to[] = {"cros-usb_pd-charger"};
>> +
>> +static int cros_usb_pd_charger_probe(struct platform_device *pd)
>> +{
>> +       struct device *dev = &pd->dev;
>> +       struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
>> +       struct cros_ec_device *ec_device;
>> +       struct charger_data *charger;
>> +       struct port_data *port;
>> +       struct power_supply_desc *psy_desc;
>> +       struct power_supply_config psy_cfg = {};
>> +       int i;
>> +       int ret = -EINVAL;
>> +
>> +       dev_dbg(dev, "cros_usb_pd_charger_probe\n");

nit: Remove? You can use kernel tracing tools to print functions calls.

>> +       if (!ec_dev) {
>> +               WARN(1, "%s: No EC dev found\n", dev_name(dev));
>> +               return -EINVAL;
>> +       }
>> +
>> +       ec_device = ec_dev->ec_dev;
>> +       if (!ec_device) {
>> +               WARN(1, "%s: No EC device found\n", dev_name(dev));
>> +               return -EINVAL;
>> +       }
>> +
>> +       charger = devm_kzalloc(dev, sizeof(struct charger_data),
>> +                                   GFP_KERNEL);

Alignement should match with parentheses.

>> +       if (!charger)
>> +               return -ENOMEM;
>> +
>> +       charger->dev = dev;
>> +       charger->ec_dev = ec_dev;
>> +       charger->ec_device = ec_device;
>> +
>> +       platform_set_drvdata(pd, charger);
>> +
>> +       if ((get_ec_num_ports(charger, &charger->num_charger_ports) < 0) ||
>> +           !charger->num_charger_ports) {
>> +               dev_err(dev, "No charging ports found\n");
>> +               ret = -ENODEV;
>> +               goto fail;

I think you can return -ENODEV directly here, you don't need to jump to 
fail.

>> +       }
>> +
>> +       for (i = 0; i < charger->num_charger_ports; i++) {
>> +               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_usb_pd_charger_get_prop;
>> +               psy_desc->set_property = cros_usb_pd_charger_set_prop;
>> +               psy_desc->property_is_writeable =
>> +                       cros_usb_pd_charger_is_writeable;
>> +               psy_desc->external_power_changed =
>> +                       cros_usb_pd_charger_power_changed;
>> +               psy_desc->properties = cros_usb_pd_charger_props;
>> +               psy_desc->num_properties = ARRAY_SIZE(
>> +                       cros_usb_pd_charger_props);
>> +
>> +               psy_cfg.drv_data = port;
>> +               psy_cfg.supplied_to = charger_supplied_to;
>> +               psy_cfg.num_supplicants = ARRAY_SIZE(charger_supplied_to);
>> +
>> +               port->psy = power_supply_register_no_ws(dev, psy_desc,
>> +                                                       &psy_cfg);

Guess you can use devm_power_supply_register_no_ws here.

>> +               if (IS_ERR(port->psy)) {
>> +                       dev_err(dev, "Failed to register power supply: %ld\n",
>> +                               PTR_ERR(port->psy));
>> +                       continue;
>> +               }
>> +
>> +               charger->ports[charger->num_registered_psy++] = port;
>> +       }
>> +
>> +       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_usb_pd_ec_event;
>> +               ret = blocking_notifier_chain_register(
>> +                       &ec_device->event_notifier, &charger->notifier);
>> +               if (ret < 0)
>> +                       dev_warn(dev, "failed to register notifier\n");
>> +       }
>> +
>> +       /* Retrieve PD event logs periodically */
>> +       INIT_DELAYED_WORK(&charger->log_work, cros_usb_pd_log_check);
>> +       charger->log_workqueue =
>> +               create_singlethread_workqueue("cros_usb_pd_log");
>> +       queue_delayed_work(charger->log_workqueue, &charger->log_work,
>> +               CROS_USB_PD_LOG_UPDATE_DELAY);
>> +
>> +       return 0;
>> +
>> +fail:
>> +       for (i = 0; i < charger->num_registered_psy; i++) {
>> +               port = charger->ports[i];
>> +               power_supply_unregister(port->psy);

If you use devm_power_supply_register_no_ws this is not needed since it 
would be called by managed resources infrastructure.

>> +               devm_kfree(dev, port);

Guess the managed resources infrastructure will do this for you.

>> +       }
>> +       platform_set_drvdata(pd, NULL);
>> +
>> +       return ret;
>> +}
>> +
>> +static int cros_usb_pd_charger_remove(struct platform_device *pd)
>> +{
>> +       struct charger_data *charger = platform_get_drvdata(pd);
>> +       struct port_data *port;
>> +       int i;
>> +
>> +       if (charger->notifier.notifier_call)
>> +               blocking_notifier_chain_unregister(
>> +                       &charger->ec_device->event_notifier,
>> +                       &charger->notifier);
>> +
>> +       for (i = 0; i < charger->num_registered_psy; i++) {
>> +               port = charger->ports[i];
>> +               power_supply_unregister(port->psy);
>> +       }
>> +       flush_delayed_work(&charger->log_work);
>> +
>> +       return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int cros_usb_pd_charger_resume(struct device *dev)
>> +{
>> +       struct charger_data *charger = dev_get_drvdata(dev);
>> +       int i;
>> +
>> +       if (!charger)
>> +               return 0;
>> +
>> +       charger->suspended = false;
>> +
>> +       dev_dbg(dev, "cros_usb_pd_charger_resume: updating power supplies\n");
>> +       for (i = 0; i < charger->num_registered_psy; i++) {
>> +               power_supply_changed(charger->ports[i]->psy);
>> +               charger->ports[i]->last_update =
>> +                               jiffies - CROS_USB_PD_CACHE_UPDATE_DELAY;
>> +       }
>> +       queue_delayed_work(charger->log_workqueue, &charger->log_work,
>> +               CROS_USB_PD_LOG_UPDATE_DELAY);
>> +
>> +       return 0;
>> +}
>> +
>> +static int cros_usb_pd_charger_suspend(struct device *dev)
>> +{
>> +       struct charger_data *charger = dev_get_drvdata(dev);
>> +
>> +       charger->suspended = true;
>> +
>> +       flush_delayed_work(&charger->log_work);
>> +
>> +       return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(cros_usb_pd_charger_pm_ops,
>> +       cros_usb_pd_charger_suspend, cros_usb_pd_charger_resume);
>> +
>> +static struct platform_driver cros_usb_pd_charger_driver = {
>> +       .driver = {
>> +               .name = "cros-usb-pd-charger",
>> +               .pm = &cros_usb_pd_charger_pm_ops,
>> +       },
>> +       .probe = cros_usb_pd_charger_probe,
>> +       .remove = cros_usb_pd_charger_remove,
>> +};
>> +
>> +module_platform_driver(cros_usb_pd_charger_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Chrome USB PD charger");
>> +MODULE_ALIAS("power_supply:cros-usb-pd-charger");
>> --
>> 2.5.5
>>

Best regards,
  Enric

Re: [PATCH v8 6/7] power: cros_usbpd-charger: Add EC-based USB PD charger driver

[Tomeu Vizoso]

On 26 April 2016 at 12:47, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> Hi Tomeu,
>
> Thanks for the patch, looks good, a few comments below.
>
>
> On 20/04/16 09:42, Tomeu Vizoso wrote:
>>
>> Hi Sebastian,
>>
>> is there any chance that you could review this patch?
>>
>> Thanks,
>>
>> Tomeu
>>
>> On 12 April 2016 at 14:32, Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> wrote:
>>>
>>> +config CHARGER_CROS_USB_PD
>>> +       tristate "Chrome OS EC based USB PD charger"
>>> +       depends on MFD_CROS_EC
>>> +       default y
>
>
> Guess you should not set default to yes here.

Ok.

>>> +static int get_ec_num_ports(struct charger_data *charger, int
>>> *num_ports)
>>> +{
>>> +       struct device *dev = charger->dev;
>>> +       struct ec_response_usb_pd_ports resp;
>>> +       int ret;
>>> +
>>> +       *num_ports = 0;
>>> +       ret = ec_command(charger->ec_dev, 0, EC_CMD_USB_PD_PORTS, NULL,
>>> 0,
>>> +                        (uint8_t *)&resp, sizeof(resp));
>>> +       if (ret < 0) {
>>> +               dev_err(dev, "Unable to query PD ports (err:0x%x)\n",
>>> ret);
>>> +               return ret;
>
>
> Generally you return -EINVAL instead of ret when ec_command fails, any
> reason why here is different?

No reason indeed, will be changing it.

>>> +       snprintf(port->manufacturer, MANUFACTURER_MODEL_LENGTH, "%x",
>>> resp.vid);
>
>
> This looks a vendor id code, generally I saw this propietry show the
> manufacturer name.

Unfortunately the EC firmware gives us only the USB vendor and product IDs.

>>> +
>>> +       dev_dbg(dev, "cros_usb_pd_charger_power_changed\n");
>
>
> nit: In my opinion this debug message is not needed and doesn't add any
> information. It only shows that the function is called. You can usethe
> kernel tracing tools for that.

Agreed.

>>> +       case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX:
>>> +               /* TODO: send a TBD host command to the EC. */
>>> +               val->intval = 0;
>
>
> If I'm not mistaken the function below sets the control limit max value, but
> here you always return a 0. This is confusing for me. Seems that this is not
> supported yet? So maybe is better remove the code related to this or store
> the value set in a variable meanwhile you're not able to ask the value to
> the EC.

Good point, I have chosen to return -ENODATA as that should make clear
to the user that we cannot really show what is being asked right now.

>>> +       msecs = ktime_to_ms(tstamp);
>>> +       do_div(msecs, MSEC_PER_SEC);
>>> +       pr_info("PDLOG %d/%02d/%02d %02d:%02d:%02d.%03llu P%d %s\n",
>>> +               rt.tm_year + 1900, rt.tm_mon + 1, rt.tm_mday,
>>> +               rt.tm_hour, rt.tm_min, rt.tm_sec, msecs,
>>> +               PD_LOG_PORT(r->size_port), buf);
>
>
> nit: Maybe is better use a debug print level here. How often is this called?

This has been mentioned before, and the reason is that it's very
useful when helping users determine if the type-c cable that they are
using is defective or not:

http://www.ibtimes.co.uk/googler-shows-how-test-compatibility-usb-c-cable-chromebook-pixel-2015-1527334

Shouldn't happen often at all.

>>> +static int cros_usb_pd_charger_probe(struct platform_device *pd)
>>> +{
>>> +       struct device *dev = &pd->dev;
>>> +       struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
>>> +       struct cros_ec_device *ec_device;
>>> +       struct charger_data *charger;
>>> +       struct port_data *port;
>>> +       struct power_supply_desc *psy_desc;
>>> +       struct power_supply_config psy_cfg = {};
>>> +       int i;
>>> +       int ret = -EINVAL;
>>> +
>>> +       dev_dbg(dev, "cros_usb_pd_charger_probe\n");
>
>
> nit: Remove? You can use kernel tracing tools to print functions calls.

Cool.

>>> +       if (!ec_dev) {
>>> +               WARN(1, "%s: No EC dev found\n", dev_name(dev));
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       ec_device = ec_dev->ec_dev;
>>> +       if (!ec_device) {
>>> +               WARN(1, "%s: No EC device found\n", dev_name(dev));
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       charger = devm_kzalloc(dev, sizeof(struct charger_data),
>>> +                                   GFP_KERNEL);
>
>
> Alignement should match with parentheses.

Ok.

>>> +       if (!charger)
>>> +               return -ENOMEM;
>>> +
>>> +       charger->dev = dev;
>>> +       charger->ec_dev = ec_dev;
>>> +       charger->ec_device = ec_device;
>>> +
>>> +       platform_set_drvdata(pd, charger);
>>> +
>>> +       if ((get_ec_num_ports(charger, &charger->num_charger_ports) < 0)
>>> ||
>>> +           !charger->num_charger_ports) {
>>> +               dev_err(dev, "No charging ports found\n");
>>> +               ret = -ENODEV;
>>> +               goto fail;
>
>
> I think you can return -ENODEV directly here, you don't need to jump to
> fail.

The idea is to undo the call to platform_set_drvdata.

>>> +       }
>>> +
>>> +       for (i = 0; i < charger->num_charger_ports; i++) {
>>> +               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_usb_pd_charger_get_prop;
>>> +               psy_desc->set_property = cros_usb_pd_charger_set_prop;
>>> +               psy_desc->property_is_writeable =
>>> +                       cros_usb_pd_charger_is_writeable;
>>> +               psy_desc->external_power_changed =
>>> +                       cros_usb_pd_charger_power_changed;
>>> +               psy_desc->properties = cros_usb_pd_charger_props;
>>> +               psy_desc->num_properties = ARRAY_SIZE(
>>> +                       cros_usb_pd_charger_props);
>>> +
>>> +               psy_cfg.drv_data = port;
>>> +               psy_cfg.supplied_to = charger_supplied_to;
>>> +               psy_cfg.num_supplicants =
>>> ARRAY_SIZE(charger_supplied_to);
>>> +
>>> +               port->psy = power_supply_register_no_ws(dev, psy_desc,
>>> +                                                       &psy_cfg);
>
>
> Guess you can use devm_power_supply_register_no_ws here.

Cool.

>>> +               if (IS_ERR(port->psy)) {
>>> +                       dev_err(dev, "Failed to register power supply:
>>> %ld\n",
>>> +                               PTR_ERR(port->psy));
>>> +                       continue;
>>> +               }
>>> +
>>> +               charger->ports[charger->num_registered_psy++] = port;
>>> +       }
>>> +
>>> +       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_usb_pd_ec_event;
>>> +               ret = blocking_notifier_chain_register(
>>> +                       &ec_device->event_notifier, &charger->notifier);
>>> +               if (ret < 0)
>>> +                       dev_warn(dev, "failed to register notifier\n");
>>> +       }
>>> +
>>> +       /* Retrieve PD event logs periodically */
>>> +       INIT_DELAYED_WORK(&charger->log_work, cros_usb_pd_log_check);
>>> +       charger->log_workqueue =
>>> +               create_singlethread_workqueue("cros_usb_pd_log");
>>> +       queue_delayed_work(charger->log_workqueue, &charger->log_work,
>>> +               CROS_USB_PD_LOG_UPDATE_DELAY);
>>> +
>>> +       return 0;
>>> +
>>> +fail:
>>> +       for (i = 0; i < charger->num_registered_psy; i++) {
>>> +               port = charger->ports[i];
>>> +               power_supply_unregister(port->psy);
>
>
> If you use devm_power_supply_register_no_ws this is not needed since it
> would be called by managed resources infrastructure.

Nod.

>>> +               devm_kfree(dev, port);
>
>
> Guess the managed resources infrastructure will do this for you.

Nod.

> Best regards,
>  Enric

Thanks a lot for the review!

Tomeu

[PATCH v8 7/7] platform/chrome: Register USB PD charger device

[Tomeu Vizoso]

Check if a EC has andy PD ports and register a USB PD charger device if
so.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

Changes in v8: None
Changes in v7:
- Rebased onto 4.6-rc1, with no conflicts.

Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3:
- Only register the PD charger device if there are any PD ports in this
  EC.
- Dropped patch using EC_CMD_GET_FEATURES to decide whether to create a
  charger device because we are now only creating one if a EC has any PD
  ports.
- Dropped patch adding power_supply types because it has been merged
  already.

Changes in v2:
- Move cros_ec_usb_pd_charger_register into cros_ec_dev.c.

 drivers/platform/chrome/cros_ec_dev.c | 44 +++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
index d45cd254ed1c..b1251f4a04c1 100644
--- a/drivers/platform/chrome/cros_ec_dev.c
+++ b/drivers/platform/chrome/cros_ec_dev.c
@@ -22,6 +22,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/uaccess.h>
+#include <linux/mfd/core.h>
 
 #include "cros_ec_dev.h"
 
@@ -87,6 +88,30 @@ exit:
 	return ret;
 }
 
+static int cros_ec_has_usb_pd_ports(struct cros_ec_dev *ec)
+{
+	struct cros_ec_command *msg;
+	struct ec_response_usb_pd_ports *resp;
+	int ret;
+
+	msg = kmalloc(sizeof(*msg) + sizeof(*resp), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->version = 0;
+	msg->command = EC_CMD_USB_PD_PORTS + ec->cmd_offset;
+	msg->insize = sizeof(struct ec_response_usb_pd_ports);
+	msg->outsize = 0;
+
+	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+	resp = (struct ec_response_usb_pd_ports *)msg->data;
+	ret = ret >= 0 && msg->result == EC_RES_SUCCESS && resp->num_ports;
+
+	kfree(msg);
+
+	return ret;
+}
+
 /* Device file ops */
 static int ec_device_open(struct inode *inode, struct file *filp)
 {
@@ -217,6 +242,13 @@ static void __remove(struct device *dev)
 	kfree(ec);
 }
 
+static const struct mfd_cell cros_usb_pd_charger_devs[] = {
+	{
+		.name = "cros-usb-pd-charger",
+		.id   = -1,
+	},
+};
+
 static int ec_device_probe(struct platform_device *pdev)
 {
 	int retval = -ENOMEM;
@@ -269,8 +301,20 @@ static int ec_device_probe(struct platform_device *pdev)
 		goto dev_reg_failed;
 	}
 
+	if (cros_ec_has_usb_pd_ports(ec)) {
+		retval = mfd_add_devices(dev, 0, cros_usb_pd_charger_devs,
+					 ARRAY_SIZE(cros_usb_pd_charger_devs),
+					 NULL, 0, NULL);
+		if (retval) {
+			dev_err(dev, "failed to add usb-pd-charger device\n");
+			goto pd_reg_failed;
+		}
+	}
+
 	return 0;
 
+pd_reg_failed:
+	put_device(&ec->class_dev);
 dev_reg_failed:
 set_named_failed:
 	dev_set_drvdata(dev, NULL);
-- 
2.5.5