Re: [PATCH v1 1/2] Revert "Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()"

[quic_zijuhu <quic_zijuhu@quicinc.com>]

On 4/20/2024 7:13 PM, Krzysztof Kozlowski wrote:
> 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.
> 
as shown by below link
https://lore.kernel.org/all/1713650800-29741-2-git-send-email-quic_zijuhu@quicinc.com/

there are no v3 patch for patch serial with this tile
the updated patch serial tile is "Fix two regression issues for QCA
controllers" shown by above mentioned link.

let us discuss it with the updated one.

v3 of the updated one has good commit message for this issue. you have
given reply with the v3 and it seems you understand what is the problem

>>
>> 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@bgdev.pl/#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.
> 
now. you understood why your merged change as shown link of 4) have
problems and introduced our discussed issue, right?

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