[PATCH 0/3] ChromeOS EC USB-C Connector Class

[PATCH 0/3] ChromeOS EC USB-C Connector Class

[Jon Flatley]

This patch set adds a basic implementation of the USB-C connector class for
devices using the ChromeOS EC. On ACPI devices an additional ACPI driver is
necessary to receive USB-C PD host events from the PD EC device "GOOG0003".
Incidentally, this ACPI driver adds notifications for events that
cros-usbpd-charger has been missing, so fix that while we're at it.

Jon Flatley (3):
  platform: chrome: Add cros-ec-usbpd-notify driver
  power: supply: cros-ec-usbpd-charger: Fix host events
  platform: chrome: Added cros-ec-typec driver

 drivers/mfd/cros_ec_dev.c                     |   7 +-
 drivers/platform/chrome/Kconfig               |  20 +
 drivers/platform/chrome/Makefile              |   2 +
 drivers/platform/chrome/cros_ec_typec.c       | 457 ++++++++++++++++++
 .../platform/chrome/cros_ec_usbpd_notify.c    | 156 ++++++
 drivers/power/supply/Kconfig                  |   2 +-
 drivers/power/supply/cros_usbpd-charger.c     |  45 +-
 .../platform_data/cros_ec_usbpd_notify.h      |  40 ++
 8 files changed, 696 insertions(+), 33 deletions(-)
 create mode 100644 drivers/platform/chrome/cros_ec_typec.c
 create mode 100644 drivers/platform/chrome/cros_ec_usbpd_notify.c
 create mode 100644 include/linux/platform_data/cros_ec_usbpd_notify.h

-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 1/3] platform: chrome: Add cros-ec-usbpd-notify driver

[Jon Flatley]

ChromiumOS uses ACPI device with HID "GOOG0003" for power delivery
related events. The existing cros-usbpd-charger driver relies on these
events without ever actually receiving them on ACPI platforms. This is
because in the ChromeOS kernel trees, the GOOG0003 device is owned by an
ACPI driver that offers firmware updates to USB-C chargers.

Let's introduce a new platform driver under cros-ec, the ChromeOS
embedded controller, that handles these PD events and dispatches them
appropriately over a notifier chain to all drivers that use them.

Signed-off-by: Jon Flatley <jflat@chromium.org>
---
 drivers/platform/chrome/Kconfig               |   9 +
 drivers/platform/chrome/Makefile              |   1 +
 .../platform/chrome/cros_ec_usbpd_notify.c    | 156 ++++++++++++++++++
 .../platform_data/cros_ec_usbpd_notify.h      |  40 +++++
 4 files changed, 206 insertions(+)
 create mode 100644 drivers/platform/chrome/cros_ec_usbpd_notify.c
 create mode 100644 include/linux/platform_data/cros_ec_usbpd_notify.h

diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index ee5f08ea57b6..d4a55b64bc29 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -118,6 +118,15 @@ config CROS_EC_SPI
 	  response time cannot be guaranteed, we support ignoring
 	  'pre-amble' bytes before the response actually starts.
 
+config CROS_EC_USBPD_NOTIFY
+	tristate "ChromeOS USB-C power delivery event notifier"
+	depends on CROS_EC
+	help
+	  If you say Y here, you get support for USB-C PD event notifications
+	  from the ChromeOS EC. On ACPI platorms this driver will bind to the
+	  GOOG0003 ACPI device, and on non-ACPI platform this driver will match
+	  "google,cros-ec-pd-update" in device tree.
+
 config CROS_EC_LPC
 	tristate "ChromeOS Embedded Controller (LPC)"
 	depends on CROS_EC && ACPI && (X86 || COMPILE_TEST)
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index 477ec3d1d1c9..efa355ab526f 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -21,5 +21,6 @@ obj-$(CONFIG_CROS_EC_VBC)		+= cros_ec_vbc.o
 obj-$(CONFIG_CROS_EC_DEBUGFS)		+= cros_ec_debugfs.o
 obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
 obj-$(CONFIG_CROS_USBPD_LOGGER)		+= cros_usbpd_logger.o
+obj-$(CONFIG_CROS_EC_USBPD_NOTIFY)      += cros_ec_usbpd_notify.o
 
 obj-$(CONFIG_WILCO_EC)			+= wilco_ec/
diff --git a/drivers/platform/chrome/cros_ec_usbpd_notify.c b/drivers/platform/chrome/cros_ec_usbpd_notify.c
new file mode 100644
index 000000000000..f654586dea2a
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_usbpd_notify.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * cros_ec_usbpd - ChromeOS EC Power Delivery Driver
+ *
+ * Copyright (C) 2019 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.
+ *
+ * This driver serves as the receiver of cros_ec PD host events.
+ */
+
+#include <linux/acpi.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_usbpd_notify.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+
+#define DRV_NAME "cros-ec-usbpd-notify"
+#define ACPI_DRV_NAME "GOOG0003"
+
+static BLOCKING_NOTIFIER_HEAD(cros_ec_usbpd_notifier_list);
+
+int cros_ec_usbpd_register_notify(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(
+			&cros_ec_usbpd_notifier_list, nb);
+}
+EXPORT_SYMBOL_GPL(cros_ec_usbpd_register_notify);
+
+void cros_ec_usbpd_unregister_notify(struct notifier_block *nb)
+{
+	blocking_notifier_chain_unregister(&cros_ec_usbpd_notifier_list, nb);
+}
+EXPORT_SYMBOL_GPL(cros_ec_usbpd_unregister_notify);
+
+static void cros_ec_usbpd_notify(u32 event)
+{
+	blocking_notifier_call_chain(&cros_ec_usbpd_notifier_list, event, NULL);
+}
+
+#ifdef CONFIG_ACPI
+
+static int cros_ec_usbpd_add_acpi(struct acpi_device *adev)
+{
+	return 0;
+}
+
+static int cros_ec_usbpd_remove_acpi(struct acpi_device *adev)
+{
+	return 0;
+}
+static void cros_ec_usbpd_notify_acpi(struct acpi_device *adev, u32 event)
+{
+	cros_ec_usbpd_notify(event);
+}
+
+static const struct acpi_device_id cros_ec_usbpd_acpi_device_ids[] = {
+	{ ACPI_DRV_NAME, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, cros_ec_usbpd_acpi_device_ids);
+
+static struct acpi_driver cros_ec_usbpd_driver = {
+	.name = DRV_NAME,
+	.class = DRV_NAME,
+	.ids = cros_ec_usbpd_acpi_device_ids,
+	.ops = {
+		.add = cros_ec_usbpd_add_acpi,
+		.remove = cros_ec_usbpd_remove_acpi,
+		.notify = cros_ec_usbpd_notify_acpi,
+	},
+};
+module_acpi_driver(cros_ec_usbpd_driver);
+
+#else /* CONFIG_ACPI */
+
+static int cros_ec_usbpd_notify_plat(struct notifier_block *nb,
+		unsigned long queued_during_suspend, void *data)
+{
+	struct cros_ec_device *ec_dev = (struct cros_ec_device *)data;
+	u32 host_event = cros_ec_get_host_event(ec_dev);
+
+	if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
+		cros_ec_usbpd_notify(host_event);
+		return NOTIFY_OK;
+	} else {
+		return NOTIFY_DONE;
+	}
+
+}
+
+static int cros_ec_usbpd_probe_plat(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cros_ec_device *ec_dev = dev_get_drvdata(dev->parent);
+	struct notifier_block *nb;
+	int ret;
+
+	nb = devm_kzalloc(dev, sizeof(*nb), GFP_KERNEL);
+	if (!nb)
+		return -ENOMEM;
+
+	nb->notifier_call = cros_ec_usbpd_notify_plat;
+	dev_set_drvdata(dev, nb);
+	ret = blocking_notifier_chain_register(&ec_dev->event_notifier, nb);
+
+	if (ret < 0)
+		dev_warn(dev, "Failed to register notifier\n");
+
+	return 0;
+}
+
+static int cros_ec_usbpd_remove_plat(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cros_ec_device *ec_dev = dev_get_drvdata(dev->parent);
+	struct notifier_block *nb =
+		(struct notifier_block *)dev_get_drvdata(dev);
+
+	blocking_notifier_chain_unregister(&ec_dev->event_notifier, nb);
+
+	return 0;
+}
+
+static const struct of_device_id cros_ec_usbpd_of_match[] = {
+	{ .compatible = "google,cros-ec-pd-update" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, cros_ec_usbpd_of_match);
+
+static struct platform_driver cros_ec_usbpd_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.of_match_table = of_match_ptr(cros_ec_usbpd_of_match),
+	},
+	.probe = cros_ec_usbpd_probe_plat,
+	.remove = cros_ec_usbpd_remove_plat,
+};
+module_platform_driver(cros_ec_usbpd_driver);
+
+#endif /* CONFIG_ACPI */
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ChromeOS power delivery device");
+MODULE_AUTHOR("Jon Flatley <jflat@chromium.org>");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/platform_data/cros_ec_usbpd_notify.h b/include/linux/platform_data/cros_ec_usbpd_notify.h
new file mode 100644
index 000000000000..fdcea146b7c4
--- /dev/null
+++ b/include/linux/platform_data/cros_ec_usbpd_notify.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * cros_ec_usbpd - ChromeOS EC Power Delivery Driver
+ *
+ * Copyright (C) 2019 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.
+ */
+
+#ifndef __LINUX_PLATFORM_DATA_CROS_EC_USBPD_NOTIFY_H
+#define __LINUX_PLATFORM_DATA_CROS_EC_USBPD_NOTIFY_H
+
+#include <linux/notifier.h>
+
+/**
+ * cros_ec_usbpd_register_notify - register a notifier callback for USB PD
+ * events. On ACPI platforms this corrisponds to to host events on the ECPD
+ * "GOOG0003" ACPI device. On non-ACPI platforms this will filter mkbp events
+ * for USB PD events.
+ *
+ * @nb: Notifier block pointer to register
+ */
+int cros_ec_usbpd_register_notify(struct notifier_block *nb);
+
+/**
+ * cros_ec_usbpd_unregister_notify - unregister a notifier callback that was
+ * previously registered with cros_ec_usbpd_register_notify().
+ *
+ * @nb: Notifier block pointer to unregister
+ */
+void cros_ec_usbpd_unregister_notify(struct notifier_block *nb);
+
+#endif  /* __LINUX_PLATFORM_DATA_CROS_EC_USBPD_NOTIFY_H */
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH 1/3] platform: chrome: Add cros-ec-usbpd-notify driver

[Enric Balletbo Serra]

Hi Jon,

Many thanks for working on this and push it upstream. Some comments below

Missatge de Jon Flatley <jflat@chromium.org> del dia dc., 13 de nov.
2019 a les 4:13:
>
> ChromiumOS uses ACPI device with HID "GOOG0003" for power delivery
> related events. The existing cros-usbpd-charger driver relies on these
> events without ever actually receiving them on ACPI platforms. This is
> because in the ChromeOS kernel trees, the GOOG0003 device is owned by an
> ACPI driver that offers firmware updates to USB-C chargers.
>
> Let's introduce a new platform driver under cros-ec, the ChromeOS
> embedded controller, that handles these PD events and dispatches them
> appropriately over a notifier chain to all drivers that use them.
>
> Signed-off-by: Jon Flatley <jflat@chromium.org>
> ---
>  drivers/platform/chrome/Kconfig               |   9 +
>  drivers/platform/chrome/Makefile              |   1 +
>  .../platform/chrome/cros_ec_usbpd_notify.c    | 156 ++++++++++++++++++
>  .../platform_data/cros_ec_usbpd_notify.h      |  40 +++++
>  4 files changed, 206 insertions(+)
>  create mode 100644 drivers/platform/chrome/cros_ec_usbpd_notify.c
>  create mode 100644 include/linux/platform_data/cros_ec_usbpd_notify.h
>
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index ee5f08ea57b6..d4a55b64bc29 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -118,6 +118,15 @@ config CROS_EC_SPI
>           response time cannot be guaranteed, we support ignoring
>           'pre-amble' bytes before the response actually starts.
>
> +config CROS_EC_USBPD_NOTIFY
> +       tristate "ChromeOS USB-C power delivery event notifier"
> +       depends on CROS_EC
> +       help
> +         If you say Y here, you get support for USB-C PD event notifications
> +         from the ChromeOS EC. On ACPI platorms this driver will bind to the
> +         GOOG0003 ACPI device, and on non-ACPI platform this driver will match
> +         "google,cros-ec-pd-update" in device tree.
> +
>  config CROS_EC_LPC
>         tristate "ChromeOS Embedded Controller (LPC)"
>         depends on CROS_EC && ACPI && (X86 || COMPILE_TEST)
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index 477ec3d1d1c9..efa355ab526f 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -21,5 +21,6 @@ obj-$(CONFIG_CROS_EC_VBC)             += cros_ec_vbc.o
>  obj-$(CONFIG_CROS_EC_DEBUGFS)          += cros_ec_debugfs.o
>  obj-$(CONFIG_CROS_EC_SYSFS)            += cros_ec_sysfs.o
>  obj-$(CONFIG_CROS_USBPD_LOGGER)                += cros_usbpd_logger.o
> +obj-$(CONFIG_CROS_EC_USBPD_NOTIFY)      += cros_ec_usbpd_notify.o
>
>  obj-$(CONFIG_WILCO_EC)                 += wilco_ec/
> diff --git a/drivers/platform/chrome/cros_ec_usbpd_notify.c b/drivers/platform/chrome/cros_ec_usbpd_notify.c
> new file mode 100644
> index 000000000000..f654586dea2a
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_ec_usbpd_notify.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * cros_ec_usbpd - ChromeOS EC Power Delivery Driver

