Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bccecef7-76d2-6d53-b3e3-20cea9957ae6@linaro.org>
Date:   Tue, 9 May 2023 09:01:52 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCHv2 5/6] dt-bindings: hwmon: intel: add hardware monitor
 bindings for SoCFPGA
Content-Language: en-US
To:     dinh.nguyen@linux.intel.com, linux-hwmon@vger.kernel.org
Cc:     dinguyen@kernel.org, devicetree@vger.kernel.org,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        linux-kernel@vger.kernel.org, jdelvare@suse.com, linux@roeck-us.net
References: <20230508212852.8413-1-dinh.nguyen@linux.intel.com>
 <20230508212852.8413-5-dinh.nguyen@linux.intel.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230508212852.8413-5-dinh.nguyen@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 08/05/2023 23:28, dinh.nguyen@linux.intel.com wrote:
> From: Dinh Nguyen <dinh.nguyen@linux.intel.com>
> 
> Document the hardware monitoring bindings for SoCFPGA 64-bit platforms.
> 
> Signed-off-by: Dinh Nguyen <dinh.nguyen@linux.intel.com>
> ---
> v2: Add intel,socfpga-agilex-hwmon, intel,socfpga-n5x-hwmon and
>     intel,socfpga-stratix10-hwmon
>     Add patternProperties
> ---
>  .../bindings/hwmon/intel,socfpga-hwmon.yaml   | 258 ++++++++++++++++++

Bindings are before they are used in driver.

>  1 file changed, 258 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
> new file mode 100644
> index 000000000000..e634311a0e81
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/intel,socfpga-hwmon.yaml
> @@ -0,0 +1,258 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/intel,socfpga-hwmon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Intel SoCFPGA Hardware monitor
> +
> +maintainers:
> +  - Dinh Nguyen <dinh.nguyen@linux.intel.com>
> +
> +description: |
> +  The Intel SoCFPGA hardware monitor unit provides on-chip voltage and
> +  temperature sensors. You can use these sensors to monitor external
> +  voltages and on-chip operating conditions such as internal power rails
> +  and on-chip junction temperatures.
> +
> +  The specific sensor configurations vary for each device family and
> +  each device within a family does not offer all potential sensor
> +  options. The information below attempts to illustrate the super set of
> +  possible sensor options that are potentially available within each
> +  device family, but the user should check the documentation for the
> +  specific device they are using to verify which sensor options it
> +  actually provides.
> +
> +  Stratix 10 Device Family
> +
> +    Stratix 10 Voltage Sensors
> +
> +      page 0, channel 2 = 0.8V VCC
> +      page 0, channel 3 = 1.0V VCCIO
> +      page 0, channel 6 = 0.9V VCCERAM
> +
> +    Stratix 10 Temperature Sensors
> +
> +      page 0, channel 0 = main die
> +      page 0, channel 1 = tile bottom left
> +      page 0, channel 2 = tile middle left
> +      page 0, channel 3 = tile top left
> +      page 0, channel 4 = tile bottom right
> +      page 0, channel 5 = tile middle right
> +      page 0, channel 6 = tile top right
> +      page 0, channel 7 = hbm2 bottom
> +      page 0, channel 8 = hbm2 top
> +
> +  Agilex Device Family
> +
> +    Agilex Voltage Sensors
> +
> +      page 0, channel 2 = 0.8V VCC
> +      page 0, channel 3 = 1.8V VCCIO_SDM
> +      page 0, channel 4 = 1.8V VCCPT
> +      page 0, channel 5 = 1.2V VCCRCORE
> +      page 0, channel 6 = 0.9V VCCH
> +      page 0, channel 7 = 0.8V VCCL
> +
> +    Agilex Temperature Sensors
> +
> +      page 0, channel 0 = main die sdm max
> +      page 0, channel 1 = main die sdm 1
> +
> +      page 1, channel 0 = main die corner bottom left max
> +      page 1, channel 1 = main die corner bottom left 1
> +      page 1, channel 2 = main die corner bottom left 2
> +
> +      page 2, channel 0 = main die corner top left max
> +      page 2, channel 1 = main die corner top left 1
> +      page 2, channel 2 = main die corner top left 2
> +
> +      page 3, channel 0 = main die corner bottom right max
> +      page 3, channel 1 = main die corner bottom right 1
> +      page 3, channel 2 = main die corner bottom right 2
> +
> +      page 4, channel 0 = main die corner top right max
> +      page 4, channel 1 = main die corner top right 1
> +      page 4, channel 2 = main die corner top right 2
> +
> +      page 5, channel 0 = tile die bottom left max
> +      page 5, channel 1 = tile die bottom left 1
> +      page 5, channel 6..2 = tile die bottom left 6..2 R-tile only
> +      page 5, channel 5..2 = tile die bottom left 5..2 F-tile only
> +      page 5, channel 4..2 = tile die bottom left 4..2 E-tile only
> +
> +      page 7, channel 0 = tile die top left max
> +      page 7, channel 1 = tile die top left 1
> +      page 7, channel 6..2 = tile die top left 6..2 R-tile only
> +      page 7, channel 5..2 = tile die top left 5..2 F-tile only
> +      page 7, channel 4..2 = tile die top left 4..2 E-tile only
> +
> +      page 8, channel 0 = tile die bottom right max
> +      page 8, channel 1 = tile die bottom right 1
> +      page 8, channel 6..2 = tile die bottom right 6..2 R-tile only
> +      page 8, channel 5..2 = tile die bottom right 5..2 F-tile only
> +      page 8, channel 4..2 = tile die bottom right 4..2 E-tile only
> +
> +      page 10, channel 0 = tile die top right max
> +      page 10, channel 1 = tile die top right 1
> +      page 10, channel 6..2 = tile die top right 6..2 R-tile only
> +      page 10, channel 5..2 = tile die top right 5..2 F-tile only
> +      page 10, channel 4..2 = tile die top right 4..2 E-tile only
> +
> +  N5X Device Family
> +
> +    N5X Voltage Sensors
> +
> +      page 0, channel 2 = 0.8V VDD
> +      page 0, channel 3 = 0.8V VDD_SDM
> +      page 0, channel 4 = 1.8V VCCADC
> +      page 0, channel 5 = 1.8V VCCPD
> +      page 0, channel 6 = 1.8V VCCIO_SDM
> +      page 0, channel 7 = 0.8V VDD_HPS
> +
> +    N5X Temperature Sensors
> +
> +      page 0, channel 0 = main die
> +
> +properties:
> +  compatible:
> +    enum:
> +      - intel,socfpga-agilex-hwmon
> +      - intel,socfpga-n5x-hwmon
> +      - intel,socfpga-stratix10-hwmon
> +      - intel,socfpga-hwmon

