* [PATCH 0/2] hwmon: (sch5627) Replace hwmon_device_register()
@ 2021-04-11 16:42 W_Armin
2021-04-11 16:42 ` [PATCH 1/2] hwmon: (sch5627) Convert to hwmon_device_register_with_info() W_Armin
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: W_Armin @ 2021-04-11 16:42 UTC (permalink / raw)
To: hdegoede; +Cc: linux-hwmon, Armin Wolf
From: Armin Wolf <W_Armin@gmx.de>
When the sch5627 driver is loaded, a warning is displayed during
bootup about hwmon_device_register() being deprecated.
The first patch solves that issue, and the second patch adds
a minor optimization when reading sensor data.
Both patches have been tested on a Fujitsu Esprimo P720
and appear to work without issues.
Armin Wolf (2):
hwmon: (sch5627) Convert to hwmon_device_register_with_info()
hwmon: (sch5627) Split sch5627_update_device()
drivers/hwmon/sch5627.c | 445 ++++++++++++++++------------------------
1 file changed, 180 insertions(+), 265 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] hwmon: (sch5627) Convert to hwmon_device_register_with_info()
2021-04-11 16:42 [PATCH 0/2] hwmon: (sch5627) Replace hwmon_device_register() W_Armin
@ 2021-04-11 16:42 ` W_Armin
2021-04-11 16:42 ` [PATCH 2/2] hwmon: (sch5627) Split sch5627_update_device() W_Armin
2021-04-13 8:11 ` [PATCH 0/2] hwmon: (sch5627) Replace hwmon_device_register() Hans de Goede
2 siblings, 0 replies; 5+ messages in thread
From: W_Armin @ 2021-04-11 16:42 UTC (permalink / raw)
To: hdegoede; +Cc: linux-hwmon, Armin Wolf
From: Armin Wolf <W_Armin@gmx.de>
hwmon_device_register() is deprecated.
Convert driver to use hwmon_device_register_with_info() and
remove sysfs attributes which are now being handled by the
hwmon subsystem.
Channel handling was inspired by corsair-cpro.
Tested on a Fujitsu Esprimo P720.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/hwmon/sch5627.c | 343 +++++++++++++---------------------------
1 file changed, 111 insertions(+), 232 deletions(-)
diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
index a7e0d7bcf923..023376082ee6 100644
--- a/drivers/hwmon/sch5627.c
+++ b/drivers/hwmon/sch5627.c
@@ -12,7 +12,6 @@
#include <linux/jiffies.h>
#include <linux/platform_device.h>
#include <linux/hwmon.h>
-#include <linux/hwmon-sysfs.h>
#include <linux/err.h>
#include <linux/mutex.h>
#include "sch56xx-common.h"
@@ -192,249 +191,135 @@ static int reg_to_rpm(u16 reg)
return 5400540 / reg;
}
-static ssize_t name_show(struct device *dev, struct device_attribute *devattr,
- char *buf)
+static umode_t sch5627_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
+ int channel)
{
- return sysfs_emit(buf, "%s\n", DEVNAME);
+ return 0444;
}
-static ssize_t temp_show(struct device *dev, struct device_attribute *devattr,
- char *buf)
+static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
+ long *val)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
struct sch5627_data *data = sch5627_update_device(dev);
- int val;
+ int ret;
if (IS_ERR(data))
return PTR_ERR(data);
- val = reg_to_temp(data->temp[attr->index]);
- return sysfs_emit(buf, "%d\n", val);
-}
-
-static ssize_t temp_fault_show(struct device *dev,
- struct device_attribute *devattr, char *buf)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
- struct sch5627_data *data = sch5627_update_device(dev);
-
- if (IS_ERR(data))
- return PTR_ERR(data);
-
- return sysfs_emit(buf, "%d\n", data->temp[attr->index] == 0);
-}
-
-static ssize_t temp_max_show(struct device *dev,
- struct device_attribute *devattr, char *buf)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
- struct sch5627_data *data = dev_get_drvdata(dev);
- int val;
-
- val = reg_to_temp_limit(data->temp_max[attr->index]);
- return sysfs_emit(buf, "%d\n", val);
-}
-
-static ssize_t temp_crit_show(struct device *dev,
- struct device_attribute *devattr, char *buf)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
- struct sch5627_data *data = dev_get_drvdata(dev);
- int val;
-
- val = reg_to_temp_limit(data->temp_crit[attr->index]);
- return sysfs_emit(buf, "%d\n", val);
-}
-
-static ssize_t fan_show(struct device *dev, struct device_attribute *devattr,
- char *buf)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
- struct sch5627_data *data = sch5627_update_device(dev);
- int val;
-
- if (IS_ERR(data))
- return PTR_ERR(data);
-
- val = reg_to_rpm(data->fan[attr->index]);
- if (val < 0)
- return val;
-
- return sysfs_emit(buf, "%d\n", val);
-}
-
-static ssize_t fan_fault_show(struct device *dev,
- struct device_attribute *devattr, char *buf)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
- struct sch5627_data *data = sch5627_update_device(dev);
-
- if (IS_ERR(data))
- return PTR_ERR(data);
-
- return sysfs_emit(buf, "%d\n",
- data->fan[attr->index] == 0xffff);
-}
-
-static ssize_t fan_min_show(struct device *dev,
- struct device_attribute *devattr, char *buf)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
- struct sch5627_data *data = dev_get_drvdata(dev);
- int val = reg_to_rpm(data->fan_min[attr->index]);
- if (val < 0)
- return val;
-
- return sysfs_emit(buf, "%d\n", val);
-}
-
-static ssize_t in_show(struct device *dev, struct device_attribute *devattr,
- char *buf)
-{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
- struct sch5627_data *data = sch5627_update_device(dev);
- int val;
-
- if (IS_ERR(data))
- return PTR_ERR(data);
+ switch (type) {
+ case hwmon_temp:
+ switch (attr) {
+ case hwmon_temp_input:
+ *val = reg_to_temp(data->temp[channel]);
+ return 0;
+ case hwmon_temp_max:
+ *val = reg_to_temp_limit(data->temp_max[channel]);
+ return 0;
+ case hwmon_temp_crit:
+ *val = reg_to_temp_limit(data->temp_crit[channel]);
+ return 0;
+ case hwmon_temp_fault:
+ *val = (data->temp[channel] == 0);
+ return 0;
+ default:
+ break;
+ }
+ break;
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_input:
+ ret = reg_to_rpm(data->fan[channel]);
+ if (ret < 0)
+ return ret;
+ *val = ret;
+ return 0;
+ case hwmon_fan_min:
+ ret = reg_to_rpm(data->fan_min[channel]);
+ if (ret < 0)
+ return ret;
+ *val = ret;
+ return 0;
+ case hwmon_fan_fault:
+ *val = (data->fan[channel] == 0xffff);
+ return 0;
+ default:
+ break;
+ }
+ break;
+ case hwmon_in:
+ switch (attr) {
+ case hwmon_in_input:
+ *val = DIV_ROUND_CLOSEST(data->in[channel] * SCH5627_REG_IN_FACTOR[channel],
+ 10000);
+ return 0;
+ default:
+ break;
+ }
+ break;
+ default:
+ break;
+ }
- val = DIV_ROUND_CLOSEST(
- data->in[attr->index] * SCH5627_REG_IN_FACTOR[attr->index],
- 10000);
- return sysfs_emit(buf, "%d\n", val);
+ return -EOPNOTSUPP;
}
-static ssize_t in_label_show(struct device *dev,
- struct device_attribute *devattr, char *buf)
+static int sch5627_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+ int channel, const char **str)
{
- struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+ switch (type) {
+ case hwmon_in:
+ switch (attr) {
+ case hwmon_in_label:
+ *str = SCH5627_IN_LABELS[channel];
+ return 0;
+ default:
+ break;
+ }
+ break;
+ default:
+ break;
+ }
- return sysfs_emit(buf, "%s\n",
- SCH5627_IN_LABELS[attr->index]);
+ return -EOPNOTSUPP;
}
-static DEVICE_ATTR_RO(name);
-static SENSOR_DEVICE_ATTR_RO(temp1_input, temp, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_input, temp, 1);
-static SENSOR_DEVICE_ATTR_RO(temp3_input, temp, 2);
-static SENSOR_DEVICE_ATTR_RO(temp4_input, temp, 3);
-static SENSOR_DEVICE_ATTR_RO(temp5_input, temp, 4);
-static SENSOR_DEVICE_ATTR_RO(temp6_input, temp, 5);
-static SENSOR_DEVICE_ATTR_RO(temp7_input, temp, 6);
-static SENSOR_DEVICE_ATTR_RO(temp8_input, temp, 7);
-static SENSOR_DEVICE_ATTR_RO(temp1_fault, temp_fault, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_fault, temp_fault, 1);
-static SENSOR_DEVICE_ATTR_RO(temp3_fault, temp_fault, 2);
-static SENSOR_DEVICE_ATTR_RO(temp4_fault, temp_fault, 3);
-static SENSOR_DEVICE_ATTR_RO(temp5_fault, temp_fault, 4);
-static SENSOR_DEVICE_ATTR_RO(temp6_fault, temp_fault, 5);
-static SENSOR_DEVICE_ATTR_RO(temp7_fault, temp_fault, 6);
-static SENSOR_DEVICE_ATTR_RO(temp8_fault, temp_fault, 7);
-static SENSOR_DEVICE_ATTR_RO(temp1_max, temp_max, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_max, temp_max, 1);
-static SENSOR_DEVICE_ATTR_RO(temp3_max, temp_max, 2);
-static SENSOR_DEVICE_ATTR_RO(temp4_max, temp_max, 3);
-static SENSOR_DEVICE_ATTR_RO(temp5_max, temp_max, 4);
-static SENSOR_DEVICE_ATTR_RO(temp6_max, temp_max, 5);
-static SENSOR_DEVICE_ATTR_RO(temp7_max, temp_max, 6);
-static SENSOR_DEVICE_ATTR_RO(temp8_max, temp_max, 7);
-static SENSOR_DEVICE_ATTR_RO(temp1_crit, temp_crit, 0);
-static SENSOR_DEVICE_ATTR_RO(temp2_crit, temp_crit, 1);
-static SENSOR_DEVICE_ATTR_RO(temp3_crit, temp_crit, 2);
-static SENSOR_DEVICE_ATTR_RO(temp4_crit, temp_crit, 3);
-static SENSOR_DEVICE_ATTR_RO(temp5_crit, temp_crit, 4);
-static SENSOR_DEVICE_ATTR_RO(temp6_crit, temp_crit, 5);
-static SENSOR_DEVICE_ATTR_RO(temp7_crit, temp_crit, 6);
-static SENSOR_DEVICE_ATTR_RO(temp8_crit, temp_crit, 7);
-
-static SENSOR_DEVICE_ATTR_RO(fan1_input, fan, 0);
-static SENSOR_DEVICE_ATTR_RO(fan2_input, fan, 1);
-static SENSOR_DEVICE_ATTR_RO(fan3_input, fan, 2);
-static SENSOR_DEVICE_ATTR_RO(fan4_input, fan, 3);
-static SENSOR_DEVICE_ATTR_RO(fan1_fault, fan_fault, 0);
-static SENSOR_DEVICE_ATTR_RO(fan2_fault, fan_fault, 1);
-static SENSOR_DEVICE_ATTR_RO(fan3_fault, fan_fault, 2);
-static SENSOR_DEVICE_ATTR_RO(fan4_fault, fan_fault, 3);
-static SENSOR_DEVICE_ATTR_RO(fan1_min, fan_min, 0);
-static SENSOR_DEVICE_ATTR_RO(fan2_min, fan_min, 1);
-static SENSOR_DEVICE_ATTR_RO(fan3_min, fan_min, 2);
-static SENSOR_DEVICE_ATTR_RO(fan4_min, fan_min, 3);
-
-static SENSOR_DEVICE_ATTR_RO(in0_input, in, 0);
-static SENSOR_DEVICE_ATTR_RO(in1_input, in, 1);
-static SENSOR_DEVICE_ATTR_RO(in2_input, in, 2);
-static SENSOR_DEVICE_ATTR_RO(in3_input, in, 3);
-static SENSOR_DEVICE_ATTR_RO(in4_input, in, 4);
-static SENSOR_DEVICE_ATTR_RO(in0_label, in_label, 0);
-static SENSOR_DEVICE_ATTR_RO(in1_label, in_label, 1);
-static SENSOR_DEVICE_ATTR_RO(in2_label, in_label, 2);
-static SENSOR_DEVICE_ATTR_RO(in3_label, in_label, 3);
-
-static struct attribute *sch5627_attributes[] = {
- &dev_attr_name.attr,
-
- &sensor_dev_attr_temp1_input.dev_attr.attr,
- &sensor_dev_attr_temp2_input.dev_attr.attr,
- &sensor_dev_attr_temp3_input.dev_attr.attr,
- &sensor_dev_attr_temp4_input.dev_attr.attr,
- &sensor_dev_attr_temp5_input.dev_attr.attr,
- &sensor_dev_attr_temp6_input.dev_attr.attr,
- &sensor_dev_attr_temp7_input.dev_attr.attr,
- &sensor_dev_attr_temp8_input.dev_attr.attr,
- &sensor_dev_attr_temp1_fault.dev_attr.attr,
- &sensor_dev_attr_temp2_fault.dev_attr.attr,
- &sensor_dev_attr_temp3_fault.dev_attr.attr,
- &sensor_dev_attr_temp4_fault.dev_attr.attr,
- &sensor_dev_attr_temp5_fault.dev_attr.attr,
- &sensor_dev_attr_temp6_fault.dev_attr.attr,
- &sensor_dev_attr_temp7_fault.dev_attr.attr,
- &sensor_dev_attr_temp8_fault.dev_attr.attr,
- &sensor_dev_attr_temp1_max.dev_attr.attr,
- &sensor_dev_attr_temp2_max.dev_attr.attr,
- &sensor_dev_attr_temp3_max.dev_attr.attr,
- &sensor_dev_attr_temp4_max.dev_attr.attr,
- &sensor_dev_attr_temp5_max.dev_attr.attr,
- &sensor_dev_attr_temp6_max.dev_attr.attr,
- &sensor_dev_attr_temp7_max.dev_attr.attr,
- &sensor_dev_attr_temp8_max.dev_attr.attr,
- &sensor_dev_attr_temp1_crit.dev_attr.attr,
- &sensor_dev_attr_temp2_crit.dev_attr.attr,
- &sensor_dev_attr_temp3_crit.dev_attr.attr,
- &sensor_dev_attr_temp4_crit.dev_attr.attr,
- &sensor_dev_attr_temp5_crit.dev_attr.attr,
- &sensor_dev_attr_temp6_crit.dev_attr.attr,
- &sensor_dev_attr_temp7_crit.dev_attr.attr,
- &sensor_dev_attr_temp8_crit.dev_attr.attr,
-
- &sensor_dev_attr_fan1_input.dev_attr.attr,
- &sensor_dev_attr_fan2_input.dev_attr.attr,
- &sensor_dev_attr_fan3_input.dev_attr.attr,
- &sensor_dev_attr_fan4_input.dev_attr.attr,
- &sensor_dev_attr_fan1_fault.dev_attr.attr,
- &sensor_dev_attr_fan2_fault.dev_attr.attr,
- &sensor_dev_attr_fan3_fault.dev_attr.attr,
- &sensor_dev_attr_fan4_fault.dev_attr.attr,
- &sensor_dev_attr_fan1_min.dev_attr.attr,
- &sensor_dev_attr_fan2_min.dev_attr.attr,
- &sensor_dev_attr_fan3_min.dev_attr.attr,
- &sensor_dev_attr_fan4_min.dev_attr.attr,
-
- &sensor_dev_attr_in0_input.dev_attr.attr,
- &sensor_dev_attr_in1_input.dev_attr.attr,
- &sensor_dev_attr_in2_input.dev_attr.attr,
- &sensor_dev_attr_in3_input.dev_attr.attr,
- &sensor_dev_attr_in4_input.dev_attr.attr,
- &sensor_dev_attr_in0_label.dev_attr.attr,
- &sensor_dev_attr_in1_label.dev_attr.attr,
- &sensor_dev_attr_in2_label.dev_attr.attr,
- &sensor_dev_attr_in3_label.dev_attr.attr,
- /* No in4_label as in4 is a generic input pin */
+static const struct hwmon_ops sch5627_ops = {
+ .is_visible = sch5627_is_visible,
+ .read = sch5627_read,
+ .read_string = sch5627_read_string,
+};
+static const struct hwmon_channel_info *sch5627_info[] = {
+ HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+ HWMON_CHANNEL_INFO(temp,
+ HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_FAULT,
+ HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_FAULT,
+ HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_FAULT,
+ HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_FAULT,
+ HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_FAULT,
+ HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_FAULT,
+ HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_FAULT,
+ HWMON_T_INPUT | HWMON_T_MAX | HWMON_T_CRIT | HWMON_T_FAULT
+ ),
+ HWMON_CHANNEL_INFO(fan,
+ HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
+ HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
+ HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
+ HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT
+ ),
+ HWMON_CHANNEL_INFO(in,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT | HWMON_I_LABEL,
+ HWMON_I_INPUT
+ ),
NULL
};
-static const struct attribute_group sch5627_group = {
- .attrs = sch5627_attributes,
+static const struct hwmon_chip_info sch5627_chip_info = {
+ .ops = &sch5627_ops,
+ .info = sch5627_info,
};
static int sch5627_remove(struct platform_device *pdev)
@@ -447,8 +332,6 @@ static int sch5627_remove(struct platform_device *pdev)
if (data->hwmon_dev)
hwmon_device_unregister(data->hwmon_dev);
- sysfs_remove_group(&pdev->dev.kobj, &sch5627_group);
-
return 0;
}
@@ -552,12 +435,8 @@ static int sch5627_probe(struct platform_device *pdev)
pr_info("firmware build: code 0x%02X, id 0x%04X, hwmon: rev 0x%02X\n",
build_code, build_id, hwmon_rev);
- /* Register sysfs interface files */
- err = sysfs_create_group(&pdev->dev.kobj, &sch5627_group);
- if (err)
- goto error;
-
- data->hwmon_dev = hwmon_device_register(&pdev->dev);
+ data->hwmon_dev = hwmon_device_register_with_info(&pdev->dev, DEVNAME, data,
+ &sch5627_chip_info, 0);
if (IS_ERR(data->hwmon_dev)) {
err = PTR_ERR(data->hwmon_dev);
data->hwmon_dev = NULL;
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] hwmon: (sch5627) Split sch5627_update_device()
2021-04-11 16:42 [PATCH 0/2] hwmon: (sch5627) Replace hwmon_device_register() W_Armin
2021-04-11 16:42 ` [PATCH 1/2] hwmon: (sch5627) Convert to hwmon_device_register_with_info() W_Armin
@ 2021-04-11 16:42 ` W_Armin
2021-04-13 16:39 ` Guenter Roeck
2021-04-13 8:11 ` [PATCH 0/2] hwmon: (sch5627) Replace hwmon_device_register() Hans de Goede
2 siblings, 1 reply; 5+ messages in thread
From: W_Armin @ 2021-04-11 16:42 UTC (permalink / raw)
To: hdegoede; +Cc: linux-hwmon, Armin Wolf
From: Armin Wolf <W_Armin@gmx.de>
An error in sch5627_update_device() could cause sch5627_read()
to fail even if the error did not affect the target sensor type.
Split sch5627_update_device() to prevent that.
Tested on a Fujitsu Esprimo P720.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/hwmon/sch5627.c | 102 +++++++++++++++++++++++++++-------------
1 file changed, 69 insertions(+), 33 deletions(-)
diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
index 023376082ee6..9735f3a81cbb 100644
--- a/drivers/hwmon/sch5627.c
+++ b/drivers/hwmon/sch5627.c
@@ -73,66 +73,96 @@ struct sch5627_data {
struct mutex update_lock;
unsigned long last_battery; /* In jiffies */
- char valid; /* !=0 if following fields are valid */
- unsigned long last_updated; /* In jiffies */
+ char temp_valid; /* !=0 if following fields are valid */
+ char fan_valid;
+ char in_valid;
+ unsigned long temp_last_updated; /* In jiffies */
+ unsigned long fan_last_updated;
+ unsigned long in_last_updated;
u16 temp[SCH5627_NO_TEMPS];
u16 fan[SCH5627_NO_FANS];
u16 in[SCH5627_NO_IN];
};
-static struct sch5627_data *sch5627_update_device(struct device *dev)
+static int sch5627_update_temp(struct sch5627_data *data)
{
- struct sch5627_data *data = dev_get_drvdata(dev);
- struct sch5627_data *ret = data;
+ int ret = 0;
int i, val;
mutex_lock(&data->update_lock);
- /* Trigger a Vbat voltage measurement every 5 minutes */
- if (time_after(jiffies, data->last_battery + 300 * HZ)) {
- sch56xx_write_virtual_reg(data->addr, SCH5627_REG_CTRL,
- data->control | 0x10);
- data->last_battery = jiffies;
- }
-
/* Cache the values for 1 second */
- if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
+ if (time_after(jiffies, data->temp_last_updated + HZ) || !data->temp_valid) {
for (i = 0; i < SCH5627_NO_TEMPS; i++) {
- val = sch56xx_read_virtual_reg12(data->addr,
- SCH5627_REG_TEMP_MSB[i],
- SCH5627_REG_TEMP_LSN[i],
- SCH5627_REG_TEMP_HIGH_NIBBLE[i]);
+ val = sch56xx_read_virtual_reg12(data->addr, SCH5627_REG_TEMP_MSB[i],
+ SCH5627_REG_TEMP_LSN[i],
+ SCH5627_REG_TEMP_HIGH_NIBBLE[i]);
if (unlikely(val < 0)) {
- ret = ERR_PTR(val);
+ ret = val;
goto abort;
}
data->temp[i] = val;
}
+ data->temp_last_updated = jiffies;
+ data->temp_valid = 1;
+ }
+abort:
+ mutex_unlock(&data->update_lock);
+ return ret;
+}
+
+static int sch5627_update_fan(struct sch5627_data *data)
+{
+ int ret = 0;
+ int i, val;
+ mutex_lock(&data->update_lock);
+
+ /* Cache the values for 1 second */
+ if (time_after(jiffies, data->fan_last_updated + HZ) || !data->fan_valid) {
for (i = 0; i < SCH5627_NO_FANS; i++) {
- val = sch56xx_read_virtual_reg16(data->addr,
- SCH5627_REG_FAN[i]);
+ val = sch56xx_read_virtual_reg16(data->addr, SCH5627_REG_FAN[i]);
if (unlikely(val < 0)) {
- ret = ERR_PTR(val);
+ ret = val;
goto abort;
}
data->fan[i] = val;
}
+ data->fan_last_updated = jiffies;
+ data->fan_valid = 1;
+ }
+abort:
+ mutex_unlock(&data->update_lock);
+ return ret;
+}
+
+static int sch5627_update_in(struct sch5627_data *data)
+{
+ int ret = 0;
+ int i, val;
+
+ mutex_lock(&data->update_lock);
+ /* Trigger a Vbat voltage measurement every 5 minutes */
+ if (time_after(jiffies, data->last_battery + 300 * HZ)) {
+ sch56xx_write_virtual_reg(data->addr, SCH5627_REG_CTRL, data->control | 0x10);
+ data->last_battery = jiffies;
+ }
+
+ /* Cache the values for 1 second */
+ if (time_after(jiffies, data->in_last_updated + HZ) || !data->in_valid) {
for (i = 0; i < SCH5627_NO_IN; i++) {
- val = sch56xx_read_virtual_reg12(data->addr,
- SCH5627_REG_IN_MSB[i],
- SCH5627_REG_IN_LSN[i],
- SCH5627_REG_IN_HIGH_NIBBLE[i]);
+ val = sch56xx_read_virtual_reg12(data->addr, SCH5627_REG_IN_MSB[i],
+ SCH5627_REG_IN_LSN[i],
+ SCH5627_REG_IN_HIGH_NIBBLE[i]);
if (unlikely(val < 0)) {
- ret = ERR_PTR(val);
+ ret = val;
goto abort;
}
data->in[i] = val;
}
-
- data->last_updated = jiffies;
- data->valid = 1;
+ data->in_last_updated = jiffies;
+ data->in_valid = 1;
}
abort:
mutex_unlock(&data->update_lock);
@@ -200,14 +230,14 @@ static umode_t sch5627_is_visible(const void *drvdata, enum hwmon_sensor_types t
static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
long *val)
{
- struct sch5627_data *data = sch5627_update_device(dev);
+ struct sch5627_data *data = dev_get_drvdata(dev);
int ret;
- if (IS_ERR(data))
- return PTR_ERR(data);
-
switch (type) {
case hwmon_temp:
+ ret = sch5627_update_temp(data);
+ if (ret < 0)
+ return ret;
switch (attr) {
case hwmon_temp_input:
*val = reg_to_temp(data->temp[channel]);
@@ -226,6 +256,9 @@ static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 at
}
break;
case hwmon_fan:
+ ret = sch5627_update_fan(data);
+ if (ret < 0)
+ return ret;
switch (attr) {
case hwmon_fan_input:
ret = reg_to_rpm(data->fan[channel]);
@@ -247,6 +280,9 @@ static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 at
}
break;
case hwmon_in:
+ ret = sch5627_update_in(data);
+ if (ret < 0)
+ return ret;
switch (attr) {
case hwmon_in_input:
*val = DIV_ROUND_CLOSEST(data->in[channel] * SCH5627_REG_IN_FACTOR[channel],
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 0/2] hwmon: (sch5627) Replace hwmon_device_register()
2021-04-11 16:42 [PATCH 0/2] hwmon: (sch5627) Replace hwmon_device_register() W_Armin
2021-04-11 16:42 ` [PATCH 1/2] hwmon: (sch5627) Convert to hwmon_device_register_with_info() W_Armin
2021-04-11 16:42 ` [PATCH 2/2] hwmon: (sch5627) Split sch5627_update_device() W_Armin
@ 2021-04-13 8:11 ` Hans de Goede
2 siblings, 0 replies; 5+ messages in thread
From: Hans de Goede @ 2021-04-13 8:11 UTC (permalink / raw)
To: W_Armin; +Cc: linux-hwmon
Hi,
On 4/11/21 6:42 PM, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
>
> When the sch5627 driver is loaded, a warning is displayed during
> bootup about hwmon_device_register() being deprecated.
>
> The first patch solves that issue, and the second patch adds
> a minor optimization when reading sensor data.
>
> Both patches have been tested on a Fujitsu Esprimo P720
> and appear to work without issues.
Thank you for doing this, both patches look good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
For both patches.
Regards,
Hans
> Armin Wolf (2):
> hwmon: (sch5627) Convert to hwmon_device_register_with_info()
> hwmon: (sch5627) Split sch5627_update_device()
>
> drivers/hwmon/sch5627.c | 445 ++++++++++++++++------------------------
> 1 file changed, 180 insertions(+), 265 deletions(-)
>
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] hwmon: (sch5627) Split sch5627_update_device()
2021-04-11 16:42 ` [PATCH 2/2] hwmon: (sch5627) Split sch5627_update_device() W_Armin
@ 2021-04-13 16:39 ` Guenter Roeck
0 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2021-04-13 16:39 UTC (permalink / raw)
To: W_Armin; +Cc: hdegoede, linux-hwmon
On Sun, Apr 11, 2021 at 06:42:25PM +0200, W_Armin@gmx.de wrote:
> From: Armin Wolf <W_Armin@gmx.de>
>
> An error in sch5627_update_device() could cause sch5627_read()
> to fail even if the error did not affect the target sensor type.
> Split sch5627_update_device() to prevent that.
>
> Tested on a Fujitsu Esprimo P720.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Applied.
Thanks,
Guenter
> ---
> drivers/hwmon/sch5627.c | 102 +++++++++++++++++++++++++++-------------
> 1 file changed, 69 insertions(+), 33 deletions(-)
>
> --
> 2.20.1
>
> diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
> index 023376082ee6..9735f3a81cbb 100644
> --- a/drivers/hwmon/sch5627.c
> +++ b/drivers/hwmon/sch5627.c
> @@ -73,66 +73,96 @@ struct sch5627_data {
>
> struct mutex update_lock;
> unsigned long last_battery; /* In jiffies */
> - char valid; /* !=0 if following fields are valid */
> - unsigned long last_updated; /* In jiffies */
> + char temp_valid; /* !=0 if following fields are valid */
> + char fan_valid;
> + char in_valid;
> + unsigned long temp_last_updated; /* In jiffies */
> + unsigned long fan_last_updated;
> + unsigned long in_last_updated;
> u16 temp[SCH5627_NO_TEMPS];
> u16 fan[SCH5627_NO_FANS];
> u16 in[SCH5627_NO_IN];
> };
>
> -static struct sch5627_data *sch5627_update_device(struct device *dev)
> +static int sch5627_update_temp(struct sch5627_data *data)
> {
> - struct sch5627_data *data = dev_get_drvdata(dev);
> - struct sch5627_data *ret = data;
> + int ret = 0;
> int i, val;
>
> mutex_lock(&data->update_lock);
>
> - /* Trigger a Vbat voltage measurement every 5 minutes */
> - if (time_after(jiffies, data->last_battery + 300 * HZ)) {
> - sch56xx_write_virtual_reg(data->addr, SCH5627_REG_CTRL,
> - data->control | 0x10);
> - data->last_battery = jiffies;
> - }
> -
> /* Cache the values for 1 second */
> - if (time_after(jiffies, data->last_updated + HZ) || !data->valid) {
> + if (time_after(jiffies, data->temp_last_updated + HZ) || !data->temp_valid) {
> for (i = 0; i < SCH5627_NO_TEMPS; i++) {
> - val = sch56xx_read_virtual_reg12(data->addr,
> - SCH5627_REG_TEMP_MSB[i],
> - SCH5627_REG_TEMP_LSN[i],
> - SCH5627_REG_TEMP_HIGH_NIBBLE[i]);
> + val = sch56xx_read_virtual_reg12(data->addr, SCH5627_REG_TEMP_MSB[i],
> + SCH5627_REG_TEMP_LSN[i],
> + SCH5627_REG_TEMP_HIGH_NIBBLE[i]);
> if (unlikely(val < 0)) {
> - ret = ERR_PTR(val);
> + ret = val;
> goto abort;
> }
> data->temp[i] = val;
> }
> + data->temp_last_updated = jiffies;
> + data->temp_valid = 1;
> + }
> +abort:
> + mutex_unlock(&data->update_lock);
> + return ret;
> +}
> +
> +static int sch5627_update_fan(struct sch5627_data *data)
> +{
> + int ret = 0;
> + int i, val;
>
> + mutex_lock(&data->update_lock);
> +
> + /* Cache the values for 1 second */
> + if (time_after(jiffies, data->fan_last_updated + HZ) || !data->fan_valid) {
> for (i = 0; i < SCH5627_NO_FANS; i++) {
> - val = sch56xx_read_virtual_reg16(data->addr,
> - SCH5627_REG_FAN[i]);
> + val = sch56xx_read_virtual_reg16(data->addr, SCH5627_REG_FAN[i]);
> if (unlikely(val < 0)) {
> - ret = ERR_PTR(val);
> + ret = val;
> goto abort;
> }
> data->fan[i] = val;
> }
> + data->fan_last_updated = jiffies;
> + data->fan_valid = 1;
> + }
> +abort:
> + mutex_unlock(&data->update_lock);
> + return ret;
> +}
> +
> +static int sch5627_update_in(struct sch5627_data *data)
> +{
> + int ret = 0;
> + int i, val;
> +
> + mutex_lock(&data->update_lock);
>
> + /* Trigger a Vbat voltage measurement every 5 minutes */
> + if (time_after(jiffies, data->last_battery + 300 * HZ)) {
> + sch56xx_write_virtual_reg(data->addr, SCH5627_REG_CTRL, data->control | 0x10);
> + data->last_battery = jiffies;
> + }
> +
> + /* Cache the values for 1 second */
> + if (time_after(jiffies, data->in_last_updated + HZ) || !data->in_valid) {
> for (i = 0; i < SCH5627_NO_IN; i++) {
> - val = sch56xx_read_virtual_reg12(data->addr,
> - SCH5627_REG_IN_MSB[i],
> - SCH5627_REG_IN_LSN[i],
> - SCH5627_REG_IN_HIGH_NIBBLE[i]);
> + val = sch56xx_read_virtual_reg12(data->addr, SCH5627_REG_IN_MSB[i],
> + SCH5627_REG_IN_LSN[i],
> + SCH5627_REG_IN_HIGH_NIBBLE[i]);
> if (unlikely(val < 0)) {
> - ret = ERR_PTR(val);
> + ret = val;
> goto abort;
> }
> data->in[i] = val;
> }
> -
> - data->last_updated = jiffies;
> - data->valid = 1;
> + data->in_last_updated = jiffies;
> + data->in_valid = 1;
> }
> abort:
> mutex_unlock(&data->update_lock);
> @@ -200,14 +230,14 @@ static umode_t sch5627_is_visible(const void *drvdata, enum hwmon_sensor_types t
> static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> long *val)
> {
> - struct sch5627_data *data = sch5627_update_device(dev);
> + struct sch5627_data *data = dev_get_drvdata(dev);
> int ret;
>
> - if (IS_ERR(data))
> - return PTR_ERR(data);
> -
> switch (type) {
> case hwmon_temp:
> + ret = sch5627_update_temp(data);
> + if (ret < 0)
> + return ret;
> switch (attr) {
> case hwmon_temp_input:
> *val = reg_to_temp(data->temp[channel]);
> @@ -226,6 +256,9 @@ static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 at
> }
> break;
> case hwmon_fan:
> + ret = sch5627_update_fan(data);
> + if (ret < 0)
> + return ret;
> switch (attr) {
> case hwmon_fan_input:
> ret = reg_to_rpm(data->fan[channel]);
> @@ -247,6 +280,9 @@ static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 at
> }
> break;
> case hwmon_in:
> + ret = sch5627_update_in(data);
> + if (ret < 0)
> + return ret;
> switch (attr) {
> case hwmon_in_input:
> *val = DIV_ROUND_CLOSEST(data->in[channel] * SCH5627_REG_IN_FACTOR[channel],
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-04-13 16:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-11 16:42 [PATCH 0/2] hwmon: (sch5627) Replace hwmon_device_register() W_Armin
2021-04-11 16:42 ` [PATCH 1/2] hwmon: (sch5627) Convert to hwmon_device_register_with_info() W_Armin
2021-04-11 16:42 ` [PATCH 2/2] hwmon: (sch5627) Split sch5627_update_device() W_Armin
2021-04-13 16:39 ` Guenter Roeck
2021-04-13 8:11 ` [PATCH 0/2] hwmon: (sch5627) Replace hwmon_device_register() Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).