Please remove 'cros_ec_usbpd -'

> + *
> + * Copyright (C) 2019 Google, Inc
> + *

I think it should be

Copyright 2019 Google LLC

> + * 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.
> + *

Remove the license boilerplate, with the SPDX identifier, is enough.

> + * This driver serves as the receiver of cros_ec PD host events.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_usbpd_notify.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME "cros-ec-usbpd-notify"
> +#define ACPI_DRV_NAME "GOOG0003"
> +
> +static BLOCKING_NOTIFIER_HEAD(cros_ec_usbpd_notifier_list);
> +
> +int cros_ec_usbpd_register_notify(struct notifier_block *nb)
> +{
> +       return blocking_notifier_chain_register(
> +                       &cros_ec_usbpd_notifier_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_usbpd_register_notify);
> +
> +void cros_ec_usbpd_unregister_notify(struct notifier_block *nb)
> +{
> +       blocking_notifier_chain_unregister(&cros_ec_usbpd_notifier_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_usbpd_unregister_notify);
> +
> +static void cros_ec_usbpd_notify(u32 event)
> +{
> +       blocking_notifier_call_chain(&cros_ec_usbpd_notifier_list, event, NULL);
> +}

That function is not needed, just call directly blocking_notifier_call_chain.

> +
> +#ifdef CONFIG_ACPI
> +

I think you can create a single platform_driver ACPI and non-ACPI
compatible and get rid of this ifdef

> +static int cros_ec_usbpd_add_acpi(struct acpi_device *adev)
> +{
> +       return 0;
> +}
> +
Could be probably be removed

> +static int cros_ec_usbpd_remove_acpi(struct acpi_device *adev)
> +{
> +       return 0;
> +}
Could be probably be removed

> +static void cros_ec_usbpd_notify_acpi(struct acpi_device *adev, u32 event)
> +{
> +       cros_ec_usbpd_notify(event);
> +}
> +
> +static const struct acpi_device_id cros_ec_usbpd_acpi_device_ids[] = {
> +       { ACPI_DRV_NAME, 0 },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(acpi, cros_ec_usbpd_acpi_device_ids);
> +
> +static struct acpi_driver cros_ec_usbpd_driver = {
> +       .name = DRV_NAME,
> +       .class = DRV_NAME,
> +       .ids = cros_ec_usbpd_acpi_device_ids,
> +       .ops = {
> +               .add = cros_ec_usbpd_add_acpi,
> +               .remove = cros_ec_usbpd_remove_acpi,
> +               .notify = cros_ec_usbpd_notify_acpi,
> +       },
> +};
> +module_acpi_driver(cros_ec_usbpd_driver);
> +
> +#else /* CONFIG_ACPI */
> +
> +static int cros_ec_usbpd_notify_plat(struct notifier_block *nb,
> +               unsigned long queued_during_suspend, void *data)
> +{
> +       struct cros_ec_device *ec_dev = (struct cros_ec_device *)data;
> +       u32 host_event = cros_ec_get_host_event(ec_dev);
> +
> +       if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
> +               cros_ec_usbpd_notify(host_event);
> +               return NOTIFY_OK;
> +       } else {

Remove the else and just return NOTIFY_DONE

> +               return NOTIFY_DONE;
> +       }
> +

Remove the extra blank line.

> +}
> +
> +static int cros_ec_usbpd_probe_plat(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct cros_ec_device *ec_dev = dev_get_drvdata(dev->parent);
> +       struct notifier_block *nb;
> +       int ret;
> +
> +       nb = devm_kzalloc(dev, sizeof(*nb), GFP_KERNEL);
> +       if (!nb)
> +               return -ENOMEM;
> +
> +       nb->notifier_call = cros_ec_usbpd_notify_plat;
> +       dev_set_drvdata(dev, nb);
> +       ret = blocking_notifier_chain_register(&ec_dev->event_notifier, nb);
> +

Remove the extra blank line

> +       if (ret < 0)
> +               dev_warn(dev, "Failed to register notifier\n");

Only a warning? shouldn't you return an error?

> +
> +       return 0;
> +}
> +
> +static int cros_ec_usbpd_remove_plat(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct cros_ec_device *ec_dev = dev_get_drvdata(dev->parent);
> +       struct notifier_block *nb =
> +               (struct notifier_block *)dev_get_drvdata(dev);
> +
> +       blocking_notifier_chain_unregister(&ec_dev->event_notifier, nb);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id cros_ec_usbpd_of_match[] = {
> +       { .compatible = "google,cros-ec-pd-update" },

This should be documented in Documentation/devicetree/bindings

> +       { }
> +};
> +MODULE_DEVICE_TABLE(of, cros_ec_usbpd_of_match);
> +
> +static struct platform_driver cros_ec_usbpd_driver = {
> +       .driver = {
> +               .name = DRV_NAME,
> +               .of_match_table = of_match_ptr(cros_ec_usbpd_of_match),
> +       },
> +       .probe = cros_ec_usbpd_probe_plat,
> +       .remove = cros_ec_usbpd_remove_plat,
> +};
> +module_platform_driver(cros_ec_usbpd_driver);
> +
> +#endif /* CONFIG_ACPI */
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("ChromeOS power delivery device");
> +MODULE_AUTHOR("Jon Flatley <jflat@chromium.org>");
> +MODULE_ALIAS("platform:" DRV_NAME);
> diff --git a/include/linux/platform_data/cros_ec_usbpd_notify.h b/include/linux/platform_data/cros_ec_usbpd_notify.h
> new file mode 100644
> index 000000000000..fdcea146b7c4
> --- /dev/null
> +++ b/include/linux/platform_data/cros_ec_usbpd_notify.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * cros_ec_usbpd - ChromeOS EC Power Delivery Driver
> + *

Remove 'cros_ec_usbpd -'

> + * Copyright (C) 2019 Google, Inc
> + *

Copyright 2019 Google LLC

> + * 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.

Remove the license boiler plate

> + */
> +
> +#ifndef __LINUX_PLATFORM_DATA_CROS_EC_USBPD_NOTIFY_H
> +#define __LINUX_PLATFORM_DATA_CROS_EC_USBPD_NOTIFY_H
> +
> +#include <linux/notifier.h>
> +
> +/**
> + * cros_ec_usbpd_register_notify - register a notifier callback for USB PD
> + * events. On ACPI platforms this corrisponds to to host events on the ECPD
> + * "GOOG0003" ACPI device. On non-ACPI platforms this will filter mkbp events
> + * for USB PD events.
> + *
> + * @nb: Notifier block pointer to register
> + */

I don't think this is kernel-doc compliant, please check running kernel-doc -v

> +int cros_ec_usbpd_register_notify(struct notifier_block *nb);
> +
> +/**
> + * cros_ec_usbpd_unregister_notify - unregister a notifier callback that was
> + * previously registered with cros_ec_usbpd_register_notify().
> + *
> + * @nb: Notifier block pointer to unregister
> + */

Same here

> +void cros_ec_usbpd_unregister_notify(struct notifier_block *nb);
> +
> +#endif  /* __LINUX_PLATFORM_DATA_CROS_EC_USBPD_NOTIFY_H */
> --
> 2.24.0.432.g9d3f5f5b63-goog
>

Thanks,
 Enric

[PATCH 2/3] power: supply: cros-ec-usbpd-charger: Fix host events

[Jon Flatley]

There's a bug on ACPI platforms where host events from the ECPD ACPI
device never make their way to the cros-ec-usbpd-charger driver. This
makes it so the only time the charger driver updates its state is when
user space accesses its sysfs attributes.

Now that these events have been unified into a single notifier chain on
both ACPI and non-ACPI platforms the charger driver can just be updated
to use this new notifer.

Signed-off-by: Jon Flatley <jflat@chromium.org>
---
 drivers/power/supply/Kconfig              |  2 +-
 drivers/power/supply/cros_usbpd-charger.c | 45 ++++++++---------------
 2 files changed, 17 insertions(+), 30 deletions(-)

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index c84a7b1caeb6..7664849d7680 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -659,7 +659,7 @@ config CHARGER_RT9455
 
 config CHARGER_CROS_USBPD
 	tristate "ChromeOS EC based USBPD charger"
-	depends on CROS_EC
+	depends on CROS_EC_USBPD_NOTIFY
 	default n
 	help
 	  Say Y here to enable ChromeOS EC based USBPD charger
diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
index 6cc7c3910e09..58cf51b51179 100644
--- a/drivers/power/supply/cros_usbpd-charger.c
+++ b/drivers/power/supply/cros_usbpd-charger.c
@@ -8,6 +8,7 @@
 #include <linux/mfd/cros_ec.h>
 #include <linux/module.h>
 #include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_usbpd_notify.h>
 #include <linux/platform_data/cros_ec_proto.h>
 #include <linux/platform_device.h>
 #include <linux/power_supply.h>
@@ -524,32 +525,22 @@ static int cros_usbpd_charger_property_is_writeable(struct power_supply *psy,
 }
 
 static int cros_usbpd_charger_ec_event(struct notifier_block *nb,
-				       unsigned long queued_during_suspend,
+				       unsigned long host_event,
 				       void *_notify)
 {
-	struct cros_ec_device *ec_device;
 	struct charger_data *charger;
-	u32 host_event;
 
 	charger = container_of(nb, struct charger_data, notifier);
-	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_usbpd_charger_power_changed(charger->ports[0]->psy);
-		return NOTIFY_OK;
-	} else {
-		return NOTIFY_DONE;
-	}
+	cros_usbpd_charger_power_changed(charger->ports[0]->psy);
+	return NOTIFY_OK;
 }
 
 static void cros_usbpd_charger_unregister_notifier(void *data)
 {
 	struct charger_data *charger = data;
-	struct cros_ec_device *ec_device = charger->ec_device;
 
-	blocking_notifier_chain_unregister(&ec_device->event_notifier,
-					   &charger->notifier);
+	cros_ec_usbpd_unregister_notify(&charger->notifier);
 }
 
 static int cros_usbpd_charger_probe(struct platform_device *pd)
@@ -683,21 +674,17 @@ static int cros_usbpd_charger_probe(struct platform_device *pd)
 		goto fail;
 	}
 
-	if (ec_device->mkbp_event_supported) {
-		/* Get PD events from the EC */
-		charger->notifier.notifier_call = cros_usbpd_charger_ec_event;
-		ret = blocking_notifier_chain_register(
-						&ec_device->event_notifier,
-						&charger->notifier);
-		if (ret < 0) {
-			dev_warn(dev, "failed to register notifier\n");
-		} else {
-			ret = devm_add_action_or_reset(dev,
-					cros_usbpd_charger_unregister_notifier,
-					charger);
-			if (ret < 0)
-				goto fail;
-		}
+	/* Get PD events from the EC */
+	charger->notifier.notifier_call = cros_usbpd_charger_ec_event;
+	ret = cros_ec_usbpd_register_notify(&charger->notifier);
+	if (ret < 0) {
+		dev_warn(dev, "failed to register notifier\n");
+	} else {
+		ret = devm_add_action_or_reset(dev,
+				cros_usbpd_charger_unregister_notifier,
+				charger);
+		if (ret < 0)
+			goto fail;
 	}
 
 	return 0;
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH 2/3] power: supply: cros-ec-usbpd-charger: Fix host events

[Enric Balletbo Serra]

Hi Jon,

Some comments below.

Missatge de Jon Flatley <jflat@chromium.org> del dia dc., 13 de nov.
2019 a les 4:19:
>
> There's a bug on ACPI platforms where host events from the ECPD ACPI
> device never make their way to the cros-ec-usbpd-charger driver. This
> makes it so the only time the charger driver updates its state is when
> user space accesses its sysfs attributes.
>
> Now that these events have been unified into a single notifier chain on
> both ACPI and non-ACPI platforms the charger driver can just be updated
> to use this new notifer.
>
> Signed-off-by: Jon Flatley <jflat@chromium.org>
> ---
>  drivers/power/supply/Kconfig              |  2 +-
>  drivers/power/supply/cros_usbpd-charger.c | 45 ++++++++---------------
>  2 files changed, 17 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index c84a7b1caeb6..7664849d7680 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -659,7 +659,7 @@ config CHARGER_RT9455
>
>  config CHARGER_CROS_USBPD
>         tristate "ChromeOS EC based USBPD charger"
> -       depends on CROS_EC
> +       depends on CROS_EC_USBPD_NOTIFY
>         default n
>         help
>           Say Y here to enable ChromeOS EC based USBPD charger
> diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
> index 6cc7c3910e09..58cf51b51179 100644
> --- a/drivers/power/supply/cros_usbpd-charger.c
> +++ b/drivers/power/supply/cros_usbpd-charger.c
> @@ -8,6 +8,7 @@
>  #include <linux/mfd/cros_ec.h>
>  #include <linux/module.h>
>  #include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_usbpd_notify.h>
>  #include <linux/platform_data/cros_ec_proto.h>
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
> @@ -524,32 +525,22 @@ static int cros_usbpd_charger_property_is_writeable(struct power_supply *psy,
>  }
>
>  static int cros_usbpd_charger_ec_event(struct notifier_block *nb,
> -                                      unsigned long queued_during_suspend,
> +                                      unsigned long host_event,
>                                        void *_notify)
>  {
> -       struct cros_ec_device *ec_device;
>         struct charger_data *charger;
> -       u32 host_event;
>
>         charger = container_of(nb, struct charger_data, notifier);
> -       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_usbpd_charger_power_changed(charger->ports[0]->psy);
> -               return NOTIFY_OK;
> -       } else {
> -               return NOTIFY_DONE;
> -       }
> +       cros_usbpd_charger_power_changed(charger->ports[0]->psy);
> +       return NOTIFY_OK;
>  }
>
>  static void cros_usbpd_charger_unregister_notifier(void *data)
>  {
>         struct charger_data *charger = data;
> -       struct cros_ec_device *ec_device = charger->ec_device;
>
> -       blocking_notifier_chain_unregister(&ec_device->event_notifier,
> -                                          &charger->notifier);
> +       cros_ec_usbpd_unregister_notify(&charger->notifier);
>  }
>

