[PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[Zijun Hu]

Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
with gpiod_get_optional()") has wrong logic for below case:

property enable-gpios is not configured for WCN6750 and WCN6855

Function devm_gpiod_get_optional(...,"enable",...) returns NULL for above
case, we normaly need to treat it as success case as the commit argued
but the property enable-gpios is marked as required by the binding spec
Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml
so we can't treat it as success case any more since it is a required
property but not configured by user.

Fix by reverting the commit's impact for WCN6750 and WCN6855, error
prompt is also added for this case.

Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 drivers/bluetooth/hci_qca.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index b621a0a40ea4..5c6bafe008ed 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2328,10 +2328,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) &&
+		if (IS_ERR_OR_NULL(qcadev->bt_en) &&
 		    (data->soc_type == QCA_WCN6750 ||
 		     data->soc_type == QCA_WCN6855)) {
-			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
+			if (IS_ERR(qcadev->bt_en))
+				dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
+			else
+				dev_err(&serdev->dev, "required BT_EN gpio is not configured\n");
 			power_ctrl_enabled = false;
 		}
 
-- 
2.7.4

Re: [PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[Krzysztof Kozlowski]

On 24/04/2024 10:35, Zijun Hu wrote:
> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
> with gpiod_get_optional()") has wrong logic for below case:
> 
> property enable-gpios is not configured for WCN6750 and WCN6855

As I said before, bindings say this property is required. I already
asked you to provide rationale describing hardware and update the
bindings if they bindings are not correct.

Best regards,
Krzysztof

Re: [PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[quic_zijuhu]

On 4/24/2024 4:44 PM, Krzysztof Kozlowski wrote:
> On 24/04/2024 10:35, Zijun Hu wrote:
>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>> with gpiod_get_optional()") has wrong logic for below case:
>>
>> property enable-gpios is not configured for WCN6750 and WCN6855
> 
> As I said before, bindings say this property is required. I already
> asked you to provide rationale describing hardware and update the
> bindings if they bindings are not correct.
> 
1)
you ever given below reply at below link
https://lore.kernel.org/linux-bluetooth/52394d97-04c3-4f77-aaae-f1e152cd5632@linaro.org/

"The pin is required on 6750, 6855 and maybe others. You cannot not have
the GPIO"

2) for property enable-gpios, they are required for WCN6750 and WCN6855
in my opinion,  i am a member of BT team.

3) only care about the case property enable-gpios is not configured,
the original BT driver design logic indeed matches with binging spec's
requirements before bartosz's wrong change

4) please ask dts owner for help if you suspect current bindings spec
has something wrong. it is not my responsibility for providing such
info, that maybe involve CCI.

5) gentle reminder, please realize that there are many lunched products
already when you try to change some important logic, i don't suggest
change important logic or setting if there are no actual issue reported.

> Best regards,
> Krzysztof
> 

RE: [v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3142 bytes --]

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=847360

---Test result---

Test Summary:
CheckPatch                    PASS      0.58 seconds
GitLint                       FAIL      0.49 seconds
SubjectPrefix                 PASS      0.09 seconds
BuildKernel                   PASS      30.11 seconds
CheckAllWarning               PASS      32.36 seconds
CheckSparse                   PASS      38.10 seconds
CheckSmatch                   FAIL      36.30 seconds
BuildKernel32                 PASS      28.81 seconds
TestRunnerSetup               PASS      527.55 seconds
TestRunner_l2cap-tester       PASS      18.31 seconds
TestRunner_iso-tester         PASS      29.06 seconds
TestRunner_bnep-tester        PASS      4.72 seconds
TestRunner_mgmt-tester        PASS      113.75 seconds
TestRunner_rfcomm-tester      PASS      7.22 seconds
TestRunner_sco-tester         PASS      15.00 seconds
TestRunner_ioctl-tester       PASS      7.72 seconds
TestRunner_mesh-tester        PASS      5.74 seconds
TestRunner_smp-tester         PASS      6.72 seconds
TestRunner_userchan-tester    PASS      4.87 seconds
IncrementalBuild              PASS      28.20 seconds

Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

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 (89>80): "[v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855"
##############################
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


