* [PATCH v3 2/5] dt-bindings: hwmon: Add possible new properties to max31827 bindings
2023-09-14 7:59 [PATCH v3 1/5] hwmon: max31827: Make code cleaner Daniel Matyas
@ 2023-09-14 7:59 ` Daniel Matyas
2023-09-14 14:42 ` Conor Dooley
2023-09-14 7:59 ` [PATCH v3 3/5] hwmon: max31827: Handle new properties from the devicetree Daniel Matyas
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Daniel Matyas @ 2023-09-14 7:59 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>
---
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 | 35 +++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
index 2dc8b07b4d3b..6bde71bdb8dd 100644
--- a/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
@@ -32,6 +32,37 @@ properties:
Must have values in the interval (1.6V; 3.6V) in order for the device to
function correctly.
+ adi,comp-int:
+ description:
+ If present interrupt mode is used. If not present comparator mode is used
+ (default).
+ type: boolean
+
+ adi,alrm-pol:
+ description:
+ Sets the alarms active state.
+ - 0 = active low
+ - 1 = active high
+ For max31827 and max31828 the default alarm polarity is low. For max31829
+ it is high.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1]
+
+ adi,flt-q:
+ description:
+ Select how many consecutive temperature faults must occur before
+ overtemperature or undertemperature faults are indicated in the
+ corresponding status bits.
+ For max31827 default fault queue is 1. For max31828 and max31829 it is 4.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [1, 2, 4, 8]
+
+ adi,timeout-enable:
+ description:
+ Enables timeout. Bus timeout resets the I2C-compatible interface when SCL
+ is low for more than 30ms (nominal).
+ type: boolean
+
required:
- compatible
- reg
@@ -49,6 +80,10 @@ examples:
compatible = "adi,max31827";
reg = <0x42>;
vref-supply = <®_vdd>;
+ adi,comp-int;
+ adi,alrm-pol = <0>;
+ adi,flt-q = <1>;
+ adi,timeout-enable;
};
};
...
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 2/5] dt-bindings: hwmon: Add possible new properties to max31827 bindings
2023-09-14 7:59 ` [PATCH v3 2/5] dt-bindings: hwmon: Add possible new properties to max31827 bindings Daniel Matyas
@ 2023-09-14 14:42 ` Conor Dooley
2023-09-15 15:31 ` Matyas, Daniel
0 siblings, 1 reply; 11+ messages in thread
From: Conor Dooley @ 2023-09-14 14:42 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: 2938 bytes --]
On Thu, Sep 14, 2023 at 10:59:45AM +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>
> ---
>
> 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 | 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:
Characters are not at a premium, is there a reason not to use the full
words? "flt-q" in particular would be quite cryptic if I saw it in a
dts.
> + 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.
This constraint can be expressed in the binding, rather than in free
form text like done here. Ditto below.
Thanks,
Conor.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1]
> +
> + adi,flt-q:
> + description:
> + Select how many consecutive temperature faults must occur before
> + overtemperature or undertemperature faults are indicated in the
> + corresponding status bits.
> + For max31827 default fault queue is 1. For max31828 and max31829 it is 4.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [1, 2, 4, 8]
> +
> + adi,timeout-enable:
> + description:
> + Enables timeout. Bus timeout resets the I2C-compatible interface when SCL
> + is low for more than 30ms (nominal).
> + type: boolean
> +
> required:
> - compatible
> - reg
> @@ -49,6 +80,10 @@ examples:
> compatible = "adi,max31827";
> reg = <0x42>;
> vref-supply = <®_vdd>;
> + adi,comp-int;
> + adi,alrm-pol = <0>;
> + adi,flt-q = <1>;
> + adi,timeout-enable;
> };
> };
> ...
> --
> 2.34.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v3 2/5] dt-bindings: hwmon: Add possible new properties to max31827 bindings
2023-09-14 14:42 ` Conor Dooley
@ 2023-09-15 15:31 ` Matyas, Daniel
2023-09-15 15:58 ` Conor Dooley
0 siblings, 1 reply; 11+ messages in thread
From: Matyas, Daniel @ 2023-09-15 15:31 UTC (permalink / raw)
To: Conor Dooley
Cc: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, linux-hwmon@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org
> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Thursday, September 14, 2023 5:42 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 v3 2/5] dt-bindings: hwmon: Add possible new
> properties to max31827 bindings
>
> [External]
>
> On Thu, Sep 14, 2023 at 10:59:45AM +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>
> > ---
> >
> > 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 | 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:
>
> Characters are not at a premium, is there a reason not to use the full
> words? "flt-q" in particular would be quite cryptic if I saw it in a dts.
>
> > + 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.
>
> This constraint can be expressed in the binding, rather than in free form
> text like done here. Ditto below.
>
> Thanks,
> Conor.
>
Ok, but how? The default values are different depending on the compatible string. I searched for similar examples, but I found nothing...
Some bindings use 'default: ' to declare the default values, but this is the default for every chip.
Kind regards,
Daniel
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [0, 1]
> > +
> > + adi,flt-q:
> > + description:
> > + Select how many consecutive temperature faults must occur before
> > + overtemperature or undertemperature faults are indicated in the
> > + corresponding status bits.
> > + For max31827 default fault queue is 1. For max31828 and
> max31829 it is 4.
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + enum: [1, 2, 4, 8]
> > +
> > + adi,timeout-enable:
> > + description:
> > + Enables timeout. Bus timeout resets the I2C-compatible interface
> when SCL
> > + is low for more than 30ms (nominal).
> > + type: boolean
> > +
> > required:
> > - compatible
> > - reg
> > @@ -49,6 +80,10 @@ examples:
> > compatible = "adi,max31827";
> > reg = <0x42>;
> > vref-supply = <®_vdd>;
> > + adi,comp-int;
> > + adi,alrm-pol = <0>;
> > + adi,flt-q = <1>;
> > + adi,timeout-enable;
> > };
> > };
> > ...
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/5] dt-bindings: hwmon: Add possible new properties to max31827 bindings
2023-09-15 15:31 ` Matyas, Daniel
@ 2023-09-15 15:58 ` Conor Dooley
0 siblings, 0 replies; 11+ messages in thread
From: Conor Dooley @ 2023-09-15 15:58 UTC (permalink / raw)
To: Matyas, Daniel
Cc: Jean Delvare, Guenter Roeck, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Corbet, linux-hwmon@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]
On Fri, Sep 15, 2023 at 03:31:13PM +0000, Matyas, Daniel wrote:
> > -----Original Message-----
> > From: Conor Dooley <conor@kernel.org>
> > On Thu, Sep 14, 2023 at 10:59:45AM +0300, Daniel Matyas wrote:
> > > + adi,alrm-pol:
> >
> > Characters are not at a premium, is there a reason not to use the full
> > words? "flt-q" in particular would be quite cryptic if I saw it in a dts.
> >
> > > + 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.
> >
> > This constraint can be expressed in the binding, rather than in free form
> > text like done here. Ditto below.
> Ok, but how? The default values are different depending on the compatible string. I searched for similar examples, but I found nothing...
>
> Some bindings use 'default: ' to declare the default values, but this is the default for every chip.
Something like
allOf:
- if:
properties:
compatible:
contains:
const: adi,max31829
then:
properties:
adi,alrm-pol:
default: 1
else:
properties:
adi,alrm-pol:
default: 0
Cheers,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 3/5] hwmon: max31827: Handle new properties from the devicetree
2023-09-14 7:59 [PATCH v3 1/5] hwmon: max31827: Make code cleaner Daniel Matyas
2023-09-14 7:59 ` [PATCH v3 2/5] dt-bindings: hwmon: Add possible new properties to max31827 bindings Daniel Matyas
@ 2023-09-14 7:59 ` Daniel Matyas
2023-09-14 7:59 ` [PATCH v3 4/5] hwmon: max31827: Add support for max31828 and max31829 Daniel Matyas
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Daniel Matyas @ 2023-09-14 7:59 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>
---
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..41c440b8a474 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,alrm-pol")) {
+ ret = fwnode_property_read_u32(fwnode, "adi,alrm-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,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 {
+ 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] 11+ messages in thread* [PATCH v3 4/5] hwmon: max31827: Add support for max31828 and max31829
2023-09-14 7:59 [PATCH v3 1/5] hwmon: max31827: Make code cleaner Daniel Matyas
2023-09-14 7:59 ` [PATCH v3 2/5] dt-bindings: hwmon: Add possible new properties to max31827 bindings Daniel Matyas
2023-09-14 7:59 ` [PATCH v3 3/5] hwmon: max31827: Handle new properties from the devicetree Daniel Matyas
@ 2023-09-14 7:59 ` Daniel Matyas
2023-09-14 7:59 ` [PATCH v3 5/5] hwmon: max31827: Add custom attribute for resolution Daniel Matyas
2023-09-15 23:26 ` [PATCH v3 1/5] hwmon: max31827: Make code cleaner Guenter Roeck
4 siblings, 0 replies; 11+ messages in thread
From: Daniel Matyas @ 2023-09-14 7:59 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: 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 41c440b8a474..1f29a1f5759d 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,alrm-pol")) {
ret = fwnode_property_read_u32(fwnode, "adi,alrm-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,flt-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] 11+ messages in thread* [PATCH v3 5/5] hwmon: max31827: Add custom attribute for resolution
2023-09-14 7:59 [PATCH v3 1/5] hwmon: max31827: Make code cleaner Daniel Matyas
` (2 preceding siblings ...)
2023-09-14 7:59 ` [PATCH v3 4/5] hwmon: max31827: Add support for max31828 and max31829 Daniel Matyas
@ 2023-09-14 7:59 ` Daniel Matyas
2023-09-15 23:26 ` [PATCH v3 1/5] hwmon: max31827: Make code cleaner Guenter Roeck
4 siblings, 0 replies; 11+ messages in thread
From: Daniel Matyas @ 2023-09-14 7:59 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>
---
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 | 128 ++++++++++++++++++++++++++++---
2 files changed, 141 insertions(+), 16 deletions(-)
diff --git a/Documentation/hwmon/max31827.rst b/Documentation/hwmon/max31827.rst
index a8bbfb85dd02..d86225a31961 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 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
@@ -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 1f29a1f5759d..848a0f3fd821 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
@@ -54,6 +57,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 };
@@ -77,6 +81,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 +109,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 +128,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.
@@ -214,9 +241,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);
@@ -374,10 +410,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;
@@ -388,6 +428,74 @@ 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 },
@@ -528,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] 11+ messages in thread* Re: [PATCH v3 1/5] hwmon: max31827: Make code cleaner
2023-09-14 7:59 [PATCH v3 1/5] hwmon: max31827: Make code cleaner Daniel Matyas
` (3 preceding siblings ...)
2023-09-14 7:59 ` [PATCH v3 5/5] hwmon: max31827: Add custom attribute for resolution Daniel Matyas
@ 2023-09-15 23:26 ` Guenter Roeck
2023-09-18 9:25 ` Matyas, Daniel
4 siblings, 1 reply; 11+ messages in thread
From: Guenter Roeck @ 2023-09-15 23:26 UTC (permalink / raw)
To: Daniel Matyas
Cc: Jean Delvare, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Corbet, linux-hwmon, devicetree, linux-kernel, linux-doc
On 9/14/23 00:59, Daniel Matyas wrote:
> Now the wait time for one-shot is 140ms, instead of the old 141
> (removed the 1ms error).
>
It was explicitly documented that the wait time was 140 + 1 milli-seconds,
presumably to be sure that the conversion is really complete.
Why was this an error ? It was _documented_ that way.
Guenter
> 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>
> ---
>
> v2 -> v3: No change.
>
> v2: Added patch.
>
> 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;
>
^ permalink raw reply [flat|nested] 11+ messages in thread* RE: [PATCH v3 1/5] hwmon: max31827: Make code cleaner
2023-09-15 23:26 ` [PATCH v3 1/5] hwmon: max31827: Make code cleaner Guenter Roeck
@ 2023-09-18 9:25 ` Matyas, Daniel
2023-09-18 14:08 ` Guenter Roeck
0 siblings, 1 reply; 11+ messages in thread
From: Matyas, Daniel @ 2023-09-18 9:25 UTC (permalink / raw)
To: Guenter Roeck
Cc: Jean Delvare, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Corbet, linux-hwmon@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org
> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Saturday, September 16, 2023 2:26 AM
> To: Matyas, Daniel <Daniel.Matyas@analog.com>
> Cc: Jean Delvare <jdelvare@suse.com>; 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 v3 1/5] hwmon: max31827: Make code cleaner
>
> [External]
>
> On 9/14/23 00:59, Daniel Matyas wrote:
> > Now the wait time for one-shot is 140ms, instead of the old 141
> > (removed the 1ms error).
> >
>
> It was explicitly documented that the wait time was 140 + 1 milli-seconds,
> presumably to be sure that the conversion is really complete.
>
> Why was this an error ? It was _documented_ that way.
>
> Guenter
>
Well... actually I developed the driver initially and I wrote the documentation, so I know. I decided to remove the error milli-second, because I realized, it isn't really needed. There is no reference about it in the documentation of the chip, and frankly, I didn’t actually encounter any error which would need the 1 milli-second.
This way, the wait time is more exact and the correspondence with the chip documentation becomes quite straightforward.
Daniel
> > 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>
> > ---
> >
> > v2 -> v3: No change.
> >
> > v2: Added patch.
> >
> > 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;
> >
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 1/5] hwmon: max31827: Make code cleaner
2023-09-18 9:25 ` Matyas, Daniel
@ 2023-09-18 14:08 ` Guenter Roeck
0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2023-09-18 14:08 UTC (permalink / raw)
To: Matyas, Daniel
Cc: Jean Delvare, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Corbet, linux-hwmon@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org
On 9/18/23 02:25, Matyas, Daniel wrote:
>
>
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Saturday, September 16, 2023 2:26 AM
>> To: Matyas, Daniel <Daniel.Matyas@analog.com>
>> Cc: Jean Delvare <jdelvare@suse.com>; 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 v3 1/5] hwmon: max31827: Make code cleaner
>>
>> [External]
>>
>> On 9/14/23 00:59, Daniel Matyas wrote:
>>> Now the wait time for one-shot is 140ms, instead of the old 141
>>> (removed the 1ms error).
>>>
>>
>> It was explicitly documented that the wait time was 140 + 1 milli-seconds,
>> presumably to be sure that the conversion is really complete.
>>
>> Why was this an error ? It was _documented_ that way.
>>
>> Guenter
>>
>
> Well... actually I developed the driver initially and I wrote the documentation, so I know. I decided to remove the error milli-second, because I realized, it isn't really needed. There is no reference about it in the documentation of the chip, and frankly, I didn’t actually encounter any error which would need the 1 milli-second.
>
> This way, the wait time is more exact and the correspondence with the chip documentation becomes quite straightforward.
>
This is all fine, but it is yet another example of more than one logical
change in a single patch, and it has nothing to do with the subject. It
is not a cleanup, but a functional change.
Please split _all_ logical changes into separate patches.
Guenter
^ permalink raw reply [flat|nested] 11+ messages in thread