On 07/02/2022 09:47, Guenter Roeck wrote:
On 2/6/22 11:05, Christian Lamparter wrote:Adds thermal_cooling device support to the tc654/tc655 driver. This make it possible to integrate it into a device-tree supported thermal-zone node as a cooling device. I have been using this patch as part of the Netgear WNDR4700 Centria NAS Router support within OpenWrt since 2016. Signed-off-by: Christian Lamparter <chunkeey@xxxxxxxxx> --- v1 -> v2: - Drop imply THERMAL - aligned ( so . everything is lining up --- drivers/hwmon/tc654.c | 94 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 17 deletions(-) diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c --- a/drivers/hwmon/tc654.c +++ b/drivers/hwmon/tc654.c @@ -367,36 +368,30 @@ static ssize_t pwm_mode_store(struct device *dev, struct device_attribute *da, static const int tc654_pwm_map[16] = { 77, 88, 102, 112, 124, 136, 148, 160, 172, 184, 196, 207, 219, 231, 243, 255}; +static int get_pwm(struct tc654_data *data) +{ + if (data->config & TC654_REG_CONFIG_SDM) + return 0; + elseNit: else after return is unnecessary. Sorry for not noticing before.
Noted.
@@ -447,6 +458,44 @@ static struct attribute *tc654_attrs[] = { ATTRIBUTE_GROUPS(tc654); +/* cooling device */ + +static int tc654_get_max_state(struct thermal_cooling_device *cdev, + unsigned long *state) +{ + *state = 255; + return 0; +} + +static int tc654_get_cur_state(struct thermal_cooling_device *cdev, + unsigned long *state) +{ + struct tc654_data *data = tc654_update_client(cdev->devdata); + + if (IS_ERR(data)) + return PTR_ERR(data); + + *state = get_pwm(data); + return 0; +} + +static int tc654_set_cur_state(struct thermal_cooling_device *cdev, + unsigned long state) +{ + struct tc654_data *data = tc654_update_client(cdev->devdata); + + if (IS_ERR(data)) + return PTR_ERR(data); + + return _set_pwm(data, clamp_val(state, 0, 255)); +}Looking into this further, wouldn't it be more appropriate to limit the cooling states to [0 .. 15], or in other words use data->duty_cycle directly ? I don't know how the thermal subsystem reacts if it requests to set the state to a certain value and the actual state is set to something completely different. Also, it doesn't seem to make much sense to bother the thermal subsystem with 256 states if the chip really only supports 16 states.
So there's more: Yes, the chip has 16 PWM states (from Duty Cycle 0=30% to Duty Cycle 15=100% - each step has a linear 4,66...% increment). The chip also has a "shutdown state/mode". where the FAN(s) are all turned off (only the chip's I2C interface remains active for the wake-up signal). This is why even the current pwm_show() looks so funny. <https://github.com/torvalds/linux/blob/master/drivers/hwmon/tc654.c#L365-L380> |static ssize_t pwm_show(struct device *dev, struct device_attribute *da, | char *buf) |{ | struct tc654_data *data = tc654_update_client(dev); | int pwm; | | if (IS_ERR(data)) | return PTR_ERR(data); |---- from here ---- | if (data->config & TC654_REG_CONFIG_SDM) | pwm = 0; | else | pwm = tc654_pwm_map[data->duty_cycle]; |---- to here ---- | return sprintf(buf, "%d\n", pwm); |} If the chip is in that TC654_REG_CONFIG_SDM (SDM=Shutdown Mode), the PWM is 0% and the fan is off. If it's not in TC654_REG_CONFIG_SDM, then that duty_cycle=0 gives you a PWM with 30%. The same goes for the pwm_store. And yes, the current behavior through the hwmon sysfs interface is a bit extreme: pwm_store with val=0 => SDM engaged - Fans turn off. pwm_store with val=1 => Fans are on 30%. (There's a steep cliff / steep climb) ... pwm_store with val=~78 > Fans are on 34% ... pwm_store with val=255 = Fans are on 100% So, I would like to keep that shutdown state in there. The Fan on the WNDR4700 is annoying and only needs to run from time to time. For now, I fiddled around by adding +1 and -1 here and there. Another option would be to extend the tc654_pwm_map. But this has the consequence that it changes the existing behavior on the hwmon interface as well. I've inlined a preliminary git diff patch, if you want to take a peek and maybe already have comments. I want to test this on the hardware, before sending out a version... Which I only can do on weekends (so it will be a week, hope that's ok). Thanks, Christian --- diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c index cf2a3acd5c91..f6b6375ffeaf 100644 --- a/drivers/hwmon/tc654.c +++ b/drivers/hwmon/tc654.c @@ -15,6 +15,7 @@ #include <linux/module.h> #include <linux/mutex.h> #include <linux/slab.h> +#include <linux/thermal.h> #include <linux/util_macros.h> enum tc654_regs { @@ -384,28 +385,19 @@ static ssize_t pwm_show(struct device *dev, struct device_attribute *da, return sprintf(buf, "%d\n", pwm); } -static ssize_t pwm_store(struct device *dev, struct device_attribute *da, - const char *buf, size_t count) +static int _set_pwm(struct tc654_data *data, unsigned long val) { - struct tc654_data *data = dev_get_drvdata(dev); struct i2c_client *client = data->client; - unsigned long val; int ret; - if (kstrtoul(buf, 10, &val)) - return -EINVAL; - if (val > 255) - return -EINVAL; - mutex_lock(&data->update_lock); - if (val == 0) + if (val == 0) { data->config |= TC654_REG_CONFIG_SDM; - else + } else { data->config &= ~TC654_REG_CONFIG_SDM; - - data->duty_cycle = find_closest(val, tc654_pwm_map, - ARRAY_SIZE(tc654_pwm_map)); + data->duty_cycle = val - 1; + } ret = i2c_smbus_write_byte_data(client, TC654_REG_CONFIG, data->config); if (ret < 0) @@ -416,6 +408,24 @@ static ssize_t pwm_store(struct device *dev, struct device_attribute *da, out: mutex_unlock(&data->update_lock); + return ret; +} + +static ssize_t pwm_store(struct device *dev, struct device_attribute *da, + const char *buf, size_t count) +{ + struct tc654_data *data = dev_get_drvdata(dev); + unsigned long val; + int ret; + + if (kstrtoul(buf, 10, &val)) + return -EINVAL; + if (val > 255) + return -EINVAL; + if (val > 0) + val = find_closest(val, tc654_pwm_map, ARRAY_SIZE(tc654_pwm_map)) + 1; + + ret = _set_pwm(data, val); return ret < 0 ? ret : count; } @@ -447,6 +457,56 @@ static struct attribute *tc654_attrs[] = { ATTRIBUTE_GROUPS(tc654); +/* thermal cooling device functions + * + * Account for the "ShutDown Mode (SDM)" state by offseting + * the 16 PWM duty cycle states by 1. + * + * State 0 = 0% PWM | Shutdown - Fan(s) are off + * State 1 = 30% PWM | duty_cycle = 0 + * State 2 = ~35% PWM | duty_cycle = 1 + * [...] + * State 15 = ~95% PWM | duty_cycle = 14 + * State 16 = 100% PWM | duty_cycle = 15 + */ +#define TC654_MAX_COOLING_STATES 16 + +static int tc654_get_max_state(struct thermal_cooling_device *cdev, unsigned long *state) +{ + return TC654_MAX_COOLING_STATES; +} + +static int tc654_get_cur_state(struct thermal_cooling_device *cdev, unsigned long *state) +{ + struct tc654_data *data = tc654_update_client(cdev->devdata); + + if (IS_ERR(data)) + return PTR_ERR(data); + + if (data->config & TC654_REG_CONFIG_SDM) + *state = 0; /* FAN is off */ + else + *state = data->duty_cycle + 1; /* offset PWM States by 1 */ + + return 0; +} + +static int tc654_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state) +{ + struct tc654_data *data = tc654_update_client(cdev->devdata); + + if (IS_ERR(data)) + return PTR_ERR(data); + + return _set_pwm(data, clamp_val(state, 0, TC654_MAX_COOLING_STATES)); +} + +static const struct thermal_cooling_device_ops tc654_fan_cool_ops = { + .get_max_state = tc654_get_max_state, + .get_cur_state = tc654_get_cur_state, + .set_cur_state = tc654_set_cur_state, +}; + /* * device probe and removal */ @@ -477,7 +537,18 @@ static int tc654_probe(struct i2c_client *client) hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name, data, tc654_groups); - return PTR_ERR_OR_ZERO(hwmon_dev); + if (IS_ERR(hwmon_dev)) + return PTR_ERR(hwmon_dev); + + if (IS_ENABLED(CONFIG_THERMAL)) { + struct thermal_cooling_device *cdev; + + cdev = devm_thermal_of_cooling_device_register(dev, dev->of_node, client->name, + hwmon_dev, &tc654_fan_cool_ops); + return PTR_ERR_OR_ZERO(cdev); + } + + return 0; } static const struct i2c_device_id tc654_id[] = { ---