---
Regards,
Linux Bluetooth

Re: [PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[Krzysztof Kozlowski]

On 24/04/2024 11:22, quic_zijuhu wrote:
> On 4/24/2024 4:44 PM, Krzysztof Kozlowski wrote:
>> On 24/04/2024 10:35, Zijun Hu wrote:
>>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>> with gpiod_get_optional()") has wrong logic for below case:
>>>
>>> property enable-gpios is not configured for WCN6750 and WCN6855
>>
>> As I said before, bindings say this property is required. I already
>> asked you to provide rationale describing hardware and update the
>> bindings if they bindings are not correct.
>>
> 1)
> you ever given below reply at below link
> https://lore.kernel.org/linux-bluetooth/52394d97-04c3-4f77-aaae-f1e152cd5632@linaro.org/
> 
> "The pin is required on 6750, 6855 and maybe others. You cannot not have
> the GPIO"
> 
> 2) for property enable-gpios, they are required for WCN6750 and WCN6855
> in my opinion,  i am a member of BT team.

If they are required, then your commit msg is not precise and code looks
incorrect.

> 
> 3) only care about the case property enable-gpios is not configured,
> the original BT driver design logic indeed matches with binging spec's
> requirements before bartosz's wrong change

What? There is no such case according to bindings. I told you already
two times: Either change bindings or this is not valid.

> 
> 4) please ask dts owner for help if you suspect current bindings spec
> has something wrong. it is not my responsibility for providing such
> info, that maybe involve CCI.

What?

What or who is DTS owner?

I do not suspect bindings are wrong. You are implying it. Anyway, if
making driver correct according to bindings is not your responsibility,
then this patch is just bogus.

> 
> 5) gentle reminder, please realize that there are many lunched products
> already when you try to change some important logic, i don't suggest
> change important logic or setting if there are no actual issue reported.

What?

Best regards,
Krzysztof

Re: [PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[quic_zijuhu]

On 4/25/2024 3:12 AM, Krzysztof Kozlowski wrote:
> On 24/04/2024 11:22, quic_zijuhu wrote:
>> On 4/24/2024 4:44 PM, Krzysztof Kozlowski wrote:
>>> On 24/04/2024 10:35, Zijun Hu wrote:
>>>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>>> with gpiod_get_optional()") has wrong logic for below case:
>>>>
>>>> property enable-gpios is not configured for WCN6750 and WCN6855
>>>
>>> As I said before, bindings say this property is required. I already
>>> asked you to provide rationale describing hardware and update the
>>> bindings if they bindings are not correct.
>>>
>> 1)
>> you ever given below reply at below link
>> https://lore.kernel.org/linux-bluetooth/52394d97-04c3-4f77-aaae-f1e152cd5632@linaro.org/
>>
>> "The pin is required on 6750, 6855 and maybe others. You cannot not have
>> the GPIO"
>>
>> 2) for property enable-gpios, they are required for WCN6750 and WCN6855
>> in my opinion,  i am a member of BT team.
> 
> If they are required, then your commit msg is not precise and code looks
> incorrect.
> 
>>
>> 3) only care about the case property enable-gpios is not configured,
>> the original BT driver design logic indeed matches with binging spec's
>> requirements before bartosz's wrong change
> 
> What? There is no such case according to bindings. I told you already
> two times: Either change bindings or this is not valid.
> 
>>
>> 4) please ask dts owner for help if you suspect current bindings spec
>> has something wrong. it is not my responsibility for providing such
>> info, that maybe involve CCI.
> 
> What?
> 
> What or who is DTS owner?
> 
> I do not suspect bindings are wrong. You are implying it. Anyway, if
> making driver correct according to bindings is not your responsibility,
> then this patch is just bogus.
> 
>>
>> 5) gentle reminder, please realize that there are many lunched products
>> already when you try to change some important logic, i don't suggest
>> change important logic or setting if there are no actual issue reported.
> 
> What?
> 
as you maybe noticed, this change is meaningless after below fix was
selected and merged, so don't need to discuss this change any more.
Commit: 48a9e64a533b ("Bluetooth: qca: set power_ctrl_enabled on NULL
returned by gpiod_get_optional()")

thanks
> Best regards,
> Krzysztof
> 

Re: [PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[Wren Turkal]

On 4/24/24 12:12 PM, Krzysztof Kozlowski wrote:
> On 24/04/2024 11:22, quic_zijuhu wrote:
>> On 4/24/2024 4:44 PM, Krzysztof Kozlowski wrote:
>>> On 24/04/2024 10:35, Zijun Hu wrote:
>>>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
>>>> with gpiod_get_optional()") has wrong logic for below case:
>>>>
>>>> property enable-gpios is not configured for WCN6750 and WCN6855
>>>
>>> As I said before, bindings say this property is required. I already
>>> asked you to provide rationale describing hardware and update the
>>> bindings if they bindings are not correct.
>>>
>> 1)
>> you ever given below reply at below link
>> https://lore.kernel.org/linux-bluetooth/52394d97-04c3-4f77-aaae-f1e152cd5632@linaro.org/
>>
>> "The pin is required on 6750, 6855 and maybe others. You cannot not have
>> the GPIO"
>>
>> 2) for property enable-gpios, they are required for WCN6750 and WCN6855
>> in my opinion,  i am a member of BT team.
> 
> If they are required, then your commit msg is not precise and code looks
> incorrect.
> 
>>
>> 3) only care about the case property enable-gpios is not configured,
>> the original BT driver design logic indeed matches with binging spec's
>> requirements before bartosz's wrong change
> 
> What? There is no such case according to bindings. I told you already
> two times: Either change bindings or this is not valid.

