Re: [PATCH 2/3] toshiba_acpi: Change error checking logic from TCI functions

[Azael Avalos]

Hi Darren,

2016-08-28 10:56 GMT-06:00 Darren Hart :
> On Tue, Aug 16, 2016 at 12:06:16PM -0600, Azael Avalos wrote:
>> Currently the success/error checking logic is intermixed, making the
>> code a bit cumbersome to understand.
>>
>> This patch changes the affected functions to first check for errors
>> and take appropriate actions, then check for the supported features.
>>
>> This patch also separates the error check from the acpi_status and
>> the tci_raw function call error check, as those two are completely
>> unrelated and were nested in if/else statements.
>
> Thanks, this is a good improvement. One questions below...
>
>>
>> Signed-off-by: Azael Avalos 
>> ---
>>  drivers/platform/x86/toshiba_acpi.c | 222 
>> ++--
>>  1 file changed, 135 insertions(+), 87 deletions(-)
>>
>> diff --git a/drivers/platform/x86/toshiba_acpi.c 
>> b/drivers/platform/x86/toshiba_acpi.c
>> index c6fc5cc..2256cf5 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -476,10 +476,15 @@ static void toshiba_illumination_available(struct 
>> toshiba_acpi_dev *dev)
>>
>>   status = tci_raw(dev, in, out);
>>   sci_close(dev);
>> - if (ACPI_FAILURE(status))
>> + if (ACPI_FAILURE(status)) {
>>   pr_err("ACPI call to query Illumination support failed\n");
>> - else if (out[0] == TOS_SUCCESS)
>> - dev->illumination_supported = 1;
>> + return;
>> + }
>> +
>> + if (out[0] != TOS_SUCCESS)
>
> Does this condition not merit a pr_err message? It reads like an error...
>
> There are several similar situations below which are equally silent. Is this a
> deliberate decision?

This was on purpose, since we are querying for all the supported features,
the kernel log will contain a lot of error strings for not supported features,
as not all laptop models support all of them.

>
> --
> Darren Hart
> Intel Open Source Technology Center
> --
> To unsubscribe from this list: send the line "unsubscribe 
> platform-driver-x86" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
-- El mundo apesta y vosotros apestais tambien --

Re: [PATCH 2/3] toshiba_acpi: Change error checking logic from TCI functions

[Azael Avalos]

Hi Darren,

2016-08-28 10:56 GMT-06:00 Darren Hart :
> On Tue, Aug 16, 2016 at 12:06:16PM -0600, Azael Avalos wrote:
>> Currently the success/error checking logic is intermixed, making the
>> code a bit cumbersome to understand.
>>
>> This patch changes the affected functions to first check for errors
>> and take appropriate actions, then check for the supported features.
>>
>> This patch also separates the error check from the acpi_status and
>> the tci_raw function call error check, as those two are completely
>> unrelated and were nested in if/else statements.
>
> Thanks, this is a good improvement. One questions below...
>
>>
>> Signed-off-by: Azael Avalos 
>> ---
>>  drivers/platform/x86/toshiba_acpi.c | 222 
>> ++--
>>  1 file changed, 135 insertions(+), 87 deletions(-)
>>
>> diff --git a/drivers/platform/x86/toshiba_acpi.c 
>> b/drivers/platform/x86/toshiba_acpi.c
>> index c6fc5cc..2256cf5 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -476,10 +476,15 @@ static void toshiba_illumination_available(struct 
>> toshiba_acpi_dev *dev)
>>
>>   status = tci_raw(dev, in, out);
>>   sci_close(dev);
>> - if (ACPI_FAILURE(status))
>> + if (ACPI_FAILURE(status)) {
>>   pr_err("ACPI call to query Illumination support failed\n");
>> - else if (out[0] == TOS_SUCCESS)
>> - dev->illumination_supported = 1;
>> + return;
>> + }
>> +
>> + if (out[0] != TOS_SUCCESS)
>
> Does this condition not merit a pr_err message? It reads like an error...
>
> There are several similar situations below which are equally silent. Is this a
> deliberate decision?

This was on purpose, since we are querying for all the supported features,
the kernel log will contain a lot of error strings for not supported features,
as not all laptop models support all of them.

>
> --
> Darren Hart
> Intel Open Source Technology Center
> --
> To unsubscribe from this list: send the line "unsubscribe 
> platform-driver-x86" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
-- El mundo apesta y vosotros apestais tambien --

