[PATCH 0/2] intel-rst: Small cleanups

[PATCH 0/2] intel-rst: Small cleanups

[Peter Ujfalusi]

Hi,

While trying to figure out why the intel-rst driver was not probing on my new
laptop (which turned out to be a differnet issue) I looked at the intel-rst
driver for clues and while reading the code I thought that these small changes
might be a good thing to send upstream.

Regards,
Peter
---
Peter Ujfalusi (2):
  intel-rst: Use ACPI_FAILURE() macro instead !ACPI_SUCCESS() for error
    checking
  intel-rst: Clean up ACPI add function

 drivers/platform/x86/intel-rst.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

-- 
2.1.0

[PATCH 1/2] intel-rst: Use ACPI_FAILURE() macro instead !ACPI_SUCCESS() for error checking

[Peter Ujfalusi]

ACPI_SUCCESS is defined as:
#define ACPI_SUCCESS(a)			(!(a))

There is no need for the the double ! since there is already a macro
defined for failures: ACPI_FAILURE()

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>
---
 drivers/platform/x86/intel-rst.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel-rst.c b/drivers/platform/x86/intel-rst.c
index d45bca3..8c6a8fe 100644
--- a/drivers/platform/x86/intel-rst.c
+++ b/drivers/platform/x86/intel-rst.c
@@ -35,7 +35,7 @@ static ssize_t irst_show_wakeup_events(struct device *dev,
 	acpi = to_acpi_device(dev);
 
 	status = acpi_evaluate_integer(acpi->handle, "GFFS", NULL, &value);
-	if (!ACPI_SUCCESS(status))
+	if (ACPI_FAILURE(status))
 		return -EINVAL;
 
 	return sprintf(buf, "%lld\n", value);
@@ -59,7 +59,7 @@ static ssize_t irst_store_wakeup_events(struct device *dev,
 
 	status = acpi_execute_simple_method(acpi->handle, "SFFS", value);
 
-	if (!ACPI_SUCCESS(status))
+	if (ACPI_FAILURE(status))
 		return -EINVAL;
 
 	return count;
@@ -81,7 +81,7 @@ static ssize_t irst_show_wakeup_time(struct device *dev,
 	acpi = to_acpi_device(dev);
 
 	status = acpi_evaluate_integer(acpi->handle, "GFTV", NULL, &value);
-	if (!ACPI_SUCCESS(status))
+	if (ACPI_FAILURE(status))
 		return -EINVAL;
 
 	return sprintf(buf, "%lld\n", value);
@@ -105,7 +105,7 @@ static ssize_t irst_store_wakeup_time(struct device *dev,
 
 	status = acpi_execute_simple_method(acpi->handle, "SFTV", value);
 
-	if (!ACPI_SUCCESS(status))
+	if (ACPI_FAILURE(status))
 		return -EINVAL;
 
 	return count;
-- 
2.1.0

Re: [PATCH 1/2] intel-rst: Use ACPI_FAILURE() macro instead !ACPI_SUCCESS() for error checking

[Darren Hart]

On Wed, Sep 17, 2014 at 12:13:55AM +0300, Peter Ujfalusi wrote:
> ACPI_SUCCESS is defined as:
> #define ACPI_SUCCESS(a)			(!(a))
> 
> There is no need for the the double ! since there is already a macro
> defined for failures: ACPI_FAILURE()
> 

Cc: linux-acpi

Thanks,

Darren

> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>
> ---
>  drivers/platform/x86/intel-rst.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel-rst.c b/drivers/platform/x86/intel-rst.c
> index d45bca3..8c6a8fe 100644
> --- a/drivers/platform/x86/intel-rst.c
> +++ b/drivers/platform/x86/intel-rst.c
> @@ -35,7 +35,7 @@ static ssize_t irst_show_wakeup_events(struct device *dev,
>  	acpi = to_acpi_device(dev);
>  
>  	status = acpi_evaluate_integer(acpi->handle, "GFFS", NULL, &value);
> -	if (!ACPI_SUCCESS(status))
> +	if (ACPI_FAILURE(status))
>  		return -EINVAL;
>  
>  	return sprintf(buf, "%lld\n", value);
> @@ -59,7 +59,7 @@ static ssize_t irst_store_wakeup_events(struct device *dev,
>  
>  	status = acpi_execute_simple_method(acpi->handle, "SFFS", value);
>  
> -	if (!ACPI_SUCCESS(status))
> +	if (ACPI_FAILURE(status))
>  		return -EINVAL;
>  
>  	return count;
> @@ -81,7 +81,7 @@ static ssize_t irst_show_wakeup_time(struct device *dev,
>  	acpi = to_acpi_device(dev);
>  
>  	status = acpi_evaluate_integer(acpi->handle, "GFTV", NULL, &value);
> -	if (!ACPI_SUCCESS(status))
> +	if (ACPI_FAILURE(status))
>  		return -EINVAL;
>  
>  	return sprintf(buf, "%lld\n", value);
> @@ -105,7 +105,7 @@ static ssize_t irst_store_wakeup_time(struct device *dev,
>  
>  	status = acpi_execute_simple_method(acpi->handle, "SFTV", value);
>  
> -	if (!ACPI_SUCCESS(status))
> +	if (ACPI_FAILURE(status))
>  		return -EINVAL;
>  
>  	return count;
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH 2/2] intel-rst: Clean up ACPI add function

[Peter Ujfalusi]

There is no need to initialize the error since it is going to be assigned
with the return status of at least on of the device_create_file() call.

We can return directly in case the first file creation fails.
All the labels for goto can be removed (along with the gotos) as well.
Tell the compiler that the failures are unlikely so it can create better
binaries.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>
---
 drivers/platform/x86/intel-rst.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/x86/intel-rst.c b/drivers/platform/x86/intel-rst.c
index 8c6a8fe..7344d84 100644
--- a/drivers/platform/x86/intel-rst.c
+++ b/drivers/platform/x86/intel-rst.c
@@ -119,21 +119,16 @@ static struct device_attribute irst_timeout_attr = {
 
 static int irst_add(struct acpi_device *acpi)
 {
-	int error = 0;
+	int error;
 
 	error = device_create_file(&acpi->dev, &irst_timeout_attr);
-	if (error)
-		goto out;
+	if (unlikely(error))
+		return error;
 
 	error = device_create_file(&acpi->dev, &irst_wakeup_attr);
-	if (error)
-		goto out_timeout;
+	if (unlikely(error))
+		device_remove_file(&acpi->dev, &irst_timeout_attr);
 
-	return 0;
-
-out_timeout:
-	device_remove_file(&acpi->dev, &irst_timeout_attr);
-out:
 	return error;
 }
 
-- 
2.1.0

Re: [PATCH 2/2] intel-rst: Clean up ACPI add function

[Darren Hart]

On Wed, Sep 17, 2014 at 12:13:56AM +0300, Peter Ujfalusi wrote:
> There is no need to initialize the error since it is going to be assigned
> with the return status of at least on of the device_create_file() call.
> 
> We can return directly in case the first file creation fails.
> All the labels for goto can be removed (along with the gotos) as well.
> Tell the compiler that the failures are unlikely so it can create better
> binaries.

Looks reasonable to me.

Cc: linux-acpi


> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>
> ---
>  drivers/platform/x86/intel-rst.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel-rst.c b/drivers/platform/x86/intel-rst.c
> index 8c6a8fe..7344d84 100644
> --- a/drivers/platform/x86/intel-rst.c
> +++ b/drivers/platform/x86/intel-rst.c
> @@ -119,21 +119,16 @@ static struct device_attribute irst_timeout_attr = {
>  
>  static int irst_add(struct acpi_device *acpi)
>  {
> -	int error = 0;
> +	int error;
>  
>  	error = device_create_file(&acpi->dev, &irst_timeout_attr);
> -	if (error)
> -		goto out;
> +	if (unlikely(error))
> +		return error;
>  
>  	error = device_create_file(&acpi->dev, &irst_wakeup_attr);
> -	if (error)
> -		goto out_timeout;
> +	if (unlikely(error))
> +		device_remove_file(&acpi->dev, &irst_timeout_attr);
>  
> -	return 0;
> -
> -out_timeout:
> -	device_remove_file(&acpi->dev, &irst_timeout_attr);
> -out:
>  	return error;
>  }
>  
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Darren Hart
Intel Open Source Technology Center