mbox series

[v2,0/4] dt-bindings: add missing subdevices to qcom-pm8xxx schema

Message ID 20221204061555.1355453-1-dmitry.baryshkov@linaro.org
Headers show
Series dt-bindings: add missing subdevices to qcom-pm8xxx schema | expand

Message

Dmitry Baryshkov Dec. 4, 2022, 6:15 a.m. UTC
Update the DT bindings of the Qualcomm PMIC devices sitting on the SSBI
bus.

Changes since v1:
- Rebased on top of [1]

[1]: https://lore.kernel.org/all/20221201131505.42292-1-krzysztof.kozlowski@linaro.org/

Dmitry Baryshkov (4):
  dt-bindings: input: qcom,pm8921-keypad: convert to YAML format
  dt-bindings: mfd: qcom-pm8xxx: add missing child nodes
  dt-bindings: iio: adc: qcom,pm8018-adc: allow specifying MPP channels
  dt-bindings: leds: Add 'cm3605' to 'linux,default-trigger'

 .../bindings/iio/adc/qcom,pm8018-adc.yaml     |  2 +-
 .../bindings/input/qcom,pm8921-keypad.yaml    | 93 +++++++++++++++++++
 .../bindings/input/qcom,pm8xxx-keypad.txt     | 90 ------------------
 .../devicetree/bindings/leds/common.yaml      |  1 +
 .../devicetree/bindings/mfd/qcom-pm8xxx.yaml  | 26 +++++-
 5 files changed, 120 insertions(+), 92 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml
 delete mode 100644 Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt

Comments

Dmitry Baryshkov Dec. 4, 2022, 3:43 p.m. UTC | #1
On Sun, 4 Dec 2022 at 17:24, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun,  4 Dec 2022 08:15:54 +0200
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>
> > Several ADC channels are bound to the Multi Purpose Pins (MPPs). Allow
> > specifying such channels using the mppN device node (as used on apq8060
> > dragonboard).
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> So I understand this more, why do the node names have to have anything to
> do with the particular pin? I'm assuming the reg value provides that
> relationship.

Yes, the reg provides this relationship. If I understand correctly,
the dts authors (see arch/arm/boot/dts/qcom-apq8060-dragonboard.dts)
wanted to point out that these channels are connected to MPP pins
rather than raw adc channels (e.g. vcoin, vbat, etc).

>
> > ---
> >  Documentation/devicetree/bindings/iio/adc/qcom,pm8018-adc.yaml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,pm8018-adc.yaml b/Documentation/devicetree/bindings/iio/adc/qcom,pm8018-adc.yaml
> > index d186b713d6a7..fee30e6ddd62 100644
> > --- a/Documentation/devicetree/bindings/iio/adc/qcom,pm8018-adc.yaml
> > +++ b/Documentation/devicetree/bindings/iio/adc/qcom,pm8018-adc.yaml
> > @@ -64,7 +64,7 @@ required:
> >    - adc-channel@f
> >
> >  patternProperties:
> > -  "^(adc-channel@)[0-9a-f]$":
> > +  "^(adc-channel|mpp[0-9]+)@[0-9a-f]$":
> >      type: object
> >      description: |
> >        ADC channel specific configuration.
>
Rob Herring (Arm) 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 (Arm) 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.