@krzysztof, I'm curious. There is no entry in the binding specifically 
for qca6390. Should there be?

> 
>>
>> 4) please ask dts owner for help if you suspect current bindings spec
>> has something wrong. it is not my responsibility for providing such
>> info, that maybe involve CCI.
> 
> What?
> 
> What or who is DTS owner?
> 
> I do not suspect bindings are wrong. You are implying it. Anyway, if
> making driver correct according to bindings is not your responsibility,
> then this patch is just bogus.
> 
>>
>> 5) gentle reminder, please realize that there are many lunched products
>> already when you try to change some important logic, i don't suggest
>> change important logic or setting if there are no actual issue reported.
> 
> What?
> 
> Best regards,
> Krzysztof
> 

-- 
You're more amazing than you think!

Re: [PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[Krzysztof Kozlowski]

On 25/04/2024 00:01, Wren Turkal wrote:
>>>
>>> 3) only care about the case property enable-gpios is not configured,
>>> the original BT driver design logic indeed matches with binging spec's
>>> requirements before bartosz's wrong change
>>
>> What? There is no such case according to bindings. I told you already
>> two times: Either change bindings or this is not valid.
> 
> @krzysztof, I'm curious. There is no entry in the binding specifically 
> for qca6390. Should there be?

qca6390 is documented in the bindings, but you are right that it lacks
if:then: entry narrowing/choosing specific supplies and pins. It should
have one, indeed.

Best regards,
Krzysztof

Re: [PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[Wren Turkal]

On 4/24/24 11:05 PM, Krzysztof Kozlowski wrote:
> On 25/04/2024 00:01, Wren Turkal wrote:
>>>>
>>>> 3) only care about the case property enable-gpios is not configured,
>>>> the original BT driver design logic indeed matches with binging spec's
>>>> requirements before bartosz's wrong change
>>>
>>> What? There is no such case according to bindings. I told you already
>>> two times: Either change bindings or this is not valid.
>>
>> @krzysztof, I'm curious. There is no entry in the binding specifically
>> for qca6390. Should there be?
> 
> qca6390 is documented in the bindings, but you are right that it lacks
> if:then: entry narrowing/choosing specific supplies and pins. It should
> have one, indeed.

