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 <[email protected]>
---
v4 -> v5: Removed comment from __bf_shf() and used ffs() instead of
ffs64().
Added tabs where there was a need.
Removed i2c_client from private structure. In init_client() passed
device structure, because I only used that.
Changed error message when the data in adi,fault-q is invalid.
Fwnode is initialized in init_client().
v3 -> v4: Renamed property names to correspond with binding.
v2 -> v3: Separated patch into 2. Fixed 'WARNING: Unexpected
indentation.'
Reported-by: kernel test robot <[email protected]>
v2: Added patch.
Documentation/hwmon/max31827.rst | 48 +++++++++++++++++++-----
drivers/hwmon/max31827.c | 64 +++++++++++++++++++++++++++++---
2 files changed, 97 insertions(+), 15 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 614bbf5d25fa..7976d668ffd4 100644
--- a/drivers/hwmon/max31827.c
+++ b/drivers/hwmon/max31827.c
@@ -12,6 +12,13 @@
#include <linux/i2c.h>
#include <linux/mutex.h>
#include <linux/regmap.h>
+#include <linux/of_device.h>
+
+#define max31827__bf_shf(x) \
+ ({ \
+ typeof(x) x_ = (x); \
+ ((x_) != 0) ? __ffs(x_) : 0x0; \
+ })
#define MAX31827_T_REG 0x0
#define MAX31827_CONFIGURATION_REG 0x2
@@ -22,6 +29,11 @@
#define MAX31827_CONFIGURATION_1SHOT_MASK BIT(0)
#define MAX31827_CONFIGURATION_CNV_RATE_MASK GENMASK(3, 1)
+#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)
@@ -361,14 +373,54 @@ 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 device *dev)
{
+ struct fwnode_handle *fwnode;
+ unsigned int res = 0;
+ u32 data, lsb_idx;
+ bool prop;
+ int ret;
+
+ fwnode = dev_fwnode(dev);
+
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;
+
+ res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
+ }
+
+ 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(dev, "Invalid data in adi,fault-q\n");
+ return -EINVAL;
+ }
+
+ res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK, lsb_idx);
+ }
- return regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
- MAX31827_CONFIGURATION_1SHOT_MASK |
- MAX31827_CONFIGURATION_CNV_RATE_MASK,
- MAX31827_DEVICE_ENABLE(1));
+ return regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, res);
}
static const struct hwmon_channel_info *max31827_info[] = {
@@ -412,7 +464,7 @@ 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);
+ err = max31827_init_client(st, dev);
if (err)
return err;
--
2.34.1
When adi,flt-q and/or adi,alrm-pol are not mentioned,
the default configuration is loaded.
Signed-off-by: Daniel Matyas <[email protected]>
---
v4 -> v5: Passed i2c_client to init_client(), because I needed it to
retrieve device id.
Used a simple if to load default configuration. No more switch.
v3 -> v4: No change.
v3: Added patch.
drivers/hwmon/max31827.c | 51 +++++++++++++++++++++++++++++++---------
1 file changed, 40 insertions(+), 11 deletions(-)
diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
index 7976d668ffd4..446232fa1710 100644
--- a/drivers/hwmon/max31827.c
+++ b/drivers/hwmon/max31827.c
@@ -39,10 +39,15 @@
#define MAX31827_12_BIT_CNV_TIME 140
+#define MAX31827_ALRM_POL_HIGH 0x1
+#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,
@@ -373,12 +378,22 @@ 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 device *dev)
+ struct i2c_client *client)
{
+ struct device *dev = &client->dev;
struct fwnode_handle *fwnode;
unsigned int res = 0;
u32 data, lsb_idx;
+ enum chips type;
bool prop;
int ret;
@@ -395,13 +410,20 @@ 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 (dev->of_node)
+ type = (enum chips)of_device_get_match_data(dev);
+ else
+ type = i2c_match_id(max31827_i2c_ids, client)->driver_data;
+
if (fwnode_property_present(fwnode, "adi,alarm-pol")) {
ret = fwnode_property_read_u32(fwnode, "adi,alarm-pol", &data);
if (ret)
return ret;
res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
- }
+ } else if (type == max31829)
+ res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
+ MAX31827_ALRM_POL_HIGH);
if (fwnode_property_present(fwnode, "adi,fault-q")) {
ret = fwnode_property_read_u32(fwnode, "adi,fault-q", &data);
@@ -418,7 +440,9 @@ static int max31827_init_client(struct max31827_state *st,
}
res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK, lsb_idx);
- }
+ } else if ((type == max31828) || (type == max31829))
+ res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
+ MAX31827_FLT_Q_4);
return regmap_write(st->regmap, MAX31827_CONFIGURATION_REG, res);
}
@@ -464,7 +488,7 @@ 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, dev);
+ err = max31827_init_client(st, client);
if (err)
return err;
@@ -475,14 +499,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
-----Original Message-----
From: Matyas, Daniel
Sent: Friday, October 27, 2023 4:01 PM
To: Guenter Roeck <[email protected]>
Cc: no To-header on input <;>; Jean Delvare <[email protected]>; Jonathan Corbet <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: RE: [PATCH v5 2/4] hwmon: max31827: Add support for max31828 and max31829
> -----Original Message-----
> From: Guenter Roeck <[email protected]> On Behalf Of Guenter Roeck
> Sent: Friday, October 27, 2023 8:02 AM
> To: Matyas, Daniel <[email protected]>
> Cc: no To-header on input <;>; Jean Delvare <[email protected]>;
> Jonathan Corbet <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v5 2/4] hwmon: max31827: Add support for
> max31828 and max31829
>
> [External]
>
> On Thu, Oct 26, 2023 at 05:44:02PM +0300, Daniel Matyas wrote:
> > When adi,flt-q and/or adi,alrm-pol are not mentioned, the default
> > configuration is loaded.
> >
> That isn't really a complete patch description.
> It should include (even if repeated) that support for additional chips
> is added.
>
> > Signed-off-by: Daniel Matyas <[email protected]>
> > ---
> >
> > v4 -> v5: Passed i2c_client to init_client(), because I needed it to
> > retrieve device id.
> > Used a simple if to load default configuration. No more switch.
> >
> > v3 -> v4: No change.
> >
> > v3: Added patch.
> >
> > drivers/hwmon/max31827.c | 51
> > +++++++++++++++++++++++++++++++---------
> > 1 file changed, 40 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
> index
> > 7976d668ffd4..446232fa1710 100644
> > --- a/drivers/hwmon/max31827.c
> > +++ b/drivers/hwmon/max31827.c
> > @@ -39,10 +39,15 @@
> >
> > #define MAX31827_12_BIT_CNV_TIME 140
> >
> > +#define MAX31827_ALRM_POL_HIGH 0x1
> > +#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,
> > @@ -373,12 +378,22 @@ 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 device *dev)
> > + struct i2c_client *client)
> > {
> > + struct device *dev = &client->dev;
> > struct fwnode_handle *fwnode;
> > unsigned int res = 0;
> > u32 data, lsb_idx;
> > + enum chips type;
> > bool prop;
> > int ret;
> >
> > @@ -395,13 +410,20 @@ 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 (dev->of_node)
> > + type = (enum chips)of_device_get_match_data(dev);
> > + else
> > + type = i2c_match_id(max31827_i2c_ids, client)-
> >driver_data;
> > +
>
> This should be something like
>
> type = (enum chips)(uintptr_t)device_get_match_data(dev);
>
> though that requires that the enum values start with 1. This avoids
> having to pass client to the function and is more generic.
>
> > if (fwnode_property_present(fwnode, "adi,alarm-pol")) {
> > ret = fwnode_property_read_u32(fwnode, "adi,alarm-
> pol", &data);
> > if (ret)
> > return ret;
> >
> > res |=
> FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
> > - }
> > + } else if (type == max31829)
>
> Not sure why checkpatch ignores this (maybe because of 'else if'), but
> the else path should also be in {}.
>
> But why is this only for max31829 ? I mean, sure, the default for that
> chip is different, but that doesn't mean the other chips have that bit
> set. There is no guarantee that the chip is in its default state when
> the driver is loaded.
>
> > + res |=
> FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
> > + MAX31827_ALRM_POL_HIGH);
> >
> > if (fwnode_property_present(fwnode, "adi,fault-q")) {
> > ret = fwnode_property_read_u32(fwnode, "adi,fault-q",
> &data); @@
> > -418,7 +440,9 @@ static int max31827_init_client(struct
> max31827_state *st,
> > }
> >
> > res |=
> FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK, lsb_idx);
> > - }
> > + } else if ((type == max31828) || (type == max31829))
>
> I _really_ dislike the notion of excessive ( ). Also, {} for the else branch.
>
> I also don't understand why that would be chip specific. I don't see
> anything along that line in the datasheet.
>
> Ah, wait ... I guess that is supposed to reflect the chip default.
> I don't see why the chip default makes a difference - a well defined
> default must be set either way. Again, there is no guarantee that the
> chip is in its default state when the driver is loaded.
The well defined default was set in v4, but I deleted it, because the default value in hex for max31827 and max31828 alarm polarity, and max31827 fault queue is 0x0. I had 2 #defines for these values, but you said:
" Since MAX31827_ALRM_POL_LOW is 0, this code doesn't really do anything and just pollutes the code. "
So, I thought I should remove it altogether, since res is set to 0 in the beginning and the default value of these chips (i.e. 0) is implicitly set.
>
> Also, why are the default values added in this patch and not in the
> previous patch ?
>
In v4 these default values were set in the previous patch.
> > + res |=
> FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
> > + MAX31827_FLT_Q_4);
> >
> > return regmap_write(st->regmap,
> MAX31827_CONFIGURATION_REG, res); }
> > @@ -464,7 +488,7 @@ 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, dev);
> > + err = max31827_init_client(st, client);
> > if (err)
> > return err;
> >
> > @@ -475,14 +499,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);
On 10/27/23 06:00, Matyas, Daniel wrote:
[ ... ]
>> I also don't understand why that would be chip specific. I don't see
>> anything along that line in the datasheet.
>>
>> Ah, wait ... I guess that is supposed to reflect the chip default.
>> I don't see why the chip default makes a difference - a well defined default
>> must be set either way. Again, there is no guarantee that the chip is in its
>> default state when the driver is loaded.
>
> The well defined default was set in v4, but I deleted it, because the default value in hex for max31827 and max31828 alarm polarity, and max31827 fault queue is 0x0. I had 2 #defines for these values, but you said:
> " Since MAX31827_ALRM_POL_LOW is 0, this code doesn't really do anything and just pollutes the code."
>
> So, I thought I should remove it altogether, since res is set to 0 in the beginning and the default value of these chips (i.e. 0) is implicitly set.
>
>>
>> Also, why are the default values added in this patch and not in the
>> previous patch ?
>>
>
> In v4 these default values were set in the previous patch.
>
I asked you (or meant to ask you) to stop overwriting 0 with 0
in a variable. I didn't mean to ask you (if I did) to stop writing
the default value into the chip. Sorry if I did; if so, that was
a misunderstanding.
Guenter
> -----Original Message-----
> From: Guenter Roeck <[email protected]> On Behalf Of Guenter Roeck
> Sent: Friday, October 27, 2023 5:52 PM
> To: Matyas, Daniel <[email protected]>
> Cc: Jean Delvare <[email protected]>; Jonathan Corbet
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v5 2/4] hwmon: max31827: Add support for
> max31828 and max31829
>
> [External]
>
> On 10/27/23 06:00, Matyas, Daniel wrote:
> [ ... ]
>
> >> I also don't understand why that would be chip specific. I don't see
> >> anything along that line in the datasheet.
> >>
> >> Ah, wait ... I guess that is supposed to reflect the chip default.
> >> I don't see why the chip default makes a difference - a well defined
> >> default must be set either way. Again, there is no guarantee that the
> >> chip is in its default state when the driver is loaded.
> >
> > The well defined default was set in v4, but I deleted it, because the
> default value in hex for max31827 and max31828 alarm polarity, and
> max31827 fault queue is 0x0. I had 2 #defines for these values, but you
> said:
> > " Since MAX31827_ALRM_POL_LOW is 0, this code doesn't really do
> anything and just pollutes the code."
> >
> > So, I thought I should remove it altogether, since res is set to 0 in the
> beginning and the default value of these chips (i.e. 0) is implicitly set.
> >
> >>
> >> Also, why are the default values added in this patch and not in the
> >> previous patch ?
> >>
> >
> > In v4 these default values were set in the previous patch.
> >
>
> I asked you (or meant to ask you) to stop overwriting 0 with 0 in a
> variable. I didn't mean to ask you (if I did) to stop writing the default value
> into the chip. Sorry if I did; if so, that was a misunderstanding.
>
> Guenter
Well, writing the default value into res, would just overwrite 0 with 0. Should I still do it?
Daniel
On 10/27/23 08:05, Matyas, Daniel wrote:
>
>
>> -----Original Message-----
>> From: Guenter Roeck <[email protected]> On Behalf Of Guenter Roeck
>> Sent: Friday, October 27, 2023 5:52 PM
>> To: Matyas, Daniel <[email protected]>
>> Cc: Jean Delvare <[email protected]>; Jonathan Corbet
>> <[email protected]>; [email protected]; linux-
>> [email protected]; [email protected]
>> Subject: Re: [PATCH v5 2/4] hwmon: max31827: Add support for
>> max31828 and max31829
>>
>> [External]
>>
>> On 10/27/23 06:00, Matyas, Daniel wrote:
>> [ ... ]
>>
>>>> I also don't understand why that would be chip specific. I don't see
>>>> anything along that line in the datasheet.
>>>>
>>>> Ah, wait ... I guess that is supposed to reflect the chip default.
>>>> I don't see why the chip default makes a difference - a well defined
>>>> default must be set either way. Again, there is no guarantee that the
>>>> chip is in its default state when the driver is loaded.
>>>
>>> The well defined default was set in v4, but I deleted it, because the
>> default value in hex for max31827 and max31828 alarm polarity, and
>> max31827 fault queue is 0x0. I had 2 #defines for these values, but you
>> said:
>>> " Since MAX31827_ALRM_POL_LOW is 0, this code doesn't really do
>> anything and just pollutes the code."
>>>
>>> So, I thought I should remove it altogether, since res is set to 0 in the
>> beginning and the default value of these chips (i.e. 0) is implicitly set.
>>>
>>>>
>>>> Also, why are the default values added in this patch and not in the
>>>> previous patch ?
>>>>
>>>
>>> In v4 these default values were set in the previous patch.
>>>
>>
>> I asked you (or meant to ask you) to stop overwriting 0 with 0 in a
>> variable. I didn't mean to ask you (if I did) to stop writing the default value
>> into the chip. Sorry if I did; if so, that was a misunderstanding.
>>
>> Guenter
>
> Well, writing the default value into res, would just overwrite 0 with 0. Should I still do it?
>
No, that is not correct. You don't know what is in the chip register.
It may not be the chip default.
Guenter