Messages in this thread | | | From | Prashant Malani <> | Date | Mon, 16 Mar 2020 00:34:53 -0700 | Subject | Re: [PATCH 2/3] platform/chrome: notify: Amend ACPI driver to plat |
| |
Hi Enric,
On Sun, Mar 15, 2020 at 2:38 PM Prashant Malani <pmalani@chromium.org> wrote: > > Hi Enric, > > Thanks a lot for reviewing the patch, kindly see inline: > > On Fri, Mar 13, 2020 at 01:42:26PM +0100, Enric Balletbo i Serra wrote: > > Hi Prashant, > > > > On 12/3/20 11:08, Prashant Malani wrote: > > > Convert the ACPI driver into the equivalent platform driver, with the > > > same ACPI match table as before. This allows the device driver to access > > > the parent platform EC device and its cros_ec_device struct, which will > > > be required to communicate with the EC to pull PD Host event information > > > from it. > > > > > > Also change the ACPI driver name to "cros-usbpd-notify-acpi" so that > > > there is no confusion between it and the "regular" platform driver on > > > platforms that have both CONFIG_ACPI and CONFIG_OF enabled. > > > > > > Signed-off-by: Prashant Malani <pmalani@chromium.org> > > > --- > > > drivers/platform/chrome/cros_usbpd_notify.c | 82 ++++++++++++++++++--- > > > 1 file changed, 70 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/platform/chrome/cros_usbpd_notify.c b/drivers/platform/chrome/cros_usbpd_notify.c > > > index edcb346024b07..d2dbf7017e29c 100644 > > > --- a/drivers/platform/chrome/cros_usbpd_notify.c > > > +++ b/drivers/platform/chrome/cros_usbpd_notify.c > > > @@ -12,6 +12,7 @@ > > > #include <linux/platform_device.h> > > > > > > #define DRV_NAME "cros-usbpd-notify" > > > +#define DRV_NAME_PLAT_ACPI "cros-usbpd-notify-acpi" > > > #define ACPI_DRV_NAME "GOOG0003" > > > > > > static BLOCKING_NOTIFIER_HEAD(cros_usbpd_notifier_list); > > > @@ -54,14 +55,72 @@ EXPORT_SYMBOL_GPL(cros_usbpd_unregister_notify); > > > > > > #ifdef CONFIG_ACPI > > > > > > -static int cros_usbpd_notify_add_acpi(struct acpi_device *adev) > > > +static void cros_usbpd_notify_acpi(acpi_handle device, u32 event, void *data) > > > { > > > + blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL); > > > +} > > > + > > > +static int cros_usbpd_notify_probe_acpi(struct platform_device *pdev) > > > +{ > > > + struct cros_usbpd_notify_data *pdnotify; > > > + struct device *dev = &pdev->dev; > > > + struct acpi_device *adev; > > > + struct cros_ec_device *ec_dev; > > > + acpi_status status; > > > + > > > + adev = ACPI_COMPANION(dev); > > > + if (!adev) { > > > > I still missing some bits of the ACPI devices but is this possible? > > > > The ACPI probe only will be called if there is a match so an ACPI device, I guess. > > > Ack. Will remove this check. I was following cros_ec_lpc.c but that is a > common driver. > > > > + dev_err(dev, "No ACPI device found.\n"); > > > + return -ENODEV; > > > + } > > > + > > > + pdnotify = devm_kzalloc(dev, sizeof(*pdnotify), GFP_KERNEL); > > > + if (!pdnotify) > > > + return -ENOMEM; > > > + > > > + /* Get the EC device pointer needed to talk to the EC. */ > > > + ec_dev = dev_get_drvdata(dev->parent); > > > + if (!ec_dev) { > > > + /* > > > + * We continue even for older devices which don't have the > > > + * correct device heirarchy, namely, GOOG0003 is a child > > > + * of GOOG0004. > > > + */ > > > + dev_warn(dev, "Couldn't get Chrome EC device pointer.\n"); > > > > I'm not sure this is correctly handled, see below ... > > > > > > > + } > > > + > > > + pdnotify->dev = dev; > > > + pdnotify->ec = ec_dev; > > > > If !ec_dev you'll assign a NULL pointer to pdnotify->ec. On the cases that > > GOOG0003 is not a child of GOOG0004 I suspect you will get a NULL dereference > > later in some other part of the code? > > > > I think there is a comment about this in the Patch 3/3 review, so will > also address it there. Basically, cros_usbpd_notify_plat() will not have > a NULL ec_dev, because the platform_probe() only happens for a cros MFD, > which will be a child of the parent EC device always. > > > > + > > > + status = acpi_install_notify_handler(adev->handle, > > > + ACPI_ALL_NOTIFY, > > > + cros_usbpd_notify_acpi, > > > + pdnotify); > > > + if (ACPI_FAILURE(status)) { > > > + dev_warn(dev, "Failed to register notify handler %08x\n", > > > + status); > > > + return -EINVAL; > > > + } > > > + > > > + dev_info(dev, "Chrome EC PD notify device registered.\n"); > > > + > > > > This is only noise to the kernel log, remove it. > > Done. > > > > > return 0; > > > } > > > > > > -static void cros_usbpd_notify_acpi(struct acpi_device *adev, u32 event) > > > +static int cros_usbpd_notify_remove_acpi(struct platform_device *pdev) > > > { > > > - blocking_notifier_call_chain(&cros_usbpd_notifier_list, event, NULL); > > > + struct device *dev = &pdev->dev; > > > + struct acpi_device *adev = ACPI_COMPANION(dev); > > > + > > > + if (!adev) { > > > + dev_err(dev, "No ACPI device found.\n"); > > > > Is this possible? > > > Ack. For ACPI probe not possible. Will remove it. > > > + return -ENODEV; > > > + } > > > + > > > + acpi_remove_notify_handler(adev->handle, ACPI_ALL_NOTIFY, > > > + cros_usbpd_notify_acpi); > > > + > > > + return 0; > > > } > > > > > > static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = { > > > @@ -70,14 +129,13 @@ static const struct acpi_device_id cros_usbpd_notify_acpi_device_ids[] = { > > > }; > > > 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, > > > +static struct platform_driver cros_usbpd_notify_acpi_driver = { > > > > Nice, so it is converted to a platform_driver, now. This makes me think again if > > we could just use a single platform_driver and register the acpi notifier in the > > ACPI match case and use the non-acpi notifier on the OF case. > > > I'd like that as well. But, I'm hesitant to make the change now, since I > don't have a platform which has CONFIG_OF and CONFIG_ACPI on which to > test the common platform driver with (which is what you use IIRC). > > Would something as follows work (pseudo code to follow): > > static int cros_usbpd_notify_probe_plat(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct acpi_device *adev = ACPI_COMPANION(dev); > > /* "Non-ACPI case" > if (dev->parent->of_node) { > /* Do non-ACPI case probe work here */ > > } else if (adev) { > /* Do ACPI case probe work here */ > } else { > return -EINVAL; > } > } > > and similarly for remove ? > > If so, I can change Patch 2/2 to do this :) Actually, I tried the above out and this causes a compilation failure for the ARM case (i.e CONFIG_OF && !CONFIG_ACPI), because struct acpi_device isn't defined for that case. It looks like we might have to stick with two separate drivers, even if both are platform. Open to suggestions though.
> > Best regards, > > -Prashant > > > > + .driver = { > > > + .name = DRV_NAME_PLAT_ACPI, > > > + .acpi_match_table = cros_usbpd_notify_acpi_device_ids, > > > }, > > > + .probe = cros_usbpd_notify_probe_acpi, > > > + .remove = cros_usbpd_notify_remove_acpi, > > > }; > > > > > > #endif /* CONFIG_ACPI */ > > > @@ -159,7 +217,7 @@ static int __init cros_usbpd_notify_init(void) > > > return ret; > > > > > > #ifdef CONFIG_ACPI > > > - acpi_bus_register_driver(&cros_usbpd_notify_acpi_driver); > > > + platform_driver_register(&cros_usbpd_notify_acpi_driver); > > > #endif > > > return 0; > > > } > > > @@ -167,7 +225,7 @@ static int __init cros_usbpd_notify_init(void) > > > static void __exit cros_usbpd_notify_exit(void) > > > { > > > #ifdef CONFIG_ACPI > > > - acpi_bus_unregister_driver(&cros_usbpd_notify_acpi_driver); > > > + platform_driver_unregister(&cros_usbpd_notify_acpi_driver); > > > #endif > > > platform_driver_unregister(&cros_usbpd_notify_plat_driver); > > > } > > >
| |