[PATCH 0/4] Why no v2?

[PATCH 0/4] Why no v2?

[Daniel Matyas]

This is kind of v2, but not exactly. Major changes were applied and
commits were separated. The latter is the main reason why I did not call
this v2.

Briefly, this is what changed compared to the previoius patch:
 - separate commit for: cleanup, dts doc, new dts props in driver,
   resolution attribute
 - timeout can be enabled now from the devicetree; no timeout attribute
 - support for max31828 and max31829

Daniel Matyas (4):
  hwmon: max31827: Make code cleaner
  dt-bindings: hwmon: Added new properties to the devicetree
  hwmon: max31827: Handle new properties from the devicetree
  hwmon: max31827: Add custom attribute for resolution

 .../bindings/hwmon/adi,max31827.yaml          |  35 ++
 Documentation/hwmon/max31827.rst              |  73 +++-
 drivers/hwmon/max31827.c                      | 397 +++++++++++++-----
 3 files changed, 395 insertions(+), 110 deletions(-)

-- 
2.34.1

[PATCH 1/4] hwmon: max31827: Make code cleaner

[Daniel Matyas]

Now the wait time for one-shot is 140ms, instead of the old 141
(removed the 1ms error).

Used enums and while loops to replace switch for selecting and getting
update interval from conversion rate bits.

Divided the write_alarm_val function into 2 functions. The new function
is more generic: it can be used not only for alarm writes, but for any
kind of writes which require the device to be in shutdown mode.

Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
---
 Documentation/hwmon/max31827.rst |   4 +-
 drivers/hwmon/max31827.c         | 127 ++++++++++++++-----------------
 2 files changed, 58 insertions(+), 73 deletions(-)

diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst
index b0971d05b8a4..9a1055a007cf 100644
--- a/Documentation/hwmon/max31827.rst
+++ b/Documentation/hwmon/max31827.rst
@@ -73,8 +73,8 @@ the conversion frequency to 1 conv/s. The conversion time varies depending on
 the resolution. The conversion time doubles with every bit of increased
 resolution. For 10 bit resolution 35ms are needed, while for 12 bit resolution
 (default) 140ms. When chip is in shutdown mode and a read operation is
-requested, one-shot is triggered, the device waits for 140 (conversion time) + 1
-(error) ms, and only after that is the temperature value register read.
+requested, one-shot is triggered, the device waits for 140 (conversion time) ms,
+and only after that is the temperature value register read.
 
 The LSB of the temperature values is 0.0625 degrees Celsius, but the values of
 the temperatures are displayed in milli-degrees. This means, that some data is
diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
index 602f4e4f81ff..f05762219995 100644
--- a/drivers/hwmon/max31827.c
+++ b/drivers/hwmon/max31827.c
@@ -25,20 +25,32 @@
 #define MAX31827_CONFIGURATION_U_TEMP_STAT_MASK	BIT(14)
 #define MAX31827_CONFIGURATION_O_TEMP_STAT_MASK	BIT(15)
 
-#define MAX31827_12_BIT_CNV_TIME	141
-
-#define MAX31827_CNV_1_DIV_64_HZ	0x1
-#define MAX31827_CNV_1_DIV_32_HZ	0x2
-#define MAX31827_CNV_1_DIV_16_HZ	0x3
-#define MAX31827_CNV_1_DIV_4_HZ		0x4
-#define MAX31827_CNV_1_HZ		0x5
-#define MAX31827_CNV_4_HZ		0x6
-#define MAX31827_CNV_8_HZ		0x7
+#define MAX31827_12_BIT_CNV_TIME	140
 
 #define MAX31827_16_BIT_TO_M_DGR(x)	(sign_extend32(x, 15) * 1000 / 16)
 #define MAX31827_M_DGR_TO_16_BIT(x)	(((x) << 4) / 1000)
 #define MAX31827_DEVICE_ENABLE(x)	((x) ? 0xA : 0x0)
 
