[PATCH v1 4/7] ACPI: scan: Combine two conditionals in acpi_bus_attach()

Posted by Rafael J. Wysocki 3 weeks, 4 days ago

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

There are two conditionals in acpi_bus_attach() that can be combined,
which slightly reduces the overhead and makes the code a bit easier
to follow, so do that.

No intentional functional impact.

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

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 019a43e3b5d7..8c5a2fcef582 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2370,16 +2370,11 @@ static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
 	if (ret < 0)
 		return 0;
 
-	if (ret > 0 && !device->flags.enumeration_by_parent) {
+	if (!device->flags.enumeration_by_parent && (ret > 0 ||
+	    (!device->pnp.type.platform_id && !device->pnp.type.backlight)))
 		acpi_device_set_enumerated(device);
-		goto ok;
-	}
-
-	if (device->pnp.type.platform_id || device->pnp.type.backlight ||
-	    device->flags.enumeration_by_parent)
-		acpi_default_enumeration(device);
 	else
-		acpi_device_set_enumerated(device);
+		acpi_default_enumeration(device);
 
 ok:
 	acpi_dev_for_each_child(device, acpi_bus_attach, first_pass);
-- 
2.51.0

Re: [PATCH v1 4/7] ACPI: scan: Combine two conditionals in acpi_bus_attach()

Posted by Andy Shevchenko 3 weeks, 4 days ago

On Mon, Aug 31, 2026 at 07:59:32PM +0200, Rafael J. Wysocki wrote:

> There are two conditionals in acpi_bus_attach() that can be combined,
> which slightly reduces the overhead and makes the code a bit easier
> to follow, so do that.
> 
> No intentional functional impact.

...

> -	if (ret > 0 && !device->flags.enumeration_by_parent) {
> +	if (!device->flags.enumeration_by_parent && (ret > 0 ||
> +	    (!device->pnp.type.platform_id && !device->pnp.type.backlight)))
>  		acpi_device_set_enumerated(device);
> -		goto ok;
> -	}
> -
> -	if (device->pnp.type.platform_id || device->pnp.type.backlight ||
> -	    device->flags.enumeration_by_parent)
> -		acpi_default_enumeration(device);
>  	else
> -		acpi_device_set_enumerated(device);
> +		acpi_default_enumeration(device);

I would leave a longer line (having logical split)

	if (!device->flags.enumeration_by_parent &&
	    (ret > 0 || (!device->pnp.type.platform_id && !device->pnp.type.backlight)))
		acpi_device_set_enumerated(device);
	else
		acpi_default_enumeration(device);

Or even going further and cleaning too many negations (if I'm not mistaken in
the logic)

	if (device->flags.enumeration_by_parent ||
// not sure what the expected ret values here, maybe < 0 or == 0 part is not needed
	    (ret <= 0 && (device->pnp.type.platform_id || device->pnp.type.backlight)))
		acpi_default_enumeration(device);
	else
		acpi_device_set_enumerated(device);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/7] ACPI: scan: Combine two conditionals in acpi_bus_attach()

Posted by Rafael J. Wysocki (Intel) 3 weeks, 3 days ago

On Tue, Sep 1, 2026 at 10:40 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Aug 31, 2026 at 07:59:32PM +0200, Rafael J. Wysocki wrote:
>
> > There are two conditionals in acpi_bus_attach() that can be combined,
> > which slightly reduces the overhead and makes the code a bit easier
> > to follow, so do that.
> >
> > No intentional functional impact.
>
> ...
>
> > -     if (ret > 0 && !device->flags.enumeration_by_parent) {
> > +     if (!device->flags.enumeration_by_parent && (ret > 0 ||
> > +         (!device->pnp.type.platform_id && !device->pnp.type.backlight)))
> >               acpi_device_set_enumerated(device);
> > -             goto ok;
> > -     }
> > -
> > -     if (device->pnp.type.platform_id || device->pnp.type.backlight ||
> > -         device->flags.enumeration_by_parent)
> > -             acpi_default_enumeration(device);
> >       else
> > -             acpi_device_set_enumerated(device);
> > +             acpi_default_enumeration(device);
>
> I would leave a longer line (having logical split)
>
>         if (!device->flags.enumeration_by_parent &&
>             (ret > 0 || (!device->pnp.type.platform_id && !device->pnp.type.backlight)))
>                 acpi_device_set_enumerated(device);
>         else
>                 acpi_default_enumeration(device);
>
> Or even going further and cleaning too many negations (if I'm not mistaken in
> the logic)
>
>         if (device->flags.enumeration_by_parent ||
> // not sure what the expected ret values here, maybe < 0 or == 0 part is not needed

!ret should be fine.

>             (ret <= 0 && (device->pnp.type.platform_id || device->pnp.type.backlight)))
>                 acpi_default_enumeration(device);
>         else
>                 acpi_device_set_enumerated(device);

Yes, it looks better this way.