All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] platform: chrome: Add cros-ec-pd-notify driver
@ 2019-12-18 23:07 Prashant Malani
  2019-12-19  9:44 ` Enric Balletbo i Serra
  2019-12-22  1:27   ` kbuild test robot
  0 siblings, 2 replies; 5+ messages in thread
From: Prashant Malani @ 2019-12-18 23:07 UTC (permalink / raw)
  To: enric.balletbo, groeck, bleung, lee.jones
  Cc: linux-kernel, Jon Flatley, Prashant Malani

From: Jon Flatley <jflat@chromium.org>

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.

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.

On non-ACPI platforms, the driver gets instantiated for ECs which
support the EC_FEATURE_USB_PD feature bit, and on such platforms, the
notification events will get delivered using the MKBP event handling
mechanism.

Signed-off-by: Jon Flatley <jflat@chromium.org>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---

Changes in v2 (pmalani@chromium.org):
- Removed dependency on DT entry; instead, we will instantiate the
  driver on detecting EC_FEATURE_USB_PD for non-ACPI platforms.
- Modified the cros-ec-pd-notify device to be an mfd_cell under
  usbpdcharger for non-ACPI platforms. Altered the platform_probe() call
  to derive the cros EC structs appropriately.
- Replaced "usbpd_notify" with "pd_notify" in functions and structures.
- Addressed comments from upstream maintainer.

 drivers/mfd/cros_ec_dev.c                     |   3 +
 drivers/platform/chrome/Kconfig               |   9 ++
 drivers/platform/chrome/Makefile              |   1 +
 drivers/platform/chrome/cros_ec_pd_notify.c   | 151 ++++++++++++++++++
 .../linux/platform_data/cros_ec_pd_notify.h   |  17 ++
 5 files changed, 181 insertions(+)
 create mode 100644 drivers/platform/chrome/cros_ec_pd_notify.c
 create mode 100644 include/linux/platform_data/cros_ec_pd_notify.h

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index c4b977a5dd966..cc6a6788cd5c2 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -85,6 +85,9 @@ static const struct mfd_cell cros_ec_sensorhub_cells[] = {
 static const struct mfd_cell cros_usbpd_charger_cells[] = {
 	{ .name = "cros-usbpd-charger", },
 	{ .name = "cros-usbpd-logger", },
+#ifndef CONFIG_ACPI
+	{ .name = "cros-ec-pd-notify", },
+#endif
 };
 
 static const struct cros_feature_to_cells cros_subdevices[] = {
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 5f57282a28da0..972c129b7b7ba 100644
--- a/drivers/platform/chrome/Kconfig
+++ b/drivers/platform/chrome/Kconfig
@@ -131,6 +131,15 @@ config CROS_EC_LPC
 	  To compile this driver as a module, choose M here: the
 	  module will be called cros_ec_lpcs.
 
+config CROS_EC_PD_NOTIFY
+	tristate "ChromeOS Type-C power delivery event notifier"
+	depends on CROS_EC
+	help
+	  If you say Y here, you get support for Type-C PD event notifications
+	  from the ChromeOS EC. On ACPI platorms this driver will bind to the
+	  GOOG0003 ACPI device, and on non-ACPI platforms this driver will get
+	  initialized on ECs which support the feature EC_FEATURE_USB_PD.
+
 config CROS_EC_PROTO
 	bool
 	help
diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
index aacd5920d8a18..6070baa8320ca 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_SENSORHUB)		+= cros_ec_sensorhub.o
 obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
 obj-$(CONFIG_CROS_USBPD_LOGGER)		+= cros_usbpd_logger.o
+obj-$(CONFIG_CROS_EC_PD_NOTIFY)		+= cros_ec_pd_notify.o
 
 obj-$(CONFIG_WILCO_EC)			+= wilco_ec/
diff --git a/drivers/platform/chrome/cros_ec_pd_notify.c b/drivers/platform/chrome/cros_ec_pd_notify.c
new file mode 100644
index 0000000000000..b1d98a171cff5
--- /dev/null
+++ b/drivers/platform/chrome/cros_ec_pd_notify.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2019 Google LLC
+ *
+ * This driver serves as the receiver of cros_ec PD host events.
+ */
+
+#include <linux/acpi.h>
+#include <linux/module.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/platform_data/cros_ec_commands.h>
+#include <linux/platform_data/cros_ec_pd_notify.h>
+#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_device.h>
+
+#define DRV_NAME "cros-ec-pd-notify"
+#define ACPI_DRV_NAME "GOOG0003"
+
+static BLOCKING_NOTIFIER_HEAD(cros_ec_pd_notifier_list);
+
+/**
+ * cros_ec_pd_register_notify - Register a notifier callback for PD events.
+ * @nb: Notifier block pointer to register
+ *
+ * On ACPI platforms this corresponds to host events on the ECPD
+ * "GOOG0003" ACPI device. On non-ACPI platforms this will filter mkbp events
+ * for USB PD events.
+ *
+ * Return: 0 on success or negative error code.
+ */
+int cros_ec_pd_register_notify(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(
+			&cros_ec_pd_notifier_list, nb);
+}
+EXPORT_SYMBOL_GPL(cros_ec_pd_register_notify);
+
+
+/**
+ * cros_ec_pd_unregister_notify - Unregister notifier callback for PD events.
+ * @nb: Notifier block pointer to unregister
+ *
+ * Unregister a notifier callback that was previously registered with
+ * cros_ec_pd_register_notify().
+ */
+void cros_ec_pd_unregister_notify(struct notifier_block *nb)
+{
+	blocking_notifier_chain_unregister(&cros_ec_pd_notifier_list, nb);
+}
+EXPORT_SYMBOL_GPL(cros_ec_pd_unregister_notify);
+
+#ifdef CONFIG_ACPI
+
+static int cros_ec_pd_notify_add_acpi(struct acpi_device *adev)
+{
+	return 0;
+}
+
+static void cros_ec_pd_notify_acpi(struct acpi_device *adev, u32 event)
+{
+	blocking_notifier_call_chain(&cros_ec_pd_notifier_list, event, NULL);
+}
+
+static const struct acpi_device_id cros_ec_pd_acpi_device_ids[] = {
+	{ ACPI_DRV_NAME, 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, cros_ec_pd_acpi_device_ids);
+
+static struct acpi_driver cros_ec_pd_notify_driver = {
+	.name = DRV_NAME,
+	.class = DRV_NAME,
+	.ids = cros_ec_pd_acpi_device_ids,
+	.ops = {
+		.add = cros_ec_pd_notify_add_acpi,
+		.notify = cros_ec_pd_notify_acpi,
+	},
+};
+module_acpi_driver(cros_ec_pd_notify_driver);
+
+#else /* CONFIG_ACPI */
+
+static int cros_ec_pd_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)
+		return NOTIFY_BAD;
+
+	if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
+		blocking_notifier_call_chain(&cros_ec_pd_notifier_list,
+				host_event, NULL);
+		return NOTIFY_OK;
+	}
+	return NOTIFY_DONE;
+}
+
+static int cros_ec_pd_notify_probe_plat(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cros_ec_dev *ecdev = 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_pd_notify_plat;
+	dev_set_drvdata(dev, nb);
+
+	ret = blocking_notifier_chain_register(&ecdev->ec_dev->event_notifier,
+						nb);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register notifier\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cros_ec_pd_notify_remove_plat(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cros_ec_dev *ecdev = dev_get_drvdata(dev->parent);
+	struct notifier_block *nb =
+		(struct notifier_block *)dev_get_drvdata(dev);
+
+	blocking_notifier_chain_unregister(&ecdev->ec_dev->event_notifier,
+			nb);
+
+	return 0;
+}
+
+static struct platform_driver cros_ec_pd_notify_driver = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.probe = cros_ec_pd_notify_probe_plat,
+	.remove = cros_ec_pd_notify_remove_plat,
+};
+module_platform_driver(cros_ec_pd_notify_driver);
+
+#endif /* CONFIG_ACPI */
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ChromeOS power delivery notifier device");
+MODULE_AUTHOR("Jon Flatley <jflat@chromium.org>");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/platform_data/cros_ec_pd_notify.h b/include/linux/platform_data/cros_ec_pd_notify.h
new file mode 100644
index 0000000000000..907be5a130d60
--- /dev/null
+++ b/include/linux/platform_data/cros_ec_pd_notify.h
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ChromeOS EC Power Delivery Notifier Driver
+ *
+ * Copyright 2019 Google LLC
+ */
+
+#ifndef __LINUX_PLATFORM_DATA_CROS_EC_PD_NOTIFY_H
+#define __LINUX_PLATFORM_DATA_CROS_EC_PD_NOTIFY_H
+
+#include <linux/notifier.h>
+
+int cros_ec_pd_register_notify(struct notifier_block *nb);
+
+void cros_ec_pd_unregister_notify(struct notifier_block *nb);
+
+#endif  /* __LINUX_PLATFORM_DATA_CROS_EC_PD_NOTIFY_H */
-- 
2.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] platform: chrome: Add cros-ec-pd-notify driver
  2019-12-18 23:07 [PATCH v2] platform: chrome: Add cros-ec-pd-notify driver Prashant Malani
