Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1201099 > unrolled thread
| Started by | Darren Hart <dvhart@infradead.org> |
|---|---|
| First post | 2015-08-05 22:20 +0200 |
| Last post | 2015-08-06 00:30 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v3 2/5] toshiba_acpi: Remove "*not supported" feature prints Darren Hart <dvhart@infradead.org> - 2015-08-05 22:20 +0200
Re: [PATCH v3 2/5] toshiba_acpi: Remove "*not supported" feature prints Azael Avalos <coproscefalo@gmail.com> - 2015-08-06 00:20 +0200
Re: [PATCH v3 2/5] toshiba_acpi: Remove "*not supported" feature prints Darren Hart <dvhart@infradead.org> - 2015-08-06 00:30 +0200
| From | Darren Hart <dvhart@infradead.org> |
|---|---|
| Date | 2015-08-05 22:20 +0200 |
| Subject | Re: [PATCH v3 2/5] toshiba_acpi: Remove "*not supported" feature prints |
| Message-ID | <pUdkm-3qW-21@gated-at.bofh.it> |
On Fri, Jul 31, 2015 at 09:58:13PM -0600, Azael Avalos wrote:
> Currently the driver prints "*not supported" if any of the features
> queried are in fact not supported, let us print the available
> features instead.
>
> This patch removes all instances pr_info printing "*not supported",
> and add a new function called "print_supported_features", which will
> print the available laptop features.
>
> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
> ---
> drivers/platform/x86/toshiba_acpi.c | 72 +++++++++++++++++++++++--------------
> 1 file changed, 46 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index d983dc4..66b596a 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -459,7 +459,7 @@ static void toshiba_illumination_available(struct toshiba_acpi_dev *dev)
> if (ACPI_FAILURE(status))
> pr_err("ACPI call to query Illumination support failed\n");
> else if (out[0] == TOS_NOT_SUPPORTED)
> - pr_info("Illumination device not available\n");
> + return;
> else if (out[0] == TOS_SUCCESS)
> dev->illumination_supported = 1;
> }
> @@ -483,7 +483,6 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
> pr_err("ACPI call for illumination failed\n");
> return;
> } else if (result == TOS_NOT_SUPPORTED) {
> - pr_info("Illumination not supported\n");
> return;
> }
I mentioned this in the previous review. For several of these, we have an if
statement that checks for a condition, and then returns, which is exactly what
would happen if we didn't have the if statement at all.
If the context is important, a comment should be sufficient. Is there a
compelling reason to add the redundant check?
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Azael Avalos <coproscefalo@gmail.com> |
|---|---|
| Date | 2015-08-06 00:20 +0200 |
| Subject | Re: [PATCH v3 2/5] toshiba_acpi: Remove "*not supported" feature prints |
| Message-ID | <pUfcv-69Y-67@gated-at.bofh.it> |
| In reply to | #1201099 |
Hi Darren,
2015-08-05 3:38 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> On Fri, Jul 31, 2015 at 09:58:13PM -0600, Azael Avalos wrote:
>> Currently the driver prints "*not supported" if any of the features
>> queried are in fact not supported, let us print the available
>> features instead.
>>
>> This patch removes all instances pr_info printing "*not supported",
>> and add a new function called "print_supported_features", which will
>> print the available laptop features.
>>
>> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
>> ---
>> drivers/platform/x86/toshiba_acpi.c | 72 +++++++++++++++++++++++--------------
>> 1 file changed, 46 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
>> index d983dc4..66b596a 100644
>> --- a/drivers/platform/x86/toshiba_acpi.c
>> +++ b/drivers/platform/x86/toshiba_acpi.c
>> @@ -459,7 +459,7 @@ static void toshiba_illumination_available(struct toshiba_acpi_dev *dev)
>> if (ACPI_FAILURE(status))
>> pr_err("ACPI call to query Illumination support failed\n");
>> else if (out[0] == TOS_NOT_SUPPORTED)
>> - pr_info("Illumination device not available\n");
>> + return;
>> else if (out[0] == TOS_SUCCESS)
>> dev->illumination_supported = 1;
>> }
>> @@ -483,7 +483,6 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
>> pr_err("ACPI call for illumination failed\n");
>> return;
>> } else if (result == TOS_NOT_SUPPORTED) {
>> - pr_info("Illumination not supported\n");
>> return;
>> }
>
> I mentioned this in the previous review. For several of these, we have an if
> statement that checks for a condition, and then returns, which is exactly what
> would happen if we didn't have the if statement at all.
>
> If the context is important, a comment should be sufficient. Is there a
> compelling reason to add the redundant check?
The "offending" lines are removed by patch 04, that's why I didn't included
a comment or removed the lines on this patch, as I was trying to "abstract"
what each patch do, which in this patch, only removes the pr_info.
>
> --
> Darren Hart
> Intel Open Source Technology Center
Cheers
Azael
--
-- El mundo apesta y vosotros apestais tambien --
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Darren Hart <dvhart@infradead.org> |
|---|---|
| Date | 2015-08-06 00:30 +0200 |
| Message-ID | <pUfmb-6lp-47@gated-at.bofh.it> |
| In reply to | #1201279 |
On Wed, Aug 05, 2015 at 04:15:13PM -0600, Azael Avalos wrote:
> Hi Darren,
>
> 2015-08-05 3:38 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> > On Fri, Jul 31, 2015 at 09:58:13PM -0600, Azael Avalos wrote:
> >> Currently the driver prints "*not supported" if any of the features
> >> queried are in fact not supported, let us print the available
> >> features instead.
> >>
> >> This patch removes all instances pr_info printing "*not supported",
> >> and add a new function called "print_supported_features", which will
> >> print the available laptop features.
> >>
> >> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
> >> ---
> >> drivers/platform/x86/toshiba_acpi.c | 72 +++++++++++++++++++++++--------------
> >> 1 file changed, 46 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> >> index d983dc4..66b596a 100644
> >> --- a/drivers/platform/x86/toshiba_acpi.c
> >> +++ b/drivers/platform/x86/toshiba_acpi.c
> >> @@ -459,7 +459,7 @@ static void toshiba_illumination_available(struct toshiba_acpi_dev *dev)
> >> if (ACPI_FAILURE(status))
> >> pr_err("ACPI call to query Illumination support failed\n");
> >> else if (out[0] == TOS_NOT_SUPPORTED)
> >> - pr_info("Illumination device not available\n");
> >> + return;
> >> else if (out[0] == TOS_SUCCESS)
> >> dev->illumination_supported = 1;
> >> }
> >> @@ -483,7 +483,6 @@ static void toshiba_illumination_set(struct led_classdev *cdev,
> >> pr_err("ACPI call for illumination failed\n");
> >> return;
> >> } else if (result == TOS_NOT_SUPPORTED) {
> >> - pr_info("Illumination not supported\n");
> >> return;
> >> }
> >
> > I mentioned this in the previous review. For several of these, we have an if
> > statement that checks for a condition, and then returns, which is exactly what
> > would happen if we didn't have the if statement at all.
> >
> > If the context is important, a comment should be sufficient. Is there a
> > compelling reason to add the redundant check?
>
> The "offending" lines are removed by patch 04, that's why I didn't included
> a comment or removed the lines on this patch, as I was trying to "abstract"
> what each patch do, which in this patch, only removes the pr_info.
Apologies, I missed that. OK, we're good on this one.
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web