Can we get rid of this function and call directly
cros_ec_usbpd_unregister_notify where the function is used?

>  static int cros_usbpd_charger_probe(struct platform_device *pd)
> @@ -683,21 +674,17 @@ static int cros_usbpd_charger_probe(struct platform_device *pd)
>                 goto fail;
>         }
>
> -       if (ec_device->mkbp_event_supported) {
> -               /* Get PD events from the EC */
> -               charger->notifier.notifier_call = cros_usbpd_charger_ec_event;
> -               ret = blocking_notifier_chain_register(
> -                                               &ec_device->event_notifier,
> -                                               &charger->notifier);
> -               if (ret < 0) {
> -                       dev_warn(dev, "failed to register notifier\n");
> -               } else {
> -                       ret = devm_add_action_or_reset(dev,
> -                                       cros_usbpd_charger_unregister_notifier,
> -                                       charger);
> -                       if (ret < 0)
> -                               goto fail;
> -               }
> +       /* Get PD events from the EC */
> +       charger->notifier.notifier_call = cros_usbpd_charger_ec_event;
> +       ret = cros_ec_usbpd_register_notify(&charger->notifier);
> +       if (ret < 0) {
> +               dev_warn(dev, "failed to register notifier\n");

Hmm, now I am wondering if should we only warn and continue or fail.
Makes sense have this without the notifier?

> +       } else {
> +               ret = devm_add_action_or_reset(dev,
> +                               cros_usbpd_charger_unregister_notifier,
> +                               charger);
> +               if (ret < 0)
> +                       goto fail;
>         }
>
>         return 0;
> --
> 2.24.0.432.g9d3f5f5b63-goog
>

Thanks,
 Enric

[PATCH 3/3] platform: chrome: Added cros-ec-typec driver

[Jon Flatley]

Adds the cros-ec-typec driver for implementing the USB Type-C connector
class for cros-ec devices.

Signed-off-by: Jon Flatley <jflat@chromium.org>
---
 drivers/mfd/cros_ec_dev.c               |   7 +-
 drivers/platform/chrome/Kconfig         |  11 +
 drivers/platform/chrome/Makefile        |   1 +
 drivers/platform/chrome/cros_ec_typec.c | 457 ++++++++++++++++++++++++
 4 files changed, 473 insertions(+), 3 deletions(-)
 create mode 100644 drivers/platform/chrome/cros_ec_typec.c

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 6e6dfd6c1871..1136ef986695 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -78,9 +78,10 @@ static const struct mfd_cell cros_ec_rtc_cells[] = {
 	{ .name = "cros-ec-rtc", },
 };
 
-static const struct mfd_cell cros_usbpd_charger_cells[] = {
+static const struct mfd_cell cros_usbpd_cells[] = {
 	{ .name = "cros-usbpd-charger", },
 	{ .name = "cros-usbpd-logger", },
+	{ .name = "cros-ec-typec" },
 };
 
 static const struct cros_feature_to_cells cros_subdevices[] = {
@@ -96,8 +97,8 @@ static const struct cros_feature_to_cells cros_subdevices[] = {
 	},
 	{
 		.id		= EC_FEATURE_USB_PD,
-		.mfd_cells	= cros_usbpd_charger_cells,
-		.num_cells	= ARRAY_SIZE(cros_usbpd_charger_cells),
+		.mfd_cells	= cros_usbpd_cells,
+		.num_cells	= ARRAY_SIZE(cros_usbpd_cells),
 	},
 };
 
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index d4a55b64bc29..1be5c86f2aad 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -210,6 +210,17 @@ config CROS_EC_SYSFS
 	  To compile this driver as a module, choose M here: the
 	  module will be called cros_ec_sysfs.
 
+config CROS_EC_TYPEC
+	tristate "ChromeOS Embedded Controller USB Type-C class subdriver"
+	depends on TYPEC && CROS_EC_USBPD_NOTIFY
+	default n
+	help
+	  Say Y here to enable the USB Type-C class subdriver for ChromeOS
+	  devices.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called cros-ec-typec.
+
 config CROS_USBPD_LOGGER
 	tristate "Logging driver for USB PD charger"
 	depends on CHARGER_CROS_USBPD
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index efa355ab526f..bb298c083f1e 100644
--- a/drivers/platform/chrome/Makefile
+++ b/drivers/platform/chrome/Makefile
@@ -22,5 +22,6 @@ obj-$(CONFIG_CROS_EC_DEBUGFS)		+= cros_ec_debugfs.o
 obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
 obj-$(CONFIG_CROS_USBPD_LOGGER)		+= cros_usbpd_logger.o
 obj-$(CONFIG_CROS_EC_USBPD_NOTIFY)      += cros_ec_usbpd_notify.o
+obj-$(CONFIG_CROS_EC_TYPEC) 		+= cros_ec_typec.o
 
 obj-$(CONFIG_WILCO_EC)			+= wilco_ec/
diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
new file mode 100644
index 000000000000..262b914f0a2e
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -0,0 +1,457 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * cros_ec_usbpd - ChromeOS EC Power Delivery Driver
+ *
+ * Copyright (C) 2019 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/kernel.h>
+#include <linux/kthread.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_usbpd_notify.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/usb/typec.h>
+#include <linux/slab.h>
+
+#define DRV_NAME "cros-ec-typec"
+
+struct port_data {
+	int port_num;
+	struct typec_port *port;
+	struct typec_partner *partner;
+	struct usb_pd_identity p_identity;
+	struct typec_data *typec;
+	struct typec_capability caps;
+};
+
+struct typec_data {
+	struct device *dev;
+	struct cros_ec_dev *ec_dev;
+	struct port_data *ports[EC_USB_PD_MAX_PORTS];
+	unsigned int num_ports;
+	struct notifier_block notifier;
+
+	int (*port_update)(struct typec_data *typec, int port_num);
+};
+
+#define caps_to_port_data(_caps_) container_of(_caps_, struct port_data, caps)
+
+static int cros_typec_ec_command(struct typec_data *typec,
+				 unsigned int version,
+				 unsigned int command,
+				 void *outdata,
+				 unsigned int outsize,
+				 void *indata,
+				 unsigned int insize)
+{
+	struct cros_ec_dev *ec_dev = typec->ec_dev;
+	struct cros_ec_command *msg;
+	int ret;
+	char host_cmd[32];
+
+	msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	msg->version = version;
+	msg->command = ec_dev->cmd_offset + command;
+	msg->outsize = outsize;
+	msg->insize = insize;
+
+	if (outsize)
+		memcpy(msg->data, outdata, outsize);
+
+	sprintf(host_cmd, "typec HC 0x%x req: ", command);
+	print_hex_dump(KERN_DEBUG, host_cmd, DUMP_PREFIX_NONE, 16, 1, outdata,
+			outsize, 0);
+
+	ret = cros_ec_cmd_xfer_status(typec->ec_dev->ec_dev, msg);
+	if (ret >= 0 && insize)
+		memcpy(indata, msg->data, insize);
+	sprintf(host_cmd, "typec HC 0x%x res: ", command);
+	print_hex_dump(KERN_DEBUG, host_cmd, DUMP_PREFIX_NONE, 16, 1, indata,
+			insize, 0);
+
+	kfree(msg);
+	return ret;
+}
+
+static int cros_typec_get_cmd_version(struct typec_data *typec, int cmd,
+		uint8_t *version_mask)
+{
+	struct ec_params_get_cmd_versions req_v0;
+	struct ec_params_get_cmd_versions_v1 req_v1;
+	struct ec_response_get_cmd_versions res;
+	int ret;
+
+	req_v1.cmd = cmd;
+	ret = cros_typec_ec_command(typec, 1, EC_CMD_GET_CMD_VERSIONS, &req_v1,
+			sizeof(req_v1), &res, sizeof(res));
+	if (ret < 0) {
+		req_v0.cmd = cmd;
+		ret = cros_typec_ec_command(typec, 0, EC_CMD_GET_CMD_VERSIONS,
+				&req_v0, sizeof(req_v0), &res, sizeof(res));
+		if (ret < 0)
+			return ret;
+	}
+	*version_mask = res.version_mask;
+	dev_dbg(typec->dev, "EC CMD 0x%hhx has version mask 0x%hhx\n", cmd,
+			*version_mask);
+	return 0;
+}
+
+static int cros_typec_query_pd_port_count(struct typec_data *typec)
+{
+	struct ec_response_usb_pd_ports res;
+	int ret;
+
+	ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
+			&res, sizeof(res));
+	if (ret < 0)
+		return ret;
+	typec->num_ports = res.num_ports;
+	return 0;
+}
+
+static int cros_typec_port_update(struct typec_data *typec,
+				  int port_num,
+				  struct ec_response_usb_pd_control *res,
+				  size_t res_size,
+				  int cmd_ver)
+{
+	struct port_data *port;
+	struct ec_params_usb_pd_control req;
+	int ret;
+
+	if (port_num < 0 || port_num >= typec->num_ports) {
+		dev_err(typec->dev, "cannot get status for invalid port %d\n",
+				port_num);
+		return -EPERM;
+	}
+	port = typec->ports[port_num];
+
+	req.port = port_num;
+	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
+	req.mux = USB_PD_CTRL_MUX_NO_CHANGE;
+	req.swap = USB_PD_CTRL_SWAP_NONE;
+
+	ret = cros_typec_ec_command(typec, cmd_ver, EC_CMD_USB_PD_CONTROL, &req,
+			sizeof(req), res, res_size);
+	if (ret < 0)
+		return ret;
+	dev_dbg(typec->dev, "Enabled %d: 0x%hhx\n", port_num, res->enabled);
+	dev_dbg(typec->dev, "Role %d: 0x%hhx\n", port_num, res->role);
+	dev_dbg(typec->dev, "Polarity %d: 0x%hhx\n", port_num, res->polarity);
+
+	return 0;
+}
+
+static int cros_typec_query_pd_info(struct typec_data *typec, int port_num)
+{
+	struct ec_params_usb_pd_info_request req;
+	struct ec_params_usb_pd_discovery_entry res;
+	int ret;
+
+	req.port = port_num;
+	ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_DISCOVERY, &req,
+			sizeof(req), &res, sizeof(res));
+	if (ret < 0)
+		return ret;
+	/* FIXME Needs the rest of the info in PD Spec 6.4.4.3.1.1 */
+	typec->ports[port_num]->p_identity.id_header = res.vid;
+
+	/* FIXME Needs bcdDevice to match PD Spec 6.4.4.3.1.3 */
+	typec->ports[port_num]->p_identity.product = res.pid << 16;
+	return 0;
+}
+
+static int cros_typec_port_update_v0(struct typec_data *typec, int port_num)
+{
+	struct port_data *port;
+	struct ec_response_usb_pd_control res;
+	enum typec_orientation polarity;
+	int ret;
+
+	port = typec->ports[port_num];
+	ret = cros_typec_port_update(typec, port_num, &res, sizeof(res), 0);
+	if (ret < 0)
+		return ret;
+	dev_dbg(typec->dev, "State %d: %hhx\n", port_num, res.state);
+
+	if (!res.enabled)
+		polarity = TYPEC_ORIENTATION_NONE;
+	else if (!res.polarity)
+		polarity = TYPEC_ORIENTATION_NORMAL;
+	else
+		polarity = TYPEC_ORIENTATION_REVERSE;
+
+	typec_set_pwr_role(port->port, res.role ? TYPEC_SOURCE : TYPEC_SINK);
+	typec_set_orientation(port->port, polarity);
+
+	return 0;
+}
+
+static int cros_typec_add_partner(struct typec_data *typec, int port_num,
+		bool pd_enabled)
+{
+	struct port_data *port;
+	struct typec_partner_desc p_desc;
+	int ret;
+
+	port = typec->ports[port_num];
+	p_desc.usb_pd = pd_enabled;
+	p_desc.identity = &port->p_identity;
+
+	port->partner = typec_register_partner(port->port, &p_desc);
+	if (IS_ERR_OR_NULL(port->partner)) {
+		dev_err(typec->dev, "Port %d partner register failed\n",
+				port_num);
+		port->partner = NULL;
+		return PTR_ERR(port->partner);
+	}
+
+	ret = cros_typec_query_pd_info(typec, port_num);
+	if (ret < 0) {
+		dev_err(typec->dev, "Port %d PD query failed\n", port_num);
+		typec_unregister_partner(port->partner);
+		port->partner = NULL;
+		return ret;
+	}
+
+	ret = typec_partner_set_identity(port->partner);
+	return ret;
+}
+
+static int cros_typec_set_port_params_v1_v2(struct typec_data *typec,
+		int port_num, struct ec_response_usb_pd_control_v1 *res)
+{
+	struct port_data *port;
+	enum typec_orientation polarity;
+	int ret;
+
+	port = typec->ports[port_num];
+	if (!(res->enabled & PD_CTRL_RESP_ENABLED_CONNECTED))
+		polarity = TYPEC_ORIENTATION_NONE;
+	else if (!res->polarity)
+		polarity = TYPEC_ORIENTATION_NORMAL;
+	else
+		polarity = TYPEC_ORIENTATION_REVERSE;
+	typec_set_orientation(port->port, polarity);
+	typec_set_data_role(port->port, res->role & PD_CTRL_RESP_ROLE_DATA ?
+			TYPEC_HOST : TYPEC_DEVICE);
+	typec_set_pwr_role(port->port, res->role & PD_CTRL_RESP_ROLE_POWER ?
+			TYPEC_SOURCE : TYPEC_SINK);
+	typec_set_vconn_role(port->port, res->role & PD_CTRL_RESP_ROLE_VCONN ?
+			TYPEC_SOURCE : TYPEC_SINK);
+
+	if (res->enabled & PD_CTRL_RESP_ENABLED_CONNECTED && !port->partner) {
+		bool pd_enabled =
+			res->enabled & PD_CTRL_RESP_ENABLED_PD_CAPABLE;
+		ret = cros_typec_add_partner(typec, port_num, pd_enabled);
+		if (!ret)
+			return ret;
+	}
+	if (!(res->enabled & PD_CTRL_RESP_ENABLED_CONNECTED) && port->partner) {
+		typec_unregister_partner(port->partner);
+		port->partner = NULL;
+	}
+	return 0;
+}
+
+static int cros_typec_port_update_v1(struct typec_data *typec, int port_num)
+{
+	/*struct port_data *port;*/
+	struct ec_response_usb_pd_control_v1 res;
+	struct ec_response_usb_pd_control *res_v0 =
+		(struct ec_response_usb_pd_control *) &res;
+	int ret;
+
+	ret = cros_typec_port_update(typec, port_num, res_v0, sizeof(res), 1);
+	if (ret < 0)
+		return ret;
+	dev_dbg(typec->dev, "State %d: %s\n", port_num, res.state);
+
+	ret = cros_typec_set_port_params_v1_v2(typec, port_num, &res);
+	return ret;
+}
+
+static int cros_typec_try_role(const struct typec_capability *cap, int role)
+{
+	return 0;
+}
+
+static int cros_typec_dr_set(const struct typec_capability *cap,
+		enum typec_data_role role)
+{
+	return 0;
+}
+
+static int cros_typec_pr_set(const struct typec_capability *cap,
+		enum typec_role role)
+{
+	return 0;
+}
+
+static int cros_typec_vconn_set(const struct typec_capability *cap,
+		enum typec_role role)
+{
+	return 0;
+}
+
+static int cros_typec_ec_event(struct notifier_block *nb,
+			       unsigned long queued_during_suspend,
+			       void *_notify)
+{
+	struct typec_data *typec;
+	int i;
+
+	typec = container_of(nb, struct typec_data, notifier);
+
+	for (i = 0; i < typec->num_ports; ++i)
+		typec->port_update(typec, i);
+
+	return NOTIFY_DONE;
+
+}
+
+static void cros_typec_unregister_notifier(void *data)
+{
+	struct typec_data *typec = data;
+
+	cros_ec_usbpd_unregister_notify(&typec->notifier);
+}
+
+static int cros_typec_probe(struct platform_device *pd)
+{
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
+	struct device *dev = &pd->dev;
+	struct typec_data *typec;
+	uint8_t ver_mask;
+	int i;
+	int ret;
+
+	dev_dbg(dev, "Probing Cros EC Type-C device.\n");
+
+	typec = devm_kzalloc(dev, sizeof(*typec), GFP_KERNEL);
+	if (!typec)
+		return -ENOMEM;
+
+	typec->dev = dev;
+	typec->ec_dev = ec_dev;
+
+	platform_set_drvdata(pd, typec);
+
+	ret = cros_typec_query_pd_port_count(typec);
+	if (ret < 0) {
+		dev_err(dev, "Failed to get PD port count from EC\n");
+		return ret;
+	}
+	if (typec->num_ports > EC_USB_PD_MAX_PORTS) {
+		dev_err(dev, "EC reported too many ports. got: %d, max: %d\n",
+				typec->num_ports, EC_USB_PD_MAX_PORTS);
+		return -EOVERFLOW;
+	}
+
+	ret = cros_typec_get_cmd_version(typec, EC_CMD_USB_PD_CONTROL,
+			&ver_mask);
+	if (ret < 0) {
+		dev_err(dev, "Failed to get supported PD command versions\n");
+		return ret;
+	}
+	/* No reason to support EC_CMD_USB_PD_CONTROL v2 as it doesn't add any
+	 * useful information
+	 */
+	if (ver_mask & EC_VER_MASK(1)) {
+		dev_dbg(dev, "Using PD command ver 1\n");
+		typec->port_update = cros_typec_port_update_v1;
+	} else {
+		dev_dbg(dev, "Using PD command ver 0\n");
+		typec->port_update = cros_typec_port_update_v0;
+	}
+
+	for (i = 0; i < typec->num_ports; ++i) {
+		struct port_data *port;
+
+		port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+		if (!port) {
+			ret = -ENOMEM;
+			goto unregister_ports;
+		}
+		port->typec = typec;
+		port->port_num = i;
+		typec->ports[i] = port;
+
+		/* TODO Make sure these values are accurate */
+		port->caps.type = TYPEC_PORT_DRP;
+		port->caps.data = TYPEC_PORT_DFP;
+		port->caps.prefer_role = TYPEC_SINK;
+
+		port->caps.try_role = cros_typec_try_role;
+		port->caps.dr_set = cros_typec_dr_set;
+		port->caps.pr_set = cros_typec_pr_set;
+		port->caps.vconn_set = cros_typec_vconn_set;
+		port->caps.port_type_set = NULL; /* Not permitted by PD spec */
+
+		port->port = typec_register_port(dev, &port->caps);
+		if (IS_ERR_OR_NULL(port->port)) {
+			dev_err(dev, "Failed to register typec port %d\n", i);
+			ret = PTR_ERR(port->port);
+			goto unregister_ports;
+		}
+
+		ret = typec->port_update(typec, i);
+		if (ret < 0) {
+			dev_err(dev, "Failed to update typec port %d\n", i);
+			goto unregister_ports;
+		}
+	}
+
+	typec->notifier.notifier_call = cros_typec_ec_event;
+	ret = cros_ec_usbpd_register_notify(&typec->notifier);
+	if (ret < 0) {
+		dev_warn(dev, "Failed to register notifier\n");
+	} else {
+		ret = devm_add_action_or_reset(dev,
+				cros_typec_unregister_notifier, typec);
+		if (ret < 0)
+			goto unregister_ports;
+		dev_dbg(dev, "Registered EC notifier\n");
+	}
+
+	return 0;
+
+unregister_ports:
+	for (i = 0; i < typec->num_ports; ++i) {
+		if (typec->ports[i] && typec->ports[i]->port)
+			typec_unregister_port(typec->ports[i]->port);
+	}
+
+	return ret;
+}
+
+static struct platform_driver cros_ec_typec_driver = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.probe = cros_typec_probe
+};
+
+module_platform_driver(cros_ec_typec_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ChromeOS EC USB-C connectors");
+MODULE_AUTHOR("Jon Flatley <jflat@chromium.org>");
+MODULE_ALIAS("platform:" DRV_NAME);
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH 3/3] platform: chrome: Added cros-ec-typec driver

