Re: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable.
From: Guenter Roeck
Date: Sat Oct 09 2021 - 10:35:40 EST
On 10/8/21 7:58 PM, Oskar Senft wrote:
This change allows LTD and RTD inputs to be configured via
device tree bindings. If the DT bindings are not present,
the input configuration is not modified and left at HW
defaults.
Signed-off-by: Oskar Senft <osk@xxxxxxxxxx>
---
drivers/hwmon/nct7802.c | 158 ++++++++++++++++++++++++++++++++++++----
1 file changed, 144 insertions(+), 14 deletions(-)
diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
index 604af2f6103a..5a7e38e8a188 100644
--- a/drivers/hwmon/nct7802.c
+++ b/drivers/hwmon/nct7802.c
@@ -51,6 +51,23 @@ static const u8 REG_VOLTAGE_LIMIT_MSB_SHIFT[2][5] = {
#define REG_CHIP_ID 0xfe
#define REG_VERSION_ID 0xff
+/*
+ * Resistance temperature detector (RTD) modes according to 7.2.32 Mode
+ * Selection Register
+ */
+#define RTD_MODE_CURRENT 0x1
+#define RTD_MODE_THERMISTOR 0x2
+#define RTD_MODE_VOLTAGE 0x3
+
+#define MODE_RTD_MASK 0x3
+#define MODE_LTD_EN 0x40
+
+/*
+ * Bit offset for sensors modes in REG_MODE.
+ * Valid for index 0..2, indicating RTD1..3.
+ */
+#define MODE_BIT_OFFSET_RTD(index) ((index) * 2)
+
/*
* Data structures and manipulation thereof
*/
@@ -74,7 +91,9 @@ static ssize_t temp_type_show(struct device *dev,
if (ret < 0)
return ret;
- return sprintf(buf, "%u\n", (mode >> (2 * sattr->index) & 3) + 2);
+ return sprintf(buf, "%u\n",
+ ((mode >> MODE_BIT_OFFSET_RTD(sattr->index)) &
+ MODE_RTD_MASK) + 2);
Please split into two patches to simplify review. The changes from
constant to define are logically separate and should thus be in a
separate patch.
}
static ssize_t temp_type_store(struct device *dev,
@@ -94,7 +113,8 @@ static ssize_t temp_type_store(struct device *dev,
if (type < 3 || type > 4)
return -EINVAL;
err = regmap_update_bits(data->regmap, REG_MODE,
- 3 << 2 * sattr->index, (type - 2) << 2 * sattr->index);
+ MODE_RTD_MASK << MODE_BIT_OFFSET_RTD(sattr->index),
+ (type - 2) << MODE_BIT_OFFSET_RTD(sattr->index));
return err ? : count;
}
@@ -682,24 +702,24 @@ static umode_t nct7802_temp_is_visible(struct kobject *kobj,
struct device *dev = kobj_to_dev(kobj);
struct nct7802_data *data = dev_get_drvdata(dev);
unsigned int reg;
+ unsigned int rtd_mode;
int err;
err = regmap_read(data->regmap, REG_MODE, ®);
if (err < 0)
return 0;
- if (index < 10 &&
- (reg & 03) != 0x01 && (reg & 0x03) != 0x02) /* RD1 */
+ rtd_mode = (reg >> MODE_BIT_OFFSET_RTD(index / 10)) & MODE_RTD_MASK;
+ if (index >= 0 && index < 20 && /* RD1, RD 2*/
+ rtd_mode != 0x01 && rtd_mode != 0x02)
return 0;
- if (index >= 10 && index < 20 &&
- (reg & 0x0c) != 0x04 && (reg & 0x0c) != 0x08) /* RD2 */
- return 0;
- if (index >= 20 && index < 30 && (reg & 0x30) != 0x20) /* RD3 */
+ if (index >= 20 && index < 30 && rtd_mode != 0x02) /* RD3 */
return 0;
- if (index >= 30 && index < 38) /* local */
- return attr->mode;
+ if (index >= 30 && index < 38 && /* local */
+ (reg & MODE_LTD_EN) != MODE_LTD_EN)
This is just a single bit, so "!(reg & MODE_LTD_EN)" is sufficient.
+ return 0;
err = regmap_read(data->regmap, REG_PECI_ENABLE, ®);
if (err < 0)
@@ -1038,7 +1058,118 @@ static const struct regmap_config nct7802_regmap_config = {
.volatile_reg = nct7802_regmap_is_volatile,
};
-static int nct7802_init_chip(struct nct7802_data *data)
+static bool nct7802_get_input_config(struct device *dev,
+ struct device_node *input, u8 *mode_mask, u8 *mode_val)
Please align continuation lines with "(".
The function should return an error code.
+{
+ u32 reg;
+ const char *type_str, *md_str;
+ u8 md;
+
+ if (!input->name || of_node_cmp(input->name, "input"))
+ return false;
+
+ if (of_property_read_u32(input, "reg", ®)) {
+ dev_err(dev, "Could not read reg value for '%s'\n",
+ input->full_name);
+ return false;
+ }
+
+ if (reg > 3) {
+ dev_err(dev,
+ "Invalid reg (%u) in '%s'\n", reg, input->full_name);
+ return false;
+ }
+
+ if (reg == 0) {
+ if (!of_device_is_available(input))
+ *mode_val &= ~MODE_LTD_EN;
+ else
+ *mode_val |= MODE_LTD_EN;
+ *mode_mask |= MODE_LTD_EN;
+ return true;
+ }
+
+ if (reg >= 1 && reg <= 3 && !of_device_is_available(input)) {
reg will always be >=1 and <=3 here.
+ *mode_val &= ~(MODE_RTD_MASK
+ << MODE_BIT_OFFSET_RTD(reg-1));
space around '-'
+ *mode_mask |= MODE_RTD_MASK
+ << MODE_BIT_OFFSET_RTD(reg-1);
Unnecessary continuation lines. There are several more of those;
I won't comment on it further. Please only use continuation lines if
the resulting line length is otherwise > 100 columns.
+ return true;
+ }
+
+ if (of_property_read_string(input, "sensor-type", &type_str)) {
+ dev_err(dev, "No type for '%s'\n", input->full_name);
+ return false;
+ }
+
+ if (!strcmp(type_str, "voltage")) {
+ *mode_val |= (RTD_MODE_VOLTAGE & MODE_RTD_MASK)
+ << MODE_BIT_OFFSET_RTD(reg-1);
+ *mode_mask |= MODE_RTD_MASK
+ << MODE_BIT_OFFSET_RTD(reg-1);
+ return true;
+ }
+
+ if (strcmp(type_str, "temperature")) {
+ dev_err(dev, "Invalid type '%s' for '%s'\n", type_str,
+ input->full_name);
+ return false;
+ }
+
+ if (reg == 3) {
+ /* RTD3 only supports thermistor mode */
+ md = RTD_MODE_THERMISTOR;
+ } else {
+ if (of_property_read_string(input, "temperature-mode",
+ &md_str)) {
+ dev_err(dev, "No mode for '%s'\n", input->full_name);
+ return false;
+ }
+
+ if (!strcmp(md_str, "thermal-diode"))
+ md = RTD_MODE_CURRENT;
+ else if (!strcmp(md_str, "thermistor"))
+ md = RTD_MODE_THERMISTOR;
+ else {
+ dev_err(dev, "Invalid mode '%s' for '%s'\n", md_str,
+ input->full_name);
+ return false;
+ }
+ }
+
+ *mode_val |= (md & MODE_RTD_MASK)
+ << MODE_BIT_OFFSET_RTD(reg-1);
space before and after '-'
+ *mode_mask |= MODE_RTD_MASK
+ << MODE_BIT_OFFSET_RTD(reg-1);
+
+ return true;
+}
+
+static int nct7802_configure_inputs(struct device *dev,
+ struct nct7802_data *data)
+{
+ bool found_input_config = false;
+ u8 mode_mask = 0, mode_val = 0;
+ struct device_node *input;
+
+ if (dev->of_node) {
+ for_each_child_of_node(dev->of_node, input) {
+ if (nct7802_get_input_config(dev, input, &mode_mask,
+ &mode_val))
+ found_input_config = true;
This is mixing errors with "dt configuration does not exist".
nct7802_get_input_config() should return an actual error if the
DT configuration is bad, and return that error to the calling code
if that is the case.
+ }
+ }
+
+ if (!found_input_config) {
+ /* Enable local temperature sensor by default */
+ mode_val |= MODE_LTD_EN;
+ mode_mask |= MODE_LTD_EN;
+ }
This can be set by default since nct7802_get_input_config()
removes it if the channel is disabled, meaning found_input_config
is really unnecessary.
+
+ return regmap_update_bits(data->regmap, REG_MODE, mode_mask, mode_val);
+}
+
+static int nct7802_init_chip(struct device *dev, struct nct7802_data *data)
{
int err;
@@ -1047,8 +1178,7 @@ static int nct7802_init_chip(struct nct7802_data *data)
if (err)
return err;
- /* Enable local temperature sensor */
- err = regmap_update_bits(data->regmap, REG_MODE, 0x40, 0x40);
+ err = nct7802_configure_inputs(dev, data);
if (err)
return err;
@@ -1074,7 +1204,7 @@ static int nct7802_probe(struct i2c_client *client)
mutex_init(&data->access_lock);
mutex_init(&data->in_alarm_lock);
- ret = nct7802_init_chip(data);
+ ret = nct7802_init_chip(dev, data);
if (ret < 0)
return ret;