| Message ID | 20220203212853.238739-1-chunkeey@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] hwmon: (tc654) Add thermal_cooling device support | expand |
On Thu, Feb 03, 2022 at 10:28:53PM +0100, 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. > > Note: Driver uses imply THERMAL. The driver previously worked fine with > just the hwmon interface. Now, if CONFIG_THERMAL is not selected, the > devm_thermal_of_cooling_device_register() will be a > "return ERR_PTR(-ENODEV)" stub. What good does this do ? THERMAL is bool, so it is either there or it isn't, and the IS_ENABLED() in the code handles that. According to Kconfig language, "imply THERMAL" means that THERMAL could be n, m, or y if SENSORS_TC654=y. THERMAL=m would, if it were supported, be clearly wrong in that case. Prior to commit 554b3529fe018 ("thermal/drivers/core: Remove the module Kconfig's option"), THERMAL was tristate, and you would have needed something like "depends on THERMAL || THERMAL=n". This is, however, no longer needed. Given this, I really don't understand what the imply is expected to do. The above text does not explain that. Please either drop the imply or provide a better explanation why it is needed. > > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > --- > drivers/hwmon/Kconfig | 1 + > drivers/hwmon/tc654.c | 95 +++++++++++++++++++++++++++++++++++-------- > 2 files changed, 79 insertions(+), 17 deletions(-) > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index 8df25f1079ba..aa570bb05b38 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1135,6 +1135,7 @@ config SENSORS_MLXREG_FAN > config SENSORS_TC654 > tristate "Microchip TC654/TC655 and compatibles" > depends on I2C > + imply THERMAL > help > If you say yes here you get support for TC654 and TC655. > The TC654 and TC655 are PWM mode fan speed controllers with > diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c > index cf2a3acd5c91..bf7aae62477a 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 { > @@ -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; > + else > + return tc654_pwm_map[data->duty_cycle]; > +} > + > 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); > > - if (data->config & TC654_REG_CONFIG_SDM) > - pwm = 0; > - else > - pwm = tc654_pwm_map[data->duty_cycle]; > - > - return sprintf(buf, "%d\n", pwm); > + return sprintf(buf, "%d\n", get_pwm(data)); > } > > -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) > @@ -416,6 +411,22 @@ 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) Please align continuation lines with '('. > +{ > + 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; > + > + ret = _set_pwm(data, val); > return ret < 0 ? ret : count; > } > > @@ -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)); > +} > + > +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 +526,19 @@ 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); Please align continuation lines with '('. You can go up to 100 columns. > + return PTR_ERR_OR_ZERO(cdev); > + } > + > + return 0; > } > > static const struct i2c_device_id tc654_id[] = {
On 05/02/2022 19:23, Guenter Roeck wrote: > On Thu, Feb 03, 2022 at 10:28:53PM +0100, 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. >> >> Note: Driver uses imply THERMAL. The driver previously worked fine with >> just the hwmon interface. Now, if CONFIG_THERMAL is not selected, the >> devm_thermal_of_cooling_device_register() will be a >> "return ERR_PTR(-ENODEV)" stub. > > What good does this do ? THERMAL is bool, so it is either there > or it isn't, and the IS_ENABLED() in the code handles that. > > According to Kconfig language, "imply THERMAL" means that > THERMAL could be n, m, or y if SENSORS_TC654=y. THERMAL=m would, > if it were supported, be clearly wrong in that case. > > Prior to commit 554b3529fe018 ("thermal/drivers/core: Remove the > module Kconfig's option"), THERMAL was tristate, and you would have > needed something like "depends on THERMAL || THERMAL=n". This > is, however, no longer needed. > > Given this, I really don't understand what the imply is expected > to do. The above text does not explain that. Please either drop > the imply or provide a better explanation why it is needed. Oh, okay. Yes, you are right. A lot happend since 2016, I have to admit, I was updating the driver code, but haven't kept track with the THERMAL change. So, I'm planning to address your comments in the following way: - Drop imply THERMAL (no replacement) - add __maybe_unused to the new functions+struct In the !THERMAL case, compilers will spot the static declarations. If you prefer, I could instead add a #ifdef CONFIG_THERMAL [...] #endif around the new cooling functions and the tc654_fan_cool_ops struct. - Changed in tc654_probe() that IS_ENABLED(CONFIG_THERMAL) to IS_BUILTIN(CONFIG_THERMAL) Thanks again for the pointer :). Cheers, Christian
On 2/5/22 13:05, Christian Lamparter wrote: > On 05/02/2022 19:23, Guenter Roeck wrote: >> On Thu, Feb 03, 2022 at 10:28:53PM +0100, 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. >>> >>> Note: Driver uses imply THERMAL. The driver previously worked fine with >>> just the hwmon interface. Now, if CONFIG_THERMAL is not selected, the >>> devm_thermal_of_cooling_device_register() will be a >>> "return ERR_PTR(-ENODEV)" stub. >> >> What good does this do ? THERMAL is bool, so it is either there >> or it isn't, and the IS_ENABLED() in the code handles that. >> >> According to Kconfig language, "imply THERMAL" means that >> THERMAL could be n, m, or y if SENSORS_TC654=y. THERMAL=m would, >> if it were supported, be clearly wrong in that case. >> >> Prior to commit 554b3529fe018 ("thermal/drivers/core: Remove the >> module Kconfig's option"), THERMAL was tristate, and you would have >> needed something like "depends on THERMAL || THERMAL=n". This >> is, however, no longer needed. >> >> Given this, I really don't understand what the imply is expected >> to do. The above text does not explain that. Please either drop >> the imply or provide a better explanation why it is needed. > > Oh, okay. Yes, you are right. A lot happend since 2016, I have to admit, > I was updating the driver code, but haven't kept track with the THERMAL > change. > > So, I'm planning to address your comments in the following way: > - Drop imply THERMAL (no replacement) > - add __maybe_unused to the new functions+struct > In the !THERMAL case, compilers will spot the static declarations. > If you prefer, I could instead add a #ifdef CONFIG_THERMAL [...] #endif > around the new cooling functions and the tc654_fan_cool_ops struct. > > - Changed in tc654_probe() that IS_ENABLED(CONFIG_THERMAL) to > IS_BUILTIN(CONFIG_THERMAL) > Please no #ifdef, and __maybe_unused should not be necessary unless you use #ifdef. I don't understand why IS_BUILTIN() instead of IS_ENABLED() would be necessary. /* * IS_BUILTIN(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y', 0 * otherwise. For boolean options, this is equivalent to * IS_ENABLED(CONFIG_FOO). */ Thanks, Guenter
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 8df25f1079ba..aa570bb05b38 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1135,6 +1135,7 @@ config SENSORS_MLXREG_FAN config SENSORS_TC654 tristate "Microchip TC654/TC655 and compatibles" depends on I2C + imply THERMAL help If you say yes here you get support for TC654 and TC655. The TC654 and TC655 are PWM mode fan speed controllers with diff --git a/drivers/hwmon/tc654.c b/drivers/hwmon/tc654.c index cf2a3acd5c91..bf7aae62477a 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 { @@ -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; + else + return tc654_pwm_map[data->duty_cycle]; +} + 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); - if (data->config & TC654_REG_CONFIG_SDM) - pwm = 0; - else - pwm = tc654_pwm_map[data->duty_cycle]; - - return sprintf(buf, "%d\n", pwm); + return sprintf(buf, "%d\n", get_pwm(data)); } -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) @@ -416,6 +411,22 @@ 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; + + ret = _set_pwm(data, val); return ret < 0 ? ret : count; } @@ -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)); +} + +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 +526,19 @@ 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[] = {
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. Note: Driver uses imply THERMAL. The driver previously worked fine with just the hwmon interface. Now, if CONFIG_THERMAL is not selected, the devm_thermal_of_cooling_device_register() will be a "return ERR_PTR(-ENODEV)" stub. Signed-off-by: Christian Lamparter <chunkeey@gmail.com> --- drivers/hwmon/Kconfig | 1 + drivers/hwmon/tc654.c | 95 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 79 insertions(+), 17 deletions(-)