Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3791-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <c1138d6a-83a9-43ec-9c49-62d59884b41c@quicinc.com>
Date: Sat, 20 Apr 2024 13:27:37 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 1/2] Revert "Bluetooth: hci_qca: don't use
 IS_ERR_OR_NULL() with gpiod_get_optional()"
From: quic_zijuhu <quic_zijuhu@quicinc.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <luiz.dentz@gmail.com>, <luiz.von.dentz@intel.com>,
        <marcel@holtmann.org>
CC: <linux-bluetooth@vger.kernel.org>, <wt@penguintechs.org>,
        <bartosz.golaszewski@linaro.org>, <kernel@quicinc.com>
References: <1713449192-25926-1-git-send-email-quic_zijuhu@quicinc.com>
 <1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com>
 <83a1c837-e403-4da2-83c6-ee3dedbc1fe0@linaro.org>
 <7c5b85ca-e897-4798-97e7-955478c57d12@quicinc.com>
 <52394d97-04c3-4f77-aaae-f1e152cd5632@linaro.org>
 <0dca7c9b-6a97-4171-9b0c-5b806a4e8a60@quicinc.com>
 <4e4d8ee0-fbbe-4dc7-b40b-b983f9f39539@linaro.org>
 <01677a26-ea91-47cc-bdc4-283cf313d8e4@quicinc.com>
Content-Language: en-US
In-Reply-To: <01677a26-ea91-47cc-bdc4-283cf313d8e4@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

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