Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191220193843.47182-1-pmalani@chromium.org> <20191220193843.47182-2-pmalani@chromium.org>
 <7eecafb2-4686-b448-2837-4181188365b1@collabora.com>
In-Reply-To: <7eecafb2-4686-b448-2837-4181188365b1@collabora.com>
From:   Prashant Malani <pmalani@chromium.org>
Date:   Mon, 23 Dec 2019 12:29:59 -0800
Message-ID: <CACeCKadFKWUNHHT-vs686Sz2-CcD0kNeUgqkJT5Q2ATji4L58w@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] mfd: cros_ec: Add cros-usbpd-notify subdevice
To:     Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc:     Guenter Roeck <groeck@chromium.org>,
        Benson Leung <bleung@chromium.org>,
        Lee Jones <lee.jones@linaro.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Enric,

On Sun, Dec 22, 2019 at 11:25 PM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Prashant,
>
> On 20/12/19 20:38, Prashant Malani wrote:
> > Add the cros-usbpd-notify driver as a subdevice on non-ACPI platforms
> > that support the EC_FEATURE_USB_PD EC feature flag.
> >
> > This driver allows other cros-ec devices to receive PD event
> > notifications from the Chrome OS Embedded Controller (EC) via a
> > notification chain.
> >
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >
> > Changes in v4:
> > - Removed #ifndef usage; instead, moved cros-usbpd-notify to a separate
> >   mfd_cell and used an IS_ENABLED() check.
> > - Changed commit title and description slightly to reflect change in
> >   code.
> >
> >  drivers/mfd/cros_ec_dev.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > index c4b977a5dd966..da198abe2b0a6 100644
> > --- a/drivers/mfd/cros_ec_dev.c
> > +++ b/drivers/mfd/cros_ec_dev.c
> > @@ -5,6 +5,7 @@
> >   * Copyright (C) 2014 Google, Inc.
> >   */
> >
> > +#include <linux/kconfig.h>
> >  #include <linux/mfd/core.h>
> >  #include <linux/mfd/cros_ec.h>
> >  #include <linux/module.h>
> > @@ -87,6 +88,10 @@ static const struct mfd_cell cros_usbpd_charger_cells[] = {
> >       { .name = "cros-usbpd-logger", },
> >  };
> >
> > +static const struct mfd_cell cros_usbpd_notify_cells[] = {
> > +     { .name = "cros-usbpd-notify", },
> > +};
> > +
> >  static const struct cros_feature_to_cells cros_subdevices[] = {
> >       {
> >               .id             = EC_FEATURE_CEC,
> > @@ -202,6 +207,22 @@ static int ec_device_probe(struct platform_device *pdev)
> >               }
> >       }
> >
> > +     /*
> > +      * The PD notifier driver cell is separate since it only needs to be
> > +      * explicitly added on non-ACPI platforms.
>
>
> Sorry to not catch this before, but a worry arose. Is non-ACPI platforms or
> non-X86 platforms or on OF platforms?
>
> ARM64 for example has the CONFIG_ACPI symbol set to yes, with the below
> condition condition will not work on Kevin for example and IIUC this is not what
> we want, I think we want IS_ENABLED(CONFIG_OF)?
Thanks for noting this. I will check with a kevin, and with the
internal build flags to verify whether there are ARM64 which have the
GOOG0003 PD notification device.
I'll update this thread with my findings.

Best,

>
> Thanks,
>  Enric
>
> > +      */
> > +     if (!IS_ENABLED(CONFIG_ACPI)) {
> > +             if (cros_ec_check_features(ec, EC_FEATURE_USB_PD)) {
> > +                     retval = mfd_add_hotplug_devices(ec->dev,
> > +                                     cros_usbpd_notify_cells,
> > +                                     ARRAY_SIZE(cros_usbpd_notify_cells));
> > +                     if (retval)
> > +                             dev_err(ec->dev,
> > +                                     "failed to add PD notify devices: %d\n",
> > +                                     retval);
> > +             }
> > +     }
> > +
> >       /*
> >        * The following subdevices cannot be detected by sending the
> >        * EC_FEATURE_GET_CMD to the Embedded Controller device.
> >