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 the charger driver can just be updated
to use this new notifer.
Signed-off-by: Jon Flatley <[email protected]>
---
drivers/power/supply/Kconfig | 2 +-
drivers/power/supply/cros_usbpd-charger.c | 45 ++++++++---------------
2 files changed, 17 insertions(+), 30 deletions(-)
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index c84a7b1caeb6..7664849d7680 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_EC_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 6cc7c3910e09..58cf51b51179 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_ec_usbpd_notify.h>
#include <linux/platform_data/cros_ec_proto.h>
#include <linux/platform_device.h>
#include <linux/power_supply.h>
@@ -524,32 +525,22 @@ 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;
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_ec_usbpd_unregister_notify(&charger->notifier);
}
static int cros_usbpd_charger_probe(struct platform_device *pd)
@@ -683,21 +674,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_ec_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.24.0.432.g9d3f5f5b63-goog
Hi Jon,
Some comments below.
Missatge de Jon Flatley <[email protected]> del dia dc., 13 de nov.
2019 a les 4:19:
>
> 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 the charger driver can just be updated
> to use this new notifer.
>
> Signed-off-by: Jon Flatley <[email protected]>
> ---
> drivers/power/supply/Kconfig | 2 +-
> drivers/power/supply/cros_usbpd-charger.c | 45 ++++++++---------------
> 2 files changed, 17 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index c84a7b1caeb6..7664849d7680 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_EC_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 6cc7c3910e09..58cf51b51179 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_ec_usbpd_notify.h>
> #include <linux/platform_data/cros_ec_proto.h>
> #include <linux/platform_device.h>
> #include <linux/power_supply.h>
> @@ -524,32 +525,22 @@ 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;
>
> 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_ec_usbpd_unregister_notify(&charger->notifier);
> }
>
Can we get rid of this function and call directly
cros_ec_usbpd_unregister_notify where the function is used?
> static int cros_usbpd_charger_probe(struct platform_device *pd)
> @@ -683,21 +674,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_ec_usbpd_register_notify(&charger->notifier);
> + if (ret < 0) {
> + dev_warn(dev, "failed to register notifier\n");
Hmm, now I am wondering if should we only warn and continue or fail.
Makes sense have this without the notifier?
> + } else {
> + ret = devm_add_action_or_reset(dev,
> + cros_usbpd_charger_unregister_notifier,
> + charger);
> + if (ret < 0)
> + goto fail;
> }
>
> return 0;
> --
> 2.24.0.432.g9d3f5f5b63-goog
>
Thanks,
Enric