[PATCH 0/6] ACPI: EC: Updates related to initialization

[PATCH 0/6] ACPI: EC: Updates related to initialization

[Rafael J. Wysocki]

Hi All,

The purpose of this series of update of the ACPI EC driver is to make its
initialization more straightforward.

They fix a couple of issues, clean up some things, remove redundant code etc.

Please refer to the changelogs of individual patches for details.

For easier access, the series is available in the git branch at

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-ec-work

on top of 5.6-rc3.

Thanks!

[PATCH 1/6] ACPI: EC: Avoid printing confusing messages in acpi_ec_setup()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

It doesn't really make sense to pass ec->handle of the ECDT EC to
acpi_handle_info(), because it is set to ACPI_ROOT_OBJECT in
acpi_ec_ecdt_probe(), so rework acpi_ec_setup() to avoid using
acpi_handle_info() for printing messages.

First, notice that the "Used as first EC" message is not really
useful, because it is immediately followed by a more meaningful one
from either acpi_ec_ecdt_probe() or acpi_ec_dsdt_probe() (the latter
also includes the EC object path), so drop it altogether.

Second, use pr_info() for printing the EC configuration information.

While at it, make the code in question avoid printing invalid GPE or
IRQ numbers and make it print the GPE/IRQ information only when the
driver is ready to handle events.

Fixes: 72c77b7ea9ce ("ACPI / EC: Cleanup first_ec/boot_ec code")
Fixes: 406857f773b0 ("ACPI: EC: add support for hardware-reduced systems")
Cc: 5.5+ <stable@vger.kernel.org> # 5.5+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index d1f1cf5d4bf0..2dc7cf2aeb21 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1584,14 +1584,19 @@ static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device,
 		return ret;
 
 	/* First EC capable of handling transactions */
-	if (!first_ec) {
+	if (!first_ec)
 		first_ec = ec;
-		acpi_handle_info(first_ec->handle, "Used as first EC\n");
+
+	pr_info("EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n", ec->command_addr,
+		ec->data_addr);
+
+	if (test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
+		if (ec->gpe >= 0)
+			pr_info("GPE=0x%x\n", ec->gpe);
+		else
+			pr_info("IRQ=%d\n", ec->irq);
 	}
 
-	acpi_handle_info(ec->handle,
-			 "GPE=0x%x, IRQ=%d, EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n",
-			 ec->gpe, ec->irq, ec->command_addr, ec->data_addr);
 	return ret;
 }
 
-- 
2.16.4

[PATCH 2/6] ACPI: EC: Avoid passing redundant argument to functions

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

After commit 406857f773b0 ("ACPI: EC: add support for hardware-reduced
systems") the handle_events argument passed to ec_install_handlers()
and acpi_ec_setup() is redundant, because it is always 'false' when
the device argument passed to them in NULL and it is always 'true'
otherwise, so the device argument can be tested against NULL instead
of testing the handle_events one.

Accordingly, modify ec_install_handlers() and acpi_ec_setup() to take
two arguments and reduce the number of checks in the former.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 2dc7cf2aeb21..3153e7684053 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1476,8 +1476,7 @@ static int install_gpio_irq_event_handler(struct acpi_ec *ec,
  *       handler is not installed, which means "not able to handle
  *       transactions".
  */
-static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
-			       bool handle_events)
+static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 {
 	acpi_status status;
 
@@ -1507,7 +1506,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
 		set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
 
-	if (!handle_events)
+	if (!device)
 		return 0;
 
 	if (!test_bit(EC_FLAGS_QUERY_METHODS_INSTALLED, &ec->flags)) {
@@ -1520,13 +1519,10 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
 	if (!test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
 		if (ec->gpe >= 0) {
 			install_gpe_event_handler(ec);
-		} else if (device) {
+		} else {
 			int ret = install_gpio_irq_event_handler(ec, device);
-
 			if (ret)
 				return ret;
-		} else { /* No GPE and no GpioInt? */
-			return -ENODEV;
 		}
 	}
 	/* EC is fully operational, allow queries */
@@ -1574,12 +1570,11 @@ static void ec_remove_handlers(struct acpi_ec *ec)
 	}
 }
 
-static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device,
-			 bool handle_events)
+static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device)
 {
 	int ret;
 
-	ret = ec_install_handlers(ec, device, handle_events);
+	ret = ec_install_handlers(ec, device);
 	if (ret)
 		return ret;
 
@@ -1660,7 +1655,7 @@ static int acpi_ec_add(struct acpi_device *device)
 		}
 	}
 
-	ret = acpi_ec_setup(ec, device, true);
+	ret = acpi_ec_setup(ec, device);
 	if (ret)
 		goto err_query;
 
@@ -1780,7 +1775,7 @@ void __init acpi_ec_dsdt_probe(void)
 	 * At this point, the GPE is not fully initialized, so do not to
 	 * handle the events.
 	 */
