[PATCH] arm64: dts: qcom: sc7180-trogdor: Remove cros-ec keyboard from detachables

[PATCH] arm64: dts: qcom: sc7180-trogdor: Remove cros-ec keyboard from detachables

[Stephen Boyd]

Trogdor devices that have a detachable keyboard still have a
non-detachable keyboard input device present because we include the
cros-ec-keyboard.dtsi snippet in the top-level sc7180-trogdor.dtsi file
that every variant board includes. We do this because the
keyboard-controller node also provides some buttons like the power
button and volume buttons. Unfortunately, this means we register a
keyboard input device that doesn't do anything on boards with a
detachable keyboard. Let's delete the rows/columns properties of the
device node to indicate that there isn't a matrix keyboard on these
boards.

Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Hsin-Yi Wang <hsinyi@chromium.org>
Cc: "Joseph S. Barrera III" <joebar@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi   | 5 +++++
 arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
index c81805ef2250..4173623cc241 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
@@ -119,6 +119,11 @@ &i2c9 {
 	status = "disabled";
 };
 
+&keyboard_controller {
+	/delete-property/keypad,num-rows;
+	/delete-property/keypad,num-columns;
+};
+
 &panel {
 	compatible = "boe,nv110wtm-n61";
 };
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
index bff2b556cc75..7205062e88b4 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
@@ -121,6 +121,11 @@ &camcc {
 	status = "okay";
 };
 
