| Message ID | 20220429233112.2851665-3-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: cros-ec-keyb: Better matrixless support | expand |
Hi, On Fri, Apr 29, 2022 at 4:31 PM Stephen Boyd <swboyd@chromium.org> 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 <krzk@kernel.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: <devicetree@vger.kernel.org> > 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> > Fixes: 4352e23a7ff2 ("Input: cros-ec-keyb - only register keyboard if rows/columns exist") > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > 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 <swboyd@chromium.org> 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 <swboyd@chromium.org> wrote: > > Quoting Doug Anderson (2022-05-02 10:02:54) > > On Fri, Apr 29, 2022 at 4:31 PM Stephen Boyd <swboyd@chromium.org> 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 <swboyd@chromium.org> wrote: > > > > Quoting Doug Anderson (2022-05-02 10:02:54) > > > On Fri, Apr 29, 2022 at 4:31 PM Stephen Boyd <swboyd@chromium.org> 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.
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
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 <krzk@kernel.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: <devicetree@vger.kernel.org> 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> Fixes: 4352e23a7ff2 ("Input: cros-ec-keyb - only register keyboard if rows/columns exist") Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- drivers/input/keyboard/cros_ec_keyb.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)