Re: [PATCH 2/3] toshiba_acpi: Change error checking logic from TCI functions

[Darren Hart]

On Tue, Aug 16, 2016 at 12:06:16PM -0600, Azael Avalos wrote:
> Currently the success/error checking logic is intermixed, making the
> code a bit cumbersome to understand.
> 
> This patch changes the affected functions to first check for errors
> and take appropriate actions, then check for the supported features.
> 
> This patch also separates the error check from the acpi_status and
> the tci_raw function call error check, as those two are completely
> unrelated and were nested in if/else statements.

Thanks, this is a good improvement. One questions below...

> 
> Signed-off-by: Azael Avalos 
> ---
>  drivers/platform/x86/toshiba_acpi.c | 222 
> ++--
>  1 file changed, 135 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c 
> b/drivers/platform/x86/toshiba_acpi.c
> index c6fc5cc..2256cf5 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -476,10 +476,15 @@ static void toshiba_illumination_available(struct 
> toshiba_acpi_dev *dev)
>  
>   status = tci_raw(dev, in, out);
>   sci_close(dev);
> - if (ACPI_FAILURE(status))
> + if (ACPI_FAILURE(status)) {
>   pr_err("ACPI call to query Illumination support failed\n");
> - else if (out[0] == TOS_SUCCESS)
> - dev->illumination_supported = 1;
> + return;
> + }
> +
> + if (out[0] != TOS_SUCCESS)

Does this condition not merit a pr_err message? It reads like an error...

There are several similar situations below which are equally silent. Is this a
deliberate decision?

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 2/3] toshiba_acpi: Change error checking logic from TCI functions

[Darren Hart]

On Tue, Aug 16, 2016 at 12:06:16PM -0600, Azael Avalos wrote:
> Currently the success/error checking logic is intermixed, making the
> code a bit cumbersome to understand.
> 
> This patch changes the affected functions to first check for errors
> and take appropriate actions, then check for the supported features.
> 
> This patch also separates the error check from the acpi_status and
> the tci_raw function call error check, as those two are completely
> unrelated and were nested in if/else statements.

Thanks, this is a good improvement. One questions below...

> 
> Signed-off-by: Azael Avalos 
> ---
>  drivers/platform/x86/toshiba_acpi.c | 222 
> ++--
>  1 file changed, 135 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c 
> b/drivers/platform/x86/toshiba_acpi.c
> index c6fc5cc..2256cf5 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -476,10 +476,15 @@ static void toshiba_illumination_available(struct 
> toshiba_acpi_dev *dev)
>  
>   status = tci_raw(dev, in, out);
>   sci_close(dev);
> - if (ACPI_FAILURE(status))
> + if (ACPI_FAILURE(status)) {
>   pr_err("ACPI call to query Illumination support failed\n");
> - else if (out[0] == TOS_SUCCESS)
> - dev->illumination_supported = 1;
> + return;
> + }
> +
> + if (out[0] != TOS_SUCCESS)

Does this condition not merit a pr_err message? It reads like an error...

There are several similar situations below which are equally silent. Is this a
deliberate decision?

-- 
Darren Hart
Intel Open Source Technology Center

[PATCH 2/3] toshiba_acpi: Change error checking logic from TCI functions

[Azael Avalos]

Currently the success/error checking logic is intermixed, making the
code a bit cumbersome to understand.

This patch changes the affected functions to first check for errors
and take appropriate actions, then check for the supported features.

This patch also separates the error check from the acpi_status and
the tci_raw function call error check, as those two are completely
unrelated and were nested in if/else statements.

Signed-off-by: Azael Avalos 
---
 drivers/platform/x86/toshiba_acpi.c | 222 ++--
 1 file changed, 135 insertions(+), 87 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c 
b/drivers/platform/x86/toshiba_acpi.c
index c6fc5cc..2256cf5 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -476,10 +476,15 @@ static void toshiba_illumination_available(struct 
toshiba_acpi_dev *dev)
 
