| Message ID | 20220429233112.2851665-2-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: > > If the device is a detachable, this device won't have a matrix keyboard > but it may have some button switches, e.g. volume buttons and power > buttons. Let's add a more specific compatible for this type of device > that indicates to the OS that there are only switches and no matrix > keyboard present. If only the switches compatible is present, then the > matrix keyboard properties are denied. This lets us gracefully migrate > devices that only have switches over to the new compatible string. I know the history here so I know the reasons for the 3 choices, but I'm not sure I'd fully understand it just from the description above. Maybe a summary in the CL desc would help? Summary: 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" > Similarly, start enforcing that the keypad rows/cols and keymap > properties exist if the google,cros-ec-keyb compatible is present. This > more clearly describes what the driver is expecting, i.e. that the > kernel driver will fail to probe if the row or column or keymap > properties are missing and only the google,cros-ec-keyb compatible is > present. > > 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> > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > --- > .../bindings/input/google,cros-ec-keyb.yaml | 95 +++++++++++++++++-- > 1 file changed, 89 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml > index e8f137abb03c..c1b079449cf3 100644 > --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml > +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml > @@ -15,14 +15,19 @@ description: | > Google's ChromeOS EC Keyboard is a simple matrix keyboard > implemented on a separate EC (Embedded Controller) device. It provides > a message for reading key scans from the EC. These are then converted > - into keycodes for processing by the kernel. > - > -allOf: > - - $ref: "/schemas/input/matrix-keymap.yaml#" > + into keycodes for processing by the kernel. This device also supports > + switches/buttons like power and volume buttons. > > properties: > compatible: > - const: google,cros-ec-keyb > + oneOf: > + - items: > + - const: google,cros-ec-keyb-switches > + - items: > + - const: google,cros-ec-keyb-switches > + - const: google,cros-ec-keyb > + - items: > + - const: google,cros-ec-keyb > > google,needs-ghost-filter: > description: > @@ -41,15 +46,40 @@ properties: > where the lower 16 bits are reserved. This property is specified only > when the keyboard has a custom design for the top row keys. > > +dependencies: > + function-row-phsymap: [ 'linux,keymap' ] > + google,needs-ghost-filter: [ 'linux,keymap' ] > + > required: > - compatible > > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: google,cros-ec-keyb > + then: > + allOf: > + - $ref: "/schemas/input/matrix-keymap.yaml#" > + - if: > + not: > + properties: > + compatible: > + contains: > + const: google,cros-ec-keyb-switches > + then: > + required: > + - keypad,num-rows > + - keypad,num-columns > + - linux,keymap I think that: 1. If you only have buttons/switches and care about backward compatibility, you use the "two compatibles" version. 2. If you care about backward compatibility then you _must_ include the matrix properties. Thus I would be tempted to say that we should just have one "if" test that says that if matrix properties are allowed then they're also required. That goes against the recently landed commit 4352e23a7ff2 ("Input: cros-ec-keyb - only register keyboard if rows/columns exist") but perhaps we should just _undo_ that that since it landed pretty recently and say that the truly supported way to specify that you only have keyboards/switches is with the compatible. What do you think? -Doug
On Mon, May 2, 2022 at 10:00 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Fri, Apr 29, 2022 at 4:31 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > If the device is a detachable, this device won't have a matrix keyboard > > but it may have some button switches, e.g. volume buttons and power > > buttons. Let's add a more specific compatible for this type of device > > that indicates to the OS that there are only switches and no matrix > > keyboard present. If only the switches compatible is present, then the > > matrix keyboard properties are denied. This lets us gracefully migrate > > devices that only have switches over to the new compatible string. > > I know the history here so I know the reasons for the 3 choices, but > I'm not sure I'd fully understand it just from the description above. > Maybe a summary in the CL desc would help? > > Summary: > > 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" > > > > Similarly, start enforcing that the keypad rows/cols and keymap > > properties exist if the google,cros-ec-keyb compatible is present. This > > more clearly describes what the driver is expecting, i.e. that the > > kernel driver will fail to probe if the row or column or keymap > > properties are missing and only the google,cros-ec-keyb compatible is > > present. > > > > 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> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org> > > --- > > .../bindings/input/google,cros-ec-keyb.yaml | 95 +++++++++++++++++-- > > 1 file changed, 89 insertions(+), 6 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml > > index e8f137abb03c..c1b079449cf3 100644 > > --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml > > +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml > > @@ -15,14 +15,19 @@ description: | > > Google's ChromeOS EC Keyboard is a simple matrix keyboard > > implemented on a separate EC (Embedded Controller) device. It provides > > a message for reading key scans from the EC. These are then converted > > - into keycodes for processing by the kernel. > > - > > -allOf: > > - - $ref: "/schemas/input/matrix-keymap.yaml#" > > + into keycodes for processing by the kernel. This device also supports > > + switches/buttons like power and volume buttons. > > > > properties: > > compatible: > > - const: google,cros-ec-keyb > > + oneOf: > > + - items: > > + - const: google,cros-ec-keyb-switches > > + - items: > > + - const: google,cros-ec-keyb-switches > > + - const: google,cros-ec-keyb > > + - items: > > + - const: google,cros-ec-keyb > > > > google,needs-ghost-filter: > > description: > > @@ -41,15 +46,40 @@ properties: > > where the lower 16 bits are reserved. This property is specified only > > when the keyboard has a custom design for the top row keys. > > > > +dependencies: > > + function-row-phsymap: [ 'linux,keymap' ] > > + google,needs-ghost-filter: [ 'linux,keymap' ] > > + > > required: > > - compatible > > > > +allOf: > > + - if: > > + properties: > > + compatible: > > + contains: > > + const: google,cros-ec-keyb > > + then: > > + allOf: > > + - $ref: "/schemas/input/matrix-keymap.yaml#" > > + - if: > > + not: > > + properties: > > + compatible: > > + contains: > > + const: google,cros-ec-keyb-switches > > + then: > > + required: > > + - keypad,num-rows > > + - keypad,num-columns > > + - linux,keymap > > I think that: > > 1. If you only have buttons/switches and care about backward > compatibility, you use the "two compatibles" version. > > 2. If you care about backward compatibility then you _must_ include > the matrix properties. > > Thus I would be tempted to say that we should just have one "if" test > that says that if matrix properties are allowed then they're also > required. > > That goes against the recently landed commit 4352e23a7ff2 ("Input: > cros-ec-keyb - only register keyboard if rows/columns exist") but > perhaps we should just _undo_ that that since it landed pretty > recently and say that the truly supported way to specify that you only > have keyboards/switches is with the compatible. > > What do you think? I am sorry, I am still confused on what exactly we are trying to solve here? Having a device with the new device tree somehow run an older kernel and fail? Why exactly do we care about this case? We have implemented the notion that without rows/columns properties we will not be creating input device for the matrix portion, all older devices should have it defined, so the newer driver is compatible with them... Thanks.
Quoting Dmitry Torokhov (2022-05-02 10:43:06) > On Mon, May 2, 2022 at 10:00 AM Doug Anderson <dianders@chromium.org> wrote: > > > > That goes against the recently landed commit 4352e23a7ff2 ("Input: > > cros-ec-keyb - only register keyboard if rows/columns exist") but > > perhaps we should just _undo_ that that since it landed pretty > > recently and say that the truly supported way to specify that you only > > have keyboards/switches is with the compatible. > > > > What do you think? > > I am sorry, I am still confused on what exactly we are trying to solve > here? Having a device with the new device tree somehow run an older > kernel and fail? Why exactly do we care about this case? Yes, we're trying to solve the problem where a new device tree is used with an older kernel because it doesn't have the driver patch to only create an input device for the matrix when rows/columns properties are present. Otherwise applying that devicetree patch to an older kernel will break bisection. > We have > implemented the notion that without rows/columns properties we will > not be creating input device for the matrix portion, all older devices > should have it defined, so the newer driver is compatible with them... > Agreed, that solves half the problem. This new compatible eases integration so that devicetrees can say they're compatible with the old binding that _requires_ the rows/column properties. By making the driver change we loosened that requirement, but the binding should have been making the properties required at the start because it fails to bind otherwise. My interpretation of what Doug is saying is that we should maintain that requirement that rows/columns exists if the original compatible google,cros-ec-keyb is present and use the new compatible to indicate that there are switches. Combining the two compatibles means there's switches and a matrix keyboard, having only the switches compatible means only switches, and having only the keyboard compatible means only matrix keyboard. It sounds OK to me.
Quoting Stephen Boyd (2022-05-02 13:41:33) > Quoting Dmitry Torokhov (2022-05-02 10:43:06) > > > We have > > implemented the notion that without rows/columns properties we will > > not be creating input device for the matrix portion, all older devices > > should have it defined, so the newer driver is compatible with them... > > > > Agreed, that solves half the problem. This new compatible eases > integration so that devicetrees can say they're compatible with the old > binding that _requires_ the rows/column properties. By making the driver > change we loosened that requirement, but the binding should have been > making the properties required at the start because it fails to bind > otherwise. > > My interpretation of what Doug is saying is that we should maintain that > requirement that rows/columns exists if the original compatible > google,cros-ec-keyb is present and use the new compatible to indicate > that there are switches. Combining the two compatibles means there's > switches and a matrix keyboard, having only the switches compatible > means only switches, and having only the keyboard compatible means only > matrix keyboard. > > It sounds OK to me. There's one more thing to mention. The switches are discovered by querying the EC. Reverting commit 4352e23a7ff2 ("Input: cros-ec-keyb - only register keyboard if rows/columns exist") makes it so that in the case you have a keyboard and switches you'll be tempted to define both compatibles because you have some switches, but for all practical purposes you don't need to change anything. The EC will still be queried for the switches. Maybe "google,cros-ec-keyb-switches" is a bad name. It should really be "google,cros-ec-keyb-v2" or "google,cros-ec-keyb-optional" where we clarify that matrix keyboard properties are optional now and are used to indicate if there's a matrix keyboard or not.
Hi, On Mon, May 2, 2022 at 1:41 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Dmitry Torokhov (2022-05-02 10:43:06) > > On Mon, May 2, 2022 at 10:00 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > That goes against the recently landed commit 4352e23a7ff2 ("Input: > > > cros-ec-keyb - only register keyboard if rows/columns exist") but > > > perhaps we should just _undo_ that that since it landed pretty > > > recently and say that the truly supported way to specify that you only > > > have keyboards/switches is with the compatible. > > > > > > What do you think? > > > > I am sorry, I am still confused on what exactly we are trying to solve > > here? Having a device with the new device tree somehow run an older > > kernel and fail? Why exactly do we care about this case? > > Yes, we're trying to solve the problem where a new device tree is used > with an older kernel because it doesn't have the driver patch to only > create an input device for the matrix when rows/columns properties are > present. Otherwise applying that devicetree patch to an older kernel > will break bisection. I mean, we can also just say that we don't care about breaking bisections and just say that the solution we already landed is fine. It would certainly be less work at this point. > > > We have > > implemented the notion that without rows/columns properties we will > > not be creating input device for the matrix portion, all older devices > > should have it defined, so the newer driver is compatible with them... > > > > Agreed, that solves half the problem. This new compatible eases > integration so that devicetrees can say they're compatible with the old > binding that _requires_ the rows/column properties. By making the driver > change we loosened that requirement, but the binding should have been > making the properties required at the start because it fails to bind > otherwise. > > My interpretation of what Doug is saying is that we should maintain that > requirement that rows/columns exists if the original compatible > google,cros-ec-keyb is present and use the new compatible to indicate > that there are switches. Combining the two compatibles means there's > switches and a matrix keyboard, having only the switches compatible > means only switches, and having only the keyboard compatible means only > matrix keyboard. That's not quite what I was saying. See my response to patch #2.
Hi Stephen, Sorry for the delay with my response. On Mon, May 02, 2022 at 01:41:33PM -0700, Stephen Boyd wrote: > Quoting Dmitry Torokhov (2022-05-02 10:43:06) > > On Mon, May 2, 2022 at 10:00 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > That goes against the recently landed commit 4352e23a7ff2 ("Input: > > > cros-ec-keyb - only register keyboard if rows/columns exist") but > > > perhaps we should just _undo_ that that since it landed pretty > > > recently and say that the truly supported way to specify that you only > > > have keyboards/switches is with the compatible. > > > > > > What do you think? > > > > I am sorry, I am still confused on what exactly we are trying to solve > > here? Having a device with the new device tree somehow run an older > > kernel and fail? Why exactly do we care about this case? > > Yes, we're trying to solve the problem where a new device tree is used > with an older kernel because it doesn't have the driver patch to only > create an input device for the matrix when rows/columns properties are > present. Otherwise applying that devicetree patch to an older kernel > will break bisection. Well, my recommendation here would be: "do not do that". How exactly will you get new DTS into a device with older kernel, and why would you do that? > > > We have > > implemented the notion that without rows/columns properties we will > > not be creating input device for the matrix portion, all older devices > > should have it defined, so the newer driver is compatible with them... > > > > Agreed, that solves half the problem. This new compatible eases > integration so that devicetrees can say they're compatible with the old > binding that _requires_ the rows/column properties. By making the driver > change we loosened that requirement, but the binding should have been > making the properties required at the start because it fails to bind > otherwise. > > My interpretation of what Doug is saying is that we should maintain that > requirement that rows/columns exists if the original compatible > google,cros-ec-keyb is present and use the new compatible to indicate > that there are switches. Combining the two compatibles means there's > switches and a matrix keyboard, having only the switches compatible > means only switches, and having only the keyboard compatible means only > matrix keyboard. > > It sounds OK to me. Have we solved module loading in the presence of multiple compatibles? IIRC we only ever try to load module on the first compatible, so you'd be breaking autoloading cros-ec-keyb on these older kernels. I think the cure that is being proposed is worse than the disease. Thanks.
Quoting Dmitry Torokhov (2022-05-12 03:22:30) > Hi Stephen, > > Sorry for the delay with my response. > > On Mon, May 02, 2022 at 01:41:33PM -0700, Stephen Boyd wrote: > > Quoting Dmitry Torokhov (2022-05-02 10:43:06) > > > On Mon, May 2, 2022 at 10:00 AM Doug Anderson <dianders@chromium.org> wrote: > > > > > > > > That goes against the recently landed commit 4352e23a7ff2 ("Input: > > > > cros-ec-keyb - only register keyboard if rows/columns exist") but > > > > perhaps we should just _undo_ that that since it landed pretty > > > > recently and say that the truly supported way to specify that you only > > > > have keyboards/switches is with the compatible. > > > > > > > > What do you think? > > > > > > I am sorry, I am still confused on what exactly we are trying to solve > > > here? Having a device with the new device tree somehow run an older > > > kernel and fail? Why exactly do we care about this case? > > > > Yes, we're trying to solve the problem where a new device tree is used > > with an older kernel because it doesn't have the driver patch to only > > create an input device for the matrix when rows/columns properties are > > present. Otherwise applying that devicetree patch to an older kernel > > will break bisection. > > Well, my recommendation here would be: "do not do that". How exactly > will you get new DTS into a device with older kernel, and why would you > do that? It's about easing the transition to a new programming model of the driver. We could "not do that" and consciously decide to only use new DTBs with new kernels. Or we could take this multiple compatible approach and things work with all combinations. I'd like to make transitions smooth so introducing a second compatible string is the solution for that. Another "what if" scenario is that the rows/columns properties should have been required per the DT binding all along. If they were required to begin with, I wouldn't have been able to make them optional without introducing a new compatible string that the schema keyed off of to figure out that they're optional sometimes. > > > > > > > We have > > > implemented the notion that without rows/columns properties we will > > > not be creating input device for the matrix portion, all older devices > > > should have it defined, so the newer driver is compatible with them... > > > > > > > Agreed, that solves half the problem. This new compatible eases > > integration so that devicetrees can say they're compatible with the old > > binding that _requires_ the rows/column properties. By making the driver > > change we loosened that requirement, but the binding should have been > > making the properties required at the start because it fails to bind > > otherwise. > > > > My interpretation of what Doug is saying is that we should maintain that > > requirement that rows/columns exists if the original compatible > > google,cros-ec-keyb is present and use the new compatible to indicate > > that there are switches. Combining the two compatibles means there's > > switches and a matrix keyboard, having only the switches compatible > > means only switches, and having only the keyboard compatible means only > > matrix keyboard. > > > > It sounds OK to me. > > Have we solved module loading in the presence of multiple compatibles? > IIRC we only ever try to load module on the first compatible, so you'd > be breaking autoloading cros-ec-keyb on these older kernels. I think the > cure that is being proposed is worse than the disease. > The first compatible is still cros-ec-keyb in the driver though? Or you mean the first compatible in the node? I'm not aware of this problem at all but I can certainly test out a fake node and module and see if it gets autoloaded.
Quoting Stephen Boyd (2022-05-12 11:58:02) > Quoting Dmitry Torokhov (2022-05-12 03:22:30) > > > > Have we solved module loading in the presence of multiple compatibles? > > IIRC we only ever try to load module on the first compatible, so you'd > > be breaking autoloading cros-ec-keyb on these older kernels. I think the > > cure that is being proposed is worse than the disease. > > > > The first compatible is still cros-ec-keyb in the driver though? Or you > mean the first compatible in the node? I'm not aware of this problem at > all but I can certainly test out a fake node and module and see if it > gets autoloaded. I can't get this test module to fail to load no matter what I do. I commented out the second match table entry, and kept it there and removed 'vendor,switch-compat' from the DTS. Module still autoloads. ----8<---- diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi index 42a87fa4976e..a6173b79ba67 100644 --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi @@ -54,6 +54,10 @@ aliases { spi11 = &spi11; }; + mynode { + compatible = "vendor,switch-compat", "vendor,keyb-compat"; + }; + clocks { xo_board: xo-board { compatible = "fixed-clock"; diff --git a/lib/Makefile b/lib/Makefile index a841be5244ac..0a784011feb5 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -74,6 +74,7 @@ UBSAN_SANITIZE_test_ubsan.o := y obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o obj-$(CONFIG_TEST_LIST_SORT) += test_list_sort.o obj-$(CONFIG_TEST_MIN_HEAP) += test_min_heap.o +obj-m += dtmod.o obj-$(CONFIG_TEST_LKM) += test_module.o obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o diff --git a/lib/dtmod.c b/lib/dtmod.c new file mode 100644 index 000000000000..c34ae37b8ff0 --- /dev/null +++ b/lib/dtmod.c @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: GPL-2.0-only + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/init.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/platform_device.h> +#include <linux/printk.h> + +static int test_probe(struct platform_device *pdev) +{ + dev_info(&pdev->dev, "I got probed\n"); + + return 0; +} + +static int test_remove(struct platform_device *pdev) +{ + dev_info(&pdev->dev, "I got removed\n"); + + return 0; +} + +static const struct of_device_id test_of_match[] = { + { .compatible = "vendor,keyb-compat" }, + { .compatible = "vendor,switch-compat" }, // comment out + {} +}; +MODULE_DEVICE_TABLE(of, test_of_match); + +static struct platform_driver test_keyb_driver = { + .probe = test_probe, + .remove = test_remove, + .driver = { + .name = "test-ec-keyb", + .of_match_table = test_of_match, + }, +}; + +module_platform_driver(test_keyb_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:test-ec-keyb");
On Thu, May 12, 2022 at 01:11:39PM -0700, Stephen Boyd wrote: > Quoting Stephen Boyd (2022-05-12 11:58:02) > > Quoting Dmitry Torokhov (2022-05-12 03:22:30) > > > > > > Have we solved module loading in the presence of multiple compatibles? > > > IIRC we only ever try to load module on the first compatible, so you'd > > > be breaking autoloading cros-ec-keyb on these older kernels. I think the > > > cure that is being proposed is worse than the disease. > > > > > > > The first compatible is still cros-ec-keyb in the driver though? Or you > > mean the first compatible in the node? I'm not aware of this problem at > > all but I can certainly test out a fake node and module and see if it > > gets autoloaded. > > I can't get this test module to fail to load no matter what I do. I > commented out the second match table entry, and kept it there and > removed 'vendor,switch-compat' from the DTS. Module still autoloads. > Ah, indeed, if the module contains both compatibles we will load it. It is broken when we have 2 or more modules and DT lists several compatibles for a device. OK, it looks like you feel very strongly regarding having a dedicated compatible. In this case please make sure that the compatible's behavior is properly documented (i.e. google,cros-ec-keyb compatible does not imply that there are *NO* switches, and users having buttons and switches in addition to matrix keys can also use google,cros-ec-keyb as a compatible for their device). We also need to mention that with the 2nd compatible the device still can report key/button events, it is simply that there is no matrix component. Should we call the other compatible google,cros-ec-bs? We should also abort binding the device if it specifies the new compatible, but EC does not report any buttons or switches. Thanks.
Quoting Dmitry Torokhov (2022-05-12 16:31:08) > On Thu, May 12, 2022 at 01:11:39PM -0700, Stephen Boyd wrote: > > Quoting Stephen Boyd (2022-05-12 11:58:02) > > > Quoting Dmitry Torokhov (2022-05-12 03:22:30) > > > > > > > > Have we solved module loading in the presence of multiple compatibles? > > > > IIRC we only ever try to load module on the first compatible, so you'd > > > > be breaking autoloading cros-ec-keyb on these older kernels. I think the > > > > cure that is being proposed is worse than the disease. > > > > > > > > > > The first compatible is still cros-ec-keyb in the driver though? Or you > > > mean the first compatible in the node? I'm not aware of this problem at > > > all but I can certainly test out a fake node and module and see if it > > > gets autoloaded. > > > > I can't get this test module to fail to load no matter what I do. I > > commented out the second match table entry, and kept it there and > > removed 'vendor,switch-compat' from the DTS. Module still autoloads. > > > > Ah, indeed, if the module contains both compatibles we will load it. It > is broken when we have 2 or more modules and DT lists several > compatibles for a device. > > OK, it looks like you feel very strongly regarding having a dedicated > compatible. In this case please make sure that the compatible's behavior > is properly documented (i.e. google,cros-ec-keyb compatible does not > imply that there are *NO* switches, and users having buttons and > switches in addition to matrix keys can also use google,cros-ec-keyb as > a compatible for their device). We also need to mention that with the > 2nd compatible the device still can report key/button events, it is > simply that there is no matrix component. Should we call the other > compatible google,cros-ec-bs? ;) I think I covered that in v3 of this series[1]. > > We should also abort binding the device if it specifies the new > compatible, but EC does not report any buttons or switches. Sure. I don't have that done in v3 so I can respin the patch series to fail probe if there aren't any switches and the cros-ec-keyb-switches compatible is present. Can you take a quick glance at v3 and let me know if anything else is needed? [1] https://lore.kernel.org/all/20220503042242.3597561-1-swboyd@chromium.org/
Quoting Stephen Boyd (2022-05-12 16:46:22) > > I think I covered that in v3 of this series[1]. Even better, see v4 https://lore.kernel.org/all/20220503204212.3907925-1-swboyd@chromium.org/
On Thu, May 12, 2022 at 04:53:13PM -0700, Stephen Boyd wrote: > Quoting Stephen Boyd (2022-05-12 16:46:22) > > > > I think I covered that in v3 of this series[1]. > > Even better, see v4 > > https://lore.kernel.org/all/20220503204212.3907925-1-swboyd@chromium.org/ I guess I was looking for the explicit verbage for when a particular compatible has or can be used, and I do not believe I see it in either the 3rd or the 4th version posted. I see some comments in the example section, but I do not think it is enough. Thanks.
diff --git a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml index e8f137abb03c..c1b079449cf3 100644 --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml @@ -15,14 +15,19 @@ description: | Google's ChromeOS EC Keyboard is a simple matrix keyboard implemented on a separate EC (Embedded Controller) device. It provides a message for reading key scans from the EC. These are then converted - into keycodes for processing by the kernel. - -allOf: - - $ref: "/schemas/input/matrix-keymap.yaml#" + into keycodes for processing by the kernel. This device also supports + switches/buttons like power and volume buttons. properties: compatible: - const: google,cros-ec-keyb + oneOf: + - items: + - const: google,cros-ec-keyb-switches + - items: + - const: google,cros-ec-keyb-switches + - const: google,cros-ec-keyb + - items: + - const: google,cros-ec-keyb google,needs-ghost-filter: description: @@ -41,15 +46,40 @@ properties: where the lower 16 bits are reserved. This property is specified only when the keyboard has a custom design for the top row keys. +dependencies: + function-row-phsymap: [ 'linux,keymap' ] + google,needs-ghost-filter: [ 'linux,keymap' ] + required: - compatible +allOf: + - if: + properties: + compatible: + contains: + const: google,cros-ec-keyb + then: + allOf: + - $ref: "/schemas/input/matrix-keymap.yaml#" + - if: + not: + properties: + compatible: + contains: + const: google,cros-ec-keyb-switches + then: + required: + - keypad,num-rows + - keypad,num-columns + - linux,keymap + unevaluatedProperties: false examples: - | #include <dt-bindings/input/input.h> - cros-ec-keyb { + keyboard-controller { compatible = "google,cros-ec-keyb"; keypad,num-rows = <8>; keypad,num-columns = <13>; @@ -113,3 +143,56 @@ examples: /* UP LEFT */ 0x070b0067 0x070c0069>; }; + + - | + keyboard-controller { + compatible = "google,cros-ec-keyb-switches", "google,cros-ec-keyb"; + /* Matrix keymap properties are allowed but ignored */ + keypad,num-rows = <8>; + keypad,num-columns = <13>; + linux,keymap = < + /* CAPSLCK F1 B F10 */ + 0x0001003a 0x0002003b 0x00030030 0x00040044 + /* N = R_ALT ESC */ + 0x00060031 0x0008000d 0x000a0064 0x01010001 + /* F4 G F7 H */ + 0x0102003e 0x01030022 0x01040041 0x01060023 + /* ' F9 BKSPACE L_CTRL */ + 0x01080028 0x01090043 0x010b000e 0x0200001d + /* TAB F3 T F6 */ + 0x0201000f 0x0202003d 0x02030014 0x02040040 + /* ] Y 102ND [ */ + 0x0205001b 0x02060015 0x02070056 0x0208001a + /* F8 GRAVE F2 5 */ + 0x02090042 0x03010029 0x0302003c 0x03030006 + /* F5 6 - \ */ + 0x0304003f 0x03060007 0x0308000c 0x030b002b + /* R_CTRL A D F */ + 0x04000061 0x0401001e 0x04020020 0x04030021 + /* S K J ; */ + 0x0404001f 0x04050025 0x04060024 0x04080027 + /* L ENTER Z C */ + 0x04090026 0x040b001c 0x0501002c 0x0502002e + /* V X , M */ + 0x0503002f 0x0504002d 0x05050033 0x05060032 + /* L_SHIFT / . SPACE */ + 0x0507002a 0x05080035 0x05090034 0x050B0039 + /* 1 3 4 2 */ + 0x06010002 0x06020004 0x06030005 0x06040003 + /* 8 7 0 9 */ + 0x06050009 0x06060008 0x0608000b 0x0609000a + /* L_ALT DOWN RIGHT Q */ + 0x060a0038 0x060b006c 0x060c006a 0x07010010 + /* E R W I */ + 0x07020012 0x07030013 0x07040011 0x07050017 + /* U R_SHIFT P O */ + 0x07060016 0x07070036 0x07080019 0x07090018 + /* UP LEFT */ + 0x070b0067 0x070c0069>; + }; + - | + /* No matrix keyboard, just buttons/switches */ + switches { + compatible = "google,cros-ec-keyb-switches"; + }; +...
If the device is a detachable, this device won't have a matrix keyboard but it may have some button switches, e.g. volume buttons and power buttons. Let's add a more specific compatible for this type of device that indicates to the OS that there are only switches and no matrix keyboard present. If only the switches compatible is present, then the matrix keyboard properties are denied. This lets us gracefully migrate devices that only have switches over to the new compatible string. Similarly, start enforcing that the keypad rows/cols and keymap properties exist if the google,cros-ec-keyb compatible is present. This more clearly describes what the driver is expecting, i.e. that the kernel driver will fail to probe if the row or column or keymap properties are missing and only the google,cros-ec-keyb compatible is present. 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> Signed-off-by: Stephen Boyd <swboyd@chromium.org> --- .../bindings/input/google,cros-ec-keyb.yaml | 95 +++++++++++++++++-- 1 file changed, 89 insertions(+), 6 deletions(-)