* [PATCH 0/3] ath6kl: Adjustments for ath6kl_sdio_power_on()
@ 2024-09-21 12:00 Markus Elfring
2024-09-21 12:02 ` [PATCH 1/3] ath6kl: Use sdio_release_host(func) call only once in ath6kl_sdio_power_on() Markus Elfring
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Markus Elfring @ 2024-09-21 12:00 UTC (permalink / raw)
To: linux-wireless, James Minor, Kalle Valo, Krzysztof Kozlowski
Cc: LKML, Steve deRosier, Julia Lawall
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 21 Sep 2024 13:43:21 +0200
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (3):
Use sdio_release_host(func) call only once
Omit a label
Reduce scopes for two variables
drivers/net/wireless/ath/ath6kl/sdio.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
--
2.46.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/3] ath6kl: Use sdio_release_host(func) call only once in ath6kl_sdio_power_on()
2024-09-21 12:00 [PATCH 0/3] ath6kl: Adjustments for ath6kl_sdio_power_on() Markus Elfring
@ 2024-09-21 12:02 ` Markus Elfring
2024-09-21 12:04 ` [PATCH 2/3] ath6kl: Omit a label " Markus Elfring
2024-09-21 12:06 ` [PATCH 3/3] ath6kl: Reduce scopes for two variables " Markus Elfring
2 siblings, 0 replies; 7+ messages in thread
From: Markus Elfring @ 2024-09-21 12:02 UTC (permalink / raw)
To: linux-wireless, James Minor, Kalle Valo, Krzysztof Kozlowski
Cc: LKML, Steve deRosier, Julia Lawall
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 21 Sep 2024 12:00:19 +0200
A sdio_release_host(func) call was immediately used after a return code
check for a sdio_enable_func() call in this function implementation.
Thus use such a function call only once instead directly before the check.
This issue was transformed by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/net/wireless/ath/ath6kl/sdio.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c b/drivers/net/wireless/ath/ath6kl/sdio.c
index 9ab091044706..5106c6909dc8 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -514,14 +514,12 @@ static int ath6kl_sdio_power_on(struct ath6kl *ar)
sdio_claim_host(func);
ret = sdio_enable_func(func);
+ sdio_release_host(func);
if (ret) {
ath6kl_err("Unable to enable sdio func: %d)\n", ret);
- sdio_release_host(func);
return ret;
}
- sdio_release_host(func);
-
/*
* Wait for hardware to initialise. It should take a lot less than
* 10 ms but let's be conservative here.
--
2.46.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] ath6kl: Omit a label in ath6kl_sdio_power_on()
2024-09-21 12:00 [PATCH 0/3] ath6kl: Adjustments for ath6kl_sdio_power_on() Markus Elfring
2024-09-21 12:02 ` [PATCH 1/3] ath6kl: Use sdio_release_host(func) call only once in ath6kl_sdio_power_on() Markus Elfring
@ 2024-09-21 12:04 ` Markus Elfring
2024-09-21 12:06 ` [PATCH 3/3] ath6kl: Reduce scopes for two variables " Markus Elfring
2 siblings, 0 replies; 7+ messages in thread
From: Markus Elfring @ 2024-09-21 12:04 UTC (permalink / raw)
To: linux-wireless, James Minor, Kalle Valo, Krzysztof Kozlowski
Cc: LKML, Steve deRosier, Julia Lawall
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 21 Sep 2024 13:04:19 +0200
Delete the label “out” so that this function implementation becomes
a bit more succinct.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/net/wireless/ath/ath6kl/sdio.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c b/drivers/net/wireless/ath/ath6kl/sdio.c
index 5106c6909dc8..e4d15cc9b36c 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -529,13 +529,11 @@ static int ath6kl_sdio_power_on(struct ath6kl *ar)
ret = ath6kl_sdio_config(ar);
if (ret) {
ath6kl_err("Failed to config sdio: %d\n", ret);
- goto out;
+ return ret;
}
ar_sdio->is_disabled = false;
-
-out:
- return ret;
+ return 0;
}
static int ath6kl_sdio_power_off(struct ath6kl *ar)
--
2.46.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] ath6kl: Reduce scopes for two variables in ath6kl_sdio_power_on()
2024-09-21 12:00 [PATCH 0/3] ath6kl: Adjustments for ath6kl_sdio_power_on() Markus Elfring
2024-09-21 12:02 ` [PATCH 1/3] ath6kl: Use sdio_release_host(func) call only once in ath6kl_sdio_power_on() Markus Elfring
2024-09-21 12:04 ` [PATCH 2/3] ath6kl: Omit a label " Markus Elfring
@ 2024-09-21 12:06 ` Markus Elfring
2024-09-23 16:41 ` Jeff Johnson
2 siblings, 1 reply; 7+ messages in thread
From: Markus Elfring @ 2024-09-21 12:06 UTC (permalink / raw)
To: linux-wireless, James Minor, Kalle Valo, Krzysztof Kozlowski
Cc: LKML, Steve deRosier, Julia Lawall
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 21 Sep 2024 13:30:48 +0200
Adjust the definitions for the local variables "func" and "ret"
so that the corresponding setting will be performed a bit later.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/net/wireless/ath/ath6kl/sdio.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c b/drivers/net/wireless/ath/ath6kl/sdio.c
index e4d15cc9b36c..689f83f6bce5 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -503,17 +503,15 @@ static void ath6kl_sdio_irq_handler(struct sdio_func *func)
static int ath6kl_sdio_power_on(struct ath6kl *ar)
{
struct ath6kl_sdio *ar_sdio = ath6kl_sdio_priv(ar);
- struct sdio_func *func = ar_sdio->func;
- int ret = 0;
if (!ar_sdio->is_disabled)
return 0;
ath6kl_dbg(ATH6KL_DBG_BOOT, "sdio power on\n");
-
+ struct sdio_func *func = ar_sdio->func;
sdio_claim_host(func);
- ret = sdio_enable_func(func);
+ int ret = sdio_enable_func(func);
sdio_release_host(func);
if (ret) {
ath6kl_err("Unable to enable sdio func: %d)\n", ret);
--
2.46.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] ath6kl: Reduce scopes for two variables in ath6kl_sdio_power_on()
2024-09-21 12:06 ` [PATCH 3/3] ath6kl: Reduce scopes for two variables " Markus Elfring
@ 2024-09-23 16:41 ` Jeff Johnson
2024-09-23 17:00 ` Markus Elfring
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Johnson @ 2024-09-23 16:41 UTC (permalink / raw)
To: Markus Elfring, linux-wireless, James Minor, Kalle Valo,
Krzysztof Kozlowski
Cc: LKML, Steve deRosier, Julia Lawall
On 9/21/2024 5:06 AM, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 21 Sep 2024 13:30:48 +0200
>
> Adjust the definitions for the local variables "func" and "ret"
> so that the corresponding setting will be performed a bit later.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/net/wireless/ath/ath6kl/sdio.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c b/drivers/net/wireless/ath/ath6kl/sdio.c
> index e4d15cc9b36c..689f83f6bce5 100644
> --- a/drivers/net/wireless/ath/ath6kl/sdio.c
> +++ b/drivers/net/wireless/ath/ath6kl/sdio.c
> @@ -503,17 +503,15 @@ static void ath6kl_sdio_irq_handler(struct sdio_func *func)
> static int ath6kl_sdio_power_on(struct ath6kl *ar)
> {
> struct ath6kl_sdio *ar_sdio = ath6kl_sdio_priv(ar);
> - struct sdio_func *func = ar_sdio->func;
> - int ret = 0;
>
> if (!ar_sdio->is_disabled)
> return 0;
>
> ath6kl_dbg(ATH6KL_DBG_BOOT, "sdio power on\n");
> -
> + struct sdio_func *func = ar_sdio->func;
> sdio_claim_host(func);
>
> - ret = sdio_enable_func(func);
> + int ret = sdio_enable_func(func);
> sdio_release_host(func);
> if (ret) {
> ath6kl_err("Unable to enable sdio func: %d)\n", ret);
> --
> 2.46.0
NAK
no maintainer wants to spend time on patches like this which bring no real
value to code that is not actively being maintained, and which violates the
established understanding that, except under certain recently established
criteria, declarations and code should not be interleaved.
please avoid sending any patches to ath* drivers that do not fix bugs.
/jeff
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] ath6kl: Reduce scopes for two variables in ath6kl_sdio_power_on()
2024-09-23 16:41 ` Jeff Johnson
@ 2024-09-23 17:00 ` Markus Elfring
2024-09-23 18:21 ` Jeff Johnson
0 siblings, 1 reply; 7+ messages in thread
From: Markus Elfring @ 2024-09-23 17:00 UTC (permalink / raw)
To: Jeff Johnson, linux-wireless, Kalle Valo, Krzysztof Kozlowski
Cc: LKML, Julia Lawall, Peter Zijlstra
>> Adjust the definitions for the local variables "func" and "ret"
>> so that the corresponding setting will be performed a bit later.
…
>> +++ b/drivers/net/wireless/ath/ath6kl/sdio.c
>> @@ -503,17 +503,15 @@ static void ath6kl_sdio_irq_handler(struct sdio_func *func)
>> static int ath6kl_sdio_power_on(struct ath6kl *ar)
>> {
>> struct ath6kl_sdio *ar_sdio = ath6kl_sdio_priv(ar);
>> - struct sdio_func *func = ar_sdio->func;
>> - int ret = 0;
>>
>> if (!ar_sdio->is_disabled)
>> return 0;
>>
>> ath6kl_dbg(ATH6KL_DBG_BOOT, "sdio power on\n");
>> -
>> + struct sdio_func *func = ar_sdio->func;
>> sdio_claim_host(func);
>>
>> - ret = sdio_enable_func(func);
>> + int ret = sdio_enable_func(func);
>> sdio_release_host(func);
>> if (ret) {
>> ath6kl_err("Unable to enable sdio func: %d)\n", ret);
>> --
>> 2.46.0
>
> NAK
>
> no maintainer wants to spend time on patches like this which bring no real
> value to code that is not actively being maintained, and which violates the
> established understanding that, except under certain recently established
> criteria, declarations and code should not be interleaved.
…
Would you find other software design options more acceptable for further
collateral evolution?
1. Additional compound statements (by using extra curly brackets)
2. Moving a bit of source code into an additional function implementation
Regards,
Markus
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 3/3] ath6kl: Reduce scopes for two variables in ath6kl_sdio_power_on()
2024-09-23 17:00 ` Markus Elfring
@ 2024-09-23 18:21 ` Jeff Johnson
0 siblings, 0 replies; 7+ messages in thread
From: Jeff Johnson @ 2024-09-23 18:21 UTC (permalink / raw)
To: Markus Elfring, linux-wireless, Kalle Valo, Krzysztof Kozlowski
Cc: LKML, Julia Lawall, Peter Zijlstra
On 9/23/2024 10:00 AM, Markus Elfring wrote:
>>> Adjust the definitions for the local variables "func" and "ret"
>>> so that the corresponding setting will be performed a bit later.
> …
>>> +++ b/drivers/net/wireless/ath/ath6kl/sdio.c
>>> @@ -503,17 +503,15 @@ static void ath6kl_sdio_irq_handler(struct sdio_func *func)
>>> static int ath6kl_sdio_power_on(struct ath6kl *ar)
>>> {
>>> struct ath6kl_sdio *ar_sdio = ath6kl_sdio_priv(ar);
>>> - struct sdio_func *func = ar_sdio->func;
>>> - int ret = 0;
>>>
>>> if (!ar_sdio->is_disabled)
>>> return 0;
>>>
>>> ath6kl_dbg(ATH6KL_DBG_BOOT, "sdio power on\n");
>>> -
>>> + struct sdio_func *func = ar_sdio->func;
>>> sdio_claim_host(func);
>>>
>>> - ret = sdio_enable_func(func);
>>> + int ret = sdio_enable_func(func);
>>> sdio_release_host(func);
>>> if (ret) {
>>> ath6kl_err("Unable to enable sdio func: %d)\n", ret);
>>> --
>>> 2.46.0
>>
>> NAK
>>
>> no maintainer wants to spend time on patches like this which bring no real
>> value to code that is not actively being maintained, and which violates the
>> established understanding that, except under certain recently established
>> criteria, declarations and code should not be interleaved.
> …
>
> Would you find other software design options more acceptable for further
> collateral evolution?
>
> 1. Additional compound statements (by using extra curly brackets)
>
> 2. Moving a bit of source code into an additional function implementation
I cannot speak for other maintainers, only myself. For myself I am far more
interested in changes which fix actual errors or warnings, and far more
interested in changes to code that is actually being widely used and where
active development is occurring, as opposed to drivers that have no ongoing
development and little deployment. At this time the ath.git maintainers are
100% focused on the ath12k MLO feature, and anything other than bug fixes to
ath12k or other ath drivers will be ignored.
/jeff
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-09-23 18:22 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-21 12:00 [PATCH 0/3] ath6kl: Adjustments for ath6kl_sdio_power_on() Markus Elfring
2024-09-21 12:02 ` [PATCH 1/3] ath6kl: Use sdio_release_host(func) call only once in ath6kl_sdio_power_on() Markus Elfring
2024-09-21 12:04 ` [PATCH 2/3] ath6kl: Omit a label " Markus Elfring
2024-09-21 12:06 ` [PATCH 3/3] ath6kl: Reduce scopes for two variables " Markus Elfring
2024-09-23 16:41 ` Jeff Johnson
2024-09-23 17:00 ` Markus Elfring
2024-09-23 18:21 ` Jeff Johnson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).