status = tci_raw(dev, in, out);
sci_close(dev);
-   if (ACPI_FAILURE(status))
+   if (ACPI_FAILURE(status)) {
pr_err("ACPI call to query Illumination support failed\n");
-   else if (out[0] == TOS_SUCCESS)
-   dev->illumination_supported = 1;
+   return;
+   }
+
+   if (out[0] != TOS_SUCCESS)
+   return;
+
+   dev->illumination_supported = 1;
 }
 
 static void toshiba_illumination_set(struct led_classdev *cdev,
@@ -542,24 +547,28 @@ static void toshiba_kbd_illum_available(struct 
toshiba_acpi_dev *dev)
sci_close(dev);
if (ACPI_FAILURE(status)) {
pr_err("ACPI call to query kbd illumination support failed\n");
-   } else if (out[0] == TOS_SUCCESS) {
-   /*
-* Check for keyboard backlight timeout max value,
-* previous kbd backlight implementation set this to
-* 0x3c0003, and now the new implementation set this
-* to 0x3c001a, use this to distinguish between them.
-*/
-   if (out[3] == SCI_KBD_TIME_MAX)
-   dev->kbd_type = 2;
-   else
-   dev->kbd_type = 1;
-   /* Get the current keyboard backlight mode */
-   dev->kbd_mode = out[2] & SCI_KBD_MODE_MASK;
-   /* Get the current time (1-60 seconds) */
-   dev->kbd_time = out[2] >> HCI_MISC_SHIFT;
-   /* Flag as supported */
-   dev->kbd_illum_supported = 1;
+   return;
}
+
+   if (out[0] != TOS_SUCCESS)
+   return;
+
+   /*
+* Check for keyboard backlight timeout max value,
+* previous kbd backlight implementation set this to
+* 0x3c0003, and now the new implementation set this
+* to 0x3c001a, use this to distinguish between them.
+*/
+   if (out[3] == SCI_KBD_TIME_MAX)
+   dev->kbd_type = 2;
+   else
+   dev->kbd_type = 1;
+   /* Get the current keyboard backlight mode */
+   dev->kbd_mode = out[2] & SCI_KBD_MODE_MASK;
+   /* Get the current time (1-60 seconds) */
+   dev->kbd_time = out[2] >> HCI_MISC_SHIFT;
+   /* Flag as supported */
+   dev->kbd_illum_supported = 1;
 }
 
 static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
