On Mon, May 2, 2022 at 10:00 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Fri, Apr 29, 2022 at 4:31 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> 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@xxxxxxxxxx>
> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > Cc: <devicetree@xxxxxxxxxxxxxxx>
> > Cc: Benson Leung <bleung@xxxxxxxxxxxx>
> > Cc: Guenter Roeck <groeck@xxxxxxxxxxxx>
> > Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> > Cc: Hsin-Yi Wang <hsinyi@xxxxxxxxxxxx>
> > Cc: "Joseph S. Barrera III" <joebar@xxxxxxxxxxxx>
> > Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> > ---
> > .../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