Would creating an entry for the qca6390 hardware fix the issue you are 
worried about?

Again, sorry for all the, what I assume are, basic questions. I am so 
far out of my depth here. I am just trying to figure out how to move 
forward. I really do appreciate your guidance here.

-- 
You're more amazing than you think!

Re: [PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[Bartosz Golaszewski]

On Thu, 25 Apr 2024 at 08:30, Wren Turkal <wt@penguintechs.org> wrote:
>
> On 4/24/24 11:05 PM, Krzysztof Kozlowski wrote:
> > On 25/04/2024 00:01, Wren Turkal wrote:
> >>>>
> >>>> 3) only care about the case property enable-gpios is not configured,
> >>>> the original BT driver design logic indeed matches with binging spec's
> >>>> requirements before bartosz's wrong change
> >>>
> >>> What? There is no such case according to bindings. I told you already
> >>> two times: Either change bindings or this is not valid.
> >>
> >> @krzysztof, I'm curious. There is no entry in the binding specifically
> >> for qca6390. Should there be?
> >
> > qca6390 is documented in the bindings, but you are right that it lacks
> > if:then: entry narrowing/choosing specific supplies and pins. It should
> > have one, indeed.
>
> Would creating an entry for the qca6390 hardware fix the issue you are
> worried about?
>
> Again, sorry for all the, what I assume are, basic questions. I am so
> far out of my depth here. I am just trying to figure out how to move
> forward. I really do appreciate your guidance here.
>

Wren, Krzysztof: I cannot stop you from doing this but I'm afraid this
will complicate the power sequencing work unnecessarily. The QCA6390
PMU bindings I'm proposing[1] are consumers of the BT enable GPIOs. In
my series I also create an entry for QCA6390 Bluetooth[2] but without
enable-gpios and with the inputs from the PMU, not host. Please
consider that if you decide to go with this.

Bartosz

[1] https://lore.kernel.org/linux-arm-kernel/20240410124628.171783-2-brgl@bgdev.pl/
[2] https://lore.kernel.org/linux-arm-kernel/20240410124628.171783-4-brgl@bgdev.pl/

Re: [PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[Krzysztof Kozlowski]

On 25/04/2024 10:30, Bartosz Golaszewski wrote:
> On Thu, 25 Apr 2024 at 08:30, Wren Turkal <wt@penguintechs.org> wrote:
>>
>> On 4/24/24 11:05 PM, Krzysztof Kozlowski wrote:
>>> On 25/04/2024 00:01, Wren Turkal wrote:
>>>>>>
>>>>>> 3) only care about the case property enable-gpios is not configured,
>>>>>> the original BT driver design logic indeed matches with binging spec's
>>>>>> requirements before bartosz's wrong change
>>>>>
>>>>> What? There is no such case according to bindings. I told you already
>>>>> two times: Either change bindings or this is not valid.
>>>>
>>>> @krzysztof, I'm curious. There is no entry in the binding specifically
>>>> for qca6390. Should there be?
>>>
>>> qca6390 is documented in the bindings, but you are right that it lacks
>>> if:then: entry narrowing/choosing specific supplies and pins. It should
>>> have one, indeed.
>>
>> Would creating an entry for the qca6390 hardware fix the issue you are
>> worried about?
>>
>> Again, sorry for all the, what I assume are, basic questions. I am so
>> far out of my depth here. I am just trying to figure out how to move
>> forward. I really do appreciate your guidance here.
>>
> 
> Wren, Krzysztof: I cannot stop you from doing this but I'm afraid this
> will complicate the power sequencing work unnecessarily. The QCA6390
> PMU bindings I'm proposing[1] are consumers of the BT enable GPIOs. In
> my series I also create an entry for QCA6390 Bluetooth[2] but without
> enable-gpios and with the inputs from the PMU, not host. Please
> consider that if you decide to go with this.

I don't have a near plan to describe QCA6390 supplies and pins, so don't
worry. I also don't think Qualcomm BT understand what are bindings, so I
don't expect patches from them.

Best regards,
Krzysztof