@@ -676,7 +685,10 @@ static void toshiba_eco_mode_available(struct 
toshiba_acpi_dev *dev)
status = tci_raw(dev, in, out);
if (ACPI_FAILURE(status)) {
pr_err("ACPI call to get ECO led failed\n");
-   } else if (out[0] == TOS_INPUT_DATA_ERROR) {
+   return;
+   }
+
+   if (out[0] == TOS_INPUT_DATA_ERROR) {
/*
 * If we receive 0x8300 (Input Data Error), it means that the
 * LED device is present, but that we just screwed the input
@@ -690,8 +702,11 @@ static void toshiba_eco_mode_available(struct 
toshiba_acpi_dev *dev)
status = tci_raw(dev, in, out);
if (ACPI_FAILURE(status))
pr_err("ACPI call to get ECO led failed\n");
-   else if (out[0] == TOS_SUCCESS)
-   dev->eco_supported = 1;
+
+   if (out[0] != TOS_SUCCESS)
+   return;
+
+   dev->eco_supported = 1;
}
 }
 
@@ -708,10 +723,11 @@ toshiba_eco_mode_get_status(struct led_classdev *cdev)
if (ACPI_FAILURE(status)) {
pr_err("ACPI call to get ECO led failed\n");
return LED_OFF;
-   } else if (out[0] != TOS_SUCCESS) {
-   return LED_OFF;
}
 
+   if (out[0] != TOS_SUCCESS)
+   return LED_OFF;
+
return out[2] ? LED_FULL : LED_OFF;
 }
 
@@ -745,10 +761,15 @@ static void toshiba_accelerometer_available(struct 
toshiba_acpi_dev *dev)
 * this call also serves as initialization
 */
status = tci_raw(dev, in, out);
-   if 

[PATCH 2/3] toshiba_acpi: Change error checking logic from TCI functions

[Azael Avalos]

Currently the success/error checking logic is intermixed, making the
code a bit cumbersome to understand.

This patch changes the affected functions to first check for errors
and take appropriate actions, then check for the supported features.

This patch also separates the error check from the acpi_status and
the tci_raw function call error check, as those two are completely
unrelated and were nested in if/else statements.

Signed-off-by: Azael Avalos 
---
 drivers/platform/x86/toshiba_acpi.c | 222 ++--
 1 file changed, 135 insertions(+), 87 deletions(-)

diff --git a/drivers/platform/x86/toshiba_acpi.c 
b/drivers/platform/x86/toshiba_acpi.c
index c6fc5cc..2256cf5 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -476,10 +476,15 @@ static void toshiba_illumination_available(struct 
toshiba_acpi_dev *dev)
 
status = tci_raw(dev, in, out);
sci_close(dev);
-   if (ACPI_FAILURE(status))
+   if (ACPI_FAILURE(status)) {
pr_err("ACPI call to query Illumination support failed\n");
-   else if (out[0] == TOS_SUCCESS)
-   dev->illumination_supported = 1;
+   return;
+   }
+
+   if (out[0] != TOS_SUCCESS)
+   return;
+
+   dev->illumination_supported = 1;
 }
 
 static void toshiba_illumination_set(struct led_classdev *cdev,
@@ -542,24 +547,28 @@ static void toshiba_kbd_illum_available(struct 
toshiba_acpi_dev *dev)
sci_close(dev);
if (ACPI_FAILURE(status)) {
pr_err("ACPI call to query kbd illumination support failed\n");
-   } else if (out[0] == TOS_SUCCESS) {
-   /*
-* Check for keyboard backlight timeout max value,
-* previous kbd backlight implementation set this to
-* 0x3c0003, and now the new implementation set this
-* to 0x3c001a, use this to distinguish between them.
-*/
-   if (out[3] == SCI_KBD_TIME_MAX)
-   dev->kbd_type = 2;
-   else
-   dev->kbd_type = 1;
-   /* Get the current keyboard backlight mode */
-   dev->kbd_mode = out[2] & SCI_KBD_MODE_MASK;
-   /* Get the current time (1-60 seconds) */
-   dev->kbd_time = out[2] >> HCI_MISC_SHIFT;
-   /* Flag as supported */
-   dev->kbd_illum_supported = 1;
+   return;
}
+
+   if (out[0] != TOS_SUCCESS)
+   return;
+
+   /*
+* Check for keyboard backlight timeout max value,
+* previous kbd backlight implementation set this to
+* 0x3c0003, and now the new implementation set this
+* to 0x3c001a, use this to distinguish between them.
+*/
+   if (out[3] == SCI_KBD_TIME_MAX)
+   dev->kbd_type = 2;
+   else
+   dev->kbd_type = 1;
+   /* Get the current keyboard backlight mode */
+   dev->kbd_mode = out[2] & SCI_KBD_MODE_MASK;
+   /* Get the current time (1-60 seconds) */
+   dev->kbd_time = out[2] >> HCI_MISC_SHIFT;
+   /* Flag as supported */
+   dev->kbd_illum_supported = 1;
 }
 
 static int toshiba_kbd_illum_status_set(struct toshiba_acpi_dev *dev, u32 time)
@@ -676,7 +685,10 @@ static void toshiba_eco_mode_available(struct 
toshiba_acpi_dev *dev)
status = tci_raw(dev, in, out);
if (ACPI_FAILURE(status)) {
pr_err("ACPI call to get ECO led failed\n");
-   } else if (out[0] == TOS_INPUT_DATA_ERROR) {
+   return;
+   }
+
+   if (out[0] == TOS_INPUT_DATA_ERROR) {
/*
 * If we receive 0x8300 (Input Data Error), it means that the
 * LED device is present, but that we just screwed the input
@@ -690,8 +702,11 @@ static void toshiba_eco_mode_available(struct 
toshiba_acpi_dev *dev)
status = tci_raw(dev, in, out);
if (ACPI_FAILURE(status))
pr_err("ACPI call to get ECO led failed\n");
-   else if (out[0] == TOS_SUCCESS)
-   dev->eco_supported = 1;
+
+   if (out[0] != TOS_SUCCESS)
+   return;
+
+   dev->eco_supported = 1;
}
 }
 
@@ -708,10 +723,11 @@ toshiba_eco_mode_get_status(struct led_classdev *cdev)
if (ACPI_FAILURE(status)) {
pr_err("ACPI call to get ECO led failed\n");
return LED_OFF;
-   } else if (out[0] != TOS_SUCCESS) {
-   return LED_OFF;
}
 
+   if (out[0] != TOS_SUCCESS)
+   return LED_OFF;
+
return out[2] ? LED_FULL : LED_OFF;
 }
 
@@ -745,10 +761,15 @@ static void toshiba_accelerometer_available(struct 
toshiba_acpi_dev *dev)
 * this call also serves as initialization
 */
status = tci_raw(dev, in, out);
-   if (ACPI_FAILURE(status))
+   if