| Message ID | 20220426225748.324759-1-swboyd@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: qcom: sc7180-trogdor: Remove cros-ec keyboard from detachables | expand |
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>
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
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/
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
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 >
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
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>;
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(+)