+enum max31827_cnv {
+	MAX31827_CNV_1_DIV_64_HZ = 1,
+	MAX31827_CNV_1_DIV_32_HZ,
+	MAX31827_CNV_1_DIV_16_HZ,
+	MAX31827_CNV_1_DIV_4_HZ,
+	MAX31827_CNV_1_HZ,
+	MAX31827_CNV_4_HZ,
+	MAX31827_CNV_8_HZ,
+};
+
+static const u16 max31827_conversions[] = {
+	[MAX31827_CNV_1_DIV_64_HZ] = 64000,
+	[MAX31827_CNV_1_DIV_32_HZ] = 32000,
+	[MAX31827_CNV_1_DIV_16_HZ] = 16000,
+	[MAX31827_CNV_1_DIV_4_HZ] = 4000,
+	[MAX31827_CNV_1_HZ] = 1000,
+	[MAX31827_CNV_4_HZ] = 250,
+	[MAX31827_CNV_8_HZ] = 125,
+};
+
 struct max31827_state {
 	/*
 	 * Prevent simultaneous access to the i2c client.
@@ -54,15 +66,13 @@ static const struct regmap_config max31827_regmap = {
 	.max_register = 0xA,
 };
 
-static int write_alarm_val(struct max31827_state *st, unsigned int reg,
-			   long val)
+static int shutdown_write(struct max31827_state *st, unsigned int reg,
+			  unsigned int val)
 {
 	unsigned int cfg;
-	unsigned int tmp;
+	unsigned int cnv_rate;
 	int ret;
 
-	val = MAX31827_M_DGR_TO_16_BIT(val);
-
 	/*
 	 * Before the Temperature Threshold Alarm and Alarm Hysteresis Threshold
 	 * register values are changed over I2C, the part must be in shutdown
@@ -82,9 +92,10 @@ static int write_alarm_val(struct max31827_state *st, unsigned int reg,
 	if (ret)
 		goto unlock;
 
-	tmp = cfg & ~(MAX31827_CONFIGURATION_1SHOT_MASK |
+	cnv_rate = MAX31827_CONFIGURATION_CNV_RATE_MASK & cfg;
+	cfg = cfg & ~(MAX31827_CONFIGURATION_1SHOT_MASK |
 		      MAX31827_CONFIGURATION_CNV_RATE_MASK);
-	ret = regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, tmp);
+	ret = regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, cfg);
 	if (ret)
 		goto unlock;
 
@@ -92,13 +103,23 @@ static int write_alarm_val(struct max31827_state *st, unsigned int reg,
 	if (ret)
 		goto unlock;
 
-	ret = regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, cfg);
+	ret = regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
+				 MAX31827_CONFIGURATION_CNV_RATE_MASK,
+				 cnv_rate);
 
 unlock:
 	mutex_unlock(&st->lock);
 	return ret;
 }
 
+static int write_alarm_val(struct max31827_state *st, unsigned int reg,
+			   long val)
+{
+	val = MAX31827_M_DGR_TO_16_BIT(val);
+
+	return shutdown_write(st, reg, val);
+}
+
 static umode_t max31827_is_visible(const void *state,
 				   enum hwmon_sensor_types type, u32 attr,
 				   int channel)
@@ -243,32 +264,7 @@ static int max31827_read(struct device *dev, enum hwmon_sensor_types type,
 
 			uval = FIELD_GET(MAX31827_CONFIGURATION_CNV_RATE_MASK,
 					 uval);
-			switch (uval) {
-			case MAX31827_CNV_1_DIV_64_HZ:
-				*val = 64000;
-				break;
-			case MAX31827_CNV_1_DIV_32_HZ:
-				*val = 32000;
-				break;
-			case MAX31827_CNV_1_DIV_16_HZ:
-				*val = 16000;
-				break;
-			case MAX31827_CNV_1_DIV_4_HZ:
-				*val = 4000;
-				break;
-			case MAX31827_CNV_1_HZ:
-				*val = 1000;
-				break;
-			case MAX31827_CNV_4_HZ:
-				*val = 250;
-				break;
-			case MAX31827_CNV_8_HZ:
-				*val = 125;
-				break;
-			default:
-				*val = 0;
-				break;
-			}
+			*val = max31827_conversions[uval];
 		}
 		break;
 
@@ -284,6 +280,7 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
 			  u32 attr, int channel, long val)
 {
 	struct max31827_state *st = dev_get_drvdata(dev);
+	int res = 1;
 	int ret;
 
 	switch (type) {
@@ -333,39 +330,27 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
 			if (!st->enable)
 				return -EINVAL;
 
-			switch (val) {
-			case 125:
-				val = MAX31827_CNV_8_HZ;
-				break;
-			case 250:
-				val = MAX31827_CNV_4_HZ;
-				break;
-			case 1000:
-				val = MAX31827_CNV_1_HZ;
-				break;
-			case 4000:
-				val = MAX31827_CNV_1_DIV_4_HZ;
-				break;
-			case 16000:
-				val = MAX31827_CNV_1_DIV_16_HZ;
-				break;
-			case 32000:
-				val = MAX31827_CNV_1_DIV_32_HZ;
-				break;
-			case 64000:
-				val = MAX31827_CNV_1_DIV_64_HZ;
-				break;
-			default:
-				return -EINVAL;
-			}
+			/*
+			 * Convert the desired conversion rate into register
+			 * bits. res is already initialized with 1.
+			 *
+			 * This was inspired by lm73 driver.
+			 */
+			while (res < ARRAY_SIZE(max31827_conversions) &&
+			       val < max31827_conversions[res])
+				res++;
+
+			if (res == ARRAY_SIZE(max31827_conversions) ||
+			    val != max31827_conversions[res])
+				return -EOPNOTSUPP;
 
-			val = FIELD_PREP(MAX31827_CONFIGURATION_CNV_RATE_MASK,
-					 val);
+			res = FIELD_PREP(MAX31827_CONFIGURATION_CNV_RATE_MASK,
+					 res);
 
 			return regmap_update_bits(st->regmap,
 						  MAX31827_CONFIGURATION_REG,
 						  MAX31827_CONFIGURATION_CNV_RATE_MASK,
-						  val);
+						  res);
 		}
 		break;
 
-- 
2.34.1

[PATCH 2/4] dt-bindings: hwmon: Added new properties to the devicetree

[Daniel Matyas]

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(+)

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 = <&reg_vdd>;
+            adi,comp-int;
+            adi,alrm-pol = <0>;
+            adi,flt-q = <1>;
+            adi,timeout-enable;
         };
     };
 ...
-- 
2.34.1

Re: [PATCH 2/4] dt-bindings: hwmon: Added new properties to the devicetree

[Krzysztof Kozlowski]

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?

> +    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

RE: [PATCH 2/4] dt-bindings: hwmon: Added new properties to the devicetree

[Matyas, Daniel]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Wednesday, September 13, 2023 6:41 PM
> To: Matyas, Daniel <Daniel.Matyas@analog.com>
> Cc: Jean Delvare <jdelvare@suse.com>; Guenter Roeck <linux@roeck-
> us.net>; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Jonathan Corbet <corbet@lwn.net>; linux-
> hwmon@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-doc@vger.kernel.org
> 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 <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?
> 
> > +    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/20230911083735.11795-2-daniel.matyas@analog.com/

Daniel

Re: [PATCH 2/4] dt-bindings: hwmon: Added new properties to the devicetree

[Krzysztof Kozlowski]

On 13/09/2023 17:48, Matyas, Daniel wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Wednesday, September 13, 2023 6:41 PM
>> To: Matyas, Daniel <Daniel.Matyas@analog.com>
>> Cc: Jean Delvare <jdelvare@suse.com>; Guenter Roeck <linux@roeck-
>> us.net>; Rob Herring <robh+dt@kernel.org>; Krzysztof Kozlowski
>> <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
>> <conor+dt@kernel.org>; Jonathan Corbet <corbet@lwn.net>; linux-
>> hwmon@vger.kernel.org; devicetree@vger.kernel.org; linux-
>> kernel@vger.kernel.org; linux-doc@vger.kernel.org
>> 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 <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?
>>
>>> +    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

Re: [PATCH 2/4] dt-bindings: hwmon: Added new properties to the devicetree

[Guenter Roeck]

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).

>> +    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

Re: [PATCH 2/4] dt-bindings: hwmon: Added new properties to the devicetree