@ 2019-12-19  9:44 ` Enric Balletbo i Serra
  2019-12-19 20:10   ` Prashant Malani
  2019-12-22  1:27   ` kbuild test robot
  1 sibling, 1 reply; 5+ messages in thread
From: Enric Balletbo i Serra @ 2019-12-19  9:44 UTC (permalink / raw)
  To: Prashant Malani, groeck, bleung, lee.jones; +Cc: linux-kernel, Jon Flatley

Hello Prashant and Jon,

Thank you for the patch.

On 19/12/19 0:07, Prashant Malani wrote:
> From: Jon Flatley <jflat@chromium.org>
> 
> 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.
> 
> 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.
> 
> On non-ACPI platforms, the driver gets instantiated for ECs which
> support the EC_FEATURE_USB_PD feature bit, and on such platforms, the
> notification events will get delivered using the MKBP event handling
> mechanism.
> 
> Signed-off-by: Jon Flatley <jflat@chromium.org>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
> 
> Changes in v2 (pmalani@chromium.org):
> - Removed dependency on DT entry; instead, we will instantiate the
>   driver on detecting EC_FEATURE_USB_PD for non-ACPI platforms.
> - Modified the cros-ec-pd-notify device to be an mfd_cell under
>   usbpdcharger for non-ACPI platforms. Altered the platform_probe() call
>   to derive the cros EC structs appropriately.
> - Replaced "usbpd_notify" with "pd_notify" in functions and structures.
> - Addressed comments from upstream maintainer.
> 
>  drivers/mfd/cros_ec_dev.c                     |   3 +

Please use a different patch for this. That way Lee can pick this patch
independently and I can pick the chrome/platform bits.


>  drivers/platform/chrome/Kconfig               |   9 ++
>  drivers/platform/chrome/Makefile              |   1 +
>  drivers/platform/chrome/cros_ec_pd_notify.c   | 151 ++++++++++++++++++
>  .../linux/platform_data/cros_ec_pd_notify.h   |  17 ++
>  5 files changed, 181 insertions(+)
>  create mode 100644 drivers/platform/chrome/cros_ec_pd_notify.c
>  create mode 100644 include/linux/platform_data/cros_ec_pd_notify.h
> 
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index c4b977a5dd966..cc6a6788cd5c2 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -85,6 +85,9 @@ static const struct mfd_cell cros_ec_sensorhub_cells[] = {
>  static const struct mfd_cell cros_usbpd_charger_cells[] = {
>  	{ .name = "cros-usbpd-charger", },
>  	{ .name = "cros-usbpd-logger", },
> +#ifndef CONFIG_ACPI
> +	{ .name = "cros-ec-pd-notify", },

Could make sense name as cros-usbpd-notify to be coherent with the other drivers
names?

> +#endif
>  };
>  
>  static const struct cros_feature_to_cells cros_subdevices[] = {
> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> index 5f57282a28da0..972c129b7b7ba 100644
> --- a/drivers/platform/chrome/Kconfig
> +++ b/drivers/platform/chrome/Kconfig
> @@ -131,6 +131,15 @@ config CROS_EC_LPC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called cros_ec_lpcs.
>  
> +config CROS_EC_PD_NOTIFY

CROS_USBPD_NOTIFY ?

> +	tristate "ChromeOS Type-C power delivery event notifier"
> +	depends on CROS_EC
> +	help
> +	  If you say Y here, you get support for Type-C PD event notifications
> +	  from the ChromeOS EC. On ACPI platorms this driver will bind to the
> +	  GOOG0003 ACPI device, and on non-ACPI platforms this driver will get
> +	  initialized on ECs which support the feature EC_FEATURE_USB_PD.
> +

Add all this below CROS_USBPD_LOGGER

>  config CROS_EC_PROTO
>  	bool
>  	help
> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> index aacd5920d8a18..6070baa8320ca 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_SENSORHUB)		+= cros_ec_sensorhub.o
>  obj-$(CONFIG_CROS_EC_SYSFS)		+= cros_ec_sysfs.o
>  obj-$(CONFIG_CROS_USBPD_LOGGER)		+= cros_usbpd_logger.o
> +obj-$(CONFIG_CROS_EC_PD_NOTIFY)		+= cros_ec_pd_notify.o
>  
>  obj-$(CONFIG_WILCO_EC)			+= wilco_ec/
> diff --git a/drivers/platform/chrome/cros_ec_pd_notify.c b/drivers/platform/chrome/cros_ec_pd_notify.c
> new file mode 100644
> index 0000000000000..b1d98a171cff5
> --- /dev/null
> +++ b/drivers/platform/chrome/cros_ec_pd_notify.c
> @@ -0,0 +1,151 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2019 Google LLC
> + *
> + * This driver serves as the receiver of cros_ec PD host events.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/module.h>
> +#include <linux/mfd/cros_ec.h>

No need to worry about this for now, but just to let you know, this include will
be unnecessary after https://lkml.org/lkml/2019/12/3/530

It is fine for now, I'll remove the include when I pick up the patch.

> +#include <linux/platform_data/cros_ec_commands.h>
> +#include <linux/platform_data/cros_ec_pd_notify.h>
> +#include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME "cros-ec-pd-notify"
> +#define ACPI_DRV_NAME "GOOG0003"
> +
> +static BLOCKING_NOTIFIER_HEAD(cros_ec_pd_notifier_list);
> +
> +/**
> + * cros_ec_pd_register_notify - Register a notifier callback for PD events.
> + * @nb: Notifier block pointer to register
> + *
> + * On ACPI platforms this corresponds to host events on the ECPD
> + * "GOOG0003" ACPI device. On non-ACPI platforms this will filter mkbp events
> + * for USB PD events.
> + *
> + * Return: 0 on success or negative error code.
> + */
> +int cros_ec_pd_register_notify(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(
> +			&cros_ec_pd_notifier_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_pd_register_notify);
> +
> +
> +/**
> + * cros_ec_pd_unregister_notify - Unregister notifier callback for PD events.
> + * @nb: Notifier block pointer to unregister
> + *
> + * Unregister a notifier callback that was previously registered with
> + * cros_ec_pd_register_notify().
> + */
> +void cros_ec_pd_unregister_notify(struct notifier_block *nb)
> +{
> +	blocking_notifier_chain_unregister(&cros_ec_pd_notifier_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(cros_ec_pd_unregister_notify);
> +
> +#ifdef CONFIG_ACPI
> +
> +static int cros_ec_pd_notify_add_acpi(struct acpi_device *adev)
> +{
> +	return 0;
> +}
> +
> +static void cros_ec_pd_notify_acpi(struct acpi_device *adev, u32 event)
> +{
> +	blocking_notifier_call_chain(&cros_ec_pd_notifier_list, event, NULL);
> +}
> +
> +static const struct acpi_device_id cros_ec_pd_acpi_device_ids[] = {
> +	{ ACPI_DRV_NAME, 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, cros_ec_pd_acpi_device_ids);
> +
> +static struct acpi_driver cros_ec_pd_notify_driver = {
> +	.name = DRV_NAME,
> +	.class = DRV_NAME,
> +	.ids = cros_ec_pd_acpi_device_ids,
> +	.ops = {
> +		.add = cros_ec_pd_notify_add_acpi,
> +		.notify = cros_ec_pd_notify_acpi,
> +	},
> +};
> +module_acpi_driver(cros_ec_pd_notify_driver);
> +
> +#else /* CONFIG_ACPI */
> +
> +static int cros_ec_pd_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)
> +		return NOTIFY_BAD;
> +
> +	if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
> +		blocking_notifier_call_chain(&cros_ec_pd_notifier_list,
> +				host_event, NULL);
> +		return NOTIFY_OK;
> +	}
> +	return NOTIFY_DONE;
> +}
> +
> +static int cros_ec_pd_notify_probe_plat(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_dev *ecdev = 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_pd_notify_plat;
> +	dev_set_drvdata(dev, nb);
> +
> +	ret = blocking_notifier_chain_register(&ecdev->ec_dev->event_notifier,
> +						nb);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register notifier\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cros_ec_pd_notify_remove_plat(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cros_ec_dev *ecdev = dev_get_drvdata(dev->parent);
> +	struct notifier_block *nb =
> +		(struct notifier_block *)dev_get_drvdata(dev);
> +
> +	blocking_notifier_chain_unregister(&ecdev->ec_dev->event_notifier,
> +			nb);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver cros_ec_pd_notify_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +	},
> +	.probe = cros_ec_pd_notify_probe_plat,
> +	.remove = cros_ec_pd_notify_remove_plat,
> +};
> +module_platform_driver(cros_ec_pd_notify_driver);
> +
> +#endif /* CONFIG_ACPI */
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("ChromeOS power delivery notifier device");
> +MODULE_AUTHOR("Jon Flatley <jflat@chromium.org>");
> +MODULE_ALIAS("platform:" DRV_NAME);
> diff --git a/include/linux/platform_data/cros_ec_pd_notify.h b/include/linux/platform_data/cros_ec_pd_notify.h
> new file mode 100644
> index 0000000000000..907be5a130d60
> --- /dev/null
> +++ b/include/linux/platform_data/cros_ec_pd_notify.h

cros_usbpd_notify.h?

> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ChromeOS EC Power Delivery Notifier Driver
> + *
> + * Copyright 2019 Google LLC
> + */
> +
> +#ifndef __LINUX_PLATFORM_DATA_CROS_EC_PD_NOTIFY_H
> +#define __LINUX_PLATFORM_DATA_CROS_EC_PD_NOTIFY_H
> +
> +#include <linux/notifier.h>
> +
> +int cros_ec_pd_register_notify(struct notifier_block *nb);
> +
> +void cros_ec_pd_unregister_notify(struct notifier_block *nb);
> +
> +#endif  /* __LINUX_PLATFORM_DATA_CROS_EC_PD_NOTIFY_H */
> 

The driver looks good to me, the only real complain is split the patch in two,
one for the MFD subsystem and another one for chrome/platform. Also, my
preference is rename cros_ec_pd_notify to cros_usbpd_notify to be coherent with
previous related drivera, but is not a must.

Thanks,
 Enric





^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] platform: chrome: Add cros-ec-pd-notify driver
  2019-12-19  9:44 ` Enric Balletbo i Serra
@ 2019-12-19 20:10   ` Prashant Malani
  0 siblings, 0 replies; 5+ messages in thread
From: Prashant Malani @ 2019-12-19 20:10 UTC (permalink / raw)
  To: Enric Balletbo i Serra
  Cc: Guenter Roeck, Benson Leung, Lee Jones,
	Linux Kernel Mailing List, Jon Flatley

Thanks for your review Enric. I've noted the addressed comments inline
(my apologies if I've trimmed the quoted text too aggressively).

On Thu, Dec 19, 2019 at 1:44 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hello Prashant and Jon,
>
> Thank you for the patch.
>
> On 19/12/19 0:07, Prashant Malani wrote:
> > From: Jon Flatley <jflat@chromium.org>
> >
> > 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.
> >
> > +++ b/drivers/mfd/cros_ec_dev.c
> > @@ -85,6 +85,9 @@ static const struct mfd_cell cros_ec_sensorhub_cells[] = {
> >  static const struct mfd_cell cros_usbpd_charger_cells[] = {
> >       { .name = "cros-usbpd-charger", },
> >       { .name = "cros-usbpd-logger", },
> > +#ifndef CONFIG_ACPI
> > +     { .name = "cros-ec-pd-notify", },
>
> Could make sense name as cros-usbpd-notify to be coherent with the other drivers
> names?
Done.
>
> > +#endif
> >  };
> >
> >  static const struct cros_feature_to_cells cros_subdevices[] = {
> > diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> > index 5f57282a28da0..972c129b7b7ba 100644
> > --- a/drivers/platform/chrome/Kconfig
> > +++ b/drivers/platform/chrome/Kconfig
> > @@ -131,6 +131,15 @@ config CROS_EC_LPC
> >         To compile this driver as a module, choose M here: the
> >         module will be called cros_ec_lpcs.
> >
> > +config CROS_EC_PD_NOTIFY
>
> CROS_USBPD_NOTIFY ?
Done
>
> > +     tristate "ChromeOS Type-C power delivery event notifier"
> > +     depends on CROS_EC
> > +     help
> > +       If you say Y here, you get support for Type-C PD event notifications
> > +       from the ChromeOS EC. On ACPI platorms this driver will bind to the
> > +       GOOG0003 ACPI device, and on non-ACPI platforms this driver will get
> > +       initialized on ECs which support the feature EC_FEATURE_USB_PD.
> > +
>
> Add all this below CROS_USBPD_LOGGER
Done
>
> >  config CROS_EC_PROTO
> >       bool
> >       help
> > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> > index aacd5920d8a18..6070baa8320ca 100644
> > --- a/drivers/platform/chrome/Makefile
> > +++ b/drivers/platform/chrome/Makefile

> > +
> > +#include <linux/acpi.h>
> > +#include <linux/module.h>
> > +#include <linux/mfd/cros_ec.h>
>
> No need to worry about this for now, but just to let you know, this include will
> be unnecessary after https://lkml.org/lkml/2019/12/3/530
>
> It is fine for now, I'll remove the include when I pick up the patch.
>
Ack.
> > diff --git a/include/linux/platform_data/cros_ec_pd_notify.h b/include/linux/platform_data/cros_ec_pd_notify.h
> > new file mode 100644
> > index 0000000000000..907be5a130d60
> > --- /dev/null
> > +++ b/include/linux/platform_data/cros_ec_pd_notify.h
>
> cros_usbpd_notify.h?
Done.
>
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * ChromeOS EC Power Delivery Notifier Driver
> > + *
> > + * Copyright 2019 Google LLC
> > + */
> > +
> > +#ifndef __LINUX_PLATFORM_DATA_CROS_EC_PD_NOTIFY_H
> > +#define __LINUX_PLATFORM_DATA_CROS_EC_PD_NOTIFY_H
> > +
> > +#include <linux/notifier.h>
> > +
> > +int cros_ec_pd_register_notify(struct notifier_block *nb);
> > +
> > +void cros_ec_pd_unregister_notify(struct notifier_block *nb);
> > +
> > +#endif  /* __LINUX_PLATFORM_DATA_CROS_EC_PD_NOTIFY_H */
> >
>
> The driver looks good to me, the only real complain is split the patch in two,
> one for the MFD subsystem and another one for chrome/platform.
Done. WIll split into two patches.

>Also, my
> preference is rename cros_ec_pd_notify to cros_usbpd_notify to be coherent with
> previous related drivera, but is not a must.
>
> Thanks,
>  Enric
>
>
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] platform: chrome: Add cros-ec-pd-notify driver
  2019-12-18 23:07 [PATCH v2] platform: chrome: Add cros-ec-pd-notify driver Prashant Malani
@ 2019-12-22  1:27   ` kbuild test robot
  2019-12-22  1:27   ` kbuild test robot
  1 sibling, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2019-12-22  1:27 UTC (permalink / raw)
  To: Prashant Malani
  Cc: kbuild-all, enric.balletbo, groeck, bleung, lee.jones,
	linux-kernel, Jon Flatley, Prashant Malani

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

Hi Prashant,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on chrome-platform-linux/for-next]
[cannot apply to ljones-mfd/for-mfd-next linus/master v5.5-rc2 next-20191220]
[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/Prashant-Malani/platform-chrome-Add-cros-ec-pd-notify-driver/20191222-065317
base:   https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

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

All errors (new ones prefixed by >>):

>> drivers/platform/chrome/cros_ec_pd_notify.c:10:10: fatal error: linux/mfd/cros_ec.h: No such file or directory
    #include <linux/mfd/cros_ec.h>
             ^~~~~~~~~~~~~~~~~~~~~
   compilation terminated.

vim +10 drivers/platform/chrome/cros_ec_pd_notify.c

  > 10	#include <linux/mfd/cros_ec.h>
    11	#include <linux/platform_data/cros_ec_commands.h>
    12	#include <linux/platform_data/cros_ec_pd_notify.h>
    13	#include <linux/platform_data/cros_ec_proto.h>
    14	#include <linux/platform_device.h>
    15	

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 70427 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] platform: chrome: Add cros-ec-pd-notify driver
@ 2019-12-22  1:27   ` kbuild test robot
  0 siblings, 0 replies; 5+ messages in thread
