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: <20191218230701.59166-1-pmalani@chromium.org> <d9aea1f0-1b5d-44bc-7fcc-eb0b8f9634fd@collabora.com>
In-Reply-To: <d9aea1f0-1b5d-44bc-7fcc-eb0b8f9634fd@collabora.com>
From:   Prashant Malani <pmalani@chromium.org>
Date:   Thu, 19 Dec 2019 12:10:03 -0800
Message-ID: <CACeCKacA=w+D8W4uPwX6cf+2M7N=CfENR5B0d_=vtBxdJv8jnQ@mail.gmail.com>
Subject: Re: [PATCH v2] platform: chrome: Add cros-ec-pd-notify driver
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>,
        Jon Flatley <jflat@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks for your review Enric. I've noted the addressed comments inline
(my apologies if I've trimmed the quoted text too aggressively).

On Thu, Dec 19, 2019 at 1:44 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hello Prashant and Jon,
>
> Thank you for the patch.
>
> On 19/12/19 0:07, Prashant Malani wrote:
> > From: Jon Flatley <jflat@chromium.org>
> >
> > 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.
> >
> > +++ b/drivers/mfd/cros_ec_dev.c
> > @@ -85,6 +85,9 @@ static const struct mfd_cell cros_ec_sensorhub_cells[] = {
> >  static const struct mfd_cell cros_usbpd_charger_cells[] = {
> >       { .name = "cros-usbpd-charger", },
> >       { .name = "cros-usbpd-logger", },
> > +#ifndef CONFIG_ACPI
> > +     { .name = "cros-ec-pd-notify", },
>
> Could make sense name as cros-usbpd-notify to be coherent with the other drivers
> names?
Done.
>
> > +#endif
> >  };
> >
> >  static const struct cros_feature_to_cells cros_subdevices[] = {
> > diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
> > index 5f57282a28da0..972c129b7b7ba 100644
> > --- a/drivers/platform/chrome/Kconfig
> > +++ b/drivers/platform/chrome/Kconfig
> > @@ -131,6 +131,15 @@ config CROS_EC_LPC
> >         To compile this driver as a module, choose M here: the
> >         module will be called cros_ec_lpcs.
> >
> > +config CROS_EC_PD_NOTIFY
>
> CROS_USBPD_NOTIFY ?
Done
>
> > +     tristate "ChromeOS Type-C power delivery event notifier"
> > +     depends on CROS_EC
> > +     help
> > +       If you say Y here, you get support for Type-C PD event notifications
> > +       from the ChromeOS EC. On ACPI platorms this driver will bind to the
> > +       GOOG0003 ACPI device, and on non-ACPI platforms this driver will get
> > +       initialized on ECs which support the feature EC_FEATURE_USB_PD.
> > +
>
> Add all this below CROS_USBPD_LOGGER
Done
>
> >  config CROS_EC_PROTO
> >       bool
> >       help
> > diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile
> > index aacd5920d8a18..6070baa8320ca 100644
> > --- a/drivers/platform/chrome/Makefile
> > +++ b/drivers/platform/chrome/Makefile

> > +
> > +#include <linux/acpi.h>
> > +#include <linux/module.h>
> > +#include <linux/mfd/cros_ec.h>
>
> No need to worry about this for now, but just to let you know, this include will
> be unnecessary after https://lkml.org/lkml/2019/12/3/530
>
> It is fine for now, I'll remove the include when I pick up the patch.
>
Ack.
> > diff --git a/include/linux/platform_data/cros_ec_pd_notify.h b/include/linux/platform_data/cros_ec_pd_notify.h
> > new file mode 100644
> > index 0000000000000..907be5a130d60
> > --- /dev/null
> > +++ b/include/linux/platform_data/cros_ec_pd_notify.h
>
> cros_usbpd_notify.h?
Done.
>
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * ChromeOS EC Power Delivery Notifier Driver
> > + *
> > + * Copyright 2019 Google LLC
> > + */
> > +
> > +#ifndef __LINUX_PLATFORM_DATA_CROS_EC_PD_NOTIFY_H
> > +#define __LINUX_PLATFORM_DATA_CROS_EC_PD_NOTIFY_H
> > +
> > +#include <linux/notifier.h>
> > +
> > +int cros_ec_pd_register_notify(struct notifier_block *nb);
> > +
> > +void cros_ec_pd_unregister_notify(struct notifier_block *nb);
> > +
> > +#endif  /* __LINUX_PLATFORM_DATA_CROS_EC_PD_NOTIFY_H */
> >
>
> The driver looks good to me, the only real complain is split the patch in two,
> one for the MFD subsystem and another one for chrome/platform.
Done. WIll split into two patches.

>Also, my
> preference is rename cros_ec_pd_notify to cros_usbpd_notify to be coherent with
> previous related drivera, but is not a must.
>
> Thanks,
>  Enric
>
>
>
>