| Message ID | 20220503042242.3597561-2-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: cros-ec-keyb: Better matrixless support | expand |
Hi, On Mon, May 2, 2022 at 9:22 PM Stephen Boyd <swboyd@chromium.org> wrote: > > @@ -113,3 +135,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 */ nit: s/allowed/required/ This now looks reasonable and well explained to me. Reviewed-by: Douglas Anderson <dianders@chromium.org>
On 03/05/2022 06:22, Stephen Boyd wrote: > If the ChromeOS board is a detachable, this cros-ec-keyb device won't > have a matrix keyboard but it may have some button switches, e.g. volume > buttons and power buttons. The driver still registers a keyboard though > and that leads to userspace confusion around where the keyboard is. (...) > > +if: > + properties: > + compatible: > + contains: > + const: google,cros-ec-keyb > +then: > + allOf: > + - $ref: "/schemas/input/matrix-keymap.yaml#" > + required: > + - keypad,num-rows > + - keypad,num-columns > + - linux,keymap else: properties: function-row-phsymap: false google,needs-ghost-filter: false Because these are not valid for the non-matrix-keyboard case, right? Best regards, Krzysztof
Hi, On Tue, May 3, 2022 at 8:42 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 03/05/2022 06:22, Stephen Boyd wrote: > > If the ChromeOS board is a detachable, this cros-ec-keyb device won't > > have a matrix keyboard but it may have some button switches, e.g. volume > > buttons and power buttons. The driver still registers a keyboard though > > and that leads to userspace confusion around where the keyboard is. > > (...) > > > > > +if: > > + properties: > > + compatible: > > + contains: > > + const: google,cros-ec-keyb > > +then: > > + allOf: > > + - $ref: "/schemas/input/matrix-keymap.yaml#" > > + required: > > + - keypad,num-rows > > + - keypad,num-columns > > + - linux,keymap > > else: > properties: > function-row-phsymap: false > google,needs-ghost-filter: false > > Because these are not valid for the non-matrix-keyboard case, right? Isn't that implicit because this file has `unevaluatedProperties: false` ? -Doug
On 03/05/2022 17:46, Doug Anderson wrote: > Hi, > > On Tue, May 3, 2022 at 8:42 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 03/05/2022 06:22, Stephen Boyd wrote: >>> If the ChromeOS board is a detachable, this cros-ec-keyb device won't >>> have a matrix keyboard but it may have some button switches, e.g. volume >>> buttons and power buttons. The driver still registers a keyboard though >>> and that leads to userspace confusion around where the keyboard is. >> >> (...) >> >>> >>> +if: >>> + properties: >>> + compatible: >>> + contains: >>> + const: google,cros-ec-keyb >>> +then: >>> + allOf: >>> + - $ref: "/schemas/input/matrix-keymap.yaml#" >>> + required: >>> + - keypad,num-rows >>> + - keypad,num-columns >>> + - linux,keymap >> >> else: >> properties: >> function-row-phsymap: false >> google,needs-ghost-filter: false >> >> Because these are not valid for the non-matrix-keyboard case, right? > > Isn't that implicit because this file has `unevaluatedProperties: false` ? But they are evaluated here, aren't they? Best regards, Krzysztof
Hi, On Tue, May 3, 2022 at 8:54 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 03/05/2022 17:46, Doug Anderson wrote: > > Hi, > > > > On Tue, May 3, 2022 at 8:42 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 03/05/2022 06:22, Stephen Boyd wrote: > >>> If the ChromeOS board is a detachable, this cros-ec-keyb device won't > >>> have a matrix keyboard but it may have some button switches, e.g. volume > >>> buttons and power buttons. The driver still registers a keyboard though > >>> and that leads to userspace confusion around where the keyboard is. > >> > >> (...) > >> > >>> > >>> +if: > >>> + properties: > >>> + compatible: > >>> + contains: > >>> + const: google,cros-ec-keyb > >>> +then: > >>> + allOf: > >>> + - $ref: "/schemas/input/matrix-keymap.yaml#" > >>> + required: > >>> + - keypad,num-rows > >>> + - keypad,num-columns > >>> + - linux,keymap > >> > >> else: > >> properties: > >> function-row-phsymap: false > >> google,needs-ghost-filter: false > >> > >> Because these are not valid for the non-matrix-keyboard case, right? > > > > Isn't that implicit because this file has `unevaluatedProperties: false` ? > > But they are evaluated here, aren't they? Only if the "if" test, though? ...ah, or is this a difference between "unevaluatedProperties" and "additionalProperties" ? -Doug
On 03/05/2022 18:14, Doug Anderson wrote: > Hi, > > On Tue, May 3, 2022 at 8:54 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On 03/05/2022 17:46, Doug Anderson wrote: >>> Hi, >>> >>> On Tue, May 3, 2022 at 8:42 AM Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>> On 03/05/2022 06:22, Stephen Boyd wrote: >>>>> If the ChromeOS board is a detachable, this cros-ec-keyb device won't >>>>> have a matrix keyboard but it may have some button switches, e.g. volume >>>>> buttons and power buttons. The driver still registers a keyboard though >>>>> and that leads to userspace confusion around where the keyboard is. >>>> >>>> (...) >>>> >>>>> >>>>> +if: >>>>> + properties: >>>>> + compatible: >>>>> + contains: >>>>> + const: google,cros-ec-keyb >>>>> +then: >>>>> + allOf: >>>>> + - $ref: "/schemas/input/matrix-keymap.yaml#" >>>>> + required: >>>>> + - keypad,num-rows >>>>> + - keypad,num-columns >>>>> + - linux,keymap >>>> >>>> else: >>>> properties: >>>> function-row-phsymap: false >>>> google,needs-ghost-filter: false >>>> >>>> Because these are not valid for the non-matrix-keyboard case, right? >>> >>> Isn't that implicit because this file has `unevaluatedProperties: false` ? >> >> But they are evaluated here, aren't they? > > Only if the "if" test, though? No, they are evaluated always. They are listed directly in properties, not in "if", therefore they will be considered as always evaluated. > ...ah, or is this a difference between > "unevaluatedProperties" and "additionalProperties" ? Best regards, Krzysztof
Quoting Krzysztof Kozlowski (2022-05-03 08:42:39) > On 03/05/2022 06:22, Stephen Boyd wrote: > > If the ChromeOS board is a detachable, this cros-ec-keyb device won't > > have a matrix keyboard but it may have some button switches, e.g. volume > > buttons and power buttons. The driver still registers a keyboard though > > and that leads to userspace confusion around where the keyboard is. > > (...) > > > > > +if: > > + properties: > > + compatible: > > + contains: > > + const: google,cros-ec-keyb > > +then: > > + allOf: > > + - $ref: "/schemas/input/matrix-keymap.yaml#" > > + required: > > + - keypad,num-rows > > + - keypad,num-columns > > + - linux,keymap > > else: > properties: > function-row-phsymap: false > google,needs-ghost-filter: false > > Because these are not valid for the non-matrix-keyboard case, right? > Yes, but they also depend on linux,keymap so they're not possible to set because the matrix-keymap.yaml schema defines linux,keymap and that is only included if google,cros-ec-keyb is present.
On 03/05/2022 21:00, Stephen Boyd wrote: >> else: >> properties: >> function-row-phsymap: false >> google,needs-ghost-filter: false >> >> Because these are not valid for the non-matrix-keyboard case, right? >> > > Yes, but they also depend on linux,keymap so they're not possible to > set because the matrix-keymap.yaml schema defines linux,keymap and that > is only included if google,cros-ec-keyb is present. Ah, right, I missed that one. Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Quoting Doug Anderson (2022-05-03 08:09:04) > Hi, > > On Mon, May 2, 2022 at 9:22 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > @@ -113,3 +135,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 */ > > nit: s/allowed/required/ Ok. I will roll it in and resend. > > This now looks reasonable and well explained to me. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> 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..6f4e0242ad5a 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,32 @@ 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 +if: + properties: + compatible: + contains: + const: google,cros-ec-keyb +then: + allOf: + - $ref: "/schemas/input/matrix-keymap.yaml#" + 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 +135,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 */ + keyboard-controller { + compatible = "google,cros-ec-keyb-switches"; + }; +...
If the ChromeOS board is a detachable, this cros-ec-keyb device won't have a matrix keyboard but it may have some button switches, e.g. volume buttons and power buttons. The driver still registers a keyboard though and that leads to userspace confusion around where the keyboard is. We tried to work around this in commit 4352e23a7ff2 ("Input: cros-ec-keyb - only register keyboard if rows/columns exist") but that led to another problem where removing the rows/columns properties breaks the binding[1]. Technically before that commit the rows/columns properties were required, otherwise the driver would fail to probe. Removing the properties from devicetrees makes the driver fail to probe unless the corresponding driver patch is present. Furthermore, this makes requiring matrix keyboard properties for devices that really have a keyboard impossible because the compatible drives the schema and now the properties are optional. 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. Otherwise, as long as the "google,cros-ec-keyb" copmatible is present, the matrix keyboard properties are required. This more clearly describes what the driver was expecting, i.e. that the kernel driver would fail to probe if the linux,{rows,cols,keymap} properties are missing and the "google,cros-ec-keyb" compatible is present. If the new "google,cros-ec-keyb-switches" compatible is present then the matrix keyboard properties are likely to be ignored by any modern driver, but they're still allowed per the binding if "google,cros-ec-keyb" is present to conform to the original binding and to keep the driver probing when used with newer DTBs. This lets us gracefully migrate devices that only have switches over to the new compatible string and properly enforce the "google,cros-ec-keyb" binding at the same time. Eventually we'll be able to use only "google,cros-ec-keyb-switches" or "google,cros-ec-keyb" when the driver support has baked for a release or two, but we'll always support the combined compatible list so that DTBs can be mixed and matched with kernels, no dependency between the two binaries is required. Link: https://lore.kernel.org/r/CAD=FV=Wey2P_=3Lp6M8GEaoyCn1XcYFhfJwfx43a5f_8H0obwg@mail.gmail.com [1] Cc: Krzysztof Kozlowski <krzk+dt@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 | 87 +++++++++++++++++-- 1 file changed, 81 insertions(+), 6 deletions(-)