Re: [PATCH v2] Bluetooth: qca: set power_ctrl_enabled on NULL returned by gpiod_get_optional()

[quic_zijuhu <quic_zijuhu@quicinc.com>]

On 4/24/2024 11:31 PM, Bartosz Golaszewski wrote:
> On Wed, Apr 24, 2024 at 5:30 PM quic_zijuhu <quic_zijuhu@quicinc.com> wrote:
>>
>> On 4/24/2024 11:24 PM, quic_zijuhu wrote:
>>> On 4/24/2024 10:52 PM, Bartosz Golaszewski wrote:
>>>> On Wed, 24 Apr 2024 at 16:46, Krzysztof Kozlowski
>>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>>
>>>>> On 24/04/2024 14:29, Bartosz Golaszewski wrote:
>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>
>>>>>
>>>>>>               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>>>>>>               if (IS_ERR(qcadev->susclk)) {
>>>>>> @@ -2355,10 +2360,13 @@ 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)) {
>>>>>> -                     dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
>>>>>> -                     power_ctrl_enabled = false;
>>>>>> +                     dev_err(&serdev->dev, "failed to acquire enable gpio\n");
>>>>>> +                     return PTR_ERR(qcadev->bt_en);
>>> please think about for QCA2066. if it is returned from here.  BT will
>>> not working at all.  if you don't return here. i will be working fine
>>> for every BT functionality.
>> sorry, correct a type error, it is QCA6390 not QCA2066.
> 
> Doesn't matter. If enable-gpios is not there, gpiod_get_optional()
> will return NULL and we will not return.
> 
i think i need to explain more for my consider case.
let me take QCA6390,  if the property enable-gpios is configured.
but IS_ERR(qcadev->bt_en) case happens, your change will do error
return, so BT will not work at all

but if you don't do error return, BT will working fine.

so your fix is not right regarding QCA6390.

> Bart
> 
>>> NAK again by me.
>>>
>>>>>>               }
>>>>>>
>>>>>> +             if (!qcadev->bt_en)
>>>>>> +                     power_ctrl_enabled = false;
>>>>>
>>>>> This looks duplicated - you already have such check earlier.
>>>>>
>>>>
>>>> It's under a different switch case!
>>>>
>>>> Bartosz
>>>
>>>
>>