[Krzysztof Kozlowski]

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

Re: [PATCH 2/4] dt-bindings: hwmon: Added new properties to the devicetree

[Guenter Roeck]

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

[PATCH 3/4] hwmon: max31827: Handle new properties from the devicetree

[Daniel Matyas]

Used fwnode to retrieve data from the devicetree in the init_client
function.

Added support for devices max31828 and max31829. For these devices, when
adi,flt-q and/or adi,alrm-pol are not mentioned, the default
configuration is loaded.

Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
---
 Documentation/hwmon/max31827.rst |  47 ++++++++--
 drivers/hwmon/max31827.c         | 142 +++++++++++++++++++++++++++----
 2 files changed, 165 insertions(+), 24 deletions(-)

diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst
index 9a1055a007cf..e0eca1eb5c06 100644
--- a/Documentation/hwmon/max31827.rst
+++ b/Documentation/hwmon/max31827.rst
@@ -52,13 +52,21 @@ MAX31827 has low and over temperature alarms with an effective value and a
 hysteresis value: -40 and -30 degrees for under temperature alarm and +100 and
 +90 degrees for over temperature alarm.
 
-The alarm can be configured in comparator and interrupt mode. Currently only
-comparator mode is implemented. In Comparator mode, the OT/UT status bits have a
-value of 1 when the temperature rises above the TH value or falls below TL,
-which is also subject to the Fault Queue selection. OT status returns to 0 when
-the temperature drops below the TH_HYST value or when shutdown mode is entered.
-Similarly, UT status returns to 0 when the temperature rises above TL_HYST value
-or when shutdown mode is entered.
+The alarm can be configured in comparator and interrupt mode from the
+devicetree. In Comparator mode, the OT/UT status bits have a value of 1 when the
+temperature rises above the TH value or falls below TL, which is also subject to
+the Fault Queue selection. OT status returns to 0 when the temperature drops
+below the TH_HYST value or when shutdown mode is entered. Similarly, UT status
+returns to 0 when the temperature rises above TL_HYST value or when shutdown
+mode is entered.
+
+In interrupt mode exceeding TH also sets OT status to 1, which remains set until
+a read operation is performed on the configuration/status register (max or min
+attribute); at this point, it returns to 0. Once OT status is set to 1 from
+exceeding TH and reset, it is set to 1 again only when the temperature drops
+below TH_HYST. The output remains asserted until it is reset by a read. It is
+set again if the temperature rises above TH, and so on. The same logic applies
+to the operation of the UT status bit.
 
 Putting the MAX31827 into shutdown mode also resets the OT/UT status bits. Note
 that if the mode is changed while OT/UT status bits are set, an OT/UT status
@@ -68,6 +76,17 @@ clear the status bits before changing the operating mode.
 
 The conversions can be manual with the one-shot functionality and automatic with
 a set frequency. When powered on, the chip measures temperatures with 1 conv/s.
+The conversion rate can be modified with update_interval attribute of the chip.
+Conversion/second = 1/update_interval. Thus, the available options according to
+the data sheet are:
+	- 64000 (ms) = 1 conv/64 sec
+	- 32000 (ms) = 1 conv/32 sec
+	- 16000 (ms) = 1 conv/16 sec
+	- 4000 (ms) = 1 conv/4 sec
+	- 1000 (ms) = 1 conv/sec (default)
+	- 250 (ms) = 4 conv/sec
+	- 125 (ms) = 8 conv/sec
+
 Enabling the device when it is already enabled has the side effect of setting
 the conversion frequency to 1 conv/s. The conversion time varies depending on
 the resolution. The conversion time doubles with every bit of increased
@@ -83,8 +102,18 @@ in the writing of alarm values too. For positive numbers the user-input value
 will always be rounded down to the nearest possible value, for negative numbers
 the user-input will always be rounded up to the nearest possible value.
 
+Bus timeout resets the I2C-compatible interface when SCL is low for more than
+30ms (nominal).
+
+Alarm polarity determines if the active state of the alarm is low or high. The
+behavior for both settings is dependent on the Fault Queue setting. The ALARM
+pin is an open-drain output and requires a pullup resistor to operate.
+
+The Fault Queue bits select how many consecutive temperature faults must occur
+before overtemperature or undertemperature faults are indicated in the
+corresponding status bits.
+
 Notes
 -----
 
-Currently fault queue, alarm polarity and resolution cannot be modified.
-PEC is not implemented either.
+PEC and resolution are not implemented.
diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
index f05762219995..4dc14642775a 100644
--- a/drivers/hwmon/max31827.c
+++ b/drivers/hwmon/max31827.c
@@ -12,6 +12,19 @@
 #include <linux/i2c.h>
 #include <linux/mutex.h>
 #include <linux/regmap.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/of_device.h>
+
+/*
+ * gcc turns __builtin_ffsll() into a call to __ffsdi2(), which is not provided
+ * by every architecture. __ffs64() is available on all architectures, but the
+ * result is not defined if no bits are set.
+ */
+#define max31827__bf_shf(x)			 \
+	({					 \
+		typeof(x) x_ = (x);		 \
+		((x_) != 0) ? __ffs64(x_) : 0x0; \
+	})
 
 #define MAX31827_T_REG			0x0
 #define MAX31827_CONFIGURATION_REG	0x2
@@ -22,15 +35,25 @@
 
 #define MAX31827_CONFIGURATION_1SHOT_MASK	BIT(0)
 #define MAX31827_CONFIGURATION_CNV_RATE_MASK	GENMASK(3, 1)
-#define MAX31827_CONFIGURATION_U_TEMP_STAT_MASK	BIT(14)
-#define MAX31827_CONFIGURATION_O_TEMP_STAT_MASK	BIT(15)
+#define MAX31827_CONFIGURATION_TIMEOUT_MASK	BIT(5)
+#define MAX31827_CONFIGURATION_RESOLUTION_MASK	GENMASK(7, 6)
+#define MAX31827_CONFIGURATION_ALRM_POL_MASK	BIT(8)
+#define MAX31827_CONFIGURATION_COMP_INT_MASK	BIT(9)
+#define MAX31827_CONFIGURATION_FLT_Q_MASK	GENMASK(11, 10)
+#define MAX31827_CONFIGURATION_U_TEMP_STAT_MASK BIT(14)
+#define MAX31827_CONFIGURATION_O_TEMP_STAT_MASK BIT(15)
 
 #define MAX31827_12_BIT_CNV_TIME	140
 
