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@xxxxxxxxx>
> ---
> 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[] = {
