Re: [PATCH v1 1/2] Revert "Bluetooth: hci_qca: don't use IS_ERR_OR_NULL() with gpiod_get_optional()"

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


On 20/04/2024 07:25, 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

Instead of repeating every time "serious regression" can you actually
explain the problem?
None of commit messages from v3 help there.

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

Sorry, not realized and you did not explain it. Neither above nor in
commit msg.

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

So where is it documented? Where is it explained? Which binding or which
commit msg?

> 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"

Please keep all discussions public, unless your customer requires some
sort of confidentiality. Although even then I would argue that you can
hide company secrets and discuss about hardware.

Best regards,

[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