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

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@ideasonboard.com> wrote:
> Add support for SW46-1 and SW46-2 as switches using the gpio-keys
> framework.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

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@linux-m68k.org

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

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

[Kieran Bingham]

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@ideasonboard.com> wrote:
>> Add support for SW46-1 and SW46-2 as switches using the gpio-keys
>> framework.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> 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 ...




> 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
> 

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

[Geert Uytterhoeven]

Hi Kieran,

On Thu, Sep 23, 2021 at 2:17 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
> On 23/09/2021 08:32, Geert Uytterhoeven wrote:
> > On Wed, Sep 22, 2021 at 10:30 PM Kieran Bingham
> > <kieran.bingham@ideasonboard.com> wrote:
> >> Add support for SW46-1 and SW46-2 as switches using the gpio-keys
> >> framework.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>         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..

That should work automatically, for a GPIO.

> Out of interest, is the OD buffer there to act as a hardware debounce or
> such? or is there another likely reason?

Perhaps to improve sharing of the GPIO through the expansion connector?
Other Renesas boards use the exact same input circuitry, with a
capacitor and resistor for debouncing, but without the OD buffer, and
they also provide access to the GPIO through an expansion connector.
It's even a plain buffer, without schmitt-trigger inputs.  Personally,
I would have taken one with schmitt-trigger functionality, if I would
have bothered with adding a buffer in the first place (but I'm not
a real hardware engineer ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

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@ideasonboard.com> wrote:
> >> Add support for SW46-1 and SW46-2 as switches using the gpio-keys
> >> framework.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > 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
> >