Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To:     alistair@alistair23.me
Cc:     alistair23@gmail.com, anarsoul@gmail.com,
        devicetree@vger.kernel.org, johan.hedberg@gmail.com,
        linux-arm-kernel@lists.infradead.org,
        linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org,
        marcel@holtmann.org, mripard@kernel.org, netdev@vger.kernel.org,
        wens@csie.org, max.chou@realtek.com, hdegoede@redhat.com
Subject: RE: [PATCH 1/3] dt-bindings: net: bluetooth: Add rtl8723bs-bluetooth
Date:   Sun,  5 Apr 2020 00:42:05 +0200
Message-Id: <20200404224205.1643238-1-martin.blumenstingl@googlemail.com>
In-Reply-To: <20200404204850.405050-1-alistair@alistair23.me>
References: <20200404204850.405050-1-alistair@alistair23.me>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Alistair,

+Cc Max Chou, he may be interested in this also

[...]
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/realtek,rtl8723bs-bt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RTL8723BS/RTL8723CS Bluetooth Device Tree Bindings
I suggest you also add RTL8822C here (as well as to the compatible enum
and the description below). commit 848fc6164158d6 ("Bluetooth: hci_h5:
btrtl: Add support for RTL8822C") adde support for that chip but didn't
add the dt-binding documentation.

[...]
> +  device-wake-gpios:
> +    description:
> +      GPIO specifier, used to wakeup the BT module (active high)
> +
> +  enable-gpios:
> +    description:
> +      GPIO specifier, used to enable the BT module (active high)
> +
> +  host-wake-gpios:
> +    desciption:
> +      GPIO specifier, used to wakeup the host processor (active high)
regarding all GPIOs here: it entirely depends on the board whether these
are active HIGH or LOW. even though the actual Bluetooth part may
require a specific polarity there can be (for example) a transistor on
the board which could be used to invert the polarity (from the SoC's
view).

also "make dt_binding_check" reports:
  properties:host-wake-gpios: 'maxItems' is a required property
I assume that it'll be the same for the other properties

> +firmware-postfix: firmware postfix to be used for firmware config
there's no other dt-binding that uses "firmware-postfix" yet. However,
there are a few that use "firmware-name". My opinion hasn't changed
since Vasily has posted this series initially: I would not add that
property for now because there seems to be a "standard" config blob
(which works for "all" boards), see Hans' analysis result of the ACPI
config blobs for RTL8723BS: [0].
Getting that "standard" config blob into linux-firmware would be
awesome (I assume licensing is not an issue here, Hans can probably give
more details here). I'm not sure about the licenses of "board specific"
config blobs and whether these can be added to linux-firmware.

also indentation seems wrong here

> +reset-gpios: GPIO specifier, used to reset the BT module (active high)
indentation seems wrong here too

also please note that there is currently no support for this property
inside the hci_h5 driver and you don't seem to add support for it within
this series either. so please double check that the reset GPIO is really
wired up on your sopine board.

> +required:
> +  - compatible
> +
> +examples:
> +  - |
> +    &uart1 {
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
> +        status = "okay";
AFAIK the "status" property should be omitted from examples

also please add a "uart-has-rtscts" propery, see
Documentation/devicetree/bindings/serial/serial.yaml
Also please update patch #3.


Martin


[0] https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/commit/rtl_bt/rtl8723bs_config-OBDA8723.bin?id=e6b9001e91110c654573b8f8e2db6155d10d3b57