Messages in this thread Patch in this message | | | From | Prashant Malani <> | Date | Tue, 21 Jan 2020 13:17:12 -0800 | Subject | Re: [PATCH v7 1/3] platform: chrome: Add cros-usbpd-notify driver |
| |
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
| |