| Message ID | 20240424122932.79120-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional() | expand |
On 4/24/2024 8:29 PM, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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 <wt@penguintechs.org> > Reported-by: Zijun Hu <quic_zijuhu@quicinc.com> > Closes: https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/ > Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()") > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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 <bartosz.golaszewski@linaro.org> > --- > 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.
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 <wt@penguintechs.org>
Reported-by: Zijun Hu <quic_zijuhu@quicinc.com>
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/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/"
##############################
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
On 4/24/2024 9:18 PM, Bartosz Golaszewski wrote: > On Wed, 24 Apr 2024 at 15:10, Wren Turkal <wt@penguintechs.org> wrote: >> >> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski<bartosz.golaszewski@linaro.org> >>> >>> 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<wt@penguintechs.org> >>> Reported-by: Zijun Hu<quic_zijuhu@quicinc.com> >>> Closes:https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/ >>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()") >>> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org> >>> Signed-off-by: Bartosz Golaszewski<bartosz.golaszewski@linaro.org> >> >> Tested-by: "Wren Turkal" <wt@penguintechs.org> >> >> >> 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<krzysztof.kozlowski@linaro.org> > 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
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 <wt@penguintechs.org> wrote: >>>> >>>> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote: >>>>> From: Bartosz Golaszewski<bartosz.golaszewski@linaro.org> >>>>> >>>>> 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<wt@penguintechs.org> >>>>> Reported-by: Zijun Hu<quic_zijuhu@quicinc.com> >>>>> Closes:https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/ >>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use >>>>> IS_ERR_OR_NULL() with gpiod_get_optional()") >>>>> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org> >>>>> Signed-off-by: Bartosz Golaszewski<bartosz.golaszewski@linaro.org> >>>> >>>> Tested-by: "Wren Turkal" <wt@penguintechs.org> >>>> >>>> >>>> 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<krzysztof.kozlowski@linaro.org> >>> >> 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 >> >
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 <wt@penguintechs.org> wrote: >>>>> >>>>> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote: >>>>>> From: Bartosz Golaszewski<bartosz.golaszewski@linaro.org> >>>>>> >>>>>> 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<wt@penguintechs.org> >>>>>> Reported-by: Zijun Hu<quic_zijuhu@quicinc.com> >>>>>> Closes:https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/ >>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use >>>>>> IS_ERR_OR_NULL() with gpiod_get_optional()") >>>>>> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org> >>>>>> Signed-off-by: Bartosz Golaszewski<bartosz.golaszewski@linaro.org> >>>>> >>>>> Tested-by: "Wren Turkal" <wt@penguintechs.org> >>>>> >>>>> >>>>> 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<krzysztof.kozlowski@linaro.org> >>>> >>> 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 >>> >> >
On 24/04/2024 14:29, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > 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
On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 24/04/2024 14:29, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > 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
On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote: > On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 24/04/2024 14:29, Bartosz Golaszewski wrote: >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>> >> >>> 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
On Wed, Apr 24, 2024 at 5:24 PM quic_zijuhu <quic_zijuhu@quicinc.com> wrote: > > On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote: > > On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 24/04/2024 14:29, Bartosz Golaszewski wrote: > >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > >>> > >> > >>> 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 >
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 >> <krzysztof.kozlowski@linaro.org> wrote: >>> >>> On 24/04/2024 14:29, Bartosz Golaszewski wrote: >>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>>> >>> >>>> 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
On 4/24/2024 11:31 PM, Bartosz Golaszewski wrote: > On Wed, Apr 24, 2024 at 5:30 PM quic_zijuhu <quic_zijuhu@quicinc.com> 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 >>>> <krzysztof.kozlowski@linaro.org> wrote: >>>>> >>>>> On 24/04/2024 14:29, Bartosz Golaszewski wrote: >>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>>>>> >>>>> >>>>>> 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 >>> >>> >>
Hi Quic_zijuhu, On Wed, Apr 24, 2024 at 11:35 AM quic_zijuhu <quic_zijuhu@quicinc.com> wrote: > > On 4/24/2024 11:31 PM, Bartosz Golaszewski wrote: > > On Wed, Apr 24, 2024 at 5:30 PM quic_zijuhu <quic_zijuhu@quicinc.com> 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 > >>>> <krzysztof.kozlowski@linaro.org> wrote: > >>>>> > >>>>> On 24/04/2024 14:29, Bartosz Golaszewski wrote: > >>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > >>>>>> > >>>>> > >>>>>> 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 > >>> > >>> > >> >
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 <quic_zijuhu@quicinc.com> wrote: >> >> On 4/24/2024 11:31 PM, Bartosz Golaszewski wrote: >>> On Wed, Apr 24, 2024 at 5:30 PM quic_zijuhu <quic_zijuhu@quicinc.com> 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 >>>>>> <krzysztof.kozlowski@linaro.org> wrote: >>>>>>> >>>>>>> On 24/04/2024 14:29, Bartosz Golaszewski wrote: >>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >>>>>>>> >>>>>>> >>>>>>>> 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 >>>>> >>>>> >>>> >> > >
On Wed, 24 Apr 2024 17:40:27 +0200, patchwork-bot+bluetooth@kernel.org said: > Hello: > > This patch was applied to bluetooth/bluetooth-next.git (master) > by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: > > On Wed, 24 Apr 2024 14:29:32 +0200 you wrote: >> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> >> >> 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 <wt@penguintechs.org> Reported-by: Zijun Hu <quic_zijuhu@quicinc.com> Closes: https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/ Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()") Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> Tested-by: Wren Turkal" <wt@penguintechs.org> Reported-by: Wren Turkal <wt@penguintechs.org> Reported-by: Zijun Hu <quic_zijuhu@quicinc.com> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> Reported-by and Reviewed-by tags are duplicated. One of the RB tags is missing a space. Bartosz
Hi Bartosz, On Fri, Apr 26, 2024 at 10:37 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Wed, 24 Apr 2024 17:40:27 +0200, patchwork-bot+bluetooth@kernel.org said: > > Hello: > > > > This patch was applied to bluetooth/bluetooth-next.git (master) > > by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: > > > > On Wed, 24 Apr 2024 14:29:32 +0200 you wrote: > >> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > >> > >> 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 <wt@penguintechs.org> > Reported-by: Zijun Hu <quic_zijuhu@quicinc.com> > Closes: https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/ > Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use > IS_ERR_OR_NULL() with gpiod_get_optional()") > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > Tested-by: Wren Turkal" <wt@penguintechs.org> > Reported-by: Wren Turkal <wt@penguintechs.org> > Reported-by: Zijun Hu <quic_zijuhu@quicinc.com> > Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > 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?
On Fri, 26 Apr 2024 at 17:09, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Bartosz, > > On Fri, Apr 26, 2024 at 10:37 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > On Wed, 24 Apr 2024 17:40:27 +0200, patchwork-bot+bluetooth@kernel.org said: > > > Hello: > > > > > > This patch was applied to bluetooth/bluetooth-next.git (master) > > > by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: > > > > > > On Wed, 24 Apr 2024 14:29:32 +0200 you wrote: > > >> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > >> > > >> 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 <wt@penguintechs.org> > > Reported-by: Zijun Hu <quic_zijuhu@quicinc.com> > > Closes: https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/ > > Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use > > IS_ERR_OR_NULL() with gpiod_get_optional()") > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Tested-by: Wren Turkal" <wt@penguintechs.org> > > Reported-by: Wren Turkal <wt@penguintechs.org> > > Reported-by: Zijun Hu <quic_zijuhu@quicinc.com> > > Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > > > > 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
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");