[v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()

Commit Message

Bartosz Golaszewski April 24, 2024, 12:29 p.m. UTC

From: Bartosz Golaszewski <[e-mail]>

Any return value from gpiod_get_optional() other than a pointer to a
GPIO descriptor or a NULL-pointer is an error and the driver should
abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
power_ctrl_enabled on NULL-pointer returned by
devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
While at it: also bail-out on error returned when trying to get the
"swctrl" GPIO.

Reported-by: Wren Turkal <[e-mail]>
Reported-by: Zijun Hu <[e-mail]>
Closes: https://lore.kernel.org/linux-bluetooth/[e-mail]/
Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by: Krzysztof Kozlowski <[e-mail]>
Signed-off-by: Bartosz Golaszewski <[e-mail]>
---
v1 -> v2:
- also restore the previous behavior for QCA6390 and other models that
  fall under the default: label in the affected switch case
- bail-out on errors when getting the swctrl GPIO too

 drivers/bluetooth/hci_qca.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

quic_zijuhu April 24, 2024, 12:35 p.m. UTC | #1

On 4/24/2024 8:29 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[e-mail]>
> 
> Any return value from gpiod_get_optional() other than a pointer to a
> GPIO descriptor or a NULL-pointer is an error and the driver should
> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> power_ctrl_enabled on NULL-pointer returned by
> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> While at it: also bail-out on error returned when trying to get the
> "swctrl" GPIO.
> 
> Reported-by: Wren Turkal <[e-mail]>
> Reported-by: Zijun Hu <[e-mail]>
> Closes: https://lore.kernel.org/linux-bluetooth/[e-mail]/
> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> Reviewed-by: Krzysztof Kozlowski <[e-mail]>
> Signed-off-by: Bartosz Golaszewski <[e-mail]>
> ---
> v1 -> v2:
> - also restore the previous behavior for QCA6390 and other models that
>   fall under the default: label in the affected switch case
> - bail-out on errors when getting the swctrl GPIO too
> 
>  drivers/bluetooth/hci_qca.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 92fa20f5ac7d..0e98ad2c0c9d 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2327,16 +2327,21 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		    (data->soc_type == QCA_WCN6750 ||
>  		     data->soc_type == QCA_WCN6855)) {
>  			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
> -			power_ctrl_enabled = false;
> +			return PTR_ERR(qcadev->bt_en);
>  		}
>  
> +		if (!qcadev->bt_en)
> +			power_ctrl_enabled = false;
> +
>  		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
>  					       GPIOD_IN);
>  		if (IS_ERR(qcadev->sw_ctrl) &&
>  		    (data->soc_type == QCA_WCN6750 ||
>  		     data->soc_type == QCA_WCN6855 ||
> -		     data->soc_type == QCA_WCN7850))
> -			dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
> +		     data->soc_type == QCA_WCN7850)) {
> +			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
> +			return PTR_ERR(qcadev->sw_ctrl);
> +		}
>  
>  		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>  		if (IS_ERR(qcadev->susclk)) {
> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>  					       GPIOD_OUT_LOW);
>  		if (IS_ERR(qcadev->bt_en)) {
> -			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> -			power_ctrl_enabled = false;
> +			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
> +			return PTR_ERR(qcadev->bt_en);
>  		}
>  
> +		if (!qcadev->bt_en)
> +			power_ctrl_enabled = false;
> +
>  		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>  		if (IS_ERR(qcadev->susclk)) {
>  			dev_warn(&serdev->dev, "failed to acquire clk\n");NAK, you don't answer my question for your v1 before send v2

quic_zijuhu April 24, 2024, 12:59 p.m. UTC | #2

On 4/24/2024 8:29 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[e-mail]>
> 
> Any return value from gpiod_get_optional() other than a pointer to a
> GPIO descriptor or a NULL-pointer is an error and the driver should
> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> power_ctrl_enabled on NULL-pointer returned by
> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> While at it: also bail-out on error returned when trying to get the
> "swctrl" GPIO.
> 
> Reported-by: Wren Turkal <[e-mail]>
> Reported-by: Zijun Hu <[e-mail]>
> Closes: https://lore.kernel.org/linux-bluetooth/[e-mail]/
> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> Reviewed-by: Krzysztof Kozlowski <[e-mail]>
is it really reviewed-by Krzysztof?
suggest reviewer give explicit review-by tag by public way, then you add
this tag.
> Signed-off-by: Bartosz Golaszewski <[e-mail]>
> ---
> v1 -> v2:
> - also restore the previous behavior for QCA6390 and other models that
>   fall under the default: label in the affected switch case
> - bail-out on errors when getting the swctrl GPIO too
> 
>  drivers/bluetooth/hci_qca.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 92fa20f5ac7d..0e98ad2c0c9d 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2327,16 +2327,21 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		    (data->soc_type == QCA_WCN6750 ||
>  		     data->soc_type == QCA_WCN6855)) {
>  			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
> -			power_ctrl_enabled = false;
> +
think about what will happen for present lunched products if this type
error really happens.
BT don't work at all with your change. BT can be used mostly without
your change.
			return PTR_ERR(qcadev->bt_en);
>  		}
>  
> +		if (!qcadev->bt_en)
> +			power_ctrl_enabled = false;
> +
you don't answer me how to treat a required enable is not configured by user
>  		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
>  					       GPIOD_IN);
>  		if (IS_ERR(qcadev->sw_ctrl) &&
>  		    (data->soc_type == QCA_WCN6750 ||
>  		     data->soc_type == QCA_WCN6855 ||
> -		     data->soc_type == QCA_WCN7850))
> -			dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
> +		     data->soc_type == QCA_WCN7850)) {
> +			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
> +			return PTR_ERR(qcadev->sw_ctrl);have the same question as above.
> +		}
>  
>  		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>  		if (IS_ERR(qcadev->susclk)) {
> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>  					       GPIOD_OUT_LOW);
>  		if (IS_ERR(qcadev->bt_en)) {
> -			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> -			power_ctrl_enabled = false;
> +			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
> +			return PTR_ERR(qcadev->bt_en);
>  		}
> 
have the same question as above.
is it right for such prompt ?
> +		if (!qcadev->bt_en)
> +			power_ctrl_enabled = false;
> +
>  		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>  		if (IS_ERR(qcadev->susclk)) {
>  			dev_warn(&serdev->dev, "failed to acquire clk\n");
have the same question as above.

how do you known the root cause of the issue reported without my earlier
debugging and fix?

do my fix regarding the issue i concerned have any  fault?

NAK by me.

Wren Turkal April 24, 2024, 1:10 p.m. UTC | #3

On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski<[e-mail]>
> 
> Any return value from gpiod_get_optional() other than a pointer to a
> GPIO descriptor or a NULL-pointer is an error and the driver should
> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> power_ctrl_enabled on NULL-pointer returned by
> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> While at it: also bail-out on error returned when trying to get the
> "swctrl" GPIO.
> 
> Reported-by: Wren Turkal<[e-mail]>
> Reported-by: Zijun Hu<[e-mail]>
> Closes:https://lore.kernel.org/linux-bluetooth/[e-mail]/
> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> Reviewed-by: Krzysztof Kozlowski<[e-mail]>
> Signed-off-by: Bartosz Golaszewski<[e-mail]>

Tested-by: "Wren Turkal" <[e-mail]>


Like this?
wt

Wren Turkal April 24, 2024, 1:15 p.m. UTC | #4

On 4/24/24 5:59 AM, quic_zijuhu wrote:
> On 4/24/2024 8:29 PM, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <[e-mail]>
>>
>> Any return value from gpiod_get_optional() other than a pointer to a
>> GPIO descriptor or a NULL-pointer is an error and the driver should
>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
>> power_ctrl_enabled on NULL-pointer returned by
>> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
>> While at it: also bail-out on error returned when trying to get the
>> "swctrl" GPIO.
>>
>> Reported-by: Wren Turkal <[e-mail]>
>> Reported-by: Zijun Hu <[e-mail]>
>> Closes: https://lore.kernel.org/linux-bluetooth/[e-mail]/
>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>> Reviewed-by: Krzysztof Kozlowski <[e-mail]>
> is it really reviewed-by Krzysztof?
> suggest reviewer give explicit review-by tag by public way, then you add
> this tag.
>> Signed-off-by: Bartosz Golaszewski <[e-mail]>
>> ---
>> v1 -> v2:
>> - also restore the previous behavior for QCA6390 and other models that
>>    fall under the default: label in the affected switch case
>> - bail-out on errors when getting the swctrl GPIO too
>>
>>   drivers/bluetooth/hci_qca.c | 18 +++++++++++++-----
>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 92fa20f5ac7d..0e98ad2c0c9d 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -2327,16 +2327,21 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>   		    (data->soc_type == QCA_WCN6750 ||
>>   		     data->soc_type == QCA_WCN6855)) {
>>   			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>> -			power_ctrl_enabled = false;
>> +
> think about what will happen for present lunched products if this type
> error really happens.
> BT don't work at all with your change. BT can be used mostly without
> your change.
> 			return PTR_ERR(qcadev->bt_en);
>>   		}
>>   
>> +		if (!qcadev->bt_en)
>> +			power_ctrl_enabled = false;
>> +
> you don't answer me how to treat a required enable is not configured by user
>>   		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
>>   					       GPIOD_IN);
>>   		if (IS_ERR(qcadev->sw_ctrl) &&
>>   		    (data->soc_type == QCA_WCN6750 ||
>>   		     data->soc_type == QCA_WCN6855 ||
>> -		     data->soc_type == QCA_WCN7850))
>> -			dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>> +		     data->soc_type == QCA_WCN7850)) {
>> +			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>> +			return PTR_ERR(qcadev->sw_ctrl);have the same question as above.
>> +		}
>>   
>>   		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>>   		if (IS_ERR(qcadev->susclk)) {
>> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>   		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>   					       GPIOD_OUT_LOW);
>>   		if (IS_ERR(qcadev->bt_en)) {
>> -			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>> -			power_ctrl_enabled = false;
>> +			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
>> +			return PTR_ERR(qcadev->bt_en);
>>   		}
>>
> have the same question as above.
> is it right for such prompt ?
>> +		if (!qcadev->bt_en)
>> +			power_ctrl_enabled = false;
>> +
>>   		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>>   		if (IS_ERR(qcadev->susclk)) {
>>   			dev_warn(&serdev->dev, "failed to acquire clk\n");
> have the same question as above.
> 
> how do you known the root cause of the issue reported without my earlier
> debugging and fix?

Without your debugging, this fix would not have been possible.

> 
> do my fix regarding the issue i concerned have any  fault?
> 
> NAK by me.
> 
> 
> 
>

[e-mail] April 24, 2024, 1:16 p.m. UTC | #5

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=847442

---Test result---

Test Summary:
CheckPatch                    FAIL      0.94 seconds
GitLint                       FAIL      0.51 seconds
SubjectPrefix                 PASS      0.10 seconds
BuildKernel                   PASS      29.92 seconds
CheckAllWarning               PASS      32.99 seconds
CheckSparse                   PASS      39.01 seconds
CheckSmatch                   FAIL      36.66 seconds
BuildKernel32                 PASS      29.51 seconds
TestRunnerSetup               PASS      537.50 seconds
TestRunner_l2cap-tester       PASS      21.34 seconds
TestRunner_iso-tester         PASS      31.05 seconds
TestRunner_bnep-tester        PASS      4.70 seconds
TestRunner_mgmt-tester        FAIL      113.32 seconds
TestRunner_rfcomm-tester      PASS      7.47 seconds
TestRunner_sco-tester         PASS      15.20 seconds
TestRunner_ioctl-tester       PASS      7.94 seconds
TestRunner_mesh-tester        PASS      5.88 seconds
TestRunner_smp-tester         PASS      7.04 seconds
TestRunner_userchan-tester    PASS      5.00 seconds
IncrementalBuild              PASS      29.56 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()
WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report
#108: 
Reported-by: Wren Turkal <[e-mail]>
Reported-by: Zijun Hu <[e-mail]>

total: 0 errors, 1 warnings, 39 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13641795.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()

WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (84>80): "[v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()"
16: B1 Line exceeds max length (106>80): "Closes: https://lore.kernel.org/linux-bluetooth/[e-mail]/"
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bpa10x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bpa10x.o'
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 492, Passed: 489 (99.4%), Failed: 1, Not Run: 2

Failed Test Cases
LL Privacy - Remove Device 4 (Disable Adv)           Timed out    2.174 seconds


---
Regards,
Linux Bluetooth

Bartosz Golaszewski April 24, 2024, 1:18 p.m. UTC | #6

On Wed, 24 Apr 2024 at 15:10, Wren Turkal <[e-mail]> wrote:
>
> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski<[e-mail]>
> >
> > Any return value from gpiod_get_optional() other than a pointer to a
> > GPIO descriptor or a NULL-pointer is an error and the driver should
> > abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> > don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> > power_ctrl_enabled on NULL-pointer returned by
> > devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> > While at it: also bail-out on error returned when trying to get the
> > "swctrl" GPIO.
> >
> > Reported-by: Wren Turkal<[e-mail]>
> > Reported-by: Zijun Hu<[e-mail]>
> > Closes:https://lore.kernel.org/linux-bluetooth/[e-mail]/
> > Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> > Reviewed-by: Krzysztof Kozlowski<[e-mail]>
> > Signed-off-by: Bartosz Golaszewski<[e-mail]>
>
> Tested-by: "Wren Turkal" <[e-mail]>
>
>
> Like this?

Yes, awesome, thanks.

This is how reviewing works too in the kernel, look at what Krzysztof
did under v1, he just wrote:

Reviewed-by: Krzysztof Kozlowski<[e-mail]>

And mailing list tools will pick it up.

Bartosz

quic_zijuhu April 24, 2024, 1:22 p.m. UTC | #7

On 4/24/2024 9:18 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 15:10, Wren Turkal <[e-mail]> wrote:
>>
>> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski<[e-mail]>
>>>
>>> Any return value from gpiod_get_optional() other than a pointer to a
>>> GPIO descriptor or a NULL-pointer is an error and the driver should
>>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
>>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
>>> power_ctrl_enabled on NULL-pointer returned by
>>> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
>>> While at it: also bail-out on error returned when trying to get the
>>> "swctrl" GPIO.
>>>
>>> Reported-by: Wren Turkal<[e-mail]>
>>> Reported-by: Zijun Hu<[e-mail]>
>>> Closes:https://lore.kernel.org/linux-bluetooth/[e-mail]/
>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>>> Reviewed-by: Krzysztof Kozlowski<[e-mail]>
>>> Signed-off-by: Bartosz Golaszewski<[e-mail]>
>>
>> Tested-by: "Wren Turkal" <[e-mail]>
>>
>>
>> Like this?
> 
> Yes, awesome, thanks.
> 
> This is how reviewing works too in the kernel, look at what Krzysztof
> did under v1, he just wrote:
> 
> Reviewed-by: Krzysztof Kozlowski<[e-mail]>
> 
v1 have obvious something wrong as i pointed and verified.
so i think it is not suitable to attach v1's review-by tag to v2 anyway.

> And mailing list tools will pick it up.
> 
> Bartosz

Wren Turkal April 24, 2024, 1:31 p.m. UTC | #8

On 4/24/24 6:22 AM, quic_zijuhu wrote:
> On 4/24/2024 9:18 PM, Bartosz Golaszewski wrote:
>> On Wed, 24 Apr 2024 at 15:10, Wren Turkal <[e-mail]> wrote:
>>>
>>> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
>>>> From: Bartosz Golaszewski<[e-mail]>
>>>>
>>>> Any return value from gpiod_get_optional() other than a pointer to a
>>>> GPIO descriptor or a NULL-pointer is an error and the driver should
>>>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
>>>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
>>>> power_ctrl_enabled on NULL-pointer returned by
>>>> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
>>>> While at it: also bail-out on error returned when trying to get the
>>>> "swctrl" GPIO.
>>>>
>>>> Reported-by: Wren Turkal<[e-mail]>
>>>> Reported-by: Zijun Hu<[e-mail]>
>>>> Closes:https://lore.kernel.org/linux-bluetooth/[e-mail]/
>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>>>> Reviewed-by: Krzysztof Kozlowski<[e-mail]>
>>>> Signed-off-by: Bartosz Golaszewski<[e-mail]>
>>>
>>> Tested-by: "Wren Turkal" <[e-mail]>
>>>
>>>
>>> Like this?
>>
>> Yes, awesome, thanks.
>>
>> This is how reviewing works too in the kernel, look at what Krzysztof
>> did under v1, he just wrote:
>>
>> Reviewed-by: Krzysztof Kozlowski<[e-mail]>
>>
> v1 have obvious something wrong as i pointed and verified.
> so i think it is not suitable to attach v1's review-by tag to v2 anyway.

@Zijun, your concern is that current DTs may not define the gpio and 
this will cause the bluetooth not to work?

Would that not more appropriately be fixed by machine-specific fixups 
for the DT?

> 
>> And mailing list tools will pick it up.
>>
>> Bartosz
>

quic_zijuhu April 24, 2024, 1:36 p.m. UTC | #9

On 4/24/2024 9:31 PM, Wren Turkal wrote:
> On 4/24/24 6:22 AM, quic_zijuhu wrote:
>> On 4/24/2024 9:18 PM, Bartosz Golaszewski wrote:
>>> On Wed, 24 Apr 2024 at 15:10, Wren Turkal <[e-mail]> wrote:
>>>>
>>>> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
>>>>> From: Bartosz Golaszewski<[e-mail]>
>>>>>
>>>>> Any return value from gpiod_get_optional() other than a pointer to a
>>>>> GPIO descriptor or a NULL-pointer is an error and the driver should
>>>>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth:
>>>>> hci_qca:
>>>>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
>>>>> power_ctrl_enabled on NULL-pointer returned by
>>>>> devm_gpiod_get_optional(). Restore this behavior but bail-out on
>>>>> errors.
>>>>> While at it: also bail-out on error returned when trying to get the
>>>>> "swctrl" GPIO.
>>>>>
>>>>> Reported-by: Wren Turkal<[e-mail]>
>>>>> Reported-by: Zijun Hu<[e-mail]>
>>>>> Closes:https://lore.kernel.org/linux-bluetooth/[e-mail]/
>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use
>>>>> IS_ERR_OR_NULL() with gpiod_get_optional()")
>>>>> Reviewed-by: Krzysztof Kozlowski<[e-mail]>
>>>>> Signed-off-by: Bartosz Golaszewski<[e-mail]>
>>>>
>>>> Tested-by: "Wren Turkal" <[e-mail]>
>>>>
>>>>
>>>> Like this?
>>>
>>> Yes, awesome, thanks.
>>>
>>> This is how reviewing works too in the kernel, look at what Krzysztof
>>> did under v1, he just wrote:
>>>
>>> Reviewed-by: Krzysztof Kozlowski<[e-mail]>
>>>
>> v1 have obvious something wrong as i pointed and verified.
>> so i think it is not suitable to attach v1's review-by tag to v2 anyway.
> 
> @Zijun, your concern is that current DTs may not define the gpio and
> this will cause the bluetooth not to work?
> 
> Would that not more appropriately be fixed by machine-specific fixups
> for the DT?
> 
for lunched production, it is difficult or not possible to change such
config.

>>
>>> And mailing list tools will pick it up.
>>>
>>> Bartosz
>>
>

Bartosz Golaszewski April 24, 2024, 1:39 p.m. UTC | #10

On Wed, Apr 24, 2024 at 3:31 PM Wren Turkal <[e-mail]> wrote:
>
> On 4/24/24 6:22 AM, quic_zijuhu wrote:
> > On 4/24/2024 9:18 PM, Bartosz Golaszewski wrote:
> >> On Wed, 24 Apr 2024 at 15:10, Wren Turkal <[e-mail]> wrote:
> >>>
> >>> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
> >>>> From: Bartosz Golaszewski<[e-mail]>
> >>>>
> >>>> Any return value from gpiod_get_optional() other than a pointer to a
> >>>> GPIO descriptor or a NULL-pointer is an error and the driver should
> >>>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> >>>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> >>>> power_ctrl_enabled on NULL-pointer returned by
> >>>> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> >>>> While at it: also bail-out on error returned when trying to get the
> >>>> "swctrl" GPIO.
> >>>>
> >>>> Reported-by: Wren Turkal<[e-mail]>
> >>>> Reported-by: Zijun Hu<[e-mail]>
> >>>> Closes:https://lore.kernel.org/linux-bluetooth/[e-mail]/
> >>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> >>>> Reviewed-by: Krzysztof Kozlowski<[e-mail]>
> >>>> Signed-off-by: Bartosz Golaszewski<[e-mail]>
> >>>
> >>> Tested-by: "Wren Turkal" <[e-mail]>
> >>>
> >>>
> >>> Like this?
> >>
> >> Yes, awesome, thanks.
> >>
> >> This is how reviewing works too in the kernel, look at what Krzysztof
> >> did under v1, he just wrote:
> >>
> >> Reviewed-by: Krzysztof Kozlowski<[e-mail]>
> >>
> > v1 have obvious something wrong as i pointed and verified.
> > so i think it is not suitable to attach v1's review-by tag to v2 anyway.
>
> @Zijun, your concern is that current DTs may not define the gpio and
> this will cause the bluetooth not to work?
>

This is simply not true. If the GPIO is not specified,
gpiod_get_optional() will return NULL and GPIO APIs will work just
fine.

That being said: the contract for whether a GPIO is needed or not is
not in the driver C code or released DT sources. It's in the bindings
documents under Documentation/devicetree/bindings/. This is the source
of truth. So if the binding for a given model has always said
"required: enable-gpios" then we're absolutely in our rights to fix
the driver to conform to that even if previously omitted. If you think
otherwise - relax the bindings first.

Bart

> Would that not more appropriately be fixed by machine-specific fixups
> for the DT?
>
> >
> >> And mailing list tools will pick it up.
> >>
> >> Bartosz
> >
>
> --
> You're more amazing than you think!

Wren Turkal April 24, 2024, 1:40 p.m. UTC | #11

On 4/24/24 6:36 AM, quic_zijuhu wrote:
> On 4/24/2024 9:31 PM, Wren Turkal wrote:
>> On 4/24/24 6:22 AM, quic_zijuhu wrote:
>>> On 4/24/2024 9:18 PM, Bartosz Golaszewski wrote:
>>>> On Wed, 24 Apr 2024 at 15:10, Wren Turkal <[e-mail]> wrote:
>>>>>
>>>>> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
>>>>>> From: Bartosz Golaszewski<[e-mail]>
>>>>>>
>>>>>> Any return value from gpiod_get_optional() other than a pointer to a
>>>>>> GPIO descriptor or a NULL-pointer is an error and the driver should
>>>>>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth:
>>>>>> hci_qca:
>>>>>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
>>>>>> power_ctrl_enabled on NULL-pointer returned by
>>>>>> devm_gpiod_get_optional(). Restore this behavior but bail-out on
>>>>>> errors.
>>>>>> While at it: also bail-out on error returned when trying to get the
>>>>>> "swctrl" GPIO.
>>>>>>
>>>>>> Reported-by: Wren Turkal<[e-mail]>
>>>>>> Reported-by: Zijun Hu<[e-mail]>
>>>>>> Closes:https://lore.kernel.org/linux-bluetooth/[e-mail]/
>>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use
>>>>>> IS_ERR_OR_NULL() with gpiod_get_optional()")
>>>>>> Reviewed-by: Krzysztof Kozlowski<[e-mail]>
>>>>>> Signed-off-by: Bartosz Golaszewski<[e-mail]>
>>>>>
>>>>> Tested-by: "Wren Turkal" <[e-mail]>
>>>>>
>>>>>
>>>>> Like this?
>>>>
>>>> Yes, awesome, thanks.
>>>>
>>>> This is how reviewing works too in the kernel, look at what Krzysztof
>>>> did under v1, he just wrote:
>>>>
>>>> Reviewed-by: Krzysztof Kozlowski<[e-mail]>
>>>>
>>> v1 have obvious something wrong as i pointed and verified.
>>> so i think it is not suitable to attach v1's review-by tag to v2 anyway.
>>
>> @Zijun, your concern is that current DTs may not define the gpio and
>> this will cause the bluetooth not to work?
>>
>> Would that not more appropriately be fixed by machine-specific fixups
>> for the DT?
>>
> for lunched production, it is difficult or not possible to change such
> config.

I am not talking about the DT in the device. I am talking about the 
mechanism the kernel has for applying fixups to DTs.

If a dev builds a new kernel for a dev and finds it not to work, the 
kernel would then have a fixup added, like described here: 
https://docs.kernel.org/devicetree/usage-model.html#platform-identification

> 
>>>
>>>> And mailing list tools will pick it up.
>>>>
>>>> Bartosz
>>>
>>
>

Krzysztof Kozlowski April 24, 2024, 2:46 p.m. UTC | #12

On 24/04/2024 14:29, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[e-mail]>
> 

>  		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>  		if (IS_ERR(qcadev->susclk)) {
> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>  					       GPIOD_OUT_LOW);
>  		if (IS_ERR(qcadev->bt_en)) {
> -			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> -			power_ctrl_enabled = false;
> +			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
> +			return PTR_ERR(qcadev->bt_en);
>  		}
>  
> +		if (!qcadev->bt_en)
> +			power_ctrl_enabled = false;

This looks duplicated - you already have such check earlier.

Best regards,
Krzysztof

Bartosz Golaszewski April 24, 2024, 2:52 p.m. UTC | #13

On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
<[e-mail]> wrote:
>
> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[e-mail]>
> >
>
> >               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
> >               if (IS_ERR(qcadev->susclk)) {
> > @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >               qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
> >                                              GPIOD_OUT_LOW);
> >               if (IS_ERR(qcadev->bt_en)) {
> > -                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> > -                     power_ctrl_enabled = false;
> > +                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
> > +                     return PTR_ERR(qcadev->bt_en);
> >               }
> >
> > +             if (!qcadev->bt_en)
> > +                     power_ctrl_enabled = false;
>
> This looks duplicated - you already have such check earlier.
>

It's under a different switch case!

Bartosz

Krzysztof Kozlowski April 24, 2024, 3:05 p.m. UTC | #14

On 24/04/2024 16:52, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
> <[e-mail]> wrote:
>>
>> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <[e-mail]>
>>>
>>
>>>               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>>>               if (IS_ERR(qcadev->susclk)) {
>>> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>               qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>>                                              GPIOD_OUT_LOW);
>>>               if (IS_ERR(qcadev->bt_en)) {
>>> -                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>> -                     power_ctrl_enabled = false;
>>> +                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
>>> +                     return PTR_ERR(qcadev->bt_en);
>>>               }
>>>
>>> +             if (!qcadev->bt_en)
>>> +                     power_ctrl_enabled = false;
>>
>> This looks duplicated - you already have such check earlier.
>>
> 
> It's under a different switch case!

Damn diff. Yeah, looks ok.

Reviewed-by: Krzysztof Kozlowski <[e-mail]>

Best regards,
Krzysztof

quic_zijuhu April 24, 2024, 3:24 p.m. UTC | #15

On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
> <[e-mail]> wrote:
>>
>> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <[e-mail]>
>>>
>>
>>>               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>>>               if (IS_ERR(qcadev->susclk)) {
>>> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>               qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>>                                              GPIOD_OUT_LOW);
>>>               if (IS_ERR(qcadev->bt_en)) {
>>> -                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>> -                     power_ctrl_enabled = false;
>>> +                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
>>> +                     return PTR_ERR(qcadev->bt_en);
please think about for QCA2066. if it is returned from here.  BT will
not working at all.  if you don't return here. i will be working fine
for every BT functionality.
NAK again by me.

>>>               }
>>>
>>> +             if (!qcadev->bt_en)
>>> +                     power_ctrl_enabled = false;
>>
>> This looks duplicated - you already have such check earlier.
>>
> 
> It's under a different switch case!
> 
> Bartosz

quic_zijuhu April 24, 2024, 3:29 p.m. UTC | #16

On 4/24/2024 11:24 PM, quic_zijuhu wrote:
> On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote:
>> On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
>> <[e-mail]> wrote:
>>>
>>> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
>>>> From: Bartosz Golaszewski <[e-mail]>
>>>>
>>>
>>>>               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>>>>               if (IS_ERR(qcadev->susclk)) {
>>>> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>               qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>>>                                              GPIOD_OUT_LOW);
>>>>               if (IS_ERR(qcadev->bt_en)) {
>>>> -                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>>> -                     power_ctrl_enabled = false;
>>>> +                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
>>>> +                     return PTR_ERR(qcadev->bt_en);
> please think about for QCA2066. if it is returned from here.  BT will
> not working at all.  if you don't return here. i will be working fine
> for every BT functionality.
sorry, correct a type error, it is QCA6390 not QCA2066.
> NAK again by me.
> 
>>>>               }
>>>>
>>>> +             if (!qcadev->bt_en)
>>>> +                     power_ctrl_enabled = false;
>>>
>>> This looks duplicated - you already have such check earlier.
>>>
>>
>> It's under a different switch case!
>>
>> Bartosz
> 
>

Bartosz Golaszewski April 24, 2024, 3:30 p.m. UTC | #17

On Wed, Apr 24, 2024 at 5:24 PM quic_zijuhu <[e-mail]> wrote:
>
> On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote:
> > On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
> > <[e-mail]> wrote:
> >>
> >> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <[e-mail]>
> >>>
> >>
> >>>               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
> >>>               if (IS_ERR(qcadev->susclk)) {
> >>> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>>               qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
> >>>                                              GPIOD_OUT_LOW);
> >>>               if (IS_ERR(qcadev->bt_en)) {
> >>> -                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> >>> -                     power_ctrl_enabled = false;
> >>> +                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
> >>> +                     return PTR_ERR(qcadev->bt_en);
> please think about for QCA2066. if it is returned from here.  BT will
> not working at all.  if you don't return here. i will be working fine
> for every BT functionality.
> NAK again by me.
>

Luiz,

This in turn is an example of Zijun making a claim that looks like a
legitimate review but is simply untrue. He's done it several times.
I'm afraid that it may affect your judgment due to the confidence the
claims are made with. As Krzysztof said multiple times: the
device-tree bindings for QCA2066 are very clear: the enable-gpios
property is *required* and so returning an error on failure here is
correct. Even changing gpiod_get_optional() to just gpiod_get() would
be in line with what the contract in the binding document says. Even
if we relaxed the bindings, returning here stil *IS CORRECT* as if the
enable-gpios is not defined, GPIOLIB will return NULL and we will NOT
return.

Bartosz

> >>>               }
> >>>
> >>> +             if (!qcadev->bt_en)
> >>> +                     power_ctrl_enabled = false;
> >>
> >> This looks duplicated - you already have such check earlier.
> >>
> >
> > It's under a different switch case!
> >
> > Bartosz
>

Bartosz Golaszewski April 24, 2024, 3:31 p.m. UTC | #18

On Wed, Apr 24, 2024 at 5:30 PM quic_zijuhu <[e-mail]> wrote:
>
> On 4/24/2024 11:24 PM, quic_zijuhu wrote:
> > On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote:
> >> On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
> >> <[e-mail]> wrote:
> >>>
> >>> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
> >>>> From: Bartosz Golaszewski <[e-mail]>
> >>>>
> >>>
> >>>>               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
> >>>>               if (IS_ERR(qcadev->susclk)) {
> >>>> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>>>               qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
> >>>>                                              GPIOD_OUT_LOW);
> >>>>               if (IS_ERR(qcadev->bt_en)) {
> >>>> -                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> >>>> -                     power_ctrl_enabled = false;
> >>>> +                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
> >>>> +                     return PTR_ERR(qcadev->bt_en);
> > please think about for QCA2066. if it is returned from here.  BT will
> > not working at all.  if you don't return here. i will be working fine
> > for every BT functionality.
> sorry, correct a type error, it is QCA6390 not QCA2066.

Doesn't matter. If enable-gpios is not there, gpiod_get_optional()
will return NULL and we will not return.

Bart

> > NAK again by me.
> >
> >>>>               }
> >>>>
> >>>> +             if (!qcadev->bt_en)
> >>>> +                     power_ctrl_enabled = false;
> >>>
> >>> This looks duplicated - you already have such check earlier.
> >>>
> >>
> >> It's under a different switch case!
> >>
> >> Bartosz
> >
> >
>

Krzysztof Kozlowski April 24, 2024, 3:31 p.m. UTC | #19

On 24/04/2024 17:24, quic_zijuhu wrote:
> On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote:
>> On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
>> <[e-mail]> wrote:
>>>
>>> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
>>>> From: Bartosz Golaszewski <[e-mail]>
>>>>
>>>
>>>>               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>>>>               if (IS_ERR(qcadev->susclk)) {
>>>> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>               qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>>>                                              GPIOD_OUT_LOW);
>>>>               if (IS_ERR(qcadev->bt_en)) {
>>>> -                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>>> -                     power_ctrl_enabled = false;
>>>> +                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
>>>> +                     return PTR_ERR(qcadev->bt_en);
> please think about for QCA2066. if it is returned from here.  BT will

Which is correct. QCA2066 requires GPIO. Look at the bindings.

> not working at all.  if you don't return here. i will be working fine
> for every BT functionality.
> NAK again by me.

Yeah, my bad, I taught you that sentence and you keep repeating it.

Best regards,
Krzysztof

quic_zijuhu April 24, 2024, 3:35 p.m. UTC | #20

On 4/24/2024 11:31 PM, Bartosz Golaszewski wrote:
> On Wed, Apr 24, 2024 at 5:30 PM quic_zijuhu <[e-mail]> wrote:
>>
>> On 4/24/2024 11:24 PM, quic_zijuhu wrote:
>>> On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote:
>>>> On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
>>>> <[e-mail]> wrote:
>>>>>
>>>>> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
>>>>>> From: Bartosz Golaszewski <[e-mail]>
>>>>>>
>>>>>
>>>>>>               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>>>>>>               if (IS_ERR(qcadev->susclk)) {
>>>>>> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>>>               qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>>>>>                                              GPIOD_OUT_LOW);
>>>>>>               if (IS_ERR(qcadev->bt_en)) {
>>>>>> -                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>>>>> -                     power_ctrl_enabled = false;
>>>>>> +                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
>>>>>> +                     return PTR_ERR(qcadev->bt_en);
>>> please think about for QCA2066. if it is returned from here.  BT will
>>> not working at all.  if you don't return here. i will be working fine
>>> for every BT functionality.
>> sorry, correct a type error, it is QCA6390 not QCA2066.
> 
> Doesn't matter. If enable-gpios is not there, gpiod_get_optional()
> will return NULL and we will not return.
> 
i think i need to explain more for my consider case.
let me take QCA6390,  if the property enable-gpios is configured.
but IS_ERR(qcadev->bt_en) case happens, your change will do error
return, so BT will not work at all

but if you don't do error return, BT will working fine.

so your fix is not right regarding QCA6390.

> Bart
> 
>>> NAK again by me.
>>>
>>>>>>               }
>>>>>>
>>>>>> +             if (!qcadev->bt_en)
>>>>>> +                     power_ctrl_enabled = false;
>>>>>
>>>>> This looks duplicated - you already have such check earlier.
>>>>>
>>>>
>>>> It's under a different switch case!
>>>>
>>>> Bartosz
>>>
>>>
>>

[e-mail] April 24, 2024, 3:40 p.m. UTC | #21

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Wed, 24 Apr 2024 14:29:32 +0200 you wrote:
> From: Bartosz Golaszewski <[e-mail]>
> 
> Any return value from gpiod_get_optional() other than a pointer to a
> GPIO descriptor or a NULL-pointer is an error and the driver should
> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> power_ctrl_enabled on NULL-pointer returned by
> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> While at it: also bail-out on error returned when trying to get the
> "swctrl" GPIO.
> 
> [...]

Here is the summary with links:
  - [v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()
    https://git.kernel.org/bluetooth/bluetooth-next/c/48a9e64a533b

You are awesome, thank you!

Luiz Augusto von Dentz April 24, 2024, 3:41 p.m. UTC | #22

Hi Quic_zijuhu,

On Wed, Apr 24, 2024 at 11:35 AM quic_zijuhu <[e-mail]> wrote:
>
> On 4/24/2024 11:31 PM, Bartosz Golaszewski wrote:
> > On Wed, Apr 24, 2024 at 5:30 PM quic_zijuhu <[e-mail]> wrote:
> >>
> >> On 4/24/2024 11:24 PM, quic_zijuhu wrote:
> >>> On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote:
> >>>> On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
> >>>> <[e-mail]> wrote:
> >>>>>
> >>>>> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
> >>>>>> From: Bartosz Golaszewski <[e-mail]>
> >>>>>>
> >>>>>
> >>>>>>               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
> >>>>>>               if (IS_ERR(qcadev->susclk)) {
> >>>>>> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >>>>>>               qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
> >>>>>>                                              GPIOD_OUT_LOW);
> >>>>>>               if (IS_ERR(qcadev->bt_en)) {
> >>>>>> -                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> >>>>>> -                     power_ctrl_enabled = false;
> >>>>>> +                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
> >>>>>> +                     return PTR_ERR(qcadev->bt_en);
> >>> please think about for QCA2066. if it is returned from here.  BT will
> >>> not working at all.  if you don't return here. i will be working fine
> >>> for every BT functionality.
> >> sorry, correct a type error, it is QCA6390 not QCA2066.
> >
> > Doesn't matter. If enable-gpios is not there, gpiod_get_optional()
> > will return NULL and we will not return.
> >
> i think i need to explain more for my consider case.
> let me take QCA6390,  if the property enable-gpios is configured.
> but IS_ERR(qcadev->bt_en) case happens, your change will do error
> return, so BT will not work at all
>
> but if you don't do error return, BT will working fine.
>
> so your fix is not right regarding QCA6390.

Actually I'd say he is probably right with respect to DT because that
seems to require GPIO, we probably need a clearer way to differentiate
if a device is being set up via DT or not (btattach), in any case DT
is probably preferable thus why I went ahead and applied this one.

> > Bart
> >
> >>> NAK again by me.
> >>>
> >>>>>>               }
> >>>>>>
> >>>>>> +             if (!qcadev->bt_en)
> >>>>>> +                     power_ctrl_enabled = false;
> >>>>>
> >>>>> This looks duplicated - you already have such check earlier.
> >>>>>
> >>>>
> >>>> It's under a different switch case!
> >>>>
> >>>> Bartosz
> >>>
> >>>
> >>
>

quic_zijuhu April 24, 2024, 3:47 p.m. UTC | #23

On 4/24/2024 11:41 PM, Luiz Augusto von Dentz wrote:
> Hi Quic_zijuhu,
> 
> On Wed, Apr 24, 2024 at 11:35 AM quic_zijuhu <[e-mail]> wrote:
>>
>> On 4/24/2024 11:31 PM, Bartosz Golaszewski wrote:
>>> On Wed, Apr 24, 2024 at 5:30 PM quic_zijuhu <[e-mail]> wrote:
>>>>
>>>> On 4/24/2024 11:24 PM, quic_zijuhu wrote:
>>>>> On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote:
>>>>>> On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
>>>>>> <[e-mail]> wrote:
>>>>>>>
>>>>>>> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
>>>>>>>> From: Bartosz Golaszewski <[e-mail]>
>>>>>>>>
>>>>>>>
>>>>>>>>               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>>>>>>>>               if (IS_ERR(qcadev->susclk)) {
>>>>>>>> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>>>>>               qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>>>>>>>                                              GPIOD_OUT_LOW);
>>>>>>>>               if (IS_ERR(qcadev->bt_en)) {
>>>>>>>> -                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>>>>>>> -                     power_ctrl_enabled = false;
>>>>>>>> +                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
>>>>>>>> +                     return PTR_ERR(qcadev->bt_en);
>>>>> please think about for QCA2066. if it is returned from here.  BT will
>>>>> not working at all.  if you don't return here. i will be working fine
>>>>> for every BT functionality.
>>>> sorry, correct a type error, it is QCA6390 not QCA2066.
>>>
>>> Doesn't matter. If enable-gpios is not there, gpiod_get_optional()
>>> will return NULL and we will not return.
>>>
>> i think i need to explain more for my consider case.
>> let me take QCA6390,  if the property enable-gpios is configured.
>> but IS_ERR(qcadev->bt_en) case happens, your change will do error
>> return, so BT will not work at all
>>
>> but if you don't do error return, BT will working fine.
>>
>> so your fix is not right regarding QCA6390.
> 
> Actually I'd say he is probably right with respect to DT because that
> seems to require GPIO, we probably need a clearer way to differentiate
> if a device is being set up via DT or not (btattach), in any case DT
> is probably preferable thus why I went ahead and applied this one.
> 
for QCA6390, i can confirm that enable-gpios is optional. it is bring-up
by my team. i can confirm reporter's machine don't config the GPIO pin.
DTS binding spec also don't mark it as required.

that is why i change my changing from initial reverting the whole change
to only focus on QCA6390 that is the machine reported the issue.


>>> Bart
>>>
>>>>> NAK again by me.
>>>>>
>>>>>>>>               }
>>>>>>>>
>>>>>>>> +             if (!qcadev->bt_en)
>>>>>>>> +                     power_ctrl_enabled = false;
>>>>>>>
>>>>>>> This looks duplicated - you already have such check earlier.
>>>>>>>
>>>>>>
>>>>>> It's under a different switch case!
>>>>>>
>>>>>> Bartosz
>>>>>
>>>>>
>>>>
>>
> 
>

quic_zijuhu April 24, 2024, 3:57 p.m. UTC | #24

On 4/24/2024 11:47 PM, quic_zijuhu wrote:
> On 4/24/2024 11:41 PM, Luiz Augusto von Dentz wrote:
>> Hi Quic_zijuhu,
>>
>> On Wed, Apr 24, 2024 at 11:35 AM quic_zijuhu <[e-mail]> wrote:
>>>
>>> On 4/24/2024 11:31 PM, Bartosz Golaszewski wrote:
>>>> On Wed, Apr 24, 2024 at 5:30 PM quic_zijuhu <[e-mail]> wrote:
>>>>>
>>>>> On 4/24/2024 11:24 PM, quic_zijuhu wrote:
>>>>>> On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote:
>>>>>>> On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
>>>>>>> <[e-mail]> wrote:
>>>>>>>>
>>>>>>>> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
>>>>>>>>> From: Bartosz Golaszewski <[e-mail]>
>>>>>>>>>
>>>>>>>>
>>>>>>>>>               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>>>>>>>>>               if (IS_ERR(qcadev->susclk)) {
>>>>>>>>> @@ -2355,10 +2360,13 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>>>>>>>>               qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>>>>>>>>>                                              GPIOD_OUT_LOW);
>>>>>>>>>               if (IS_ERR(qcadev->bt_en)) {
>>>>>>>>> -                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>>>>>>>> -                     power_ctrl_enabled = false;
>>>>>>>>> +                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
>>>>>>>>> +                     return PTR_ERR(qcadev->bt_en);
>>>>>> please think about for QCA2066. if it is returned from here.  BT will
>>>>>> not working at all.  if you don't return here. i will be working fine
>>>>>> for every BT functionality.
>>>>> sorry, correct a type error, it is QCA6390 not QCA2066.
>>>>
>>>> Doesn't matter. If enable-gpios is not there, gpiod_get_optional()
>>>> will return NULL and we will not return.
>>>>
>>> i think i need to explain more for my consider case.
>>> let me take QCA6390,  if the property enable-gpios is configured.
>>> but IS_ERR(qcadev->bt_en) case happens, your change will do error
>>> return, so BT will not work at all
>>>
>>> but if you don't do error return, BT will working fine.
>>>
>>> so your fix is not right regarding QCA6390.
>>
>> Actually I'd say he is probably right with respect to DT because that
>> seems to require GPIO, we probably need a clearer way to differentiate
>> if a device is being set up via DT or not (btattach), in any case DT
>> is probably preferable thus why I went ahead and applied this one.
>>
> for QCA6390, i can confirm that enable-gpios is optional. it is bring-up
> by my team. i can confirm reporter's machine don't config the GPIO pin.
> DTS binding spec also don't mark it as required.
> 
> that is why i change my changing from initial reverting the whole change
> to only focus on QCA6390 that is the machine reported the issue.
> 
> 
>>>> Bart
>>>>
>>>>>> NAK again by me.
>>>>>>
>>>>>>>>>               }
>>>>>>>>>
>>>>>>>>> +             if (!qcadev->bt_en)
>>>>>>>>> +                     power_ctrl_enabled = false;
>>>>>>>>
>>>>>>>> This looks duplicated - you already have such check earlier.
>>>>>>>>
>>>>>>>
>>>>>>> It's under a different switch case!
>>>>>>>
>>>>>>> Bartosz
>>>>>>
>>>>>>
>>>>>
>>>
>>
>>
> 
> 
i find the change have been merged.
i think it is not good manner to merge change in this way even if i give
reasonable doubt

Bartosz Golaszewski April 26, 2024, 2:37 p.m. UTC | #25

On Wed, 24 Apr 2024 17:40:27 +0200, [e-mail] said:
> Hello:
>
> This patch was applied to bluetooth/bluetooth-next.git (master)
> by Luiz Augusto von Dentz <[e-mail]>:
>
> On Wed, 24 Apr 2024 14:29:32 +0200 you wrote:
>> From: Bartosz Golaszewski <[e-mail]>
>>
>> Any return value from gpiod_get_optional() other than a pointer to a
>> GPIO descriptor or a NULL-pointer is an error and the driver should
>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
>> power_ctrl_enabled on NULL-pointer returned by
>> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
>> While at it: also bail-out on error returned when trying to get the
>> "swctrl" GPIO.
>>
>> [...]
>
> Here is the summary with links:
>   - [v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()
>     https://git.kernel.org/bluetooth/bluetooth-next/c/48a9e64a533b
>
> You are awesome, thank you!
> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
>
>
>

Luiz,

I think patchwork borked when picking up this one, here's what the commit
trailer looks like in next:

    Reported-by: Wren Turkal <[e-mail]>
    Reported-by: Zijun Hu <[e-mail]>
    Closes: https://lore.kernel.org/linux-bluetooth/[e-mail]/
    Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use
IS_ERR_OR_NULL() with gpiod_get_optional()")
    Reviewed-by: Krzysztof Kozlowski <[e-mail]>
    Signed-off-by: Bartosz Golaszewski <[e-mail]>
    Tested-by: Wren Turkal" <[e-mail]>
    Reported-by: Wren Turkal <[e-mail]>
    Reported-by: Zijun Hu <[e-mail]>
    Reviewed-by: Krzysztof Kozlowski<[e-mail]>
    Reviewed-by: Krzysztof Kozlowski <[e-mail]>
    Signed-off-by: Luiz Augusto von Dentz <[e-mail]>

Reported-by and Reviewed-by tags are duplicated. One of the RB tags is missing
a space.

Bartosz

Luiz Augusto von Dentz April 26, 2024, 3:09 p.m. UTC | #26

Hi Bartosz,

On Fri, Apr 26, 2024 at 10:37 AM Bartosz Golaszewski <[e-mail]> wrote:
>
> On Wed, 24 Apr 2024 17:40:27 +0200, [e-mail] said:
> > Hello:
> >
> > This patch was applied to bluetooth/bluetooth-next.git (master)
> > by Luiz Augusto von Dentz <[e-mail]>:
> >
> > On Wed, 24 Apr 2024 14:29:32 +0200 you wrote:
> >> From: Bartosz Golaszewski <[e-mail]>
> >>
> >> Any return value from gpiod_get_optional() other than a pointer to a
> >> GPIO descriptor or a NULL-pointer is an error and the driver should
> >> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> >> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> >> power_ctrl_enabled on NULL-pointer returned by
> >> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> >> While at it: also bail-out on error returned when trying to get the
> >> "swctrl" GPIO.
> >>
> >> [...]
> >
> > Here is the summary with links:
> >   - [v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()
> >     https://git.kernel.org/bluetooth/bluetooth-next/c/48a9e64a533b
> >
> > You are awesome, thank you!
> > --
> > Deet-doot-dot, I am a bot.
> > https://korg.docs.kernel.org/patchwork/pwbot.html
> >
> >
> >
>
> Luiz,
>
> I think patchwork borked when picking up this one, here's what the commit
> trailer looks like in next:
>
>     Reported-by: Wren Turkal <[e-mail]>
>     Reported-by: Zijun Hu <[e-mail]>
>     Closes: https://lore.kernel.org/linux-bluetooth/[e-mail]/
>     Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use
> IS_ERR_OR_NULL() with gpiod_get_optional()")
>     Reviewed-by: Krzysztof Kozlowski <[e-mail]>
>     Signed-off-by: Bartosz Golaszewski <[e-mail]>
>     Tested-by: Wren Turkal" <[e-mail]>
>     Reported-by: Wren Turkal <[e-mail]>
>     Reported-by: Zijun Hu <[e-mail]>
>     Reviewed-by: Krzysztof Kozlowski<[e-mail]>
>     Reviewed-by: Krzysztof Kozlowski <[e-mail]>
>     Signed-off-by: Luiz Augusto von Dentz <[e-mail]>
>
> Reported-by and Reviewed-by tags are duplicated. One of the RB tags is missing
> a space.

Oh crap, should probably not trust patchwork would pick up the tags
properly, that said the pull-request was already merged, not sure if
we can do something about it now?

Bartosz Golaszewski April 26, 2024, 5:23 p.m. UTC | #27

On Fri, 26 Apr 2024 at 17:09, Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Bartosz,
>
> On Fri, Apr 26, 2024 at 10:37 AM Bartosz Golaszewski <[e-mail]> wrote:
> >
> > On Wed, 24 Apr 2024 17:40:27 +0200, [e-mail] said:
> > > Hello:
> > >
> > > This patch was applied to bluetooth/bluetooth-next.git (master)
> > > by Luiz Augusto von Dentz <[e-mail]>:
> > >
> > > On Wed, 24 Apr 2024 14:29:32 +0200 you wrote:
> > >> From: Bartosz Golaszewski <[e-mail]>
> > >>
> > >> Any return value from gpiod_get_optional() other than a pointer to a
> > >> GPIO descriptor or a NULL-pointer is an error and the driver should
> > >> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> > >> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> > >> power_ctrl_enabled on NULL-pointer returned by
> > >> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> > >> While at it: also bail-out on error returned when trying to get the
> > >> "swctrl" GPIO.
> > >>
> > >> [...]
> > >
> > > Here is the summary with links:
> > >   - [v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()
> > >     https://git.kernel.org/bluetooth/bluetooth-next/c/48a9e64a533b
> > >
> > > You are awesome, thank you!
> > > --
> > > Deet-doot-dot, I am a bot.
> > > https://korg.docs.kernel.org/patchwork/pwbot.html
> > >
> > >
> > >
> >
> > Luiz,
> >
> > I think patchwork borked when picking up this one, here's what the commit
> > trailer looks like in next:
> >
> >     Reported-by: Wren Turkal <[e-mail]>
> >     Reported-by: Zijun Hu <[e-mail]>
> >     Closes: https://lore.kernel.org/linux-bluetooth/[e-mail]/
> >     Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use
> > IS_ERR_OR_NULL() with gpiod_get_optional()")
> >     Reviewed-by: Krzysztof Kozlowski <[e-mail]>
> >     Signed-off-by: Bartosz Golaszewski <[e-mail]>
> >     Tested-by: Wren Turkal" <[e-mail]>
> >     Reported-by: Wren Turkal <[e-mail]>
> >     Reported-by: Zijun Hu <[e-mail]>
> >     Reviewed-by: Krzysztof Kozlowski<[e-mail]>
> >     Reviewed-by: Krzysztof Kozlowski <[e-mail]>
> >     Signed-off-by: Luiz Augusto von Dentz <[e-mail]>
> >
> > Reported-by and Reviewed-by tags are duplicated. One of the RB tags is missing
> > a space.
>
> Oh crap, should probably not trust patchwork would pick up the tags
> properly, that said the pull-request was already merged, not sure if
> we can do something about it now?
>

Nope, if it's gone upstream then it's too late.

BTW As a fresh b4 convert I highly recommend it for managing patches. :)

Bart

> --
> Luiz Augusto von Dentz

Patch

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 92fa20f5ac7d..0e98ad2c0c9d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2327,16 +2327,21 @@  static int qca_serdev_probe(struct serdev_device *serdev)
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855)) {
 			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
-			power_ctrl_enabled = false;
+			return PTR_ERR(qcadev->bt_en);
 		}
 
+		if (!qcadev->bt_en)
+			power_ctrl_enabled = false;
+
 		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
 					       GPIOD_IN);
 		if (IS_ERR(qcadev->sw_ctrl) &&
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855 ||
-		     data->soc_type == QCA_WCN7850))
-			dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+		     data->soc_type == QCA_WCN7850)) {
+			dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+			return PTR_ERR(qcadev->sw_ctrl);
+		}
 
 		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
 		if (IS_ERR(qcadev->susclk)) {
@@ -2355,10 +2360,13 @@  static int qca_serdev_probe(struct serdev_device *serdev)
 		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
 					       GPIOD_OUT_LOW);
 		if (IS_ERR(qcadev->bt_en)) {
-			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
-			power_ctrl_enabled = false;
+			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
+			return PTR_ERR(qcadev->bt_en);
 		}
 
+		if (!qcadev->bt_en)
+			power_ctrl_enabled = false;
+
 		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
 		if (IS_ERR(qcadev->susclk)) {
 			dev_warn(&serdev->dev, "failed to acquire clk\n");