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

[PATCH 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.

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          | 12 +++++++++---
 drivers/input/keyboard/cros_ec_keyb.c                |  8 ++++++++
 2 files changed, 17 insertions(+), 3 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 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.

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          | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 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..edc1194d558d 100644
--- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
+++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
@@ -15,14 +15,20 @@ 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.
+  into keycodes for processing by the kernel. This device also supports
+  switches/buttons like power and volume buttons.
 
 allOf:
   - $ref: "/schemas/input/matrix-keymap.yaml#"
 
 properties:
   compatible:
-    const: google,cros-ec-keyb
+    oneOf:
+      - items:
+          - const: google,cros-ec-keyb-switches
+          - const: google,cros-ec-keyb
+      - items:
+          - const: google,cros-ec-keyb
 
   google,needs-ghost-filter:
     description:
@@ -50,7 +56,7 @@ examples:
   - |
     #include <dt-bindings/input/input.h>
     cros-ec-keyb {
-        compatible = "google,cros-ec-keyb";
+        compatible = "google,cros-ec-keyb-switches", "google,cros-ec-keyb";
         keypad,num-rows = <8>;
         keypad,num-columns = <13>;
         google,needs-ghost-filter;
-- 
https://chromeos.dev

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

[Krzysztof Kozlowski]

On 27/04/2022 22:30, Stephen Boyd 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.
> 
> 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          | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 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..edc1194d558d 100644
> --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> @@ -15,14 +15,20 @@ 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.
> +  into keycodes for processing by the kernel. This device also supports
> +  switches/buttons like power and volume buttons.
>  
>  allOf:
>    - $ref: "/schemas/input/matrix-keymap.yaml#"
>  
>  properties:
>    compatible:
> -    const: google,cros-ec-keyb
> +    oneOf:
> +      - items:
> +          - const: google,cros-ec-keyb-switches
> +          - const: google,cros-ec-keyb
> +      - items:
> +          - const: google,cros-ec-keyb
>  

In such case matrix-keymap properties are not valid, right? The
matrix-keymap should not be referenced, IOW, you need to move allOf
below "required" and add:
if:not:...then: $ref: "/schemas/input/matrix-keymap.yaml

Best regards,
Krzysztof

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

[Stephen Boyd]

Quoting Krzysztof Kozlowski (2022-04-27 23:12:47)
> On 27/04/2022 22:30, Stephen Boyd 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.
> >
> > 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          | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 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..edc1194d558d 100644
> > --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> > +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> > @@ -15,14 +15,20 @@ 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.
> > +  into keycodes for processing by the kernel. This device also supports
> > +  switches/buttons like power and volume buttons.
> >
> >  allOf:
> >    - $ref: "/schemas/input/matrix-keymap.yaml#"
> >
> >  properties:
> >    compatible:
> > -    const: google,cros-ec-keyb
> > +    oneOf:
> > +      - items:
> > +          - const: google,cros-ec-keyb-switches
> > +          - const: google,cros-ec-keyb
> > +      - items:
> > +          - const: google,cros-ec-keyb
> >
>
> In such case matrix-keymap properties are not valid, right? The
> matrix-keymap should not be referenced, IOW, you need to move allOf
> below "required" and add:
> if:not:...then: $ref: "/schemas/input/matrix-keymap.yaml
>

Eventually that sounds doable, but for the time being I want to merely
add this new compatible in front of the original compatible so that
updated DTBs still work with older kernels, i.e. the switches still get
registered because the driver works with the original
google,cros-ec-keyb compatible. Given that none of the properties are
required for google,cros-ec-keyb it didn't seem necessary to make having
the google,cros-ec-keyb-switches compatible deny the existence of the
matrix-keymap properties.

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

[Krzysztof Kozlowski]

On 28/04/2022 08:24, Stephen Boyd wrote:
> Quoting Krzysztof Kozlowski (2022-04-27 23:12:47)
>> On 27/04/2022 22:30, Stephen Boyd 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.
>>>
>>> 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          | 12 +++++++++---
>>>  1 file changed, 9 insertions(+), 3 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..edc1194d558d 100644
>>> --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
>>> +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
>>> @@ -15,14 +15,20 @@ 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.
>>> +  into keycodes for processing by the kernel. This device also supports
>>> +  switches/buttons like power and volume buttons.
>>>
>>>  allOf:
>>>    - $ref: "/schemas/input/matrix-keymap.yaml#"
>>>
>>>  properties:
>>>    compatible:
>>> -    const: google,cros-ec-keyb
>>> +    oneOf:
>>> +      - items:
>>> +          - const: google,cros-ec-keyb-switches
>>> +          - const: google,cros-ec-keyb
>>> +      - items:
>>> +          - const: google,cros-ec-keyb
>>>
>>
>> In such case matrix-keymap properties are not valid, right? The
>> matrix-keymap should not be referenced, IOW, you need to move allOf
>> below "required" and add:
>> if:not:...then: $ref: "/schemas/input/matrix-keymap.yaml
>>
> 
> Eventually that sounds doable, but for the time being I want to merely
> add this new compatible in front of the original compatible so that
> updated DTBs still work with older kernels, i.e. the switches still get
> registered because the driver works with the original
> google,cros-ec-keyb compatible. 

The bindings here do not invalidate (break) existing DTBs. Old DTBs can
work in old way, we talk only about binding.

> Given that none of the properties are
> required for google,cros-ec-keyb it didn't seem necessary to make having
> the google,cros-ec-keyb-switches compatible deny the existence of the
> matrix-keymap properties.

Maybe I misunderstood the commit msg. Are the
"google,cros-ec-keyb-switches" devices coming with matrix keyboard or
not? I mean physically.

Best regards,
Krzysztof

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

[Stephen Boyd]

Quoting Krzysztof Kozlowski (2022-04-28 00:27:52)
> On 28/04/2022 08:24, Stephen Boyd wrote:
> > Quoting Krzysztof Kozlowski (2022-04-27 23:12:47)
> >> On 27/04/2022 22:30, Stephen Boyd 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.
> >>>
> >>> 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          | 12 +++++++++---
> >>>  1 file changed, 9 insertions(+), 3 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..edc1194d558d 100644
> >>> --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> >>> +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> >>> @@ -15,14 +15,20 @@ 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.
> >>> +  into keycodes for processing by the kernel. This device also supports
> >>> +  switches/buttons like power and volume buttons.
> >>>
> >>>  allOf:
> >>>    - $ref: "/schemas/input/matrix-keymap.yaml#"
> >>>
> >>>  properties:
> >>>    compatible:
> >>> -    const: google,cros-ec-keyb
> >>> +    oneOf:
> >>> +      - items:
> >>> +          - const: google,cros-ec-keyb-switches
> >>> +          - const: google,cros-ec-keyb
> >>> +      - items:
> >>> +          - const: google,cros-ec-keyb
> >>>
> >>
> >> In such case matrix-keymap properties are not valid, right? The
> >> matrix-keymap should not be referenced, IOW, you need to move allOf
> >> below "required" and add:
> >> if:not:...then: $ref: "/schemas/input/matrix-keymap.yaml
> >>
> >
> > Eventually that sounds doable, but for the time being I want to merely
> > add this new compatible in front of the original compatible so that
> > updated DTBs still work with older kernels, i.e. the switches still get
> > registered because the driver works with the original
> > google,cros-ec-keyb compatible.
>
> The bindings here do not invalidate (break) existing DTBs. Old DTBs can
> work in old way, we talk only about binding.

Ok, got it.

>
> > Given that none of the properties are
> > required for google,cros-ec-keyb it didn't seem necessary to make having
> > the google,cros-ec-keyb-switches compatible deny the existence of the
> > matrix-keymap properties.
>
> Maybe I misunderstood the commit msg. Are the
> "google,cros-ec-keyb-switches" devices coming with matrix keyboard or
> not? I mean physically.
>

The answer is "sometimes, physically". Sometimes there are switches like
volume buttons and power buttons and also a matrix keyboard (convertible
and clamshells). Other times there are volume buttons and power buttons
and no matrix keyboard (detachable). This device node represents both
the keyboard and the switches.

Unfortunately the EC firmware on older Chromebooks that don't have a
matrix keyboard still report that they have some number of columns and
rows. I was hoping to make this fully dynamic by querying the EC but
that isn't possible.

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

[Krzysztof Kozlowski]

On 28/04/2022 18:01, Stephen Boyd wrote:
>>
>>> Given that none of the properties are
>>> required for google,cros-ec-keyb it didn't seem necessary to make having
>>> the google,cros-ec-keyb-switches compatible deny the existence of the
>>> matrix-keymap properties.
>>
>> Maybe I misunderstood the commit msg. Are the
>> "google,cros-ec-keyb-switches" devices coming with matrix keyboard or
>> not? I mean physically.
>>
> 
> The answer is "sometimes, physically". Sometimes there are switches like
> volume buttons and power buttons and also a matrix keyboard (convertible
> and clamshells). Other times there are volume buttons and power buttons
> and no matrix keyboard (detachable). This device node represents both
> the keyboard and the switches.
> 
> Unfortunately the EC firmware on older Chromebooks that don't have a
> matrix keyboard still report that they have some number of columns and
> rows. I was hoping to make this fully dynamic by querying the EC but
> that isn't possible.

OK, then it's indeed slightly different case. Let's skip my comment.


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 27/04/2022 22:30, Stephen Boyd 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.
> 
> 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>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

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

[Doug Anderson]

Hi,

On Wed, Apr 27, 2022 at 1:30 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.
>
> 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          | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 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..edc1194d558d 100644
> --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> @@ -15,14 +15,20 @@ 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.
> +  into keycodes for processing by the kernel. This device also supports
> +  switches/buttons like power and volume buttons.
>
>  allOf:
>    - $ref: "/schemas/input/matrix-keymap.yaml#"
>
>  properties:
>    compatible:
> -    const: google,cros-ec-keyb
> +    oneOf:
> +      - items:
> +          - const: google,cros-ec-keyb-switches
> +          - const: google,cros-ec-keyb
> +      - items:
> +          - const: google,cros-ec-keyb

nit: if I come back and read this binding later I'm not sure it would
be obvious which compatible I should pick. Can we give any description
here that indicates that the first choice is for devices that _only_
have buttons and switches (the google,cros-ec-keyb is just for
backward compatibility) and the second choice is for devices that have
a physical keyboard and _also_ possibly some buttons/switches?

I could also imagine people in the future being confused about whether
it's allowed to specify matrix properties even for devices that don't
have a matrix keyboard. It might be worth noting that it's allowed (to
support old drivers that might still be matching against the
google,cros-ec-keyb compatible) but not required.


>    google,needs-ghost-filter:
>      description:
> @@ -50,7 +56,7 @@ examples:
>    - |
>      #include <dt-bindings/input/input.h>
>      cros-ec-keyb {
> -        compatible = "google,cros-ec-keyb";
> +        compatible = "google,cros-ec-keyb-switches", "google,cros-ec-keyb";

Feels like we should create a second example?

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

[Krzysztof Kozlowski]

On 29/04/2022 18:31, Doug Anderson wrote:
>>    - $ref: "/schemas/input/matrix-keymap.yaml#"
>>
>>  properties:
>>    compatible:
>> -    const: google,cros-ec-keyb
>> +    oneOf:
>> +      - items:
>> +          - const: google,cros-ec-keyb-switches
>> +          - const: google,cros-ec-keyb
>> +      - items:
>> +          - const: google,cros-ec-keyb
> 
> nit: if I come back and read this binding later I'm not sure it would
> be obvious which compatible I should pick. Can we give any description
> here that indicates that the first choice is for devices that _only_
> have buttons and switches (the google,cros-ec-keyb is just for
> backward compatibility) and the second choice is for devices that have
> a physical keyboard and _also_ possibly some buttons/switches?
> 
> I could also imagine people in the future being confused about whether
> it's allowed to specify matrix properties even for devices that don't
> have a matrix keyboard. It might be worth noting that it's allowed (to
> support old drivers that might still be matching against the
> google,cros-ec-keyb compatible) but not required.

+1

> 
> 
>>    google,needs-ghost-filter:
>>      description:
>> @@ -50,7 +56,7 @@ examples:
>>    - |
>>      #include <dt-bindings/input/input.h>
>>      cros-ec-keyb {
>> -        compatible = "google,cros-ec-keyb";
>> +        compatible = "google,cros-ec-keyb-switches", "google,cros-ec-keyb";
> 
> Feels like we should create a second example?

+1 as well, because it really would confuse what's the difference
between them.

Best regards,
Krzysztof

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

[Stephen Boyd]

Quoting Krzysztof Kozlowski (2022-04-29 09:35:58)
> On 29/04/2022 18:31, Doug Anderson wrote:
> >>    - $ref: "/schemas/input/matrix-keymap.yaml#"
> >>
> >>  properties:
> >>    compatible:
> >> -    const: google,cros-ec-keyb
> >> +    oneOf:
> >> +      - items:
> >> +          - const: google,cros-ec-keyb-switches
> >> +          - const: google,cros-ec-keyb
> >> +      - items:
> >> +          - const: google,cros-ec-keyb
> >
> > nit: if I come back and read this binding later I'm not sure it would
> > be obvious which compatible I should pick. Can we give any description
> > here that indicates that the first choice is for devices that _only_
> > have buttons and switches (the google,cros-ec-keyb is just for
> > backward compatibility) and the second choice is for devices that have
> > a physical keyboard and _also_ possibly some buttons/switches?

Sounds fair. I have to figure out how to add a description to the
choices. I guess a comment is the approach?

> >
> > I could also imagine people in the future being confused about whether
> > it's allowed to specify matrix properties even for devices that don't
> > have a matrix keyboard. It might be worth noting that it's allowed (to
> > support old drivers that might still be matching against the
> > google,cros-ec-keyb compatible) but not required.
>
> +1

Sure. I'll work that into the description for the first one with two
compatibles.

>
> >
> >
> >>    google,needs-ghost-filter:
> >>      description:
> >> @@ -50,7 +56,7 @@ examples:
> >>    - |
> >>      #include <dt-bindings/input/input.h>
> >>      cros-ec-keyb {
> >> -        compatible = "google,cros-ec-keyb";
> >> +        compatible = "google,cros-ec-keyb-switches", "google,cros-ec-keyb";
> >
> > Feels like we should create a second example?
>
> +1 as well, because it really would confuse what's the difference
> between them.

Ok.

[PATCH 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.

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

I treat the compatible as the deciding factor so that the
keypad,num-{rows,cols} properties didn't have to be removed. An
alternative would be for the driver to look for missing rows/cols if the
new compatible was present and otherwise treat them missing as an error.
That doesn't work though because an existing DTB could add the new
compatible and remove the rows/cols properties and then break an older
driver, i.e. we couldn't use a newer DTB on an older kernel. This seems
to be the best approach.

 drivers/input/keyboard/cros_ec_keyb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index eef909e52e23..a544be1d52d4 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -546,6 +546,14 @@ static int cros_ec_keyb_register_matrix(struct cros_ec_keyb *ckdev)
 	    !device_property_present(dev, "keypad,num-cols"))
 		return 0;
 
+	/*
+	 * Some devices only have switches but define keypad,num-{rows,cols} so
+	 * we add a more specific compatible in this situation indicating there
+	 * isn't a keyboard.
+	 */
+	if (of_device_is_compatible(dev->of_node, "google,cros-ec-keyb-switches"))
+		return 0;
+
 	err = matrix_keypad_parse_properties(dev, &ckdev->rows, &ckdev->cols);
 	if (err)
 		return err;
-- 
https://chromeos.dev