diff mbox series

[v2,1/4] dt-bindings: input: qcom,pm8921-keypad: convert to YAML format

Message ID 20221204061555.1355453-2-dmitry.baryshkov@linaro.org (mailing list archive)
State Handled Elsewhere
Headers show
Series dt-bindings: add missing subdevices to qcom-pm8xxx schema | expand

Commit Message

Dmitry Baryshkov Dec. 4, 2022, 6:15 a.m. UTC 
Convert the bindings for the keypad subdevices of Qualcomm PM8921 and
PM8058 PMICs from text to YAML format.

While doing the conversion also change linux,keypad-no-autorepeat
property to linux,input-no-autorepeat. The former property was never
used by DT and was never handled by the driver.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../bindings/input/qcom,pm8921-keypad.yaml    | 93 +++++++++++++++++++
 .../bindings/input/qcom,pm8xxx-keypad.txt     | 90 ------------------
 2 files changed, 93 insertions(+), 90 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml
 delete mode 100644 Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt

Comments

kernel test robot Dec. 5, 2022, 1:45 a.m. UTC | #1 
Hi Dmitry,

I love your patch! Perhaps something to improve:

[auto build test WARNING on pavel-leds/for-next]
[also build test WARNING on robh/for-next linus/master v6.1-rc8 next-20221202]
[cannot apply to lee-mfd/for-mfd-next dtor-input/next dtor-input/for-linus jic23-iio/togreg lee-mfd/for-mfd-fixes]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Dmitry-Baryshkov/dt-bindings-add-missing-subdevices-to-qcom-pm8xxx-schema/20221204-141636
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link:    https://lore.kernel.org/r/20221204061555.1355453-2-dmitry.baryshkov%40linaro.org
patch subject: [PATCH v2 1/4] dt-bindings: input: qcom,pm8921-keypad: convert to YAML format
reproduce:
        # https://github.com/intel-lab-lkp/linux/commit/bf8956f7ed4b2e21f980a771e4dbc1c451addc8d
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Dmitry-Baryshkov/dt-bindings-add-missing-subdevices-to-qcom-pm8xxx-schema/20221204-141636
        git checkout bf8956f7ed4b2e21f980a771e4dbc1c451addc8d
        make menuconfig
        # enable CONFIG_COMPILE_TEST, CONFIG_WARN_MISSING_DOCUMENTS, CONFIG_WARN_ABI_ERRORS
        make htmldocs

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> Warning: Documentation/devicetree/bindings/power/wakeup-source.txt references a file that doesn't exist: Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
Rob Herring Dec. 5, 2022, 10:04 p.m. UTC | #2 
On Sun, Dec 04, 2022 at 08:15:52AM +0200, Dmitry Baryshkov wrote:
> Convert the bindings for the keypad subdevices of Qualcomm PM8921 and
> PM8058 PMICs from text to YAML format.
> 
> While doing the conversion also change linux,keypad-no-autorepeat
> property to linux,input-no-autorepeat. The former property was never
> used by DT and was never handled by the driver.

Changing from the documented one to one some drivers use. I guess 
that's a slight improvement. Please see this discussion[1]. 

Rob

[1] https://lore.kernel.org/all/YowEgvwBOSEK+kd2@google.com/
Dmitry Baryshkov Dec. 6, 2022, 3:20 a.m. UTC | #3 
6 декабря 2022 г. 00:04:33 GMT+02:00, Rob Herring <robh@kernel.org> пишет:
>On Sun, Dec 04, 2022 at 08:15:52AM +0200, Dmitry Baryshkov wrote:
>> Convert the bindings for the keypad subdevices of Qualcomm PM8921 and
>> PM8058 PMICs from text to YAML format.
>> 
>> While doing the conversion also change linux,keypad-no-autorepeat
>> property to linux,input-no-autorepeat. The former property was never
>> used by DT and was never handled by the driver.
>
>Changing from the documented one to one some drivers use. I guess 
>that's a slight improvement. Please see this discussion[1]. 

Well, the problem is that the documentation is misleading. The driver doesn't handle the documented property, so we should change either the driver, or the docs. Which change is the preferred one?

