diff mbox series

[v1,3/6] dt-bindings: hwmon: Add hpe,gxp-fan-ctrl

Message ID 20221104193657.105130-4-nick.hawkins@hpe.com
State New
Headers
Series ARM: Add GXP Fan and SPI controllers |

Commit Message

Hawkins, Nick Nov. 4, 2022, 7:36 p.m. UTC 
  From: Nick Hawkins <nick.hawkins@hpe.com>

This provides the base registers address, programmable logic registers
address, and the function 2 registers to allow control access of the HPE
fans on the GXP SoC.

Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
---
 .../bindings/hwmon/hpe,gxp-fan-ctrl.yaml      | 41 +++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml

Comments

Krzysztof Kozlowski Nov. 6, 2022, 10:38 a.m. UTC | #1 
On 04/11/2022 20:36, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> This provides the base registers address, programmable logic registers
> address, and the function 2 registers to allow control access of the HPE
> fans on the GXP SoC.

What is "This"? If "This patch", then drop it.
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

If "This hardware" then please instead describe the hardware, not it
components. What are its features? If it controls the fan, then why
there are no PWM-related cells? How do you set the speed?

> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>  .../bindings/hwmon/hpe,gxp-fan-ctrl.yaml      | 41 +++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml b/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
> new file mode 100644
> index 000000000000..40a5d9cd0a30
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/hpe,gxp-fan-ctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GXP Fan Controller
> +
> +maintainers:
> +  - Nick Hawkins <nick.hawkins@hpe.com>
> +
> +properties:
> +  compatible:
> +    const: hpe,gxp-fan-ctrl
> +
> +  reg:
> +    items:
> +      - description: Fan controller base register
> +      - description: Programmable logic registers base
> +      - description: Function 2 registers base

Drop "register" and "base" from all descriptions

> +
> +  reg-names:
> +    items:
> +      - const: base
> +      - const: plreg

Drop reg suffix

> +      - const: fn2reg

Drop reg suffix

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    fanctrl@1000c00 {

fan-controller

> +      compatible = "hpe,gxp-fan-ctrl";
> +      reg = <0x1000c00 0x200>, <0xd1000000 0xff>, <0x80200000 0x100000>;
> +      reg-names = "base", "plreg", "fn2reg";
> +    };

Best regards,
Krzysztof
Hawkins, Nick Nov. 7, 2022, 10:36 p.m. UTC | #2 
> > This provides the base registers address, programmable logic registers
    > > address, and the function 2 registers to allow control access of the HPE
    > > fans on the GXP SoC.

    > What is "This"? If "This patch", then drop it.
    > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

    > If "This hardware" then please instead describe the hardware, not it
    components. What are its features? If it controls the fan, then why
    there are no PWM-related cells? How do you set the speed?

Greetings Krzysztof,

    Thank you for the feedback. The intention was this binding.. however, that was an error on my part, and I will correct it to reflect the hardware situation of the GXP with the fan controller and how each of the mapped registers provide control to the system. To answer your questions: The fans speeds are controlled through an external CPLD device which we provide a PWM value (0-255) using the "base" register to the CIF interface. This interface provides access to the CPLD. The CPLD then drives the fan. The CPLD can generate up to 8 unique different PWMs to multiple fans. The CPLD monitors the fans and reports the status back to the SoC through the CIF interface to the "plreg base". The plreg includes the installation, failed, and identification statuses. The function 2 register base is used to check the power state of the system as that influences the PWM values read back.

As the PWM generation happens outside the SoC do we still need pwm-cells? If so, should we have a custom compatible for that?

Thanks,

-Nick
Krzysztof Kozlowski Nov. 8, 2022, 11:22 a.m. UTC | #3 
On 07/11/2022 23:36, Hawkins, Nick wrote:
> 
>     > > This provides the base registers address, programmable logic registers
>     > > address, and the function 2 registers to allow control access of the HPE
>     > > fans on the GXP SoC.
> 
>     > What is "This"? If "This patch", then drop it.
>     > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> 
>     > If "This hardware" then please instead describe the hardware, not it
>     components. What are its features? If it controls the fan, then why
>     there are no PWM-related cells? How do you set the speed?
> 
> Greetings Krzysztof,
> 
>     Thank you for the feedback. The intention was this binding.. however, that was an error on my part, and I will correct it to reflect the hardware situation of the GXP with the fan controller and how each of the mapped registers provide control to the system. To answer your questions: The fans speeds are controlled through an external CPLD device which we provide a PWM value (0-255) using the "base" register to the CIF interface. 

Wrap your emails, it's impossible to simply reply to it.

Then your CIF interface is a PWM device?


> This interface provides access to the CPLD. The CPLD then drives the fan. The CPLD can generate up to 8 unique different PWMs to multiple fans. 

So you have other CPLD (not external) which generates PWM based on first
CPLD base register? Hm, I think it's one CPLD.

> The CPLD monitors the fans and reports the status back to the SoC through the CIF interface to the "plreg base". The plreg includes the installation, failed, and identification statuses. The function 2 register base is used to check the power state of the system as that influences the PWM values read back.

> As the PWM generation happens outside the SoC do we still need pwm-cells? If so, should we have a custom compatible for that?
> 

Depends, if these are actually tightly coupled and you cannot use PWM
for anything else, then you do not need.

> Thanks,
> 
> -Nick
> 
> 

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml b/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
new file mode 100644
index 000000000000..40a5d9cd0a30
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
@@ -0,0 +1,41 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/hpe,gxp-fan-ctrl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GXP Fan Controller
+
+maintainers:
+  - Nick Hawkins <nick.hawkins@hpe.com>
+
+properties:
+  compatible:
+    const: hpe,gxp-fan-ctrl
+
+  reg:
+    items:
+      - description: Fan controller base register
+      - description: Programmable logic registers base
+      - description: Function 2 registers base
+
+  reg-names:
+    items:
+      - const: base
+      - const: plreg
+      - const: fn2reg
+
+required:
+  - compatible
+  - reg
+  - reg-names
+
+additionalProperties: false
+
+examples:
+  - |
+    fanctrl@1000c00 {
+      compatible = "hpe,gxp-fan-ctrl";
+      reg = <0x1000c00 0x200>, <0xd1000000 0xff>, <0x80200000 0x100000>;
+      reg-names = "base", "plreg", "fn2reg";
+    };