On 20/04/2024 07:25, 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 Instead of repeating every time "serious regression" can you actually explain the problem? None of commit messages from v3 help there. > > 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. Sorry, not realized and you did not explain it. Neither above nor in commit msg. > > > 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). What? > > 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. What? > > 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. So where is it documented? Where is it explained? Which binding or which commit msg? > > 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" Please keep all discussions public, unless your customer requires some sort of confidentiality. Although even then I would argue that you can hide company secrets and discuss about hardware. Best regards, Krzysztof
