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 <[email protected]>
---
.../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).
+ 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;
};
};
...
--
2.34.1
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 <[email protected]>
>> ---
>> .../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).
>> + 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 ?
Guenter
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 <[email protected]>
> ---
> .../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?
> + 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?
Best regards,
Krzysztof
On 13/09/2023 17:48, Matyas, Daniel wrote:
>
>
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[email protected]>
>> Sent: Wednesday, September 13, 2023 6:41 PM
>> To: Matyas, Daniel <[email protected]>
>> Cc: Jean Delvare <[email protected]>; Guenter Roeck <linux@roeck-
>> us.net>; Rob Herring <[email protected]>; Krzysztof Kozlowski
>> <[email protected]>; Conor Dooley
>> <[email protected]>; Jonathan Corbet <[email protected]>; linux-
>> [email protected]; [email protected]; linux-
>> [email protected]; [email protected]
>> Subject: Re: [PATCH 2/4] dt-bindings: hwmon: Added new properties to
>> the devicetree
>>
>> [External]
>>
>> 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 <[email protected]>
>>> ---
>>> .../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?
>>
>>> + 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?
Code is okay, after Guenter's explanation. However please fix the
subject and improve the commit msg.
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 <[email protected]>
>>>> ---
>>>> .../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
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 <[email protected]>
>>> ---
>>> .../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
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Wednesday, September 13, 2023 6:41 PM
> To: Matyas, Daniel <[email protected]>
> Cc: Jean Delvare <[email protected]>; Guenter Roeck <linux@roeck-
> us.net>; Rob Herring <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley
> <[email protected]>; Jonathan Corbet <[email protected]>; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH 2/4] dt-bindings: hwmon: Added new properties to
> the devicetree
>
> [External]
>
> 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 <[email protected]>
> > ---
> > .../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?
>
> > + 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?
>
>
> Best regards,
> Krzysztof
Thank you for your quick reply, Krzysztof! I do not know exactly why does are
a property of hardware. I just did what Guenter suggested here:
https://marc.info/?l=linux-i2c&m=169201795715687&w=2
and here:
https://patchwork.kernel.org/project/linux-hwmon/patch/[email protected]/
Daniel