lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 17 Aug 2020 14:12:11 -0600
From:   Rob Herring <robh@...nel.org>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linuxarm@...wei.com, mauro.chehab@...wei.com,
        Lee Jones <lee.jones@...aro.org>,
        Stephen Boyd <sboyd@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v3 43/44] dt: document HiSilicon SPMI controller and
 mfd/regulator properties

On Mon, Aug 17, 2020 at 09:11:02AM +0200, Mauro Carvalho Chehab wrote:
> Add documentation for the properties needed by the HiSilicon
> 6421v600 driver, and by the SPMI controller used to access
> the chipset.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> ---
>  .../mfd/hisilicon,hi6421-spmi-pmic.yaml       | 182 ++++++++++++++++++
>  .../spmi/hisilicon,hisi-spmi-controller.yaml  |  54 ++++++
>  2 files changed, 236 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
>  create mode 100644 Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
> new file mode 100644
> index 000000000000..95494114554d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
> @@ -0,0 +1,182 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/hisilicon,hi6421-spmi-pmic.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HiSilicon 6421v600 SPMI PMIC
> +
> +maintainers:
> +  - Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> +
> +description: |
> +  HiSilicon 6421v600 uses a MIPI System Power Management (SPMI) bus in order
> +  to provide interrupts and power supply.
> +
> +  The GPIO and interrupt settings are represented as part of the top-level PMIC
> +  node.
> +
> +  The SPMI controller part is provided by
> +  Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml.
> +
> +properties:
> +  $nodename:
> +    pattern: "pmic@[0-9a-f]"
> +
> +  compatible:
> +    const: hisilicon,hi6421-spmi-pmic

-spmi-pmic is redundant. Can the hi6421 be anything else?

> +
> +  reg:
> +    maxItems: 1
> +
> +  spmi-channel:
> +    description: number of the SPMI channel where the PMIC is connected

This looks like a common (to SPMI), but it's not something defined in 
spmi.txt (which should ideally be converted to schema first). Minimally, 
it needs a better explanation and determination if it should be common 
or is HiSilicon specific.

> +
> +  '#interrupt-cells':
> +    const: 2
> +
> +  interrupt-controller:
> +    description:
> +      Identify that the PMIC is capable of behaving as an interrupt controller.

No need to redefine common properties if nothing specific to this device 
to say. Just:

interrupt-controller: true

> +
> +  gpios:
> +    maxItems: 1
> +
> +  irq-num:
> +    description: Interrupt request number
> +
> +  'irq-array':
> +    description: Interrupt request array
> +
> +  'irq-mask-addr':
> +    description: Address for the interrupt request mask
> +
> +  'irq-addr':
> +    description: Address for the interrupt request

What's all these non-standard interrupt properties?

> +
> +  regulators:
> +    type: object

additionalProperties: false

> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      '^ldo@[0-9]+$':

Unit-addresses are hex.

Also, doesn't match the example.

> +        type: object
> +
> +        $ref: "/schemas/regulator/regulator.yaml#"
> +
> +        properties:
> +          reg:
> +            description: Enable register.
> +

> +          '#address-cells':
> +            const: 1
> +
> +          '#size-cells':
> +            const: 0

No child nodes, you don't need these.

> +
> +          vsel-reg:
> +            description: Voltage selector register.

'reg' can have multiple entries if you want.

> +
> +          enable-mask:
> +            description: Bitmask used to enable the regulator.

But if there's a shared enable reg, then you shouldn't have duplicate 
addresses (same 'reg' value in multiple nodes).

These perhaps should be driver data rather than in DT as it's all fixed 
for this chip. We don't try to parameterize everything in DT.

> +
> +          voltage-table:
> +            description: Table with the selector items for the voltage regulator.
> +            minItems: 2
> +            maxItems: 16

Needs a type $ref.

> +
> +          off-on-delay-us:
> +            description: Time required for changing state to enabled in microseconds.
> +
> +          startup-delay-us:
> +            description: Startup time in microseconds.
> +
> +          idle-mode-mask:
> +            description: Bitmask used to put the regulator on idle mode.
> +
> +          eco-microamp:
> +            description: Maximum current while on idle mode.
> +
> +        required:
> +          - reg
> +          - vsel-reg
> +          - enable-mask
> +          - voltage-table
> +          - off-on-delay-us
> +          - startup-delay-us
> +
> +required:
> +  - compatible
> +  - reg
> +  - regulators