+#define MAX31827_FLT_Q_1	0x0
+#define MAX31827_FLT_Q_4	0x2
+
 #define MAX31827_16_BIT_TO_M_DGR(x)	(sign_extend32(x, 15) * 1000 / 16)
 #define MAX31827_M_DGR_TO_16_BIT(x)	(((x) << 4) / 1000)
 #define MAX31827_DEVICE_ENABLE(x)	((x) ? 0xA : 0x0)
 
+enum chips { max31827, max31828, max31829 };
+
 enum max31827_cnv {
 	MAX31827_CNV_1_DIV_64_HZ = 1,
 	MAX31827_CNV_1_DIV_32_HZ,
@@ -58,6 +81,7 @@ struct max31827_state {
 	struct mutex lock;
 	struct regmap *regmap;
 	bool enable;
+	struct i2c_client *client;
 };
 
 static const struct regmap_config max31827_regmap = {
@@ -361,14 +385,93 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
 	return -EOPNOTSUPP;
 }
 
-static int max31827_init_client(struct max31827_state *st)
+static const struct i2c_device_id max31827_i2c_ids[] = {
+       { "max31827", max31827 },
+       { "max31828", max31828 },
+       { "max31829", max31829 },
+       { }
+};
+MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
+
+static int max31827_init_client(struct max31827_state *st,
+				struct fwnode_handle *fwnode)
 {
+	bool prop;
+	u32 data, lsb_idx;
+	unsigned int res = 0;
+	enum chips type;
+	int ret;
+
 	st->enable = true;
+	res |= MAX31827_DEVICE_ENABLE(1);
+
+	res |= MAX31827_CONFIGURATION_RESOLUTION_MASK;
 
-	return regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
-				  MAX31827_CONFIGURATION_1SHOT_MASK |
-					  MAX31827_CONFIGURATION_CNV_RATE_MASK,
-				  MAX31827_DEVICE_ENABLE(1));
+	prop = fwnode_property_read_bool(fwnode, "adi,comp-int");
+	res |= FIELD_PREP(MAX31827_CONFIGURATION_COMP_INT_MASK, prop);
+
+	prop = fwnode_property_read_bool(fwnode, "adi,timeout-enable");
+	res |= FIELD_PREP(MAX31827_CONFIGURATION_TIMEOUT_MASK, !prop);
+
+	if (st->client->dev.of_node)
+		type = (enum chips)of_device_get_match_data(&st->client->dev);
+	else
+		type = i2c_match_id(max31827_i2c_ids, st->client)->driver_data;
+
+	if (fwnode_property_present(fwnode, "adi,alrm-pol")) {
+		ret = fwnode_property_read_u32(fwnode, "adi,alrm-pol", &data);
+		if (ret)
+			return ret;
+
+		res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
+	} else {
+		switch (type) {
+		case max31827:
+		case max31828:
+			res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
+					  0x0);
+			break;
+		case max31829:
+			res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
+					  0x1);
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+	}
+
+	if (fwnode_property_present(fwnode, "adi,flt-q")) {
+		ret = fwnode_property_read_u32(fwnode, "adi,flt-q", &data);
+		if (ret)
+			return ret;
+
+		/*
+		 * Convert the desired fault queue into register bits.
+		 */
+		lsb_idx = max31827__bf_shf(data);
+		if (lsb_idx > 3 || data != BIT(lsb_idx)) {
+			dev_err(&st->client->dev, "Invalid data in fault queue\n");
+			return -EOPNOTSUPP;
+		}
+
+		res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK, lsb_idx);
+	} else {
+		switch (type) {
+		case max31827:
+			res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
+					  MAX31827_FLT_Q_1);
+			break;
+		case max31828:
+		case max31829:
+			res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
+					  MAX31827_FLT_Q_4);
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+	}
+
+	return regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, res);
 }
 
 static const struct hwmon_channel_info *max31827_info[] = {
@@ -396,6 +499,7 @@ static int max31827_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct device *hwmon_dev;
 	struct max31827_state *st;
+	struct fwnode_handle *fwnode;
 	int err;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
@@ -412,7 +516,10 @@ static int max31827_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(st->regmap),
 				     "Failed to allocate regmap.\n");
 
-	err = max31827_init_client(st);
+	st->client = client;
+	fwnode = dev_fwnode(dev);
+
+	err = max31827_init_client(st, fwnode);
 	if (err)
 		return err;
 
