* [PATCH v4 1/2] dt-bindings: hwmon: Add nct7802 bindings @ 2021-10-09 2:58 Oskar Senft 2021-10-09 2:58 ` [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable Oskar Senft 2021-10-09 14:12 ` [PATCH v4 1/2] dt-bindings: hwmon: Add nct7802 bindings Guenter Roeck 0 siblings, 2 replies; 8+ messages in thread From: Oskar Senft @ 2021-10-09 2:58 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck, Rob Herring, linux-hwmon, linux-kernel, devicetree Cc: Oskar Senft Document bindings for the Nuvoton NCT7802Y driver. Signed-off-by: Oskar Senft <osk@google.com> --- .../bindings/hwmon/nuvoton,nct7802.yaml | 142 ++++++++++++++++++ 1 file changed, 142 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml new file mode 100644 index 000000000000..a97b89d0d197 --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml @@ -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 <linux@roeck-us.net> + +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"; + }; + }; + }; -- 2.33.0.882.g93a45727a2-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable. 2021-10-09 2:58 [PATCH v4 1/2] dt-bindings: hwmon: Add nct7802 bindings Oskar Senft @ 2021-10-09 2:58 ` Oskar Senft 2021-10-09 12:02 ` Oskar Senft 2021-10-09 14:35 ` Guenter Roeck 2021-10-09 14:12 ` [PATCH v4 1/2] dt-bindings: hwmon: Add nct7802 bindings Guenter Roeck 1 sibling, 2 replies; 8+ messages in thread From: Oskar Senft @ 2021-10-09 2:58 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck, Rob Herring, linux-hwmon, linux-kernel, devicetree Cc: 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 <osk@google.com> --- 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); } 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; -- 2.33.0.882.g93a45727a2-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable. 2021-10-09 2:58 ` [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable Oskar Senft @ 2021-10-09 12:02 ` Oskar Senft 2021-10-09 14:35 ` Guenter Roeck 1 sibling, 0 replies; 8+ messages in thread From: Oskar Senft @ 2021-10-09 12:02 UTC (permalink / raw) To: Jean Delvare, Guenter Roeck, linux-hwmon, linux-kernel, devicetree, Rob Herring 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable. 2021-10-09 2:58 ` [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable Oskar Senft 2021-10-09 12:02 ` Oskar Senft @ 2021-10-09 14:35 ` Guenter Roeck 2021-10-09 14:50 ` Oskar Senft 1 sibling, 1 reply; 8+ messages in thread From: Guenter Roeck @ 2021-10-09 14:35 UTC (permalink / raw) To: Oskar Senft, Jean Delvare, Rob Herring, linux-hwmon, linux-kernel, devicetree 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@google.com> > --- > 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; > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable. 2021-10-09 14:35 ` Guenter Roeck @ 2021-10-09 14:50 ` Oskar Senft 2021-10-09 15:03 ` Oskar Senft 2021-10-09 15:50 ` Guenter Roeck 0 siblings, 2 replies; 8+ messages in thread From: Oskar Senft @ 2021-10-09 14:50 UTC (permalink / raw) To: Guenter Roeck Cc: Jean Delvare, Rob Herring, linux-hwmon, linux-kernel, devicetree Hi Guenter Thanks for the review! > > + 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. > > + 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. > > +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? > The function should return an error code. Ok, I'll look into that. > > + if (reg >= 1 && reg <= 3 && !of_device_is_available(input)) { > > reg will always be >=1 and <=3 here. Good catch! > > + *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. > > + *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. > > + 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. > > + 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. Ok. Should we actually phase out the "LTD enabled by default" completely? Or is that for a future change? Thanks Oskar. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable. 2021-10-09 14:50 ` Oskar Senft @ 2021-10-09 15:03 ` Oskar Senft 2021-10-09 15:50 ` Guenter Roeck 1 sibling, 0 replies; 8+ messages in thread From: Oskar Senft @ 2021-10-09 15:03 UTC (permalink / raw) To: Guenter Roeck Cc: Jean Delvare, Rob Herring, linux-hwmon, linux-kernel, devicetree > > 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? > > 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. > > 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable. 2021-10-09 14:50 ` Oskar Senft 2021-10-09 15:03 ` Oskar Senft @ 2021-10-09 15:50 ` Guenter Roeck 1 sibling, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2021-10-09 15:50 UTC (permalink / raw) To: Oskar Senft Cc: Jean Delvare, Rob Herring, linux-hwmon, linux-kernel, devicetree On 10/9/21 7:50 AM, Oskar Senft wrote: > Hi Guenter > > Thanks for the review! > >>> + 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. > >>> + 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. > >>> +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. >> The function should return an error code. > Ok, I'll look into that. > >>> + if (reg >= 1 && reg <= 3 && !of_device_is_available(input)) { >> >> reg will always be >=1 and <=3 here. > Good catch! > >>> + *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. >>> + *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. >>> + 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? No, bail out on the first error. > That would make it easier to get the DT right in one pass, but makes > the code more complicated. > >>> + 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. > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v4 1/2] dt-bindings: hwmon: Add nct7802 bindings 2021-10-09 2:58 [PATCH v4 1/2] dt-bindings: hwmon: Add nct7802 bindings Oskar Senft 2021-10-09 2:58 ` [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable Oskar Senft @ 2021-10-09 14:12 ` Guenter Roeck 1 sibling, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2021-10-09 14:12 UTC (permalink / raw) To: Oskar Senft, Jean Delvare, Rob Herring, linux-hwmon, linux-kernel, devicetree On 10/8/21 7:58 PM, Oskar Senft wrote: > Document bindings for the Nuvoton NCT7802Y driver. > > Signed-off-by: Oskar Senft <osk@google.com> > --- > .../bindings/hwmon/nuvoton,nct7802.yaml | 142 ++++++++++++++++++ > 1 file changed, 142 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml > > diff --git a/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml > new file mode 100644 > index 000000000000..a97b89d0d197 > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/nuvoton,nct7802.yaml > @@ -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 <linux@roeck-us.net> > + > +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: Please use "channel". > + 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"; > + }; > + }; > + }; > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-10-09 15:50 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-09 2:58 [PATCH v4 1/2] dt-bindings: hwmon: Add nct7802 bindings Oskar Senft 2021-10-09 2:58 ` [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable Oskar Senft 2021-10-09 12:02 ` Oskar Senft 2021-10-09 14:35 ` Guenter Roeck 2021-10-09 14:50 ` Oskar Senft 2021-10-09 15:03 ` Oskar Senft 2021-10-09 15:50 ` Guenter Roeck 2021-10-09 14:12 ` [PATCH v4 1/2] dt-bindings: hwmon: Add nct7802 bindings Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.