Message-ID: <36765047-7b88-4899-966c-e4c4362127ff@kernel.org>
Date: Fri, 13 Jun 2025 08:13:50 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Zhangchao Zhang <ot_zhangchao.zhang@...iatek.com>,
 Marcel Holtmann <marcel@...tmann.org>,
 Johan Hedberg <johan.hedberg@...il.com>,
 Luiz Von Dentz <luiz.dentz@...il.com>
Cc: Sean Wang <sean.wang@...iatek.com>, Deren Wu <deren.Wu@...iatek.com>,
 Aaron Hou <aaron.hou@...iatek.com>, Chris Lu <chris.lu@...iatek.com>,
 Hao Qin <Hao.qin@...iatek.com>,
 linux-bluetooth <linux-bluetooth@...r.kernel.org>,
 linux-kernel <linux-kernel@...r.kernel.org>,
 linux-mediatek <linux-mediatek@...ts.infradead.org>
Subject: Re: [PATCH v2] Bluetooth: mediatek: add gpio pin to reset bt

On 12/06/2025 09:22, Zhangchao Zhang wrote:
> This V2 patch provides two methods btmtk_reset_by_gpio,

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95


And v2 makes no sense here. Describe the commit, why you are doing this,
what you are doing here, not your process.

> btmtk_reset_by_gpio_work for mediatek controller,
> it has been tested locally many times and can reset normally.
> 
> The pin is configured in dts files, bluetooth is reset by pulling

That's redundant. Do not explain us how DTS works. We all know it.
Explain WHY you are doing this, what sort of problem or hardware you are
dealing here with.

> the pin, when exception or coredump occurs, the above methods will
> be used to reset the bluetooth, if the pin is not found, it also can
> reset bluetooth successfully by software reset.
> 
> Compared with the previously submitted version, the following
> information has been revised in version V2

This goes to changelog or cover letter. See submitting patches.

And that's a v3, because you already sent v2.


> 1)-Changed the capitalization of co-developer names,
>    using the correct capitalization of abbreviations and full
>    name, and corrected obvious spelling errors.
> 2)-Add a revision history.
> 3)-Remove the "BT Driver" in the prefix.
> 4)-Add the bt-binding document, include inforamtion related to reset
>    pin and compatibility matching.
> 5)-Add a comment before the schedule_delayed_work function call,
>    although schedule_delayed_work is asynchronous, there is no risk.
>    Even if it is not completed within 200ms, it will only postpone
>    the subsequent probe and will not have any impact.
> 6-)Add a comment before the btmtk_reset_by_gpio function call,
>    if the compatibility filed or pin cannot be found in the dts
>    files, it can still reset bluetooth using software reset.
> 
> Co-developed-by Hao Qin <hao.qin@...iatek.com>
> Co-developed-Chris Lu <chris.lu@...iatek.com>
> Co-developed-Jiande Lu <jiande.lu@...iatek.com>
> Signed-off-by: Zhangchao Zhang <ot_zhangchao.zhang@...iatek.com>
> ---
>  .../bluetooth/mediatek,mt7925-bluetooth.yaml  | 54 +++++++++++++++

Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

>  drivers/bluetooth/btmtk.c                     | 69 +++++++++++++++++++
>  drivers/bluetooth/btmtk.h                     |  5 ++
>  3 files changed, 128 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7925-bluetooth.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7925-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7925-bluetooth.yaml
> new file mode 100644
> index 000000000000..bcdb46effdba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7925-bluetooth.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/bluetooth/mediatek,mt7925-bluetooth.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek MT7925 Bluetooth
> +
> +maintainers:
> +  - Zhangchao Zhang <zhangchao.zhang@...iatek.com>
> +
> +description:
> +  7925 uses the USB bus to communicate with the host.
> +  Two methods are used to reset Bluetooth.
> +  When Bluetooth crashes or core dumps,
> +  the pin will be pulled low, then held for 200ms,
> +  and then pulled high again before next probe.
> +
> +allOf:
> +  - $ref: bluetooth-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt7925-bluetooth
> +
> +  reg:
> +    const: 2
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      An active-high reset pin for the Bluetooth core;

Blank line and drop final ;

> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    &xhci0 {
> +      vbus-supply = <&pp5000_usb>;
> +      usb2-lpm-disable;

Drop node above

> +
> +      status = "okay";

Drop

> +
> +      bt_reset: bt-reset{

Messy style, missing space. See DTS coding style.

> +        compatible = "mediatek,usb-bluetooth";

Look at your binding which you *just* wrote.

> +        reset-gpios = <&pio 248 GPIO_ACTIVE_HIGH>;

That's so incomplete... and will fail tests. You need to write COMPLETE
binding and then COMPLETE example.

> +      };
> +    };
> \ No newline at end of file

You have patch warnings.


> diff --git a/drivers/bluetooth/btmtk.c b/drivers/bluetooth/btmtk.c
> index 4390fd571dbd..3e5f3ca6f0d5 100644
> --- a/drivers/bluetooth/btmtk.c
> +++ b/drivers/bluetooth/btmtk.c
> @@ -6,6 +6,8 @@
>  #include <linux/firmware.h>
>  #include <linux/usb.h>
>  #include <linux/iopoll.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>  #include <linux/unaligned.h>
>  
>  #include <net/bluetooth/bluetooth.h>
> @@ -109,6 +111,65 @@ static void btmtk_coredump_notify(struct hci_dev *hdev, int state)
>  	}
>  }
>  
> +static void btmtk_reset_by_gpio_work(struct work_struct *work)
> +{
> +	struct btmtk_reset_gpio *reset_gpio_data =
> +			container_of(work, struct btmtk_reset_gpio, reset_work.work);
> +
> +	gpio_direction_output(reset_gpio_data->gpio_number, 1);
> +	kfree(reset_gpio_data);
> +}
> +
> +static int btmtk_reset_by_gpio(struct hci_dev *hdev)
> +{
> +	struct btmtk_data *data = hci_get_priv(hdev);
> +	struct btmtk_reset_gpio *reset_gpio_data;
> +	struct device_node *node;
> +	int reset_gpio_number;
> +
> +	node = of_find_compatible_node(NULL, NULL, "mediatek,usb-bluetooth");

Same comment as before. Nothing improved.

Best regards,
Krzysztof

Powered by blists - more mailing lists