@@ -423,14 +530,19 @@ static int max31827_probe(struct i2c_client *client)
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
-static const struct i2c_device_id max31827_i2c_ids[] = {
-	{ "max31827", 0 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
-
 static const struct of_device_id max31827_of_match[] = {
-	{ .compatible = "adi,max31827" },
+	{
+		.compatible = "adi,max31827",
+		.data = (void *)max31827
+	},
+	{
+		.compatible = "adi,max31828",
+		.data = (void *)max31828
+	},
+	{
+		.compatible = "adi,max31829",
+		.data = (void *)max31829
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, max31827_of_match);
-- 
2.34.1

Re: [PATCH 3/4] hwmon: max31827: Handle new properties from the devicetree

[Guenter Roeck]

On 9/13/23 08:21, Daniel Matyas wrote:
> Used fwnode to retrieve data from the devicetree in the init_client
> function.
> 
> Added support for devices max31828 and max31829. For these devices, when
> adi,flt-q and/or adi,alrm-pol are not mentioned, the default
> configuration is loaded.
> 

Still more than one logical change in a single patch.

> Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
> ---
>   Documentation/hwmon/max31827.rst |  47 ++++++++--
>   drivers/hwmon/max31827.c         | 142 +++++++++++++++++++++++++++----
>   2 files changed, 165 insertions(+), 24 deletions(-)
> 
> diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst
> index 9a1055a007cf..e0eca1eb5c06 100644
> --- a/Documentation/hwmon/max31827.rst
> +++ b/Documentation/hwmon/max31827.rst
> @@ -52,13 +52,21 @@ MAX31827 has low and over temperature alarms with an effective value and a
>   hysteresis value: -40 and -30 degrees for under temperature alarm and +100 and
>   +90 degrees for over temperature alarm.
>   
> -The alarm can be configured in comparator and interrupt mode. Currently only
> -comparator mode is implemented. In Comparator mode, the OT/UT status bits have a
> -value of 1 when the temperature rises above the TH value or falls below TL,
> -which is also subject to the Fault Queue selection. OT status returns to 0 when
> -the temperature drops below the TH_HYST value or when shutdown mode is entered.
> -Similarly, UT status returns to 0 when the temperature rises above TL_HYST value
> -or when shutdown mode is entered.
> +The alarm can be configured in comparator and interrupt mode from the
> +devicetree. In Comparator mode, the OT/UT status bits have a value of 1 when the
> +temperature rises above the TH value or falls below TL, which is also subject to
> +the Fault Queue selection. OT status returns to 0 when the temperature drops
> +below the TH_HYST value or when shutdown mode is entered. Similarly, UT status
> +returns to 0 when the temperature rises above TL_HYST value or when shutdown
> +mode is entered.
> +
> +In interrupt mode exceeding TH also sets OT status to 1, which remains set until
> +a read operation is performed on the configuration/status register (max or min
> +attribute); at this point, it returns to 0. Once OT status is set to 1 from
> +exceeding TH and reset, it is set to 1 again only when the temperature drops
> +below TH_HYST. The output remains asserted until it is reset by a read. It is
> +set again if the temperature rises above TH, and so on. The same logic applies
> +to the operation of the UT status bit.
>   
>   Putting the MAX31827 into shutdown mode also resets the OT/UT status bits. Note
>   that if the mode is changed while OT/UT status bits are set, an OT/UT status
> @@ -68,6 +76,17 @@ clear the status bits before changing the operating mode.
>   
>   The conversions can be manual with the one-shot functionality and automatic with
>   a set frequency. When powered on, the chip measures temperatures with 1 conv/s.
> +The conversion rate can be modified with update_interval attribute of the chip.
> +Conversion/second = 1/update_interval. Thus, the available options according to
> +the data sheet are:
> +	- 64000 (ms) = 1 conv/64 sec
> +	- 32000 (ms) = 1 conv/32 sec
> +	- 16000 (ms) = 1 conv/16 sec
> +	- 4000 (ms) = 1 conv/4 sec
> +	- 1000 (ms) = 1 conv/sec (default)
> +	- 250 (ms) = 4 conv/sec
> +	- 125 (ms) = 8 conv/sec
> +
>   Enabling the device when it is already enabled has the side effect of setting
>   the conversion frequency to 1 conv/s. The conversion time varies depending on
>   the resolution. The conversion time doubles with every bit of increased
> @@ -83,8 +102,18 @@ in the writing of alarm values too. For positive numbers the user-input value
>   will always be rounded down to the nearest possible value, for negative numbers
>   the user-input will always be rounded up to the nearest possible value.
>   
> +Bus timeout resets the I2C-compatible interface when SCL is low for more than
> +30ms (nominal).
> +
> +Alarm polarity determines if the active state of the alarm is low or high. The
> +behavior for both settings is dependent on the Fault Queue setting. The ALARM
> +pin is an open-drain output and requires a pullup resistor to operate.
> +
> +The Fault Queue bits select how many consecutive temperature faults must occur
> +before overtemperature or undertemperature faults are indicated in the
> +corresponding status bits.
> +
>   Notes
>   -----
>   
> -Currently fault queue, alarm polarity and resolution cannot be modified.
> -PEC is not implemented either.
> +PEC and resolution are not implemented.
> diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
> index f05762219995..4dc14642775a 100644
> --- a/drivers/hwmon/max31827.c
> +++ b/drivers/hwmon/max31827.c
> @@ -12,6 +12,19 @@
>   #include <linux/i2c.h>
>   #include <linux/mutex.h>
>   #include <linux/regmap.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/of_device.h>
> +
> +/*
> + * gcc turns __builtin_ffsll() into a call to __ffsdi2(), which is not provided
> + * by every architecture. __ffs64() is available on all architectures, but the
> + * result is not defined if no bits are set.
> + */
> +#define max31827__bf_shf(x)			 \
> +	({					 \
> +		typeof(x) x_ = (x);		 \
> +		((x_) != 0) ? __ffs64(x_) : 0x0; \
> +	})
>   
>   #define MAX31827_T_REG			0x0
>   #define MAX31827_CONFIGURATION_REG	0x2
> @@ -22,15 +35,25 @@
>   
>   #define MAX31827_CONFIGURATION_1SHOT_MASK	BIT(0)
>   #define MAX31827_CONFIGURATION_CNV_RATE_MASK	GENMASK(3, 1)
> -#define MAX31827_CONFIGURATION_U_TEMP_STAT_MASK	BIT(14)
> -#define MAX31827_CONFIGURATION_O_TEMP_STAT_MASK	BIT(15)
> +#define MAX31827_CONFIGURATION_TIMEOUT_MASK	BIT(5)
> +#define MAX31827_CONFIGURATION_RESOLUTION_MASK	GENMASK(7, 6)
> +#define MAX31827_CONFIGURATION_ALRM_POL_MASK	BIT(8)
> +#define MAX31827_CONFIGURATION_COMP_INT_MASK	BIT(9)
> +#define MAX31827_CONFIGURATION_FLT_Q_MASK	GENMASK(11, 10)
> +#define MAX31827_CONFIGURATION_U_TEMP_STAT_MASK BIT(14)
> +#define MAX31827_CONFIGURATION_O_TEMP_STAT_MASK BIT(15)
>   
>   #define MAX31827_12_BIT_CNV_TIME	140
>   
> +#define MAX31827_FLT_Q_1	0x0
> +#define MAX31827_FLT_Q_4	0x2
> +
>   #define MAX31827_16_BIT_TO_M_DGR(x)	(sign_extend32(x, 15) * 1000 / 16)
>   #define MAX31827_M_DGR_TO_16_BIT(x)	(((x) << 4) / 1000)
>   #define MAX31827_DEVICE_ENABLE(x)	((x) ? 0xA : 0x0)
>   
> +enum chips { max31827, max31828, max31829 };
> +
>   enum max31827_cnv {
>   	MAX31827_CNV_1_DIV_64_HZ = 1,
>   	MAX31827_CNV_1_DIV_32_HZ,
> @@ -58,6 +81,7 @@ struct max31827_state {
>   	struct mutex lock;
>   	struct regmap *regmap;
>   	bool enable;
> +	struct i2c_client *client;
>   };
>   
>   static const struct regmap_config max31827_regmap = {
> @@ -361,14 +385,93 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
>   	return -EOPNOTSUPP;
>   }
>   
> -static int max31827_init_client(struct max31827_state *st)
> +static const struct i2c_device_id max31827_i2c_ids[] = {
> +       { "max31827", max31827 },
> +       { "max31828", max31828 },
> +       { "max31829", max31829 },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
> +
> +static int max31827_init_client(struct max31827_state *st,
> +				struct fwnode_handle *fwnode)
>   {
> +	bool prop;
> +	u32 data, lsb_idx;
> +	unsigned int res = 0;
> +	enum chips type;
> +	int ret;
> +
>   	st->enable = true;
> +	res |= MAX31827_DEVICE_ENABLE(1);
> +
> +	res |= MAX31827_CONFIGURATION_RESOLUTION_MASK;
>   
> -	return regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
> -				  MAX31827_CONFIGURATION_1SHOT_MASK |
> -					  MAX31827_CONFIGURATION_CNV_RATE_MASK,
> -				  MAX31827_DEVICE_ENABLE(1));
> +	prop = fwnode_property_read_bool(fwnode, "adi,comp-int");
> +	res |= FIELD_PREP(MAX31827_CONFIGURATION_COMP_INT_MASK, prop);
> +
> +	prop = fwnode_property_read_bool(fwnode, "adi,timeout-enable");
> +	res |= FIELD_PREP(MAX31827_CONFIGURATION_TIMEOUT_MASK, !prop);
> +
> +	if (st->client->dev.of_node)
> +		type = (enum chips)of_device_get_match_data(&st->client->dev);
> +	else
> +		type = i2c_match_id(max31827_i2c_ids, st->client)->driver_data;
> +
> +	if (fwnode_property_present(fwnode, "adi,alrm-pol")) {
> +		ret = fwnode_property_read_u32(fwnode, "adi,alrm-pol", &data);
> +		if (ret)
> +			return ret;
> +
> +		res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
> +	} else {
> +		switch (type) {
> +		case max31827:
> +		case max31828:
> +			res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
> +					  0x0);
> +			break;
> +		case max31829:
> +			res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
> +					  0x1);
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	if (fwnode_property_present(fwnode, "adi,flt-q")) {
> +		ret = fwnode_property_read_u32(fwnode, "adi,flt-q", &data);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Convert the desired fault queue into register bits.
> +		 */
> +		lsb_idx = max31827__bf_shf(data);
> +		if (lsb_idx > 3 || data != BIT(lsb_idx)) {
> +			dev_err(&st->client->dev, "Invalid data in fault queue\n");
> +			return -EOPNOTSUPP;
> +		}
> +
> +		res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK, lsb_idx);
> +	} else {
> +		switch (type) {
> +		case max31827:
> +			res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
> +					  MAX31827_FLT_Q_1);
> +			break;
> +		case max31828:
> +		case max31829:
> +			res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
> +					  MAX31827_FLT_Q_4);
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +	}
> +
> +	return regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, res);
>   }
>   
>   static const struct hwmon_channel_info *max31827_info[] = {
> @@ -396,6 +499,7 @@ static int max31827_probe(struct i2c_client *client)
>   	struct device *dev = &client->dev;
>   	struct device *hwmon_dev;
>   	struct max31827_state *st;
> +	struct fwnode_handle *fwnode;
>   	int err;
>   
>   	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> @@ -412,7 +516,10 @@ static int max31827_probe(struct i2c_client *client)
>   		return dev_err_probe(dev, PTR_ERR(st->regmap),
>   				     "Failed to allocate regmap.\n");
>   
> -	err = max31827_init_client(st);
> +	st->client = client;
> +	fwnode = dev_fwnode(dev);
> +
> +	err = max31827_init_client(st, fwnode);
>   	if (err)
>   		return err;
>   
> @@ -423,14 +530,19 @@ static int max31827_probe(struct i2c_client *client)
>   	return PTR_ERR_OR_ZERO(hwmon_dev);
>   }
>   
> -static const struct i2c_device_id max31827_i2c_ids[] = {
> -	{ "max31827", 0 },
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
> -
>   static const struct of_device_id max31827_of_match[] = {
> -	{ .compatible = "adi,max31827" },
> +	{
> +		.compatible = "adi,max31827",
> +		.data = (void *)max31827
> +	},
> +	{
> +		.compatible = "adi,max31828",
> +		.data = (void *)max31828
> +	},
> +	{
> +		.compatible = "adi,max31829",
> +		.data = (void *)max31829
> +	},
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(of, max31827_of_match);

Re: [PATCH 3/4] hwmon: max31827: Handle new properties from the devicetree

[kernel test robot]

Hi Daniel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.6-rc1 next-20230913]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Matyas/hwmon-max31827-Make-code-cleaner/20230913-232729
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230913152135.457892-4-daniel.matyas%40analog.com
patch subject: [PATCH 3/4] hwmon: max31827: Handle new properties from the devicetree
reproduce: (https://download.01.org/0day-ci/archive/20230914/202309141254.Dpetnhzg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309141254.Dpetnhzg-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Documentation/hwmon/max31827.rst:82: WARNING: Unexpected indentation.

vim +82 Documentation/hwmon/max31827.rst

    76	
    77	The conversions can be manual with the one-shot functionality and automatic with
    78	a set frequency. When powered on, the chip measures temperatures with 1 conv/s.
    79	The conversion rate can be modified with update_interval attribute of the chip.
    80	Conversion/second = 1/update_interval. Thus, the available options according to
    81	the data sheet are:
  > 82		- 64000 (ms) = 1 conv/64 sec
    83		- 32000 (ms) = 1 conv/32 sec
    84		- 16000 (ms) = 1 conv/16 sec
    85		- 4000 (ms) = 1 conv/4 sec
    86		- 1000 (ms) = 1 conv/sec (default)
    87		- 250 (ms) = 4 conv/sec
    88		- 125 (ms) = 8 conv/sec
    89	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 4/4] hwmon: max31827: Add custom attribute for resolution

[Daniel Matyas]

Added custom channel-specific (temp1) attribute for resolution. The wait
time for a conversion in one-shot mode (enable = 0) depends on the
resolution.

When resolution is 12-bit, the conversion time is 140ms, but the minimum
update_interval is 125ms. Handled this problem by waiting an additional
15ms (125ms + 15ms = 140ms).

Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
---
 Documentation/hwmon/max31827.rst |  28 +++++--
 drivers/hwmon/max31827.c         | 136 +++++++++++++++++++++++++++----
 2 files changed, 144 insertions(+), 20 deletions(-)

diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst
index e0eca1eb5c06..219ab8abfd6a 100644
--- a/Documentation/hwmon/max31827.rst
+++ b/Documentation/hwmon/max31827.rst
@@ -89,11 +89,27 @@ the data sheet are:
 
 Enabling the device when it is already enabled has the side effect of setting
 the conversion frequency to 1 conv/s. The conversion time varies depending on
-the resolution. The conversion time doubles with every bit of increased
-resolution. For 10 bit resolution 35ms are needed, while for 12 bit resolution
-(default) 140ms. When chip is in shutdown mode and a read operation is
-requested, one-shot is triggered, the device waits for 140 (conversion time) ms,
-and only after that is the temperature value register read.
+the resolution.
+
+The conversion time doubles with every bit of increased resolution. The
+available resolutions are:
+	- 8 bit -> 8.75 ms conversion time
+	- 9 bit -> 17.5 ms conversion time
+	- 10 bit -> 35 ms conversion time
+	- 12 bit (default) -> 140 ms conversion time
+
+There is a temp1_resolution attribute which indicates the unit change in the
+input temperature in milli-degrees C.
+	- 1000 mC -> 8 bit
+	- 500 mC -> 9 bit
+	- 250 mC -> 10 bit
+	- 62 mC -> 12 bit (default) - actually this is 62.5, but the file
+	returns 62
+
+When chip is in shutdown mode and a read operation is requested, one-shot is
+triggered, the device waits for <conversion time> ms, and only after that is
+the temperature value register read. Note that the conversion times are rounded
+up to the nearest possible integer.
 
 The LSB of the temperature values is 0.0625 degrees Celsius, but the values of
 the temperatures are displayed in milli-degrees. This means, that some data is
@@ -116,4 +132,4 @@ corresponding status bits.
 Notes
 -----
 
-PEC and resolution are not implemented.
+PEC is not implemented.
diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
index 4dc14642775a..396292c0bc32 100644
--- a/drivers/hwmon/max31827.c
+++ b/drivers/hwmon/max31827.c
@@ -43,6 +43,9 @@
 #define MAX31827_CONFIGURATION_U_TEMP_STAT_MASK BIT(14)
 #define MAX31827_CONFIGURATION_O_TEMP_STAT_MASK BIT(15)
 
+#define MAX31827_8_BIT_CNV_TIME		9
+#define MAX31827_9_BIT_CNV_TIME		18
+#define MAX31827_10_BIT_CNV_TIME	35
 #define MAX31827_12_BIT_CNV_TIME	140
 
 #define MAX31827_FLT_Q_1	0x0
@@ -51,6 +54,7 @@
 #define MAX31827_16_BIT_TO_M_DGR(x)	(sign_extend32(x, 15) * 1000 / 16)
 #define MAX31827_M_DGR_TO_16_BIT(x)	(((x) << 4) / 1000)
 #define MAX31827_DEVICE_ENABLE(x)	((x) ? 0xA : 0x0)
+#define MAX31827_FLT_Q
 
 enum chips { max31827, max31828, max31829 };
 
@@ -74,6 +78,27 @@ static const u16 max31827_conversions[] = {
 	[MAX31827_CNV_8_HZ] = 125,
 };
 
+enum max31827_resolution {
+	MAX31827_RES_8_BIT = 0,
+	MAX31827_RES_9_BIT,
+	MAX31827_RES_10_BIT,
+	MAX31827_RES_12_BIT,
+};
+
+static const u16 max31827_resolutions[] = {
+	[MAX31827_RES_8_BIT] = 1000,
+	[MAX31827_RES_9_BIT] = 500,
+	[MAX31827_RES_10_BIT] = 250,
+	[MAX31827_RES_12_BIT] = 62,
+};
+
+static const u16 max31827_conv_times[] = {
+	[MAX31827_RES_8_BIT] = MAX31827_8_BIT_CNV_TIME,
+	[MAX31827_RES_9_BIT] = MAX31827_9_BIT_CNV_TIME,
+	[MAX31827_RES_10_BIT] = MAX31827_10_BIT_CNV_TIME,
+	[MAX31827_RES_12_BIT] = MAX31827_12_BIT_CNV_TIME,
+};
+
 struct max31827_state {
 	/*
 	 * Prevent simultaneous access to the i2c client.
@@ -81,6 +106,8 @@ struct max31827_state {
 	struct mutex lock;
 	struct regmap *regmap;
 	bool enable;
+	unsigned int resolution;
+	unsigned int update_interval;
 	struct i2c_client *client;
 };
 
@@ -98,9 +125,9 @@ static int shutdown_write(struct max31827_state *st, unsigned int reg,
 	int ret;
 
 	/*
-	 * Before the Temperature Threshold Alarm and Alarm Hysteresis Threshold
-	 * register values are changed over I2C, the part must be in shutdown
-	 * mode.
+	 * Before the Temperature Threshold Alarm, Alarm Hysteresis Threshold
+	 * and Resolution bits from Configuration register are changed over I2C,
+	 * the part must be in shutdown mode.
 	 *
 	 * Mutex is used to ensure, that some other process doesn't change the
 	 * configuration register.
@@ -211,9 +238,18 @@ static int max31827_read(struct device *dev, enum hwmon_sensor_types type,
 					mutex_unlock(&st->lock);
 					return ret;
 				}
-
-				msleep(MAX31827_12_BIT_CNV_TIME);
+				msleep(max31827_conv_times[st->resolution]);
 			}
+
+			/*
+			 * For 12-bit resolution the conversion time is 140 ms,
+			 * thus an additional 15 ms is needed to complete the
+			 * conversion: 125 ms + 15 ms = 140 ms
+			 */
+			if (max31827_resolutions[st->resolution] == 12 &&
+			    st->update_interval == 125)
+				usleep_range(15000, 20000);
+
 			ret = regmap_read(st->regmap, MAX31827_T_REG, &uval);
 
 			mutex_unlock(&st->lock);
@@ -371,10 +407,14 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
 			res = FIELD_PREP(MAX31827_CONFIGURATION_CNV_RATE_MASK,
 					 res);
 
-			return regmap_update_bits(st->regmap,
-						  MAX31827_CONFIGURATION_REG,
-						  MAX31827_CONFIGURATION_CNV_RATE_MASK,
-						  res);
+			ret = regmap_update_bits(st->regmap,
+						 MAX31827_CONFIGURATION_REG,
+						 MAX31827_CONFIGURATION_CNV_RATE_MASK,
+						 res);
+			if (ret)
+				return ret;
+
+			st->update_interval = val;
 		}
 		break;
 
@@ -385,11 +425,79 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
 	return -EOPNOTSUPP;
 }
 
+static ssize_t temp1_resolution_show(struct device *dev,
+				     struct device_attribute *devattr,
+				     char *buf)
+{
+	struct max31827_state *st = dev_get_drvdata(dev);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(st->regmap, MAX31827_CONFIGURATION_REG, &val);
+	if (ret)
+		return ret;
+
+	val = FIELD_GET(MAX31827_CONFIGURATION_RESOLUTION_MASK, val);
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n", max31827_resolutions[val]);
+}
+
+static ssize_t temp1_resolution_store(struct device *dev,
+				      struct device_attribute *devattr,
+				      const char *buf, size_t count)
+{
+	struct max31827_state *st = dev_get_drvdata(dev);
+	unsigned int idx = 0;
+	unsigned int val, cfg;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	/*
+	 * Convert the desired conversion rate into register
+	 * bits. idx is already initialized with 1.
+	 *
+	 * This was inspired by lm73 driver.
+	 */
+	while (idx < ARRAY_SIZE(max31827_resolutions) &&
+	       val < max31827_resolutions[idx])
+		idx++;
+
+	if (idx == ARRAY_SIZE(max31827_resolutions) ||
+	    val != max31827_resolutions[idx])
+		return -EOPNOTSUPP;
+
+	st->resolution = idx;
+
+	ret = regmap_read(st->regmap, MAX31827_CONFIGURATION_REG, &cfg);
+	if (ret)
+		return ret;
+
+	cfg = (cfg & ~MAX31827_CONFIGURATION_RESOLUTION_MASK) |
+	       FIELD_PREP(MAX31827_CONFIGURATION_RESOLUTION_MASK, idx);
+	cfg &= ~(MAX31827_CONFIGURATION_CNV_RATE_MASK |
+		 MAX31827_CONFIGURATION_1SHOT_MASK);
+
+	ret = shutdown_write(st, MAX31827_CONFIGURATION_REG, cfg);
+
+	return (ret) ? ret : count;
+}
+
+static DEVICE_ATTR_RW(temp1_resolution);
+
+static struct attribute *max31827_attrs[] = {
+	&dev_attr_temp1_resolution.attr,
+	NULL
+};
+ATTRIBUTE_GROUPS(max31827);
+
 static const struct i2c_device_id max31827_i2c_ids[] = {
-       { "max31827", max31827 },
-       { "max31828", max31828 },
-       { "max31829", max31829 },
-       { }
+	{ "max31827", max31827 },
+	{ "max31828", max31828 },
+	{ "max31829", max31829 },
+	{ }
 };
 MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
 
@@ -525,7 +633,7 @@ static int max31827_probe(struct i2c_client *client)
 
 	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, st,
 							 &max31827_chip_info,
-							 NULL);
+							 max31827_groups);
 
 	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