>
>Rob
>
>[1] https://lore.kernel.org/all/YowEgvwBOSEK+kd2@google.com/
Rob Herring Dec. 7, 2022, 5:07 p.m. UTC | #4 
On Tue, Dec 06, 2022 at 05:20:16AM +0200, Dmitry Baryshkov wrote:
> 6 декабря 2022 г. 00:04:33 GMT+02:00, Rob Herring <robh@kernel.org> пишет:
> >On Sun, Dec 04, 2022 at 08:15:52AM +0200, Dmitry Baryshkov wrote:
> >> Convert the bindings for the keypad subdevices of Qualcomm PM8921 and
> >> PM8058 PMICs from text to YAML format.
> >> 
> >> While doing the conversion also change linux,keypad-no-autorepeat
> >> property to linux,input-no-autorepeat. The former property was never
> >> used by DT and was never handled by the driver.
> >
> >Changing from the documented one to one some drivers use. I guess 
> >that's a slight improvement. Please see this discussion[1]. 
> 
> Well, the problem is that the documentation is misleading. The driver 
> doesn't handle the documented property, so we should change either 
> the driver, or the docs. Which change is the preferred one?

The preference is autorepeat is not the default and setting 
'autorepeat' enables it. You can't really change that unless you don't 
really need autorepeat by default. I can't see why it would be 
needed for the power button, but I haven't looked what else you have.

Of all the no autorepeat options, I prefer 'linux,no-autorepeat' as I 
find 'input' or 'keypad' redundant. But Dmitry T. didn't think it should 
be a common property at the time.

Rob
Dmitry Baryshkov Dec. 7, 2022, 6:36 p.m. UTC | #5 
On Wed, 7 Dec 2022 at 19:07, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Dec 06, 2022 at 05:20:16AM +0200, Dmitry Baryshkov wrote:
> > 6 декабря 2022 г. 00:04:33 GMT+02:00, Rob Herring <robh@kernel.org> пишет:
> > >On Sun, Dec 04, 2022 at 08:15:52AM +0200, Dmitry Baryshkov wrote:
> > >> Convert the bindings for the keypad subdevices of Qualcomm PM8921 and
> > >> PM8058 PMICs from text to YAML format.
> > >>
> > >> While doing the conversion also change linux,keypad-no-autorepeat
> > >> property to linux,input-no-autorepeat. The former property was never
> > >> used by DT and was never handled by the driver.
> > >
> > >Changing from the documented one to one some drivers use. I guess
> > >that's a slight improvement. Please see this discussion[1].
> >
> > Well, the problem is that the documentation is misleading. The driver
> > doesn't handle the documented property, so we should change either
> > the driver, or the docs. Which change is the preferred one?
>
> The preference is autorepeat is not the default and setting
> 'autorepeat' enables it. You can't really change that unless you don't
> really need autorepeat by default. I can't see why it would be
> needed for the power button, but I haven't looked what else you have.

It's not a pon/resin. this is a full-fledged keypad. For example for
apq8060-dragonboard:

linux,keymap = <
        MATRIX_KEY(0, 0, KEY_MENU)
        MATRIX_KEY(0, 2, KEY_1)
        MATRIX_KEY(0, 3, KEY_4)
        MATRIX_KEY(0, 4, KEY_7)
        MATRIX_KEY(1, 0, KEY_UP)
        MATRIX_KEY(1, 1, KEY_LEFT)
        MATRIX_KEY(1, 2, KEY_DOWN)
        MATRIX_KEY(1, 3, KEY_5)
        MATRIX_KEY(1, 3, KEY_8)
        MATRIX_KEY(2, 0, KEY_HOME)
        MATRIX_KEY(2, 1, KEY_REPLY)
        MATRIX_KEY(2, 2, KEY_2)
        MATRIX_KEY(2, 3, KEY_6)
        MATRIX_KEY(3, 0, KEY_VOLUMEUP)
        MATRIX_KEY(3, 1, KEY_RIGHT)
        MATRIX_KEY(3, 2, KEY_3)
        MATRIX_KEY(3, 3, KEY_9)
        MATRIX_KEY(3, 4, KEY_SWITCHVIDEOMODE)
        MATRIX_KEY(4, 0, KEY_VOLUMEDOWN)
        MATRIX_KEY(4, 1, KEY_BACK)
        MATRIX_KEY(4, 2, KEY_CAMERA)
        MATRIX_KEY(4, 3, KEY_KBDILLUMTOGGLE)
                                        >;

