* [PATCH v2 1/5] hwmon: (sch5627) Use bit macros when accessing the control register
2023-09-07 5:26 [PATCH v2 0/5] hwmon: (sch5627) Make limit registers writable Armin Wolf
@ 2023-09-07 5:26 ` Armin Wolf
2023-09-11 0:48 ` Guenter Roeck
2023-09-07 5:26 ` [PATCH v2 2/5] hwmon: (sch5627) Disallow write access if virtual registers are locked Armin Wolf
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Armin Wolf @ 2023-09-07 5:26 UTC (permalink / raw)
To: hdegoede; +Cc: jdelvare, linux, linux-hwmon, linux-kernel
Use bit macros then accessing SCH5627_REG_CTRL, so that people
do not need to look at the datasheet to find out what each bit
does.
Tested on a Fujitsu Esprimo P720.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/hwmon/sch5627.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
index 1bbda3b05532..0eefb8c0aef2 100644
--- a/drivers/hwmon/sch5627.c
+++ b/drivers/hwmon/sch5627.c
@@ -6,6 +6,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/bits.h>
#include <linux/module.h>
#include <linux/mod_devicetable.h>
#include <linux/init.h>
@@ -32,6 +33,9 @@
#define SCH5627_REG_PRIMARY_ID 0x3f
#define SCH5627_REG_CTRL 0x40
+#define SCH5627_CTRL_START BIT(0)
+#define SCH5627_CTRL_VBAT BIT(4)
+
#define SCH5627_NO_TEMPS 8
#define SCH5627_NO_FANS 4
#define SCH5627_NO_IN 5
@@ -147,7 +151,8 @@ static int sch5627_update_in(struct sch5627_data *data)
/* 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);
+ sch56xx_write_virtual_reg(data->addr, SCH5627_REG_CTRL,
+ data->control | SCH5627_CTRL_VBAT);
data->last_battery = jiffies;
}
@@ -483,14 +488,13 @@ static int sch5627_probe(struct platform_device *pdev)
return val;
data->control = val;
- if (!(data->control & 0x01)) {
+ if (!(data->control & SCH5627_CTRL_START)) {
pr_err("hardware monitoring not enabled\n");
return -ENODEV;
}
/* Trigger a Vbat voltage measurement, so that we get a valid reading
the first time we read Vbat */
- sch56xx_write_virtual_reg(data->addr, SCH5627_REG_CTRL,
- data->control | 0x10);
+ sch56xx_write_virtual_reg(data->addr, SCH5627_REG_CTRL, data->control | SCH5627_CTRL_VBAT);
data->last_battery = jiffies;
/*
--
2.39.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v2 2/5] hwmon: (sch5627) Disallow write access if virtual registers are locked
2023-09-07 5:26 [PATCH v2 0/5] hwmon: (sch5627) Make limit registers writable Armin Wolf
2023-09-07 5:26 ` [PATCH v2 1/5] hwmon: (sch5627) Use bit macros when accessing the control register Armin Wolf
@ 2023-09-07 5:26 ` Armin Wolf
2023-09-11 0:49 ` Guenter Roeck
2023-09-07 5:26 ` [PATCH v2 3/5] hwmon: (sch5627) Use regmap for pwm map register caching Armin Wolf
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Armin Wolf @ 2023-09-07 5:26 UTC (permalink / raw)
To: hdegoede; +Cc: jdelvare, linux, linux-hwmon, linux-kernel
When the lock bit inside SCH5627_REG_CTRL is set, then the virtual
registers become read-only until the next power cycle.
Disallow write access to those registers in such a case.
Tested on a Fujitsu Esprimo P720.
Fixes: aa9f833dfc12 ("hwmon: (sch5627) Add pwmX_auto_channels_temp support")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/hwmon/sch5627.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
index 0eefb8c0aef2..bf408e35e2c3 100644
--- a/drivers/hwmon/sch5627.c
+++ b/drivers/hwmon/sch5627.c
@@ -34,6 +34,7 @@
#define SCH5627_REG_CTRL 0x40
#define SCH5627_CTRL_START BIT(0)
+#define SCH5627_CTRL_LOCK BIT(1)
#define SCH5627_CTRL_VBAT BIT(4)
#define SCH5627_NO_TEMPS 8
@@ -231,6 +232,14 @@ static int reg_to_rpm(u16 reg)
static umode_t sch5627_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
int channel)
{
+ const struct sch5627_data *data = drvdata;
+
+ /* Once the lock bit is set, the virtual registers become read-only
+ * until the next power cycle.
+ */
+ if (data->control & SCH5627_CTRL_LOCK)
+ return 0444;
+
if (type == hwmon_pwm && attr == hwmon_pwm_auto_channels_temp)
return 0644;
--
2.39.2
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v2 3/5] hwmon: (sch5627) Use regmap for pwm map register caching
2023-09-07 5:26 [PATCH v2 0/5] hwmon: (sch5627) Make limit registers writable Armin Wolf
2023-09-07 5:26 ` [PATCH v2 1/5] hwmon: (sch5627) Use bit macros when accessing the control register Armin Wolf
2023-09-07 5:26 ` [PATCH v2 2/5] hwmon: (sch5627) Disallow write access if virtual registers are locked Armin Wolf
@ 2023-09-07 5:26 ` Armin Wolf
2023-09-11 0:52 ` Guenter Roeck
2023-09-07 5:26 ` [PATCH v2 4/5] hwmon: (sch5627) Add support for writing limit registers Armin Wolf
2023-09-07 5:26 ` [PATCH v2 5/5] hwmon: (sch5627) Document behaviour of " Armin Wolf
4 siblings, 1 reply; 11+ messages in thread
From: Armin Wolf @ 2023-09-07 5:26 UTC (permalink / raw)
To: hdegoede; +Cc: jdelvare, linux, linux-hwmon, linux-kernel
Accessing virtual registers is very inefficient, so pwm map values
should be cached when possible, else userspace could effectively do
a DOS attack by reading pwm map values in a while loop.
Use the regmap cache to cache those values.
Tested on a Fujitsu Esprimo P720.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/hwmon/Kconfig | 1 +
drivers/hwmon/sch5627.c | 72 +++++++++++++++++++++++++-------
drivers/hwmon/sch56xx-common.c | 76 ++++++++++++++++++++++++++++++++++
drivers/hwmon/sch56xx-common.h | 3 ++
4 files changed, 138 insertions(+), 14 deletions(-)
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ec38c8892158..1c672195b5b3 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1909,6 +1909,7 @@ config SENSORS_SMSC47B397
config SENSORS_SCH56XX_COMMON
tristate
+ select REGMAP
config SENSORS_SCH5627
tristate "SMSC SCH5627"
diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
index bf408e35e2c3..85f8352cb3cf 100644
--- a/drivers/hwmon/sch5627.c
+++ b/drivers/hwmon/sch5627.c
@@ -9,7 +9,9 @@
#include <linux/bits.h>
#include <linux/module.h>
#include <linux/mod_devicetable.h>
+#include <linux/pm.h>
#include <linux/init.h>
+#include <linux/regmap.h>
#include <linux/slab.h>
#include <linux/jiffies.h>
#include <linux/platform_device.h>
@@ -72,6 +74,7 @@ static const char * const SCH5627_IN_LABELS[SCH5627_NO_IN] = {
"VCC", "VTT", "VBAT", "VTR", "V_IN" };
struct sch5627_data {
+ struct regmap *regmap;
unsigned short addr;
u8 control;
u8 temp_max[SCH5627_NO_TEMPS];
@@ -91,6 +94,26 @@ struct sch5627_data {
u16 in[SCH5627_NO_IN];
};
+static const struct regmap_range sch5627_tunables_ranges[] = {
+ regmap_reg_range(0xA0, 0xA3),
+};
+
+static const struct regmap_access_table sch5627_tunables_table = {
+ .yes_ranges = sch5627_tunables_ranges,
+ .n_yes_ranges = ARRAY_SIZE(sch5627_tunables_ranges),
+};
+
+static const struct regmap_config sch5627_regmap_config = {
+ .reg_bits = 16,
+ .val_bits = 8,
+ .wr_table = &sch5627_tunables_table,
+ .rd_table = &sch5627_tunables_table,
+ .cache_type = REGCACHE_RBTREE,
+ .use_single_read = true,
+ .use_single_write = true,
+ .can_sleep = true,
+};
+
static int sch5627_update_temp(struct sch5627_data *data)
{
int ret = 0;
@@ -250,7 +273,7 @@ static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 at
long *val)
{
struct sch5627_data *data = dev_get_drvdata(dev);
- int ret;
+ int ret, value;
switch (type) {
case hwmon_temp:
@@ -301,15 +324,11 @@ static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 at
case hwmon_pwm:
switch (attr) {
case hwmon_pwm_auto_channels_temp:
- mutex_lock(&data->update_lock);
- ret = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_PWM_MAP[channel]);
- mutex_unlock(&data->update_lock);
-
+ ret = regmap_read(data->regmap, SCH5627_REG_PWM_MAP[channel], &value);
if (ret < 0)
return ret;
- *val = ret;
-
+ *val = value;
return 0;
default:
break;
@@ -359,7 +378,6 @@ static int sch5627_write(struct device *dev, enum hwmon_sensor_types type, u32 a
long val)
{
struct sch5627_data *data = dev_get_drvdata(dev);
- int ret;
switch (type) {
case hwmon_pwm:
@@ -369,12 +387,7 @@ static int sch5627_write(struct device *dev, enum hwmon_sensor_types type, u32 a
if (val > U8_MAX || val < 0)
return -EINVAL;
- mutex_lock(&data->update_lock);
- ret = sch56xx_write_virtual_reg(data->addr, SCH5627_REG_PWM_MAP[channel],
- val);
- mutex_unlock(&data->update_lock);
-
- return ret;
+ return regmap_write(data->regmap, SCH5627_REG_PWM_MAP[channel], val);
default:
break;
}
@@ -501,6 +514,12 @@ static int sch5627_probe(struct platform_device *pdev)
pr_err("hardware monitoring not enabled\n");
return -ENODEV;
}
+
+ data->regmap = devm_regmap_init_sch56xx(&pdev->dev, &data->update_lock, data->addr,
+ &sch5627_regmap_config);
+ if (IS_ERR(data->regmap))
+ return PTR_ERR(data->regmap);
+
/* Trigger a Vbat voltage measurement, so that we get a valid reading
the first time we read Vbat */
sch56xx_write_virtual_reg(data->addr, SCH5627_REG_CTRL, data->control | SCH5627_CTRL_VBAT);
@@ -531,6 +550,30 @@ static int sch5627_probe(struct platform_device *pdev)
return 0;
}
+static int sch5627_suspend(struct device *dev)
+{
+ struct sch5627_data *data = dev_get_drvdata(dev);
+
+ regcache_cache_only(data->regmap, true);
+ regcache_mark_dirty(data->regmap);
+
+ return 0;
+}
+
+static int sch5627_resume(struct device *dev)
+{
+ struct sch5627_data *data = dev_get_drvdata(dev);
+
+ regcache_cache_only(data->regmap, false);
+ /* We must not access the virtual registers when the lock bit is set */
+ if (data->control & SCH5627_CTRL_LOCK)
+ return regcache_drop_region(data->regmap, 0, U16_MAX);
+
+ return regcache_sync(data->regmap);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(sch5627_dev_pm_ops, sch5627_suspend, sch5627_resume);
+
static const struct platform_device_id sch5627_device_id[] = {
{
.name = "sch5627",
@@ -542,6 +585,7 @@ MODULE_DEVICE_TABLE(platform, sch5627_device_id);
static struct platform_driver sch5627_driver = {
.driver = {
.name = DRVNAME,
+ .pm = pm_sleep_ptr(&sch5627_dev_pm_ops),
},
.probe = sch5627_probe,
.id_table = sch5627_device_id,
diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
index de3a0886c2f7..a3abafdaa3e6 100644
--- a/drivers/hwmon/sch56xx-common.c
+++ b/drivers/hwmon/sch56xx-common.c
@@ -10,6 +10,7 @@
#include <linux/mod_devicetable.h>
#include <linux/init.h>
#include <linux/platform_device.h>
+#include <linux/regmap.h>
#include <linux/dmi.h>
#include <linux/err.h>
#include <linux/io.h>
@@ -64,6 +65,11 @@ struct sch56xx_watchdog_data {
u8 watchdog_output_enable;
};
+struct sch56xx_bus_context {
+ struct mutex *lock; /* Used to serialize access to the mailbox registers */
+ u16 addr;
+};
+
static struct platform_device *sch56xx_pdev;
/* Super I/O functions */
@@ -243,6 +249,76 @@ int sch56xx_read_virtual_reg12(u16 addr, u16 msb_reg, u16 lsn_reg,
}
EXPORT_SYMBOL(sch56xx_read_virtual_reg12);
+/*
+ * Regmap support
+ */
+
+static int sch56xx_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+ struct sch56xx_bus_context *bus = context;
+ int ret;
+
+ mutex_lock(bus->lock);
+ ret = sch56xx_write_virtual_reg(bus->addr, (u16)reg, (u8)val);
+ mutex_unlock(bus->lock);
+
+ return ret;
+}
+
+static int sch56xx_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+ struct sch56xx_bus_context *bus = context;
+ int ret;
+
+ mutex_lock(bus->lock);
+ ret = sch56xx_read_virtual_reg(bus->addr, (u16)reg);
+ mutex_unlock(bus->lock);
+
+ if (ret < 0)
+ return ret;
+
+ *val = ret;
+
+ return 0;
+}
+
+static void sch56xx_free_context(void *context)
+{
+ kfree(context);
+}
+
+static const struct regmap_bus sch56xx_bus = {
+ .reg_write = sch56xx_reg_write,
+ .reg_read = sch56xx_reg_read,
+ .free_context = sch56xx_free_context,
+ .reg_format_endian_default = REGMAP_ENDIAN_LITTLE,
+ .val_format_endian_default = REGMAP_ENDIAN_LITTLE,
+};
+
+struct regmap *devm_regmap_init_sch56xx(struct device *dev, struct mutex *lock, u16 addr,
+ const struct regmap_config *config)
+{
+ struct sch56xx_bus_context *context;
+ struct regmap *map;
+
+ if (config->reg_bits != 16 && config->val_bits != 8)
+ return ERR_PTR(-EOPNOTSUPP);
+
+ context = kzalloc(sizeof(*context), GFP_KERNEL);
+ if (!context)
+ return ERR_PTR(-ENOMEM);
+
+ context->lock = lock;
+ context->addr = addr;
+
+ map = devm_regmap_init(dev, &sch56xx_bus, context, config);
+ if (IS_ERR(map))
+ kfree(context);
+
+ return map;
+}
+EXPORT_SYMBOL(devm_regmap_init_sch56xx);
+
/*
* Watchdog routines
*/
diff --git a/drivers/hwmon/sch56xx-common.h b/drivers/hwmon/sch56xx-common.h
index e907d9da0dd5..3fb1cddbf977 100644
--- a/drivers/hwmon/sch56xx-common.h
+++ b/drivers/hwmon/sch56xx-common.h
@@ -5,9 +5,12 @@
***************************************************************************/
#include <linux/mutex.h>
+#include <linux/regmap.h>
struct sch56xx_watchdog_data;
+struct regmap *devm_regmap_init_sch56xx(struct device *dev, struct mutex *lock, u16 addr,
+ const struct regmap_config *config);
int sch56xx_read_virtual_reg(u16 addr, u16 reg);
int sch56xx_write_virtual_reg(u16 addr, u16 reg, u8 val);
int sch56xx_read_virtual_reg16(u16 addr, u16 reg);
--
2.39.2
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 3/5] hwmon: (sch5627) Use regmap for pwm map register caching
2023-09-07 5:26 ` [PATCH v2 3/5] hwmon: (sch5627) Use regmap for pwm map register caching Armin Wolf
@ 2023-09-11 0:52 ` Guenter Roeck
0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2023-09-11 0:52 UTC (permalink / raw)
To: Armin Wolf; +Cc: hdegoede, jdelvare, linux-hwmon, linux-kernel
On Thu, Sep 07, 2023 at 07:26:37AM +0200, Armin Wolf wrote:
> Accessing virtual registers is very inefficient, so pwm map values
> should be cached when possible, else userspace could effectively do
> a DOS attack by reading pwm map values in a while loop.
> Use the regmap cache to cache those values.
>
> Tested on a Fujitsu Esprimo P720.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Applied.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 4/5] hwmon: (sch5627) Add support for writing limit registers
2023-09-07 5:26 [PATCH v2 0/5] hwmon: (sch5627) Make limit registers writable Armin Wolf
` (2 preceding siblings ...)
2023-09-07 5:26 ` [PATCH v2 3/5] hwmon: (sch5627) Use regmap for pwm map register caching Armin Wolf
@ 2023-09-07 5:26 ` Armin Wolf
2023-09-11 0:54 ` Guenter Roeck
2023-09-07 5:26 ` [PATCH v2 5/5] hwmon: (sch5627) Document behaviour of " Armin Wolf
4 siblings, 1 reply; 11+ messages in thread
From: Armin Wolf @ 2023-09-07 5:26 UTC (permalink / raw)
To: hdegoede; +Cc: jdelvare, linux, linux-hwmon, linux-kernel
After some testing on a Fujitsu Esprimo P720, it turned out that
the limit registers are indeed writable and affect the fan control
algorithm. This is supported by the datasheet, which says that the
fan control functions are based on the limit and parameter registers.
Since accessing those registers is very inefficient, the existing
regmap cache is used to cache those registers values.
Tested on a Fujitsu Esprimo P720.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/hwmon/sch5627.c | 174 ++++++++++++++++++++++-----------
drivers/hwmon/sch56xx-common.c | 31 ++++++
drivers/hwmon/sch56xx-common.h | 3 +
3 files changed, 153 insertions(+), 55 deletions(-)
diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
index 85f8352cb3cf..1891d4d75aa9 100644
--- a/drivers/hwmon/sch5627.c
+++ b/drivers/hwmon/sch5627.c
@@ -7,6 +7,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/bits.h>
+#include <linux/minmax.h>
#include <linux/module.h>
#include <linux/mod_devicetable.h>
#include <linux/pm.h>
@@ -77,9 +78,6 @@ struct sch5627_data {
struct regmap *regmap;
unsigned short addr;
u8 control;
- u8 temp_max[SCH5627_NO_TEMPS];
- u8 temp_crit[SCH5627_NO_TEMPS];
- u16 fan_min[SCH5627_NO_FANS];
struct mutex update_lock;
unsigned long last_battery; /* In jiffies */
@@ -95,7 +93,17 @@ struct sch5627_data {
};
static const struct regmap_range sch5627_tunables_ranges[] = {
+ regmap_reg_range(0x57, 0x57),
+ regmap_reg_range(0x59, 0x59),
+ regmap_reg_range(0x5B, 0x5B),
+ regmap_reg_range(0x5D, 0x5D),
+ regmap_reg_range(0x5F, 0x5F),
+ regmap_reg_range(0x61, 0x69),
+ regmap_reg_range(0x96, 0x9B),
regmap_reg_range(0xA0, 0xA3),
+ regmap_reg_range(0x184, 0x184),
+ regmap_reg_range(0x186, 0x186),
+ regmap_reg_range(0x1A8, 0x1A9),
};
static const struct regmap_access_table sch5627_tunables_table = {
@@ -200,38 +208,6 @@ static int sch5627_update_in(struct sch5627_data *data)
return ret;
}
-static int sch5627_read_limits(struct sch5627_data *data)
-{
- int i, val;
-
- for (i = 0; i < SCH5627_NO_TEMPS; i++) {
- /*
- * Note what SMSC calls ABS, is what lm_sensors calls max
- * (aka high), and HIGH is what lm_sensors calls crit.
- */
- val = sch56xx_read_virtual_reg(data->addr,
- SCH5627_REG_TEMP_ABS[i]);
- if (val < 0)
- return val;
- data->temp_max[i] = val;
-
- val = sch56xx_read_virtual_reg(data->addr,
- SCH5627_REG_TEMP_HIGH[i]);
- if (val < 0)
- return val;
- data->temp_crit[i] = val;
- }
- for (i = 0; i < SCH5627_NO_FANS; i++) {
- val = sch56xx_read_virtual_reg16(data->addr,
- SCH5627_REG_FAN_MIN[i]);
- if (val < 0)
- return val;
- data->fan_min[i] = val;
- }
-
- return 0;
-}
-
static int reg_to_temp(u16 reg)
{
return (reg * 625) / 10 - 64000;
@@ -252,6 +228,25 @@ static int reg_to_rpm(u16 reg)
return 5400540 / reg;
}
+static u8 sch5627_temp_limit_to_reg(long value)
+{
+ long limit = (value / 1000) + 64;
+
+ return clamp_val(limit, 0, U8_MAX);
+}
+
+static u16 sch5627_rpm_to_reg(long value)
+{
+ long pulses;
+
+ if (value <= 0)
+ return U16_MAX - 1;
+
+ pulses = 5400540 / value;
+
+ return clamp_val(pulses, 1, U16_MAX - 1);
+}
+
static umode_t sch5627_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
int channel)
{
@@ -263,8 +258,35 @@ static umode_t sch5627_is_visible(const void *drvdata, enum hwmon_sensor_types t
if (data->control & SCH5627_CTRL_LOCK)
return 0444;
- if (type == hwmon_pwm && attr == hwmon_pwm_auto_channels_temp)
- return 0644;
+ switch (type) {
+ case hwmon_temp:
+ switch (attr) {
+ case hwmon_temp_max:
+ case hwmon_temp_crit:
+ return 0644;
+ default:
+ break;
+ }
+ break;
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_min:
+ return 0644;
+ default:
+ break;
+ }
+ break;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_auto_channels_temp:
+ return 0644;
+ default:
+ break;
+ }
+ break;
+ default:
+ break;
+ }
return 0444;
}
@@ -277,20 +299,33 @@ static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 at
switch (type) {
case hwmon_temp:
- ret = sch5627_update_temp(data);
- if (ret < 0)
- return ret;
switch (attr) {
case hwmon_temp_input:
+ ret = sch5627_update_temp(data);
+ if (ret < 0)
+ return ret;
+
*val = reg_to_temp(data->temp[channel]);
return 0;
case hwmon_temp_max:
- *val = reg_to_temp_limit(data->temp_max[channel]);
+ ret = regmap_read(data->regmap, SCH5627_REG_TEMP_ABS[channel], &value);
+ if (ret < 0)
+ return ret;
+
+ *val = reg_to_temp_limit((u8)value);
return 0;
case hwmon_temp_crit:
- *val = reg_to_temp_limit(data->temp_crit[channel]);
+ ret = regmap_read(data->regmap, SCH5627_REG_TEMP_HIGH[channel], &value);
+ if (ret < 0)
+ return ret;
+
+ *val = reg_to_temp_limit((u8)value);
return 0;
case hwmon_temp_fault:
+ ret = sch5627_update_temp(data);
+ if (ret < 0)
+ return ret;
+
*val = (data->temp[channel] == 0);
return 0;
default:
@@ -298,23 +333,35 @@ 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 = sch5627_update_fan(data);
+ if (ret < 0)
+ return ret;
+
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]);
+ ret = sch56xx_regmap_read16(data->regmap, SCH5627_REG_FAN_MIN[channel],
+ &value);
if (ret < 0)
return ret;
+
+ ret = reg_to_rpm((u16)value);
+ if (ret < 0)
+ return ret;
+
*val = ret;
return 0;
case hwmon_fan_fault:
+ ret = sch5627_update_fan(data);
+ if (ret < 0)
+ return ret;
+
*val = (data->fan[channel] == 0xffff);
return 0;
default:
@@ -378,8 +425,33 @@ static int sch5627_write(struct device *dev, enum hwmon_sensor_types type, u32 a
long val)
{
struct sch5627_data *data = dev_get_drvdata(dev);
+ u16 fan;
+ u8 temp;
switch (type) {
+ case hwmon_temp:
+ temp = sch5627_temp_limit_to_reg(val);
+
+ switch (attr) {
+ case hwmon_temp_max:
+ return regmap_write(data->regmap, SCH5627_REG_TEMP_ABS[channel], temp);
+ case hwmon_temp_crit:
+ return regmap_write(data->regmap, SCH5627_REG_TEMP_HIGH[channel], temp);
+ default:
+ break;
+ }
+ break;
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_min:
+ fan = sch5627_rpm_to_reg(val);
+
+ return sch56xx_regmap_write16(data->regmap, SCH5627_REG_FAN_MIN[channel],
+ fan);
+ default:
+ break;
+ }
+ break;
case hwmon_pwm:
switch (attr) {
case hwmon_pwm_auto_channels_temp:
@@ -449,7 +521,7 @@ static int sch5627_probe(struct platform_device *pdev)
{
struct sch5627_data *data;
struct device *hwmon_dev;
- int err, build_code, build_id, hwmon_rev, val;
+ int build_code, build_id, hwmon_rev, val;
data = devm_kzalloc(&pdev->dev, sizeof(struct sch5627_data),
GFP_KERNEL);
@@ -525,14 +597,6 @@ static int sch5627_probe(struct platform_device *pdev)
sch56xx_write_virtual_reg(data->addr, SCH5627_REG_CTRL, data->control | SCH5627_CTRL_VBAT);
data->last_battery = jiffies;
- /*
- * Read limits, we do this only once as reading a register on
- * the sch5627 is quite expensive (and they don't change).
- */
- err = sch5627_read_limits(data);
- if (err)
- return err;
-
pr_info("found %s chip at %#hx\n", DEVNAME, data->addr);
pr_info("firmware build: code 0x%02X, id 0x%04X, hwmon: rev 0x%02X\n",
build_code, build_id, hwmon_rev);
diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
index a3abafdaa3e6..36b38626dcdf 100644
--- a/drivers/hwmon/sch56xx-common.c
+++ b/drivers/hwmon/sch56xx-common.c
@@ -253,6 +253,37 @@ EXPORT_SYMBOL(sch56xx_read_virtual_reg12);
* Regmap support
*/
+int sch56xx_regmap_read16(struct regmap *map, unsigned int reg, unsigned int *val)
+{
+ int lsb, msb, ret;
+
+ /* See sch56xx_read_virtual_reg16() */
+ ret = regmap_read(map, reg, &lsb);
+ if (ret < 0)
+ return ret;
+
+ ret = regmap_read(map, reg + 1, &msb);
+ if (ret < 0)
+ return ret;
+
+ *val = lsb | (msb << 8);
+
+ return 0;
+}
+EXPORT_SYMBOL(sch56xx_regmap_read16);
+
+int sch56xx_regmap_write16(struct regmap *map, unsigned int reg, unsigned int val)
+{
+ int ret;
+
+ ret = regmap_write(map, reg, val & 0xff);
+ if (ret < 0)
+ return ret;
+
+ return regmap_write(map, reg + 1, (val >> 8) & 0xff);
+}
+EXPORT_SYMBOL(sch56xx_regmap_write16);
+
static int sch56xx_reg_write(void *context, unsigned int reg, unsigned int val)
{
struct sch56xx_bus_context *bus = context;
diff --git a/drivers/hwmon/sch56xx-common.h b/drivers/hwmon/sch56xx-common.h
index 3fb1cddbf977..7479a549a026 100644
--- a/drivers/hwmon/sch56xx-common.h
+++ b/drivers/hwmon/sch56xx-common.h
@@ -11,6 +11,9 @@ struct sch56xx_watchdog_data;
struct regmap *devm_regmap_init_sch56xx(struct device *dev, struct mutex *lock, u16 addr,
const struct regmap_config *config);
+int sch56xx_regmap_read16(struct regmap *map, unsigned int reg, unsigned int *val);
+int sch56xx_regmap_write16(struct regmap *map, unsigned int reg, unsigned int val);
+
int sch56xx_read_virtual_reg(u16 addr, u16 reg);
int sch56xx_write_virtual_reg(u16 addr, u16 reg, u8 val);
int sch56xx_read_virtual_reg16(u16 addr, u16 reg);
--
2.39.2
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 4/5] hwmon: (sch5627) Add support for writing limit registers
2023-09-07 5:26 ` [PATCH v2 4/5] hwmon: (sch5627) Add support for writing limit registers Armin Wolf
@ 2023-09-11 0:54 ` Guenter Roeck
0 siblings, 0 replies; 11+ messages in thread
From: Guenter Roeck @ 2023-09-11 0:54 UTC (permalink / raw)
To: Armin Wolf; +Cc: hdegoede, jdelvare, linux-hwmon, linux-kernel
On Thu, Sep 07, 2023 at 07:26:38AM +0200, Armin Wolf wrote:
> After some testing on a Fujitsu Esprimo P720, it turned out that
> the limit registers are indeed writable and affect the fan control
> algorithm. This is supported by the datasheet, which says that the
> fan control functions are based on the limit and parameter registers.
> Since accessing those registers is very inefficient, the existing
> regmap cache is used to cache those registers values.
>
> Tested on a Fujitsu Esprimo P720.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Applied.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 5/5] hwmon: (sch5627) Document behaviour of limit registers
2023-09-07 5:26 [PATCH v2 0/5] hwmon: (sch5627) Make limit registers writable Armin Wolf
` (3 preceding siblings ...)
2023-09-07 5:26 ` [PATCH v2 4/5] hwmon: (sch5627) Add support for writing limit registers Armin Wolf
@ 2023-09-07 5:26 ` Armin Wolf
2023-09-11 0:54 ` Guenter Roeck
4 siblings, 1 reply; 11+ messages in thread
From: Armin Wolf @ 2023-09-07 5:26 UTC (permalink / raw)
To: hdegoede; +Cc: jdelvare, linux, linux-hwmon, linux-kernel
The values of the limit registers affect the fan speed in a
particular way. Document this behaviour so that future users
can exploit it if required.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
Documentation/hwmon/sch5627.rst | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/hwmon/sch5627.rst b/Documentation/hwmon/sch5627.rst
index ecb4fc84d045..8639dff234fc 100644
--- a/Documentation/hwmon/sch5627.rst
+++ b/Documentation/hwmon/sch5627.rst
@@ -33,3 +33,13 @@ The hardware monitoring part of the SMSC SCH5627 is accessed by talking
through an embedded microcontroller. An application note describing the
protocol for communicating with the microcontroller is available upon
request. Please mail me if you want a copy.
+
+
+Controlling fan speed
+---------------------
+
+The SCH5627 allows for partially controlling the fan speed. If a temperature
+channel excedes tempX_max, all fans are forced to maximum speed. The same is not
+true for tempX_crit, presumably some other measures to cool down the system are
+take in this case.
+In which way the value of fanX_min affects the fan speed is currently unknown.
--
2.39.2
^ permalink raw reply [flat|nested] 11+ messages in thread