dt-bindings: hwmon: Add nct7802 bindings
Subsystems (2)
PATCH v4 1/2 dt-bindings: hwmon: Add nct7802 bindings
Document bindings for the Nuvoton NCT7802Y driver.
--- .../bindings/hwmon/nuvoton,nct7802.yaml | 142 ++++++++++++++++++ 1 file changed, 142 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml
Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml new file +142 -0
@@ -0,0 +1,142 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- + +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct7802.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton NCT7802Y Hardware Monitoring IC + +maintainers: + - Guenter Roeck <[email protected]> + +description: | + The NCT7802Y is a hardware monitor IC which supports one on-die and up to + 5 remote temperature sensors with SMBus interface. + + Datasheets: + https://www.nuvoton.com/export/resource-files/Nuvoton_NCT7802Y_Datasheet_V12.pdf + +properties: + compatible: + enum: + - nuvoton,nct7802 + + reg: + maxItems: 1 + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + input@0: + type: object + description: Local Temperature Sensor ("LTD") + properties: + reg: + const: 0 + required: + - reg + + input@1: + type: object + description: Remote Temperature Sensor or Voltage Sensor ("RTD1") + properties: + reg: + const: 1 + sensor-type: + items: + - enum: + - temperature + - voltage + temperature-mode: + items: + - enum: + - thermistor + - thermal-diode + required: + - reg + - sensor-type + + input@2: + type: object + description: Remote Temperature Sensor or Voltage Sensor ("RTD2") + properties: + reg: + const: 2 + sensor-type: + items: + - enum: + - temperature + - voltage + temperature-mode: + items: + - enum: + - thermistor + - thermal-diode + required: + - reg + - sensor-type + + input@3: + type: object + description: Remote Temperature Sensor or Voltage Sensor ("RTD3") + properties: + reg: + const: 3 + sensor-type: + items: + - enum: + - temperature + - voltage + required: + - reg + - sensor-type + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + nct7802@28 { + compatible = "nuvoton,nct7802"; + reg = <0x28>; + + #address-cells = <1>; + #size-cells = <0>; + + input@0 { /* LTD */ + reg = <0>; + status = "okay"; + }; + + input@1 { /* RTD1 */ + reg = <1>; + status = "okay"; + sensor-type = "temperature"; + temperature-mode = "thermistor"; + }; + + input@2 { /* RTD2 */ + reg = <2>; + status = "okay"; + sensor-type = "temperature"; + temperature-mode = "thermal-diode"; + }; + + input@3 { /* RTD3 */ + reg = <3>; + status = "okay"; + sensor-type = "voltage"; + }; + }; + };
Signature · 1 line
2.33.0.882.g93a45727a2-goog
PATCH v4 2/2 hwmon: (nct7802) Make temperature sensors configurable.
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.
--- drivers/hwmon/nct7802.c | 158 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 144 insertions(+), 14 deletions(-)
drivers/hwmon/nct7802.c +144 -14
@@ -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 ); } 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 ) +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) +{ +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)) { + *mode_val &= ~(MODE_RTD_MASK + <<MODE_BIT_OFFSET_RTD (reg-1 )); + *mode_mask |=MODE_RTD_MASK + <<MODE_BIT_OFFSET_RTD (reg-1 ); +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 ); + *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; + } + } + +if (!found_input_config) { +/* Enable local temperature sensor by default */ + mode_val |=MODE_LTD_EN ; + mode_mask |=MODE_LTD_EN ; + } + +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;
Signature · 1 line
2.33.0.882.g93a45727a2-goog
Re: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable.
Changes from "PATCH v2 2/2" to "PATCH v4 2/2" (v3 was sent with a typo by accident):
- Added "RTD_MODE_*" #defines for readability.
- Improved readability in "nct7802_temp_is_visible" and fixed error due to missing parentheses
- Refactored "nct7802_configure_temperature_sensors_from_device_tree" into "nct7802_get_input_config" and "nct7802_configure_inputs" for readability
- Restructured "nct7802_get_input_config" for v4 dt-bindings.
Thanks
Oskar.
Guenter Roeck
maintainerRe: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable.
In reply to Oskar SenftQuoted 39 lines · Oskar Senft
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 <[email protected]>--- drivers/hwmon/nct7802.c | 158 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 144 insertions(+), 14 deletions(-)drivers/hwmon/nct7802.c +17
-0@@ -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 */
drivers/hwmon/nct7802.c static ssize_t temp_type_show(struct device *dev, · line 96
Quoted 7 lines
drivers/hwmon/nct7802.c +3
-1@@ -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.
Quoted 32 lines
drivers/hwmon/nct7802.c +6
-4} 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 &&
drivers/hwmon/nct7802.c static umode_t nct7802_temp_is_visible(struct kobject *kobj, · line 721
Quoted 10 lines
drivers/hwmon/nct7802.c +3
-5- (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.
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1062
Quoted 11 lines
drivers/hwmon/nct7802.c +3
-1+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.
Quoted 20 lines
drivers/hwmon/nct7802.c +20
-0+{ +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; + } +
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1092
Quoted 10 lines
drivers/hwmon/nct7802.c +10
-0+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.
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1094
Quoted 2 lines
drivers/hwmon/nct7802.c +2
-0+ *mode_val &= ~(MODE_RTD_MASK + <<MODE_BIT_OFFSET_RTD (reg-1 ));
space around '-'
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1096
Quoted 2 lines
drivers/hwmon/nct7802.c +2
-0+ *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.
Quoted 35 lines
drivers/hwmon/nct7802.c +35
-0+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" ))
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1141
Quoted 10 lines
drivers/hwmon/nct7802.c +10
-0+ 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 '-'
Quoted 8 lines
drivers/hwmon/nct7802.c +8
-0+ *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)
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1159
Quoted 10 lines
drivers/hwmon/nct7802.c +10
-0+{ +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.
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1167
Quoted 8 lines
drivers/hwmon/nct7802.c +8
-0+ } + } + +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.
Quoted 28 lines
drivers/hwmon/nct7802.c +7
-3+ +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;
Re: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable.
Hi Guenter
Thanks for the review!
Quoted 7 lines
drivers/hwmon/nct7802.c static ssize_t temp_type_show(struct device *dev, · line 96
Quoted 3 lines
drivers/hwmon/nct7802.c +3
-0+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.
Ok, will do.
Quoted 4 lines
drivers/hwmon/nct7802.c static umode_t nct7802_temp_is_visible(struct kobject *kobj, · line 721
Quoted 2 lines
drivers/hwmon/nct7802.c +2
-0+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.
Ack.
Quoted 4 lines
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1062
Quoted 2 lines
drivers/hwmon/nct7802.c +2
-0+static bool nct7802_get_input_config (struct device *dev, +struct device_node *input,u8 *mode_mask,u8 *mode_val)Please align continuation lines with "(".
Oh, even if that would result in a lot of extra lines? Or just start the first argument on a new line?
Quoted 1 line
The function should return an error code.
Ok, I'll look into that.
Quoted 3 lines
Quoted 1 line
+ if (reg >= 1 && reg <= 3 && !of_device_is_available(input)) {
reg will always be >=1 and <=3 here.
Good catch!
Quoted 4 lines
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1094
Quoted 2 lines
drivers/hwmon/nct7802.c +2
-0+ *mode_val &= ~(MODE_RTD_MASK + <<MODE_BIT_OFFSET_RTD (reg-1 ));space around '-'
Oh yeah, I'm sorry. Is there a code formatter I should have run? I did run "checkpatch.pl", hoping that it would catch those.
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ *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.
Argh, yeah. After refactoring that function, I thought I caught all of them, but obviously I didn't. According to [1] we should stay within 80 columns (and use tabs that are 8 spaces wide). I assume that still applies? The rest of this code follows that rule.
Quoted 10 lines
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1159
Quoted 5 lines
drivers/hwmon/nct7802.c +5
-0+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.
Ok, I'll change that. I wasn't sure whether we'd rather configure "as much as we can" or fail completely without configuring anything. Shall we allow all of the configuration to be validated before erroring out? That would make it easier to get the DT right in one pass, but makes the code more complicated.
Quoted 9 lines
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1167
Quoted 5 lines
drivers/hwmon/nct7802.c +5
-0+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, meaningfound_input_configis really unnecessary.
Ok. Should we actually phase out the "LTD enabled by default" completely? Or is that for a future change?
Thanks
Oskar.
Re: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable.
Quoted 8 lines
Quoted 1 line
Please align continuation lines with "(".
Oh, even if that would result in a lot of extra lines? Or just start the first argument on a new line?
Quoted 1 line
space around '-'
Oh yeah, I'm sorry. Is there a code formatter I should have run? I did run "checkpatch.pl", hoping that it would catch those.
Quoted 7 lines
Quoted 3 lines
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.
Argh, yeah. After refactoring that function, I thought I caught all of them, but obviously I didn't. According to [1] we should stay within 80 columns (and use tabs that are 8 spaces wide). I assume that still applies? The rest of this code follows that rule.
For all of the above: I found that running clang-format within subl for the new code does what we want. Sorry again for that, I'll start doing that from now.
Oskar.
Guenter Roeck
maintainerRe: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable.
In reply to Oskar SenftQuoted 17 lines · Oskar Senft
Hi Guenter
Thanks for the review!
Quoted 7 lines
drivers/hwmon/nct7802.c static ssize_t temp_type_show(struct device *dev, · line 96
Quoted 3 lines
drivers/hwmon/nct7802.c +3
-0+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.
Ok, will do.
Quoted 4 lines
drivers/hwmon/nct7802.c static umode_t nct7802_temp_is_visible(struct kobject *kobj, · line 721
Quoted 2 lines
drivers/hwmon/nct7802.c +2
-0+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.
Quoted 9 lines
Ack.
Quoted 4 lines
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1062
Quoted 2 lines
drivers/hwmon/nct7802.c +2
-0+static bool nct7802_get_input_config (struct device *dev, +struct device_node *input,u8 *mode_mask,u8 *mode_val)Please align continuation lines with "(".
Oh, even if that would result in a lot of extra lines? Or just start the first argument on a new line?
I sincerely doubt that will happen with the 100-column limit, but yes unless it really doesn't work.
Quoted 6 lines
Quoted 1 line
The function should return an error code.
Ok, I'll look into that.
Quoted 3 lines
Quoted 1 line
+ if (reg >= 1 && reg <= 3 && !of_device_is_available(input)) {
reg will always be >=1 and <=3 here.
Quoted 9 lines
Good catch!
Quoted 4 lines
drivers/hwmon/nct7802.c static const struct regmap_config nct7802_regmap_config = { · line 1094
Quoted 2 lines
drivers/hwmon/nct7802.c +2
-0+ *mode_val &= ~(MODE_RTD_MASK + <<MODE_BIT_OFFSET_RTD (reg-1 ));space around '-'
Oh yeah, I'm sorry. Is there a code formatter I should have run? I did run "checkpatch.pl", hoping that it would catch those.
For some reason checkpatch doesn't always catch this.
Quoted 11 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ *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.
Argh, yeah. After refactoring that function, I thought I caught all of them, but obviously I didn't. According to [1] we should stay within 80 columns (and use tabs that are 8 spaces wide). I assume that still applies? The rest of this code follows that rule.
From checkpatch, commit bdc48fa11e46 ("checkpatch/coding-style:
deprecate 80-column warning"):
Yes, staying withing 80 columns is certainly still _preferred_. But it's not the hard limit that the checkpatch warnings imply, and other concerns can most certainly dominate.
I prefer readability over the 80 column limit.
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
drivers/hwmon/nct7802.c +5
-0+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;
Quoted 7 lines
Quoted 4 lines
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.Ok, I'll change that. I wasn't sure whether we'd rather configure "as much as we can" or fail completely without configuring anything. Shall we allow all of the configuration to be validated before erroring out?
No, bail out on the first error.
Quoted 9 lines
That would make it easier to get the DT right in one pass, but makes the code more complicated.
Quoted 6 lines
Quoted 5 lines
drivers/hwmon/nct7802.c +5
-0+if (!found_input_config) { +/* Enable local temperature sensor by default */ + mode_val |=MODE_LTD_EN ; + mode_mask |=MODE_LTD_EN ; + }
Quoted 6 lines
Quoted 3 lines
This can be set by default since
nct7802_get_input_config()removes it if the channel is disabled, meaningfound_input_configis really unnecessary.Ok. Should we actually phase out the "LTD enabled by default" completely? Or is that for a future change?
Why ? That would change code behavior and would be unexpected. Just initialize mode_val and mode_mask variables with MODE_LTD_EN.
Thanks,
Guenter
Guenter Roeck
maintainerQuoted 38 lines · Oskar Senft
Document bindings for the Nuvoton NCT7802Y driver.
Signed-off-by Oskar Senft <[email protected]>--- .../bindings/hwmon/nuvoton,nct7802.yaml | 142 ++++++++++++++++++ 1 file changed, 142 insertions(+)create mode 100644
Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yamlDocumentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml new file +24
-0@@ -0,0 +1,142 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- + +$id: http://devicetree.org/schemas/hwmon/nuvoton,nct7802.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Nuvoton NCT7802Y Hardware Monitoring IC + +maintainers: + - Guenter Roeck <[email protected]> + +description: | + The NCT7802Y is a hardware monitor IC which supports one on-die and up to + 5 remote temperature sensors with SMBus interface. + + Datasheets: + https://www.nuvoton.com/export/resource-files/Nuvoton_NCT7802Y_Datasheet_V12.pdf + +properties: + compatible: + enum: + - nuvoton,nct7802 +
Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml line 34
Quoted 10 lines
Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml new file +10
-0+ reg: + maxItems: 1 + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + input@0:
Please use "channel".
Quoted 109 lines
Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml new file +108
-0+ type: object + description: Local Temperature Sensor ("LTD") + properties: + reg: + const: 0 + required: + - reg + + input@1: + type: object + description: Remote Temperature Sensor or Voltage Sensor ("RTD1") + properties: + reg: + const: 1 + sensor-type: + items: + - enum: + - temperature + - voltage + temperature-mode: + items: + - enum: + - thermistor + - thermal-diode + required: + - reg + - sensor-type + + input@2: + type: object + description: Remote Temperature Sensor or Voltage Sensor ("RTD2") + properties: + reg: + const: 2 + sensor-type: + items: + - enum: + - temperature + - voltage + temperature-mode: + items: + - enum: + - thermistor + - thermal-diode + required: + - reg + - sensor-type + + input@3: + type: object + description: Remote Temperature Sensor or Voltage Sensor ("RTD3") + properties: + reg: + const: 3 + sensor-type: + items: + - enum: + - temperature + - voltage + required: + - reg + - sensor-type + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + nct7802@28 { + compatible = "nuvoton,nct7802"; + reg = <0x28>; + + #address-cells = <1>; + #size-cells = <0>; + + input@0 { /* LTD */ + reg = <0>; + status = "okay"; + }; + + input@1 { /* RTD1 */ + reg = <1>; + status = "okay"; + sensor-type = "temperature"; + temperature-mode = "thermistor"; + }; + + input@2 { /* RTD2 */ + reg = <2>; + status = "okay"; + sensor-type = "temperature"; + temperature-mode = "thermal-diode"; + }; + + input@3 { /* RTD3 */ + reg = <3>; + status = "okay"; + sensor-type = "voltage"; + }; + }; + };