| Message ID | 20200117002820.56872-1-pmalani@chromium.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [v7,1/3] platform: chrome: Add cros-usbpd-notify driver | expand |
Hi Prashant, Sorry, I am still having issues with this patch. It doesn't work as expected on my Samsung Chromebook Plus because the below code tries to register twice the driver and then hangs for some reason. [ 9.381368] Error: Driver 'cros-usbpd-notify' is already registered, aborting... Also I have the problem that I don't have any x86 device using the cros_usbpd_charger so I can't really test for x86. Let me propose some changes and let's see if it works for you. On 17/1/20 1:28, 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 platforms that don't have the ACPI device defined, the driver gets > instantiated for ECs which support the EC_FEATURE_USB_PD feature bit, > and the notification events will get delivered using the MKBP event > handling mechanism. > > Co-Developed-by: Prashant Malani <pmalani@chromium.org> > Reviewed-by: Gwendal Grignou <gwendal@chromium.org> > Reviewed-by: Benson Leung <bleung@chromium.org> > Signed-off-by: Jon Flatley <jflat@chromium.org> > Signed-off-by: Prashant Malani <pmalani@chromium.org> > --- > > Changes in v7(pmalani@chromium.org): > - Removed use of module_platform_driver() and module_acpi_driver() since > that was causing redefinition compilation errors on arm64 defconfig. > Instead, explicitly defined the init and exit routines and > register/unregister the platform and ACPI drivers there. > - Alphabetize #include header. > > Changes in v6(pmalani@chromium.org): > - Fix build error from typo in cros_usbpd_notify_acpi_device_ids > variable name. > > Changes in v5(pmalani@chromium.org): > - Split the driver into platform and ACPI variants, each enclosed by > CONFIG_OF and CONFIG_ACPI #ifdefs respectively. > - Updated the copyright year to 2020. > - Reworded the commit message and Kconfig description to incorporate > the modified driver structure. > > Changes in v4(pmalani@chromium.org): > - No code changes, but added new version so that versioning is > consistent with the next patch in the series. > > Changes in v3 (pmalani@chromium.org): > - Renamed driver and files from "cros_ec_pd_notify" to > "cros_usbpd_notify" to be more consistent with other naming. > - Moved the change to include cros-usbpd-notify in the charger MFD > into a separate follow-on patch. > > 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/platform/chrome/Kconfig | 10 + > drivers/platform/chrome/Makefile | 1 + > drivers/platform/chrome/cros_usbpd_notify.c | 180 ++++++++++++++++++ > .../linux/platform_data/cros_usbpd_notify.h | 17 ++ > 4 files changed, 208 insertions(+) > create mode 100644 drivers/platform/chrome/cros_usbpd_notify.c > create mode 100644 include/linux/platform_data/cros_usbpd_notify.h > > diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig > index 5f57282a28da0..89df6c991089d 100644 > --- a/drivers/platform/chrome/Kconfig > +++ b/drivers/platform/chrome/Kconfig > @@ -226,6 +226,16 @@ config CROS_USBPD_LOGGER > To compile this driver as a module, choose M here: the > module will be called cros_usbpd_logger. > > +config 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 platforms which don't have this device it Please use a max length of 80 characters. > + will get initialized on ECs which support the feature > + EC_FEATURE_USB_PD. > + Please add: To compile this driver as a module, choose M here: the module will be called cros_usbpd_notify. So checkpatch --strict is more happy. > source "drivers/platform/chrome/wilco_ec/Kconfig" > > endif # CHROMEOS_PLATFORMS > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile > index aacd5920d8a18..f6465f8ef0b5e 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_USBPD_NOTIFY) += cros_usbpd_notify.o > > obj-$(CONFIG_WILCO_EC) += wilco_ec/ > diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c > new file mode 100644 > index 0000000000000..4705164e38bf4 > --- /dev/null > +++ b/drivers/platform/chrome/cros_usbpd_notify.c > @@ -0,0 +1,180 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright 2020 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> I think this include is not needed. > +#include <linux/platform_data/cros_ec_proto.h> > +#include <linux/platform_data/cros_usbpd_notify.h> > +#include <linux/platform_device.h> > + > +#define DRV_NAME "cros-usbpd-notify" > +#define ACPI_DRV_NAME "GOOG0003" > + > +static BLOCKING_NOTIFIER_HEAD(cros_usbpd_notifier_list); > + > +/** > + * cros_usbpd_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_usbpd_register_notify(struct notifier_block *nb) > +{ > + return blocking_notifier_chain_register( > + &cros_usbpd_notifier_list, nb); > +} > +EXPORT_SYMBOL_GPL(cros_usbpd_register_notify); > + > + Please don't use multiple blank lines > +/** > + * cros_usbpd_unregister_notify - Unregister notifier callback for PD events. > + * @nb: Notifier block pointer to unregister > + * > + * Unregister a notifier callback that was previously registered with > + * cros_usbpd_register_notify(). > + */ > +void cros_usbpd_unregister_notify(struct notifier_block *nb) > +{ > + blocking_notifier_chain_unregister(&cros_usbpd_notifier_list, nb); > +} > +EXPORT_SYMBOL_GPL(cros_usbpd_unregister_notify); > + > +#ifdef CONFIG_ACPI > + > +static int cros_usbpd_notify_add_acpi(struct acpi_device *adev) > +{ > + return 0; > +} > + > +static void cros_usbpd_notify_acpi(struct acpi_device *adev, u32 event) > +{ > + blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL); > +} > + > +static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = { > + { ACPI_DRV_NAME, 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, cros_usbpd_notify_acpi_device_ids); > + > +static struct acpi_driver cros_usbpd_notify_acpi_driver = { > + .name = DRV_NAME, > + .class = DRV_NAME, > + .ids = cros_usbpd_notify_acpi_device_ids, > + .ops = { > + .add = cros_usbpd_notify_add_acpi, > + .notify = cros_usbpd_notify_acpi, > + }, > +}; > + > +#endif /* CONFIG_ACPI */ > + > +#ifdef CONFIG_OF > + I propose to remove this ... > +static int cros_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) > + return NOTIFY_BAD; > + > + if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) { > + blocking_notifier_call_chain(&cros_usbpd_notifier_list, > + host_event, NULL); > + return NOTIFY_OK; > + } > + return NOTIFY_DONE; > +} > + > +static int cros_usbpd_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; > + What about this? + /* + * We only need to register the notifier if we are really using + * device-tree, otherwise, for ACPI case it will register an + * acpi_driver and use the .notify callback. + */ + if (IS_ENABLED(CONFIG_OF) && !dev->of_node) + return 0; + > + nb = devm_kzalloc(dev, sizeof(*nb), GFP_KERNEL); > + if (!nb) > + return -ENOMEM; > + > + nb->notifier_call = cros_usbpd_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_usbpd_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); > + Only unregister for the device-tree case + if (IS_ENABLED(CONFIG_OF) && dev->of_node) > + blocking_notifier_chain_unregister(&ecdev->ec_dev->event_notifier, > + nb); > + > + return 0; > +} > + > +static struct platform_driver cros_usbpd_notify_plat_driver = { > + .driver = { > + .name = DRV_NAME, > + }, > + .probe = cros_usbpd_notify_probe_plat, > + .remove = cros_usbpd_notify_remove_plat, > +}; > + > +#endif /* CONFIG_OF */ > + > +static int __init cros_usbpd_notify_init(void) > +{ > + int ret = 0; > +#ifdef CONFIG_OF > + ret = platform_driver_register(&cros_usbpd_notify_plat_driver); > + if (ret != 0) > + pr_err("cros-usbpd-notify platform driver register failed.\n"); > +#endif To simplify the ifdery, let's register the platform driver always, it'll be noop in ACPI case. int ret; ret = platform_driver_register(&cros_usbpd_notify_driver); if (ret < 0) return ret; > +#ifdef CONFIG_ACPI > + ret = acpi_bus_register_driver(&cros_usbpd_notify_acpi_driver); > + if (ret != 0) > + pr_err("cros-usbpd-notify ACPI driver register failed.\n"); > +#endif > + return ret; And register the ACPI driver if CONFIG_ACPI is enabled, note that the error is ignored because that call will fail on device-tree based devices with CONFIG_ACPI enabled. +#ifdef CONFIG_ACPI + acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver) +#endif > +} > + > +static void __exit cros_usbpd_notify_exit(void) > +{ > +#ifdef CONFIG_OF > + platform_driver_unregister(&cros_usbpd_notify_plat_driver); > +#endif > +#ifdef CONFIG_ACPI > + acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver); > +#endif And for the remove, only unregister the acpi driver if CONFIG_ACPI is enabled. +#ifdef CONFIG_ACPI + acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver); +#endif + platform_driver_unregister(&cros_usbpd_notify_driver); > +} > + > +module_init(cros_usbpd_notify_init); > +module_exit(cros_usbpd_notify_exit); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("ChromeOS power delivery notifier device"); > +MODULE_AUTHOR("Jon Flatley <jflat@chromium.org>"); > +MODULE_ALIAS("platform:" DRV_NAME); Double check that the code builds with different CONFIG options. I didn't check the ifery involved. CONFIG_ACPI && CONFIG_OF CONFIG_ACPI && !CONFIG_OF !CONFIG_ACPI && CONFIG_OF And we need to _make sure_ that the platform driver is _only_ instantiated via cros_ec_dev when we are really under a device-tree based device, so the check probably should be something like this: diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c index 39e611695053..c43026754718 100644 --- a/drivers/mfd/cros_ec_dev.c +++ b/drivers/mfd/cros_ec_dev.c @@ -211,7 +211,7 @@ static int ec_device_probe(struct platform_device *pdev) * explicitly added on platforms that don't have the PD notifier ACPI * device entry defined. */ - if (IS_ENABLED(CONFIG_OF)) { + if (IS_ENABLED(CONFIG_OF) && &pdev->dev.of_node) { if (cros_ec_check_features(ec, EC_FEATURE_USB_PD)) { retval = mfd_add_hotplug_devices(ec->dev, cros_usbpd_notify_cells, > diff --git a/include/linux/platform_data/cros_usbpd_notify.h b/include/linux/platform_data/cros_usbpd_notify.h > new file mode 100644 > index 0000000000000..4f2791722b6d3 > --- /dev/null > +++ b/include/linux/platform_data/cros_usbpd_notify.h > @@ -0,0 +1,17 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * ChromeOS EC Power Delivery Notifier Driver > + * > + * Copyright 2020 Google LLC > + */ > + > +#ifndef __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H > +#define __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H > + > +#include <linux/notifier.h> > + > +int cros_usbpd_register_notify(struct notifier_block *nb); > + > +void cros_usbpd_unregister_notify(struct notifier_block *nb); > + > +#endif /* __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H */ > Cheers, Enric
Hi Enric, Thanks for the recommendations; Please see some notes inline: On Tue, Jan 21, 2020 at 4:10 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote: > > Hi Prashant, > > Sorry, I am still having issues with this patch. It doesn't work as expected on > my Samsung Chromebook Plus because the below code tries to register twice the > driver and then hangs for some reason. > > [ 9.381368] Error: Driver 'cros-usbpd-notify' is already registered, > aborting... Hmm, is this because platform_driver_register and acpi_bus_driver_register both try to register a device with the same name? If so, we could simply rename the drivers (cros-usbpd-notify-platform and cros-usbpd-notify-acpi) and then we don't have to make any other modifications? > > Also I have the problem that I don't have any x86 device using the > cros_usbpd_charger so I can't really test for x86. > > Let me propose some changes and let's see if it works for you. > > On 17/1/20 1:28, Prashant Malani wrote: > > From: Jon Flatley <jflat@chromium.org> > > @@ -226,6 +226,16 @@ config CROS_USBPD_LOGGER > > To compile this driver as a module, choose M here: the > > module will be called cros_usbpd_logger. > > > > +config 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 platforms which don't have this device it > > Please use a max length of 80 characters. My text editor says that it is 80 chars, but I will verify it again. > > > + will get initialized on ECs which support the feature > > + EC_FEATURE_USB_PD. > > + > > Please add: > > To compile this driver as a module, choose M here: the > module will be called cros_usbpd_notify. > > So checkpatch --strict is more happy. Done > > > source "drivers/platform/chrome/wilco_ec/Kconfig" > > > > endif # CHROMEOS_PLATFORMS > > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile > > index aacd5920d8a18..f6465f8ef0b5e 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_USBPD_NOTIFY) += cros_usbpd_notify.o > > > > obj-$(CONFIG_WILCO_EC) += wilco_ec/ > > diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c > > new file mode 100644 > > index 0000000000000..4705164e38bf4 > > --- /dev/null > > +++ b/drivers/platform/chrome/cros_usbpd_notify.c > > @@ -0,0 +1,180 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright 2020 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> > > I think this include is not needed. Done > > > +#include <linux/platform_data/cros_ec_proto.h> > > +#include <linux/platform_data/cros_usbpd_notify.h> > > +#include <linux/platform_device.h> > > + > > +#define DRV_NAME "cros-usbpd-notify" > > +#define ACPI_DRV_NAME "GOOG0003" > > + > > +static BLOCKING_NOTIFIER_HEAD(cros_usbpd_notifier_list); > > + > > +/** > > + * cros_usbpd_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_usbpd_register_notify(struct notifier_block *nb) > > +{ > > + return blocking_notifier_chain_register( > > + &cros_usbpd_notifier_list, nb); > > +} > > +EXPORT_SYMBOL_GPL(cros_usbpd_register_notify); > > + > > + > > Please don't use multiple blank lines Sorry, done. > > > +/** > > + * cros_usbpd_unregister_notify - Unregister notifier callback for PD events. > > + * @nb: Notifier block pointer to unregister > > + * > > + * Unregister a notifier callback that was previously registered with > > + * cros_usbpd_register_notify(). > > + */ > > +void cros_usbpd_unregister_notify(struct notifier_block *nb) > > +{ > > + blocking_notifier_chain_unregister(&cros_usbpd_notifier_list, nb); > > +} > > +EXPORT_SYMBOL_GPL(cros_usbpd_unregister_notify); > > + > > +#ifdef CONFIG_ACPI > > + > > +static int cros_usbpd_notify_add_acpi(struct acpi_device *adev) > > +{ > > + return 0; > > +} > > + > > +static void cros_usbpd_notify_acpi(struct acpi_device *adev, u32 event) > > +{ > > + blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL); > > +} > > + > > +static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = { > > + { ACPI_DRV_NAME, 0 }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(acpi, cros_usbpd_notify_acpi_device_ids); > > + > > +static struct acpi_driver cros_usbpd_notify_acpi_driver = { > > + .name = DRV_NAME, > > + .class = DRV_NAME, > > + .ids = cros_usbpd_notify_acpi_device_ids, > > + .ops = { > > + .add = cros_usbpd_notify_add_acpi, > > + .notify = cros_usbpd_notify_acpi, > > + }, > > +}; > > + > > +#endif /* CONFIG_ACPI */ > > + > > +#ifdef CONFIG_OF > > + > > I propose to remove this ... > > > +static int cros_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) > > + return NOTIFY_BAD; > > + > > + if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) { > > + blocking_notifier_call_chain(&cros_usbpd_notifier_list, > > + host_event, NULL); > > + return NOTIFY_OK; > > + } > > + return NOTIFY_DONE; > > +} > > + > > +static int cros_usbpd_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; > > + > > What about this? > > + /* > + * We only need to register the notifier if we are really using > + * device-tree, otherwise, for ACPI case it will register an > + * acpi_driver and use the .notify callback. > + */ > + if (IS_ENABLED(CONFIG_OF) && !dev->of_node) > + return 0; > + If we add your proposed check in cros_ec_dev.c, do we need this check here? It seems like both check the same thing, i.e only go ahead and initialize the module and add the device if "IS_ENABLED(CONFIG_OF) && dev->of_node" is true. Also, kindly see my note further down regarding of_node ("dev->of_node" evaluates to false even for the DT case on the ARM builds I am testing with) > > > + nb = devm_kzalloc(dev, sizeof(*nb), GFP_KERNEL); > > + if (!nb) > > + return -ENOMEM; > > + > > + nb->notifier_call = cros_usbpd_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_usbpd_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); > > + > > Only unregister for the device-tree case > > + if (IS_ENABLED(CONFIG_OF) && dev->of_node) Same as above; is this required if we check for of_node in cros_ec_dev? > > > + blocking_notifier_chain_unregister(&ecdev->ec_dev->event_notifier, > > + nb); > > + > > + return 0; > > +} > > + > > +static struct platform_driver cros_usbpd_notify_plat_driver = { > > + .driver = { > > + .name = DRV_NAME, > > + }, > > + .probe = cros_usbpd_notify_probe_plat, > > + .remove = cros_usbpd_notify_remove_plat, > > +}; > > + > > +#endif /* CONFIG_OF */ > > + > > +static int __init cros_usbpd_notify_init(void) > > +{ > > + int ret = 0; > > +#ifdef CONFIG_OF > > + ret = platform_driver_register(&cros_usbpd_notify_plat_driver); > > + if (ret != 0) > > + pr_err("cros-usbpd-notify platform driver register failed.\n"); > > +#endif > > > To simplify the ifdery, let's register the platform driver always, it'll be noop > in ACPI case. > > int ret; > > ret = platform_driver_register(&cros_usbpd_notify_driver); > if (ret < 0) > return ret; > > > > +#ifdef CONFIG_ACPI > > + ret = acpi_bus_register_driver(&cros_usbpd_notify_acpi_driver); > > + if (ret != 0) > > + pr_err("cros-usbpd-notify ACPI driver register failed.\n"); > > +#endif > > + return ret; We should return 0 here, correct? > > And register the ACPI driver if CONFIG_ACPI is enabled, note that the error is > ignored because that call will fail on device-tree based devices with > CONFIG_ACPI enabled. My initial thought was that this would fail for the case where we have CONFIG_ACPI && actually use an ACPI device (like newer x86 Chromebooks), because the acpi_bus_register_driver() would fail, since the platform driver was already registered. However, on testing with CONFIG_ACPI && !CONFIG_OF on a system which has the GOOG0003 device, I see the "notify" call being invoked successfully, so this works. > > +#ifdef CONFIG_ACPI > + acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver) > +#endif Just checking, the above edit is not needed here, right? Sounds like it's a duplicate of what you added in _exit(). > > > +} > > + > > +static void __exit cros_usbpd_notify_exit(void) > > +{ > > +#ifdef CONFIG_OF > > + platform_driver_unregister(&cros_usbpd_notify_plat_driver); > > +#endif > > +#ifdef CONFIG_ACPI > > + acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver); > > +#endif > > And for the remove, only unregister the acpi driver if CONFIG_ACPI is enabled. Done. > > +#ifdef CONFIG_ACPI > + acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver); > +#endif > + platform_driver_unregister(&cros_usbpd_notify_driver); > > > > +} > > + > > +module_init(cros_usbpd_notify_init); > > +module_exit(cros_usbpd_notify_exit); > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_DESCRIPTION("ChromeOS power delivery notifier device"); > > +MODULE_AUTHOR("Jon Flatley <jflat@chromium.org>"); > > +MODULE_ALIAS("platform:" DRV_NAME); > > > Double check that the code builds with different CONFIG options. I didn't check > the ifery involved. > > CONFIG_ACPI && CONFIG_OF > CONFIG_ACPI && !CONFIG_OF > !CONFIG_ACPI && CONFIG_OF > > And we need to _make sure_ that the platform driver is _only_ instantiated via > cros_ec_dev when we are really under a device-tree based device, so the check > probably should be something like this: > > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c > index 39e611695053..c43026754718 100644 > --- a/drivers/mfd/cros_ec_dev.c > +++ b/drivers/mfd/cros_ec_dev.c > @@ -211,7 +211,7 @@ static int ec_device_probe(struct platform_device *pdev) > * explicitly added on platforms that don't have the PD notifier ACPI > * device entry defined. > */ > - if (IS_ENABLED(CONFIG_OF)) { > + if (IS_ENABLED(CONFIG_OF) && &pdev->dev.of_node) { I observe that when we have CONFIG_OF && !CONFIG_ACPI , the cros_usbpd_notify_probe_plat() function doesn't get called. I think adding "pdev->dev.of_node" causes the if clause to always evaluate to false. I think the check here would be : if (IS_ENABLED(CONFIG_OF) && ec->ec_dev->dev->of_node) and then remove check for of_node from cros_usbpd_notify.c Also, should this change be split into a separate patch (because it is in the mfd driver)? So, in summary, my proposed additions to the edits you suggested are: - Remove IS_ENABLED(CONFIG_OF)... checks from cros_usbpd_notify_probe_plat() and cros_usbpd_notify_remove_plat() - Change check in cros_ec_dev to be: if (IS_ENABLED(CONFIG_OF) && ec->ec_dev->dev->of_node) WDYT? Could you kindly try this on your kevin configuration? I've tried it for the cases "CONFIG_OF && !CONFIG_ACPI" and "!CONFIG_OF && CONFIG_ACPI" but not the third one (CONFIG_OF && CONFIG_ACPI) since I don't have an environment to test with (I can confirm it builds). Thanks as always for helping iterate on this. > if (cros_ec_check_features(ec, EC_FEATURE_USB_PD)) { > retval = mfd_add_hotplug_devices(ec->dev, > cros_usbpd_notify_cells, > > > > diff --git a/include/linux/platform_data/cros_usbpd_notify.h b/include/linux/platform_data/cros_usbpd_notify.h > > new file mode 100644 > > index 0000000000000..4f2791722b6d3 > > --- /dev/null > > +++ b/include/linux/platform_data/cros_usbpd_notify.h > > @@ -0,0 +1,17 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * ChromeOS EC Power Delivery Notifier Driver > > + * > > + * Copyright 2020 Google LLC > > + */ > > + > > +#ifndef __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H > > +#define __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H > > + > > +#include <linux/notifier.h> > > + > > +int cros_usbpd_register_notify(struct notifier_block *nb); > > + > > +void cros_usbpd_unregister_notify(struct notifier_block *nb); > > + > > +#endif /* __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H */ > > > > Cheers, > Enric
Hi Prashant, On 21/1/20 22:17, Prashant Malani wrote: > Hi Enric, > > Thanks for the recommendations; Please see some notes inline: > On Tue, Jan 21, 2020 at 4:10 AM Enric Balletbo i Serra > <enric.balletbo@collabora.com> wrote: >> >> Hi Prashant, >> >> Sorry, I am still having issues with this patch. It doesn't work as expected on >> my Samsung Chromebook Plus because the below code tries to register twice the >> driver and then hangs for some reason. >> >> [ 9.381368] Error: Driver 'cros-usbpd-notify' is already registered, >> aborting... > Hmm, is this because platform_driver_register and > acpi_bus_driver_register both try to register a device with the same > name? > If so, we could simply rename the drivers (cros-usbpd-notify-platform > and cros-usbpd-notify-acpi) and then we don't have to make any other > modifications? >> >> Also I have the problem that I don't have any x86 device using the >> cros_usbpd_charger so I can't really test for x86. >> >> Let me propose some changes and let's see if it works for you. >> >> On 17/1/20 1:28, Prashant Malani wrote: >>> From: Jon Flatley <jflat@chromium.org> >>> @@ -226,6 +226,16 @@ config CROS_USBPD_LOGGER >>> To compile this driver as a module, choose M here: the >>> module will be called cros_usbpd_logger. >>> >>> +config 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 platforms which don't have this device it >> >> Please use a max length of 80 characters. > My text editor says that it is 80 chars, but I will verify it again. >> >>> + will get initialized on ECs which support the feature >>> + EC_FEATURE_USB_PD. >>> + >> >> Please add: >> >> To compile this driver as a module, choose M here: the >> module will be called cros_usbpd_notify. >> >> So checkpatch --strict is more happy. > Done >> >>> source "drivers/platform/chrome/wilco_ec/Kconfig" >>> >>> endif # CHROMEOS_PLATFORMS >>> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile >>> index aacd5920d8a18..f6465f8ef0b5e 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_USBPD_NOTIFY) += cros_usbpd_notify.o >>> >>> obj-$(CONFIG_WILCO_EC) += wilco_ec/ >>> diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c >>> new file mode 100644 >>> index 0000000000000..4705164e38bf4 >>> --- /dev/null >>> +++ b/drivers/platform/chrome/cros_usbpd_notify.c >>> @@ -0,0 +1,180 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +/* >>> + * Copyright 2020 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> >> >> I think this include is not needed. > Done >> >>> +#include <linux/platform_data/cros_ec_proto.h> >>> +#include <linux/platform_data/cros_usbpd_notify.h> >>> +#include <linux/platform_device.h> >>> + >>> +#define DRV_NAME "cros-usbpd-notify" >>> +#define ACPI_DRV_NAME "GOOG0003" >>> + >>> +static BLOCKING_NOTIFIER_HEAD(cros_usbpd_notifier_list); >>> + >>> +/** >>> + * cros_usbpd_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_usbpd_register_notify(struct notifier_block *nb) >>> +{ >>> + return blocking_notifier_chain_register( >>> + &cros_usbpd_notifier_list, nb); >>> +} >>> +EXPORT_SYMBOL_GPL(cros_usbpd_register_notify); >>> + >>> + >> >> Please don't use multiple blank lines > Sorry, done. >> >>> +/** >>> + * cros_usbpd_unregister_notify - Unregister notifier callback for PD events. >>> + * @nb: Notifier block pointer to unregister >>> + * >>> + * Unregister a notifier callback that was previously registered with >>> + * cros_usbpd_register_notify(). >>> + */ >>> +void cros_usbpd_unregister_notify(struct notifier_block *nb) >>> +{ >>> + blocking_notifier_chain_unregister(&cros_usbpd_notifier_list, nb); >>> +} >>> +EXPORT_SYMBOL_GPL(cros_usbpd_unregister_notify); >>> + >>> +#ifdef CONFIG_ACPI >>> + >>> +static int cros_usbpd_notify_add_acpi(struct acpi_device *adev) >>> +{ >>> + return 0; >>> +} >>> + >>> +static void cros_usbpd_notify_acpi(struct acpi_device *adev, u32 event) >>> +{ >>> + blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL); >>> +} >>> + >>> +static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = { >>> + { ACPI_DRV_NAME, 0 }, >>> + { } >>> +}; >>> +MODULE_DEVICE_TABLE(acpi, cros_usbpd_notify_acpi_device_ids); >>> + >>> +static struct acpi_driver cros_usbpd_notify_acpi_driver = { >>> + .name = DRV_NAME, >>> + .class = DRV_NAME, >>> + .ids = cros_usbpd_notify_acpi_device_ids, >>> + .ops = { >>> + .add = cros_usbpd_notify_add_acpi, >>> + .notify = cros_usbpd_notify_acpi, >>> + }, >>> +}; >>> + >>> +#endif /* CONFIG_ACPI */ >>> + >>> +#ifdef CONFIG_OF >>> + >> >> I propose to remove this ... >> >>> +static int cros_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) >>> + return NOTIFY_BAD; >>> + >>> + if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) { >>> + blocking_notifier_call_chain(&cros_usbpd_notifier_list, >>> + host_event, NULL); >>> + return NOTIFY_OK; >>> + } >>> + return NOTIFY_DONE; >>> +} >>> + >>> +static int cros_usbpd_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; >>> + >> >> What about this? >> >> + /* >> + * We only need to register the notifier if we are really using >> + * device-tree, otherwise, for ACPI case it will register an >> + * acpi_driver and use the .notify callback. >> + */ >> + if (IS_ENABLED(CONFIG_OF) && !dev->of_node) >> + return 0; >> + > If we add your proposed check in cros_ec_dev.c, do we need this check > here? It seems like both check the same thing, i.e only go ahead and > initialize the module and add the device if "IS_ENABLED(CONFIG_OF) && > dev->of_node" is true. I think that we still need because the probe function will be called on both case. But better to check and make sure. > Also, kindly see my note further down regarding of_node > ("dev->of_node" evaluates to false even for the DT case on the ARM > builds I am testing with) >> >>> + nb = devm_kzalloc(dev, sizeof(*nb), GFP_KERNEL); >>> + if (!nb) >>> + return -ENOMEM; >>> + >>> + nb->notifier_call = cros_usbpd_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_usbpd_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); >>> + >> >> Only unregister for the device-tree case >> >> + if (IS_ENABLED(CONFIG_OF) && dev->of_node) > Same as above; is this required if we check for of_node in cros_ec_dev? >> >>> + blocking_notifier_chain_unregister(&ecdev->ec_dev->event_notifier, >>> + nb); >>> + >>> + return 0; >>> +} >>> + >>> +static struct platform_driver cros_usbpd_notify_plat_driver = { >>> + .driver = { >>> + .name = DRV_NAME, >>> + }, >>> + .probe = cros_usbpd_notify_probe_plat, >>> + .remove = cros_usbpd_notify_remove_plat, >>> +}; >>> + >>> +#endif /* CONFIG_OF */ >>> + >>> +static int __init cros_usbpd_notify_init(void) >>> +{ >>> + int ret = 0; >>> +#ifdef CONFIG_OF >>> + ret = platform_driver_register(&cros_usbpd_notify_plat_driver); >>> + if (ret != 0) >>> + pr_err("cros-usbpd-notify platform driver register failed.\n"); >>> +#endif >> >> >> To simplify the ifdery, let's register the platform driver always, it'll be noop >> in ACPI case. >> >> int ret; >> >> ret = platform_driver_register(&cros_usbpd_notify_driver); >> if (ret < 0) >> return ret; >> >> >>> +#ifdef CONFIG_ACPI >>> + ret = acpi_bus_register_driver(&cros_usbpd_notify_acpi_driver); >>> + if (ret != 0) >>> + pr_err("cros-usbpd-notify ACPI driver register failed.\n"); >>> +#endif >>> + return ret; > We should return 0 here, correct? Yes. >> >> And register the ACPI driver if CONFIG_ACPI is enabled, note that the error is >> ignored because that call will fail on device-tree based devices with >> CONFIG_ACPI enabled. > > My initial thought was that this would fail for the case where we have > CONFIG_ACPI && actually use an ACPI device (like newer x86 > Chromebooks), because the acpi_bus_register_driver() would fail, since > the platform driver was already registered. > However, on testing with CONFIG_ACPI && !CONFIG_OF on a system which > has the GOOG0003 device, I see the "notify" call being invoked > successfully, so this works. >> >> +#ifdef CONFIG_ACPI >> + acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver) >> +#endif > Just checking, the above edit is not needed here, right? Sounds like > it's a duplicate of what you added in _exit(). I think is still needed, but just let's check >> >>> +} >>> + >>> +static void __exit cros_usbpd_notify_exit(void) >>> +{ >>> +#ifdef CONFIG_OF >>> + platform_driver_unregister(&cros_usbpd_notify_plat_driver); >>> +#endif >>> +#ifdef CONFIG_ACPI >>> + acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver); >>> +#endif >> >> And for the remove, only unregister the acpi driver if CONFIG_ACPI is enabled. > Done. >> >> +#ifdef CONFIG_ACPI >> + acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver); >> +#endif >> + platform_driver_unregister(&cros_usbpd_notify_driver); >> >> >>> +} >>> + >>> +module_init(cros_usbpd_notify_init); >>> +module_exit(cros_usbpd_notify_exit); >>> + >>> +MODULE_LICENSE("GPL"); >>> +MODULE_DESCRIPTION("ChromeOS power delivery notifier device"); >>> +MODULE_AUTHOR("Jon Flatley <jflat@chromium.org>"); >>> +MODULE_ALIAS("platform:" DRV_NAME); >> >> >> Double check that the code builds with different CONFIG options. I didn't check >> the ifery involved. >> >> CONFIG_ACPI && CONFIG_OF >> CONFIG_ACPI && !CONFIG_OF >> !CONFIG_ACPI && CONFIG_OF >> >> And we need to _make sure_ that the platform driver is _only_ instantiated via >> cros_ec_dev when we are really under a device-tree based device, so the check >> probably should be something like this: >> >> >> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c >> index 39e611695053..c43026754718 100644 >> --- a/drivers/mfd/cros_ec_dev.c >> +++ b/drivers/mfd/cros_ec_dev.c >> @@ -211,7 +211,7 @@ static int ec_device_probe(struct platform_device *pdev) >> * explicitly added on platforms that don't have the PD notifier ACPI >> * device entry defined. >> */ >> - if (IS_ENABLED(CONFIG_OF)) { >> + if (IS_ENABLED(CONFIG_OF) && &pdev->dev.of_node) { > I observe that when we have CONFIG_OF && !CONFIG_ACPI , the > cros_usbpd_notify_probe_plat() function doesn't get called. I think > adding "pdev->dev.of_node" causes the if clause to always evaluate to > false. > I think the check here would be : > if (IS_ENABLED(CONFIG_OF) && ec->ec_dev->dev->of_node) > and then remove check for of_node from cros_usbpd_notify.c > I'd double check > Also, should this change be split into a separate patch (because it is > in the mfd driver)? > Yes > So, in summary, my proposed additions to the edits you suggested are: > - Remove IS_ENABLED(CONFIG_OF)... checks from > cros_usbpd_notify_probe_plat() and cros_usbpd_notify_remove_plat() > - Change check in cros_ec_dev to be: > if (IS_ENABLED(CONFIG_OF) && ec->ec_dev->dev->of_node) > > WDYT? Could you kindly try this on your kevin configuration? I've > tried it for the cases "CONFIG_OF && !CONFIG_ACPI" and "!CONFIG_OF && > CONFIG_ACPI" but not the third one (CONFIG_OF && CONFIG_ACPI) since I > don't have an environment to test with (I can confirm it builds). > > Thanks as always for helping iterate on this. > > Let's continue talking with a new version. Could you send another one and I can check. I have now setup both systems one with ACPi and another one with OF. Thanks, Enric >> if (cros_ec_check_features(ec, EC_FEATURE_USB_PD)) { >> retval = mfd_add_hotplug_devices(ec->dev, >> cros_usbpd_notify_cells, >> >> >>> diff --git a/include/linux/platform_data/cros_usbpd_notify.h b/include/linux/platform_data/cros_usbpd_notify.h >>> new file mode 100644 >>> index 0000000000000..4f2791722b6d3 >>> --- /dev/null >>> +++ b/include/linux/platform_data/cros_usbpd_notify.h >>> @@ -0,0 +1,17 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +/* >>> + * ChromeOS EC Power Delivery Notifier Driver >>> + * >>> + * Copyright 2020 Google LLC >>> + */ >>> + >>> +#ifndef __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H >>> +#define __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H >>> + >>> +#include <linux/notifier.h> >>> + >>> +int cros_usbpd_register_notify(struct notifier_block *nb); >>> + >>> +void cros_usbpd_unregister_notify(struct notifier_block *nb); >>> + >>> +#endif /* __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H */ >>> >> >> Cheers, >> Enric
Hi Enric (removed lot of the context text to keep the message length small) > > So, in summary, my proposed additions to the edits you suggested are: > > - Remove IS_ENABLED(CONFIG_OF)... checks from > > cros_usbpd_notify_probe_plat() and cros_usbpd_notify_remove_plat() > > - Change check in cros_ec_dev to be: > > if (IS_ENABLED(CONFIG_OF) && ec->ec_dev->dev->of_node) > > > > WDYT? Could you kindly try this on your kevin configuration? I've > > tried it for the cases "CONFIG_OF && !CONFIG_ACPI" and "!CONFIG_OF && > > CONFIG_ACPI" but not the third one (CONFIG_OF && CONFIG_ACPI) since I > > don't have an environment to test with (I can confirm it builds). > > > > Thanks as always for helping iterate on this. > > > > > > Let's continue talking with a new version. Could you send another one and I can > check. I have now setup both systems one with ACPi and another one with OF. Sounds good. Will push a new version. Thanks. > > Thanks, > Enric > > > >> if (cros_ec_check_features(ec, EC_FEATURE_USB_PD)) { > >> retval = mfd_add_hotplug_devices(ec->dev, > >> cros_usbpd_notify_cells, > >> > >> > >>> diff --git a/include/linux/platform_data/cros_usbpd_notify.h b/include/linux/platform_data/cros_usbpd_notify.h > >>> new file mode 100644 > >>> index 0000000000000..4f2791722b6d3 > >>> --- /dev/null > >>> +++ b/include/linux/platform_data/cros_usbpd_notify.h > >>> @@ -0,0 +1,17 @@ > >>> +// SPDX-License-Identifier: GPL-2.0-only > >>> +/* > >>> + * ChromeOS EC Power Delivery Notifier Driver > >>> + * > >>> + * Copyright 2020 Google LLC > >>> + */ > >>> + > >>> +#ifndef __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H > >>> +#define __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H > >>> + > >>> +#include <linux/notifier.h> > >>> + > >>> +int cros_usbpd_register_notify(struct notifier_block *nb); > >>> + > >>> +void cros_usbpd_unregister_notify(struct notifier_block *nb); > >>> + > >>> +#endif /* __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H */ > >>> > >> > >> Cheers, > >> Enric
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig index 5f57282a28da0..89df6c991089d 100644 --- a/drivers/platform/chrome/Kconfig +++ b/drivers/platform/chrome/Kconfig @@ -226,6 +226,16 @@ config CROS_USBPD_LOGGER To compile this driver as a module, choose M here: the module will be called cros_usbpd_logger. +config 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 platforms which don't have this device it + will get initialized on ECs which support the feature + EC_FEATURE_USB_PD. + source "drivers/platform/chrome/wilco_ec/Kconfig" endif # CHROMEOS_PLATFORMS diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile index aacd5920d8a18..f6465f8ef0b5e 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_USBPD_NOTIFY) += cros_usbpd_notify.o obj-$(CONFIG_WILCO_EC) += wilco_ec/ diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c new file mode 100644 index 0000000000000..4705164e38bf4 --- /dev/null +++ b/drivers/platform/chrome/cros_usbpd_notify.c @@ -0,0 +1,180 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright 2020 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_proto.h> +#include <linux/platform_data/cros_usbpd_notify.h> +#include <linux/platform_device.h> + +#define DRV_NAME "cros-usbpd-notify" +#define ACPI_DRV_NAME "GOOG0003" + +static BLOCKING_NOTIFIER_HEAD(cros_usbpd_notifier_list); + +/** + * cros_usbpd_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_usbpd_register_notify(struct notifier_block *nb) +{ + return blocking_notifier_chain_register( + &cros_usbpd_notifier_list, nb); +} +EXPORT_SYMBOL_GPL(cros_usbpd_register_notify); + + +/** + * cros_usbpd_unregister_notify - Unregister notifier callback for PD events. + * @nb: Notifier block pointer to unregister + * + * Unregister a notifier callback that was previously registered with + * cros_usbpd_register_notify(). + */ +void cros_usbpd_unregister_notify(struct notifier_block *nb) +{ + blocking_notifier_chain_unregister(&cros_usbpd_notifier_list, nb); +} +EXPORT_SYMBOL_GPL(cros_usbpd_unregister_notify); + +#ifdef CONFIG_ACPI + +static int cros_usbpd_notify_add_acpi(struct acpi_device *adev) +{ + return 0; +} + +static void cros_usbpd_notify_acpi(struct acpi_device *adev, u32 event) +{ + blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL); +} + +static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = { + { ACPI_DRV_NAME, 0 }, + { } +}; +MODULE_DEVICE_TABLE(acpi, cros_usbpd_notify_acpi_device_ids); + +static struct acpi_driver cros_usbpd_notify_acpi_driver = { + .name = DRV_NAME, + .class = DRV_NAME, + .ids = cros_usbpd_notify_acpi_device_ids, + .ops = { + .add = cros_usbpd_notify_add_acpi, + .notify = cros_usbpd_notify_acpi, + }, +}; + +#endif /* CONFIG_ACPI */ + +#ifdef CONFIG_OF + +static int cros_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) + return NOTIFY_BAD; + + if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) { + blocking_notifier_call_chain(&cros_usbpd_notifier_list, + host_event, NULL); + return NOTIFY_OK; + } + return NOTIFY_DONE; +} + +static int cros_usbpd_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_usbpd_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_usbpd_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_usbpd_notify_plat_driver = { + .driver = { + .name = DRV_NAME, + }, + .probe = cros_usbpd_notify_probe_plat, + .remove = cros_usbpd_notify_remove_plat, +}; + +#endif /* CONFIG_OF */ + +static int __init cros_usbpd_notify_init(void) +{ + int ret = 0; +#ifdef CONFIG_OF + ret = platform_driver_register(&cros_usbpd_notify_plat_driver); + if (ret != 0) + pr_err("cros-usbpd-notify platform driver register failed.\n"); +#endif +#ifdef CONFIG_ACPI + ret = acpi_bus_register_driver(&cros_usbpd_notify_acpi_driver); + if (ret != 0) + pr_err("cros-usbpd-notify ACPI driver register failed.\n"); +#endif + return ret; +} + +static void __exit cros_usbpd_notify_exit(void) +{ +#ifdef CONFIG_OF + platform_driver_unregister(&cros_usbpd_notify_plat_driver); +#endif +#ifdef CONFIG_ACPI + acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver); +#endif +} + +module_init(cros_usbpd_notify_init); +module_exit(cros_usbpd_notify_exit); + +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_usbpd_notify.h b/include/linux/platform_data/cros_usbpd_notify.h new file mode 100644 index 0000000000000..4f2791722b6d3 --- /dev/null +++ b/include/linux/platform_data/cros_usbpd_notify.h @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * ChromeOS EC Power Delivery Notifier Driver + * + * Copyright 2020 Google LLC + */ + +#ifndef __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H +#define __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H + +#include <linux/notifier.h> + +int cros_usbpd_register_notify(struct notifier_block *nb); + +void cros_usbpd_unregister_notify(struct notifier_block *nb); + +#endif /* __LINUX_PLATFORM_DATA_CROS_USBPD_NOTIFY_H */