-- 
2.34.1

Re: [PATCH 4/4] hwmon: max31827: Add custom attribute for resolution

[kernel test robot]

Hi Daniel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.6-rc1 next-20230914]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Matyas/hwmon-max31827-Make-code-cleaner/20230913-232729
base:   https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link:    https://lore.kernel.org/r/20230913152135.457892-5-daniel.matyas%40analog.com
patch subject: [PATCH 4/4] hwmon: max31827: Add custom attribute for resolution
reproduce: (https://download.01.org/0day-ci/archive/20230914/202309142140.qvHsiLc3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309142140.qvHsiLc3-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Documentation/hwmon/max31827.rst:107: WARNING: Bullet list ends without a blank line; unexpected unindent.

vim +107 Documentation/hwmon/max31827.rst

    93	
    94	The conversion time doubles with every bit of increased resolution. The
    95	available resolutions are:
    96		- 8 bit -> 8.75 ms conversion time
    97		- 9 bit -> 17.5 ms conversion time
    98		- 10 bit -> 35 ms conversion time
    99		- 12 bit (default) -> 140 ms conversion time
   100	
   101	There is a temp1_resolution attribute which indicates the unit change in the
   102	input temperature in milli-degrees C.
   103		- 1000 mC -> 8 bit
   104		- 500 mC -> 9 bit
   105		- 250 mC -> 10 bit
   106		- 62 mC -> 12 bit (default) - actually this is 62.5, but the file
 > 107		returns 62
   108	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 0/4] Why no v2?

[Guenter Roeck]

On 9/13/23 08:21, Daniel Matyas wrote:
> This is kind of v2, but not exactly. Major changes were applied and
> commits were separated. The latter is the main reason why I did not call
> this v2.
> 

That is neither a reason nor a valid argument. Just call it v2 and mark
the added patch(es) with a changelog along the line of "v2: Added patch".

And a subject of "Why no v2?" is really useless and pointless.

Guenter