Messages in this thread | | | From | Enric Balletbo Serra <> | Date | Thu, 14 Nov 2019 22:43:42 +0100 | Subject | Re: [PATCH 1/3] platform: chrome: Add cros-ec-usbpd-notify driver |
| |
Hi Jon,
Many thanks for working on this and push it upstream. Some comments below
Missatge de Jon Flatley <jflat@chromium.org> del dia dc., 13 de nov. 2019 a les 4:13: > > ChromiumOS uses ACPI device with HID "GOOG0003" for power delivery > related events. The existing cros-usbpd-charger driver relies on these > events without ever actually receiving them on ACPI platforms. This is > because in the ChromeOS kernel trees, the GOOG0003 device is owned by an > ACPI driver that offers firmware updates to USB-C chargers. > > Let's introduce a new platform driver under cros-ec, the ChromeOS > embedded controller, that handles these PD events and dispatches them > appropriately over a notifier chain to all drivers that use them. > > Signed-off-by: Jon Flatley <jflat@chromium.org> > --- > drivers/platform/chrome/Kconfig | 9 + > drivers/platform/chrome/Makefile | 1 + > .../platform/chrome/cros_ec_usbpd_notify.c | 156 ++++++++++++++++++ > .../platform_data/cros_ec_usbpd_notify.h | 40 +++++ > 4 files changed, 206 insertions(+) > create mode 100644 drivers/platform/chrome/cros_ec_usbpd_notify.c > create mode 100644 include/linux/platform_data/cros_ec_usbpd_notify.h > > diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig > index ee5f08ea57b6..d4a55b64bc29 100644 > --- a/drivers/platform/chrome/Kconfig > +++ b/drivers/platform/chrome/Kconfig > @@ -118,6 +118,15 @@ config CROS_EC_SPI > response time cannot be guaranteed, we support ignoring > 'pre-amble' bytes before the response actually starts. > > +config CROS_EC_USBPD_NOTIFY > + tristate "ChromeOS USB-C power delivery event notifier" > + depends on CROS_EC > + help > + If you say Y here, you get support for USB-C PD event notifications > + from the ChromeOS EC. On ACPI platorms this driver will bind to the > + GOOG0003 ACPI device, and on non-ACPI platform this driver will match > + "google,cros-ec-pd-update" in device tree. > + > config CROS_EC_LPC > tristate "ChromeOS Embedded Controller (LPC)" > depends on CROS_EC && ACPI && (X86 || COMPILE_TEST) > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile > index 477ec3d1d1c9..efa355ab526f 100644 > --- a/drivers/platform/chrome/Makefile > +++ b/drivers/platform/chrome/Makefile > @@ -21,5 +21,6 @@ obj-$(CONFIG_CROS_EC_VBC) += cros_ec_vbc.o > obj-$(CONFIG_CROS_EC_DEBUGFS) += cros_ec_debugfs.o > obj-$(CONFIG_CROS_EC_SYSFS) += cros_ec_sysfs.o > obj-$(CONFIG_CROS_USBPD_LOGGER) += cros_usbpd_logger.o > +obj-$(CONFIG_CROS_EC_USBPD_NOTIFY) += cros_ec_usbpd_notify.o > > obj-$(CONFIG_WILCO_EC) += wilco_ec/ > diff --git a/drivers/platform/chrome/cros_ec_usbpd_notify.c b/drivers/platform/chrome/cros_ec_usbpd_notify.c > new file mode 100644 > index 000000000000..f654586dea2a > --- /dev/null > +++ b/drivers/platform/chrome/cros_ec_usbpd_notify.c > @@ -0,0 +1,156 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * cros_ec_usbpd - ChromeOS EC Power Delivery Driver
Please remove 'cros_ec_usbpd -'
> + * > + * Copyright (C) 2019 Google, Inc > + *
I think it should be
Copyright 2019 Google LLC
> + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + *
Remove the license boilerplate, with the SPDX identifier, is enough.
> + * This driver serves as the receiver of cros_ec PD host events. > + */ > + > +#include <linux/acpi.h> > +#include <linux/mfd/cros_ec.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_data/cros_ec_commands.h> > +#include <linux/platform_data/cros_ec_usbpd_notify.h> > +#include <linux/platform_data/cros_ec_proto.h> > +#include <linux/platform_device.h> > + > +#define DRV_NAME "cros-ec-usbpd-notify" > +#define ACPI_DRV_NAME "GOOG0003" > + > +static BLOCKING_NOTIFIER_HEAD(cros_ec_usbpd_notifier_list); > + > +int cros_ec_usbpd_register_notify(struct notifier_block *nb) > +{ > + return blocking_notifier_chain_register( > + &cros_ec_usbpd_notifier_list, nb); > +} > +EXPORT_SYMBOL_GPL(cros_ec_usbpd_register_notify); > + > +void cros_ec_usbpd_unregister_notify(struct notifier_block *nb) > +{ > + blocking_notifier_chain_unregister(&cros_ec_usbpd_notifier_list, nb); > +} > +EXPORT_SYMBOL_GPL(cros_ec_usbpd_unregister_notify); > + > +static void cros_ec_usbpd_notify(u32 event) > +{ > + blocking_notifier_call_chain(&cros_ec_usbpd_notifier_list, event, NULL); > +}
That function is not needed, just call directly blocking_notifier_call_chain.
> + > +#ifdef CONFIG_ACPI > +
I think you can create a single platform_driver ACPI and non-ACPI compatible and get rid of this ifdef
> +static int cros_ec_usbpd_add_acpi(struct acpi_device *adev) > +{ > + return 0; > +} > + Could be probably be removed
> +static int cros_ec_usbpd_remove_acpi(struct acpi_device *adev) > +{ > + return 0; > +} Could be probably be removed
> +static void cros_ec_usbpd_notify_acpi(struct acpi_device *adev, u32 event) > +{ > + cros_ec_usbpd_notify(event); > +} > + > +static const struct acpi_device_id cros_ec_usbpd_acpi_device_ids[] = { > + { ACPI_DRV_NAME, 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, cros_ec_usbpd_acpi_device_ids); > + > +static struct acpi_driver cros_ec_usbpd_driver = { > + .name = DRV_NAME, > + .class = DRV_NAME, > + .ids = cros_ec_usbpd_acpi_device_ids, > + .ops = { > + .add = cros_ec_usbpd_add_acpi, > + .remove = cros_ec_usbpd_remove_acpi, > + .notify = cros_ec_usbpd_notify_acpi, > + }, > +}; > +module_acpi_driver(cros_ec_usbpd_driver); > + > +#else /* CONFIG_ACPI */ > + > +static int cros_ec_usbpd_notify_plat(struct notifier_block *nb, > + unsigned long queued_during_suspend, void *data) > +{ > + struct cros_ec_device *ec_dev = (struct cros_ec_device *)data; > + u32 host_event = cros_ec_get_host_event(ec_dev); > + > + if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) { > + cros_ec_usbpd_notify(host_event); > + return NOTIFY_OK; > + } else {
Remove the else and just return NOTIFY_DONE
> + return NOTIFY_DONE; > + } > +
Remove the extra blank line.
> +} > + > +static int cros_ec_usbpd_probe_plat(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct cros_ec_device *ec_dev = dev_get_drvdata(dev->parent); > + struct notifier_block *nb; > + int ret; > + > + nb = devm_kzalloc(dev, sizeof(*nb), GFP_KERNEL); > + if (!nb) > + return -ENOMEM; > + > + nb->notifier_call = cros_ec_usbpd_notify_plat; > + dev_set_drvdata(dev, nb); > + ret = blocking_notifier_chain_register(&ec_dev->event_notifier, nb); > +
Remove the extra blank line
> + if (ret < 0) > + dev_warn(dev, "Failed to register notifier\n");
Only a warning? shouldn't you return an error?
> + > + return 0; > +} > + > +static int cros_ec_usbpd_remove_plat(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct cros_ec_device *ec_dev = dev_get_drvdata(dev->parent); > + struct notifier_block *nb = > + (struct notifier_block *)dev_get_drvdata(dev); > + > + blocking_notifier_chain_unregister(&ec_dev->event_notifier, nb); > + > + return 0; > +} > + > +static const struct of_device_id cros_ec_usbpd_of_match[] = { > + { .compatible = "google,cros-ec-pd-update" },
This should be documented in Documentation/devicetree/bindings
> + { } > +}; > +MODULE_DEVICE_TABLE(of, cros_ec_usbpd_of_match); > + > +static struct platform_driver cros_ec_usbpd_driver = { > + .driver = { > + .name = DRV_NAME, > + .of_match_table = of_match_ptr(cros_ec_usbpd_of_match), > + }, > + .probe = cros_ec_usbpd_probe_plat, > + .remove = cros_ec_usbpd_remove_plat, > +}; > +module_platform_driver(cros_ec_usbpd_driver); > + > +#endif /* CONFIG_ACPI */ > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("ChromeOS power delivery device"); > +MODULE_AUTHOR("Jon Flatley <jflat@chromium.org>"); > +MODULE_ALIAS("platform:" DRV_NAME); > diff --git a/include/linux/platform_data/cros_ec_usbpd_notify.h b/include/linux/platform_data/cros_ec_usbpd_notify.h > new file mode 100644 > index 000000000000..fdcea146b7c4 > --- /dev/null > +++ b/include/linux/platform_data/cros_ec_usbpd_notify.h > @@ -0,0 +1,40 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * cros_ec_usbpd - ChromeOS EC Power Delivery Driver > + *
Remove 'cros_ec_usbpd -'
> + * Copyright (C) 2019 Google, Inc > + *
Copyright 2019 Google LLC
> + * This software is licensed under the terms of the GNU General Public > + * License version 2, as published by the Free Software Foundation, and > + * may be copied, distributed, and modified under those terms. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details.
Remove the license boiler plate
> + */ > + > +#ifndef __LINUX_PLATFORM_DATA_CROS_EC_USBPD_NOTIFY_H > +#define __LINUX_PLATFORM_DATA_CROS_EC_USBPD_NOTIFY_H > + > +#include <linux/notifier.h> > + > +/** > + * cros_ec_usbpd_register_notify - register a notifier callback for USB PD > + * events. On ACPI platforms this corrisponds to to host events on the ECPD > + * "GOOG0003" ACPI device. On non-ACPI platforms this will filter mkbp events > + * for USB PD events. > + * > + * @nb: Notifier block pointer to register > + */
I don't think this is kernel-doc compliant, please check running kernel-doc -v
> +int cros_ec_usbpd_register_notify(struct notifier_block *nb); > + > +/** > + * cros_ec_usbpd_unregister_notify - unregister a notifier callback that was > + * previously registered with cros_ec_usbpd_register_notify(). > + * > + * @nb: Notifier block pointer to unregister > + */
Same here
> +void cros_ec_usbpd_unregister_notify(struct notifier_block *nb); > + > +#endif /* __LINUX_PLATFORM_DATA_CROS_EC_USBPD_NOTIFY_H */ > -- > 2.24.0.432.g9d3f5f5b63-goog >
Thanks, Enric
| |