[kbuild test robot]

Hi Jon,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on ljones-mfd/for-mfd-next]
[cannot apply to v5.4-rc7 next-20191113]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Jon-Flatley/ChromeOS-EC-USB-C-Connector-Class/20191113-193504
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


coccinelle warnings: (new ones prefixed by >>)

>> drivers/platform/chrome/cros_ec_typec.c:223:9-16: ERROR: PTR_ERR applied after initialization to constant on line 222

vim +223 drivers/platform/chrome/cros_ec_typec.c

   206	
   207	static int cros_typec_add_partner(struct typec_data *typec, int port_num,
   208			bool pd_enabled)
   209	{
   210		struct port_data *port;
   211		struct typec_partner_desc p_desc;
   212		int ret;
   213	
   214		port = typec->ports[port_num];
   215		p_desc.usb_pd = pd_enabled;
   216		p_desc.identity = &port->p_identity;
   217	
   218		port->partner = typec_register_partner(port->port, &p_desc);
   219		if (IS_ERR_OR_NULL(port->partner)) {
   220			dev_err(typec->dev, "Port %d partner register failed\n",
   221					port_num);
 > 222			port->partner = NULL;
 > 223			return PTR_ERR(port->partner);
   224		}
   225	
   226		ret = cros_typec_query_pd_info(typec, port_num);
   227		if (ret < 0) {
   228			dev_err(typec->dev, "Port %d PD query failed\n", port_num);
   229			typec_unregister_partner(port->partner);
   230			port->partner = NULL;
   231			return ret;
   232		}
   233	
   234		ret = typec_partner_set_identity(port->partner);
   235		return ret;
   236	}
   237	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

