In commit 4352e23a7ff2 ("Input: cros-ec-keyb - only register keyboard if
rows/columns exist") we skipped registration of the keyboard if the
row/columns property didn't exist, but that has a slight problem for
existing DTBs. The DTBs have the rows/columns properties, so removing
the properties to indicate only switches exist makes this keyboard
driver fail to probe, resulting in broken power and volume buttons. Ease
the migration of existing DTBs by skipping keyboard registration if the
google,cros-ec-keyb-switches compatible exists.
The end result is that new DTBs can either choose to remove the matrix
keymap properties or leave them in place and add this new compatible
indicating the matrix keyboard properties should be ignored. Existing
DTBs will continue to work, but they will keep registering the keyboard
that does nothing. To fix that problem we can add this extra compatible
to existing DTBs and the keyboard will stop being registered. Finally,
if google,cros-ec-keyb is missing then this driver won't even attempt to
register the matrix keyboard.
Cc: Krzysztof Kozlowski <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: <[email protected]>
Cc: Benson Leung <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Hsin-Yi Wang <[email protected]>
Cc: "Joseph S. Barrera III" <[email protected]>
Fixes: 4352e23a7ff2 ("Input: cros-ec-keyb - only register keyboard if rows/columns exist")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/input/keyboard/cros_ec_keyb.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index eef909e52e23..1bbe2987bf52 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -536,6 +536,12 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
u32 *physmap;
u32 key_pos;
unsigned int row, col, scancode, n_physmap;
+ bool register_keyboard;
+
+ /* Skip matrix registration if no keyboard */
+ register_keyboard = device_get_match_data(dev);
+ if (!register_keyboard)
+ return 0;
/*
* No rows and columns? There isn't a matrix but maybe there are
@@ -718,8 +724,13 @@ static int cros_ec_keyb_remove(struct platform_device *pdev)
#ifdef CONFIG_OF
static const struct of_device_id cros_ec_keyb_of_match[] = {
- { .compatible = "google,cros-ec-keyb" },
- {},
+ {
+ /* Must be first */
+ .compatible = "google,cros-ec-keyb",
+ .data = (void *)true
+ },
+ { .compatible = "google,cros-ec-keyb-switches" },
+ {}
};
MODULE_DEVICE_TABLE(of, cros_ec_keyb_of_match);
#endif
--
https://chromeos.dev
Hi,
On Fri, Apr 29, 2022 at 4:31 PM Stephen Boyd <[email protected]> wrote:
>
> In commit 4352e23a7ff2 ("Input: cros-ec-keyb - only register keyboard if
> rows/columns exist") we skipped registration of the keyboard if the
> row/columns property didn't exist, but that has a slight problem for
> existing DTBs. The DTBs have the rows/columns properties, so removing
> the properties to indicate only switches exist makes this keyboard
> driver fail to probe, resulting in broken power and volume buttons. Ease
> the migration of existing DTBs by skipping keyboard registration if the
> google,cros-ec-keyb-switches compatible exists.
>
> The end result is that new DTBs can either choose to remove the matrix
> keymap properties or leave them in place and add this new compatible
> indicating the matrix keyboard properties should be ignored. Existing
> DTBs will continue to work, but they will keep registering the keyboard
> that does nothing. To fix that problem we can add this extra compatible
> to existing DTBs and the keyboard will stop being registered. Finally,
> if google,cros-ec-keyb is missing then this driver won't even attempt to
> register the matrix keyboard.
>
> Cc: Krzysztof Kozlowski <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: <[email protected]>
> Cc: Benson Leung <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Hsin-Yi Wang <[email protected]>
> Cc: "Joseph S. Barrera III" <[email protected]>
> Fixes: 4352e23a7ff2 ("Input: cros-ec-keyb - only register keyboard if rows/columns exist")
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/input/keyboard/cros_ec_keyb.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index eef909e52e23..1bbe2987bf52 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -536,6 +536,12 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
> u32 *physmap;
> u32 key_pos;
> unsigned int row, col, scancode, n_physmap;
> + bool register_keyboard;
> +
> + /* Skip matrix registration if no keyboard */
> + register_keyboard = device_get_match_data(dev);
> + if (!register_keyboard)
> + return 0;
>
> /*
> * No rows and columns? There isn't a matrix but maybe there are
As per my comments in patch #1, I wonder if it makes sense to delete
the "No rows and columns?" logic and settle on the compatible as the
one true way to specify this.
-Doug
Quoting Doug Anderson (2022-05-02 10:02:54)
> On Fri, Apr 29, 2022 at 4:31 PM Stephen Boyd <[email protected]> wrote:
> >
> >
> > diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> > index eef909e52e23..1bbe2987bf52 100644
> > --- a/drivers/input/keyboard/cros_ec_keyb.c
> > +++ b/drivers/input/keyboard/cros_ec_keyb.c
> > @@ -536,6 +536,12 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
> > u32 *physmap;
> > u32 key_pos;
> > unsigned int row, col, scancode, n_physmap;
> > + bool register_keyboard;
> > +
> > + /* Skip matrix registration if no keyboard */
> > + register_keyboard = device_get_match_data(dev);
> > + if (!register_keyboard)
> > + return 0;
> >
> > /*
> > * No rows and columns? There isn't a matrix but maybe there are
>
> As per my comments in patch #1, I wonder if it makes sense to delete
> the "No rows and columns?" logic and settle on the compatible as the
> one true way to specify this.
>
Ok. My only concern is that means we have to check for both compatibles
which is not really how DT compatible strings work. The compatible
string usually finds the more specific compatible that is first in the
list of compatibles in DT. You're essentially proposing that the
switches compatible could be first or last, the order doesn't matter.
If that isn't a problem then we can roll in a revert of commit
4352e23a7ff2 ("Input: cros-ec-keyb - only register keyboard if
rows/columns exist") and leave the rest of this patch alone and it will
implement this logic.
Hi,
On Mon, May 2, 2022 at 3:02 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Doug Anderson (2022-05-02 10:02:54)
> > On Fri, Apr 29, 2022 at 4:31 PM Stephen Boyd <[email protected]> wrote:
> > >
> > >
> > > diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> > > index eef909e52e23..1bbe2987bf52 100644
> > > --- a/drivers/input/keyboard/cros_ec_keyb.c
> > > +++ b/drivers/input/keyboard/cros_ec_keyb.c
> > > @@ -536,6 +536,12 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
> > > u32 *physmap;
> > > u32 key_pos;
> > > unsigned int row, col, scancode, n_physmap;
> > > + bool register_keyboard;
> > > +
> > > + /* Skip matrix registration if no keyboard */
> > > + register_keyboard = device_get_match_data(dev);
> > > + if (!register_keyboard)
> > > + return 0;
> > >
> > > /*
> > > * No rows and columns? There isn't a matrix but maybe there are
> >
> > As per my comments in patch #1, I wonder if it makes sense to delete
> > the "No rows and columns?" logic and settle on the compatible as the
> > one true way to specify this.
> >
>
> Ok. My only concern is that means we have to check for both compatibles
> which is not really how DT compatible strings work. The compatible
> string usually finds the more specific compatible that is first in the
> list of compatibles in DT. You're essentially proposing that the
> switches compatible could be first or last, the order doesn't matter.
It's not quite what I was proposing. I think my summary really sums it up:
1. If you have a matrix keyboard and maybe also some buttons/switches
then use the compatible: google,cros-ec-keyb
2. If you only have buttons/switches but you want to be compatible
with the old driver in Linux that looked for the compatible
"google,cros-ec-keyb" and required the matrix properties, use the
compatible: "google,cros-ec-keyb-switches", "google,cros-ec-keyb"
3. If you have only buttons/switches and don't need compatibility with
old Linux drivers, use the compatible: "google,cros-ec-keyb-switches"
...but just to say it another way:
* If you have the compatible "google,cros-ec-keyb-switches" I mean to
say that you _only_ have switches and buttons. You'd _never_ have this
compatible string if you truly have a matrix keyboard. If you have
this, it will always be first.
* If you only have switches and buttons but you care about backward
compatibility then you can add a fallback compatible second:
"google,cros-ec-keyb"
* In order for the fallback compatible to be at all useful as a
fallback (it's only useful at all if you're on an old driver), if you
specify it you should pretend that you have matrix properties even
though you don't really have them, just like we used to do.
-Doug
Quoting Doug Anderson (2022-05-02 18:06:39)
> Hi,
>
> On Mon, May 2, 2022 at 3:02 PM Stephen Boyd <[email protected]> wrote:
> >
> > Quoting Doug Anderson (2022-05-02 10:02:54)
> > > On Fri, Apr 29, 2022 at 4:31 PM Stephen Boyd <[email protected]> wrote:
> > > >
> > > >
> > > > diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> > > > index eef909e52e23..1bbe2987bf52 100644
> > > > --- a/drivers/input/keyboard/cros_ec_keyb.c
> > > > +++ b/drivers/input/keyboard/cros_ec_keyb.c
> > > > @@ -536,6 +536,12 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
> > > > u32 *physmap;
> > > > u32 key_pos;
> > > > unsigned int row, col, scancode, n_physmap;
> > > > + bool register_keyboard;
> > > > +
> > > > + /* Skip matrix registration if no keyboard */
> > > > + register_keyboard = device_get_match_data(dev);
> > > > + if (!register_keyboard)
> > > > + return 0;
> > > >
> > > > /*
> > > > * No rows and columns? There isn't a matrix but maybe there are
> > >
> > > As per my comments in patch #1, I wonder if it makes sense to delete
> > > the "No rows and columns?" logic and settle on the compatible as the
> > > one true way to specify this.
> > >
> >
> > Ok. My only concern is that means we have to check for both compatibles
> > which is not really how DT compatible strings work. The compatible
> > string usually finds the more specific compatible that is first in the
> > list of compatibles in DT. You're essentially proposing that the
> > switches compatible could be first or last, the order doesn't matter.
>
> It's not quite what I was proposing. I think my summary really sums it up:
Alright, I'm glad I misunderstood.
>
> 1. If you have a matrix keyboard and maybe also some buttons/switches
> then use the compatible: google,cros-ec-keyb
>
> 2. If you only have buttons/switches but you want to be compatible
> with the old driver in Linux that looked for the compatible
> "google,cros-ec-keyb" and required the matrix properties, use the
> compatible: "google,cros-ec-keyb-switches", "google,cros-ec-keyb"
>
> 3. If you have only buttons/switches and don't need compatibility with
> old Linux drivers, use the compatible: "google,cros-ec-keyb-switches"
>
> ...but just to say it another way:
>
> * If you have the compatible "google,cros-ec-keyb-switches" I mean to
> say that you _only_ have switches and buttons. You'd _never_ have this
> compatible string if you truly have a matrix keyboard. If you have
> this, it will always be first.
>
> * If you only have switches and buttons but you care about backward
> compatibility then you can add a fallback compatible second:
> "google,cros-ec-keyb"
>
> * In order for the fallback compatible to be at all useful as a
> fallback (it's only useful at all if you're on an old driver), if you
> specify it you should pretend that you have matrix properties even
> though you don't really have them, just like we used to do.
>
Another important point is that the matrix properties are willfully
ignored by the new driver if the "google,cros-ec-keyb-switches"
compatible is present. Maybe it should be "google,cros-ec-no-keyb" to
describe the true intent, i.e. ignore the keyboard properties. Or
"google,cros-ec-keyboardless". I think it's confusing that I put
"switches" in the compatible. It really should be about not registering
the keyboard input device.
Anyway, I agree that we don't need to use the matrix keyboard properties
to figure out what to do. In fact, it isn't possible to remove the
properties if "google,cros-ec-keyb" is present, so checking for them is
redundant.