+&keyboard_controller {
+	/delete-property/keypad,num-rows;
+	/delete-property/keypad,num-columns;
+};
+
 &panel {
 	compatible = "samsung,atna33xc20";
 	enable-gpios = <&tlmm 12 GPIO_ACTIVE_HIGH>;
-- 
https://chromeos.dev

Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Remove cros-ec keyboard from detachables

[Doug Anderson]

Hi,

On Tue, Apr 26, 2022 at 3:57 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Trogdor devices that have a detachable keyboard still have a
> non-detachable keyboard input device present because we include the
> cros-ec-keyboard.dtsi snippet in the top-level sc7180-trogdor.dtsi file
> that every variant board includes. We do this because the
> keyboard-controller node also provides some buttons like the power
> button and volume buttons. Unfortunately, this means we register a
> keyboard input device that doesn't do anything on boards with a
> detachable keyboard. Let's delete the rows/columns properties of the
> device node to indicate that there isn't a matrix keyboard on these
> boards.
>
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Hsin-Yi Wang <hsinyi@chromium.org>
> Cc: "Joseph S. Barrera III" <joebar@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi   | 5 +++++
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi | 5 +++++
>  2 files changed, 10 insertions(+)

Presumably we need to do this same thing for wormdingler [1]

[1] https://lore.kernel.org/r/20220426151204.1.Id2821de5fde55ebe928e8fc87a71c8d535edb383@changeid

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Remove cros-ec keyboard from detachables

[Doug Anderson]

Hi,

On Tue, Apr 26, 2022 at 5:17 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Apr 26, 2022 at 3:57 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Trogdor devices that have a detachable keyboard still have a
> > non-detachable keyboard input device present because we include the
> > cros-ec-keyboard.dtsi snippet in the top-level sc7180-trogdor.dtsi file
> > that every variant board includes. We do this because the
> > keyboard-controller node also provides some buttons like the power
> > button and volume buttons. Unfortunately, this means we register a
> > keyboard input device that doesn't do anything on boards with a
> > detachable keyboard. Let's delete the rows/columns properties of the
> > device node to indicate that there isn't a matrix keyboard on these
> > boards.
> >
> > Cc: Benson Leung <bleung@chromium.org>
> > Cc: Guenter Roeck <groeck@chromium.org>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Hsin-Yi Wang <hsinyi@chromium.org>
> > Cc: "Joseph S. Barrera III" <joebar@chromium.org>
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi   | 5 +++++
> >  arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi | 5 +++++
> >  2 files changed, 10 insertions(+)
>
> Presumably we need to do this same thing for wormdingler [1]
>
> [1] https://lore.kernel.org/r/20220426151204.1.Id2821de5fde55ebe928e8fc87a71c8d535edb383@changeid
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Do we need to delay landing this patch for a release? I haven't tested
myself, but from re-reading through the code it looks as if
cros_ec_keyb_register_matrix() will return an error code if we have
the device tree patch _without_ commit 4352e23a7ff2 ("Input:
cros-ec-keyb - only register keyboard if rows/columns exist"). That
will cause it to skip registering the buttons/switches, right?

-Doug

Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Remove cros-ec keyboard from detachables

[Stephen Boyd]

Quoting Doug Anderson (2022-04-27 08:09:59)
> Hi,
>
> On Tue, Apr 26, 2022 at 5:17 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Tue, Apr 26, 2022 at 3:57 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Trogdor devices that have a detachable keyboard still have a
> > > non-detachable keyboard input device present because we include the
> > > cros-ec-keyboard.dtsi snippet in the top-level sc7180-trogdor.dtsi file
> > > that every variant board includes. We do this because the
> > > keyboard-controller node also provides some buttons like the power
> > > button and volume buttons. Unfortunately, this means we register a
> > > keyboard input device that doesn't do anything on boards with a
> > > detachable keyboard. Let's delete the rows/columns properties of the
> > > device node to indicate that there isn't a matrix keyboard on these
> > > boards.
> > >
> > > Cc: Benson Leung <bleung@chromium.org>
> > > Cc: Guenter Roeck <groeck@chromium.org>
> > > Cc: Douglas Anderson <dianders@chromium.org>
> > > Cc: Hsin-Yi Wang <hsinyi@chromium.org>
> > > Cc: "Joseph S. Barrera III" <joebar@chromium.org>
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi   | 5 +++++
> > >  arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi | 5 +++++
> > >  2 files changed, 10 insertions(+)
> >
> > Presumably we need to do this same thing for wormdingler [1]
> >
> > [1] https://lore.kernel.org/r/20220426151204.1.Id2821de5fde55ebe928e8fc87a71c8d535edb383@changeid
> >
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> Do we need to delay landing this patch for a release? I haven't tested
> myself, but from re-reading through the code it looks as if
> cros_ec_keyb_register_matrix() will return an error code if we have
> the device tree patch _without_ commit 4352e23a7ff2 ("Input:
> cros-ec-keyb - only register keyboard if rows/columns exist"). That
> will cause it to skip registering the buttons/switches, right?

Yes, if the driver patch isn't applied then we'll skip registering
switches when these properties are removed. I suppose a better way to
gracefully migrate the logic here would be to add another compatible
string. Then we could make the compatible be

	compatible = "google,cros-ec-keyb-switches", "google,cros-ec-keyb";

on detachables and the driver can skip registering the keyboard if the more
specific "google,cros-ec-keyb-switches" compatible is present. The
driver will continue to probe and we don't have to remove any
properties.

The driver patch has been accepted[1] so in theory this patch can be
applied and when the two meet up in linux-next things will work but when
bisecting down the DTS the switches won't work. Not a huge problem but
sort of annoying that the switches are busted.

[1] https://lore.kernel.org/all/Ymh1J9zQ+5EyQadE@google.com/

Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Remove cros-ec keyboard from detachables

[Stephen Boyd]

Quoting Stephen Boyd (2022-04-27 11:49:25)
>
> Yes, if the driver patch isn't applied then we'll skip registering
> switches when these properties are removed. I suppose a better way to
> gracefully migrate the logic here would be to add another compatible
> string. Then we could make the compatible be
>
>         compatible = "google,cros-ec-keyb-switches", "google,cros-ec-keyb";
>
> on detachables and the driver can skip registering the keyboard if the more
> specific "google,cros-ec-keyb-switches" compatible is present. The
> driver will continue to probe and we don't have to remove any
> properties.
>

I implemented this at
https://lore.kernel.org/r/20220427203026.828183-1-swboyd@chromium.org

Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Remove cros-ec keyboard from detachables

[Bjorn Andersson]

On Tue 26 Apr 17:57 CDT 2022, Stephen Boyd wrote:

> Trogdor devices that have a detachable keyboard still have a
> non-detachable keyboard input device present because we include the
> cros-ec-keyboard.dtsi snippet in the top-level sc7180-trogdor.dtsi file
> that every variant board includes. We do this because the
> keyboard-controller node also provides some buttons like the power
> button and volume buttons. Unfortunately, this means we register a
> keyboard input device that doesn't do anything on boards with a
> detachable keyboard. Let's delete the rows/columns properties of the
> device node to indicate that there isn't a matrix keyboard on these
> boards.
> 

As this seems to directly relate to the final design of each device,
would it make sense to push out the &keyboard_controller from
trogdor.dtsi? Or do you think it would be too much duplication for it to
be worth it?

Regards,
Bjorn

> Cc: Benson Leung <bleung@chromium.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Hsin-Yi Wang <hsinyi@chromium.org>
> Cc: "Joseph S. Barrera III" <joebar@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi   | 5 +++++
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> index c81805ef2250..4173623cc241 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> @@ -119,6 +119,11 @@ &i2c9 {
>  	status = "disabled";
>  };
>  
> +&keyboard_controller {
> +	/delete-property/keypad,num-rows;
> +	/delete-property/keypad,num-columns;
> +};
> +
>  &panel {
>  	compatible = "boe,nv110wtm-n61";
>  };
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
> index bff2b556cc75..7205062e88b4 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-homestar.dtsi
> @@ -121,6 +121,11 @@ &camcc {
>  	status = "okay";
>  };
>  
> +&keyboard_controller {
> +	/delete-property/keypad,num-rows;
> +	/delete-property/keypad,num-columns;
> +};
> +
>  &panel {
>  	compatible = "samsung,atna33xc20";
>  	enable-gpios = <&tlmm 12 GPIO_ACTIVE_HIGH>;
> -- 
> https://chromeos.dev
> 

Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Remove cros-ec keyboard from detachables

[Stephen Boyd]

Quoting Bjorn Andersson (2022-05-05 20:40:22)
> On Tue 26 Apr 17:57 CDT 2022, Stephen Boyd wrote:
>
> > Trogdor devices that have a detachable keyboard still have a
> > non-detachable keyboard input device present because we include the
> > cros-ec-keyboard.dtsi snippet in the top-level sc7180-trogdor.dtsi file
> > that every variant board includes. We do this because the
> > keyboard-controller node also provides some buttons like the power
> > button and volume buttons. Unfortunately, this means we register a
> > keyboard input device that doesn't do anything on boards with a
> > detachable keyboard. Let's delete the rows/columns properties of the
> > device node to indicate that there isn't a matrix keyboard on these
> > boards.
> >
>
> As this seems to directly relate to the final design of each device,
> would it make sense to push out the &keyboard_controller from
> trogdor.dtsi? Or do you think it would be too much duplication for it to
> be worth it?

I tried it out a few days ago but I'm waiting to see how the driver
patch goes before sending the dts bits. See the WIP patches up to the
end of the chain on chromium gerrit[1].

[1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3609017