Re: [PATCH 3/3] platform: chrome: Added cros-ec-typec driver

[Jon Flatley]

On Wed, Nov 13, 2019 at 4:55 AM kbuild test robot <lkp@intel.com> wrote:
>
> Hi Jon,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on ljones-mfd/for-mfd-next]
> [cannot apply to v5.4-rc7 next-20191113]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url:    https://github.com/0day-ci/linux/commits/Jon-Flatley/ChromeOS-EC-USB-C-Connector-Class/20191113-193504
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
>
>
> coccinelle warnings: (new ones prefixed by >>)
>
> >> drivers/platform/chrome/cros_ec_typec.c:223:9-16: ERROR: PTR_ERR applied after initialization to constant on line 222
>
> vim +223 drivers/platform/chrome/cros_ec_typec.c
>
>    206
>    207  static int cros_typec_add_partner(struct typec_data *typec, int port_num,
>    208                  bool pd_enabled)
>    209  {
>    210          struct port_data *port;
>    211          struct typec_partner_desc p_desc;
>    212          int ret;
>    213
>    214          port = typec->ports[port_num];
>    215          p_desc.usb_pd = pd_enabled;
>    216          p_desc.identity = &port->p_identity;
>    217
>    218          port->partner = typec_register_partner(port->port, &p_desc);
>    219          if (IS_ERR_OR_NULL(port->partner)) {
>    220                  dev_err(typec->dev, "Port %d partner register failed\n",
>    221                                  port_num);
>  > 222                  port->partner = NULL;
>  > 223                  return PTR_ERR(port->partner);
>    224          }
>    225
>    226          ret = cros_typec_query_pd_info(typec, port_num);
>    227          if (ret < 0) {
>    228                  dev_err(typec->dev, "Port %d PD query failed\n", port_num);
>    229                  typec_unregister_partner(port->partner);
>    230                  port->partner = NULL;
>    231                  return ret;
>    232          }
>    233
>    234          ret = typec_partner_set_identity(port->partner);
>    235          return ret;
>    236  }
>    237
>
> ---
> 0-DAY kernel test infrastructure                 Open Source Technology Center
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

This patch is based off of the chrome-platform for-next branch.

base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git
for-next

Is this incorrect? I'm happy to rebase if necessary.

Thanks,
-Jon

Re: [kbuild-all] Re: [PATCH 3/3] platform: chrome: Added cros-ec-typec driver

[Rong Chen]

