Re: [PATCH v6 3/3] power: supply: cros-ec-usbpd-charger: Fix host events

[Prashant Malani <pmalani@chromium.org>]

On Thu, Jan 16, 2020 at 11:46 AM Benson Leung <bleung@google.com> wrote:
>
> On Tue, Jan 14, 2020 at 03:22:22PM -0800, Prashant Malani wrote:
> > From: Jon Flatley <jflat@chromium.org>
> >
> > There's a bug on ACPI platforms where host events from the ECPD ACPI
> > device never make their way to the cros-ec-usbpd-charger driver. This
> > makes it so the only time the charger driver updates its state is when
> > user space accesses its sysfs attributes.
> >
> > Now that these events have been unified into a single notifier chain on
> > both ACPI and non-ACPI platforms, update the charger driver to use this
> > new notifier.
> >
> > Signed-off-by: Jon Flatley <jflat@chromium.org>
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
>
> Only a minor nit. Otherwise,
> Reviewed-by: Benson Leung <bleung@chromium.org>
>
>
> > ---
> >
> > Changes in v6(pmalani@chromium.org):
> > - Patch first introduced into the series in v6.
> >
> >  drivers/power/supply/Kconfig              |  2 +-
> >  drivers/power/supply/cros_usbpd-charger.c | 50 ++++++++---------------
> >  2 files changed, 19 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> > index 27164a1d3c7c4..ba74ddd793c3d 100644
> > --- a/drivers/power/supply/Kconfig
> > +++ b/drivers/power/supply/Kconfig
> > @@ -659,7 +659,7 @@ config CHARGER_RT9455
> >
> >  config CHARGER_CROS_USBPD
> >       tristate "ChromeOS EC based USBPD charger"
> > -     depends on CROS_EC
> > +     depends on CROS_USBPD_NOTIFY
> >       default n
> >       help
> >         Say Y here to enable ChromeOS EC based USBPD charger
> > diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
> > index 6cc7c3910e098..7f7e051262170 100644
> > --- a/drivers/power/supply/cros_usbpd-charger.c
> > +++ b/drivers/power/supply/cros_usbpd-charger.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/mfd/cros_ec.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_data/cros_ec_commands.h>
> > +#include <linux/platform_data/cros_usbpd_notify.h>
>
> Really minor nit. Alphabetize this #include. This insertion should
> be one line below.
Done. Thanks.
>
> >  #include <linux/platform_data/cros_ec_proto.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/power_supply.h>
> > @@ -524,32 +525,21 @@ static int cros_usbpd_charger_property_is_writeable(struct power_supply *psy,
> >  }
> >
> >  static int cros_usbpd_charger_ec_event(struct notifier_block *nb,
> > -                                    unsigned long queued_during_suspend,
> > +                                    unsigned long host_event,
> >                                      void *_notify)
> >  {
> > -     struct cros_ec_device *ec_device;
> > -     struct charger_data *charger;
> > -     u32 host_event;
> > +     struct charger_data *charger = container_of(nb, struct charger_data,
> > +                                                 notifier);
> >
> > -     charger = container_of(nb, struct charger_data, notifier);
> > -     ec_device = charger->ec_device;
> > -
> > -     host_event = cros_ec_get_host_event(ec_device);
> > -     if (host_event & EC_HOST_EVENT_MASK(EC_HOST_EVENT_PD_MCU)) {
> > -             cros_usbpd_charger_power_changed(charger->ports[0]->psy);
> > -             return NOTIFY_OK;
> > -     } else {
> > -             return NOTIFY_DONE;
> > -     }
> > +     cros_usbpd_charger_power_changed(charger->ports[0]->psy);
> > +     return NOTIFY_OK;
> >  }
> >
> >  static void cros_usbpd_charger_unregister_notifier(void *data)
> >  {
> >       struct charger_data *charger = data;
> > -     struct cros_ec_device *ec_device = charger->ec_device;
> >
> > -     blocking_notifier_chain_unregister(&ec_device->event_notifier,
> > -                                        &charger->notifier);
> > +     cros_usbpd_unregister_notify(&charger->notifier);
> >  }
> >
> >  static int cros_usbpd_charger_probe(struct platform_device *pd)
> > @@ -683,21 +673,17 @@ static int cros_usbpd_charger_probe(struct platform_device *pd)
> >               goto fail;
> >       }
> >
> > -     if (ec_device->mkbp_event_supported) {
> > -             /* Get PD events from the EC */
> > -             charger->notifier.notifier_call = cros_usbpd_charger_ec_event;
> > -             ret = blocking_notifier_chain_register(
> > -                                             &ec_device->event_notifier,
> > -                                             &charger->notifier);
> > -             if (ret < 0) {
> > -                     dev_warn(dev, "failed to register notifier\n");
> > -             } else {
> > -                     ret = devm_add_action_or_reset(dev,
> > -                                     cros_usbpd_charger_unregister_notifier,
> > -                                     charger);
> > -                     if (ret < 0)
> > -                             goto fail;
> > -             }
> > +     /* Get PD events from the EC */
> > +     charger->notifier.notifier_call = cros_usbpd_charger_ec_event;
> > +     ret = cros_usbpd_register_notify(&charger->notifier);
> > +     if (ret < 0) {
> > +             dev_warn(dev, "failed to register notifier\n");
> > +     } else {
> > +             ret = devm_add_action_or_reset(dev,
> > +                             cros_usbpd_charger_unregister_notifier,
> > +                             charger);
> > +             if (ret < 0)
> > +                     goto fail;
> >       }
> >
> >       return 0;
> > --
> > 2.25.0.341.g760bfbb309-goog
> >
>
> --
> Benson Leung
> Staff Software Engineer
> Chrome OS Kernel
> Google Inc.
> bleung@google.com
> Chromium OS Project
> bleung@chromium.org