-	ret = acpi_ec_setup(ec, NULL, false);
+	ret = acpi_ec_setup(ec, NULL);
 	if (ret) {
 		acpi_ec_free(ec);
 		return;
@@ -1967,7 +1962,7 @@ void __init acpi_ec_ecdt_probe(void)
 	 * At this point, the namespace is not initialized, so do not find
 	 * the namespace objects, or handle the events.
 	 */
-	ret = acpi_ec_setup(ec, NULL, false);
+	ret = acpi_ec_setup(ec, NULL);
 	if (ret) {
 		acpi_ec_free(ec);
 		return;
-- 
2.16.4

[PATCH 3/6] ACPI: EC: Drop AE_NOT_FOUND special case from ec_install_handlers()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

If the status value returned by acpi_install_address_space_handler()
in ec_install_handlers() is AE_NOT_FOUND, it is treated in a special
way, apparently because it might mean a _REG method evaluation
failure (at least that is the case according to the comment in
there), but acpi_install_address_space_handler() does not take
_REG evaluation errors into account at all, so the AE_NOT_FOUND
special handling is confusing at best.

For this reason, change ec_install_handlers() to stop the EC and
return -ENODEV on all acpi_install_address_space_handler() errors.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 3153e7684053..6f501d552e6e 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1489,19 +1489,8 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 							    &acpi_ec_space_handler,
 							    NULL, ec);
 		if (ACPI_FAILURE(status)) {
-			if (status == AE_NOT_FOUND) {
-				/*
-				 * Maybe OS fails in evaluating the _REG
-				 * object. The AE_NOT_FOUND error will be
-				 * ignored and OS * continue to initialize
-				 * EC.
-				 */
-				pr_err("Fail in evaluating the _REG object"
-					" of EC device. Broken bios is suspected.\n");
-			} else {
-				acpi_ec_stop(ec, false);
-				return -ENODEV;
-			}
+			acpi_ec_stop(ec, false);
+			return -ENODEV;
 		}
 		set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
-- 
2.16.4

[PATCH 4/6] ACPI: EC: Unify handling of event handler installation failures

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

Commit 406857f773b0 ("ACPI: EC: add support for hardware-reduced
systems") made ec_install_handlers() return an error on failures
to configure a GPIO IRQ for the EC, but that is inconsistent with
the handling of the GPE event handler installation failures even
though it is exactly the same issue and the driver can respond to
it in the same way in both cases (the EC can be actively polled
for events through its registers if the event handler installation
fails).

Moreover, it requires acpi_ec_add() to take that special case into
account and disagrees with the ec_install_handlers() header comment.

For this reason, consolidate the handling of event handler
installation failures in ec_install_handlers() so that they do not
prevent the EC from being used in any case.  On top of that, reduce
code duplication between install_gpe_event_handler() and
install_gpio_irq_event_handler() by moving some code from there
into ec_install_handlers() and simplify the error code path in
acpi_ec_add().

While at it, turn the ec_install_handlers() header comment into
a proper kerneldoc one and add some general control flow information
to it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c | 96 +++++++++++++++++++++++++++----------------------------
 1 file changed, 48 insertions(+), 48 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 6f501d552e6e..3f6127e919ab 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1427,54 +1427,53 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 	return AE_CTRL_TERMINATE;
 }
 
-static void install_gpe_event_handler(struct acpi_ec *ec)
-{
-	acpi_status status =
-		acpi_install_gpe_raw_handler(NULL, ec->gpe,
-					     ACPI_GPE_EDGE_TRIGGERED,
-					     &acpi_ec_gpe_handler,
-					     ec);
-	if (ACPI_SUCCESS(status)) {
-		/* This is not fatal as we can poll EC events */
-		set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
-		acpi_ec_leave_noirq(ec);
-		if (test_bit(EC_FLAGS_STARTED, &ec->flags) &&
-		    ec->reference_count >= 1)
-			acpi_ec_enable_gpe(ec, true);
-	}
+static bool install_gpe_event_handler(struct acpi_ec *ec)
+{
+	acpi_status status;
+
+	status = acpi_install_gpe_raw_handler(NULL, ec->gpe,
+					      ACPI_GPE_EDGE_TRIGGERED,
+					      &acpi_ec_gpe_handler, ec);
+	if (ACPI_FAILURE(status))
+		return false;
+
+	if (test_bit(EC_FLAGS_STARTED, &ec->flags) && ec->reference_count >= 1)
+		acpi_ec_enable_gpe(ec, true);
+
+	return true;
 }
 
-/* ACPI reduced hardware platforms use a GpioInt specified in _CRS. */
-static int install_gpio_irq_event_handler(struct acpi_ec *ec,
-					  struct acpi_device *device)
+static bool install_gpio_irq_event_handler(struct acpi_ec *ec,
+					   struct acpi_device *device)
 {
-	int irq = acpi_dev_gpio_irq_get(device, 0);
-	int ret;
+	int irq, ret;
 
+	/* ACPI reduced hardware platforms use a GpioInt specified in _CRS. */
+	irq = acpi_dev_gpio_irq_get(device, 0);
 	if (irq < 0)
-		return irq;
-
-	ret = request_irq(irq, acpi_ec_irq_handler, IRQF_SHARED,
-			  "ACPI EC", ec);
+		return false;
 
-	/*
-	 * Unlike the GPE case, we treat errors here as fatal, we'll only
-	 * implement GPIO polling if we find a case that needs it.
-	 */
+	ret = request_irq(irq, acpi_ec_irq_handler, IRQF_SHARED, "ACPI EC", ec);
 	if (ret < 0)
-		return ret;
+		return false;
 
 	ec->irq = irq;
-	set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
-	acpi_ec_leave_noirq(ec);
 
-	return 0;
+	return true;
 }
 
-/*
- * Note: This function returns an error code only when the address space
- *       handler is not installed, which means "not able to handle
- *       transactions".
+/**
+ * ec_install_handlers - Install service callbacks and register query methods.
+ * @ec: Target EC.
+ * @device: ACPI device object corresponding to @ec.
+ *
+ * Install a handler for the EC address space type unless it has been installed
+ * already.  If @device is not NULL, also look for EC query methods in the
+ * namespace and register them, and install an event (either GPE or GPIO IRQ)
+ * handler for the EC, if possible.
+ *
+ * Return -ENODEV if the address space handler cannot be installed, which means
+ * "unable to handle transactions", or 0 (success) otherwise.
  */
 static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 {
@@ -1506,13 +1505,17 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 		set_bit(EC_FLAGS_QUERY_METHODS_INSTALLED, &ec->flags);
 	}
 	if (!test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
-		if (ec->gpe >= 0) {
-			install_gpe_event_handler(ec);
-		} else {
-			int ret = install_gpio_irq_event_handler(ec, device);
-			if (ret)
-				return ret;
+		bool ready = ec->gpe >= 0 ?
+				install_gpe_event_handler(ec) :
+				install_gpio_irq_event_handler(ec, device);
+		if (ready) {
+			set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
+			acpi_ec_leave_noirq(ec);
 		}
+		/*
+		 * Failures to install an event handler are not fatal, because
+		 * the EC can be polled for events.
+		 */
 	}
 	/* EC is fully operational, allow queries */
 	acpi_ec_enable_event(ec);
@@ -1625,7 +1628,7 @@ static int acpi_ec_add(struct acpi_device *device)
 		status = ec_parse_device(device->handle, 0, ec, NULL);
 		if (status != AE_CTRL_TERMINATE) {
 			ret = -EINVAL;
-			goto err_alloc;
+			goto err;
 		}
 
 		if (boot_ec && ec->command_addr == boot_ec->command_addr &&
@@ -1646,7 +1649,7 @@ static int acpi_ec_add(struct acpi_device *device)
 
 	ret = acpi_ec_setup(ec, device);
 	if (ret)
-		goto err_query;
+		goto err;
 
 	if (ec == boot_ec)
 		acpi_handle_info(boot_ec->handle,
@@ -1667,10 +1670,7 @@ static int acpi_ec_add(struct acpi_device *device)
 	acpi_handle_debug(ec->handle, "enumerated.\n");
 	return 0;
 
-err_query:
-	if (ec != boot_ec)
-		acpi_ec_remove_query_handlers(ec, true, 0);
-err_alloc:
+err:
 	if (ec != boot_ec)
 		acpi_ec_free(ec);
 	return ret;
-- 
2.16.4

[PATCH 5/6] ACPI: EC: Simplify acpi_ec_add()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

First, notice that if the device ID in acpi_ec_add() is equal to
ACPI_ECDT_HID, boot_ec_is_ecdt must be set, because this means
that the device object passed to acpi_ec_add() comes from
acpi_ec_ecdt_start() which fails if boot_ec_is_ecdt is unset.
Accordingly, boot_ec_is_ecdt need not be set again in that case,
so drop that redundant update of it from the code.

Next, ec->handle must be a valid ACPI handle right before
returning 0 from acpi_ec_add(), because it either is the handle
of the device object passed to that function, or it is the boot EC
handle coming from acpi_ec_ecdt_start() which fails if it cannot
find a valid handle for the boot EC.  Moreover, the object with
that handle is regarded as a valid representation of the EC in all
cases, so there is no reason to avoid the _DEP list update walk if
that handle is the boot EC handle.  Accordingly, drop the dep_update
local variable from acpi_ec_add() and call acpi_walk_dep_device_list()
for ec->handle unconditionally before returning 0 from it.

Finally, the ec local variable in acpi_ec_add() need not be
initialized to NULL and the status local variable declaration
can be moved to the block in which it is used, so change the code
in accordance with these observations.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 3f6127e919ab..4ed0c6155d3f 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1608,19 +1608,18 @@ static bool acpi_ec_ecdt_get_handle(acpi_handle *phandle)
 
 static int acpi_ec_add(struct acpi_device *device)
 {
-	struct acpi_ec *ec = NULL;
-	bool dep_update = true;
-	acpi_status status;
+	struct acpi_ec *ec;
 	int ret;
 
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 
 	if (!strcmp(acpi_device_hid(device), ACPI_ECDT_HID)) {
-		boot_ec_is_ecdt = true;
+		/* Fast path: this device corresponds to the boot EC. */
 		ec = boot_ec;
-		dep_update = false;
 	} else {
+		acpi_status status;
+
 		ec = acpi_ec_alloc();
 		if (!ec)
 			return -ENOMEM;
@@ -1663,16 +1662,16 @@ static int acpi_ec_add(struct acpi_device *device)
 	ret = !!request_region(ec->command_addr, 1, "EC cmd");
 	WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr);
 
-	if (dep_update) {
-		/* Reprobe devices depending on the EC */
-		acpi_walk_dep_device_list(ec->handle);
-	}
+	/* Reprobe devices depending on the EC */
+	acpi_walk_dep_device_list(ec->handle);
+
 	acpi_handle_debug(ec->handle, "enumerated.\n");
 	return 0;
 
 err:
 	if (ec != boot_ec)
 		acpi_ec_free(ec);
+
 	return ret;
 }
 
-- 
2.16.4

[PATCH 6/6] ACPI: EC: Use fast path in acpi_ec_add() for DSDT boot EC

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

If the boot EC comes from the DSDT, its ACPI handle is equal to the
handle of a device object with the PNP0C09 device ID.  If that
device object is passed to acpi_ec_add(), it is not necessary to
allocate a new EC structure for it and parse it, because that has
been done already, so change the function to use the fast path in
that case.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/ec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 4ed0c6155d3f..920a08c01ab6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1614,7 +1614,8 @@ static int acpi_ec_add(struct acpi_device *device)
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 
-	if (!strcmp(acpi_device_hid(device), ACPI_ECDT_HID)) {
+	if ((boot_ec && boot_ec->handle == device->handle) ||
+	    !strcmp(acpi_device_hid(device), ACPI_ECDT_HID)) {
 		/* Fast path: this device corresponds to the boot EC. */
 		ec = boot_ec;
 	} else {
-- 
2.16.4

Re: [PATCH 0/6] ACPI: EC: Updates related to initialization

[Daniel Drake]

On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> The purpose of this series of update of the ACPI EC driver is to make its
> initialization more straightforward.
>
> They fix a couple of issues, clean up some things, remove redundant code etc.
>
> Please refer to the changelogs of individual patches for details.
>
> For easier access, the series is available in the git branch at
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
>  acpi-ec-work
>
> on top of 5.6-rc3.

Jian-Hong, can you please test this on Asus UX434DA?
Check if the screen brightness hotkeys are still working after these changes.

Thanks
Daniel

Re: [PATCH 0/6] ACPI: EC: Updates related to initialization

[Jian-Hong Pan]

Daniel Drake <drake@endlessm.com> 於 2020年2月28日 週五 下午5:43寫道：
>
> On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > The purpose of this series of update of the ACPI EC driver is to make its
> > initialization more straightforward.
> >
> > They fix a couple of issues, clean up some things, remove redundant code etc.
> >
> > Please refer to the changelogs of individual patches for details.
> >
> > For easier access, the series is available in the git branch at
> >
> >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> >  acpi-ec-work
> >
> > on top of 5.6-rc3.
>
> Jian-Hong, can you please test this on Asus UX434DA?
> Check if the screen brightness hotkeys are still working after these changes.

Hi Rafael,

Thanks for your patches, but we found an issue:
The laptops like ASUS UX434DA's screen brightness hotkeys work before
this patch series.  However, the hotkeys are failed with the patch
"ACPI: EC: Unify handling of event handler installation failures".

Jian-Hong Pan

Re: [PATCH 0/6] ACPI: EC: Updates related to initialization

[Rafael J. Wysocki]

On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
>
> Daniel Drake <drake@endlessm.com> 於 2020年2月28日 週五 下午5:43寫道：
> >
> > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > The purpose of this series of update of the ACPI EC driver is to make its
> > > initialization more straightforward.
> > >
> > > They fix a couple of issues, clean up some things, remove redundant code etc.
> > >
> > > Please refer to the changelogs of individual patches for details.
> > >
> > > For easier access, the series is available in the git branch at
> > >
> > >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > >  acpi-ec-work
> > >
> > > on top of 5.6-rc3.
> >
> > Jian-Hong, can you please test this on Asus UX434DA?
> > Check if the screen brightness hotkeys are still working after these changes.
>
> Hi Rafael,
>
> Thanks for your patches, but we found an issue:
> The laptops like ASUS UX434DA's screen brightness hotkeys work before
> this patch series.  However, the hotkeys are failed with the patch
> "ACPI: EC: Unify handling of event handler installation failures".

Thanks for checking!

Re: [PATCH 0/6] ACPI: EC: Updates related to initialization

[Rafael J. Wysocki]

On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
>
> Daniel Drake <drake@endlessm.com> 於 2020年2月28日 週五 下午5:43寫道：
> >
> > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > The purpose of this series of update of the ACPI EC driver is to make its
> > > initialization more straightforward.
> > >
> > > They fix a couple of issues, clean up some things, remove redundant code etc.
> > >
> > > Please refer to the changelogs of individual patches for details.
> > >
> > > For easier access, the series is available in the git branch at
> > >
> > >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > >  acpi-ec-work
> > >
> > > on top of 5.6-rc3.
> >
> > Jian-Hong, can you please test this on Asus UX434DA?
> > Check if the screen brightness hotkeys are still working after these changes.
>
> Hi Rafael,
>
> Thanks for your patches, but we found an issue:
> The laptops like ASUS UX434DA's screen brightness hotkeys work before
> this patch series.  However, the hotkeys are failed with the patch
> "ACPI: EC: Unify handling of event handler installation failures".

So I have modified the series to avoid the change that can possibly break this.

Can you please pull the new series from

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-ec-work

(same branch) and retest?

I'll post the updated patches later this week, but it would be good to
try them on now if possible.

Thanks!

Re: [PATCH 0/6] ACPI: EC: Updates related to initialization

[Rafael J. Wysocki]

On Mon, Mar 2, 2020 at 11:38 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> >
> > Daniel Drake <drake@endlessm.com> 於 2020年2月28日 週五 下午5:43寫道：
> > >
> > > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > The purpose of this series of update of the ACPI EC driver is to make its
> > > > initialization more straightforward.
> > > >
> > > > They fix a couple of issues, clean up some things, remove redundant code etc.
> > > >
> > > > Please refer to the changelogs of individual patches for details.
> > > >
> > > > For easier access, the series is available in the git branch at
> > > >
> > > >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > > >  acpi-ec-work
> > > >
> > > > on top of 5.6-rc3.
> > >
> > > Jian-Hong, can you please test this on Asus UX434DA?
> > > Check if the screen brightness hotkeys are still working after these changes.
> >
> > Hi Rafael,
> >
> > Thanks for your patches, but we found an issue:
> > The laptops like ASUS UX434DA's screen brightness hotkeys work before
> > this patch series.  However, the hotkeys are failed with the patch
> > "ACPI: EC: Unify handling of event handler installation failures".
>
> So I have modified the series to avoid the change that can possibly break this.
>
> Can you please pull the new series from
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
>  acpi-ec-work
>
> (same branch) and retest?

Note that the current top-most commit in that branch is

0957d98f50da ACPI: EC: Consolidate event handler installation code



>
> I'll post the updated patches later this week, but it would be good to
> try them on now if possible.
>
> Thanks!

Re: [PATCH 0/6] ACPI: EC: Updates related to initialization

[Jian-Hong Pan]

Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月2日 週一 下午7:45寫道：
>
> On Mon, Mar 2, 2020 at 11:38 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> > >
> > > Daniel Drake <drake@endlessm.com> 於 2020年2月28日 週五 下午5:43寫道：
> > > >
> > > > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > The purpose of this series of update of the ACPI EC driver is to make its
> > > > > initialization more straightforward.
> > > > >
> > > > > They fix a couple of issues, clean up some things, remove redundant code etc.
> > > > >
> > > > > Please refer to the changelogs of individual patches for details.
> > > > >
> > > > > For easier access, the series is available in the git branch at
> > > > >
> > > > >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > > > >  acpi-ec-work
> > > > >
> > > > > on top of 5.6-rc3.
> > > >
> > > > Jian-Hong, can you please test this on Asus UX434DA?
> > > > Check if the screen brightness hotkeys are still working after these changes.
> > >
> > > Hi Rafael,
> > >
> > > Thanks for your patches, but we found an issue:
> > > The laptops like ASUS UX434DA's screen brightness hotkeys work before
> > > this patch series.  However, the hotkeys are failed with the patch
> > > "ACPI: EC: Unify handling of event handler installation failures".
> >
> > So I have modified the series to avoid the change that can possibly break this.
> >
> > Can you please pull the new series from
> >
> >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> >  acpi-ec-work
> >
> > (same branch) and retest?
>
> Note that the current top-most commit in that branch is
>
> 0957d98f50da ACPI: EC: Consolidate event handler installation code

I tested the commits in acpi-ec-work branch whose last commit is
0957d98f50da ("ACPI: EC: Consolidate event handler installation
code").  The screen brightness hotkeys are still failed with
0957d98f50da ("ACPI: EC: Consolidate event handler installation
code").

I tweak and add some debug messages:

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 85f1fe8e208a..3887f427283c 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1443,23 +1443,27 @@ static bool install_gpe_event_handler(struct
acpi_ec *ec)
        return true;
 }

-static bool install_gpio_irq_event_handler(struct acpi_ec *ec,
+static int install_gpio_irq_event_handler(struct acpi_ec *ec,
                                           struct acpi_device *device)
 {
        int irq, ret;

        /* ACPI reduced hardware platforms use a GpioInt specified in _CRS. */
        irq = acpi_dev_gpio_irq_get(device, 0);
-       if (irq < 0)
-               return false;
+       if (irq < 0) {
+               pr_err("%s: acpi_dev_gpio_irq_get returns %d\n", __func__, irq);
+               return irq;
+       }

        ret = request_irq(irq, acpi_ec_irq_handler, IRQF_SHARED, "ACPI EC", ec);
-       if (ret < 0)
-               return false;
+       if (ret < 0) {
+               pr_err("%s: request_irq returns %d\n", __func__, ret);
+               return ret;
+       }

        ec->irq = irq;

-       return true;
+       return 0;
 }

 /**
@@ -1517,9 +1521,11 @@ static int ec_install_handlers(struct acpi_ec
*ec, struct acpi_device *device)
                         * fatal, because the EC can be polled for events.
                         */
                } else {
-                       ready = install_gpio_irq_event_handler(ec, device);
-                       if (!ready)
-                               return -ENXIO;
+                       pr_err("%s: install_gpio_irq_event_handler\n",
__func__);
+                       int ret = install_gpio_irq_event_handler(ec, device);
+                       if (ret)
+                               return ret;
+                       ready = true;
                }
                if (ready) {
                        set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);

The dmesg shows:

[    0.121117] ACPI: EC: ec_install_handlers: install_gpio_irq_event_handler
[    0.121133] ACPI: EC: install_gpio_irq_event_handler:
acpi_dev_gpio_irq_get returns -517

Originally, ec_install_handlers() will return the returned value from
install_gpio_irq_event_handler() from acpi_dev_gpio_irq_get(), which
is -EPROBE_DEFER, instead of -ENXIO.  However, ec_install_handlers()
returns -ENXIO directly if install_gpio_irq_event_handler() returns
false in patch ("ACPI: EC: Consolidate event handler installation
code").  Here needs some modification.

Jian-Hong Pan

Re: [PATCH 0/6] ACPI: EC: Updates related to initialization

[Rafael J. Wysocki]

On Tue, Mar 3, 2020 at 8:29 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
>
> Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月2日 週一 下午7:45寫道：
> >
> > On Mon, Mar 2, 2020 at 11:38 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Mon, Mar 2, 2020 at 6:54 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> > > >
> > > > Daniel Drake <drake@endlessm.com> 於 2020年2月28日 週五 下午5:43寫道：
> > > > >
> > > > > On Thu, Feb 27, 2020 at 10:25 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > > > > The purpose of this series of update of the ACPI EC driver is to make its
> > > > > > initialization more straightforward.
> > > > > >
> > > > > > They fix a couple of issues, clean up some things, remove redundant code etc.
> > > > > >
> > > > > > Please refer to the changelogs of individual patches for details.
> > > > > >
> > > > > > For easier access, the series is available in the git branch at
> > > > > >
> > > > > >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > > > > >  acpi-ec-work
> > > > > >
> > > > > > on top of 5.6-rc3.
> > > > >
> > > > > Jian-Hong, can you please test this on Asus UX434DA?
> > > > > Check if the screen brightness hotkeys are still working after these changes.
> > > >
> > > > Hi Rafael,
> > > >
> > > > Thanks for your patches, but we found an issue:
> > > > The laptops like ASUS UX434DA's screen brightness hotkeys work before
> > > > this patch series.  However, the hotkeys are failed with the patch
> > > > "ACPI: EC: Unify handling of event handler installation failures".
> > >
> > > So I have modified the series to avoid the change that can possibly break this.
> > >
> > > Can you please pull the new series from
> > >
> > >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > >  acpi-ec-work
> > >
> > > (same branch) and retest?
> >
> > Note that the current top-most commit in that branch is
> >
> > 0957d98f50da ACPI: EC: Consolidate event handler installation code
>
> I tested the commits in acpi-ec-work branch whose last commit is
> 0957d98f50da ("ACPI: EC: Consolidate event handler installation
> code").  The screen brightness hotkeys are still failed with
> 0957d98f50da ("ACPI: EC: Consolidate event handler installation
> code").
>
> I tweak and add some debug messages:
>
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 85f1fe8e208a..3887f427283c 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -1443,23 +1443,27 @@ static bool install_gpe_event_handler(struct
> acpi_ec *ec)
>         return true;
>  }
>
> -static bool install_gpio_irq_event_handler(struct acpi_ec *ec,
> +static int install_gpio_irq_event_handler(struct acpi_ec *ec,
>                                            struct acpi_device *device)
>  {
>         int irq, ret;
>
>         /* ACPI reduced hardware platforms use a GpioInt specified in _CRS. */
>         irq = acpi_dev_gpio_irq_get(device, 0);
> -       if (irq < 0)
> -               return false;
> +       if (irq < 0) {
> +               pr_err("%s: acpi_dev_gpio_irq_get returns %d\n", __func__, irq);
> +               return irq;
> +       }
>
>         ret = request_irq(irq, acpi_ec_irq_handler, IRQF_SHARED, "ACPI EC", ec);
> -       if (ret < 0)
> -               return false;
> +       if (ret < 0) {
> +               pr_err("%s: request_irq returns %d\n", __func__, ret);
> +               return ret;
> +       }
>
>         ec->irq = irq;
>
> -       return true;
> +       return 0;
>  }
>
>  /**
> @@ -1517,9 +1521,11 @@ static int ec_install_handlers(struct acpi_ec
> *ec, struct acpi_device *device)
>                          * fatal, because the EC can be polled for events.
>                          */
>                 } else {
> -                       ready = install_gpio_irq_event_handler(ec, device);
> -                       if (!ready)
> -                               return -ENXIO;
> +                       pr_err("%s: install_gpio_irq_event_handler\n",
> __func__);
> +                       int ret = install_gpio_irq_event_handler(ec, device);
> +                       if (ret)
> +                               return ret;
> +                       ready = true;
>                 }
>                 if (ready) {
>                         set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
>
> The dmesg shows:
>
> [    0.121117] ACPI: EC: ec_install_handlers: install_gpio_irq_event_handler
> [    0.121133] ACPI: EC: install_gpio_irq_event_handler:
> acpi_dev_gpio_irq_get returns -517
>
> Originally, ec_install_handlers() will return the returned value from
> install_gpio_irq_event_handler() from acpi_dev_gpio_irq_get(), which
> is -EPROBE_DEFER, instead of -ENXIO.  However, ec_install_handlers()
> returns -ENXIO directly if install_gpio_irq_event_handler() returns
> false in patch ("ACPI: EC: Consolidate event handler installation
> code").  Here needs some modification.

Thanks, I forgot about the -EPROBE_DEFER case.

Re: [PATCH 0/6] ACPI: EC: Updates related to initialization

[Rafael J. Wysocki]

On Tue, Mar 3, 2020 at 10:09 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Mar 3, 2020 at 8:29 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> >
> > Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月2日 週一 下午7:45寫道：
> > >

[cut]

> >
> > Originally, ec_install_handlers() will return the returned value from
> > install_gpio_irq_event_handler() from acpi_dev_gpio_irq_get(), which
> > is -EPROBE_DEFER, instead of -ENXIO.  However, ec_install_handlers()
> > returns -ENXIO directly if install_gpio_irq_event_handler() returns
> > false in patch ("ACPI: EC: Consolidate event handler installation
> > code").  Here needs some modification.
>
> Thanks, I forgot about the -EPROBE_DEFER case.

The top-most commit in the git branch at

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-ec-work

has been updated to take that case into account (I think that it
should be spelled out explicitly or it will be very easy to overlook
in the future).

Please test this one if possible.

Re: [PATCH 0/6] ACPI: EC: Updates related to initialization

[Jian-Hong Pan]

Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月4日 週三 上午6:23寫道：
>
> On Tue, Mar 3, 2020 at 10:09 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Mar 3, 2020 at 8:29 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> > >
> > > Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月2日 週一 下午7:45寫道：
> > > >
>
> [cut]
>
> > >
> > > Originally, ec_install_handlers() will return the returned value from
> > > install_gpio_irq_event_handler() from acpi_dev_gpio_irq_get(), which
> > > is -EPROBE_DEFER, instead of -ENXIO.  However, ec_install_handlers()
> > > returns -ENXIO directly if install_gpio_irq_event_handler() returns
> > > false in patch ("ACPI: EC: Consolidate event handler installation
> > > code").  Here needs some modification.
> >
> > Thanks, I forgot about the -EPROBE_DEFER case.
>
> The top-most commit in the git branch at
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
>  acpi-ec-work
>
> has been updated to take that case into account (I think that it
> should be spelled out explicitly or it will be very easy to overlook
> in the future).
>
> Please test this one if possible.

Tested the commits on some laptops including ASUS UX434DA.  The
brightness hotkeys are working now.

Jian-Hong Pan

Re: [PATCH 0/6] ACPI: EC: Updates related to initialization

[Rafael J. Wysocki]

On Wed, Mar 4, 2020 at 3:54 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
>
> Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月4日 週三 上午6:23寫道：
> >
> > On Tue, Mar 3, 2020 at 10:09 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Tue, Mar 3, 2020 at 8:29 AM Jian-Hong Pan <jian-hong@endlessm.com> wrote:
> > > >
> > > > Rafael J. Wysocki <rafael@kernel.org> 於 2020年3月2日 週一 下午7:45寫道：
> > > > >
> >
> > [cut]
> >
> > > >
> > > > Originally, ec_install_handlers() will return the returned value from
> > > > install_gpio_irq_event_handler() from acpi_dev_gpio_irq_get(), which
> > > > is -EPROBE_DEFER, instead of -ENXIO.  However, ec_install_handlers()
> > > > returns -ENXIO directly if install_gpio_irq_event_handler() returns
> > > > false in patch ("ACPI: EC: Consolidate event handler installation
> > > > code").  Here needs some modification.
> > >
> > > Thanks, I forgot about the -EPROBE_DEFER case.
> >
> > The top-most commit in the git branch at
> >
> >  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> >  acpi-ec-work
> >
> > has been updated to take that case into account (I think that it
> > should be spelled out explicitly or it will be very easy to overlook
> > in the future).
> >
> > Please test this one if possible.
>
> Tested the commits on some laptops including ASUS UX434DA.  The
> brightness hotkeys are working now.

Thank you!

[PATCH v2 0/6] ACPI: EC: Updates related to initialization

[Rafael J. Wysocki]

Hi All,

On Thursday, February 27, 2020 11:19:19 PM CET Rafael J. Wysocki wrote:
> 
> The purpose of this series of update of the ACPI EC driver is to make its
> initialization more straightforward.
> 
> They fix a couple of issues, clean up some things, remove redundant code etc.
> 
> Please refer to the changelogs of individual patches for details.
> 
> For easier access, the series is available in the git branch at
> 
>  git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
>  acpi-ec-work
> 
> on top of 5.6-rc3.

The above is still true, including the location of the git branch containing
the changes.

Since the original submission, the series has been rearranged to reduce
artificial dependencies between the patches and the last patch (previously
[4/6]) has been reworked to take deferred probing into account properly.

Thanks!

[PATCH v2 1/6] ACPI: EC: Avoid printing confusing messages in acpi_ec_setup()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

It doesn't really make sense to pass ec->handle of the ECDT EC to
acpi_handle_info(), because it is set to ACPI_ROOT_OBJECT in
acpi_ec_ecdt_probe(), so rework acpi_ec_setup() to avoid using
acpi_handle_info() for printing messages.

First, notice that the "Used as first EC" message is not really
useful, because it is immediately followed by a more meaningful one
from either acpi_ec_ecdt_probe() or acpi_ec_dsdt_probe() (the latter
also includes the EC object path), so drop it altogether.

Second, use pr_info() for printing the EC configuration information.

While at it, make the code in question avoid printing invalid GPE or
IRQ numbers and make it print the GPE/IRQ information only when the
driver is ready to handle events.

Fixes: 72c77b7ea9ce ("ACPI / EC: Cleanup first_ec/boot_ec code")
Fixes: 406857f773b0 ("ACPI: EC: add support for hardware-reduced systems")
Cc: 5.5+ <stable@vger.kernel.org> # 5.5+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: No changes.

---
 drivers/acpi/ec.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index d1f1cf5d4bf0..2dc7cf2aeb21 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1584,14 +1584,19 @@ static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device,
 		return ret;
 
 	/* First EC capable of handling transactions */
-	if (!first_ec) {
+	if (!first_ec)
 		first_ec = ec;
-		acpi_handle_info(first_ec->handle, "Used as first EC\n");
+
+	pr_info("EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n", ec->command_addr,
+		ec->data_addr);
+
+	if (test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
+		if (ec->gpe >= 0)
+			pr_info("GPE=0x%x\n", ec->gpe);
+		else
+			pr_info("IRQ=%d\n", ec->irq);
 	}
 
-	acpi_handle_info(ec->handle,
-			 "GPE=0x%x, IRQ=%d, EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n",
-			 ec->gpe, ec->irq, ec->command_addr, ec->data_addr);
 	return ret;
 }
 
-- 
2.16.4

[PATCH v2 2/6] ACPI: EC: Avoid passing redundant argument to functions

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

After commit 406857f773b0 ("ACPI: EC: add support for hardware-reduced
systems") the handle_events argument passed to ec_install_handlers()
and acpi_ec_setup() is redundant, because it is always 'false' when
the device argument passed to them in NULL and it is always 'true'
otherwise, so the device argument can be tested against NULL instead
of testing the handle_events one.

Accordingly, modify ec_install_handlers() and acpi_ec_setup() to take
two arguments and reduce the number of checks in the former.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: No changes.

---
 drivers/acpi/ec.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 2dc7cf2aeb21..3153e7684053 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1476,8 +1476,7 @@ static int install_gpio_irq_event_handler(struct acpi_ec *ec,
  *       handler is not installed, which means "not able to handle
  *       transactions".
  */
-static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
-			       bool handle_events)
+static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 {
 	acpi_status status;
 
@@ -1507,7 +1506,7 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
 		set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
 
-	if (!handle_events)
+	if (!device)
 		return 0;
 
 	if (!test_bit(EC_FLAGS_QUERY_METHODS_INSTALLED, &ec->flags)) {
@@ -1520,13 +1519,10 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
 	if (!test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
 		if (ec->gpe >= 0) {
 			install_gpe_event_handler(ec);
-		} else if (device) {
+		} else {
 			int ret = install_gpio_irq_event_handler(ec, device);
-
 			if (ret)
 				return ret;
-		} else { /* No GPE and no GpioInt? */
-			return -ENODEV;
 		}
 	}
 	/* EC is fully operational, allow queries */
@@ -1574,12 +1570,11 @@ static void ec_remove_handlers(struct acpi_ec *ec)
 	}
 }
 
-static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device,
-			 bool handle_events)
+static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device)
 {
 	int ret;
 
-	ret = ec_install_handlers(ec, device, handle_events);
+	ret = ec_install_handlers(ec, device);
 	if (ret)
 		return ret;
 
@@ -1660,7 +1655,7 @@ static int acpi_ec_add(struct acpi_device *device)
 		}
 	}
 
-	ret = acpi_ec_setup(ec, device, true);
+	ret = acpi_ec_setup(ec, device);
 	if (ret)
 		goto err_query;
 
@@ -1780,7 +1775,7 @@ void __init acpi_ec_dsdt_probe(void)
 	 * At this point, the GPE is not fully initialized, so do not to
 	 * handle the events.
 	 */
-	ret = acpi_ec_setup(ec, NULL, false);
+	ret = acpi_ec_setup(ec, NULL);
 	if (ret) {
 		acpi_ec_free(ec);
 		return;
@@ -1967,7 +1962,7 @@ void __init acpi_ec_ecdt_probe(void)
 	 * At this point, the namespace is not initialized, so do not find
 	 * the namespace objects, or handle the events.
 	 */
-	ret = acpi_ec_setup(ec, NULL, false);
+	ret = acpi_ec_setup(ec, NULL);
 	if (ret) {
 		acpi_ec_free(ec);
 		return;
-- 
2.16.4

[PATCH v2 3/6] ACPI: EC: Drop AE_NOT_FOUND special case from ec_install_handlers()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

If the status value returned by acpi_install_address_space_handler()
in ec_install_handlers() is AE_NOT_FOUND, it is treated in a special
way, apparently because it might mean a _REG method evaluation
failure (at least that is the case according to the comment in
there), but acpi_install_address_space_handler() does not take
_REG evaluation errors into account at all, so the AE_NOT_FOUND
special handling is confusing at best.

For this reason, change ec_install_handlers() to stop the EC and
return -ENODEV on all acpi_install_address_space_handler() errors.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: No changes.

---
 drivers/acpi/ec.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 3153e7684053..6f501d552e6e 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1489,19 +1489,8 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 							    &acpi_ec_space_handler,
 							    NULL, ec);
 		if (ACPI_FAILURE(status)) {
-			if (status == AE_NOT_FOUND) {
-				/*
-				 * Maybe OS fails in evaluating the _REG
-				 * object. The AE_NOT_FOUND error will be
-				 * ignored and OS * continue to initialize
-				 * EC.
-				 */
-				pr_err("Fail in evaluating the _REG object"
-					" of EC device. Broken bios is suspected.\n");
-			} else {
-				acpi_ec_stop(ec, false);
-				return -ENODEV;
-			}
+			acpi_ec_stop(ec, false);
+			return -ENODEV;
 		}
 		set_bit(EC_FLAGS_EC_HANDLER_INSTALLED, &ec->flags);
 	}
-- 
2.16.4

[PATCH v2 4/6] ACPI: EC: Simplify acpi_ec_add()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

First, notice that if the device ID in acpi_ec_add() is equal to
ACPI_ECDT_HID, boot_ec_is_ecdt must be set, because this means
that the device object passed to acpi_ec_add() comes from
acpi_ec_ecdt_start() which fails if boot_ec_is_ecdt is unset.
Accordingly, boot_ec_is_ecdt need not be set again in that case,
so drop that redundant update of it from the code.

Next, ec->handle must be a valid ACPI handle right before
returning 0 from acpi_ec_add(), because it either is the handle
of the device object passed to that function, or it is the boot EC
handle coming from acpi_ec_ecdt_start() which fails if it cannot
find a valid handle for the boot EC.  Moreover, the object with
that handle is regarded as a valid representation of the EC in all
cases, so there is no reason to avoid the _DEP list update walk if
that handle is the boot EC handle.  Accordingly, drop the dep_update
local variable from acpi_ec_add() and call acpi_walk_dep_device_list()
for ec->handle unconditionally before returning 0 from it.

Finally, the ec local variable in acpi_ec_add() need not be
initialized to NULL and the status local variable declaration
can be moved to the block in which it is used, so change the code
in accordance with these observations.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: Reorder (previously [5/6]) and rebase.

---
 drivers/acpi/ec.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 6f501d552e6e..116163add41b 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1605,19 +1605,18 @@ static bool acpi_ec_ecdt_get_handle(acpi_handle *phandle)
 
 static int acpi_ec_add(struct acpi_device *device)
 {
-	struct acpi_ec *ec = NULL;
-	bool dep_update = true;
-	acpi_status status;
+	struct acpi_ec *ec;
 	int ret;
 
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 
 	if (!strcmp(acpi_device_hid(device), ACPI_ECDT_HID)) {
-		boot_ec_is_ecdt = true;
+		/* Fast path: this device corresponds to the boot EC. */
 		ec = boot_ec;
-		dep_update = false;
 	} else {
+		acpi_status status;
+
 		ec = acpi_ec_alloc();
 		if (!ec)
 			return -ENOMEM;
@@ -1660,10 +1659,9 @@ static int acpi_ec_add(struct acpi_device *device)
 	ret = !!request_region(ec->command_addr, 1, "EC cmd");
 	WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr);
 
-	if (dep_update) {
-		/* Reprobe devices depending on the EC */
-		acpi_walk_dep_device_list(ec->handle);
-	}
+	/* Reprobe devices depending on the EC */
+	acpi_walk_dep_device_list(ec->handle);
+
 	acpi_handle_debug(ec->handle, "enumerated.\n");
 	return 0;
 
-- 
2.16.4

[PATCH v2 5/6] ACPI: EC: Use fast path in acpi_ec_add() for DSDT boot EC

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

If the boot EC comes from the DSDT, its ACPI handle is equal to the
handle of a device object with the PNP0C09 device ID.  If that
device object is passed to acpi_ec_add(), it is not necessary to
allocate a new EC structure for it and parse it, because that has
been done already, so change the function to use the fast path in
that case.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

-> v2: Reorder (previously [6/6]) and rebase.

---
 drivers/acpi/ec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 116163add41b..355d6973cb70 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1611,7 +1611,8 @@ static int acpi_ec_add(struct acpi_device *device)
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
 
-	if (!strcmp(acpi_device_hid(device), ACPI_ECDT_HID)) {
+	if ((boot_ec && boot_ec->handle == device->handle) ||
+	    !strcmp(acpi_device_hid(device), ACPI_ECDT_HID)) {
 		/* Fast path: this device corresponds to the boot EC. */
 		ec = boot_ec;
 	} else {
-- 
2.16.4

[PATCH v2 6/6] ACPI: EC: Consolidate event handler installation code

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

Commit 406857f773b0 ("ACPI: EC: add support for hardware-reduced
systems") made ec_install_handlers() return an error on failures
to configure a GPIO IRQ for the EC, but that is inconsistent with
the handling of the GPE event handler installation failures even
though it is exactly the same issue and the driver can respond to
it in the same way in both cases (the EC can be actively polled
for events through its registers if the event handler installation
fails).

Moreover, it requires acpi_ec_add() to take that special case into
account and disagrees with the ec_install_handlers() header comment.

For this reason, rework the event handler installation code in
ec_install_handlers() to explicitly take deferred probing (that
may be needed in the GPIO IRQ case) into account and to avoid
failing the EC initialization in any other case.

Among other things, reduce code duplication between
install_gpe_event_handler() and install_gpio_irq_event_handler() by
moving some code from there into ec_install_handlers() itself and
simplify the error code path in acpi_ec_add().

While at it, turn the ec_install_handlers() header comment into
a proper kerneldoc one and add some general control flow information
to it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Jian-Hong Pan <jian-hong@endlessm.com>
---

-> v2:
   * Reorder (previously [4/6]) and rebase.
   * Take possible deferred GPIO IRQ acquisition into account.

---
 drivers/acpi/ec.c | 114 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 61 insertions(+), 53 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 355d6973cb70..47baeec9190d 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1427,54 +1427,43 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 	return AE_CTRL_TERMINATE;
 }
 
-static void install_gpe_event_handler(struct acpi_ec *ec)
-{
-	acpi_status status =
-		acpi_install_gpe_raw_handler(NULL, ec->gpe,
-					     ACPI_GPE_EDGE_TRIGGERED,
-					     &acpi_ec_gpe_handler,
-					     ec);
-	if (ACPI_SUCCESS(status)) {
-		/* This is not fatal as we can poll EC events */
-		set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
-		acpi_ec_leave_noirq(ec);
-		if (test_bit(EC_FLAGS_STARTED, &ec->flags) &&
-		    ec->reference_count >= 1)
-			acpi_ec_enable_gpe(ec, true);
-	}
-}
-
-/* ACPI reduced hardware platforms use a GpioInt specified in _CRS. */
-static int install_gpio_irq_event_handler(struct acpi_ec *ec,
-					  struct acpi_device *device)
+static bool install_gpe_event_handler(struct acpi_ec *ec)
 {
-	int irq = acpi_dev_gpio_irq_get(device, 0);
-	int ret;
-
-	if (irq < 0)
-		return irq;
+	acpi_status status;
 
-	ret = request_irq(irq, acpi_ec_irq_handler, IRQF_SHARED,
-			  "ACPI EC", ec);
+	status = acpi_install_gpe_raw_handler(NULL, ec->gpe,
+					      ACPI_GPE_EDGE_TRIGGERED,
+					      &acpi_ec_gpe_handler, ec);
+	if (ACPI_FAILURE(status))
+		return false;
 
-	/*
-	 * Unlike the GPE case, we treat errors here as fatal, we'll only
-	 * implement GPIO polling if we find a case that needs it.
-	 */
-	if (ret < 0)
-		return ret;
+	if (test_bit(EC_FLAGS_STARTED, &ec->flags) && ec->reference_count >= 1)
+		acpi_ec_enable_gpe(ec, true);
 
-	ec->irq = irq;
-	set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
-	acpi_ec_leave_noirq(ec);
+	return true;
+}
 
-	return 0;
+static bool install_gpio_irq_event_handler(struct acpi_ec *ec)
+{
+	return request_irq(ec->irq, acpi_ec_irq_handler, IRQF_SHARED,
+			   "ACPI EC", ec) >= 0;
 }
 
-/*
- * Note: This function returns an error code only when the address space
- *       handler is not installed, which means "not able to handle
- *       transactions".
+/**
+ * ec_install_handlers - Install service callbacks and register query methods.
+ * @ec: Target EC.
+ * @device: ACPI device object corresponding to @ec.
+ *
+ * Install a handler for the EC address space type unless it has been installed
+ * already.  If @device is not NULL, also look for EC query methods in the
+ * namespace and register them, and install an event (either GPE or GPIO IRQ)
+ * handler for the EC, if possible.
+ *
+ * Return:
+ * -ENODEV if the address space handler cannot be installed, which means
+ *  "unable to handle transactions",
+ * -EPROBE_DEFER if GPIO IRQ acquisition needs to be deferred,
+ * or 0 (success) otherwise.
  */
 static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 {
@@ -1498,6 +1487,19 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 	if (!device)
 		return 0;
 
+	if (ec->gpe < 0) {
+		/* ACPI reduced hardware platforms use a GpioInt from _CRS. */
+		int irq = acpi_dev_gpio_irq_get(device, 0);
+		/*
+		 * Bail out right away for deferred probing or complete the
+		 * initialization regardless of any other errors.
+		 */
+		if (irq == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		else if (irq >= 0)
+			ec->irq = irq;
+	}
+
 	if (!test_bit(EC_FLAGS_QUERY_METHODS_INSTALLED, &ec->flags)) {
 		/* Find and register all query methods */
 		acpi_walk_namespace(ACPI_TYPE_METHOD, ec->handle, 1,
@@ -1506,13 +1508,21 @@ static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device)
 		set_bit(EC_FLAGS_QUERY_METHODS_INSTALLED, &ec->flags);
 	}
 	if (!test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
-		if (ec->gpe >= 0) {
-			install_gpe_event_handler(ec);
-		} else {
-			int ret = install_gpio_irq_event_handler(ec, device);
-			if (ret)
-				return ret;
+		bool ready = false;
+
+		if (ec->gpe >= 0)
+			ready = install_gpe_event_handler(ec);
+		else if (ec->irq >= 0)
+			ready = install_gpio_irq_event_handler(ec);
+
+		if (ready) {
+			set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
+			acpi_ec_leave_noirq(ec);
 		}
+		/*
+		 * Failures to install an event handler are not fatal, because
+		 * the EC can be polled for events.
+		 */
 	}
 	/* EC is fully operational, allow queries */
 	acpi_ec_enable_event(ec);
@@ -1625,7 +1635,7 @@ static int acpi_ec_add(struct acpi_device *device)
 		status = ec_parse_device(device->handle, 0, ec, NULL);
 		if (status != AE_CTRL_TERMINATE) {
 			ret = -EINVAL;
-			goto err_alloc;
+			goto err;
 		}
 
 		if (boot_ec && ec->command_addr == boot_ec->command_addr &&
@@ -1646,7 +1656,7 @@ static int acpi_ec_add(struct acpi_device *device)
 
 	ret = acpi_ec_setup(ec, device);
 	if (ret)
-		goto err_query;
+		goto err;
 
 	if (ec == boot_ec)
 		acpi_handle_info(boot_ec->handle,
@@ -1666,12 +1676,10 @@ static int acpi_ec_add(struct acpi_device *device)
 	acpi_handle_debug(ec->handle, "enumerated.\n");
 	return 0;
 
-err_query:
-	if (ec != boot_ec)
-		acpi_ec_remove_query_handlers(ec, true, 0);
-err_alloc:
+err:
 	if (ec != boot_ec)
 		acpi_ec_free(ec);
+
 	return ret;
 }
 
-- 
2.16.4