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 <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: <[email protected]>
Cc: Benson Leung <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Hsin-Yi Wang <[email protected]>
Cc: "Joseph S. Barrera III" <[email protected]>
Signed-off-by: Stephen Boyd <[email protected]>
---
.../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
Quoting Dmitry Torokhov (2022-05-02 10:43:06)
> On Mon, May 2, 2022 at 10:00 AM Doug Anderson <[email protected]> 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.
Hi,
On Fri, Apr 29, 2022 at 4:31 PM Stephen Boyd <[email protected]> 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 <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: <[email protected]>
> Cc: Benson Leung <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Hsin-Yi Wang <[email protected]>
> Cc: "Joseph S. Barrera III" <[email protected]>
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> .../bindings/input/google,cros-ec-keyb.yaml | 95 +++++++++++++++++--
> 1 file changed, 89 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> index e8f137abb03c..c1b079449cf3 100644
> --- a/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> +++ b/Documentation/devicetree/bindings/input/google,cros-ec-keyb.yaml
> @@ -15,14 +15,19 @@ description: |
> Google's ChromeOS EC Keyboard is a simple matrix keyboard
> implemented on a separate EC (Embedded Controller) device. It provides
> a message for reading key scans from the EC. These are then converted
> - into keycodes for processing by the kernel.
> -
> -allOf:
> - - $ref: "/schemas/input/matrix-keymap.yaml#"
> + into keycodes for processing by the kernel. This device also supports
> + switches/buttons like power and volume buttons.
>
> properties:
> compatible:
> - const: google,cros-ec-keyb
> + oneOf:
> + - items:
> + - const: google,cros-ec-keyb-switches
> + - items:
> + - const: google,cros-ec-keyb-switches
> + - const: google,cros-ec-keyb
> + - items:
> + - const: google,cros-ec-keyb
>
> google,needs-ghost-filter:
> description:
> @@ -41,15 +46,40 @@ properties:
> where the lower 16 bits are reserved. This property is specified only
> when the keyboard has a custom design for the top row keys.
>
> +dependencies:
> + function-row-phsymap: [ 'linux,keymap' ]
> + google,needs-ghost-filter: [ 'linux,keymap' ]
> +
> required:
> - compatible
>
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: google,cros-ec-keyb
> + then:
> + allOf:
> + - $ref: "/schemas/input/matrix-keymap.yaml#"
> + - if:
> + not:
> + properties:
> + compatible:
> + contains:
> + const: google,cros-ec-keyb-switches
> + then:
> + required:
> + - keypad,num-rows
> + - keypad,num-columns
> + - linux,keymap
I think that:
1. If you only have buttons/switches and care about backward
compatibility, you use the "two compatibles" version.
2. If you care about backward compatibility then you _must_ include
the matrix properties.
Thus I would be tempted to say that we should just have one "if" test
that says that if matrix properties are allowed then they're also
required.
That goes against the recently landed commit 4352e23a7ff2 ("Input:
cros-ec-keyb - only register keyboard if rows/columns exist") but
perhaps we should just _undo_ that that since it landed pretty
recently and say that the truly supported way to specify that you only
have keyboards/switches is with the compatible.
What do you think?
-Doug
On Mon, May 2, 2022 at 10:00 AM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Fri, Apr 29, 2022 at 4:31 PM Stephen Boyd <[email protected]> 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 <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: <[email protected]>
> > Cc: Benson Leung <[email protected]>
> > Cc: Guenter Roeck <[email protected]>
> > Cc: Douglas Anderson <[email protected]>
> > Cc: Hsin-Yi Wang <[email protected]>
> > Cc: "Joseph S. Barrera III" <[email protected]>
> > Signed-off-by: Stephen Boyd <[email protected]>
> > ---
> > .../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
Quoting Stephen Boyd (2022-05-02 13:41:33)
> Quoting Dmitry Torokhov (2022-05-02 10:43:06)
>
> > We have
> > implemented the notion that without rows/columns properties we will
> > not be creating input device for the matrix portion, all older devices
> > should have it defined, so the newer driver is compatible with them...
> >
>
> Agreed, that solves half the problem. This new compatible eases
> integration so that devicetrees can say they're compatible with the old
> binding that _requires_ the rows/column properties. By making the driver
> change we loosened that requirement, but the binding should have been
> making the properties required at the start because it fails to bind
> otherwise.
>
> My interpretation of what Doug is saying is that we should maintain that
> requirement that rows/columns exists if the original compatible
> google,cros-ec-keyb is present and use the new compatible to indicate
> that there are switches. Combining the two compatibles means there's
> switches and a matrix keyboard, having only the switches compatible
> means only switches, and having only the keyboard compatible means only
> matrix keyboard.
>
> It sounds OK to me.
There's one more thing to mention. The switches are discovered by
querying the EC. Reverting commit 4352e23a7ff2 ("Input: cros-ec-keyb -
only register keyboard if rows/columns exist") makes it so that in the
case you have a keyboard and switches you'll be tempted to define both
compatibles because you have some switches, but for all practical
purposes you don't need to change anything. The EC will still be queried
for the switches. Maybe "google,cros-ec-keyb-switches" is a bad name. It
should really be "google,cros-ec-keyb-v2" or
"google,cros-ec-keyb-optional" where we clarify that matrix keyboard
properties are optional now and are used to indicate if there's a matrix
keyboard or not.
Hi,
On Mon, May 2, 2022 at 1:41 PM Stephen Boyd <[email protected]> wrote:
>
> Quoting Dmitry Torokhov (2022-05-02 10:43:06)
> > On Mon, May 2, 2022 at 10:00 AM Doug Anderson <[email protected]> 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.
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 <[email protected]> wrote:
> > > >
> > > > That goes against the recently landed commit 4352e23a7ff2 ("Input:
> > > > cros-ec-keyb - only register keyboard if rows/columns exist") but
> > > > perhaps we should just _undo_ that that since it landed pretty
> > > > recently and say that the truly supported way to specify that you only
> > > > have keyboards/switches is with the compatible.
> > > >
> > > > What do you think?
> > >
> > > I am sorry, I am still confused on what exactly we are trying to solve
> > > here? Having a device with the new device tree somehow run an older
> > > kernel and fail? Why exactly do we care about this case?
> >
> > Yes, we're trying to solve the problem where a new device tree is used
> > with an older kernel because it doesn't have the driver patch to only
> > create an input device for the matrix when rows/columns properties are
> > present. Otherwise applying that devicetree patch to an older kernel
> > will break bisection.
>
> Well, my recommendation here would be: "do not do that". How exactly
> will you get new DTS into a device with older kernel, and why would you
> do that?
It's about easing the transition to a new programming model of the
driver. We could "not do that" and consciously decide to only use new
DTBs with new kernels. Or we could take this multiple compatible
approach and things work with all combinations. I'd like to make
transitions smooth so introducing a second compatible string is the
solution for that.
Another "what if" scenario is that the rows/columns properties should
have been required per the DT binding all along. If they were required
to begin with, I wouldn't have been able to make them optional without
introducing a new compatible string that the schema keyed off of to
figure out that they're optional sometimes.
>
>
> >
> > > We have
> > > implemented the notion that without rows/columns properties we will
> > > not be creating input device for the matrix portion, all older devices
> > > should have it defined, so the newer driver is compatible with them...
> > >
> >
> > Agreed, that solves half the problem. This new compatible eases
> > integration so that devicetrees can say they're compatible with the old
> > binding that _requires_ the rows/column properties. By making the driver
> > change we loosened that requirement, but the binding should have been
> > making the properties required at the start because it fails to bind
> > otherwise.
> >
> > My interpretation of what Doug is saying is that we should maintain that
> > requirement that rows/columns exists if the original compatible
> > google,cros-ec-keyb is present and use the new compatible to indicate
> > that there are switches. Combining the two compatibles means there's
> > switches and a matrix keyboard, having only the switches compatible
> > means only switches, and having only the keyboard compatible means only
> > matrix keyboard.
> >
> > It sounds OK to me.
>
> Have we solved module loading in the presence of multiple compatibles?
> IIRC we only ever try to load module on the first compatible, so you'd
> be breaking autoloading cros-ec-keyb on these older kernels. I think the
> cure that is being proposed is worse than the disease.
>
The first compatible is still cros-ec-keyb in the driver though? Or you
mean the first compatible in the node? I'm not aware of this problem at
all but I can certainly test out a fake node and module and see if it
gets autoloaded.
Quoting Stephen Boyd (2022-05-12 16:46:22)
>
> I think I covered that in v3 of this series[1].
Even better, see v4
https://lore.kernel.org/all/[email protected]/
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 <[email protected]> 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
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/[email protected]/
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/[email protected]/
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
Quoting Stephen Boyd (2022-05-12 11:58:02)
> Quoting Dmitry Torokhov (2022-05-12 03:22:30)
> >
> > Have we solved module loading in the presence of multiple compatibles?
> > IIRC we only ever try to load module on the first compatible, so you'd
> > be breaking autoloading cros-ec-keyb on these older kernels. I think the
> > cure that is being proposed is worse than the disease.
> >
>
> The first compatible is still cros-ec-keyb in the driver though? Or you
> mean the first compatible in the node? I'm not aware of this problem at
> all but I can certainly test out a fake node and module and see if it
> gets autoloaded.
I can't get this test module to fail to load no matter what I do. I
commented out the second match table entry, and kept it there and
removed 'vendor,switch-compat' from the DTS. Module still autoloads.
----8<----
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 42a87fa4976e..a6173b79ba67 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -54,6 +54,10 @@ aliases {
spi11 = &spi11;
};
+ mynode {
+ compatible = "vendor,switch-compat", "vendor,keyb-compat";
+ };
+
clocks {
xo_board: xo-board {
compatible = "fixed-clock";
diff --git a/lib/Makefile b/lib/Makefile
index a841be5244ac..0a784011feb5 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -74,6 +74,7 @@ UBSAN_SANITIZE_test_ubsan.o := y
obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
obj-$(CONFIG_TEST_LIST_SORT) += test_list_sort.o
obj-$(CONFIG_TEST_MIN_HEAP) += test_min_heap.o
+obj-m += dtmod.o
obj-$(CONFIG_TEST_LKM) += test_module.o
obj-$(CONFIG_TEST_VMALLOC) += test_vmalloc.o
obj-$(CONFIG_TEST_OVERFLOW) += test_overflow.o
diff --git a/lib/dtmod.c b/lib/dtmod.c
new file mode 100644
index 000000000000..c34ae37b8ff0
--- /dev/null
+++ b/lib/dtmod.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+
+static int test_probe(struct platform_device *pdev)
+{
+ dev_info(&pdev->dev, "I got probed\n");
+
+ return 0;
+}
+
+static int test_remove(struct platform_device *pdev)
+{
+ dev_info(&pdev->dev, "I got removed\n");
+
+ return 0;
+}
+
+static const struct of_device_id test_of_match[] = {
+ { .compatible = "vendor,keyb-compat" },
+ { .compatible = "vendor,switch-compat" }, // comment out
+ {}
+};
+MODULE_DEVICE_TABLE(of, test_of_match);
+
+static struct platform_driver test_keyb_driver = {
+ .probe = test_probe,
+ .remove = test_remove,
+ .driver = {
+ .name = "test-ec-keyb",
+ .of_match_table = test_of_match,
+ },
+};
+
+module_platform_driver(test_keyb_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:test-ec-keyb");
On Thu, May 12, 2022 at 01:11:39PM -0700, Stephen Boyd wrote:
> Quoting Stephen Boyd (2022-05-12 11:58:02)
> > Quoting Dmitry Torokhov (2022-05-12 03:22:30)
> > >
> > > Have we solved module loading in the presence of multiple compatibles?
> > > IIRC we only ever try to load module on the first compatible, so you'd
> > > be breaking autoloading cros-ec-keyb on these older kernels. I think the
> > > cure that is being proposed is worse than the disease.
> > >
> >
> > The first compatible is still cros-ec-keyb in the driver though? Or you
> > mean the first compatible in the node? I'm not aware of this problem at
> > all but I can certainly test out a fake node and module and see if it
> > gets autoloaded.
>
> I can't get this test module to fail to load no matter what I do. I
> commented out the second match table entry, and kept it there and
> removed 'vendor,switch-compat' from the DTS. Module still autoloads.
>
Ah, indeed, if the module contains both compatibles we will load it. It
is broken when we have 2 or more modules and DT lists several
compatibles for a device.
OK, it looks like you feel very strongly regarding having a dedicated
compatible. In this case please make sure that the compatible's behavior
is properly documented (i.e. google,cros-ec-keyb compatible does not
imply that there are *NO* switches, and users having buttons and
switches in addition to matrix keys can also use google,cros-ec-keyb as
a compatible for their device). We also need to mention that with the
2nd compatible the device still can report key/button events, it is
simply that there is no matrix component. Should we call the other
compatible google,cros-ec-bs?
We should also abort binding the device if it specifies the new
compatible, but EC does not report any buttons or switches.
Thanks.
--
Dmitry