On 4/20/2024 1:25 PM, quic_zijuhu wrote: > On 4/19/2024 9:49 PM, Krzysztof Kozlowski wrote: > > Hi Krzysztof,bartosz, > > let me summarize our discussion here in order to reduce unneccessary > disagreements here. > > 1) i only revert your change IS_ERR() to my change IS_ERR_OR_NULL. > > 2) your change will cause serious regression issues for many lunched > products > > 3) we only need to discuss how to handle devm_gpiod_get_optional(..., > "enable", ...) returning NULL since this is only difference between your > change and mine. > > 4) your change doesn't solve any actual issue and the reason you > submitted is that "The optional variants for the gpiod_get() family of > functions return NULL if the GPIO in question is not associated with > this device, and should not treat it as error". > > code applet of your merged change is shown by below link > https://patchwork.kernel.org/project/bluetooth/patch/20240208164017.26699-1-brgl@xxxxxxxx/#25705104 > > qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable", > GPIOD_OUT_LOW); > - if (IS_ERR_OR_NULL(qcadev->bt_en)) { > + if (IS_ERR(qcadev->bt_en)) { > dev_warn(&serdev->dev, "failed to acquire enable gpio\n"); > power_ctrl_enabled = false; > } > > 5) Original BT driver design agree with your point mentioned at 4), so > for case "qcadev->bt_en == nullptr", qca_serdev_probe() don't do error > return for this scenario and use dev_warn() instead of dev_err() to give > user prompt. > > 6) your wrong fix changes flag power_ctrl_enabled set logic and will > cause serious BT regression issue, hope you will realize this point. > > > i would like to give below extra comments even if these comments are > irrelevant to the critical point of this issue mentioned at above 3) > > A) you need to investigate it is a) the prompting approach or message > error or b) the if condition error even if if dev_err() is used to give > prompt instead of dev_warn() in above 4). > > B) don't talk about how about devm_gpiod_get_optional() returning error > case since it is meaningless as explained by above 3). also don't > require a fix to fix another unreported issue. a fix is a good fix > if it fix the issue in question and don't introduce new issue. > > C) per DTS property enable-gpios of BT, different soc types have > different requirements, many are required and another many are NOT > mandatory as shown be below link. > https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/Documentation/devicetree/bindings/net/bluetooth/qualcomm-bluetooth.yaml. > > for a soc type which are attached to 3rd platform, customer doesn't > would like to or are not able to congfig BT reset pin within DTS for QCA > driver even if QC strongly suggest customer config it and also be marked > as required within above DTS bindings spec link. i often meet this > scenario. there are many of such lunched products. > > i will try to fix this issue due your change product by product in new > patch thread based on this DTS comment. > > D) you maybe ping me offline about this issue if you are a member of QC > since you known "go/upstream" > >> On 19/04/2024 01:17, quic_zijuhu wrote: >>> On 4/19/2024 6:37 AM, Krzysztof Kozlowski wrote: >>>> On 18/04/2024 23:16, quic_zijuhu wrote: >>>>> On 4/19/2024 12:52 AM, Krzysztof Kozlowski wrote: >>>>>> On 18/04/2024 16:06, Zijun Hu wrote: >>>>>>> This reverts commit 56d074d26c5828773b00b2185dd7e1d08273b8e8. >>>>>>> >>>>>>> Commit 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() >>>>>>> with gpiod_get_optional()") will cause serious regression issue for >>>>>>> several QCA controllers such as QCA_WCN6750,QCA_WCN6855,QCA_ROME, >>>>>> >>>>>> The pin is required on 6750, 6855 and maybe others. You cannot not have >>>>>> the GPIO. >>>>>> >>>>>> This is no correct fix. You provide wrong DTS and, instead fixing it, >>>>>> try to revert kernel code. >>>>>> >>>>>> No, fix your DTS first. >>>>>> >>>>> no. your point is not right. >>>>> >>>>> 1) do you have any evidence that the hci_qca driver must use reset GPIO? >>>> >>>> I think we talk here about enable-gpios, right? Then the evidence are >>>> bindings. >>>> >>> yes. properties within bindings only means driver supporting it, don't means user must >>> config it. the gpio is got by devm_gpiod_get_optional() variant. that means it is optional >>> about if user need to config it. >> >> No. Read writing bindings and other presentations explaining what are >> Devicetree bindings. >> >> You miss entirely the point and use downstream narrative. This won't >> work and it was told so many times, that I expect you to do the homework >> first. >> >> Use "go/upstream" before posting more on this topic. >> >> >>>>> 2) why does original design do error return when get GPIO error if GPIO is mandatory? >>>> >>>> If GPIO is mandatory, then it is expected to return error. What is the >>>> problem here? >>>> >>> sorry, i miss a NOT for my question. my question is that >>> 2) why does original design NOT do error return when get GPIO error if GPIO is mandatory? >>>> >>>>> 3) i meet many customer cases that BT are working fine without hci_qca operating the GPIO, >>>>> there is why HCI_QUIRK_NON_PERSISTENT_SETUP are introduced. >>>> >>>> Bindings tell different story and nothing in the commit msg explained >>>> this. You did not correct bindings either. >>>> >>> don't need to correct bindings. i believe bindings does not say enable gpio >>> must be configured. >> >> They say. Read the bindings. Test your DTS. Or better: upstream your DTS >> and prove to us that dtbs_check allows lack of enable-gpios. >> >> >>>> >>>>> 4) does the reverted change solve the issue your mentioned ? >>>> >>>> ??? I did not mention any issue. I am saying that your rationale is >>>> either not complete or not correct. >>>> >>> do you suggest about how to make it complete? >> >> Yes, read what are bindings and then describe your change including >> that: what is the issue, how it can be reproduced, what is the hardware, >> why the bindings are not correct (if they are not correct) etc. >> >> >> Best regards, >> Krzysztof >> >
