[PATCH v2 0/2] Input: cros-ec-keyb: Better matrixless support

[PATCH v2 0/2] Input: cros-ec-keyb: Better matrixless support

[Stephen Boyd]

This is a followup to my previous patch[1] that skips keyboard registration
when the matrix properties aren't present. This adds a compatible string
for this scenario so we can ease existing DTBs over to the new design.

Changes from v1 (https://lore.kernel.org/r/20220427203026.828183-1-swboyd@chromium.org):
 * Better enforcement of properties in DT binding
 * Skip registration by means of adding compatible to device id list

Stephen Boyd (2):
  dt-bindings: google,cros-ec-keyb: Introduce switches only compatible
  Input: cros-ec-keyb - skip keyboard registration for switches
    compatible

 .../bindings/input/google,cros-ec-keyb.yaml   | 95 +++++++++++++++++--
 drivers/input/keyboard/cros_ec_keyb.c         | 15 ++-
 2 files changed, 102 insertions(+), 8 deletions(-)

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>

[1] https://lore.kernel.org/all/20220425210726.3813477-1-swboyd@chromium.org/

base-commit: 4352e23a7ff2f8a4ff229dd1283ed2f2b708ec51
-- 
https://chromeos.dev

[PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Stephen Boyd]

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(-)

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";
+    };
+...
-- 
https://chromeos.dev

Re: [PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Doug Anderson]

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

Re: [PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Stephen Boyd]

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.

Re: [PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Stephen Boyd]

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.

Re: [PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Doug Anderson]

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.

Re: [PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Stephen Boyd]

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.

Re: [PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Stephen Boyd]

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");

Re: [PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Stephen Boyd]

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/

Re: [PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Stephen Boyd]

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/

Re: [PATCH v2 1/2] dt-bindings: google,cros-ec-keyb: Introduce switches only compatible

[Dmitry Torokhov]

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.

-- 
Dmitry

[PATCH v2 2/2] Input: cros-ec-keyb - skip keyboard registration for switches compatible

[Stephen Boyd]

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
@@ -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

Re: [PATCH v2 2/2] Input: cros-ec-keyb - skip keyboard registration for switches compatible

[Doug Anderson]

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

Re: [PATCH v2 2/2] Input: cros-ec-keyb - skip keyboard registration for switches compatible

[Stephen Boyd]

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.

Re: [PATCH v2 2/2] Input: cros-ec-keyb - skip keyboard registration for switches compatible

[Doug Anderson]

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

Re: [PATCH v2 2/2] Input: cros-ec-keyb - skip keyboard registration for switches compatible

[Stephen Boyd]

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.