On 11/14/19 1:23 AM, Jon Flatley wrote:
> On Wed, Nov 13, 2019 at 4:55 AM kbuild test robot <lkp@intel.com> wrote:
>> Hi Jon,
>>
>> Thank you for the patch! Perhaps something to improve:
>>
>> [auto build test WARNING on ljones-mfd/for-mfd-next]
>> [cannot apply to v5.4-rc7 next-20191113]
>> [if your patch is applied to the wrong git tree, please drop us a note to help
>> improve the system. BTW, we also suggest to use '--base' option to specify the
>> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>>
>> url:    https://github.com/0day-ci/linux/commits/Jon-Flatley/ChromeOS-EC-USB-C-Connector-Class/20191113-193504
>> base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
>>
>> If you fix the issue, kindly add following tag
>> Reported-by: kbuild test robot <lkp@intel.com>
>>
>>
>> coccinelle warnings: (new ones prefixed by >>)
>>
>>>> drivers/platform/chrome/cros_ec_typec.c:223:9-16: ERROR: PTR_ERR applied after initialization to constant on line 222
>> vim +223 drivers/platform/chrome/cros_ec_typec.c
>>
>>     206
>>     207  static int cros_typec_add_partner(struct typec_data *typec, int port_num,
>>     208                  bool pd_enabled)
>>     209  {
>>     210          struct port_data *port;
>>     211          struct typec_partner_desc p_desc;
>>     212          int ret;
>>     213
>>     214          port = typec->ports[port_num];
>>     215          p_desc.usb_pd = pd_enabled;
>>     216          p_desc.identity = &port->p_identity;
>>     217
>>     218          port->partner = typec_register_partner(port->port, &p_desc);
>>     219          if (IS_ERR_OR_NULL(port->partner)) {
>>     220                  dev_err(typec->dev, "Port %d partner register failed\n",
>>     221                                  port_num);
>>   > 222                  port->partner = NULL;
>>   > 223                  return PTR_ERR(port->partner);
>>     224          }
>>     225
>>     226          ret = cros_typec_query_pd_info(typec, port_num);
>>     227          if (ret < 0) {
>>     228                  dev_err(typec->dev, "Port %d PD query failed\n", port_num);
>>     229                  typec_unregister_partner(port->partner);
>>     230                  port->partner = NULL;
>>     231                  return ret;
>>     232          }
>>     233
>>     234          ret = typec_partner_set_identity(port->partner);
>>     235          return ret;
>>     236  }
>>     237
>>
>> ---
>> 0-DAY kernel test infrastructure                 Open Source Technology Center
>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
> This patch is based off of the chrome-platform for-next branch.
>
> base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git
> for-next
>
> Is this incorrect? I'm happy to rebase if necessary.
>
> Thanks,
> -Jon

Hi Jon,

Thanks for the response, we'll fix the wrong base ASAP.

Best Regards,
Rong Chen

Re: [PATCH 3/3] platform: chrome: Added cros-ec-typec driver

[Guenter Roeck]

On Wed, Nov 13, 2019 at 9:23 AM Jon Flatley <jflat@chromium.org> wrote:
>
> On Wed, Nov 13, 2019 at 4:55 AM kbuild test robot <lkp@intel.com> wrote:
> >
> > Hi Jon,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on ljones-mfd/for-mfd-next]
> > [cannot apply to v5.4-rc7 next-20191113]
> > [if your patch is applied to the wrong git tree, please drop us a note to help
> > improve the system. BTW, we also suggest to use '--base' option to specify the
> > base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
> >
> > url:    https://github.com/0day-ci/linux/commits/Jon-Flatley/ChromeOS-EC-USB-C-Connector-Class/20191113-193504
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
> >
> > If you fix the issue, kindly add following tag
> > Reported-by: kbuild test robot <lkp@intel.com>
> >
> >
> > coccinelle warnings: (new ones prefixed by >>)
> >
> > >> drivers/platform/chrome/cros_ec_typec.c:223:9-16: ERROR: PTR_ERR applied after initialization to constant on line 222
> >
> > vim +223 drivers/platform/chrome/cros_ec_typec.c
> >
> >    206
> >    207  static int cros_typec_add_partner(struct typec_data *typec, int port_num,
> >    208                  bool pd_enabled)
> >    209  {
> >    210          struct port_data *port;
> >    211          struct typec_partner_desc p_desc;
> >    212          int ret;
> >    213
> >    214          port = typec->ports[port_num];
> >    215          p_desc.usb_pd = pd_enabled;
> >    216          p_desc.identity = &port->p_identity;
> >    217
> >    218          port->partner = typec_register_partner(port->port, &p_desc);
> >    219          if (IS_ERR_OR_NULL(port->partner)) {
> >    220                  dev_err(typec->dev, "Port %d partner register failed\n",
> >    221                                  port_num);
> >  > 222                  port->partner = NULL;
> >  > 223                  return PTR_ERR(port->partner);
> >    224          }
> >    225
> >    226          ret = cros_typec_query_pd_info(typec, port_num);
> >    227          if (ret < 0) {
> >    228                  dev_err(typec->dev, "Port %d PD query failed\n", port_num);
> >    229                  typec_unregister_partner(port->partner);
> >    230                  port->partner = NULL;
> >    231                  return ret;
> >    232          }
> >    233
> >    234          ret = typec_partner_set_identity(port->partner);
> >    235          return ret;
> >    236  }
> >    237
> >
> > ---
> > 0-DAY kernel test infrastructure                 Open Source Technology Center
> > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
>
> This patch is based off of the chrome-platform for-next branch.
>
> base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git
> for-next
>
> Is this incorrect? I'm happy to rebase if necessary.
>

You are missing the problem.

              port->partner = NULL;
              return PTR_ERR(port->partner);

Does not make sense, and always returns 0 (no error).

Guenter

> Thanks,
> -Jon

Re: [PATCH 3/3] platform: chrome: Added cros-ec-typec driver

[Heikki Krogerus]

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

On Tue, Nov 12, 2019 at 07:10:44PM -0800, Jon Flatley wrote:
> Adds the cros-ec-typec driver for implementing the USB Type-C connector
> class for cros-ec devices.
> 
> Signed-off-by: Jon Flatley <jflat@chromium.org>
> ---
>  drivers/mfd/cros_ec_dev.c               |   7 +-
>  drivers/platform/chrome/Kconfig         |  11 +
>  drivers/platform/chrome/Makefile        |   1 +
>  drivers/platform/chrome/cros_ec_typec.c | 457 ++++++++++++++++++++++++
>  4 files changed, 473 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/platform/chrome/cros_ec_typec.c
> 
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 6e6dfd6c1871..1136ef986695 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -78,9 +78,10 @@ static const struct mfd_cell cros_ec_rtc_cells[] = {
>  	{ .name = "cros-ec-rtc", },
>  };
>  
> -static const struct mfd_cell cros_usbpd_charger_cells[] = {
> +static const struct mfd_cell cros_usbpd_cells[] = {
>  	{ .name = "cros-usbpd-charger", },
>  	{ .name = "cros-usbpd-logger", },
> +	{ .name = "cros-ec-typec" },
>  };
>  
>  static const struct cros_feature_to_cells cros_subdevices[] = {
> @@ -96,8 +97,8 @@ static const struct cros_feature_to_cells cros_subdevices[] = {
>  	},
>  	{
>  		.id		= EC_FEATURE_USB_PD,
> -		.mfd_cells	= cros_usbpd_charger_cells,
> -		.num_cells	= ARRAY_SIZE(cros_usbpd_charger_cells),
> +		.mfd_cells	= cros_usbpd_cells,
> +		.num_cells	= ARRAY_SIZE(cros_usbpd_cells),
>  	},
>  };
>  
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index d4a55b64bc29..1be5c86f2aad 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -210,6 +210,17 @@ config CROS_EC_SYSFS
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called cros_ec_sysfs.
>  
> +config CROS_EC_TYPEC
> +	tristate "ChromeOS Embedded Controller USB Type-C class subdriver"
> +	depends on TYPEC && CROS_EC_USBPD_NOTIFY
> +	default n
> +	help
> +	  Say Y here to enable the USB Type-C class subdriver for ChromeOS
> +	  devices.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called cros-ec-typec.
> +
>  config CROS_USBPD_LOGGER
>  	tristate "Logging driver for USB PD charger"
>  	depends on CHARGER_CROS_USBPD
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index efa355ab526f..bb298c083f1e 100644
> --- a/drivers/platform/chrome/Makefile
> +++ b/drivers/platform/chrome/Makefile
> @@ -22,5 +22,6 @@ obj-$(CONFIG_CROS_EC_DEBUGFS)		+= cros_ec_debugfs.o
>  obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
>  obj-$(CONFIG_CROS_USBPD_LOGGER)		+= cros_usbpd_logger.o
>  obj-$(CONFIG_CROS_EC_USBPD_NOTIFY)      += cros_ec_usbpd_notify.o
> +obj-$(CONFIG_CROS_EC_TYPEC) 		+= cros_ec_typec.o
>  
>  obj-$(CONFIG_WILCO_EC)			+= wilco_ec/
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> new file mode 100644
> index 000000000000..262b914f0a2e
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -0,0 +1,457 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * cros_ec_usbpd - ChromeOS EC Power Delivery Driver
> + *
> + * Copyright (C) 2019 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/kernel.h>
> +#include <linux/kthread.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_usbpd_notify.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/usb/typec.h>
> +#include <linux/slab.h>
> +
> +#define DRV_NAME "cros-ec-typec"
> +
> +struct port_data {
> +	int port_num;
> +	struct typec_port *port;
> +	struct typec_partner *partner;
> +	struct usb_pd_identity p_identity;
> +	struct typec_data *typec;
> +	struct typec_capability caps;
> +};
> +
> +struct typec_data {
> +	struct device *dev;
> +	struct cros_ec_dev *ec_dev;
> +	struct port_data *ports[EC_USB_PD_MAX_PORTS];
> +	unsigned int num_ports;
> +	struct notifier_block notifier;
> +
> +	int (*port_update)(struct typec_data *typec, int port_num);
> +};
> +
> +#define caps_to_port_data(_caps_) container_of(_caps_, struct port_data, caps)
> +
> +static int cros_typec_ec_command(struct typec_data *typec,
> +				 unsigned int version,
> +				 unsigned int command,
> +				 void *outdata,
> +				 unsigned int outsize,
> +				 void *indata,
> +				 unsigned int insize)
> +{
> +	struct cros_ec_dev *ec_dev = typec->ec_dev;
> +	struct cros_ec_command *msg;
> +	int ret;
> +	char host_cmd[32];
> +
> +	msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	msg->version = version;
> +	msg->command = ec_dev->cmd_offset + command;
> +	msg->outsize = outsize;
> +	msg->insize = insize;
> +
> +	if (outsize)
> +		memcpy(msg->data, outdata, outsize);
> +
> +	sprintf(host_cmd, "typec HC 0x%x req: ", command);
> +	print_hex_dump(KERN_DEBUG, host_cmd, DUMP_PREFIX_NONE, 16, 1, outdata,
> +			outsize, 0);
> +
> +	ret = cros_ec_cmd_xfer_status(typec->ec_dev->ec_dev, msg);
> +	if (ret >= 0 && insize)
> +		memcpy(indata, msg->data, insize);
> +	sprintf(host_cmd, "typec HC 0x%x res: ", command);
> +	print_hex_dump(KERN_DEBUG, host_cmd, DUMP_PREFIX_NONE, 16, 1, indata,
> +			insize, 0);
> +
> +	kfree(msg);
> +	return ret;
> +}
> +
> +static int cros_typec_get_cmd_version(struct typec_data *typec, int cmd,
> +		uint8_t *version_mask)
> +{
> +	struct ec_params_get_cmd_versions req_v0;
> +	struct ec_params_get_cmd_versions_v1 req_v1;
> +	struct ec_response_get_cmd_versions res;
> +	int ret;
> +
> +	req_v1.cmd = cmd;
> +	ret = cros_typec_ec_command(typec, 1, EC_CMD_GET_CMD_VERSIONS, &req_v1,
> +			sizeof(req_v1), &res, sizeof(res));
> +	if (ret < 0) {
> +		req_v0.cmd = cmd;
> +		ret = cros_typec_ec_command(typec, 0, EC_CMD_GET_CMD_VERSIONS,
> +				&req_v0, sizeof(req_v0), &res, sizeof(res));
> +		if (ret < 0)
> +			return ret;
> +	}
> +	*version_mask = res.version_mask;
> +	dev_dbg(typec->dev, "EC CMD 0x%hhx has version mask 0x%hhx\n", cmd,
> +			*version_mask);
> +	return 0;
> +}
> +
> +static int cros_typec_query_pd_port_count(struct typec_data *typec)
> +{
> +	struct ec_response_usb_pd_ports res;
> +	int ret;
> +
> +	ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_PORTS, NULL, 0,
> +			&res, sizeof(res));
> +	if (ret < 0)
> +		return ret;
> +	typec->num_ports = res.num_ports;
> +	return 0;
> +}
> +
> +static int cros_typec_port_update(struct typec_data *typec,
> +				  int port_num,
> +				  struct ec_response_usb_pd_control *res,
> +				  size_t res_size,
> +				  int cmd_ver)
> +{
> +	struct port_data *port;
> +	struct ec_params_usb_pd_control req;
> +	int ret;
> +
> +	if (port_num < 0 || port_num >= typec->num_ports) {
> +		dev_err(typec->dev, "cannot get status for invalid port %d\n",
> +				port_num);
> +		return -EPERM;
> +	}
> +	port = typec->ports[port_num];
> +
> +	req.port = port_num;
> +	req.role = USB_PD_CTRL_ROLE_NO_CHANGE;
> +	req.mux = USB_PD_CTRL_MUX_NO_CHANGE;
> +	req.swap = USB_PD_CTRL_SWAP_NONE;

Unless I'm mistaken, those constants are all 0. How about this:

	struct ec_params_usb_pd_control req = { };
        ...
        req.port = port_num;

> +	ret = cros_typec_ec_command(typec, cmd_ver, EC_CMD_USB_PD_CONTROL, &req,
> +			sizeof(req), res, res_size);
> +	if (ret < 0)
> +		return ret;
> +	dev_dbg(typec->dev, "Enabled %d: 0x%hhx\n", port_num, res->enabled);
> +	dev_dbg(typec->dev, "Role %d: 0x%hhx\n", port_num, res->role);
> +	dev_dbg(typec->dev, "Polarity %d: 0x%hhx\n", port_num, res->polarity);
> +
> +	return 0;
> +}
> +
> +static int cros_typec_query_pd_info(struct typec_data *typec, int port_num)
> +{
> +	struct ec_params_usb_pd_info_request req;
> +	struct ec_params_usb_pd_discovery_entry res;
> +	int ret;
> +
> +	req.port = port_num;
> +	ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_DISCOVERY, &req,
> +			sizeof(req), &res, sizeof(res));
> +	if (ret < 0)
> +		return ret;
> +	/* FIXME Needs the rest of the info in PD Spec 6.4.4.3.1.1 */
> +	typec->ports[port_num]->p_identity.id_header = res.vid;
> +
> +	/* FIXME Needs bcdDevice to match PD Spec 6.4.4.3.1.3 */
> +	typec->ports[port_num]->p_identity.product = res.pid << 16;
> +	return 0;
> +}
> +
> +static int cros_typec_port_update_v0(struct typec_data *typec, int port_num)
> +{
> +	struct port_data *port;
> +	struct ec_response_usb_pd_control res;
> +	enum typec_orientation polarity;
> +	int ret;
> +
> +	port = typec->ports[port_num];
> +	ret = cros_typec_port_update(typec, port_num, &res, sizeof(res), 0);
> +	if (ret < 0)
> +		return ret;
> +	dev_dbg(typec->dev, "State %d: %hhx\n", port_num, res.state);
> +
> +	if (!res.enabled)
> +		polarity = TYPEC_ORIENTATION_NONE;
> +	else if (!res.polarity)
> +		polarity = TYPEC_ORIENTATION_NORMAL;
> +	else
> +		polarity = TYPEC_ORIENTATION_REVERSE;
> +
> +	typec_set_pwr_role(port->port, res.role ? TYPEC_SOURCE : TYPEC_SINK);
> +	typec_set_orientation(port->port, polarity);
> +
> +	return 0;
> +}
> +
> +static int cros_typec_add_partner(struct typec_data *typec, int port_num,
> +		bool pd_enabled)
> +{
> +	struct port_data *port;
> +	struct typec_partner_desc p_desc;
> +	int ret;
> +
> +	port = typec->ports[port_num];
> +	p_desc.usb_pd = pd_enabled;
> +	p_desc.identity = &port->p_identity;
> +
> +	port->partner = typec_register_partner(port->port, &p_desc);
> +	if (IS_ERR_OR_NULL(port->partner)) {
> +		dev_err(typec->dev, "Port %d partner register failed\n",
> +				port_num);
> +		port->partner = NULL;
> +		return PTR_ERR(port->partner);

That is the same as:
                return PTR_ERR(NULL);

> +	}
> +
> +	ret = cros_typec_query_pd_info(typec, port_num);
> +	if (ret < 0) {
> +		dev_err(typec->dev, "Port %d PD query failed\n", port_num);
> +		typec_unregister_partner(port->partner);
> +		port->partner = NULL;
> +		return ret;
> +	}
> +
> +	ret = typec_partner_set_identity(port->partner);
> +	return ret;
> +}
> +
> +static int cros_typec_set_port_params_v1_v2(struct typec_data *typec,
> +		int port_num, struct ec_response_usb_pd_control_v1 *res)
> +{
> +	struct port_data *port;
> +	enum typec_orientation polarity;
> +	int ret;
> +
> +	port = typec->ports[port_num];
> +	if (!(res->enabled & PD_CTRL_RESP_ENABLED_CONNECTED))
> +		polarity = TYPEC_ORIENTATION_NONE;
> +	else if (!res->polarity)
> +		polarity = TYPEC_ORIENTATION_NORMAL;
> +	else
> +		polarity = TYPEC_ORIENTATION_REVERSE;
> +	typec_set_orientation(port->port, polarity);
> +	typec_set_data_role(port->port, res->role & PD_CTRL_RESP_ROLE_DATA ?
> +			TYPEC_HOST : TYPEC_DEVICE);
> +	typec_set_pwr_role(port->port, res->role & PD_CTRL_RESP_ROLE_POWER ?
> +			TYPEC_SOURCE : TYPEC_SINK);
> +	typec_set_vconn_role(port->port, res->role & PD_CTRL_RESP_ROLE_VCONN ?
> +			TYPEC_SOURCE : TYPEC_SINK);
> +
> +	if (res->enabled & PD_CTRL_RESP_ENABLED_CONNECTED && !port->partner) {
> +		bool pd_enabled =
> +			res->enabled & PD_CTRL_RESP_ENABLED_PD_CAPABLE;
> +		ret = cros_typec_add_partner(typec, port_num, pd_enabled);
> +		if (!ret)
> +			return ret;
> +	}
> +	if (!(res->enabled & PD_CTRL_RESP_ENABLED_CONNECTED) && port->partner) {
> +		typec_unregister_partner(port->partner);
> +		port->partner = NULL;
> +	}
> +	return 0;
> +}
> +
> +static int cros_typec_port_update_v1(struct typec_data *typec, int port_num)
> +{
> +	/*struct port_data *port;*/
> +	struct ec_response_usb_pd_control_v1 res;
> +	struct ec_response_usb_pd_control *res_v0 =
> +		(struct ec_response_usb_pd_control *) &res;
> +	int ret;
> +
> +	ret = cros_typec_port_update(typec, port_num, res_v0, sizeof(res), 1);
> +	if (ret < 0)
> +		return ret;
> +	dev_dbg(typec->dev, "State %d: %s\n", port_num, res.state);
> +
> +	ret = cros_typec_set_port_params_v1_v2(typec, port_num, &res);
> +	return ret;
> +}
> +
> +static int cros_typec_try_role(const struct typec_capability *cap, int role)
> +{
> +	return 0;
> +}
> +
> +static int cros_typec_dr_set(const struct typec_capability *cap,
> +		enum typec_data_role role)
> +{
> +	return 0;
> +}
> +
> +static int cros_typec_pr_set(const struct typec_capability *cap,
> +		enum typec_role role)
> +{
> +	return 0;
> +}
> +
> +static int cros_typec_vconn_set(const struct typec_capability *cap,
> +		enum typec_role role)
> +{
> +	return 0;
> +}

Those stubs are not needed.

