| Message ID | 20200404204850.405050-1-alistair@alistair23.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] dt-bindings: net: bluetooth: Add rtl8723bs-bluetooth | expand |
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
On 4/04/2020 3:42 pm, Martin Blumenstingl wrote: > 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
On 4/04/2020 3:42 pm, Martin Blumenstingl wrote: > 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. Done! > > [...] >> + 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). I have removed the "(active..." part from the GPIOs. > > 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 Added. > >> +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]. I have removed the 'firmware-postfix" part from this series. > 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 Fixed. > > 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. Removed. > >> +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 Removed. > Z > also please add a "uart-has-rtscts" propery, see > Documentation/devicetree/bindings/serial/serial.yaml > Also please update patch #3. Added. Thanks for the review. Alistair > > > Martin > > > [0] https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/commit/rtl_bt/rtl8723bs_config-OBDA8723.bin?id=e6b9001e91110c654573b8f8e2db6155d10d3b57
diff --git a/Documentation/devicetree/bindings/net/realtek,rtl8723bs-bt.yaml b/Documentation/devicetree/bindings/net/realtek,rtl8723bs-bt.yaml new file mode 100644 index 000000000000..9e212954f629 --- /dev/null +++ b/Documentation/devicetree/bindings/net/realtek,rtl8723bs-bt.yaml @@ -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 + +maintainers: + - Vasily Khoruzhick <anarsoul@gmail.com> + - Alistair Francis <alistair@alistair23.me> + +description: + RTL8723CS/RTL8723CS is WiFi + BT chip. WiFi part is connected over SDIO, while + BT is connected over serial. It speaks H5 protocol with few extra commands + to upload firmware and change module speed. + +properties: + compatible: + oneOf: + - "realtek,rtl8723bs-bt" + - "realtek,rtl8723cs-bt" + + 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) + +firmware-postfix: firmware postfix to be used for firmware config +reset-gpios: GPIO specifier, used to reset the BT module (active high) + +required: + - compatible + +examples: + - | + &uart1 { + pinctrl-names = "default"; + pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>; + status = "okay"; + + bluetooth { + compatible = "realtek,rtl8723bs-bt"; + reset-gpios = <&r_pio 0 4 GPIO_ACTIVE_LOW>; /* PL4 */ + device-wake-gpios = <&r_pio 0 5 GPIO_ACTIVE_HIGH>; /* PL5 */ + host-wakeup-gpios = <&r_pio 0 6 GPIO_ACTIVE_HIGH>; /* PL6 */ + firmware-postfix="pine64"; + }; + };