From: kbuild test robot @ 2019-12-22  1:27 UTC (permalink / raw)
  To: kbuild-all

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

Hi Prashant,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on chrome-platform-linux/for-next]
[cannot apply to ljones-mfd/for-mfd-next linus/master v5.5-rc2 next-20191220]
[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/Prashant-Malani/platform-chrome-Add-cros-ec-pd-notify-driver/20191222-065317
base:   https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

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

All errors (new ones prefixed by >>):

>> drivers/platform/chrome/cros_ec_pd_notify.c:10:10: fatal error: linux/mfd/cros_ec.h: No such file or directory
    #include <linux/mfd/cros_ec.h>
             ^~~~~~~~~~~~~~~~~~~~~
   compilation terminated.

vim +10 drivers/platform/chrome/cros_ec_pd_notify.c

  > 10	#include <linux/mfd/cros_ec.h>
    11	#include <linux/platform_data/cros_ec_commands.h>
    12	#include <linux/platform_data/cros_ec_pd_notify.h>
    13	#include <linux/platform_data/cros_ec_proto.h>
    14	#include <linux/platform_device.h>
    15	

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

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 70427 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-12-22  1:28 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-18 23:07 [PATCH v2] platform: chrome: Add cros-ec-pd-notify driver Prashant Malani
2019-12-19  9:44 ` Enric Balletbo i Serra
2019-12-19 20:10   ` Prashant Malani
2019-12-22  1:27 ` kbuild test robot
2019-12-22  1:27   ` kbuild test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.