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 devicetrees 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. Of course, this driver won't
probe until this patch is applied in that scenario, but that's OK. This
last case is likely only going to be used by new devicetrees created
after this commit.
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 | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index eef909e52e23..04c550aaf897 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -536,14 +536,11 @@ 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;
- /*
- * No rows and columns? There isn't a matrix but maybe there are
- * switches to register in cros_ec_keyb_register_bs() because
- * this is a detachable device.
- */
- if (!device_property_present(dev, "keypad,num-rows") &&
- !device_property_present(dev, "keypad,num-cols"))
+ /* Skip matrix registration if no keyboard */
+ register_keyboard = device_get_match_data(dev);
+ if (!register_keyboard)
return 0;
err = matrix_keypad_parse_properties(dev, &ckdev->rows, &ckdev->cols);
@@ -718,8 +715,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
Quoting Doug Anderson (2022-05-03 08:09:11)
> On Mon, May 2, 2022 at 9:22 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..04c550aaf897 100644
> > --- a/drivers/input/keyboard/cros_ec_keyb.c
> > +++ b/drivers/input/keyboard/cros_ec_keyb.c
> > @@ -536,14 +536,11 @@ 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;
> >
> > - /*
> > - * No rows and columns? There isn't a matrix but maybe there are
> > - * switches to register in cros_ec_keyb_register_bs() because
> > - * this is a detachable device.
> > - */
> > - if (!device_property_present(dev, "keypad,num-rows") &&
> > - !device_property_present(dev, "keypad,num-cols"))
> > + /* Skip matrix registration if no keyboard */
> > + register_keyboard = device_get_match_data(dev);
> > + if (!register_keyboard)
> > return 0;
>
> I'm a little on the fence about the local variable. It could have been
> shorter as:
>
> /* Skip matrix registration if no keyboard */
> if (!device_get_match_data(dev))
>
> ...but I guess the "register_keyboard" maybe makes it more a little
> more obvious?
Yes, the idea is to make it more obvious to the point that the comment
isn't needed. I'll change it to 'has_keyboard' and then drop the
comment!
>
> In any case, I'm happy either way:
>
> Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Mon, May 2, 2022 at 9:22 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 devicetrees 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. Of course, this driver won't
> probe until this patch is applied in that scenario, but that's OK. This
> last case is likely only going to be used by new devicetrees created
> after this commit.
>
> 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 | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index eef909e52e23..04c550aaf897 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -536,14 +536,11 @@ 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;
>
> - /*
> - * No rows and columns? There isn't a matrix but maybe there are
> - * switches to register in cros_ec_keyb_register_bs() because
> - * this is a detachable device.
> - */
> - if (!device_property_present(dev, "keypad,num-rows") &&
> - !device_property_present(dev, "keypad,num-cols"))
> + /* Skip matrix registration if no keyboard */
> + register_keyboard = device_get_match_data(dev);
> + if (!register_keyboard)
> return 0;
I'm a little on the fence about the local variable. It could have been
shorter as:
/* Skip matrix registration if no keyboard */
if (!device_get_match_data(dev))
...but I guess the "register_keyboard" maybe makes it more a little
more obvious?
In any case, I'm happy either way:
Reviewed-by: Douglas Anderson <[email protected]>