| Message ID | 20230913152135.457892-3-daniel.matyas@analog.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Why no v2? | expand |
On 13/09/2023 18:43, Guenter Roeck wrote: > On 9/13/23 08:40, Krzysztof Kozlowski wrote: >> On 13/09/2023 17:21, Daniel Matyas wrote: >> >> Subject: not much improved. I am sorry, but you are not adding new >> properties to entire devicetree of entire world. You are actually not >> adding anything to any devicetree, because these are bindings (which is >> obvious, as said by prefix). >> >> You got comments on this. >> >>> These attributes are: >>> - adi,comp-int - boolean property >>> - adi,alrm-pol - can be 0, 1 (if not present, default value) >>> - adi,flt-q - can be 1, 2, 4, 8 (if not present, default value) >>> - adi,timeout-enable - boolean property >> >> Don't repeat what the code does. Explain why you are adding it, what is >> the purpose. >> >>> >>> These modify the corresponding bits in the configuration register. >>> >>> Signed-off-by: Daniel Matyas <daniel.matyas@analog.com> >>> --- >>> .../bindings/hwmon/adi,max31827.yaml | 35 +++++++++++++++++++ >>> 1 file changed, 35 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml >>> index 2dc8b07b4d3b..6bde71bdb8dd 100644 >>> --- a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml >>> +++ b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml >>> @@ -32,6 +32,37 @@ properties: >>> Must have values in the interval (1.6V; 3.6V) in order for the device to >>> function correctly. >>> >>> + adi,comp-int: >>> + description: >>> + If present interrupt mode is used. If not present comparator mode is used >>> + (default). >> >> Why this is a property of hardware? >> > > Because it affects the behavior of the interrupt signal and whatever > it is connected to. For example, it could be connected to an interrupt > controller (interupt mode), or it could be connected to a fan which is > enabled while the signal is active (comparator mode). That makes sense. Pardon my naive questions, I just could not figure out use case out of the field description. Based on this very short description itself, I could imagine sysfs entry. > >>> + type: boolean >>> + >>> + adi,alrm-pol: >>> + description: >>> + Sets the alarms active state. >>> + - 0 = active low >>> + - 1 = active high >>> + For max31827 and max31828 the default alarm polarity is low. For max31829 >>> + it is high. >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [0, 1] >>> + >>> + adi,flt-q: >>> + description: >>> + Select how many consecutive temperature faults must occur before >>> + overtemperature or undertemperature faults are indicated in the >>> + corresponding status bits. >>> + For max31827 default fault queue is 1. For max31828 and max31829 it is 4. >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [1, 2, 4, 8] >>> + >>> + adi,timeout-enable: >>> + description: >>> + Enables timeout. Bus timeout resets the I2C-compatible interface when SCL >>> + is low for more than 30ms (nominal). >> >> Why this is a property of hardware? >> > > Because it affects i2c bus operation. > > I am not sure I understand what you are trying to say here. > This is a much a "hardware" property as the i2c bus speed > and many similar properties, and the need for it is system > dependent (for example, a system with many devices on the > i2c bus or with a less than perfect i2c controller may need > it because the bus tends to get stuck). > > Those are not properties where one would, at runtime, > decide to enable bus timeout or the interrupt mode or > the bus speed. Typically that kind of functionality > has to be configured early when the system is started. > If devicetree must not or no longer be used to describe the > system to a point where it can be configured to get it > to a working state, what is the suggested alternative ? I could imagine enabling it always, unconditionally. I wanted to understand why different boards with this chip will have it enabled or disabled. Best regards, Krzysztof
On 9/13/23 22:41, Krzysztof Kozlowski wrote: > On 13/09/2023 18:43, Guenter Roeck wrote: >> On 9/13/23 08:40, Krzysztof Kozlowski wrote: >>> On 13/09/2023 17:21, Daniel Matyas wrote: >>> >>> Subject: not much improved. I am sorry, but you are not adding new >>> properties to entire devicetree of entire world. You are actually not >>> adding anything to any devicetree, because these are bindings (which is >>> obvious, as said by prefix). >>> >>> You got comments on this. >>> >>>> These attributes are: >>>> - adi,comp-int - boolean property >>>> - adi,alrm-pol - can be 0, 1 (if not present, default value) >>>> - adi,flt-q - can be 1, 2, 4, 8 (if not present, default value) >>>> - adi,timeout-enable - boolean property >>> >>> Don't repeat what the code does. Explain why you are adding it, what is >>> the purpose. >>> >>>> >>>> These modify the corresponding bits in the configuration register. >>>> >>>> Signed-off-by: Daniel Matyas <daniel.matyas@analog.com> >>>> --- >>>> .../bindings/hwmon/adi,max31827.yaml | 35 +++++++++++++++++++ >>>> 1 file changed, 35 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml >>>> index 2dc8b07b4d3b..6bde71bdb8dd 100644 >>>> --- a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml >>>> +++ b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml >>>> @@ -32,6 +32,37 @@ properties: >>>> Must have values in the interval (1.6V; 3.6V) in order for the device to >>>> function correctly. >>>> >>>> + adi,comp-int: >>>> + description: >>>> + If present interrupt mode is used. If not present comparator mode is used >>>> + (default). >>> >>> Why this is a property of hardware? >>> >> >> Because it affects the behavior of the interrupt signal and whatever >> it is connected to. For example, it could be connected to an interrupt >> controller (interupt mode), or it could be connected to a fan which is >> enabled while the signal is active (comparator mode). > > That makes sense. Pardon my naive questions, I just could not figure out > use case out of the field description. Based on this very short > description itself, I could imagine sysfs entry. > >> >>>> + type: boolean >>>> + >>>> + adi,alrm-pol: >>>> + description: >>>> + Sets the alarms active state. >>>> + - 0 = active low >>>> + - 1 = active high >>>> + For max31827 and max31828 the default alarm polarity is low. For max31829 >>>> + it is high. >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + enum: [0, 1] >>>> + >>>> + adi,flt-q: >>>> + description: >>>> + Select how many consecutive temperature faults must occur before >>>> + overtemperature or undertemperature faults are indicated in the >>>> + corresponding status bits. >>>> + For max31827 default fault queue is 1. For max31828 and max31829 it is 4. >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + enum: [1, 2, 4, 8] >>>> + >>>> + adi,timeout-enable: >>>> + description: >>>> + Enables timeout. Bus timeout resets the I2C-compatible interface when SCL >>>> + is low for more than 30ms (nominal). >>> >>> Why this is a property of hardware? >>> >> >> Because it affects i2c bus operation. >> >> I am not sure I understand what you are trying to say here. >> This is a much a "hardware" property as the i2c bus speed >> and many similar properties, and the need for it is system >> dependent (for example, a system with many devices on the >> i2c bus or with a less than perfect i2c controller may need >> it because the bus tends to get stuck). >> >> Those are not properties where one would, at runtime, >> decide to enable bus timeout or the interrupt mode or >> the bus speed. Typically that kind of functionality >> has to be configured early when the system is started. >> If devicetree must not or no longer be used to describe the >> system to a point where it can be configured to get it >> to a working state, what is the suggested alternative ? > > I could imagine enabling it always, unconditionally. I wanted to > understand why different boards with this chip will have it enabled or > disabled. > Some boards may need a larger timeout, for example if there is a chip on the same bus which supports clock stretching. Guenter
diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml index 2dc8b07b4d3b..6bde71bdb8dd 100644 --- a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml +++ b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml @@ -32,6 +32,37 @@ properties: Must have values in the interval (1.6V; 3.6V) in order for the device to function correctly. + adi,comp-int: + description: + If present interrupt mode is used. If not present comparator mode is used + (default). + type: boolean + + adi,alrm-pol: + description: + Sets the alarms active state. + - 0 = active low + - 1 = active high + For max31827 and max31828 the default alarm polarity is low. For max31829 + it is high. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] + + adi,flt-q: + description: + Select how many consecutive temperature faults must occur before + overtemperature or undertemperature faults are indicated in the + corresponding status bits. + For max31827 default fault queue is 1. For max31828 and max31829 it is 4. + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [1, 2, 4, 8] + + adi,timeout-enable: + description: + Enables timeout. Bus timeout resets the I2C-compatible interface when SCL + is low for more than 30ms (nominal). + type: boolean + required: - compatible - reg @@ -49,6 +80,10 @@ examples: compatible = "adi,max31827"; reg = <0x42>; vref-supply = <®_vdd>; + adi,comp-int; + adi,alrm-pol = <0>; + adi,flt-q = <1>; + adi,timeout-enable; }; }; ...
These attributes are: - adi,comp-int - boolean property - adi,alrm-pol - can be 0, 1 (if not present, default value) - adi,flt-q - can be 1, 2, 4, 8 (if not present, default value) - adi,timeout-enable - boolean property These modify the corresponding bits in the configuration register. Signed-off-by: Daniel Matyas <daniel.matyas@analog.com> --- .../bindings/hwmon/adi,max31827.yaml | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+)