| Message ID | 20221204061555.1355453-2-dmitry.baryshkov@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | dt-bindings: add missing subdevices to qcom-pm8xxx schema | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 93 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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
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/
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/
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
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.
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 --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>; - };
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