> +static int cros_typec_ec_event(struct notifier_block *nb,
> +			       unsigned long queued_during_suspend,
> +			       void *_notify)
> +{
> +	struct typec_data *typec;
> +	int i;
> +
> +	typec = container_of(nb, struct typec_data, notifier);
> +
> +	for (i = 0; i < typec->num_ports; ++i)
> +		typec->port_update(typec, i);
> +
> +	return NOTIFY_DONE;
> +
> +}
> +
> +static void cros_typec_unregister_notifier(void *data)
> +{
> +	struct typec_data *typec = data;
> +
> +	cros_ec_usbpd_unregister_notify(&typec->notifier);
> +}
> +
> +static int cros_typec_probe(struct platform_device *pd)
> +{
> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
> +	struct device *dev = &pd->dev;
> +	struct typec_data *typec;
> +	uint8_t ver_mask;
> +	int i;
> +	int ret;
> +
> +	dev_dbg(dev, "Probing Cros EC Type-C device.\n");
> +
> +	typec = devm_kzalloc(dev, sizeof(*typec), GFP_KERNEL);
> +	if (!typec)
> +		return -ENOMEM;
> +
> +	typec->dev = dev;
> +	typec->ec_dev = ec_dev;
> +
> +	platform_set_drvdata(pd, typec);
> +
> +	ret = cros_typec_query_pd_port_count(typec);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to get PD port count from EC\n");
> +		return ret;
> +	}
> +	if (typec->num_ports > EC_USB_PD_MAX_PORTS) {
> +		dev_err(dev, "EC reported too many ports. got: %d, max: %d\n",
> +				typec->num_ports, EC_USB_PD_MAX_PORTS);
> +		return -EOVERFLOW;
> +	}

This step should to be done in the mfd driver. You can then register
a separate device for each port.

For exact hardware description you will need a software node that
represents the "port controller" that you'll give to this device. That
software node will need a child node named "connector" that represents
the Type-C connector. That child node has the actual capability
properties.

I just remembered that the MFD framework does not yet support software
nodes directly, but I have a patch for that. I'm attaching here.

For examples how to use software nodes you can check
drivers/platform/x86/intel_cht_int33fe_typec.c

Or if you prefer, I can also write the mfd patch for you?

