Re: [PATCH 1/3] dt-bindings: net: bluetooth: Add rtl8723bs-bluetooth

[Alistair Francis]

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