Re: [PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[quic_zijuhu]

On 4/25/2024 5:14 PM, Krzysztof Kozlowski wrote:
> On 25/04/2024 10:30, Bartosz Golaszewski wrote:
>> On Thu, 25 Apr 2024 at 08:30, Wren Turkal <wt@penguintechs.org> wrote:
>>>
>>> On 4/24/24 11:05 PM, Krzysztof Kozlowski wrote:
>>>> On 25/04/2024 00:01, Wren Turkal wrote:
>>>>>>>
>>>>>>> 3) only care about the case property enable-gpios is not configured,
>>>>>>> the original BT driver design logic indeed matches with binging spec's
>>>>>>> requirements before bartosz's wrong change
>>>>>>
>>>>>> What? There is no such case according to bindings. I told you already
>>>>>> two times: Either change bindings or this is not valid.
>>>>>
>>>>> @krzysztof, I'm curious. There is no entry in the binding specifically
>>>>> for qca6390. Should there be?
>>>>
>>>> qca6390 is documented in the bindings, but you are right that it lacks
>>>> if:then: entry narrowing/choosing specific supplies and pins. It should
>>>> have one, indeed.
>>>
>>> Would creating an entry for the qca6390 hardware fix the issue you are
>>> worried about?
>>>
>>> Again, sorry for all the, what I assume are, basic questions. I am so
>>> far out of my depth here. I am just trying to figure out how to move
>>> forward. I really do appreciate your guidance here.
>>>
>>
>> Wren, Krzysztof: I cannot stop you from doing this but I'm afraid this
>> will complicate the power sequencing work unnecessarily. The QCA6390
>> PMU bindings I'm proposing[1] are consumers of the BT enable GPIOs. In
>> my series I also create an entry for QCA6390 Bluetooth[2] but without
>> enable-gpios and with the inputs from the PMU, not host. Please
>> consider that if you decide to go with this.
> 
> I don't have a near plan to describe QCA6390 supplies and pins, so don't
> worry. I also don't think Qualcomm BT understand what are bindings, so I
> don't expect patches from them.
> 

For property enable-gpios of QCA6390, it is optional so not marked as
required by the dts spec, dts spec is right, i would like to share
history about QCA6390.

1) it was firstly brought-up by Rocky Liao who is the same team as me.
e5d6468fe9d8 Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC
QCA6390

2) for its first product at that time, there are no H/W pin for
enable-gpios, so has issue that BT enable failure after warm reboot. so
i submitted below commit to solve it.
Commit: 7e7bbddd029b ("Bluetooth: hci_qca: Fix qca6390 enable failure
after warm reboot")

3) then Krzysztof Kozlowski submitted below commit to solve
use-after-free issue but also introduced the regression issue
Commit: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
serdev")

4) the issue is reported by Wren recently by below link, and i was
notified to notice this issue and co-work with you to solve it.
https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
https://bugzilla.kernel.org/show_bug.cgi?id=218726

5) i known the root cause when i saw the issue description and have
below formal fix [v7,2/2] and it is verified several times and can
solve the issue reported. and the first debugging change included the
fix logic.
https://patchwork.kernel.org/project/bluetooth/list/?series=847290

6) it does not matter if my fix is not expected, please wait for other
good fix.

7) i really don't want to discuss any more since i really talk two much
as shown by below link, i will disappear for a short of time.
https://lore.kernel.org/linux-bluetooth/1713771497-5733-1-git-send-email-quic_zijuhu@quicinc.com/#t

> Best regards,
> Krzysztof
> 

