Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-3717-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <2a887d94-d5fc-4317-afc8-641d331e35c2@penguintechs.org>
Date: Thu, 18 Apr 2024 01:09:24 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] Revert "Bluetooth: hci_qca: don't use IS_ERR_OR_NULL()
 with gpiod_get_optional()"
To: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>,
 Zijun Hu <quic_zijuhu@quicinc.com>
Cc: luiz.dentz@gmail.com, luiz.von.dentz@intel.com, marcel@holtmann.org,
 linux-bluetooth@vger.kernel.org
References: <1713354823-17826-1-git-send-email-quic_zijuhu@quicinc.com>
 <CACMJSeuQMz-JCK7tqiQCmt7z37L1UncNK0un28JHAa8qRhnp-g@mail.gmail.com>
Content-Language: en-US
From: Wren Turkal <wt@penguintechs.org>
In-Reply-To: <CACMJSeuQMz-JCK7tqiQCmt7z37L1UncNK0un28JHAa8qRhnp-g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 4/17/24 11:59 AM, Bartosz Golaszewski wrote:
> On Wed, 17 Apr 2024 at 13:53, Zijun Hu <quic_zijuhu@quicinc.com> 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,
>> QCA_QCA6390 and QCA_QCA2066, the regression issue is that BT can't be
>> enabled any more once BT is disabled if BT reset pin is not configured
>> by DT or ACPI.
>>
>> if BT reset pin is not configured, devm_gpiod_get_optional() will return
>> NULL, and we should NOT set quirk HCI_QUIRK_NON_PERSISTENT_SETUP, but the
>> reverted commit SET the quirk since NULL is not a error case, and cause
>> qca_setup() call failure triggered by the 2nd and later BT enable
>> operations since there are no available BT reset pin to clear BT firmware
>> downloaded by the 1st enable operation, fixed by reverting the commit.
>>
> 
> Then you just go back to bad usage of the GPIO API. Please see my
> suggestion below.

This revert fixes all the issues I have seen with QCA6390 hardware 
recently. I use a bluetooth mouse, and the hardware hasn't been working 
well for sometime, but really fully broke sometime during the last few 
months. It made my laptop unusable without shutting it down every time I 
wanted to just put it to sleep.

I frankly would prefer the bad usage to live another dev cycle if that's 
what it takes to find a proper solution that doesn't re-break my hardware.

>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218726
>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> ---
>>   drivers/bluetooth/hci_qca.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 92fa20f5ac7d..160175a23a49 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -2323,7 +2323,7 @@ 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");
>> @@ -2332,7 +2332,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>
>>                  qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
>>                                                 GPIOD_IN);
>> -               if (IS_ERR(qcadev->sw_ctrl) &&
>> +               if (IS_ERR_OR_NULL(qcadev->sw_ctrl) &&
>>                      (data->soc_type == QCA_WCN6750 ||
>>                       data->soc_type == QCA_WCN6855 ||
>>                       data->soc_type == QCA_WCN7850))
>> @@ -2354,7 +2354,7 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>>          default:
>>                  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)) {
>>                          dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>                          power_ctrl_enabled = false;
> 
> Can you just move this out of this block? Or just simply check the
> presence of the GPIO descriptor in the if block setting the quirk bit?
> Warning on a missing *optional* GPIO is wrong. It's not an unexpected
> situation, it's normal.

Do you have a proposed patch? I am happy to help test it if that helps 
get to a conclusion.

> 
> Bartosz
> 
>>                  }
>> --
>> 2.7.4
>>

-- 
You're more amazing than you think!