Re: [PATCH] arm64: dts: renesas: r8a779a0: falcon-cpu: Add SW46 switch support

[Geert Uytterhoeven]

Hi Kieran,

CC input

On Wed, Sep 22, 2021 at 10:30 PM Kieran Bingham
<kieran.bingham@xxxxxxxxxxxxxxxx> wrote:
> Add support for SW46-1 and SW46-2 as switches using the gpio-keys
> framework.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>

Thanks for your patch!

> ---
>
> SW_LID and SW_DOCK are selected as low-impact switch events for the
> default configuration. Would SW_RFKILL_ALL, and SW_MUTE_DEVICE be
> preferred as more 'functional' defaults? (I have otherwise avoided these
> to hopefully prevent unwanted / undocumented effects occuring on
> development hardware running a full interface which may parse these)
>
> I'd expect them to be overridden by any platform using them anyway.

That's a good question

BTW, I'm happy you brought this up.  I discovered EV_SW only
recently, and had just started wondering whether we should use it
for the various slide switches on Renesas R-Car Gen2 and Gen3 boards,
which are modelled using the default EV_KEY and KEY_[1-4].

I see several DTS files using EV_SW (or hardcoded 5) with KEY_*
codes instead of EV_* codes, so perhaps KEY_A or KEY_B would be
suited better, to avoid strange effects? But SW_LID (and KEY_RESERVED)
seem to be quite popular, too.

Any input^Wgood advice from the input people? TIA!

> --- a/arch/arm64/boot/dts/renesas/r8a779a0-falcon-cpu.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a779a0-falcon-cpu.dtsi
> @@ -52,6 +52,24 @@ keys {
>                 pinctrl-0 = <&keys_pins>;
>                 pinctrl-names = "default";
>
> +               sw-1 {
> +                       gpios = <&gpio1 28 GPIO_ACTIVE_LOW>;
> +                       linux,code = <SW_LID>;
> +                       linux,input-type = <EV_SW>;
> +                       label = "SW46-1";
> +                       wakeup-source;
> +                       debounce-interval = <20>;
> +               };
> +
> +               sw-2 {
> +                       gpios = <&gpio1 29 GPIO_ACTIVE_LOW>;
> +                       linux,code = <SW_DOCK>;
> +                       linux,input-type = <EV_SW>;
> +                       label = "SW46-2";
> +                       wakeup-source;
> +                       debounce-interval = <20>;
> +               };
> +
>                 key-1 {
>                         gpios = <&gpio6 18 GPIO_ACTIVE_LOW>;
>                         linux,code = <KEY_1>;

Looks good to me.

> @@ -193,7 +211,8 @@ i2c6_pins: i2c6 {
>         };
>
>         keys_pins: keys {
> -               pins = "GP_6_18", "GP_6_19", "GP_6_20";
> +               pins = "GP_1_28", "GP_1_29",
> +                      "GP_6_18", "GP_6_19", "GP_6_20";
>                 bias-pull-up;
>         };

This part is not needed, as the GPIOs connected to the slide switches
have external pull-up resistors (unlike the GPIOs connected to the
push switches, which are driven low by open-drain buffers, without
external pull-up resistors).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds