Re: [PATCH] arm64: dts: mediatek: mt8192-asurada-hayato: Enable Bluetooth

[Chen-Yu Tsai]

On Fri, Apr 21, 2023 at 10:01 PM Nícolas F. R. A. Prado
<nfraprado@xxxxxxxxxxxxx> wrote:
>
> On Fri, Apr 21, 2023 at 07:03:27PM +0800, Chen-Yu Tsai wrote:
> > Hayato's Realtek WiFi/BT module has it's Bluetooth function wired to
> > UART1.
> >
> > Add and enable the relevant device nodes for it.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> > ---
> >  .../dts/mediatek/mt8192-asurada-hayato-r1.dts | 80 +++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada-hayato-r1.dts b/arch/arm64/boot/dts/mediatek/mt8192-asurada-hayato-r1.dts
> > index 43a823990a92..6a7d7870525b 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8192-asurada-hayato-r1.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada-hayato-r1.dts
> > @@ -40,9 +40,89 @@ CROS_STD_MAIN_KEYMAP
> >       >;
> >  };
> >
> > +&pio {
> > +     bt_pins: bt-pins {
> > +             bt_kill: pins-bt-kill {
>
> Drop this label and for the other pinconfigs below as they'll never be
> referenced.

Ack.

> > +                     pinmux = <PINMUX_GPIO144__FUNC_GPIO144>; /* BT_KILL_L */
>
> I'd also drop this and the other comments, as they're already documented in the
> gpio-line-names property.

Ack.

> > +                     output-low;
> > +             };
> > +
> > +             bt_wake: pins-bt-wake {
> > +                     pinmux = <PINMUX_GPIO22__FUNC_GPIO22>;  /* bt to wake ap */
> > +                     bias-pull-up;
> > +             };
> > +
> > +             ap_wake_bt: pins-ap-wake-bt {
> > +                     pinmux = <PINMUX_GPIO168__FUNC_GPIO168>; /* AP_WAKE_BT_H */
> > +                     output-low;
> > +             };
> > +     };
> > +
> > +     uart1_pins: uart1-pins {
> > +             pins-rx {
> > +                     pinmux = <PINMUX_GPIO94__FUNC_URXD1>;
> > +                     input-enable;
> > +                     bias-pull-up;
> > +             };
> > +
> > +             pins-tx {
> > +                     pinmux = <PINMUX_GPIO95__FUNC_UTXD1>;
> > +             };
> > +
> > +             pins-cts {
> > +                     pinmux = <PINMUX_GPIO166__FUNC_UCTS1>;
> > +                     input-enable;
> > +             };
> > +
> > +             pins-rts {
> > +                     pinmux = <PINMUX_GPIO167__FUNC_URTS1>;
> > +                     output-enable;
>
> Looks like the dt-binding doesn't currently support output-enable, but the
> driver does, so please just add a patch with
>
>           output-enable: true
>
> on mediatek,mt8192-pinctrl.yaml

Looking at the driver and the datasheet, there is no actual output-enable
hardware. The driver simply sets an pin direction register, which is the
same thing done when setting the GPIO direction.

On the other hand, there is actually an input enable function, but the
driver also sets the pin direction. All pins have "input enable" set
by default in hardware doesn't help.

I'll drop this for now and look into fixing the pinctrl driver.

> > +             };
> > +     };
> > +
> > +     uart1_pins_sleep: uart1-pins-sleep {
>
> "-pins" needs to come last in the name otherwise the dt-binding will complain.

Ack.

> > +             pins-rx {
> > +                     pinmux = <PINMUX_GPIO94__FUNC_GPIO94>;
> > +                     input-enable;
> > +                     bias-pull-up;
> > +             };
> > +             pins-tx {
> > +                     pinmux = <PINMUX_GPIO95__FUNC_UTXD1>;
> > +             };
> > +             pins-cts {
> > +                     pinmux = <PINMUX_GPIO166__FUNC_UCTS1>;
> > +                     input-enable;
> > +             };
> > +             pins-rts {
> > +                     pinmux = <PINMUX_GPIO167__FUNC_URTS1>;
> > +                     output-enable;
> > +             };
> > +     };
> > +};
> > +
> >  &touchscreen {
> >       compatible = "hid-over-i2c";
> >       post-power-on-delay-ms = <10>;
> >       hid-descr-addr = <0x0001>;
> >       vdd-supply = <&pp3300_u>;
> >  };
> > +
> > +&uart1 {
> > +     status = "okay";
> > +     pinctrl-names = "default", "sleep";
> > +     pinctrl-0 = <&uart1_pins>;
> > +     pinctrl-1 = <&uart1_pins_sleep>;
> > +     /delete-property/ interrupts;
> > +     interrupts-extended = <&gic GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH 0>,
> > +                           <&pio 94 IRQ_TYPE_EDGE_FALLING>;
> > +
> > +     bluetooth: bluetooth {
>
> I'd also drop this label and only introduce it if/when needed.

Ack.

> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@xxxxxxxxxxxxx>
>
> Thanks,
> Nícolas
>
> > +             compatible = "realtek,rtl8822cs-bt";
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&bt_pins>;
> > +
> > +             enable-gpios = <&pio 144 GPIO_ACTIVE_HIGH>;
> > +             device-wake-gpios = <&pio 168 GPIO_ACTIVE_HIGH>;
> > +             host-wake-gpios = <&pio 22 GPIO_ACTIVE_LOW>;
> > +     };
> > +};
> > --
> > 2.40.0.634.g4ca3ef3211-goog
> >