Re: [PATCH v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855

[Aiqun Yu (Maria)]

On 4/25/2024 6:28 PM, quic_zijuhu wrote:
> On 4/25/2024 5:14 PM, Krzysztof Kozlowski wrote:
>> On 25/04/2024 10:30, Bartosz Golaszewski wrote:
>>> On Thu, 25 Apr 2024 at 08:30, Wren Turkal <wt@penguintechs.org> wrote:
>>>>
>>>> On 4/24/24 11:05 PM, Krzysztof Kozlowski wrote:
>>>>> On 25/04/2024 00:01, Wren Turkal wrote:
>>>>>>>>
>>>>>>>> 3) only care about the case property enable-gpios is not configured,
>>>>>>>> the original BT driver design logic indeed matches with binging spec's
>>>>>>>> requirements before bartosz's wrong change
>>>>>>>
>>>>>>> What? There is no such case according to bindings. I told you already
>>>>>>> two times: Either change bindings or this is not valid.
>>>>>>
>>>>>> @krzysztof, I'm curious. There is no entry in the binding specifically
>>>>>> for qca6390. Should there be?
>>>>>
>>>>> qca6390 is documented in the bindings, but you are right that it lacks
>>>>> if:then: entry narrowing/choosing specific supplies and pins. It should
>>>>> have one, indeed.
>>>>
>>>> Would creating an entry for the qca6390 hardware fix the issue you are
>>>> worried about?
>>>>
>>>> Again, sorry for all the, what I assume are, basic questions. I am so
>>>> far out of my depth here. I am just trying to figure out how to move
>>>> forward. I really do appreciate your guidance here.
>>>>
>>>
>>> Wren, Krzysztof: I cannot stop you from doing this but I'm afraid this
>>> will complicate the power sequencing work unnecessarily. The QCA6390
>>> PMU bindings I'm proposing[1] are consumers of the BT enable GPIOs. In
>>> my series I also create an entry for QCA6390 Bluetooth[2] but without
>>> enable-gpios and with the inputs from the PMU, not host. Please
>>> consider that if you decide to go with this.
>>
>> I don't have a near plan to describe QCA6390 supplies and pins, so don't
>> worry. I also don't think Qualcomm BT understand what are bindings, so I
>> don't expect patches from them.
>>
> 
> For property enable-gpios of QCA6390, it is optional so not marked as
> required by the dts spec, dts spec is right, i would like to share
> history about QCA6390.
History information/links can be given like the change log under --- for
the initial patch set.
It will benefit the reviewers know the histories.
> 
> 1) it was firstly brought-up by Rocky Liao who is the same team as me.
> e5d6468fe9d8 Bluetooth: hci_qca: Add support for Qualcomm Bluetooth SoC
> QCA6390
> 
> 2) for its first product at that time, there are no H/W pin for
> enable-gpios, so has issue that BT enable failure after warm reboot. so
> i submitted below commit to solve it.
> Commit: 7e7bbddd029b ("Bluetooth: hci_qca: Fix qca6390 enable failure
> after warm reboot")
> 
> 3) then Krzysztof Kozlowski submitted below commit to solve
> use-after-free issue but also introduced the regression issue
> Commit: 272970be3dab ("Bluetooth: hci_qca: Fix driver shutdown on closed
> serdev")
Fixes tag can be given in the commit message for a specific commit id.
> 
> 4) the issue is reported by Wren recently by below link, and i was
> notified to notice this issue and co-work with you to solve it.
> https://lore.kernel.org/linux-bluetooth/ea20bb9b-6b60-47fc-ae42-5eed918ad7b4@quicinc.com/T/#m73d6a71d2f454bb03588c66f3ef7912274d37c6f
> https://bugzilla.kernel.org/show_bug.cgi?id=218726
It is suggested to add the Link into the commit message itself.
> 
> 5) i known the root cause when i saw the issue description and have
> below formal fix [v7,2/2] and it is verified several times and can
> solve the issue reported. and the first debugging change included the
> fix logic.
> https://patchwork.kernel.org/project/bluetooth/list/?series=847290
> 
> 6) it does not matter if my fix is not expected, please wait for other
> good fix.
> 
> 7) i really don't want to discuss any more since i really talk two much
> as shown by below link, i will disappear for a short of time.
> https://lore.kernel.org/linux-bluetooth/1713771497-5733-1-git-send-email-quic_zijuhu@quicinc.com/#t
> 
>> Best regards,
>> Krzysztof
>>
> 

-- 
Thx and BRs,
Aiqun(Maria) Yu