Add:

additionalProperties: false

> +
> +examples:
> +  - |
> +    /* pmic properties */
> +
> +    pmic: pmic@0 {
> +      compatible = "hisilicon,hi6421-spmi-pmic";
> +      slave_id = <0>;

Not documented. I believe this is part of 'reg'.

> +      reg = <0 0>;
> +
> +      #interrupt-cells = <2>;
> +      interrupt-controller;
> +      gpios = <&gpio28 0 0>;
> +      irq-num = <16>;
> +      irq-array = <2>;
> +      irq-mask-addr = <0x202 2>;
> +      irq-addr = <0x212 2>;
> +
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      regulators {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +        ldo3: ldo3@16 {
> +          reg = <0x16>;
> +          vsel-reg = <0x51>;
> +
> +          regulator-name = "ldo3";
> +          regulator-min-microvolt = <1500000>;
> +          regulator-max-microvolt = <2000000>;
> +          regulator-boot-on;
> +
> +          enable-mask = <0x01>;
> +
> +          voltage-table = <1500000>, <1550000>, <1600000>, <1650000>,
> +                          <1700000>, <1725000>, <1750000>, <1775000>,
> +                          <1800000>, <1825000>, <1850000>, <1875000>,
> +                          <1900000>, <1925000>, <1950000>, <2000000>;
> +          off-on-delay-us = <20000>;
> +          startup-delay-us = <120>;
> +        };
> +
> +        ldo4: ldo4@17 { /* 40 PIN */
> +          reg = <0x17>;
> +          vsel-reg = <0x52>;
> +
> +          regulator-name = "ldo4";
> +          regulator-min-microvolt = <1725000>;
> +          regulator-max-microvolt = <1900000>;
> +          regulator-boot-on;
> +
> +          enable-mask = <0x01>;
> +          idle-mode-mask = <0x10>;
> +          eco-microamp = <10000>;
> +
> +          hi6421-vsel = <0x52 0x07>;

Not documented.

> +          voltage-table = <1725000>, <1750000>, <1775000>, <1800000>,
> +                          <1825000>, <1850000>, <1875000>, <1900000>;
> +          off-on-delay-us = <20000>;
> +          startup-delay-us = <120>;
> +        };
> +      };
> +    };
> diff --git a/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml b/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml
> new file mode 100644
> index 000000000000..5aeb2ae12024
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spmi/hisilicon,hisi-spmi-controller.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spmi/hisilicon,hisi-spmi-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: HiSilicon SPMI controller
> +
> +maintainers:
> +  - Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> +
> +description: |
> +  The HiSilicon SPMI controller is found on some Kirin-based designs.
> +  It is a MIPI System Power Management (SPMI) controller.
> +
> +  The PMIC part is provided by
> +  Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml.
> +
> +properties:
> +  $nodename:
> +    pattern: "spmi@[0-9a-f]"
> +
> +  compatible:
> +    const: hisilicon,spmi-controller

Needs an SoC specific compatible.

> +
> +  reg:
> +    maxItems: 1
> +
> +  "#address-cells":
> +    const: 2
> +
> +  "#size-cells":
> +    const: 0
> +
> +  spmi-channel:
> +    description: number of the SPMI channel where the PMIC is connected
> +
> +patternProperties:
> +  "^pmic@[0-9a-f]$":
> +    $ref: "/schemas/mfd/hisilicon,hi6421-spmi-pmic.yaml#"
> +
> +examples:
> +  - |
> +    spmi: spmi@...24000 {
> +      compatible = "hisilicon,spmi-controller";
> +      #address-cells = <2>;
> +      #size-cells = <0>;
> +      status = "ok";
> +      reg = <0x0 0xfff24000 0x0 0x1000>;
> +      spmi-channel = <2>;

Does this go in the SPMI controller or child (pmic)?

> +
> +      /* pmic properties */
> +
> +    };
> -- 
> 2.26.2
> 

Powered by blists - more mailing lists