drivers/bluetooth/hci_qca.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Any return value from gpiod_get_optional() other than a pointer to a
GPIO descriptor or a NULL-pointer is an error and the driver should
abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
power_ctrl_enabled on NULL-pointer returned by
devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
While at it: also bail-out on error returned when trying to get the
"swctrl" GPIO.
Reported-by: Wren Turkal <wt@penguintechs.org>
Reported-by: Zijun Hu <quic_zijuhu@quicinc.com>
Closes: https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/
Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
v1 -> v2:
- also restore the previous behavior for QCA6390 and other models that
fall under the default: label in the affected switch case
- bail-out on errors when getting the swctrl GPIO too
drivers/bluetooth/hci_qca.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 92fa20f5ac7d..0e98ad2c0c9d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -2327,16 +2327,21 @@ static int qca_serdev_probe(struct serdev_device *serdev)
(data->soc_type == QCA_WCN6750 ||
data->soc_type == QCA_WCN6855)) {
dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
- power_ctrl_enabled = false;
+ return PTR_ERR(qcadev->bt_en);
}
+ if (!qcadev->bt_en)
+ power_ctrl_enabled = false;
+
qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
GPIOD_IN);
if (IS_ERR(qcadev->sw_ctrl) &&
(data->soc_type == QCA_WCN6750 ||
data->soc_type == QCA_WCN6855 ||
- data->soc_type == QCA_WCN7850))
- dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+ data->soc_type == QCA_WCN7850)) {
+ dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
+ return PTR_ERR(qcadev->sw_ctrl);
+ }
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);
}
+ if (!qcadev->bt_en)
+ power_ctrl_enabled = false;
+
qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
if (IS_ERR(qcadev->susclk)) {
dev_warn(&serdev->dev, "failed to acquire clk\n");
--
2.40.1On 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);
> }
>
> + if (!qcadev->bt_en)
> + power_ctrl_enabled = false;
This looks duplicated - you already have such check earlier.
Best regards,
Krzysztof
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);
> > }
> >
> > + 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
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.
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
On 24/04/2024 17:24, 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
Which is correct. QCA2066 requires GPIO. Look at the bindings.
> not working at all. if you don't return here. i will be working fine
> for every BT functionality.
> NAK again by me.
Yeah, my bad, I taught you that sentence and you keep repeating it.
Best regards,
Krzysztof
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.
> 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
>
>
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.
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
> >
> >
>
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
>>>
>>>
>>
Hi Quic_zijuhu,
On Wed, Apr 24, 2024 at 11:35 AM quic_zijuhu <quic_zijuhu@quicinc.com> wrote:
>
> 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.
Actually I'd say he is probably right with respect to DT because that
seems to require GPIO, we probably need a clearer way to differentiate
if a device is being set up via DT or not (btattach), in any case DT
is probably preferable thus why I went ahead and applied this one.
> > 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
> >>>
> >>>
> >>
>
--
Luiz Augusto von Dentz
On 4/24/2024 11:41 PM, Luiz Augusto von Dentz wrote:
> Hi Quic_zijuhu,
>
> On Wed, Apr 24, 2024 at 11:35 AM quic_zijuhu <quic_zijuhu@quicinc.com> wrote:
>>
>> 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.
>
> Actually I'd say he is probably right with respect to DT because that
> seems to require GPIO, we probably need a clearer way to differentiate
> if a device is being set up via DT or not (btattach), in any case DT
> is probably preferable thus why I went ahead and applied this one.
>
for QCA6390, i can confirm that enable-gpios is optional. it is bring-up
by my team. i can confirm reporter's machine don't config the GPIO pin.
DTS binding spec also don't mark it as required.
that is why i change my changing from initial reverting the whole change
to only focus on QCA6390 that is the machine reported the issue.
>>> 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
>>>>>
>>>>>
>>>>
>>
>
>
On 4/24/2024 11:47 PM, quic_zijuhu wrote:
> On 4/24/2024 11:41 PM, Luiz Augusto von Dentz wrote:
>> Hi Quic_zijuhu,
>>
>> On Wed, Apr 24, 2024 at 11:35 AM quic_zijuhu <quic_zijuhu@quicinc.com> wrote:
>>>
>>> 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.
>>
>> Actually I'd say he is probably right with respect to DT because that
>> seems to require GPIO, we probably need a clearer way to differentiate
>> if a device is being set up via DT or not (btattach), in any case DT
>> is probably preferable thus why I went ahead and applied this one.
>>
> for QCA6390, i can confirm that enable-gpios is optional. it is bring-up
> by my team. i can confirm reporter's machine don't config the GPIO pin.
> DTS binding spec also don't mark it as required.
>
> that is why i change my changing from initial reverting the whole change
> to only focus on QCA6390 that is the machine reported the issue.
>
>
>>>> 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
>>>>>>
>>>>>>
>>>>>
>>>
>>
>>
>
>
i find the change have been merged.
i think it is not good manner to merge change in this way even if i give
reasonable doubt
On Wed, Apr 24, 2024 at 5:24 PM quic_zijuhu <quic_zijuhu@quicinc.com> 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.
> NAK again by me.
>
Luiz,
This in turn is an example of Zijun making a claim that looks like a
legitimate review but is simply untrue. He's done it several times.
I'm afraid that it may affect your judgment due to the confidence the
claims are made with. As Krzysztof said multiple times: the
device-tree bindings for QCA2066 are very clear: the enable-gpios
property is *required* and so returning an error on failure here is
correct. Even changing gpiod_get_optional() to just gpiod_get() would
be in line with what the contract in the binding document says. Even
if we relaxed the bindings, returning here stil *IS CORRECT* as if the
enable-gpios is not defined, GPIOLIB will return NULL and we will NOT
return.
Bartosz
> >>> }
> >>>
> >>> + 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
>
On 24/04/2024 16:52, 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);
>>> }
>>>
>>> + if (!qcadev->bt_en)
>>> + power_ctrl_enabled = false;
>>
>> This looks duplicated - you already have such check earlier.
>>
>
> It's under a different switch case!
Damn diff. Yeah, looks ok.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
>
> Any return value from gpiod_get_optional() other than a pointer to a
> GPIO descriptor or a NULL-pointer is an error and the driver should
> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> power_ctrl_enabled on NULL-pointer returned by
> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> While at it: also bail-out on error returned when trying to get the
> "swctrl" GPIO.
>
> Reported-by: Wren Turkal<wt@penguintechs.org>
> Reported-by: Zijun Hu<quic_zijuhu@quicinc.com>
> Closes:https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/
> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
> Signed-off-by: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
Tested-by: "Wren Turkal" <wt@penguintechs.org>
Like this?
wt
--
You're more amazing than you think!
On Wed, 24 Apr 2024 at 15:10, Wren Turkal <wt@penguintechs.org> wrote:
>
> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
> >
> > Any return value from gpiod_get_optional() other than a pointer to a
> > GPIO descriptor or a NULL-pointer is an error and the driver should
> > abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> > don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> > power_ctrl_enabled on NULL-pointer returned by
> > devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> > While at it: also bail-out on error returned when trying to get the
> > "swctrl" GPIO.
> >
> > Reported-by: Wren Turkal<wt@penguintechs.org>
> > Reported-by: Zijun Hu<quic_zijuhu@quicinc.com>
> > Closes:https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/
> > Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> > Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
>
> Tested-by: "Wren Turkal" <wt@penguintechs.org>
>
>
> Like this?
Yes, awesome, thanks.
This is how reviewing works too in the kernel, look at what Krzysztof
did under v1, he just wrote:
Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
And mailing list tools will pick it up.
Bartosz
On 4/24/2024 9:18 PM, Bartosz Golaszewski wrote:
> On Wed, 24 Apr 2024 at 15:10, Wren Turkal <wt@penguintechs.org> wrote:
>>
>> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
>>>
>>> Any return value from gpiod_get_optional() other than a pointer to a
>>> GPIO descriptor or a NULL-pointer is an error and the driver should
>>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
>>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
>>> power_ctrl_enabled on NULL-pointer returned by
>>> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
>>> While at it: also bail-out on error returned when trying to get the
>>> "swctrl" GPIO.
>>>
>>> Reported-by: Wren Turkal<wt@penguintechs.org>
>>> Reported-by: Zijun Hu<quic_zijuhu@quicinc.com>
>>> Closes:https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/
>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>>> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
>>> Signed-off-by: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
>>
>> Tested-by: "Wren Turkal" <wt@penguintechs.org>
>>
>>
>> Like this?
>
> Yes, awesome, thanks.
>
> This is how reviewing works too in the kernel, look at what Krzysztof
> did under v1, he just wrote:
>
> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
>
v1 have obvious something wrong as i pointed and verified.
so i think it is not suitable to attach v1's review-by tag to v2 anyway.
> And mailing list tools will pick it up.
>
> Bartosz
On 4/24/24 6:22 AM, quic_zijuhu wrote:
> On 4/24/2024 9:18 PM, Bartosz Golaszewski wrote:
>> On Wed, 24 Apr 2024 at 15:10, Wren Turkal <wt@penguintechs.org> wrote:
>>>
>>> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
>>>> From: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
>>>>
>>>> Any return value from gpiod_get_optional() other than a pointer to a
>>>> GPIO descriptor or a NULL-pointer is an error and the driver should
>>>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
>>>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
>>>> power_ctrl_enabled on NULL-pointer returned by
>>>> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
>>>> While at it: also bail-out on error returned when trying to get the
>>>> "swctrl" GPIO.
>>>>
>>>> Reported-by: Wren Turkal<wt@penguintechs.org>
>>>> Reported-by: Zijun Hu<quic_zijuhu@quicinc.com>
>>>> Closes:https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/
>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>>>> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
>>>> Signed-off-by: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
>>>
>>> Tested-by: "Wren Turkal" <wt@penguintechs.org>
>>>
>>>
>>> Like this?
>>
>> Yes, awesome, thanks.
>>
>> This is how reviewing works too in the kernel, look at what Krzysztof
>> did under v1, he just wrote:
>>
>> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
>>
> v1 have obvious something wrong as i pointed and verified.
> so i think it is not suitable to attach v1's review-by tag to v2 anyway.
@Zijun, your concern is that current DTs may not define the gpio and
this will cause the bluetooth not to work?
Would that not more appropriately be fixed by machine-specific fixups
for the DT?
>
>> And mailing list tools will pick it up.
>>
>> Bartosz
>
--
You're more amazing than you think!
On Wed, Apr 24, 2024 at 3:31 PM Wren Turkal <wt@penguintechs.org> wrote:
>
> On 4/24/24 6:22 AM, quic_zijuhu wrote:
> > On 4/24/2024 9:18 PM, Bartosz Golaszewski wrote:
> >> On Wed, 24 Apr 2024 at 15:10, Wren Turkal <wt@penguintechs.org> wrote:
> >>>
> >>> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
> >>>> From: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
> >>>>
> >>>> Any return value from gpiod_get_optional() other than a pointer to a
> >>>> GPIO descriptor or a NULL-pointer is an error and the driver should
> >>>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> >>>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> >>>> power_ctrl_enabled on NULL-pointer returned by
> >>>> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> >>>> While at it: also bail-out on error returned when trying to get the
> >>>> "swctrl" GPIO.
> >>>>
> >>>> Reported-by: Wren Turkal<wt@penguintechs.org>
> >>>> Reported-by: Zijun Hu<quic_zijuhu@quicinc.com>
> >>>> Closes:https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/
> >>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> >>>> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
> >>>> Signed-off-by: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
> >>>
> >>> Tested-by: "Wren Turkal" <wt@penguintechs.org>
> >>>
> >>>
> >>> Like this?
> >>
> >> Yes, awesome, thanks.
> >>
> >> This is how reviewing works too in the kernel, look at what Krzysztof
> >> did under v1, he just wrote:
> >>
> >> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
> >>
> > v1 have obvious something wrong as i pointed and verified.
> > so i think it is not suitable to attach v1's review-by tag to v2 anyway.
>
> @Zijun, your concern is that current DTs may not define the gpio and
> this will cause the bluetooth not to work?
>
This is simply not true. If the GPIO is not specified,
gpiod_get_optional() will return NULL and GPIO APIs will work just
fine.
That being said: the contract for whether a GPIO is needed or not is
not in the driver C code or released DT sources. It's in the bindings
documents under Documentation/devicetree/bindings/. This is the source
of truth. So if the binding for a given model has always said
"required: enable-gpios" then we're absolutely in our rights to fix
the driver to conform to that even if previously omitted. If you think
otherwise - relax the bindings first.
Bart
> Would that not more appropriately be fixed by machine-specific fixups
> for the DT?
>
> >
> >> And mailing list tools will pick it up.
> >>
> >> Bartosz
> >
>
> --
> You're more amazing than you think!
On 4/24/2024 9:31 PM, Wren Turkal wrote:
> On 4/24/24 6:22 AM, quic_zijuhu wrote:
>> On 4/24/2024 9:18 PM, Bartosz Golaszewski wrote:
>>> On Wed, 24 Apr 2024 at 15:10, Wren Turkal <wt@penguintechs.org> wrote:
>>>>
>>>> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
>>>>> From: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
>>>>>
>>>>> Any return value from gpiod_get_optional() other than a pointer to a
>>>>> GPIO descriptor or a NULL-pointer is an error and the driver should
>>>>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth:
>>>>> hci_qca:
>>>>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
>>>>> power_ctrl_enabled on NULL-pointer returned by
>>>>> devm_gpiod_get_optional(). Restore this behavior but bail-out on
>>>>> errors.
>>>>> While at it: also bail-out on error returned when trying to get the
>>>>> "swctrl" GPIO.
>>>>>
>>>>> Reported-by: Wren Turkal<wt@penguintechs.org>
>>>>> Reported-by: Zijun Hu<quic_zijuhu@quicinc.com>
>>>>> Closes:https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/
>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use
>>>>> IS_ERR_OR_NULL() with gpiod_get_optional()")
>>>>> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
>>>>> Signed-off-by: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
>>>>
>>>> Tested-by: "Wren Turkal" <wt@penguintechs.org>
>>>>
>>>>
>>>> Like this?
>>>
>>> Yes, awesome, thanks.
>>>
>>> This is how reviewing works too in the kernel, look at what Krzysztof
>>> did under v1, he just wrote:
>>>
>>> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
>>>
>> v1 have obvious something wrong as i pointed and verified.
>> so i think it is not suitable to attach v1's review-by tag to v2 anyway.
>
> @Zijun, your concern is that current DTs may not define the gpio and
> this will cause the bluetooth not to work?
>
> Would that not more appropriately be fixed by machine-specific fixups
> for the DT?
>
for lunched production, it is difficult or not possible to change such
config.
>>
>>> And mailing list tools will pick it up.
>>>
>>> Bartosz
>>
>
On 4/24/24 6:36 AM, quic_zijuhu wrote:
> On 4/24/2024 9:31 PM, Wren Turkal wrote:
>> On 4/24/24 6:22 AM, quic_zijuhu wrote:
>>> On 4/24/2024 9:18 PM, Bartosz Golaszewski wrote:
>>>> On Wed, 24 Apr 2024 at 15:10, Wren Turkal <wt@penguintechs.org> wrote:
>>>>>
>>>>> On 4/24/24 5:29 AM, Bartosz Golaszewski wrote:
>>>>>> From: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
>>>>>>
>>>>>> Any return value from gpiod_get_optional() other than a pointer to a
>>>>>> GPIO descriptor or a NULL-pointer is an error and the driver should
>>>>>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth:
>>>>>> hci_qca:
>>>>>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
>>>>>> power_ctrl_enabled on NULL-pointer returned by
>>>>>> devm_gpiod_get_optional(). Restore this behavior but bail-out on
>>>>>> errors.
>>>>>> While at it: also bail-out on error returned when trying to get the
>>>>>> "swctrl" GPIO.
>>>>>>
>>>>>> Reported-by: Wren Turkal<wt@penguintechs.org>
>>>>>> Reported-by: Zijun Hu<quic_zijuhu@quicinc.com>
>>>>>> Closes:https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/
>>>>>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use
>>>>>> IS_ERR_OR_NULL() with gpiod_get_optional()")
>>>>>> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
>>>>>> Signed-off-by: Bartosz Golaszewski<bartosz.golaszewski@linaro.org>
>>>>>
>>>>> Tested-by: "Wren Turkal" <wt@penguintechs.org>
>>>>>
>>>>>
>>>>> Like this?
>>>>
>>>> Yes, awesome, thanks.
>>>>
>>>> This is how reviewing works too in the kernel, look at what Krzysztof
>>>> did under v1, he just wrote:
>>>>
>>>> Reviewed-by: Krzysztof Kozlowski<krzysztof.kozlowski@linaro.org>
>>>>
>>> v1 have obvious something wrong as i pointed and verified.
>>> so i think it is not suitable to attach v1's review-by tag to v2 anyway.
>>
>> @Zijun, your concern is that current DTs may not define the gpio and
>> this will cause the bluetooth not to work?
>>
>> Would that not more appropriately be fixed by machine-specific fixups
>> for the DT?
>>
> for lunched production, it is difficult or not possible to change such
> config.
I am not talking about the DT in the device. I am talking about the
mechanism the kernel has for applying fixups to DTs.
If a dev builds a new kernel for a dev and finds it not to work, the
kernel would then have a fixup added, like described here:
https://docs.kernel.org/devicetree/usage-model.html#platform-identification
>
>>>
>>>> And mailing list tools will pick it up.
>>>>
>>>> Bartosz
>>>
>>
>
--
You're more amazing than you think!
On 4/24/2024 8:29 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Any return value from gpiod_get_optional() other than a pointer to a
> GPIO descriptor or a NULL-pointer is an error and the driver should
> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> power_ctrl_enabled on NULL-pointer returned by
> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> While at it: also bail-out on error returned when trying to get the
> "swctrl" GPIO.
>
> Reported-by: Wren Turkal <wt@penguintechs.org>
> Reported-by: Zijun Hu <quic_zijuhu@quicinc.com>
> Closes: https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/
> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
is it really reviewed-by Krzysztof?
suggest reviewer give explicit review-by tag by public way, then you add
this tag.
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> v1 -> v2:
> - also restore the previous behavior for QCA6390 and other models that
> fall under the default: label in the affected switch case
> - bail-out on errors when getting the swctrl GPIO too
>
> drivers/bluetooth/hci_qca.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 92fa20f5ac7d..0e98ad2c0c9d 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2327,16 +2327,21 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> (data->soc_type == QCA_WCN6750 ||
> data->soc_type == QCA_WCN6855)) {
> dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
> - power_ctrl_enabled = false;
> +
think about what will happen for present lunched products if this type
error really happens.
BT don't work at all with your change. BT can be used mostly without
your change.
return PTR_ERR(qcadev->bt_en);
> }
>
> + if (!qcadev->bt_en)
> + power_ctrl_enabled = false;
> +
you don't answer me how to treat a required enable is not configured by user
> qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
> GPIOD_IN);
> if (IS_ERR(qcadev->sw_ctrl) &&
> (data->soc_type == QCA_WCN6750 ||
> data->soc_type == QCA_WCN6855 ||
> - data->soc_type == QCA_WCN7850))
> - dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
> + data->soc_type == QCA_WCN7850)) {
> + dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
> + return PTR_ERR(qcadev->sw_ctrl);have the same question as above.
> + }
>
> 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);
> }
>
have the same question as above.
is it right for such prompt ?
> + if (!qcadev->bt_en)
> + power_ctrl_enabled = false;
> +
> qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
> if (IS_ERR(qcadev->susclk)) {
> dev_warn(&serdev->dev, "failed to acquire clk\n");
have the same question as above.
how do you known the root cause of the issue reported without my earlier
debugging and fix?
do my fix regarding the issue i concerned have any fault?
NAK by me.
On 4/24/24 5:59 AM, quic_zijuhu wrote:
> On 4/24/2024 8:29 PM, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> Any return value from gpiod_get_optional() other than a pointer to a
>> GPIO descriptor or a NULL-pointer is an error and the driver should
>> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
>> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
>> power_ctrl_enabled on NULL-pointer returned by
>> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
>> While at it: also bail-out on error returned when trying to get the
>> "swctrl" GPIO.
>>
>> Reported-by: Wren Turkal <wt@penguintechs.org>
>> Reported-by: Zijun Hu <quic_zijuhu@quicinc.com>
>> Closes: https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/
>> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> is it really reviewed-by Krzysztof?
> suggest reviewer give explicit review-by tag by public way, then you add
> this tag.
>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>> ---
>> v1 -> v2:
>> - also restore the previous behavior for QCA6390 and other models that
>> fall under the default: label in the affected switch case
>> - bail-out on errors when getting the swctrl GPIO too
>>
>> drivers/bluetooth/hci_qca.c | 18 +++++++++++++-----
>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> index 92fa20f5ac7d..0e98ad2c0c9d 100644
>> --- a/drivers/bluetooth/hci_qca.c
>> +++ b/drivers/bluetooth/hci_qca.c
>> @@ -2327,16 +2327,21 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>> (data->soc_type == QCA_WCN6750 ||
>> data->soc_type == QCA_WCN6855)) {
>> dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>> - power_ctrl_enabled = false;
>> +
> think about what will happen for present lunched products if this type
> error really happens.
> BT don't work at all with your change. BT can be used mostly without
> your change.
> return PTR_ERR(qcadev->bt_en);
>> }
>>
>> + if (!qcadev->bt_en)
>> + power_ctrl_enabled = false;
>> +
> you don't answer me how to treat a required enable is not configured by user
>> qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
>> GPIOD_IN);
>> if (IS_ERR(qcadev->sw_ctrl) &&
>> (data->soc_type == QCA_WCN6750 ||
>> data->soc_type == QCA_WCN6855 ||
>> - data->soc_type == QCA_WCN7850))
>> - dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>> + data->soc_type == QCA_WCN7850)) {
>> + dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>> + return PTR_ERR(qcadev->sw_ctrl);have the same question as above.
>> + }
>>
>> 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);
>> }
>>
> have the same question as above.
> is it right for such prompt ?
>> + if (!qcadev->bt_en)
>> + power_ctrl_enabled = false;
>> +
>> qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
>> if (IS_ERR(qcadev->susclk)) {
>> dev_warn(&serdev->dev, "failed to acquire clk\n");
> have the same question as above.
>
> how do you known the root cause of the issue reported without my earlier
> debugging and fix?
Without your debugging, this fix would not have been possible.
>
> do my fix regarding the issue i concerned have any fault?
>
> NAK by me.
>
>
>
>
--
You're more amazing than you think!
On 4/24/2024 8:29 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Any return value from gpiod_get_optional() other than a pointer to a
> GPIO descriptor or a NULL-pointer is an error and the driver should
> abort probing. That being said: commit 56d074d26c58 ("Bluetooth: hci_qca:
> don't use IS_ERR_OR_NULL() with gpiod_get_optional()") no longer sets
> power_ctrl_enabled on NULL-pointer returned by
> devm_gpiod_get_optional(). Restore this behavior but bail-out on errors.
> While at it: also bail-out on error returned when trying to get the
> "swctrl" GPIO.
>
> Reported-by: Wren Turkal <wt@penguintechs.org>
> Reported-by: Zijun Hu <quic_zijuhu@quicinc.com>
> Closes: https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@quicinc.com/
> Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> v1 -> v2:
> - also restore the previous behavior for QCA6390 and other models that
> fall under the default: label in the affected switch case
> - bail-out on errors when getting the swctrl GPIO too
>
> drivers/bluetooth/hci_qca.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 92fa20f5ac7d..0e98ad2c0c9d 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -2327,16 +2327,21 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> (data->soc_type == QCA_WCN6750 ||
> data->soc_type == QCA_WCN6855)) {
> dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
> - power_ctrl_enabled = false;
> + return PTR_ERR(qcadev->bt_en);
> }
>
> + if (!qcadev->bt_en)
> + power_ctrl_enabled = false;
> +
> qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
> GPIOD_IN);
> if (IS_ERR(qcadev->sw_ctrl) &&
> (data->soc_type == QCA_WCN6750 ||
> data->soc_type == QCA_WCN6855 ||
> - data->soc_type == QCA_WCN7850))
> - dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
> + data->soc_type == QCA_WCN7850)) {
> + dev_err(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
> + return PTR_ERR(qcadev->sw_ctrl);
> + }
>
> 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);
> }
>
> + if (!qcadev->bt_en)
> + power_ctrl_enabled = false;
> +
> qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
> if (IS_ERR(qcadev->susclk)) {
> dev_warn(&serdev->dev, "failed to acquire clk\n");NAK, you don't answer my question for your v1 before send v2
© 2016 - 2026 Red Hat, Inc.