Re: Re: [PATCH v1 1/2] dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor

["Huan He" <hehuan1@eswincomputing.com>]

> > 
> > Add device tree binding documentation for ESWIN EIC7700 Process, Voltage
> > and Temperature sensor.
> > 
> > The EIC7700 SoC integrates two PVT instances for monitoring SoC and DDR
> > power domains respectively.
> > 
> > Signed-off-by: Yulin Lu <luyulin@eswincomputing.com>
> > Signed-off-by: Huan He <hehuan1@eswincomputing.com>
> > ---
> >  .../bindings/hwmon/eswin,eic7700-pvt.yaml     | 92 +++++++++++++++++++
> >  1 file changed, 92 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml b/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
> > new file mode 100644
> > index 000000000000..72693828a461
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
> > @@ -0,0 +1,92 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/eswin,eic7700-pvt.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ESWIN EIC7700 PVT Sensor
> > +
> > +maintainers:
> > +  - Yulin Lu <luyulin@eswincomputing.com>
> > +  - Huan He <hehuan1@eswincomputing.com>
> > +
> > +description:
> > +  ESWIN EIC7700 SoC integrates embedded process, voltage and temperature
> > +  sensors to monitor the internal SoC environment. The system includes two
> > +  PVT sensor instances. The PVT0 monitors the main SoC power domain. The
> > +  PVT1 sensor monitors the DDR core power domain.
> > +
> 
> You miss ref to hwmon-common
> 
> > +properties:
> > +  compatible:
> > +    const: eswin,eic7700-pvt
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: clk
> 
> Drop clock-names, useless. There is little sense in calling a clock
> "clock".
> 
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  label:
> > +    description:
> > +      Human readable identifier used to distinguish between different PVT
> > +      instances and generate descriptive sensor labels. Typically "pvt0"
> > +      for SoC PVT sensor and "pvt1" for DDR core PVT sensor.
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  reset-names:
> > +    items:
> > +      - const: rst
> 
> Same problem.
> 
> 
> > +
> > +  '#thermal-sensor-cells':
> > +    description: Thermal sensor cells if used for thermal sensoring.
> > +    const: 0
> > +
> > +additionalProperties: false
> 
> This goes after required block.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - label
> > +  - resets
> > +  - reset-names
> > +
> > +examples:
> > +  - |
> > +    pvt@50b00000 {
> > +      compatible = "eswin,eic7700-pvt";
> > +      reg = <0x50b00000 0x10000>;
> > +      clocks = <&clocks 244>;
> > +      clock-names = "clk";
> > +      interrupts = <349>;
> > +      interrupt-parent = <&plic>;
> > +      label = "pvt0";
> > +      resets = <&reset 111>;
> > +      reset-names = "rst";
> > +      #thermal-sensor-cells = <0>;
> > +    };
> > +
> > +    pvt@52360000 {
> > +      compatible = "eswin,eic7700-pvt";
> 
> Drop entire node, one example is enough. Do not grow your binding with
> redundant data. We are not accepting commits based on number of lines.
> 

Thank you very much for taking the time to review the patch and for your
valuable feedback.
All these issues will be addressed in the next patch.

Best regards,
Huan He