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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 Jul 2023 09:35:28 +0200
From:   Carsten Spieß <mail@...sten-spiess.de>
To:     Guenter Roeck <linux@...ck-us.net>,
        Jean Delvare <jdelvare@...e.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Magnus Damm <magnus.damm@...il.com>
Cc:     linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH 2/2] dt-bindings: hwmon: add renesas,isl28022

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

Content of type "application/pgp-signature" skipped