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

[Kieran Bingham]

Is there anyone from linux-input that can help with the feedback
requested below please?

I've held off sending my v2 of this patch awaiting to see if anyone has
further comments on the use of 'generic' test switches.

...

Quoting Kieran Bingham (2021-09-23 13:17:09)
> On 23/09/2021 08:32, Geert Uytterhoeven wrote:
> > 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
> 
> I hoped it would start a discussion ;-) I noticed no one else was using
> EV_SW ... and ... well the slide switches just aren't buttons ;-)
> 
> > 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].
> 
> Indeed, that was my dilemma - there isn't really a 'generic' zero-impact
> choice for the slide switches. They all imply that they are likely to be
> interpreted by a window manager / gui to make some adjustment to the system.
> 
> Which is of course desired in a product/device - but on a test board
> like the evaluation modules - I can imagine someone saying they can't
> understand why the screen isn't working / is in powersave ... because
> ... of the undocumented feature that the SW46-1 position indicating that
> the 'lid' is closed ...
> 

We have evaluation boards, with switches and buttons. Buttons are not so
painful for an impact, as they default to 'not-pressed' ... however a
switch ... always has a position.

>From my understanding of the available switch codes, they all imply some
sort of system configuration, meaning that the position of the switch
may be likely to cause an effect in a graphical user interface when
running on these boards if the switches are configured to be one of
those keys.

Is there an accepted default/test key already that should be used? or
should I propose a new code which is not expected to have any effects
other than report its position?

--
Kieran

> > 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.
> 
> It feels 'horrible' reporting Key events on switch events ... but if
> it's an approved solution - I'm fine with that.
> 
> As long as there is no further side impact of suddenly 'KEY_B' is
> constantly pressed, and so the WM is going to act as though a key
> modifier is active ...
> 
> 
> > Any input^Wgood advice from the input people? TIA!
> 
> Yes please ;-)
> 
> Maybe we need some 'test' keys / events that can be hooked up that allow
> testing/validation but represent that these keys/switches/buttons have
> no current definition for their operation...
> 
> They're just generic buttons and switches ..
> 
> > 
> >> --- 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).
> > 
> 
> Ah - for some reason I thought it was required to configure the PFC
> regardless, and show that these pins are acquired by the gpio function -
> but of course I'd expect 'getting' the gpio would do that..
> 
> I'll await some feedback on the best key codes to use before reposting.
> 
> 
> Out of interest, is the OD buffer there to act as a hardware debounce or
> such? or is there another likely reason?
> 
> --
> Kieran
> 
> 
> 
> > Gr{oetje,eeting}s,
> > 
> >                         Geert
> >