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@xxxxxxxxxx> >>> --- >>> .../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
