Messages in this thread Patch in this message | | | Date | Sun, 15 Mar 2020 14:38:27 -0700 | From | Prashant Malani <> | Subject | Re: [PATCH 2/3] platform/chrome: notify: Amend ACPI driver to plat |
| |
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 :)
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); > > } > >
| |