* [PATCH v4 2/7] hwmon: max31827: Modify conversion wait time
2023-09-19 9:34 [PATCH v4 1/7] hwmon: max31827: Make code cleaner Daniel Matyas
@ 2023-09-19 9:34 ` Daniel Matyas
2023-10-25 21:44 ` Guenter Roeck
2023-09-19 9:34 ` [PATCH v4 3/7] dt-bindings: hwmon: Add possible new properties to max31827 bindings Daniel Matyas
` (5 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Daniel Matyas @ 2023-09-19 9:34 UTC (permalink / raw)
Cc: Daniel Matyas, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-hwmon,
devicetree, linux-kernel, linux-doc
There is nothing in the datasheet indicating that the 1ms error is
needed and I didn't encounter any error during testing with 140ms wait
time.
Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
---
v4: Added patch.
Documentation/hwmon/max31827.rst | 4 ++--
drivers/hwmon/max31827.c | 2 +-
2 files changed, 3 insertions(+), 3 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 47124d665fa7..f05762219995 100644
--- a/drivers/hwmon/max31827.c
+++ b/drivers/hwmon/max31827.c
@@ -25,7 +25,7 @@
#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_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)
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 3/7] dt-bindings: hwmon: Add possible new properties to max31827 bindings
2023-09-19 9:34 [PATCH v4 1/7] hwmon: max31827: Make code cleaner Daniel Matyas
2023-09-19 9:34 ` [PATCH v4 2/7] hwmon: max31827: Modify conversion wait time Daniel Matyas
@ 2023-09-19 9:34 ` Daniel Matyas
2023-09-19 11:00 ` Conor Dooley
2023-10-25 21:44 ` Guenter Roeck
2023-09-19 9:34 ` [PATCH v4 4/7] hwmon: max31827: Handle new properties from the devicetree Daniel Matyas
` (4 subsequent siblings)
6 siblings, 2 replies; 13+ messages in thread
From: Daniel Matyas @ 2023-09-19 9:34 UTC (permalink / raw)
Cc: Daniel Matyas, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-hwmon,
devicetree, linux-kernel, linux-doc
These modify the corresponding bits in the configuration register.
adi,comp-int is a hardware property, because it affects the behavior
of the interrupt signal and whatever it is connected to.
adi,timeout-enable is a hardware property, because it affects i2c
bus operation.
Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
---
v3 -> v4: Changed property names (adi,flt-q = adi,fault-q; adi,alrm-pol
= adi,alarm-pol). Expressed default values in binding.
v2 -> v3: Changed commit subject and message
v1 -> v2: Added adi,timeout-enable property to binding. Fixed
dt_binding_check errors.
.../bindings/hwmon/adi,max31827.yaml | 66 +++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
index 2dc8b07b4d3b..f60e06ab7d0a 100644
--- a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
@@ -32,6 +32,68 @@ 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,alarm-pol:
+ description:
+ Sets the alarms active state.
+ - 0 = active low
+ - 1 = active high
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1]
+
+ adi,fault-q:
+ description:
+ Select how many consecutive temperature faults must occur before
+ overtemperature or undertemperature faults are indicated in the
+ corresponding status bits.
+ $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
+
+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: adi,max31829
+
+ then:
+ properties:
+ adi,alarm-pol:
+ default: 1
+
+ else:
+ properties:
+ adi,alarm-pol:
+ default: 0
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: adi,max31827
+
+ then:
+ properties:
+ adi,fault-q:
+ default: 1
+
+ else:
+ properties:
+ adi,fault-q:
+ default: 4
+
+
required:
- compatible
- reg
@@ -49,6 +111,10 @@ examples:
compatible = "adi,max31827";
reg = <0x42>;
vref-supply = <®_vdd>;
+ adi,comp-int;
+ adi,alarm-pol = <0>;
+ adi,fault-q = <1>;
+ adi,timeout-enable;
};
};
...
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v4 3/7] dt-bindings: hwmon: Add possible new properties to max31827 bindings
2023-09-19 9:34 ` [PATCH v4 3/7] dt-bindings: hwmon: Add possible new properties to max31827 bindings Daniel Matyas
@ 2023-09-19 11:00 ` Conor Dooley
2023-10-25 21:44 ` Guenter Roeck
1 sibling, 0 replies; 13+ messages in thread
From: Conor Dooley @ 2023-09-19 11:00 UTC (permalink / raw)
To: Daniel Matyas
Cc: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, linux-hwmon, devicetree,
linux-kernel, linux-doc
[-- Attachment #1: Type: text/plain, Size: 3376 bytes --]
Hey,
On Tue, Sep 19, 2023 at 12:34:51PM +0300, Daniel Matyas wrote:
> These modify the corresponding bits in the configuration register.
>
> adi,comp-int is a hardware property, because it affects the behavior
> of the interrupt signal and whatever it is connected to.
>
> adi,timeout-enable is a hardware property, because it affects i2c
> bus operation.
>
> Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
Thanks for the updates. This looks okay to me now.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Thanks,
Conor.
> ---
>
> v3 -> v4: Changed property names (adi,flt-q = adi,fault-q; adi,alrm-pol
> = adi,alarm-pol). Expressed default values in binding.
>
> v2 -> v3: Changed commit subject and message
>
> v1 -> v2: Added adi,timeout-enable property to binding. Fixed
> dt_binding_check errors.
>
> .../bindings/hwmon/adi,max31827.yaml | 66 +++++++++++++++++++
> 1 file changed, 66 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> index 2dc8b07b4d3b..f60e06ab7d0a 100644
> --- a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> @@ -32,6 +32,68 @@ 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,alarm-pol:
> + description:
> + Sets the alarms active state.
> + - 0 = active low
> + - 1 = active high
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1]
> +
> + adi,fault-q:
> + description:
> + Select how many consecutive temperature faults must occur before
> + overtemperature or undertemperature faults are indicated in the
> + corresponding status bits.
> + $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
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: adi,max31829
> +
> + then:
> + properties:
> + adi,alarm-pol:
> + default: 1
> +
> + else:
> + properties:
> + adi,alarm-pol:
> + default: 0
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: adi,max31827
> +
> + then:
> + properties:
> + adi,fault-q:
> + default: 1
> +
> + else:
> + properties:
> + adi,fault-q:
> + default: 4
> +
> +
> required:
> - compatible
> - reg
> @@ -49,6 +111,10 @@ examples:
> compatible = "adi,max31827";
> reg = <0x42>;
> vref-supply = <®_vdd>;
> + adi,comp-int;
> + adi,alarm-pol = <0>;
> + adi,fault-q = <1>;
> + adi,timeout-enable;
> };
> };
> ...
> --
> 2.34.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/7] dt-bindings: hwmon: Add possible new properties to max31827 bindings
2023-09-19 9:34 ` [PATCH v4 3/7] dt-bindings: hwmon: Add possible new properties to max31827 bindings Daniel Matyas
2023-09-19 11:00 ` Conor Dooley
@ 2023-10-25 21:44 ` Guenter Roeck
1 sibling, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2023-10-25 21:44 UTC (permalink / raw)
To: Daniel Matyas; +Cc: no To-header on input <
On Tue, Sep 19, 2023 at 12:34:51PM +0300, Daniel Matyas wrote:
> These modify the corresponding bits in the configuration register.
>
> adi,comp-int is a hardware property, because it affects the behavior
> of the interrupt signal and whatever it is connected to.
>
> adi,timeout-enable is a hardware property, because it affects i2c
> bus operation.
>
> Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Applied.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 4/7] hwmon: max31827: Handle new properties from the devicetree
2023-09-19 9:34 [PATCH v4 1/7] hwmon: max31827: Make code cleaner Daniel Matyas
2023-09-19 9:34 ` [PATCH v4 2/7] hwmon: max31827: Modify conversion wait time Daniel Matyas
2023-09-19 9:34 ` [PATCH v4 3/7] dt-bindings: hwmon: Add possible new properties to max31827 bindings Daniel Matyas
@ 2023-09-19 9:34 ` Daniel Matyas
2023-10-25 22:01 ` Guenter Roeck
2023-10-26 0:45 ` Guenter Roeck
2023-09-19 9:34 ` [PATCH v4 5/7] hwmon: max31827: Add support for max31828 and max31829 Daniel Matyas
` (3 subsequent siblings)
6 siblings, 2 replies; 13+ messages in thread
From: Daniel Matyas @ 2023-09-19 9:34 UTC (permalink / raw)
Cc: Daniel Matyas, kernel test robot, Jean Delvare, Guenter Roeck,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet,
linux-hwmon, devicetree, linux-kernel, linux-doc
Used fwnode to retrieve data from the devicetree in the init_client
function.
If the uint32 properties are not present, the default values are used
for max31827 chip.
Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
---
v3 -> v4: Renamed property names to correspond with binding.
v2 -> v3: Separated patch into 2. Fixed 'WARNING: Unexpected
indentation.'
Reported-by: kernel test robot <lkp@intel.com>
v2: Added patch
Documentation/hwmon/max31827.rst | 48 ++++++++++++++----
drivers/hwmon/max31827.c | 85 +++++++++++++++++++++++++++++---
2 files changed, 116 insertions(+), 17 deletions(-)
diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst
index 9a1055a007cf..a8bbfb85dd02 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,18 @@ 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 +103,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..2bddca60666d 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,11 +35,19 @@
#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_ALRM_POL_LOW 0x0
+#define MAX31827_FLT_Q_1 0x0
+
#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)
@@ -58,6 +79,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 +383,57 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
return -EOPNOTSUPP;
}
-static int max31827_init_client(struct max31827_state *st)
+static int max31827_init_client(struct max31827_state *st,
+ struct fwnode_handle *fwnode)
{
+ bool prop;
+ u32 data, lsb_idx;
+ unsigned int res = 0;
+ int ret;
+
st->enable = true;
+ res |= MAX31827_DEVICE_ENABLE(1);
+
+ res |= MAX31827_CONFIGURATION_RESOLUTION_MASK;
+
+ 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 (fwnode_property_present(fwnode, "adi,alarm-pol")) {
+ ret = fwnode_property_read_u32(fwnode, "adi,alarm-pol", &data);
+ if (ret)
+ return ret;
- return regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
- MAX31827_CONFIGURATION_1SHOT_MASK |
- MAX31827_CONFIGURATION_CNV_RATE_MASK,
- MAX31827_DEVICE_ENABLE(1));
+ res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
+ } else {
+ res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
+ MAX31827_ALRM_POL_LOW);
+ }
+
+ if (fwnode_property_present(fwnode, "adi,fault-q")) {
+ ret = fwnode_property_read_u32(fwnode, "adi,fault-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 {
+ res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
+ MAX31827_FLT_Q_1);
+ }
+
+ return regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, res);
}
static const struct hwmon_channel_info *max31827_info[] = {
@@ -396,6 +461,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 +478,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;
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v4 4/7] hwmon: max31827: Handle new properties from the devicetree
2023-09-19 9:34 ` [PATCH v4 4/7] hwmon: max31827: Handle new properties from the devicetree Daniel Matyas
@ 2023-10-25 22:01 ` Guenter Roeck
2023-10-26 0:45 ` Guenter Roeck
1 sibling, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2023-10-25 22:01 UTC (permalink / raw)
To: Daniel Matyas; +Cc: no To-header on input <
On Tue, Sep 19, 2023 at 12:34:52PM +0300, Daniel Matyas wrote:
> Used fwnode to retrieve data from the devicetree in the init_client
> function.
>
> If the uint32 properties are not present, the default values are used
> for max31827 chip.
>
> Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
> ---
>
> v3 -> v4: Renamed property names to correspond with binding.
>
> v2 -> v3: Separated patch into 2. Fixed 'WARNING: Unexpected
> indentation.'
> Reported-by: kernel test robot <lkp@intel.com>
>
> v2: Added patch
>
> Documentation/hwmon/max31827.rst | 48 ++++++++++++++----
> drivers/hwmon/max31827.c | 85 +++++++++++++++++++++++++++++---
> 2 files changed, 116 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst
> index 9a1055a007cf..a8bbfb85dd02 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,18 @@ 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 +103,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..2bddca60666d 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>
What is this supposed to be needed for ?
> +#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; \
> + })
You lost me here. The passed parameter is u32, making __ffs64
unnecessary. Why not use ffs() ?
>
> #define MAX31827_T_REG 0x0
> #define MAX31827_CONFIGURATION_REG 0x2
> @@ -22,11 +35,19 @@
>
> #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)
Why drop the <tab> after MAX31827_CONFIGURATION_U_TEMP_STAT_MASK
and MAX31827_CONFIGURATION_O_TEMP_STAT_MASK ?
>
> #define MAX31827_12_BIT_CNV_TIME 140
>
> +#define MAX31827_ALRM_POL_LOW 0x0
> +#define MAX31827_FLT_Q_1 0x0
> +
> #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)
> @@ -58,6 +79,7 @@ struct max31827_state {
> struct mutex lock;
> struct regmap *regmap;
> bool enable;
> + struct i2c_client *client;
Only used in max31827_init_client(). Pass it as parameter
to that function.
> };
>
> static const struct regmap_config max31827_regmap = {
> @@ -361,14 +383,57 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
> return -EOPNOTSUPP;
> }
>
> -static int max31827_init_client(struct max31827_state *st)
> +static int max31827_init_client(struct max31827_state *st,
> + struct fwnode_handle *fwnode)
> {
> + bool prop;
> + u32 data, lsb_idx;
> + unsigned int res = 0;
> + int ret;
> +
> st->enable = true;
> + res |= MAX31827_DEVICE_ENABLE(1);
> +
> + res |= MAX31827_CONFIGURATION_RESOLUTION_MASK;
> +
> + 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 (fwnode_property_present(fwnode, "adi,alarm-pol")) {
> + ret = fwnode_property_read_u32(fwnode, "adi,alarm-pol", &data);
> + if (ret)
> + return ret;
>
> - return regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
> - MAX31827_CONFIGURATION_1SHOT_MASK |
> - MAX31827_CONFIGURATION_CNV_RATE_MASK,
> - MAX31827_DEVICE_ENABLE(1));
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
> + } else {
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
> + MAX31827_ALRM_POL_LOW);
> + }
> +
> + if (fwnode_property_present(fwnode, "adi,fault-q")) {
> + ret = fwnode_property_read_u32(fwnode, "adi,fault-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");
This is misleading. It is not "Invalid data in fault queue",
it is an invalid value in the "adi,fault-q" property.
> + return -EOPNOTSUPP;
return -EINVAL;
As you state yourself, this is invalid data, not an unsupported operation.
> + }
> +
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK, lsb_idx);
> + } else {
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
> + MAX31827_FLT_Q_1);
> + }
> +
> + return regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, res);
> }
>
> static const struct hwmon_channel_info *max31827_info[] = {
> @@ -396,6 +461,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 +478,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);
Both fwnode and client are only used in that function.
Actually, client is only used to get client->dev.
Just pass dev and st, get fwnode in max31827_init_client(),
and use dev directly there.
Guenter
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v4 4/7] hwmon: max31827: Handle new properties from the devicetree
2023-09-19 9:34 ` [PATCH v4 4/7] hwmon: max31827: Handle new properties from the devicetree Daniel Matyas
2023-10-25 22:01 ` Guenter Roeck
@ 2023-10-26 0:45 ` Guenter Roeck
1 sibling, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2023-10-26 0:45 UTC (permalink / raw)
To: Daniel Matyas; +Cc: no To-header on input <
On Tue, Sep 19, 2023 at 12:34:52PM +0300, Daniel Matyas wrote:
> Used fwnode to retrieve data from the devicetree in the init_client
> function.
>
> If the uint32 properties are not present, the default values are used
> for max31827 chip.
>
> Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
> ---
[ ... ]
>
> +#define MAX31827_ALRM_POL_LOW 0x0
> +#define MAX31827_FLT_Q_1 0x0
> +
[ ... ]
> - return regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
> - MAX31827_CONFIGURATION_1SHOT_MASK |
> - MAX31827_CONFIGURATION_CNV_RATE_MASK,
> - MAX31827_DEVICE_ENABLE(1));
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
> + } else {
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
> + MAX31827_ALRM_POL_LOW);
Since MAX31827_ALRM_POL_LOW is 0, this code doesn't really do
anything and just pollutes the code. Maybe that is acceptable
or even common in other subsystems, but I just find it confusing
and won't accept it. If a field is zero, leave it at that and
don't touch it.
> + }
> +
> + if (fwnode_property_present(fwnode, "adi,fault-q")) {
> + ret = fwnode_property_read_u32(fwnode, "adi,fault-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 {
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
> + MAX31827_FLT_Q_1);
Same here and everywhere else where I missed it.
Guenter
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v4 5/7] hwmon: max31827: Add support for max31828 and max31829
2023-09-19 9:34 [PATCH v4 1/7] hwmon: max31827: Make code cleaner Daniel Matyas
` (2 preceding siblings ...)
2023-09-19 9:34 ` [PATCH v4 4/7] hwmon: max31827: Handle new properties from the devicetree Daniel Matyas
@ 2023-09-19 9:34 ` Daniel Matyas
2023-09-19 9:34 ` [PATCH v4 6/7] hwmon: max31827: Update bits with shutdown_write() Daniel Matyas
` (2 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Daniel Matyas @ 2023-09-19 9:34 UTC (permalink / raw)
Cc: Daniel Matyas, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-hwmon,
devicetree, linux-kernel, linux-doc
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>
---
v3 -> v4: No change.
v3: Added patch.
drivers/hwmon/max31827.c | 68 +++++++++++++++++++++++++++++++++-------
1 file changed, 57 insertions(+), 11 deletions(-)
diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
index 2bddca60666d..d4127440ee32 100644
--- a/drivers/hwmon/max31827.c
+++ b/drivers/hwmon/max31827.c
@@ -46,12 +46,17 @@
#define MAX31827_12_BIT_CNV_TIME 140
#define MAX31827_ALRM_POL_LOW 0x0
+#define MAX31827_ALRM_POL_HIGH 0x1
+
#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,
@@ -383,12 +388,21 @@ static int max31827_write(struct device *dev, enum hwmon_sensor_types type,
return -EOPNOTSUPP;
}
+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;
@@ -402,6 +416,11 @@ static int max31827_init_client(struct max31827_state *st,
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,alarm-pol")) {
ret = fwnode_property_read_u32(fwnode, "adi,alarm-pol", &data);
if (ret)
@@ -409,8 +428,19 @@ static int max31827_init_client(struct max31827_state *st,
res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
} else {
- res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
- MAX31827_ALRM_POL_LOW);
+ switch (type) {
+ case max31827:
+ case max31828:
+ res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
+ MAX31827_ALRM_POL_LOW);
+ break;
+ case max31829:
+ res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
+ MAX31827_ALRM_POL_HIGH);
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
}
if (fwnode_property_present(fwnode, "adi,fault-q")) {
@@ -429,8 +459,19 @@ static int max31827_init_client(struct max31827_state *st,
res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK, lsb_idx);
} else {
- res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
- MAX31827_FLT_Q_1);
+ 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);
@@ -492,14 +533,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
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 6/7] hwmon: max31827: Update bits with shutdown_write()
2023-09-19 9:34 [PATCH v4 1/7] hwmon: max31827: Make code cleaner Daniel Matyas
` (3 preceding siblings ...)
2023-09-19 9:34 ` [PATCH v4 5/7] hwmon: max31827: Add support for max31828 and max31829 Daniel Matyas
@ 2023-09-19 9:34 ` Daniel Matyas
2023-09-19 9:34 ` [PATCH v4 7/7] hwmon: max31827: Add custom attribute for resolution Daniel Matyas
2023-10-25 21:43 ` [PATCH v4 1/7] hwmon: max31827: Make code cleaner Guenter Roeck
6 siblings, 0 replies; 13+ messages in thread
From: Daniel Matyas @ 2023-09-19 9:34 UTC (permalink / raw)
Cc: Daniel Matyas, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-hwmon,
devicetree, linux-kernel, linux-doc
Added 'mask' parameter to the shutdown_write() function. Now it can
either write or update bits, depending on the value of mask. This is
needed, because for alarms a write is necessary, but for resolution only
the resolution bits should be updated.
Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
---
v4: Added patch.
drivers/hwmon/max31827.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
index d4127440ee32..957d898978fb 100644
--- a/drivers/hwmon/max31827.c
+++ b/drivers/hwmon/max31827.c
@@ -94,7 +94,7 @@ static const struct regmap_config max31827_regmap = {
};
static int shutdown_write(struct max31827_state *st, unsigned int reg,
- unsigned int val)
+ unsigned int mask, unsigned int val)
{
unsigned int cfg;
unsigned int cnv_rate;
@@ -111,7 +111,10 @@ static int shutdown_write(struct max31827_state *st, unsigned int reg,
mutex_lock(&st->lock);
if (!st->enable) {
- ret = regmap_write(st->regmap, reg, val);
+ if (!mask)
+ ret = regmap_write(st->regmap, reg, val);
+ else
+ ret = regmap_update_bits(st->regmap, reg, mask, val);
goto unlock;
}
@@ -126,7 +129,11 @@ static int shutdown_write(struct max31827_state *st, unsigned int reg,
if (ret)
goto unlock;
- ret = regmap_write(st->regmap, reg, val);
+ if (!mask)
+ ret = regmap_write(st->regmap, reg, val);
+ else
+ ret = regmap_update_bits(st->regmap, reg, mask, val);
+
if (ret)
goto unlock;
@@ -144,7 +151,7 @@ static int write_alarm_val(struct max31827_state *st, unsigned int reg,
{
val = MAX31827_M_DGR_TO_16_BIT(val);
- return shutdown_write(st, reg, val);
+ return shutdown_write(st, reg, 0, val);
}
static umode_t max31827_is_visible(const void *state,
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v4 7/7] hwmon: max31827: Add custom attribute for resolution
2023-09-19 9:34 [PATCH v4 1/7] hwmon: max31827: Make code cleaner Daniel Matyas
` (4 preceding siblings ...)
2023-09-19 9:34 ` [PATCH v4 6/7] hwmon: max31827: Update bits with shutdown_write() Daniel Matyas
@ 2023-09-19 9:34 ` Daniel Matyas
2023-10-25 21:43 ` [PATCH v4 1/7] hwmon: max31827: Make code cleaner Guenter Roeck
6 siblings, 0 replies; 13+ messages in thread
From: Daniel Matyas @ 2023-09-19 9:34 UTC (permalink / raw)
Cc: Daniel Matyas, Jean Delvare, Guenter Roeck, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jonathan Corbet, linux-hwmon,
devicetree, linux-kernel, linux-doc
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>
---
v3 -> v4: No change.
v2 -> v3: Fixed indentation problems in .rst.
v1 -> v2: Changed subject. Separated patch. Removed timeout sysfs
attribute and kept only resolution. Added temp1_ prefix to resolution.
Changed value of resolution from bits to milli-degrees Celsius. Added
appropriate documentation.
Documentation/hwmon/max31827.rst | 29 ++++++--
drivers/hwmon/max31827.c | 121 ++++++++++++++++++++++++++++---
2 files changed, 134 insertions(+), 16 deletions(-)
diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst
index a8bbfb85dd02..44ab9dc064cb 100644
--- a/Documentation/hwmon/max31827.rst
+++ b/Documentation/hwmon/max31827.rst
@@ -90,11 +90,28 @@ 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 fil 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
@@ -117,4 +134,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 957d898978fb..0c2964cf4ba1 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_ALRM_POL_LOW 0x0
@@ -77,6 +80,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.
@@ -84,6 +108,8 @@ struct max31827_state {
struct mutex lock;
struct regmap *regmap;
bool enable;
+ unsigned int resolution;
+ unsigned int update_interval;
struct i2c_client *client;
};
@@ -101,9 +127,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.
@@ -221,9 +247,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);
@@ -381,10 +416,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;
@@ -395,6 +434,68 @@ 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;
+ int ret;
+
+ ret = kstrtouint(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ /*
+ * Convert the desired resolution into register
+ * bits. idx is already initialized with 0.
+ *
+ * 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 = shutdown_write(st, MAX31827_CONFIGURATION_REG,
+ MAX31827_CONFIGURATION_RESOLUTION_MASK,
+ FIELD_PREP(MAX31827_CONFIGURATION_RESOLUTION_MASK,
+ idx));
+
+ 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 },
@@ -535,7 +636,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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v4 1/7] hwmon: max31827: Make code cleaner
2023-09-19 9:34 [PATCH v4 1/7] hwmon: max31827: Make code cleaner Daniel Matyas
` (5 preceding siblings ...)
2023-09-19 9:34 ` [PATCH v4 7/7] hwmon: max31827: Add custom attribute for resolution Daniel Matyas
@ 2023-10-25 21:43 ` Guenter Roeck
6 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2023-10-25 21:43 UTC (permalink / raw)
To: Daniel Matyas; +Cc: no To-header on input <
On Tue, Sep 19, 2023 at 12:34:49PM +0300, Daniel Matyas wrote:
> 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>
> ---
Applied, with one change.
> @@ -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;
Changing the return value from -EINVAL to -EOPNOTSUPP was inappropriate
here. This needs to return -EINVAL because it is the result of an
invalid value provided by userspace, not the result of an unsupported
operation.
Guenter
^ permalink raw reply [flat|nested] 13+ messages in thread