RE: [PATCH v4 8/8] dt-bindings: mfd: dlg,da9063: Convert da9062 to json-schema

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Krzysztof Kozlowski,

Thanks for the feedback.

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Thursday, December 7, 2023 8:44 AM
> Subject: Re: [PATCH v4 8/8] dt-bindings: mfd: dlg,da9063: Convert da9062
> to json-schema
> 
> On 06/12/2023 16:57, Biju Das wrote:
> > Convert the da9062 PMIC device tree binding documentation to json-
> schema.
> >
> > Update the example to match reality.
> >
> > While at it, update description with link to product information.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v3->v4:
> >  * Split the thermal binding patch separate.
> >  * Updated the description.
> > v2->v3:
> >  * Fixed bot errors related to MAINTAINERS entry, invalid doc
> >    references and thermal examples by merging patch#4.
> > v2:
> >  * New patch
> > ---
> >  .../bindings/input/dlg,da9062-onkey.yaml      |   3 +-
> >  .../devicetree/bindings/mfd/da9062.txt        | 124 ------------
> >  .../devicetree/bindings/mfd/dlg,da9063.yaml   | 186 +++++++++++++++++-
> >  .../bindings/thermal/dlg,da9062-thermal.yaml  |   2 +-
> >  4 files changed, 183 insertions(+), 132 deletions(-)  delete mode
> > 100644 Documentation/devicetree/bindings/mfd/da9062.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/input/dlg,da9062-onkey.yaml
> > b/Documentation/devicetree/bindings/input/dlg,da9062-onkey.yaml
> > index 4cff91f4bd34..18b6a3f02c07 100644
> > --- a/Documentation/devicetree/bindings/input/dlg,da9062-onkey.yaml
> > +++ b/Documentation/devicetree/bindings/input/dlg,da9062-onkey.yaml
> > @@ -11,8 +11,7 @@ maintainers:
> >
> >  description: |
> >    This module is part of the DA9061/DA9062/DA9063. For more details
> > about entire
> > -  DA9062 and DA9061 chips see
> > Documentation/devicetree/bindings/mfd/da9062.txt
> > -  For DA9063 see
> > Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
> > +  DA906{1,2,3} chips see
> > + Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
> >
> >    This module provides the KEY_POWER event.
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/da9062.txt
> > b/Documentation/devicetree/bindings/mfd/da9062.txt
> > deleted file mode 100644
> > index c8a7f119ac84..000000000000
> > --- a/Documentation/devicetree/bindings/mfd/da9062.txt
> > +++ /dev/null
> > @@ -1,124 +0,0 @@
> > -* Dialog DA9062 Power Management Integrated Circuit (PMIC)
> > -
> > -Product information for the DA9062 and DA9061 devices can be found
> here:
> > --
&sdata=yWQlq55fIMgsfBxaNXCj4zwBiK14xZcd6l3NYKVnAWM%3D&re
> > served=0
> > -
> > -The DA9062 PMIC consists of:
> > -
> > -Device                   Supply Names    Description
> > -------                   ------------    -----------
> > -da9062-regulator        :               : LDOs & BUCKs
> > -da9062-rtc              :               : Real-Time Clock
> > -da9062-onkey            :               : On Key
> > -da9062-watchdog         :               : Watchdog Timer
> > -da9062-thermal          :               : Thermal
> > -da9062-gpio             :               : GPIOs
> > -
> > -The DA9061 PMIC consists of:
> > -
> > -Device                   Supply Names    Description
> > -------                   ------------    -----------
> > -da9062-regulator        :               : LDOs & BUCKs
> > -da9062-onkey            :               : On Key
> > -da9062-watchdog         :               : Watchdog Timer
> > -da9062-thermal          :               : Thermal
> > -
> > -======
> > -
> > -Required properties:
> > -
> > -- compatible : Should be
> > -    "dlg,da9062" for DA9062
> > -    "dlg,da9061" for DA9061
> > -- reg : Specifies the I2C slave address (this defaults to 0x58 but it
> > can be
> > -  modified to match the chip's OTP settings).
> > -
> > -Optional properties:
> > -
> > -- gpio-controller : Marks the device as a gpio controller.
> > -- #gpio-cells     : Should be two. The first cell is the pin number and
> the
> > -                    second cell is used to specify the gpio polarity.
> > -
> > -See Documentation/devicetree/bindings/gpio/gpio.txt for further
> > information on -GPIO bindings.
> > -
> > -- interrupts : IRQ line information.
> > -- interrupt-controller
> > -
> > -See
> > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> for -further information on IRQ bindings.
> > -
> > -Sub-nodes:
> > -
> > -- regulators : This node defines the settings for the LDOs and BUCKs.
> > -  The DA9062 regulators are bound using their names listed below:
> > -
> > -    buck1    : BUCK_1
> > -    buck2    : BUCK_2
> > -    buck3    : BUCK_3
> > -    buck4    : BUCK_4
> > -    ldo1     : LDO_1
> > -    ldo2     : LDO_2
> > -    ldo3     : LDO_3
> > -    ldo4     : LDO_4
> > -
> > -  The DA9061 regulators are bound using their names listed below:
> > -
> > -    buck1    : BUCK_1
> > -    buck2    : BUCK_2
> > -    buck3    : BUCK_3
> > -    ldo1     : LDO_1
> > -    ldo2     : LDO_2
> > -    ldo3     : LDO_3
> > -    ldo4     : LDO_4
> > -
> > -  The component follows the standard regulator framework and the
> > bindings
> > -  details of individual regulator device can be found in:
> > -  Documentation/devicetree/bindings/regulator/regulator.txt
> > -
> > -  regulator-initial-mode may be specified for buck regulators using
> > mode values
> > -  from include/dt-bindings/regulator/dlg,da9063-regulator.h.
> > -
> > -- rtc : This node defines settings required for the Real-Time Clock
> > associated
> > -  with the DA9062. There are currently no entries in this binding,
> > however
> > -  compatible = "dlg,da9062-rtc" should be added if a node is created.
> > -
> > -- onkey : See ../input/dlg,da9062-onkey.yaml
> > -
> > -- watchdog: See ../watchdog/dlg,da9062-watchdog.yaml
> > -
> > -- thermal : See ../thermal/dlg,da9062-thermal.yaml
> > -
> > -Example:
> > -
> > -	pmic0: da9062@58 {
> > -		compatible = "dlg,da9062";
> > -		reg = <0x58>;
> > -		interrupt-parent = <&gpio6>;
> > -		interrupts = <11 IRQ_TYPE_LEVEL_LOW>;
> > -		interrupt-controller;
> > -
> > -		rtc {
> > -			compatible = "dlg,da9062-rtc";
> > -		};
> > -
> > -		regulators {
> > -			DA9062_BUCK1: buck1 {
> > -				regulator-name = "BUCK1";
> > -				regulator-min-microvolt = <300000>;
> > -				regulator-max-microvolt = <1570000>;
> > -				regulator-min-microamp = <500000>;
> > -				regulator-max-microamp = <2000000>;
> > -				regulator-initial-mode = <DA9063_BUCK_MODE_SYNC>;
> > -				regulator-boot-on;
> > -			};
> > -			DA9062_LDO1: ldo1 {
> > -				regulator-name = "LDO_1";
> > -				regulator-min-microvolt = <900000>;
> > -				regulator-max-microvolt = <3600000>;
> > -				regulator-boot-on;
> > -			};
> > -		};
> > -	};
> > -
> > diff --git a/Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
> > b/Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
> > index 676b4f2566ae..54bb23dbc73f 100644
> > --- a/Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/dlg,da9063.yaml
> > @@ -4,7 +4,7 @@
> >  $id:
> >
> > -title: Dialog DA9063/DA9063L Power Management Integrated Circuit
> > (PMIC)
> > +title: Dialog DA906{3L,3,2,1} Power Management Integrated Circuit
> > +(PMIC)
> >
> >  maintainers:
> >    - Steve Twiss <stwiss.opensource@xxxxxxxxxxx> @@ -17,10 +17,17 @@
> > description: |
> >    moment where all voltage monitors are disabled. Next, as da9063 only
> supports
> >    UV *and* OV monitoring, both must be set to the same severity and
> value
> >    (0: disable, 1: enable).
> > +  Product information for the DA906{3L,3,2,1} devices can be found
> here:
> > +  -
> >
> >  properties:
> >    compatible:
> >      enum:
> > +      - dlg,da9061
> > +      - dlg,da9062
> >        - dlg,da9063
> >        - dlg,da9063l
> >
> > @@ -35,6 +42,19 @@ properties:
> >    "#interrupt-cells":
> >      const: 2
> >
> > +  gpio:
> 
> Old binding did not have such node and nothing in commit msg explained
> changes from pure conversion.

OK will update the commit message. Check patch complained about undocumented compatible.

> 
> > +    type: object
> > +    $ref: /schemas/gpio/gpio.yaml#
> 
> ?!? Why? First: It's always selected. Second, so you have two gpio
> controllers? And original binding had 0? Why this is not explained at all?
> Open the binding and look at its properties.


I have referred[1] and [2] to add gpio controller property. 


[1]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/nxp/imx/imx6qdl-phytec-phycore-som.dtsi?h=next-20231207#n95

[2]

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mfd/da9062.txt?h=next-20231207#n38

> 
> 
> > +    unevaluatedProperties: false
> > +    properties:
> > +      compatible:
> > +        const: dlg,da9062-gpio
> > +
> > +  gpio-controller: true
> 
> And here is the second gpio-controller...

So you mean it is redundant as $ref: /schemas/gpio/gpio.yaml
has already defined gpio-controller for this object??

> 
> > +
> > +  "#gpio-cells":
> > +    const: 2
> > +
> >    onkey:
> >      $ref: /schemas/input/dlg,da9062-onkey.yaml
> >
> > @@ -42,7 +62,7 @@ properties:
> >      type: object
> >      additionalProperties: false
> >      patternProperties:
> > -      "^(ldo([1-9]|1[01])|bcore([1-2]|s-
> merged)|b(pro|mem|io|peri)|bmem-bio-merged)$":
> > +      "^(ldo([1-9]|1[01])|bcore([1-2]|s-
> merged)|b(pro|mem|io|peri)|bmem-bio-merged|buck[1-4])$":
> >          $ref: /schemas/regulator/regulator.yaml
> >          unevaluatedProperties: false
> >
> > @@ -52,7 +72,12 @@ properties:
> >      unevaluatedProperties: false
> >      properties:
> >        compatible:
> > -        const: dlg,da9063-rtc
> > +        enum:
> > +          - dlg,da9063-rtc
> > +          - dlg,da9062-rtc
> > +
> > +  thermal:
> > +    $ref: /schemas/thermal/dlg,da9062-thermal.yaml
> >
> >    watchdog:
> >      $ref: /schemas/watchdog/dlg,da9062-watchdog.yaml
> > @@ -60,8 +85,65 @@ properties:
> >  required:
> >    - compatible
> >    - reg
> > -  - interrupts
> > -  - interrupt-controller
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - dlg,da9063
> > +              - dlg,da9063l
> > +    then:
> > +      properties:
> > +        thermal: false
> > +        gpio: false
> > +        gpio-controller: false
> > +        "#gpio-cells": false
> > +        regulators:
> > +          patternProperties:
> > +            "^buck[1-4]$": false
> > +      required:
> > +        - interrupts
> > +        - interrupt-controller
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - dlg,da9062
> > +    then:
> > +      properties:
> > +        regulators:
> > +          patternProperties:
> > +            "^(ldo([5-9]|10|11)|bcore([1-2]|s-
> merged)|b(pro|mem|io|peri)|bmem-bio-merged)$": false
> > +      required:
> > +        - gpio
> > +        - onkey
> > +        - rtc
> > +        - thermal
> > +        - watchdog
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - dlg,da9061
> > +    then:
> > +      properties:
> > +        gpio: false
> > +        gpio-controller: false
> > +        "#gpio-cells": false
> > +        regulators:
> > +          patternProperties:
> > +            "^(ldo([5-9]|10|11)|bcore([1-2]|s-
> merged)|b(pro|mem|io|peri)|bmem-bio-merged|buck4)$": false
> > +        rtc: false
> > +      required:
> > +        - onkey
> > +        - thermal
> > +        - watchdog
> >
> >  additionalProperties: false
> >
> > @@ -118,4 +200,98 @@ examples:
> >          };
> >        };
> >      };
> > +
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/regulator/dlg,da9063-regulator.h>
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +      pmic@58 {
> > +        compatible = "dlg,da9062";
> > +        reg = <0x58>;
> > +        #interrupt-cells = <2>;
> > +        interrupt-parent = <&gpio1>;
> > +        interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
> > +        interrupt-controller;
> > +
> > +        gpio {
> > +          compatible = "dlg,da9062-gpio";
> > +          status = "disabled";
> 
> Why?
> Where are gpio-controller and cells? For this node and for parent? Why
> this example is incomplete?

I have used a real example [1] here.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/nxp/imx/imx6qdl-phytec-phycore-som.dtsi?h=next-20231207#n95

Currently I don't see any driver is using this compatible other than MFD.

Cheers,
Biju

> 
> > +        };
> > +
> > +        onkey {
> > +          compatible = "dlg,da9062-onkey";
> > +        };
> 
> 
> Best regards,
> Krzysztof






[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux