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

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

[Huan He]

Hi Sashiko,

Thank you very much for your detailed review. We appreciate the feedback.

> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The 'label' property is required and restricted to an enum, misusing it for hardware instance identification.
> - [Medium] The '#thermal-sensor-cells' property is defined but omitted from the 'required' list.
> --
> 
> commit 5a5ed34d17ea4e88cd80b5a8e96e21dcc65e9f11
> Author: Huan He <hehuan1@eswincomputing.com>
> 
> dt-bindings: hwmon: Add Eswin EIC7700 PVT sensor
> 
> This commit adds the device tree binding documentation for the Eswin
> EIC7700 Process, Voltage and Temperature sensor. The SoC integrates two
> PVT instances for monitoring the main SoC and DDR power domains.
> 
> > 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 0000000000000..ba89b829ee947
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/eswin,eic7700-pvt.yaml
> 
> [ ... ]
> 
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  label:
> > +    enum:
> > +      - pvt0
> > +      - pvt1
> 
> Is it appropriate to restrict the standard label property to a strict enum
> and require it? 
> 
> The devicetree specification generally defines label as an optional,
> human-readable string. Should the hardware instances be differentiated by
> their reg addresses, aliases, or dedicated properties instead of imposing
> a strict enum on a standard free-form string?

I am planning to update the next revision as follows. Would this be
acceptable?

YAML:
-  label:
-    enum:
-      - pvt0
-      - pvt1
+  label: true

required:
  - compatible
  - reg
  - clocks
  - interrupts
- - label

Driver:
 static int eic7700_pvt_create_hwmon(struct pvt_hwmon *pvt)
 {
-   struct device *dev = pvt->dev;
-   struct device_node *np = dev->of_node;
-   const char *node_label;
-   int type;
-   const char *names[2] = {"soc_pvt", "ddr_pvt"};
-
-   if (of_property_read_string(np, "label", &node_label)) {
-       dev_err(dev, "Missing 'label' property in DTS node\n");
-       return -EINVAL;
-   }
-
-   if (strcmp(node_label, "pvt0") == 0) {
-       type = 0;
-   } else if (strcmp(node_label, "pvt1") == 0) {
-       type = 1;
-   } else {
-       dev_err(pvt->dev, "Unsupported label: %s\n", node_label);
-       return -EINVAL;
-   }
+   const char *name = "pvt";
+
+   of_property_read_string(pvt->dev->of_node, "label", &name);
 
-   pvt->hwmon = devm_hwmon_device_register_with_info(pvt->dev, names[type],
+   pvt->hwmon = devm_hwmon_device_register_with_info(pvt->dev, name,
                              pvt, &pvt_hwmon_info,
                              NULL);

> 
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  '#thermal-sensor-cells':
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - interrupts
> > +  - label
> > +  - resets
> 
> Should #thermal-sensor-cells be included in this required list?
> 
> Since the device acts as a thermal sensor provider, omitting its cell-count
> property allows incomplete device tree nodes to silently pass schema
> validation. This might lead to runtime failures or misconfigurations when
> dependent thermal zones attempt to reference the sensor.

I will add "#thermal-sensor-cells" to the required list in the next
revision.

Best regards,
Huan He