| Message ID | 1713947712-4307-1-git-send-email-quic_zijuhu@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] Bluetooth: qca: Correct property enable-gpios handling logic for WCN6750 and WCN6855 | expand |
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 >> >
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; }
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(-)