> +	ret = cros_typec_get_cmd_version(typec, EC_CMD_USB_PD_CONTROL,
> +			&ver_mask);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to get supported PD command versions\n");
> +		return ret;
> +	}
> +	/* No reason to support EC_CMD_USB_PD_CONTROL v2 as it doesn't add any
> +	 * useful information
> +	 */
> +	if (ver_mask & EC_VER_MASK(1)) {
> +		dev_dbg(dev, "Using PD command ver 1\n");
> +		typec->port_update = cros_typec_port_update_v1;
> +	} else {
> +		dev_dbg(dev, "Using PD command ver 0\n");
> +		typec->port_update = cros_typec_port_update_v0;
> +	}
> +
> +	for (i = 0; i < typec->num_ports; ++i) {

After there is a one device per port, this loop is of course no longer
needed. This driver just needs to know the port number that its port
has, and that it can get also from a device property. That property
you need to give to the parent software node that represents the
"port controller", so not the "connector" child node.

> +		struct port_data *port;
> +
> +		port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
> +		if (!port) {
> +			ret = -ENOMEM;
> +			goto unregister_ports;
> +		}
> +		port->typec = typec;
> +		port->port_num = i;
> +		typec->ports[i] = port;
> +
> +		/* TODO Make sure these values are accurate */
> +		port->caps.type = TYPEC_PORT_DRP;
> +		port->caps.data = TYPEC_PORT_DFP;
> +		port->caps.prefer_role = TYPEC_SINK;

These are details that we can get from the device properties.

> +		port->caps.try_role = cros_typec_try_role;
> +		port->caps.dr_set = cros_typec_dr_set;
> +		port->caps.pr_set = cros_typec_pr_set;
> +		port->caps.vconn_set = cros_typec_vconn_set;
> +		port->caps.port_type_set = NULL; /* Not permitted by PD spec */
> +
> +		port->port = typec_register_port(dev, &port->caps);
> +		if (IS_ERR_OR_NULL(port->port)) {
> +			dev_err(dev, "Failed to register typec port %d\n", i);
> +			ret = PTR_ERR(port->port);
> +			goto unregister_ports;
> +		}
> +
> +		ret = typec->port_update(typec, i);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to update typec port %d\n", i);
> +			goto unregister_ports;
> +		}
> +	}
> +
> +	typec->notifier.notifier_call = cros_typec_ec_event;
> +	ret = cros_ec_usbpd_register_notify(&typec->notifier);
> +	if (ret < 0) {
> +		dev_warn(dev, "Failed to register notifier\n");
> +	} else {
> +		ret = devm_add_action_or_reset(dev,
> +				cros_typec_unregister_notifier, typec);
> +		if (ret < 0)
> +			goto unregister_ports;
> +		dev_dbg(dev, "Registered EC notifier\n");
> +	}
> +
> +	return 0;
> +
> +unregister_ports:
> +	for (i = 0; i < typec->num_ports; ++i) {
> +		if (typec->ports[i] && typec->ports[i]->port)
> +			typec_unregister_port(typec->ports[i]->port);
> +	}
> +
> +	return ret;
> +}
> +
> +static struct platform_driver cros_ec_typec_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +	},
> +	.probe = cros_typec_probe
> +};
> +
> +module_platform_driver(cros_ec_typec_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("ChromeOS EC USB-C connectors");
> +MODULE_AUTHOR("Jon Flatley <jflat@chromium.org>");
> +MODULE_ALIAS("platform:" DRV_NAME);


thanks,

-- 
heikki

[-- Attachment #2: 0001-mfd-core-Propagate-software-fwnode-to-the-sub-device.patch --]
[-- Type: text/plain, Size: 1846 bytes --]

From dc204a8644fd4bb7aac9492e6ee304d81be7dbbd Mon Sep 17 00:00:00 2001
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Date: Mon, 5 Aug 2019 14:54:37 +0300
Subject: [PATCH] mfd: core: Propagate software fwnode to the sub devices

When ever device properties are supplied for a sub device, a
software node (fwnode) is actually created and then
associated with that device. By allowing the drivers to
supply the complete software node instead of just the
properties in it, the drivers can take advantage of the
other features the software nodes have on top of supplying
the device properties.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/mfd/mfd-core.c   | 8 ++++++++
 include/linux/mfd/core.h | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index cb3e0a14bbdd..3fda7e420c5d 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -174,6 +174,14 @@ static int mfd_add_device(struct device *parent, int id,
 			goto fail_alias;
 	}
 
+	if (cell->node) {
+		ret = software_node_register(cell->node);
+		if (ret)
+			goto fail_alias;
+
+		pdev->dev.fwnode = software_node_fwnode(cell->node);
+	}
+
 	for (r = 0; r < cell->num_resources; r++) {
 		res[r].name = cell->resources[r].name;
 		res[r].flags = cell->resources[r].flags;
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index d01d1299e49d..5ec90ba5865a 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -72,6 +72,9 @@ struct mfd_cell {
 	/* device properties passed to the sub devices drivers */
 	struct property_entry *properties;
 
+	/* Software fwnode for the sub device */
+	const struct software_node *node;
+
 	/*
 	 * Device Tree compatible string
 	 * See: Documentation/devicetree/usage-model.txt Chapter 2.2 for details
-- 
2.24.0

Re: [PATCH 0/3] ChromeOS EC USB-C Connector Class

[Benson Leung]

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

Hi Jon,

Thanks for posting this.

Adding Heikki, the typec connector class maintainer, and Enric, co-maintainer
of platform/chrome.

Benson

On Tue, Nov 12, 2019 at 07:10:41PM -0800, Jon Flatley wrote:
> This patch set adds a basic implementation of the USB-C connector class for
> devices using the ChromeOS EC. On ACPI devices an additional ACPI driver is
> necessary to receive USB-C PD host events from the PD EC device "GOOG0003".
> Incidentally, this ACPI driver adds notifications for events that
> cros-usbpd-charger has been missing, so fix that while we're at it.
> 
> Jon Flatley (3):
>   platform: chrome: Add cros-ec-usbpd-notify driver
>   power: supply: cros-ec-usbpd-charger: Fix host events
>   platform: chrome: Added cros-ec-typec driver
> 
>  drivers/mfd/cros_ec_dev.c                     |   7 +-
>  drivers/platform/chrome/Kconfig               |  20 +
>  drivers/platform/chrome/Makefile              |   2 +
>  drivers/platform/chrome/cros_ec_typec.c       | 457 ++++++++++++++++++
>  .../platform/chrome/cros_ec_usbpd_notify.c    | 156 ++++++
>  drivers/power/supply/Kconfig                  |   2 +-
>  drivers/power/supply/cros_usbpd-charger.c     |  45 +-
>  .../platform_data/cros_ec_usbpd_notify.h      |  40 ++
>  8 files changed, 696 insertions(+), 33 deletions(-)
>  create mode 100644 drivers/platform/chrome/cros_ec_typec.c
>  create mode 100644 drivers/platform/chrome/cros_ec_usbpd_notify.c
>  create mode 100644 include/linux/platform_data/cros_ec_usbpd_notify.h
> 
> -- 
> 2.24.0.432.g9d3f5f5b63-goog
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

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

Re: [PATCH 0/3] ChromeOS EC USB-C Connector Class

[Heikki Krogerus]

Hi guys,

On Wed, Nov 13, 2019 at 09:51:27AM -0800, Benson Leung wrote:
> Hi Jon,
> 
> Thanks for posting this.
> 
> Adding Heikki, the typec connector class maintainer, and Enric, co-maintainer
> of platform/chrome.

Thanks Benson.

> On Tue, Nov 12, 2019 at 07:10:41PM -0800, Jon Flatley wrote:
> > This patch set adds a basic implementation of the USB-C connector class for
> > devices using the ChromeOS EC. On ACPI devices an additional ACPI driver is
> > necessary to receive USB-C PD host events from the PD EC device "GOOG0003".
> > Incidentally, this ACPI driver adds notifications for events that
> > cros-usbpd-charger has been missing, so fix that while we're at it.
> 
> > Jon Flatley (3):
> >   platform: chrome: Add cros-ec-usbpd-notify driver
> >   power: supply: cros-ec-usbpd-charger: Fix host events
> >   platform: chrome: Added cros-ec-typec driver
> > 
> >  drivers/mfd/cros_ec_dev.c                     |   7 +-
> >  drivers/platform/chrome/Kconfig               |  20 +
> >  drivers/platform/chrome/Makefile              |   2 +
> >  drivers/platform/chrome/cros_ec_typec.c       | 457 ++++++++++++++++++
> >  .../platform/chrome/cros_ec_usbpd_notify.c    | 156 ++++++
> >  drivers/power/supply/Kconfig                  |   2 +-
> >  drivers/power/supply/cros_usbpd-charger.c     |  45 +-
> >  .../platform_data/cros_ec_usbpd_notify.h      |  40 ++
> >  8 files changed, 696 insertions(+), 33 deletions(-)
> >  create mode 100644 drivers/platform/chrome/cros_ec_typec.c
> >  create mode 100644 drivers/platform/chrome/cros_ec_usbpd_notify.c
> >  create mode 100644 include/linux/platform_data/cros_ec_usbpd_notify.h

I'll go over these tomorrow, but I have one question already. Can you
guys influence what goes to the ACPI tables?

Ideally every Type-C connector is always described in its own ACPI
node (or DT node if DT is used). Otherwise getting the correct
capabilities and especially connections to other devices (like the
muxes) for every port may get difficult.

Br,

-- 
heikki

Re: [PATCH 0/3] ChromeOS EC USB-C Connector Class

[Jon Flatley]

On Wed, Nov 13, 2019 at 10:25 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi guys,
>
> On Wed, Nov 13, 2019 at 09:51:27AM -0800, Benson Leung wrote:
> > Hi Jon,
> >
> > Thanks for posting this.
> >
> > Adding Heikki, the typec connector class maintainer, and Enric, co-maintainer
> > of platform/chrome.
>
> Thanks Benson.
>
> > On Tue, Nov 12, 2019 at 07:10:41PM -0800, Jon Flatley wrote:
> > > This patch set adds a basic implementation of the USB-C connector class for
> > > devices using the ChromeOS EC. On ACPI devices an additional ACPI driver is
> > > necessary to receive USB-C PD host events from the PD EC device "GOOG0003".
> > > Incidentally, this ACPI driver adds notifications for events that
> > > cros-usbpd-charger has been missing, so fix that while we're at it.
> >
> > > Jon Flatley (3):
> > >   platform: chrome: Add cros-ec-usbpd-notify driver
> > >   power: supply: cros-ec-usbpd-charger: Fix host events
> > >   platform: chrome: Added cros-ec-typec driver
> > >
> > >  drivers/mfd/cros_ec_dev.c                     |   7 +-
> > >  drivers/platform/chrome/Kconfig               |  20 +
> > >  drivers/platform/chrome/Makefile              |   2 +
> > >  drivers/platform/chrome/cros_ec_typec.c       | 457 ++++++++++++++++++
> > >  .../platform/chrome/cros_ec_usbpd_notify.c    | 156 ++++++
> > >  drivers/power/supply/Kconfig                  |   2 +-
> > >  drivers/power/supply/cros_usbpd-charger.c     |  45 +-
> > >  .../platform_data/cros_ec_usbpd_notify.h      |  40 ++
> > >  8 files changed, 696 insertions(+), 33 deletions(-)
> > >  create mode 100644 drivers/platform/chrome/cros_ec_typec.c
> > >  create mode 100644 drivers/platform/chrome/cros_ec_usbpd_notify.c
> > >  create mode 100644 include/linux/platform_data/cros_ec_usbpd_notify.h
>
> I'll go over these tomorrow, but I have one question already. Can you
> guys influence what goes to the ACPI tables?
>
> Ideally every Type-C connector is always described in its own ACPI
> node (or DT node if DT is used). Otherwise getting the correct
> capabilities and especially connections to other devices (like the
> muxes) for every port may get difficult.

Hey Heikki, thank you for your quick response!

In general we do have control over the ACPI tables and over DT. The
difference for ChromeOS is that the PD implementation and policy
decisions are handled by the embedded controller. This includes
alternate mode transitions and control over the muxes. I don't believe
there is any information about port capabilities in ACPI or DT, that's
all handled by the EC. With current EC firmware we are mostly limited
to querying the EC for port capabilities and state. There may be some
exceptions to this, such as with Rockchip platforms, but even then the
EC is largely in control.

I think you raise a valid point, but such a change is probably out of
scope for this implementation.

Thanks,
-Jon

>
> Br,
>
> --
> heikki

Re: [PATCH 0/3] ChromeOS EC USB-C Connector Class

[Heikki Krogerus]

Hi Jon,

On Wed, Nov 13, 2019 at 05:09:56PM -0800, Jon Flatley wrote:
> > I'll go over these tomorrow, but I have one question already. Can you
> > guys influence what goes to the ACPI tables?
> >
> > Ideally every Type-C connector is always described in its own ACPI
> > node (or DT node if DT is used). Otherwise getting the correct
> > capabilities and especially connections to other devices (like the
> > muxes) for every port may get difficult.
> 
> Hey Heikki, thank you for your quick response!
> 
> In general we do have control over the ACPI tables and over DT. The
> difference for ChromeOS is that the PD implementation and policy
> decisions are handled by the embedded controller. This includes
> alternate mode transitions and control over the muxes. I don't believe
> there is any information about port capabilities in ACPI or DT, that's
> all handled by the EC. With current EC firmware we are mostly limited
> to querying the EC for port capabilities and state. There may be some
> exceptions to this, such as with Rockchip platforms, but even then the
> EC is largely in control.

The capabilities here mean things like is the port: source, sink or
DRP; host, device or DRD; etc. So static information.

I do understand that the EC is in control of the Port Controller (or
PD controller), the muxes, the policy decisions and what have you, and
that is fine. My point is that the operating system should not have to
get also the hardware description from the EC. That part should always
come from ACPI tables or DT, even when the components are attached to
the EC instead of the host CPU. Otherwise we loose scalability for no
good reason.

Note. The device properties for the port capabilities are already
documented in kernel:
Documentation/devicetree/bindings/connector/usb-connector.txt (the
same properties work in ACPI as well).

> I think you raise a valid point, but such a change is probably out of
> scope for this implementation.

This implementation should already be made so that it works with a
properly prepared ACPI tables or DT. If there are already boards that
don't supply the nodes in ACPI tables for the ports, then software
nodes can be used with those, but all new boards really should have a
real firmware node represeting every Type-C port.

thanks,

-- 
heikki

Re: [PATCH 0/3] ChromeOS EC USB-C Connector Class

[Jon Flatley]

On Thu, Nov 14, 2019 at 7:24 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Jon,
>
> On Wed, Nov 13, 2019 at 05:09:56PM -0800, Jon Flatley wrote:
> > > I'll go over these tomorrow, but I have one question already. Can you
> > > guys influence what goes to the ACPI tables?
> > >
> > > Ideally every Type-C connector is always described in its own ACPI
> > > node (or DT node if DT is used). Otherwise getting the correct
> > > capabilities and especially connections to other devices (like the
> > > muxes) for every port may get difficult.
> >
> > Hey Heikki, thank you for your quick response!
> >
> > In general we do have control over the ACPI tables and over DT. The
> > difference for ChromeOS is that the PD implementation and policy
> > decisions are handled by the embedded controller. This includes
> > alternate mode transitions and control over the muxes. I don't believe
> > there is any information about port capabilities in ACPI or DT, that's
> > all handled by the EC. With current EC firmware we are mostly limited
> > to querying the EC for port capabilities and state. There may be some
> > exceptions to this, such as with Rockchip platforms, but even then the
> > EC is largely in control.
>
> The capabilities here mean things like is the port: source, sink or
> DRP; host, device or DRD; etc. So static information.
>
> I do understand that the EC is in control of the Port Controller (or
> PD controller), the muxes, the policy decisions and what have you, and
> that is fine. My point is that the operating system should not have to
> get also the hardware description from the EC. That part should always
> come from ACPI tables or DT, even when the components are attached to
> the EC instead of the host CPU. Otherwise we loose scalability for no
> good reason.
>
> Note. The device properties for the port capabilities are already
> documented in kernel:
> Documentation/devicetree/bindings/connector/usb-connector.txt (the
> same properties work in ACPI as well).
>
> > I think you raise a valid point, but such a change is probably out of
> > scope for this implementation.
>
> This implementation should already be made so that it works with a
> properly prepared ACPI tables or DT. If there are already boards that
> don't supply the nodes in ACPI tables for the ports, then software
> nodes can be used with those, but all new boards really should have a
> real firmware node represeting every Type-C port.

Hey Heikki,

I spoke with Benson and Prashant and you raise a good point. Moving
forward we should probably be describing these capabilities in ACPI.
We do want to support existing devices, and making changes to the ACPI
tables would mean firmware modifications for each and every one, which
is a complicated process.

To date the port capabilities on all ChromeOS devices have been the
same. I recall now that we don't (and with current firmware can't)
query the port capabilities from the EC; they're just hard coded into
the driver. In the absence of these nodes in the ACPI tables we can
populate these capabilities in software nodes. This would allow us to
support existing systems without the expensive firmware change, and I
think it still provides the scalability you're asking for.

Are you suggesting that every port on the device gets its own ACPI/DT node?

Thanks,
-Jon

>
> thanks,

>
> --
> heikki

Re: [PATCH 0/3] ChromeOS EC USB-C Connector Class

[Heikki Krogerus]

Hi,

On Thu, Nov 14, 2019 at 01:00:42PM -0800, Jon Flatley wrote:
> On Thu, Nov 14, 2019 at 7:24 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Hi Jon,
> >
> > On Wed, Nov 13, 2019 at 05:09:56PM -0800, Jon Flatley wrote:
> > > > I'll go over these tomorrow, but I have one question already. Can you
> > > > guys influence what goes to the ACPI tables?
> > > >
> > > > Ideally every Type-C connector is always described in its own ACPI
> > > > node (or DT node if DT is used). Otherwise getting the correct
> > > > capabilities and especially connections to other devices (like the
> > > > muxes) for every port may get difficult.
> > >
> > > Hey Heikki, thank you for your quick response!
> > >
> > > In general we do have control over the ACPI tables and over DT. The
> > > difference for ChromeOS is that the PD implementation and policy
> > > decisions are handled by the embedded controller. This includes
> > > alternate mode transitions and control over the muxes. I don't believe
> > > there is any information about port capabilities in ACPI or DT, that's
> > > all handled by the EC. With current EC firmware we are mostly limited
> > > to querying the EC for port capabilities and state. There may be some
> > > exceptions to this, such as with Rockchip platforms, but even then the
> > > EC is largely in control.
> >
> > The capabilities here mean things like is the port: source, sink or
> > DRP; host, device or DRD; etc. So static information.
> >
> > I do understand that the EC is in control of the Port Controller (or
> > PD controller), the muxes, the policy decisions and what have you, and
> > that is fine. My point is that the operating system should not have to
> > get also the hardware description from the EC. That part should always
> > come from ACPI tables or DT, even when the components are attached to
> > the EC instead of the host CPU. Otherwise we loose scalability for no
> > good reason.
> >
> > Note. The device properties for the port capabilities are already
> > documented in kernel:
> > Documentation/devicetree/bindings/connector/usb-connector.txt (the
> > same properties work in ACPI as well).
> >
> > > I think you raise a valid point, but such a change is probably out of
> > > scope for this implementation.
> >
> > This implementation should already be made so that it works with a
> > properly prepared ACPI tables or DT. If there are already boards that
> > don't supply the nodes in ACPI tables for the ports, then software
> > nodes can be used with those, but all new boards really should have a
> > real firmware node represeting every Type-C port.
> 
> Hey Heikki,
> 
> I spoke with Benson and Prashant and you raise a good point. Moving
> forward we should probably be describing these capabilities in ACPI.
> We do want to support existing devices, and making changes to the ACPI
> tables would mean firmware modifications for each and every one, which
> is a complicated process.
> 
> To date the port capabilities on all ChromeOS devices have been the
> same. I recall now that we don't (and with current firmware can't)
> query the port capabilities from the EC; they're just hard coded into
> the driver. In the absence of these nodes in the ACPI tables we can
> populate these capabilities in software nodes. This would allow us to
> support existing systems without the expensive firmware change, and I
> think it still provides the scalability you're asking for.
> 
> Are you suggesting that every port on the device gets its own ACPI/DT node?

Yes. Every port (or maybe I should say "connector") should always have
its own ACPI/DT node.

There should also be a separate parent node for the port nodes that
represents the USB Type-C controller part (in EC), meaning you
probable don't want to simply make the port nodes child nodes directly
under the root EC node (the one that has ACPI HID GOOG0004 or
GOOG0008). The controller node is the one that is child of the root EC
node, and the port nodes are children of the controller. The
controller node ideally will have its own ACPI HID, but the port nodes
themselves don't need HIDs. I guess this part is clear..

I proposed in my review (patch 3/3) that there should be actually a
single parent controller node for every port, so that you would have
always a controller node with a single port node for every connector,
but that may be overkill. I was thinking about past problems where it
is no clear which port (number) should be mapped to which port node,
but you do not have that problem.

So I think you only need a single "EC Type-C Controller" node that
is the parent for the port nodes.

I'll put here a short ASL example snippet that has nodes for two
ports, that hopefully helps explain how I think this should be
described in the ACPI tables:

DefinitionBlock ("usbc.aml", "SSDT", 5, "GOOGLE", "USBC", 1)
{
    /* I'm assuming here that H_EC is the EC device. */
    Scope (\_SB.H_EC)
    {
        /*
         * This is the controller (port parent) device that communicates with
         * the EC. It is the node that would create the device instance for the
         * driver (drivers/platform/chrome/cros_ec_typec.c).
         */
        Device (USBC)
        {
            Name (_HID, "GOOGXXXX") /* FIXME!!!!!! */
            Name (_DDN, "ChromeOS Embedded Controller USB Type-C Control")

            /*
             * Each connector shall have its own ACPI device entry (node),
             * under the actual interface (controller) device.
             */
            Device (CON1)
            {
                /* Port number 1 */
                Name (_ADR, One)

                /* The port capability device properties. */
                Name (_DSD, Package () {
                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                    Package() {
                        Package () {"power-role", "dual"},
                        Package () {"data-role", "dual"},
                    },
                })
            }

            Device (CON2)
            {
                /* Port number 2 */
                Name (_ADR, Two)

                Name (_DSD, Package () {
                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                    Package() {
                        Package () {"power-role", "dual"},
                        Package () {"data-role", "dual"},
                    },
                })
            }
        }
    }
}

To play it safe (and be compatible with DT), the port number does not
have to be the node address (_ADR). You can get the port number also
from a device property as well of course.

Br,

-- 
heikki