Re: [PATCH] ACPI: prefer bool over int for predicates

2018-05-02 Thread Rafael J. Wysocki
On Sunday, April 8, 2018 10:46:27 AM CEST Joey Pabalinas wrote:
> 
> --dohnnvt2hwn6rmo4
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
> 
> Prefer bool over int for variables / returns which are
> predicate expressions to make it explicit that these
> expressions are evaluating simple "yes or no?" queries.

In new code - yes.

But changing old code just for this purpose is at least questionable.

> This makes it more obvious which expressions are _not_
> that simple and require more attention, e.g. an `int ret`
> meant to hold 0 or -ENOENT as a return value or an
> `unsigned nmemb` meant to refer to the number of valid
> members in some arbitrary array.
> 
> Change relevant variable / return types from int to bool and
> prefer a true / false value for predicate expressions versus
> a plain 1 / 0 value.

This makes a bunch of unrelated changes without a clear pattern
in many places and I'm not convinced that the reason is good enough.

> Signed-off-by: Joey Pabalinas 
> 
>  drivers/acpi/battery.c  |  4 ++--
>  drivers/acpi/ec.c   | 20 +---
>  drivers/acpi/pci_root.c | 17 ++---
>  drivers/acpi/scan.c |  6 +++---
>  include/acpi/acpi_bus.h |  2 +-
>  5 files changed, 21 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index bdb24d636d9acc9c1a..f1a5fb5252969f0478 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -1416,7 +1416,7 @@ static int acpi_battery_add(struct acpi_device *devic=
> e)
>   battery->pm_nb.notifier_call =3D battery_notify;
>   register_pm_notifier(&battery->pm_nb);
> =20

Broken whitespace.

> - device_init_wakeup(&device->dev, 1);
> + device_init_wakeup(&device->dev, true);
> =20
>   return result;
> =20
> @@ -1434,7 +1434,7 @@ static int acpi_battery_remove(struct acpi_device *de=
> vice)
> =20
>   if (!device || !acpi_driver_data(device))
>   return -EINVAL;
> - device_init_wakeup(&device->dev, 0);
> + device_init_wakeup(&device->dev, false);
>   battery =3D acpi_driver_data(device);
>   unregister_pm_notifier(&battery->pm_nb);
>  #ifdef CONFIG_ACPI_PROCFS_POWER
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 30a5729565575f83cb..d4a564ab9cdd53046c 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -350,7 +350,7 @@ static inline bool acpi_ec_is_gpe_raised(struct acpi_ec=
>  *ec)
>   acpi_event_status gpe_status =3D 0;
> =20
>   (void)acpi_get_gpe_status(NULL, ec->gpe, &gpe_status);
> - return (gpe_status & ACPI_EVENT_FLAG_STATUS_SET) ? true : false;
> + return gpe_status & ACPI_EVENT_FLAG_STATUS_SET;

And this isn't entirely correct even.

You should apply !! to the expression to get the same result really.

Thanks,
Rafael



[PATCH] ACPI: prefer bool over int for predicates

2018-04-08 Thread Joey Pabalinas
Prefer bool over int for variables / returns which are
predicate expressions to make it explicit that these
expressions are evaluating simple "yes or no?" queries.

This makes it more obvious which expressions are _not_
that simple and require more attention, e.g. an `int ret`
meant to hold 0 or -ENOENT as a return value or an
`unsigned nmemb` meant to refer to the number of valid
members in some arbitrary array.

Change relevant variable / return types from int to bool and
prefer a true / false value for predicate expressions versus
a plain 1 / 0 value.

Signed-off-by: Joey Pabalinas 

 drivers/acpi/battery.c  |  4 ++--
 drivers/acpi/ec.c   | 20 +---
 drivers/acpi/pci_root.c | 17 ++---
 drivers/acpi/scan.c |  6 +++---
 include/acpi/acpi_bus.h |  2 +-
 5 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index bdb24d636d9acc9c1a..f1a5fb5252969f0478 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -1416,7 +1416,7 @@ static int acpi_battery_add(struct acpi_device *device)
battery->pm_nb.notifier_call = battery_notify;
register_pm_notifier(&battery->pm_nb);
 
-   device_init_wakeup(&device->dev, 1);
+   device_init_wakeup(&device->dev, true);
 
return result;
 
@@ -1434,7 +1434,7 @@ static int acpi_battery_remove(struct acpi_device *device)
 
if (!device || !acpi_driver_data(device))
return -EINVAL;
-   device_init_wakeup(&device->dev, 0);
+   device_init_wakeup(&device->dev, false);
battery = acpi_driver_data(device);
unregister_pm_notifier(&battery->pm_nb);
 #ifdef CONFIG_ACPI_PROCFS_POWER
diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 30a5729565575f83cb..d4a564ab9cdd53046c 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -350,7 +350,7 @@ static inline bool acpi_ec_is_gpe_raised(struct acpi_ec *ec)
acpi_event_status gpe_status = 0;
 
(void)acpi_get_gpe_status(NULL, ec->gpe, &gpe_status);
-   return (gpe_status & ACPI_EVENT_FLAG_STATUS_SET) ? true : false;
+   return gpe_status & ACPI_EVENT_FLAG_STATUS_SET;
 }
 
 static inline void acpi_ec_enable_gpe(struct acpi_ec *ec, bool open)
@@ -580,28 +580,26 @@ static bool acpi_ec_guard_event(struct acpi_ec *ec)
return guarded;
 }
 
-static int ec_transaction_polled(struct acpi_ec *ec)
+static bool ec_transaction_polled(struct acpi_ec *ec)
 {
unsigned long flags;
-   int ret = 0;
+   bool polled;
 
spin_lock_irqsave(&ec->lock, flags);
-   if (ec->curr && (ec->curr->flags & ACPI_EC_COMMAND_POLL))
-   ret = 1;
+   polled = ec->curr && (ec->curr->flags & ACPI_EC_COMMAND_POLL);
spin_unlock_irqrestore(&ec->lock, flags);
-   return ret;
+   return polled;
 }
 
-static int ec_transaction_completed(struct acpi_ec *ec)
+static bool ec_transaction_completed(struct acpi_ec *ec)
 {
unsigned long flags;
-   int ret = 0;
+   bool completed;
 
spin_lock_irqsave(&ec->lock, flags);
-   if (ec->curr && (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE))
-   ret = 1;
+   completed = ec->curr && (ec->curr->flags & ACPI_EC_COMMAND_COMPLETE);
spin_unlock_irqrestore(&ec->lock, flags);
-   return ret;
+   return completed;
 }
 
 static inline void ec_transaction_transition(struct acpi_ec *ec, unsigned long 
flag)
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 6fc204a524932e97f4..61c0c079cff346e492 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -81,20 +81,15 @@ static DEFINE_MUTEX(osc_lock);
  * Note: we could make this API take a struct acpi_device * instead, but
  * for now, it's more convenient to operate on an acpi_handle.
  */
-int acpi_is_root_bridge(acpi_handle handle)
+bool acpi_is_root_bridge(acpi_handle handle)
 {
-   int ret;
struct acpi_device *device;
 
-   ret = acpi_bus_get_device(handle, &device);
-   if (ret)
-   return 0;
-
-   ret = acpi_match_device_ids(device, root_device_ids);
-   if (ret)
-   return 0;
-   else
-   return 1;
+   if (acpi_bus_get_device(handle, &device))
+   return false;
+   if (acpi_match_device_ids(device, root_device_ids))
+   return false;
+   return true;
 }
 EXPORT_SYMBOL_GPL(acpi_is_root_bridge);
 
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 490498eca0d3db7d6a..8e3d436184104b5799 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -625,7 +625,7 @@ int acpi_device_add(struct acpi_device *device,
 {
int result;
struct acpi_device_bus_id *acpi_device_bus_id, *new_bus_id;
-   int found = 0;
+   bool found = false;
 
if (device->handle) {
acpi_status status;
@@ -667,7 +667,7 @@ int acpi_device_add(struct acpi_device *device,
if (!strcmp(acpi_device_bus_id->bus_i