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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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@xxxxxxxxxxxxxxxx>
Reported-by: Zijun Hu <quic_zijuhu@xxxxxxxxxxx>
Closes: https://lore.kernel.org/linux-bluetooth/1713449192-25926-2-git-send-email-quic_zijuhu@xxxxxxxxxxx/
Fixes: 56d074d26c58 ("Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()")
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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!




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux