lkml.org 
[lkml]   [2020]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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);
> > > }
> > >

\
 
 \ /
  Last update: 2020-03-16 08:35    [W:0.591 / U:0.608 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site