The last one is too gerneric. Anyway your driver suggests all of them
are compatible.

> +
> +patternProperties:
> +  "^(voltage)|input@([2-7])$":

That's not what your DTS is saying. Are you sure you tested it?

> +    type: object
> +    description: Specifies the possible mappings of the voltage sensors on the
> +      SoCFPGA analog to digital converter of the Secure Device Manager(SDM).

additionalProperties: false

> +
> +    properties:
> +      reg:
> +        description:
> +          The bit mask of 0x1 represents channel 1. The supported
> +          page and channel is dependent on the SoCFPGA variant.
> +          Page number greater than 0 is only supported on the
> +          temperature sensors.
> +        items:
> +          minimum: 0
> +          maximum: 1

voltage does not have reg. also, the minimum/maximum does not fit your
2-7 at all. Entire part should be probably just maxItems, but anyway I
don't really understand what you want to express here. Especially that
it does not match your DTS.

> +
> +      label:
> +        description: A descriptive name for this channel, i.e "0.8V VCC" or "1.8V VCCIO_SDM".
> +
> +  "^(temperature)|input@([10001-40002])$":
> +    type: object
> +    description: Specifies the possible mappings of the temperature sensors on
> +      the SoCFPGA analog to digital converter of the Secure Device Manager(SDM).

additionalProperties: false

> +
> +    properties:
> +      reg:
> +        description:
> +          The sensor mapping address is denoted by the lower 16-bits being
> +          the channel mask location that defines the channel number.
> +          The upper 16-bits denotes the page number.
> +        items:
> +          minimum: 0
> +          maximum: 1

Same problems, does not make any sense. 40002 is higher than 1.

> +
> +      label:
> +        description: A descriptive name for this channel, i.e "Main Die SDM" or
> +          "Main Die corner bottom left 1".
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false


Best regards,
Krzysztof