Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 7/27/23 00:35, Carsten Spieß wrote:
thanks all for your input.
I conclude here in one answer:

On 7/26/23 18:14, Krzysztof Kozlowski wrote:
It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
sorry i missed that, fixed for [Patch v2].


On 7/26/23 18:14, Krzysztof Kozlowski wrote:
+  shunt-gain:
1. Missing vendor prefix (does not look like generic property)
2. -microvolt
And then enum is for 40, 80, 160 and 320.
replaced with
   renesas,shunt-range-milli-volts:
     description: |
       maximal shunt voltage range of 40mV, 80mV, 160mV or 320mV
     $ref: /schemas/types.yaml#/definitions/uint32-array
     default: 320
     enum: [40, 80, 160, 320]

On 26/07/2023 18:16, Guenter Roeck wrote:
+  shunt-resistor-micro-ohms:
+    description:
+      Shunt resistor value in micro-Ohm
+      defaults to <0> when not set
+      monitoring of current and power not supported when <0>
+
Should not default to 0 (disabled).
When shunt resistor value is not known, current and power calculation can not be done,
but the shunt voltage still can be measured.
As the resistor is externaly connected to the ISL28022 chip there is no default.
When i set default to any value (e.g 1000 micro Ohm), current and power calculation
will give wrong results (except when the shunt _is_ 1000 micro Ohm)


Just like many other hardware monitoring drivers do. That is why /etc/sensors3.conf
exists. That is not a valid argument and not a reason to effectively make the driver
useless on systems where devicetree or the matching ACPI data is not provided.

On 26/07/2023 18:16, Guenter Roeck wrote:
+  average:
+    description: |
+      Number of samples to be used to report voltage, current and power values.
+      defaults to <0> when not set
+    enum: [0, 1, 2, 4, 8, 16, 32, 64, 128]
Should not default to 0 (disabled).
 From datasheet table 6 on page 15 there are two similar ADC modes:
   3 - without samples
   8 - with one? sample
I intended enum 0 to ADC mode 3, 1 to ADC mode 8,
but i can live without ADC mode 3.


Looking at the datasheet, 3 and 8 seem to be identical. There is actually no
"0 samples", which is why I assumed you wanted to use that to disable the sensor
(though, thinking about it, that doesn't really make sense). What is missing
is that 0..3 actually control the sensor accuracy (12..15 bit).

Guenter




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux