* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-26 15:22 ` [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022 Carsten Spieß
@ 2023-07-26 16:14 ` Krzysztof Kozlowski
2023-07-26 16:19 ` Krzysztof Kozlowski
2023-07-26 18:19 ` Guenter Roeck
2023-07-26 16:16 ` Guenter Roeck
2023-07-26 17:13 ` Rob Herring
2 siblings, 2 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-26 16:14 UTC (permalink / raw)
To: Carsten Spieß, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm
Cc: linux-hwmon, devicetree, linux-kernel, linux-renesas-soc
On 26/07/2023 17:22, Carsten Spieß wrote:
> Add dt-bindings for Renesase ISL28022 power monitor.
>
> Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>
> ---
Thank you for your patch. There is something to discuss/improve.
> .../bindings/hwmon/renesas,isl28022.yaml | 67 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 68 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
> new file mode 100644
> index 000000000000..5ecf892db269
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
No blank line.
> +$id: http://devicetree.org/schemas/hwmon/renesas,isl28022.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas ISL28022 power monitor
> +
> +maintainers:
> + - Carsten Spieß <mail@carsten-spiess.de>
> +
> +description: |
> + The ISL28022 is a power monitor with I2C interface. The device monitors
> + voltage, current via shunt resistor and calculated power.
> +
> + Datasheets:
> + https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
> +
> +Required properties:
> + compatible:
> + enum:
> + - renesas,isl28022
> +
> + reg:
> + maxItems: 1
> +
> +Optional properties:
It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.
These are "properties"
> + 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>
> +
> + shunt-gain:
1. Missing vendor prefix (does not look like generic property)
2. -microvolt
> + description:
> + Shunt gain to scale maximal shunt voltage to
> + 40mV, 80mV, 160mV, 320mV
> + defaults to <8> (320mV) when not set
And then enum is for 40, 80, 160 and 320.
> + enum: [1, 2, 4, 8]
> +
> + 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]
I am sure hwmon has some property for this. Are you sure it is called
"average"?
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-26 16:14 ` Krzysztof Kozlowski
@ 2023-07-26 16:19 ` Krzysztof Kozlowski
2023-07-26 18:19 ` Guenter Roeck
1 sibling, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-26 16:19 UTC (permalink / raw)
To: Carsten Spieß, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm
Cc: linux-hwmon, devicetree, linux-kernel, linux-renesas-soc
On 26/07/2023 18:14, Krzysztof Kozlowski 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>
>> +
>> + shunt-gain:
>
> 1. Missing vendor prefix (does not look like generic property)
> 2. -microvolt
>
>> + description:
>> + Shunt gain to scale maximal shunt voltage to
>> + 40mV, 80mV, 160mV, 320mV
>> + defaults to <8> (320mV) when not set
>
> And then enum is for 40, 80, 160 and 320.
Also use "default: X" to choose default, instead of free-form text.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-26 16:14 ` Krzysztof Kozlowski
2023-07-26 16:19 ` Krzysztof Kozlowski
@ 2023-07-26 18:19 ` Guenter Roeck
2023-07-26 18:27 ` Krzysztof Kozlowski
1 sibling, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2023-07-26 18:19 UTC (permalink / raw)
To: Krzysztof Kozlowski, Carsten Spieß, Jean Delvare,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm
Cc: linux-hwmon, devicetree, linux-kernel, linux-renesas-soc
On 7/26/23 09:14, Krzysztof Kozlowski wrote:
> On 26/07/2023 17:22, Carsten Spieß wrote:
>> Add dt-bindings for Renesase ISL28022 power monitor.
>>
>> Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>
>> ---
>
> Thank you for your patch. There is something to discuss/improve.
>
>
>> .../bindings/hwmon/renesas,isl28022.yaml | 67 +++++++++++++++++++
>> MAINTAINERS | 1 +
>> 2 files changed, 68 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
>> new file mode 100644
>> index 000000000000..5ecf892db269
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
>> @@ -0,0 +1,67 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +
>
> No blank line.
>
>> +$id: http://devicetree.org/schemas/hwmon/renesas,isl28022.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Renesas ISL28022 power monitor
>> +
>> +maintainers:
>> + - Carsten Spieß <mail@carsten-spiess.de>
>> +
>> +description: |
>> + The ISL28022 is a power monitor with I2C interface. The device monitors
>> + voltage, current via shunt resistor and calculated power.
>> +
>> + Datasheets:
>> + https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
>> +
>> +Required properties:
>> + compatible:
>> + enum:
>> + - renesas,isl28022
>> +
>> + reg:
>> + maxItems: 1
>> +
>> +Optional properties:
>
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
>
> These are "properties"
>
>> + 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>
>> +
>> + shunt-gain:
>
> 1. Missing vendor prefix (does not look like generic property)
> 2. -microvolt
>
>> + description:
>> + Shunt gain to scale maximal shunt voltage to
>> + 40mV, 80mV, 160mV, 320mV
>> + defaults to <8> (320mV) when not set
>
> And then enum is for 40, 80, 160 and 320.
>
>> + enum: [1, 2, 4, 8]
>> +
>> + 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]
>
> I am sure hwmon has some property for this. Are you sure it is called
> "average"?
>
Something with samples. adi,power-sample-average is similar. Others
use average-samples, qcom,avg-samples, touchscreen-average-samples.
Guenter
>> +
>> +required:
>> + - compatible
>> + - reg
>> +
>> +additionalProperties: false
>> +
>
>
> Best regards,
> Krzysztof
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-26 18:19 ` Guenter Roeck
@ 2023-07-26 18:27 ` Krzysztof Kozlowski
2023-07-26 18:59 ` Guenter Roeck
0 siblings, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-26 18:27 UTC (permalink / raw)
To: Guenter Roeck, Carsten Spieß, Jean Delvare, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm
Cc: linux-hwmon, devicetree, linux-kernel, linux-renesas-soc
On 26/07/2023 20:19, Guenter Roeck wrote:
>>
>>> + enum: [1, 2, 4, 8]
>>> +
>>> + 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]
>>
>> I am sure hwmon has some property for this. Are you sure it is called
>> "average"?
>>
>
> Something with samples. adi,power-sample-average is similar. Others
> use average-samples, qcom,avg-samples, touchscreen-average-samples.
So probably it's a time to come with something generic, e.g.:
average-samples in some hwmon.yaml
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-26 18:27 ` Krzysztof Kozlowski
@ 2023-07-26 18:59 ` Guenter Roeck
2023-07-26 19:05 ` Krzysztof Kozlowski
0 siblings, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2023-07-26 18:59 UTC (permalink / raw)
To: Krzysztof Kozlowski, Carsten Spieß, Jean Delvare,
Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Geert Uytterhoeven, Magnus Damm
Cc: linux-hwmon, devicetree, linux-kernel, linux-renesas-soc
On 7/26/23 11:27, Krzysztof Kozlowski wrote:
> On 26/07/2023 20:19, Guenter Roeck wrote:
>>>
>>>> + enum: [1, 2, 4, 8]
>>>> +
>>>> + 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]
>>>
>>> I am sure hwmon has some property for this. Are you sure it is called
>>> "average"?
>>>
>>
>> Something with samples. adi,power-sample-average is similar. Others
>> use average-samples, qcom,avg-samples, touchscreen-average-samples.
>
> So probably it's a time to come with something generic, e.g.:
> average-samples in some hwmon.yaml
>
Maybe, but that is going to take a lot of time. Many configuration options
of hardware monitoring chips (such as the thermal sensor type, the ideality
factor of a thermal diode, or the number of pulses per revolution reported
by fans) are actually properties of the connected device, and Rob has made
clear that he expects such properties to be associated with the actual
device (fan, thermal sensor, etc). I don't even know how to model that,
and I really don't have the time (nor, really, the expertise) to come up
with acceptable generic bindings.
Guenter
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-26 18:59 ` Guenter Roeck
@ 2023-07-26 19:05 ` Krzysztof Kozlowski
0 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-26 19:05 UTC (permalink / raw)
To: Guenter Roeck, Carsten Spieß, Jean Delvare, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm
Cc: linux-hwmon, devicetree, linux-kernel, linux-renesas-soc
On 26/07/2023 20:59, Guenter Roeck wrote:
> On 7/26/23 11:27, Krzysztof Kozlowski wrote:
>> On 26/07/2023 20:19, Guenter Roeck wrote:
>>>>
>>>>> + enum: [1, 2, 4, 8]
>>>>> +
>>>>> + 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]
>>>>
>>>> I am sure hwmon has some property for this. Are you sure it is called
>>>> "average"?
>>>>
>>>
>>> Something with samples. adi,power-sample-average is similar. Others
>>> use average-samples, qcom,avg-samples, touchscreen-average-samples.
>>
>> So probably it's a time to come with something generic, e.g.:
>> average-samples in some hwmon.yaml
>>
>
> Maybe, but that is going to take a lot of time. Many configuration options
> of hardware monitoring chips (such as the thermal sensor type, the ideality
> factor of a thermal diode, or the number of pulses per revolution reported
> by fans) are actually properties of the connected device, and Rob has made
> clear that he expects such properties to be associated with the actual
> device (fan, thermal sensor, etc).
Sure, we can go then with usual vendor like property, so:
renesas,average-samples
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-26 15:22 ` [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022 Carsten Spieß
2023-07-26 16:14 ` Krzysztof Kozlowski
@ 2023-07-26 16:16 ` Guenter Roeck
2023-07-27 7:35 ` Carsten Spieß
2023-07-26 17:13 ` Rob Herring
2 siblings, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2023-07-26 16:16 UTC (permalink / raw)
To: Carsten Spieß, Jean Delvare, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm
Cc: linux-hwmon, devicetree, linux-kernel, linux-renesas-soc
On 7/26/23 08:22, Carsten Spieß wrote:
> Add dt-bindings for Renesase ISL28022 power monitor.
>
> Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>
> ---
> .../bindings/hwmon/renesas,isl28022.yaml | 67 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 68 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
> new file mode 100644
> index 000000000000..5ecf892db269
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/hwmon/renesas,isl28022.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas ISL28022 power monitor
> +
> +maintainers:
> + - Carsten Spieß <mail@carsten-spiess.de>
> +
> +description: |
> + The ISL28022 is a power monitor with I2C interface. The device monitors
> + voltage, current via shunt resistor and calculated power.
> +
> + Datasheets:
> + https://www.renesas.com/us/en/www/doc/datasheet/isl28022.pdf
> +
> +Required properties:
> + compatible:
> + enum:
> + - renesas,isl28022
> +
> + reg:
> + maxItems: 1
> +
> +Optional properties:
> + 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).
> + shunt-gain:
> + description:
> + Shunt gain to scale maximal shunt voltage to
> + 40mV, 80mV, 160mV, 320mV
> + defaults to <8> (320mV) when not set
> + enum: [1, 2, 4, 8]
> +
> + 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).
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + power-monitor@40 {
> + compatible = "renesas,isl28022";
> + reg = <0x10>;
> + shunt-resistor-micro-ohms = <8000>;
> + shunt-gain = <1>;
> + average = <128>;
> + };
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c61aa688cd11..ec9b97ace50b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11076,6 +11076,7 @@ ISL28022 HARDWARE MONITORING DRIVER
> M: Carsten Spieß <mail@carsten-spiess.de>
> L: linux-hwmon@vger.kernel.org
> S: Maintained
> +F: Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
> F: Documentation/hwmon/isl28022.rst
> F: drivers/hwmon/isl28022.c
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-26 16:16 ` Guenter Roeck
@ 2023-07-27 7:35 ` Carsten Spieß
2023-07-27 8:07 ` Conor Dooley
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Carsten Spieß @ 2023-07-27 7:35 UTC (permalink / raw)
To: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Geert Uytterhoeven, Magnus Damm
Cc: linux-hwmon, devicetree, linux-kernel, linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 3002 bytes --]
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)
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.
On 26/07/2023 20:59, Guenter Roeck wrote:
> On 7/26/23 11:27, Krzysztof Kozlowski wrote:
>> On 26/07/2023 20:19, Guenter Roeck wrote:
>>>>> + average:
>>>>> + enum: [0, 1, 2, 4, 8, 16, 32, 64, 128]
>>>>
>>>> I am sure hwmon has some property for this. Are you sure it is called
>>>> "average"?
>>>>
>>>
>>> Something with samples. adi,power-sample-average is similar. Others
>>> use average-samples, qcom,avg-samples, touchscreen-average-samples.
>>
>> So probably it's a time to come with something generic, e.g.:
>> average-samples in some hwmon.yaml
As there's no generic yet, replaced with
renesas,average-samples:
description: |
Number of samples to be used to report voltage, current and power values.
default: 1
$ref: /schemas/types.yaml#/definitions/uint32-array
enum: [1, 2, 4, 8, 16, 32, 64, 128]
On 26/07/2023 19:13, Rob Herring wrote:
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
Fixed for [Patch v2], runs without errors or warnings
Regards, Carsten
[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-27 7:35 ` Carsten Spieß
@ 2023-07-27 8:07 ` Conor Dooley
2023-07-27 8:25 ` Krzysztof Kozlowski
2023-07-27 13:55 ` Guenter Roeck
2 siblings, 0 replies; 15+ messages in thread
From: Conor Dooley @ 2023-07-27 8:07 UTC (permalink / raw)
To: Carsten Spieß
Cc: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Geert Uytterhoeven, Magnus Damm, linux-hwmon,
devicetree, linux-kernel, linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 725 bytes --]
On Thu, Jul 27, 2023 at 09:35:28AM +0200, Carsten Spieß wrote:
> 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]
The point of using -microvolt is that it is a standard suffix:
https://github.com/devicetree-org/dt-schema/blob/c1e73ebea9fc07f7d7848f304301f755f1278a67/dtschema/schemas/property-units.yaml#L101
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-27 7:35 ` Carsten Spieß
2023-07-27 8:07 ` Conor Dooley
@ 2023-07-27 8:25 ` Krzysztof Kozlowski
2023-07-27 9:30 ` Carsten Spieß
2023-07-27 13:55 ` Guenter Roeck
2 siblings, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-27 8:25 UTC (permalink / raw)
To: Carsten Spieß, Guenter Roeck, Jean Delvare, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm
Cc: linux-hwmon, devicetree, linux-kernel, linux-renesas-soc
On 27/07/2023 09: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:
That's not correct unit in DT, so again please use -microvolt:
https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
> description: |
> maximal shunt voltage range of 40mV, 80mV, 160mV or 320mV
> $ref: /schemas/types.yaml#/definitions/uint32-array
And then drop this line.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-27 8:25 ` Krzysztof Kozlowski
@ 2023-07-27 9:30 ` Carsten Spieß
0 siblings, 0 replies; 15+ messages in thread
From: Carsten Spieß @ 2023-07-27 9:30 UTC (permalink / raw)
To: Krzysztof Kozlowski, Guenter Roeck, Jean Delvare, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm
Cc: linux-hwmon, devicetree, linux-kernel, linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 543 bytes --]
On 7/27/23 10:25, Krzysztof Kozlowski wrote:
> > replaced with
> > renesas,shunt-range-milli-volts:
>
> That's not correct unit in DT, so again please use -microvolt:
> https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml
a bit unhandy but o.k., applied to [Patch v2]
> > description: |
> > maximal shunt voltage range of 40mV, 80mV, 160mV or 320mV
> > $ref: /schemas/types.yaml#/definitions/uint32-array
>
> And then drop this line.
Applied.
Regards, Carsten
[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-27 7:35 ` Carsten Spieß
2023-07-27 8:07 ` Conor Dooley
2023-07-27 8:25 ` Krzysztof Kozlowski
@ 2023-07-27 13:55 ` Guenter Roeck
2 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2023-07-27 13:55 UTC (permalink / raw)
To: Carsten Spieß, Jean Delvare, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Geert Uytterhoeven,
Magnus Damm
Cc: linux-hwmon, devicetree, linux-kernel, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022
2023-07-26 15:22 ` [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022 Carsten Spieß
2023-07-26 16:14 ` Krzysztof Kozlowski
2023-07-26 16:16 ` Guenter Roeck
@ 2023-07-26 17:13 ` Rob Herring
2 siblings, 0 replies; 15+ messages in thread
From: Rob Herring @ 2023-07-26 17:13 UTC (permalink / raw)
To: Carsten Spieß
Cc: linux-hwmon, Krzysztof Kozlowski, devicetree, Magnus Damm,
Jean Delvare, Guenter Roeck, linux-kernel, Rob Herring,
Geert Uytterhoeven, linux-renesas-soc, Conor Dooley
On Wed, 26 Jul 2023 17:22:34 +0200, Carsten Spieß wrote:
> Add dt-bindings for Renesase ISL28022 power monitor.
>
> Signed-off-by: Carsten Spieß <mail@carsten-spiess.de>
> ---
> .../bindings/hwmon/renesas,isl28022.yaml | 67 +++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 68 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml: 'anyOf' conditional failed, one must be fixed:
'type' is a required property
'properties' is a required property
'patternProperties' is a required property
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml: 'Required properties' is not one of ['$id', '$schema', 'title', 'description', 'examples', 'required', 'allOf', 'anyOf', 'oneOf', 'definitions', '$defs', 'additionalProperties', 'dependencies', 'dependentRequired', 'dependentSchemas', 'patternProperties', 'properties', 'not', 'if', 'then', 'else', 'unevaluatedProperties', 'deprecated', 'maintainers', 'select', '$ref']
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml: 'Optional properties' is not one of ['$id', '$schema', 'title', 'description', 'examples', 'required', 'allOf', 'anyOf', 'oneOf', 'definitions', '$defs', 'additionalProperties', 'dependencies', 'dependentRequired', 'dependentSchemas', 'patternProperties', 'properties', 'not', 'if', 'then', 'else', 'unevaluatedProperties', 'deprecated', 'maintainers', 'select', '$ref']
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/hwmon/renesas,isl28022.yaml: 'anyOf' conditional failed, one must be fixed:
'properties' is a required property
'patternProperties' is a required property
hint: Metaschema for devicetree binding documentation
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
Documentation/devicetree/bindings/hwmon/renesas,isl28022.example.dts:22.30-28.15: Warning (i2c_bus_reg): /example-0/i2c/power-monitor@40: I2C bus unit address format error, expected "10"
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230726152235.249569-3-mail@carsten-spiess.de
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 15+ messages in thread