> Of all the no autorepeat options, I prefer 'linux,no-autorepeat' as I
> find 'input' or 'keypad' redundant. But Dmitry T. didn't think it should
> be a common property at the time.

We have not used any of the options in the in-kernel DTs. However the
driver for the keypad has supported the 'linux,input-no-autorepeat'
since March 2014. I'm just changing the docs to document the correct
option. I can split the patch into two distinct patches (one for the
bugfix, one for conversion), if you think that it would be better.
Dmitry Torokhov Dec. 7, 2022, 7:32 p.m. UTC | #6 
On Wed, Dec 07, 2022 at 11:07:53AM -0600, Rob Herring wrote:
> On Tue, Dec 06, 2022 at 05:20:16AM +0200, Dmitry Baryshkov wrote:
> > 6 декабря 2022 г. 00:04:33 GMT+02:00, Rob Herring <robh@kernel.org> пишет:
> > >On Sun, Dec 04, 2022 at 08:15:52AM +0200, Dmitry Baryshkov wrote:
> > >> Convert the bindings for the keypad subdevices of Qualcomm PM8921 and
> > >> PM8058 PMICs from text to YAML format.
> > >> 
> > >> While doing the conversion also change linux,keypad-no-autorepeat
> > >> property to linux,input-no-autorepeat. The former property was never
> > >> used by DT and was never handled by the driver.
> > >
> > >Changing from the documented one to one some drivers use. I guess 
> > >that's a slight improvement. Please see this discussion[1]. 
> > 
> > Well, the problem is that the documentation is misleading. The driver 
> > doesn't handle the documented property, so we should change either 
> > the driver, or the docs. Which change is the preferred one?
> 
> The preference is autorepeat is not the default and setting 
> 'autorepeat' enables it. You can't really change that unless you don't 
> really need autorepeat by default. I can't see why it would be 
> needed for the power button, but I haven't looked what else you have.
> 
> Of all the no autorepeat options, I prefer 'linux,no-autorepeat' as I 
> find 'input' or 'keypad' redundant. But Dmitry T. didn't think it should 
> be a common property at the time.

Right, I would prefer for new bindings we used assertive "autorepeat",
instead of negating "no-autorepeat". However here we are dealing with
existing binding.

The issue is complicated with the driver using one option, binding
specifying another, and existing in-kernel DTSes not using any and thus
activating autorepeat as the default driver behavior.

