[PATCH v2 0/2] bluetooth: mt7921s: Add binding and fixup existing dts

[PATCH v2 0/2] bluetooth: mt7921s: Add binding and fixup existing dts

[Chen-Yu Tsai]

Hi everyone,

This is v2 of my MT7921S Bluetooth binding series.

Changes since v1:
- Reworded descriptions in binding
- Moved binding maintainer section before binding description
- Added missing reference to bluetooth-controller.yaml
- Added missing GPIO header to example

This short series adds a binding document for the MT7921S SDIO Bluetooth
controller. The MT7921S is a SDIO-based WiFi/Bluetooth combo. WiFi and
Bluetooth are separate SDIO functions. The chip has extra per-subsystem
reset lines that can reset only WiFi or Bluetooth cores.

Patch 1 documents the SDIO function and the reset line, based on
existing device tree and driver usage. I listed Sean Wang, the original
driver author and maintainer, as the maintainer of the binding.

Patch 2 fixes up the sole existing usage of the compatible string by
making it a proper SDIO function node.

Please take a look. Not sure which tree patch 1 should be merged
through? I suppose with proper acks it could go through the soc/mediatek
tree together with patch 2.


Regards
ChenYu


Chen-Yu Tsai (2):
  dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth
  arm64: dts: mediatek: mt8183-pico6: Fix bluetooth node

 .../bluetooth/mediatek,mt7921s-bluetooth.yaml | 53 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 .../mediatek/mt8183-kukui-jacuzzi-pico6.dts   |  3 +-
 3 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml

-- 
2.43.0.429.g432eaa2c6b-goog

[PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[Chen-Yu Tsai]

The MediaTek MT7921S is a WiFi/Bluetooth combo chip that works over
SDIO. While the Bluetooth function is fully discoverable, the chip
has a pin that can reset just the Bluetooth side, as opposed to the
full chip. This needs to be described in the device tree.

Add a device tree binding for MT7921S Bluetooth over SDIO specifically
ot document the reset line.

Cc: Sean Wang <sean.wang@mediatek.com>
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
Changes since v1:
- Reworded descriptions
- Moved binding maintainer section before description
- Added missing reference to bluetooth-controller.yaml
- Added missing GPIO header to example

 .../bluetooth/mediatek,mt7921s-bluetooth.yaml | 53 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml

diff --git a/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
new file mode 100644
index 000000000000..ff11c95c816c
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/bluetooth/mediatek,mt7921s-bluetooth.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek MT7921S Bluetooth
+
+maintainers:
+  - Sean Wang <sean.wang@mediatek.com>
+
+description:
+  MT7921S is an SDIO-attached dual-radio WiFi+Bluetooth Combo chip; each
+  function is its own SDIO function on a shared SDIO interface. The chip
+  has two dedicated reset lines, one for each function core.
+  This binding only covers the Bluetooth part of the chip.
+
+allOf:
+  - $ref: bluetooth-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - mediatek,mt7921s-bluetooth
+  reg:
+    const: 2
+
+  reset-gpios:
+    maxItems: 1
+    description:
+      An active-low reset line for the Bluetooth core; on typical M.2
+      key E modules this is the W_DISABLE2# pin.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    mmc {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        bluetooth@2 {
+            compatible = "mediatek,mt7921s-bluetooth";
+            reg = <2>;
+            reset-gpios = <&pio 8 GPIO_ACTIVE_LOW>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index b64a64ca7916..662957146852 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13657,6 +13657,7 @@ M:	Sean Wang <sean.wang@mediatek.com>
 L:	linux-bluetooth@vger.kernel.org
 L:	linux-mediatek@lists.infradead.org (moderated for non-subscribers)
 S:	Maintained
+F:	Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
 F:	Documentation/devicetree/bindings/net/mediatek-bluetooth.txt
 F:	drivers/bluetooth/btmtkuart.c
 
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[AngeloGioacchino Del Regno]

Il 26/01/24 07:34, Chen-Yu Tsai ha scritto:
> The MediaTek MT7921S is a WiFi/Bluetooth combo chip that works over
> SDIO. While the Bluetooth function is fully discoverable, the chip
> has a pin that can reset just the Bluetooth side, as opposed to the
> full chip. This needs to be described in the device tree.
> 
> Add a device tree binding for MT7921S Bluetooth over SDIO specifically
> ot document the reset line.
> 
> Cc: Sean Wang <sean.wang@mediatek.com>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

> ---
> Changes since v1:
> - Reworded descriptions
> - Moved binding maintainer section before description
> - Added missing reference to bluetooth-controller.yaml
> - Added missing GPIO header to example
> 
>   .../bluetooth/mediatek,mt7921s-bluetooth.yaml | 53 +++++++++++++++++++
>   MAINTAINERS                                   |  1 +
>   2 files changed, 54 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
> new file mode 100644
> index 000000000000..ff11c95c816c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/bluetooth/mediatek,mt7921s-bluetooth.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek MT7921S Bluetooth
> +
> +maintainers:
> +  - Sean Wang <sean.wang@mediatek.com>
> +
> +description:
> +  MT7921S is an SDIO-attached dual-radio WiFi+Bluetooth Combo chip; each
> +  function is its own SDIO function on a shared SDIO interface. The chip
> +  has two dedicated reset lines, one for each function core.
> +  This binding only covers the Bluetooth part of the chip.
> +
> +allOf:
> +  - $ref: bluetooth-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt7921s-bluetooth
> +  reg:
> +    const: 2
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      An active-low reset line for the Bluetooth core; on typical M.2
> +      key E modules this is the W_DISABLE2# pin.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    mmc {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        bluetooth@2 {
> +            compatible = "mediatek,mt7921s-bluetooth";
> +            reg = <2>;
> +            reset-gpios = <&pio 8 GPIO_ACTIVE_LOW>;
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b64a64ca7916..662957146852 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13657,6 +13657,7 @@ M:	Sean Wang <sean.wang@mediatek.com>
>   L:	linux-bluetooth@vger.kernel.org
>   L:	linux-mediatek@lists.infradead.org (moderated for non-subscribers)
>   S:	Maintained
> +F:	Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
>   F:	Documentation/devicetree/bindings/net/mediatek-bluetooth.txt
>   F:	drivers/bluetooth/btmtkuart.c
>   

Re: [PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[Krzysztof Kozlowski]

On 26/01/2024 07:34, Chen-Yu Tsai wrote:
> The MediaTek MT7921S is a WiFi/Bluetooth combo chip that works over
> SDIO. While the Bluetooth function is fully discoverable, the chip
> has a pin that can reset just the Bluetooth side, as opposed to the
> full chip. This needs to be described in the device tree.
> 
> Add a device tree binding for MT7921S Bluetooth over SDIO specifically
> ot document the reset line.

s/ot/to/

> 
> Cc: Sean Wang <sean.wang@mediatek.com>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> Changes since v1:
> - Reworded descriptions
> - Moved binding maintainer section before description
> - Added missing reference to bluetooth-controller.yaml
> - Added missing GPIO header to example
> 
>  .../bluetooth/mediatek,mt7921s-bluetooth.yaml | 53 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
> new file mode 100644
> index 000000000000..ff11c95c816c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/bluetooth/mediatek,mt7921s-bluetooth.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek MT7921S Bluetooth
> +
> +maintainers:
> +  - Sean Wang <sean.wang@mediatek.com>
> +
> +description:
> +  MT7921S is an SDIO-attached dual-radio WiFi+Bluetooth Combo chip; each
> +  function is its own SDIO function on a shared SDIO interface. The chip
> +  has two dedicated reset lines, one for each function core.
> +  This binding only covers the Bluetooth part of the chip.
> +
> +allOf:
> +  - $ref: bluetooth-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt7921s-bluetooth

Can it be also WiFi on separate bus? How many device nodes do you need
for this device?

Missing blank line.


> +  reg:
> +    const: 2
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      An active-low reset line for the Bluetooth core; on typical M.2
> +      key E modules this is the W_DISABLE2# pin.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false

Instead 'unevaluatedProperties: false'


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[Chen-Yu Tsai]

On Fri, Jan 26, 2024 at 6:40 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 26/01/2024 07:34, Chen-Yu Tsai wrote:
> > The MediaTek MT7921S is a WiFi/Bluetooth combo chip that works over
> > SDIO. While the Bluetooth function is fully discoverable, the chip
> > has a pin that can reset just the Bluetooth side, as opposed to the
> > full chip. This needs to be described in the device tree.
> >
> > Add a device tree binding for MT7921S Bluetooth over SDIO specifically
> > ot document the reset line.
>
> s/ot/to/
>
> >
> > Cc: Sean Wang <sean.wang@mediatek.com>
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > Changes since v1:
> > - Reworded descriptions
> > - Moved binding maintainer section before description
> > - Added missing reference to bluetooth-controller.yaml
> > - Added missing GPIO header to example
> >
> >  .../bluetooth/mediatek,mt7921s-bluetooth.yaml | 53 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 54 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml b/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
> > new file mode 100644
> > index 000000000000..ff11c95c816c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/bluetooth/mediatek,mt7921s-bluetooth.yaml
> > @@ -0,0 +1,53 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/bluetooth/mediatek,mt7921s-bluetooth.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek MT7921S Bluetooth
> > +
> > +maintainers:
> > +  - Sean Wang <sean.wang@mediatek.com>
> > +
> > +description:
> > +  MT7921S is an SDIO-attached dual-radio WiFi+Bluetooth Combo chip; each
> > +  function is its own SDIO function on a shared SDIO interface. The chip
> > +  has two dedicated reset lines, one for each function core.
> > +  This binding only covers the Bluetooth part of the chip.
> > +
> > +allOf:
> > +  - $ref: bluetooth-controller.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - mediatek,mt7921s-bluetooth
>
> Can it be also WiFi on separate bus? How many device nodes do you need
> for this device?

For the "S" variant, WiFi is also on SDIO. For the other two variants,
"U" and "E", WiFi goes over USB and PCIe respectively. On both those
variants, Bluetooth can either go over USB or UART. That is what I
gathered from the pinouts. There are a dozen GPIO pins which don't
have detailed descriptions though. If you want a comprehensive
binding of the whole chip and all its variants, I suggest we ask
MediaTek to provide it instead. My goal with the binding is to document
existing usage and allow me to upstream new device trees.

For now we only need the Bluetooth node. The WiFi part is perfectly
detectable, and the driver doesn't seem to need the WiFi reset pin.
The Bluetooth driver only uses its reset pin to reset a hung controller.

> Missing blank line.

Will fix.

> > +  reg:
> > +    const: 2
> > +
> > +  reset-gpios:
> > +    maxItems: 1
> > +    description:
> > +      An active-low reset line for the Bluetooth core; on typical M.2
> > +      key E modules this is the W_DISABLE2# pin.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
>
> Instead 'unevaluatedProperties: false'

Will fix.


Thanks
ChenYu

Re: [PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[Krzysztof Kozlowski]

On 29/01/2024 04:38, Chen-Yu Tsai wrote:

>>> +allOf:
>>> +  - $ref: bluetooth-controller.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - mediatek,mt7921s-bluetooth
>>
>> Can it be also WiFi on separate bus? How many device nodes do you need
>> for this device?
> 
> For the "S" variant, WiFi is also on SDIO. For the other two variants,
> "U" and "E", WiFi goes over USB and PCIe respectively. On both those
> variants, Bluetooth can either go over USB or UART. That is what I
> gathered from the pinouts. There are a dozen GPIO pins which don't
> have detailed descriptions though. If you want a comprehensive
> binding of the whole chip and all its variants, I suggest we ask
> MediaTek to provide it instead. My goal with the binding is to document
> existing usage and allow me to upstream new device trees.
> 
> For now we only need the Bluetooth node. The WiFi part is perfectly
> detectable, and the driver doesn't seem to need the WiFi reset pin.
> The Bluetooth driver only uses its reset pin to reset a hung controller.

Then suffix "bluetooth" seems redundant.



Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[Chen-Yu Tsai]

On Mon, Jan 29, 2024 at 3:34 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 29/01/2024 04:38, Chen-Yu Tsai wrote:
>
> >>> +allOf:
> >>> +  - $ref: bluetooth-controller.yaml#
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - mediatek,mt7921s-bluetooth
> >>
> >> Can it be also WiFi on separate bus? How many device nodes do you need
> >> for this device?
> >
> > For the "S" variant, WiFi is also on SDIO. For the other two variants,
> > "U" and "E", WiFi goes over USB and PCIe respectively. On both those
> > variants, Bluetooth can either go over USB or UART. That is what I
> > gathered from the pinouts. There are a dozen GPIO pins which don't
> > have detailed descriptions though. If you want a comprehensive
> > binding of the whole chip and all its variants, I suggest we ask
> > MediaTek to provide it instead. My goal with the binding is to document
> > existing usage and allow me to upstream new device trees.
> >
> > For now we only need the Bluetooth node. The WiFi part is perfectly
> > detectable, and the driver doesn't seem to need the WiFi reset pin.
> > The Bluetooth driver only uses its reset pin to reset a hung controller.
>
> Then suffix "bluetooth" seems redundant.

I think keeping the suffix makes more sense though. The chip is a two
function piece, and this only targets one of the functions. Also, the
compatible string is already used in an existing driver [1] and
soon-to-be in-tree device tree [2].


ChenYu

[1] https://elixir.bootlin.com/linux/latest/source/drivers/bluetooth/btmtksdio.c#L1414
[2] https://elixir.bootlin.com/linux/v6.8-rc1/source/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts#L86

Re: [PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[Krzysztof Kozlowski]

On 30/01/2024 04:32, Chen-Yu Tsai wrote:
> On Mon, Jan 29, 2024 at 3:34 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 29/01/2024 04:38, Chen-Yu Tsai wrote:
>>
>>>>> +allOf:
>>>>> +  - $ref: bluetooth-controller.yaml#
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - mediatek,mt7921s-bluetooth
>>>>
>>>> Can it be also WiFi on separate bus? How many device nodes do you need
>>>> for this device?
>>>
>>> For the "S" variant, WiFi is also on SDIO. For the other two variants,
>>> "U" and "E", WiFi goes over USB and PCIe respectively. On both those
>>> variants, Bluetooth can either go over USB or UART. That is what I
>>> gathered from the pinouts. There are a dozen GPIO pins which don't
>>> have detailed descriptions though. If you want a comprehensive
>>> binding of the whole chip and all its variants, I suggest we ask
>>> MediaTek to provide it instead. My goal with the binding is to document
>>> existing usage and allow me to upstream new device trees.
>>>
>>> For now we only need the Bluetooth node. The WiFi part is perfectly
>>> detectable, and the driver doesn't seem to need the WiFi reset pin.
>>> The Bluetooth driver only uses its reset pin to reset a hung controller.
>>
>> Then suffix "bluetooth" seems redundant.
> 
> I think keeping the suffix makes more sense though. The chip is a two
> function piece, and this only targets one of the functions. Also, the

That's why I asked and you said there is only one interface: SDIO.

> compatible string is already used in an existing driver [1] and
> soon-to-be in-tree device tree [2].

That's not the way to upstream compatible. You cannot send it bypassing
bindings and review and later claim that's an ABI.


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[Chen-Yu Tsai]

On Tue, Jan 30, 2024 at 3:37 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 30/01/2024 04:32, Chen-Yu Tsai wrote:
> > On Mon, Jan 29, 2024 at 3:34 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 29/01/2024 04:38, Chen-Yu Tsai wrote:
> >>
> >>>>> +allOf:
> >>>>> +  - $ref: bluetooth-controller.yaml#
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    enum:
> >>>>> +      - mediatek,mt7921s-bluetooth
> >>>>
> >>>> Can it be also WiFi on separate bus? How many device nodes do you need
> >>>> for this device?
> >>>
> >>> For the "S" variant, WiFi is also on SDIO. For the other two variants,
> >>> "U" and "E", WiFi goes over USB and PCIe respectively. On both those
> >>> variants, Bluetooth can either go over USB or UART. That is what I
> >>> gathered from the pinouts. There are a dozen GPIO pins which don't
> >>> have detailed descriptions though. If you want a comprehensive
> >>> binding of the whole chip and all its variants, I suggest we ask
> >>> MediaTek to provide it instead. My goal with the binding is to document
> >>> existing usage and allow me to upstream new device trees.
> >>>
> >>> For now we only need the Bluetooth node. The WiFi part is perfectly
> >>> detectable, and the driver doesn't seem to need the WiFi reset pin.
> >>> The Bluetooth driver only uses its reset pin to reset a hung controller.
> >>
> >> Then suffix "bluetooth" seems redundant.
> >
> > I think keeping the suffix makes more sense though. The chip is a two
> > function piece, and this only targets one of the functions. Also, the
>
> That's why I asked and you said there is only one interface: SDIO.

There's only one interface, SDIO, but two SDIO functions. The two
functions, if both were to be described in the device tree, would
be two separate nodes. We just don't have any use for the WiFi one
right now. Does that make sense to keep the suffix?

> > compatible string is already used in an existing driver [1] and
> > soon-to-be in-tree device tree [2].
>
> That's not the way to upstream compatible. You cannot send it bypassing
> bindings and review and later claim that's an ABI.

I get that. I can fix up the existing users where necessary. A proper
binding would make the driver lookup be more efficient as well.


Thanks
ChenYu

Re: [PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[Krzysztof Kozlowski]

On 30/01/2024 08:47, Chen-Yu Tsai wrote:
> On Tue, Jan 30, 2024 at 3:37 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 30/01/2024 04:32, Chen-Yu Tsai wrote:
>>> On Mon, Jan 29, 2024 at 3:34 PM Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 29/01/2024 04:38, Chen-Yu Tsai wrote:
>>>>
>>>>>>> +allOf:
>>>>>>> +  - $ref: bluetooth-controller.yaml#
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    enum:
>>>>>>> +      - mediatek,mt7921s-bluetooth
>>>>>>
>>>>>> Can it be also WiFi on separate bus? How many device nodes do you need
>>>>>> for this device?
>>>>>
>>>>> For the "S" variant, WiFi is also on SDIO. For the other two variants,
>>>>> "U" and "E", WiFi goes over USB and PCIe respectively. On both those
>>>>> variants, Bluetooth can either go over USB or UART. That is what I
>>>>> gathered from the pinouts. There are a dozen GPIO pins which don't
>>>>> have detailed descriptions though. If you want a comprehensive
>>>>> binding of the whole chip and all its variants, I suggest we ask
>>>>> MediaTek to provide it instead. My goal with the binding is to document
>>>>> existing usage and allow me to upstream new device trees.
>>>>>
>>>>> For now we only need the Bluetooth node. The WiFi part is perfectly
>>>>> detectable, and the driver doesn't seem to need the WiFi reset pin.
>>>>> The Bluetooth driver only uses its reset pin to reset a hung controller.
>>>>
>>>> Then suffix "bluetooth" seems redundant.
>>>
>>> I think keeping the suffix makes more sense though. The chip is a two
>>> function piece, and this only targets one of the functions. Also, the
>>
>> That's why I asked and you said there is only one interface: SDIO.
> 
> There's only one interface, SDIO, but two SDIO functions. The two
> functions, if both were to be described in the device tree, would
> be two separate nodes. We just don't have any use for the WiFi one
> right now. Does that make sense to keep the suffix?

Number of functions does not really matter. Number of interfaces on the
bus would matter. Why would you have two separate nodes for the same
SDIO interface? Or do you want to say there are two interfaces?



Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[Rob Herring]

On Tue, Jan 30, 2024 at 05:25:38PM +0100, Krzysztof Kozlowski wrote:
> On 30/01/2024 08:47, Chen-Yu Tsai wrote:
> > On Tue, Jan 30, 2024 at 3:37 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 30/01/2024 04:32, Chen-Yu Tsai wrote:
> >>> On Mon, Jan 29, 2024 at 3:34 PM Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@linaro.org> wrote:
> >>>>
> >>>> On 29/01/2024 04:38, Chen-Yu Tsai wrote:
> >>>>
> >>>>>>> +allOf:
> >>>>>>> +  - $ref: bluetooth-controller.yaml#
> >>>>>>> +
> >>>>>>> +properties:
> >>>>>>> +  compatible:
> >>>>>>> +    enum:
> >>>>>>> +      - mediatek,mt7921s-bluetooth
> >>>>>>
> >>>>>> Can it be also WiFi on separate bus? How many device nodes do you need
> >>>>>> for this device?
> >>>>>
> >>>>> For the "S" variant, WiFi is also on SDIO. For the other two variants,
> >>>>> "U" and "E", WiFi goes over USB and PCIe respectively. On both those
> >>>>> variants, Bluetooth can either go over USB or UART. That is what I
> >>>>> gathered from the pinouts. There are a dozen GPIO pins which don't
> >>>>> have detailed descriptions though. If you want a comprehensive
> >>>>> binding of the whole chip and all its variants, I suggest we ask
> >>>>> MediaTek to provide it instead. My goal with the binding is to document
> >>>>> existing usage and allow me to upstream new device trees.
> >>>>>
> >>>>> For now we only need the Bluetooth node. The WiFi part is perfectly
> >>>>> detectable, and the driver doesn't seem to need the WiFi reset pin.
> >>>>> The Bluetooth driver only uses its reset pin to reset a hung controller.
> >>>>
> >>>> Then suffix "bluetooth" seems redundant.
> >>>
> >>> I think keeping the suffix makes more sense though. The chip is a two
> >>> function piece, and this only targets one of the functions. Also, the
> >>
> >> That's why I asked and you said there is only one interface: SDIO.
> > 
> > There's only one interface, SDIO, but two SDIO functions. The two
> > functions, if both were to be described in the device tree, would
> > be two separate nodes. We just don't have any use for the WiFi one
> > right now. Does that make sense to keep the suffix?
> 
> Number of functions does not really matter. Number of interfaces on the
> bus would matter. Why would you have two separate nodes for the same
> SDIO interface? Or do you want to say there are two interfaces?

Right, one device at 2 addresses on a bus should be a node with 2 "reg" 
entries, not 2 nodes with 1 "reg" address each.

Rob

Re: [PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[Chen-Yu Tsai]

(+CC Ulf Hansson)

On Wed, Jan 31, 2024 at 6:38 AM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Jan 30, 2024 at 05:25:38PM +0100, Krzysztof Kozlowski wrote:
> > On 30/01/2024 08:47, Chen-Yu Tsai wrote:
> > > On Tue, Jan 30, 2024 at 3:37 PM Krzysztof Kozlowski
> > > <krzysztof.kozlowski@linaro.org> wrote:
> > >>
> > >> On 30/01/2024 04:32, Chen-Yu Tsai wrote:
> > >>> On Mon, Jan 29, 2024 at 3:34 PM Krzysztof Kozlowski
> > >>> <krzysztof.kozlowski@linaro.org> wrote:
> > >>>>
> > >>>> On 29/01/2024 04:38, Chen-Yu Tsai wrote:
> > >>>>
> > >>>>>>> +allOf:
> > >>>>>>> +  - $ref: bluetooth-controller.yaml#
> > >>>>>>> +
> > >>>>>>> +properties:
> > >>>>>>> +  compatible:
> > >>>>>>> +    enum:
> > >>>>>>> +      - mediatek,mt7921s-bluetooth
> > >>>>>>
> > >>>>>> Can it be also WiFi on separate bus? How many device nodes do you need
> > >>>>>> for this device?
> > >>>>>
> > >>>>> For the "S" variant, WiFi is also on SDIO. For the other two variants,
> > >>>>> "U" and "E", WiFi goes over USB and PCIe respectively. On both those
> > >>>>> variants, Bluetooth can either go over USB or UART. That is what I
> > >>>>> gathered from the pinouts. There are a dozen GPIO pins which don't
> > >>>>> have detailed descriptions though. If you want a comprehensive
> > >>>>> binding of the whole chip and all its variants, I suggest we ask
> > >>>>> MediaTek to provide it instead. My goal with the binding is to document
> > >>>>> existing usage and allow me to upstream new device trees.
> > >>>>>
> > >>>>> For now we only need the Bluetooth node. The WiFi part is perfectly
> > >>>>> detectable, and the driver doesn't seem to need the WiFi reset pin.
> > >>>>> The Bluetooth driver only uses its reset pin to reset a hung controller.
> > >>>>
> > >>>> Then suffix "bluetooth" seems redundant.
> > >>>
> > >>> I think keeping the suffix makes more sense though. The chip is a two
> > >>> function piece, and this only targets one of the functions. Also, the
> > >>
> > >> That's why I asked and you said there is only one interface: SDIO.
> > >
> > > There's only one interface, SDIO, but two SDIO functions. The two
> > > functions, if both were to be described in the device tree, would
> > > be two separate nodes. We just don't have any use for the WiFi one
> > > right now. Does that make sense to keep the suffix?
> >
> > Number of functions does not really matter. Number of interfaces on the
> > bus would matter. Why would you have two separate nodes for the same
> > SDIO interface? Or do you want to say there are two interfaces?

There is only one external interface. I don't know how the functions
are stitched together internally.

It could be that the separate functions have nothing in common other
than sharing a standard external SDIO interface. Each function can be
individually controlled, and operations for different functions are
directed internally to the corresponding core.

> Right, one device at 2 addresses on a bus should be a node with 2 "reg"
> entries, not 2 nodes with 1 "reg" address each.

AFAICU that's not what the MMC controller binding, which I quote below,
says. It implies that each SDIO function shall be a separate node under
the MMC controller node.


patternProperties:
  "^.*@[0-9]+$":
    type: object
    description: |
      On embedded systems the cards connected to a host may need
      additional properties. These can be specified in subnodes to the
      host controller node. The subnodes are identified by the
      standard \'reg\' property. Which information exactly can be
      specified depends on the bindings for the SDIO function driver
      for the subnode, as specified by the compatible string.

    properties:
      compatible:
        description: |
          Name of SDIO function following generic names recommended
          practice

      reg:
        items:
          - minimum: 0
            maximum: 7
            description:
              Must contain the SDIO function number of the function this
              subnode describes. A value of 0 denotes the memory SD
              function, values from 1 to 7 denote the SDIO functions.


ChenYu

Re: [PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[Ulf Hansson]

On Wed, 31 Jan 2024 at 04:39, Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> (+CC Ulf Hansson)
>
> On Wed, Jan 31, 2024 at 6:38 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Jan 30, 2024 at 05:25:38PM +0100, Krzysztof Kozlowski wrote:
> > > On 30/01/2024 08:47, Chen-Yu Tsai wrote:
> > > > On Tue, Jan 30, 2024 at 3:37 PM Krzysztof Kozlowski
> > > > <krzysztof.kozlowski@linaro.org> wrote:
> > > >>
> > > >> On 30/01/2024 04:32, Chen-Yu Tsai wrote:
> > > >>> On Mon, Jan 29, 2024 at 3:34 PM Krzysztof Kozlowski
> > > >>> <krzysztof.kozlowski@linaro.org> wrote:
> > > >>>>
> > > >>>> On 29/01/2024 04:38, Chen-Yu Tsai wrote:
> > > >>>>
> > > >>>>>>> +allOf:
> > > >>>>>>> +  - $ref: bluetooth-controller.yaml#
> > > >>>>>>> +
> > > >>>>>>> +properties:
> > > >>>>>>> +  compatible:
> > > >>>>>>> +    enum:
> > > >>>>>>> +      - mediatek,mt7921s-bluetooth
> > > >>>>>>
> > > >>>>>> Can it be also WiFi on separate bus? How many device nodes do you need
> > > >>>>>> for this device?
> > > >>>>>
> > > >>>>> For the "S" variant, WiFi is also on SDIO. For the other two variants,
> > > >>>>> "U" and "E", WiFi goes over USB and PCIe respectively. On both those
> > > >>>>> variants, Bluetooth can either go over USB or UART. That is what I
> > > >>>>> gathered from the pinouts. There are a dozen GPIO pins which don't
> > > >>>>> have detailed descriptions though. If you want a comprehensive
> > > >>>>> binding of the whole chip and all its variants, I suggest we ask
> > > >>>>> MediaTek to provide it instead. My goal with the binding is to document
> > > >>>>> existing usage and allow me to upstream new device trees.
> > > >>>>>
> > > >>>>> For now we only need the Bluetooth node. The WiFi part is perfectly
> > > >>>>> detectable, and the driver doesn't seem to need the WiFi reset pin.
> > > >>>>> The Bluetooth driver only uses its reset pin to reset a hung controller.
> > > >>>>
> > > >>>> Then suffix "bluetooth" seems redundant.
> > > >>>
> > > >>> I think keeping the suffix makes more sense though. The chip is a two
> > > >>> function piece, and this only targets one of the functions. Also, the
> > > >>
> > > >> That's why I asked and you said there is only one interface: SDIO.
> > > >
> > > > There's only one interface, SDIO, but two SDIO functions. The two
> > > > functions, if both were to be described in the device tree, would
> > > > be two separate nodes. We just don't have any use for the WiFi one
> > > > right now. Does that make sense to keep the suffix?
> > >
> > > Number of functions does not really matter. Number of interfaces on the
> > > bus would matter. Why would you have two separate nodes for the same
> > > SDIO interface? Or do you want to say there are two interfaces?
>
> There is only one external interface. I don't know how the functions
> are stitched together internally.
>
> It could be that the separate functions have nothing in common other
> than sharing a standard external SDIO interface. Each function can be
> individually controlled, and operations for different functions are
> directed internally to the corresponding core.
>
> > Right, one device at 2 addresses on a bus should be a node with 2 "reg"
> > entries, not 2 nodes with 1 "reg" address each.
>
> AFAICU that's not what the MMC controller binding, which I quote below,
> says. It implies that each SDIO function shall be a separate node under
> the MMC controller node.

Yes, that's what we decided to go with, a long time ago. At least in
this particular case, I think it makes sense, as each function
(child-node) may also describe additional resources routed to each
function.

A typical description could be for a WiFi-Bluetooth combo-chip, where
each function may have its own clocks, irqs and regulators being
routed.

>
>
> patternProperties:
>   "^.*@[0-9]+$":
>     type: object
>     description: |
>       On embedded systems the cards connected to a host may need
>       additional properties. These can be specified in subnodes to the
>       host controller node. The subnodes are identified by the
>       standard \'reg\' property. Which information exactly can be
>       specified depends on the bindings for the SDIO function driver
>       for the subnode, as specified by the compatible string.
>
>     properties:
>       compatible:
>         description: |
>           Name of SDIO function following generic names recommended
>           practice
>
>       reg:
>         items:
>           - minimum: 0
>             maximum: 7
>             description:
>               Must contain the SDIO function number of the function this
>               subnode describes. A value of 0 denotes the memory SD
>               function, values from 1 to 7 denote the SDIO functions.
>
>
> ChenYu

Kind regards
Uffe

Re: [PATCH v2 1/2] dt-bindings: net: bluetooth: Add MediaTek MT7921S SDIO Bluetooth

[Chen-Yu Tsai]

On Tue, Feb 6, 2024 at 1:50 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 31 Jan 2024 at 04:39, Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > (+CC Ulf Hansson)
> >
> > On Wed, Jan 31, 2024 at 6:38 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Tue, Jan 30, 2024 at 05:25:38PM +0100, Krzysztof Kozlowski wrote:
> > > > On 30/01/2024 08:47, Chen-Yu Tsai wrote:
> > > > > On Tue, Jan 30, 2024 at 3:37 PM Krzysztof Kozlowski
> > > > > <krzysztof.kozlowski@linaro.org> wrote:
> > > > >>
> > > > >> On 30/01/2024 04:32, Chen-Yu Tsai wrote:
> > > > >>> On Mon, Jan 29, 2024 at 3:34 PM Krzysztof Kozlowski
> > > > >>> <krzysztof.kozlowski@linaro.org> wrote:
> > > > >>>>
> > > > >>>> On 29/01/2024 04:38, Chen-Yu Tsai wrote:
> > > > >>>>
> > > > >>>>>>> +allOf:
> > > > >>>>>>> +  - $ref: bluetooth-controller.yaml#
> > > > >>>>>>> +
> > > > >>>>>>> +properties:
> > > > >>>>>>> +  compatible:
> > > > >>>>>>> +    enum:
> > > > >>>>>>> +      - mediatek,mt7921s-bluetooth
> > > > >>>>>>
> > > > >>>>>> Can it be also WiFi on separate bus? How many device nodes do you need
> > > > >>>>>> for this device?
> > > > >>>>>
> > > > >>>>> For the "S" variant, WiFi is also on SDIO. For the other two variants,
> > > > >>>>> "U" and "E", WiFi goes over USB and PCIe respectively. On both those
> > > > >>>>> variants, Bluetooth can either go over USB or UART. That is what I
> > > > >>>>> gathered from the pinouts. There are a dozen GPIO pins which don't
> > > > >>>>> have detailed descriptions though. If you want a comprehensive
> > > > >>>>> binding of the whole chip and all its variants, I suggest we ask
> > > > >>>>> MediaTek to provide it instead. My goal with the binding is to document
> > > > >>>>> existing usage and allow me to upstream new device trees.
> > > > >>>>>
> > > > >>>>> For now we only need the Bluetooth node. The WiFi part is perfectly
> > > > >>>>> detectable, and the driver doesn't seem to need the WiFi reset pin.
> > > > >>>>> The Bluetooth driver only uses its reset pin to reset a hung controller.
> > > > >>>>
> > > > >>>> Then suffix "bluetooth" seems redundant.
> > > > >>>
> > > > >>> I think keeping the suffix makes more sense though. The chip is a two
> > > > >>> function piece, and this only targets one of the functions. Also, the
> > > > >>
> > > > >> That's why I asked and you said there is only one interface: SDIO.
> > > > >
> > > > > There's only one interface, SDIO, but two SDIO functions. The two
> > > > > functions, if both were to be described in the device tree, would
> > > > > be two separate nodes. We just don't have any use for the WiFi one
> > > > > right now. Does that make sense to keep the suffix?
> > > >
> > > > Number of functions does not really matter. Number of interfaces on the
> > > > bus would matter. Why would you have two separate nodes for the same
> > > > SDIO interface? Or do you want to say there are two interfaces?
> >
> > There is only one external interface. I don't know how the functions
> > are stitched together internally.
> >
> > It could be that the separate functions have nothing in common other
> > than sharing a standard external SDIO interface. Each function can be
> > individually controlled, and operations for different functions are
> > directed internally to the corresponding core.
> >
> > > Right, one device at 2 addresses on a bus should be a node with 2 "reg"
> > > entries, not 2 nodes with 1 "reg" address each.
> >
> > AFAICU that's not what the MMC controller binding, which I quote below,
> > says. It implies that each SDIO function shall be a separate node under
> > the MMC controller node.
>
> Yes, that's what we decided to go with, a long time ago. At least in
> this particular case, I think it makes sense, as each function
> (child-node) may also describe additional resources routed to each
> function.
>
> A typical description could be for a WiFi-Bluetooth combo-chip, where
> each function may have its own clocks, irqs and regulators being
> routed.

Rob, Krzysztof, does that help you understand why the binding and example
are written with bluetooth being one node and WiFi (should it ever be added)
being a separate node? It is based on the existing MMC controller bindings.

ChenYu


> >
> >
> > patternProperties:
> >   "^.*@[0-9]+$":
> >     type: object
> >     description: |
> >       On embedded systems the cards connected to a host may need
> >       additional properties. These can be specified in subnodes to the
> >       host controller node. The subnodes are identified by the
> >       standard \'reg\' property. Which information exactly can be
> >       specified depends on the bindings for the SDIO function driver
> >       for the subnode, as specified by the compatible string.
> >
> >     properties:
> >       compatible:
> >         description: |
> >           Name of SDIO function following generic names recommended
> >           practice
> >
> >       reg:
> >         items:
> >           - minimum: 0
> >             maximum: 7
> >             description:
> >               Must contain the SDIO function number of the function this
> >               subnode describes. A value of 0 denotes the memory SD
> >               function, values from 1 to 7 denote the SDIO functions.
> >
> >
> > ChenYu
>
> Kind regards
> Uffe

[PATCH v2 2/2] arm64: dts: mediatek: mt8183-pico6: Fix bluetooth node

[Chen-Yu Tsai]

Bluetooth is not a random device connected to the MMC/SD controller. It
is function 2 of the SDIO device.

Fix the address of the bluetooth node. Also fix the node name and drop
the label.

Fixes: 055ef10ccdd4 ("arm64: dts: mt8183: Add jacuzzi pico/pico6 board")
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
Changes since v1:
- Collected reviewed-by

 arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts b/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
index a2e74b829320..6a7ae616512d 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
@@ -82,7 +82,8 @@ pins-clk {
 };
 
 &mmc1 {
-	bt_reset: bt-reset {
+	bluetooth@2 {
+		reg = <2>;
 		compatible = "mediatek,mt7921s-bluetooth";
 		pinctrl-names = "default";
 		pinctrl-0 = <&bt_pins_reset>;
-- 
2.43.0.429.g432eaa2c6b-goog

Re: [PATCH v2 2/2] arm64: dts: mediatek: mt8183-pico6: Fix bluetooth node

[Matthias Brugger]

On 26/01/2024 07:34, Chen-Yu Tsai wrote:
> Bluetooth is not a random device connected to the MMC/SD controller. It
> is function 2 of the SDIO device.
> 
> Fix the address of the bluetooth node. Also fix the node name and drop
> the label.
> 
> Fixes: 055ef10ccdd4 ("arm64: dts: mt8183: Add jacuzzi pico/pico6 board")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
> Changes since v1:
> - Collected reviewed-by
> 
>   arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts b/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
> index a2e74b829320..6a7ae616512d 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
> @@ -82,7 +82,8 @@ pins-clk {
>   };
>   
>   &mmc1 {
> -	bt_reset: bt-reset {
> +	bluetooth@2 {
> +		reg = <2>;
>   		compatible = "mediatek,mt7921s-bluetooth";
>   		pinctrl-names = "default";
>   		pinctrl-0 = <&bt_pins_reset>;

Re: [PATCH v2 2/2] arm64: dts: mediatek: mt8183-pico6: Fix bluetooth node

[Paul Menzel]

Dear Chen-Yu,


Thank you for your patch.

Am 26.01.24 um 07:34 schrieb Chen-Yu Tsai:
> Bluetooth is not a random device connected to the MMC/SD controller. It
> is function 2 of the SDIO device.
> 
> Fix the address of the bluetooth node. Also fix the node name and drop
> the label.

Excuse my ignorance: Is this a cosmetic fix or does it fix the device 
somehow?

> Fixes: 055ef10ccdd4 ("arm64: dts: mt8183: Add jacuzzi pico/pico6 board")
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
> Changes since v1:
> - Collected reviewed-by
> 
>   arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts b/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
> index a2e74b829320..6a7ae616512d 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
> @@ -82,7 +82,8 @@ pins-clk {
>   };
>   
>   &mmc1 {
> -	bt_reset: bt-reset {
> +	bluetooth@2 {
> +		reg = <2>;

To avoid confusion, would it be possible to use sdio as a “name”.

>   		compatible = "mediatek,mt7921s-bluetooth";
>   		pinctrl-names = "default";
>   		pinctrl-0 = <&bt_pins_reset>;


Kind regards,

Paul

Re: [PATCH v2 2/2] arm64: dts: mediatek: mt8183-pico6: Fix bluetooth node

[AngeloGioacchino Del Regno]

Il 29/01/24 17:31, Paul Menzel ha scritto:
> Dear Chen-Yu,
> 
> 
> Thank you for your patch.
> 
> Am 26.01.24 um 07:34 schrieb Chen-Yu Tsai:
>> Bluetooth is not a random device connected to the MMC/SD controller. It
>> is function 2 of the SDIO device.
>>
>> Fix the address of the bluetooth node. Also fix the node name and drop
>> the label.
> 
> Excuse my ignorance: Is this a cosmetic fix or does it fix the device somehow?
> 
>> Fixes: 055ef10ccdd4 ("arm64: dts: mt8183: Add jacuzzi pico/pico6 board")
>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>> Changes since v1:
>> - Collected reviewed-by
>>
>>   arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts 
>> b/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
>> index a2e74b829320..6a7ae616512d 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
>> +++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
>> @@ -82,7 +82,8 @@ pins-clk {
>>   };
>>   &mmc1 {
>> -    bt_reset: bt-reset {
>> +    bluetooth@2 {
>> +        reg = <2>;
> 
> To avoid confusion, would it be possible to use sdio as a “name”.
> 

Names must be generic and descriptive; this is the Bluetooth part of the MT7921s
hence it's called "bluetooth". If its functionality was "sdio", it'd be as such,
but this is BT anyway.

"sdio@xxx" would be applicable to, for example, a controller that provides support
exclusively for SDIO (with no support for eMMC/SD).

Regards,
Angelo

Re: [PATCH v2 2/2] arm64: dts: mediatek: mt8183-pico6: Fix bluetooth node

[Chen-Yu Tsai]

On Tue, Jan 30, 2024 at 12:31 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Chen-Yu,
>
>
> Thank you for your patch.
>
> Am 26.01.24 um 07:34 schrieb Chen-Yu Tsai:
> > Bluetooth is not a random device connected to the MMC/SD controller. It
> > is function 2 of the SDIO device.
> >
> > Fix the address of the bluetooth node. Also fix the node name and drop
> > the label.
>
> Excuse my ignorance: Is this a cosmetic fix or does it fix the device
> somehow?

It's a cosmetic change, since the driver already searches the whole device
tree for the specific compatible string. However it also fixes the device
tree description to match the actual hardware.

> > Fixes: 055ef10ccdd4 ("arm64: dts: mt8183: Add jacuzzi pico/pico6 board")
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > ---
> > Changes since v1:
> > - Collected reviewed-by
> >
> >   arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts b/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
> > index a2e74b829320..6a7ae616512d 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui-jacuzzi-pico6.dts
> > @@ -82,7 +82,8 @@ pins-clk {
> >   };
> >
> >   &mmc1 {
> > -     bt_reset: bt-reset {
> > +     bluetooth@2 {
> > +             reg = <2>;
>
> To avoid confusion, would it be possible to use sdio as a “name”.

Not sure where the confusion is. Bluetooth is the functionality this
SDIO function provides.

ChenYu

>
> >               compatible = "mediatek,mt7921s-bluetooth";
> >               pinctrl-names = "default";
> >               pinctrl-0 = <&bt_pins_reset>;
>
>
> Kind regards,
>
> Paul