| Message ID | 20240819045813.2154642-10-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Remove support for platform data from samsung keypad | expand |
On Sun, Aug 18, 2024 at 09:58:06PM -0700, Dmitry Torokhov wrote: > The binding with a sub-node per each key is very verbose and is hard to > use with static device properties. Allow standard matrix keymap binding > in addition to the verbose one. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > .../input/samsung,s3c6410-keypad.yaml | 57 ++++++++++++++++++- > 1 file changed, 54 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml > index a53569aa0ee7..28a318a0ff7e 100644 > --- a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml > +++ b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml > @@ -16,6 +16,10 @@ description: > maintainers: > - Krzysztof Kozlowski <krzk@kernel.org> > > +allOf: > + - $ref: input.yaml# > + - $ref: matrix-keymap.yaml# > + > properties: > compatible: > enum: > @@ -37,6 +41,10 @@ properties: > > wakeup-source: true > > + keypad,num-columns: true > + keypad,num-rows: true > + linux,keymap: true > + > linux,input-no-autorepeat: > type: boolean > description: > @@ -81,12 +89,33 @@ patternProperties: > - keypad,row > - linux,code > > +dependencies: > + linux,keymap: > + required: Why "required" keyword? The dependencies should have just list of properties. See example-schema. > + - keypad,num-columns > + - keypad,num-rows > + > required: > - compatible > - reg > - interrupts > - - samsung,keypad-num-columns > - - samsung,keypad-num-rows > + > +if: put allOf: here and this within allOf, so you the "if" could grow in the future. Best regards, Krzysztof
On Mon, Aug 19, 2024 at 03:02:07PM +0200, Krzysztof Kozlowski wrote: > On Sun, Aug 18, 2024 at 09:58:06PM -0700, Dmitry Torokhov wrote: > > The binding with a sub-node per each key is very verbose and is hard to > > use with static device properties. Allow standard matrix keymap binding > > in addition to the verbose one. > > > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > --- > > .../input/samsung,s3c6410-keypad.yaml | 57 ++++++++++++++++++- > > 1 file changed, 54 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml > > index a53569aa0ee7..28a318a0ff7e 100644 > > --- a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml > > +++ b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml > > @@ -16,6 +16,10 @@ description: > > maintainers: > > - Krzysztof Kozlowski <krzk@kernel.org> > > > > +allOf: > > + - $ref: input.yaml# > > + - $ref: matrix-keymap.yaml# > > + > > properties: > > compatible: > > enum: > > @@ -37,6 +41,10 @@ properties: > > > > wakeup-source: true > > > > + keypad,num-columns: true > > + keypad,num-rows: true > > + linux,keymap: true > > + > > linux,input-no-autorepeat: > > type: boolean > > description: > > @@ -81,12 +89,33 @@ patternProperties: > > - keypad,row > > - linux,code > > > > +dependencies: > > + linux,keymap: > > + required: > > Why "required" keyword? The dependencies should have just list of > properties. See example-schema. OK, changed this to dependencies: linux,keymap: [ "keypad,num-columns", "keypad,num-rows" ] > > > + - keypad,num-columns > > + - keypad,num-rows > > + > > required: > > - compatible > > - reg > > - interrupts > > - - samsung,keypad-num-columns > > - - samsung,keypad-num-rows > > + > > +if: > > put allOf: here and this within allOf, so you the "if" could grow in the > future. Hmm, there is already "allOf" at the beginning of the file, so adding another one results in complaints about duplicate "allOf". I can move it all to the top, like this: allOf: - $ref: input.yaml# - $ref: matrix-keymap.yaml# - if: required: - linux,keymap then: properties: samsung,keypad-num-columns: false samsung,keypad-num-rows: false patternProperties: '^key-[0-9a-z]+$': false else: properties: keypad,num-columns: false keypad,num-rows: false required: - samsung,keypad-num-columns - samsung,keypad-num-rows Is this OK? I don't quite like that "tweaks" are listed before main body of properties. Thanks.
On Mon, Aug 19, 2024 at 08:49:10AM -0700, Dmitry Torokhov wrote: > On Mon, Aug 19, 2024 at 03:02:07PM +0200, Krzysztof Kozlowski wrote: > > On Sun, Aug 18, 2024 at 09:58:06PM -0700, Dmitry Torokhov wrote: > > > > > + - keypad,num-columns > > > + - keypad,num-rows > > > + > > > required: > > > - compatible > > > - reg > > > - interrupts > > > - - samsung,keypad-num-columns > > > - - samsung,keypad-num-rows > > > + > > > +if: > > > > put allOf: here and this within allOf, so you the "if" could grow in the > > future. > > Hmm, there is already "allOf" at the beginning of the file, so adding > another one results in complaints about duplicate "allOf". I can move it > all to the top, like this: > > allOf: > - $ref: input.yaml# > - $ref: matrix-keymap.yaml# > - if: > required: > - linux,keymap > then: > properties: > samsung,keypad-num-columns: false > samsung,keypad-num-rows: false > patternProperties: > '^key-[0-9a-z]+$': false > else: > properties: > keypad,num-columns: false > keypad,num-rows: false > required: > - samsung,keypad-num-columns > - samsung,keypad-num-rows > > Is this OK? I don't quite like that "tweaks" are listed before main > body of properties. The normal thing to do is to put the allOf at the end, not the start, in cases like this, for the reason you mention.
On Mon, Aug 19, 2024 at 05:48:06PM +0100, Conor Dooley wrote: > On Mon, Aug 19, 2024 at 08:49:10AM -0700, Dmitry Torokhov wrote: > > On Mon, Aug 19, 2024 at 03:02:07PM +0200, Krzysztof Kozlowski wrote: > > > On Sun, Aug 18, 2024 at 09:58:06PM -0700, Dmitry Torokhov wrote: > > > > > > > > + - keypad,num-columns > > > > + - keypad,num-rows > > > > + > > > > required: > > > > - compatible > > > > - reg > > > > - interrupts > > > > - - samsung,keypad-num-columns > > > > - - samsung,keypad-num-rows > > > > + > > > > +if: > > > > > > put allOf: here and this within allOf, so you the "if" could grow in the > > > future. > > > > Hmm, there is already "allOf" at the beginning of the file, so adding > > another one results in complaints about duplicate "allOf". I can move it > > all to the top, like this: > > > > allOf: > > - $ref: input.yaml# > > - $ref: matrix-keymap.yaml# > > - if: > > required: > > - linux,keymap > > then: > > properties: > > samsung,keypad-num-columns: false > > samsung,keypad-num-rows: false > > patternProperties: > > '^key-[0-9a-z]+$': false > > else: > > properties: > > keypad,num-columns: false > > keypad,num-rows: false > > required: > > - samsung,keypad-num-columns > > - samsung,keypad-num-rows > > > > Is this OK? I don't quite like that "tweaks" are listed before main > > body of properties. > > The normal thing to do is to put the allOf at the end, not the start, in > cases like this, for the reason you mention. I see, thanks. It would be nice if it could combine several "allOf"s into one internally. Thanks.
diff --git a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml index a53569aa0ee7..28a318a0ff7e 100644 --- a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml +++ b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml @@ -16,6 +16,10 @@ description: maintainers: - Krzysztof Kozlowski <krzk@kernel.org> +allOf: + - $ref: input.yaml# + - $ref: matrix-keymap.yaml# + properties: compatible: enum: @@ -37,6 +41,10 @@ properties: wakeup-source: true + keypad,num-columns: true + keypad,num-rows: true + linux,keymap: true + linux,input-no-autorepeat: type: boolean description: @@ -81,12 +89,33 @@ patternProperties: - keypad,row - linux,code +dependencies: + linux,keymap: + required: + - keypad,num-columns + - keypad,num-rows + required: - compatible - reg - interrupts - - samsung,keypad-num-columns - - samsung,keypad-num-rows + +if: + required: + - linux,keymap +then: + properties: + samsung,keypad-num-columns: false + samsung,keypad-num-rows: false + patternProperties: + '^key-[0-9a-z]+$': false +else: + properties: + keypad,num-columns: false + keypad,num-rows: false + required: + - samsung,keypad-num-columns + - samsung,keypad-num-rows additionalProperties: false @@ -94,8 +123,9 @@ examples: - | #include <dt-bindings/clock/exynos4.h> #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/input/input.h> - keypad@100a0000 { + keypad1@100a0000 { compatible = "samsung,s5pv210-keypad"; reg = <0x100a0000 0x100>; interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>; @@ -119,3 +149,24 @@ examples: linux,code = <3>; }; }; + - | + #include <dt-bindings/clock/exynos4.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/input/input.h> + + keypad2@100a0000 { + compatible = "samsung,s5pv210-keypad"; + reg = <0x100a0000 0x100>; + interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clock CLK_KEYIF>; + clock-names = "keypad"; + + keypad,num-rows = <2>; + keypad,num-columns = <8>; + linux,keymap = < + MATRIX_KEY(0, 3, 2) + MATRIX_KEY(0, 4, 3) + >; + linux,input-no-autorepeat; + wakeup-source; + };
The binding with a sub-node per each key is very verbose and is hard to use with static device properties. Allow standard matrix keymap binding in addition to the verbose one. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- .../input/samsung,s3c6410-keypad.yaml | 57 ++++++++++++++++++- 1 file changed, 54 insertions(+), 3 deletions(-)