Do we have an idea if there are out-of-tree users of this? If we are
reasonable sure there are not we could converge on the standard
"autorepeat" property.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml b/Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml
new file mode 100644
index 000000000000..e3c53a8234c5
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml
@@ -0,0 +1,93 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/qcom,pm8921-keypad.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm PM8921 PMIC KeyPad
+
+maintainers:
+  - Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
+
+allOf:
+  - $ref: input.yaml#
+  - $ref: matrix-keymap.yaml#
+
+properties:
+  compatible:
+    enum:
+      - qcom,pm8058-keypad
+      - qcom,pm8921-keypad
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    items:
+      - description: key sense
+      - description: key stuck
+
+  linux,input-no-autorepeat:
+    type: boolean
+    description: don't enable autorepeat feature.
+
+  wakeup-source:
+    type: boolean
+    description: use any event on keypad as wakeup event
+
+  linux,keypad-wakeup:
+    type: boolean
+    deprecated: true
+    description: legacy version of the wakeup-source property
+
+  debounce:
+    description:
+      Time in microseconds that key must be pressed or
+      released for state change interrupt to trigger.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  scan-delay:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: time in microseconds to pause between successive scans of the
+      matrix array
+
+  row-hold:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: time in nanoseconds to pause between scans of each row in the
+      matrix array.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - linux,keymap
+
+unevaluatedProperties: false
+
+examples:
+  - |
+   #include <dt-bindings/input/input.h>
+   #include <dt-bindings/interrupt-controller/irq.h>
+   pmic {
+       #address-cells = <1>;
+       #size-cells = <0>;
+
+       keypad@148 {
+           compatible = "qcom,pm8921-keypad";
+           reg = <0x148>;
+           interrupt-parent = <&pmicintc>;
+           interrupts = <74 IRQ_TYPE_EDGE_RISING>, <75 IRQ_TYPE_EDGE_RISING>;
+           linux,keymap = <
+               MATRIX_KEY(0, 0, KEY_VOLUMEUP)
+               MATRIX_KEY(0, 1, KEY_VOLUMEDOWN)
+               MATRIX_KEY(0, 2, KEY_CAMERA_FOCUS)
+               MATRIX_KEY(0, 3, KEY_CAMERA)
+           >;
+           keypad,num-rows = <1>;
+           keypad,num-columns = <5>;
+           debounce = <15>;
+           scan-delay = <32>;
+           row-hold = <91500>;
+       };
+   };
+...
diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
deleted file mode 100644
index 4a9dc6ba96b1..000000000000
--- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
+++ /dev/null
@@ -1,90 +0,0 @@ 
-Qualcomm PM8xxx PMIC Keypad
-
-PROPERTIES
-
-- compatible:
-	Usage: required
-	Value type: <string>
-	Definition: must be one of:
-		    "qcom,pm8058-keypad"
-		    "qcom,pm8921-keypad"
-
-- reg:
-	Usage: required
-	Value type: <prop-encoded-array>
-	Definition: address of keypad control register
-
-- interrupts:
-	Usage: required
-	Value type: <prop-encoded-array>
-	Definition: the first interrupt specifies the key sense interrupt
-		    and the second interrupt specifies the key stuck interrupt.
-		    The format of the specifier is defined by the binding
-		    document describing the node's interrupt parent.
-
-- linux,keymap:
-	Usage: required
-	Value type: <prop-encoded-array>
-	Definition: the linux keymap. More information can be found in
-		    input/matrix-keymap.txt.
-
-- linux,keypad-no-autorepeat:
-	Usage: optional
-	Value type: <bool>
-	Definition: don't enable autorepeat feature.
-
-- wakeup-source:
-	Usage: optional
-	Value type: <bool>
-	Definition: use any event on keypad as wakeup event.
-		    (Legacy property supported: "linux,keypad-wakeup")
-
-- keypad,num-rows:
-	Usage: required
-	Value type: <u32>
-	Definition: number of rows in the keymap. More information can be found
-		    in input/matrix-keymap.txt.
-
-- keypad,num-columns:
-	Usage: required
-	Value type: <u32>
-	Definition: number of columns in the keymap. More information can be
-		    found in input/matrix-keymap.txt.
-
-- debounce:
-	Usage: optional
-	Value type: <u32>
-	Definition: time in microseconds that key must be pressed or release
-		    for key sense interrupt to trigger.
-
-- scan-delay:
-	Usage: optional
-	Value type: <u32>
-	Definition: time in microseconds to pause between successive scans
-		    of the matrix array.
-
-- row-hold:
-	Usage: optional
-	Value type: <u32>
-	Definition: time in nanoseconds to pause between scans of each row in
-		    the matrix array.
-
-EXAMPLE
-
-	keypad@148 {
-		compatible = "qcom,pm8921-keypad";
-		reg = <0x148>;
-		interrupt-parent = <&pmicintc>;
-		interrupts = <74 1>, <75 1>;
-		linux,keymap = <
-			MATRIX_KEY(0, 0, KEY_VOLUMEUP)
-			MATRIX_KEY(0, 1, KEY_VOLUMEDOWN)
-			MATRIX_KEY(0, 2, KEY_CAMERA_FOCUS)
-			MATRIX_KEY(0, 3, KEY_CAMERA)
-			>;
-		keypad,num-rows = <1>;
-		keypad,num-columns = <5>;
-		debounce = <15>;
-		scan-delay = <32>;
-		row-hold = <91500>;
-	};