| Message ID | 20190418200452.5703-1-jeff.dagenais@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] hwmon: max6650: add thermal cooling device capability | expand |
On Thu, Apr 18, 2019 at 04:04:52PM -0400, Jean-Francois Dagenais wrote: > This allows max6650 devices to be referenced in dts as a cooling device. > > The pwm value seems duplicated in cooling_dev_state but since pwm goes > through rounding logic into data->dac, it is modified and messes with > the thermal zone state algorithms. It's also better to serve a cache > value, thus avoiding periodic actual i2c traffic. > > Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com> > --- > Changes in v2: > Removed left-over debug printk. > Changes in v3: > Add missing dependency in Kconfig > > drivers/hwmon/Kconfig | 1 + > drivers/hwmon/max6650.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 91 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index d0f1dfe2bcbb..46d69fcdd48b 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -898,6 +898,7 @@ config SENSORS_MAX6642 > config SENSORS_MAX6650 > tristate "Maxim MAX6650 sensor chip" > depends on I2C > + depends on THERMAL || THERMAL=n > help > If you say yes here you get support for the MAX6650 / MAX6651 > sensor chips. > diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c > index 61135a2d0cff..f1cbbaaa1206 100644 > --- a/drivers/hwmon/max6650.c > +++ b/drivers/hwmon/max6650.c > @@ -40,6 +40,7 @@ > #include <linux/hwmon-sysfs.h> > #include <linux/err.h> > #include <linux/of_device.h> > +#include <linux/thermal.h> > > /* > * Insmod parameters > @@ -113,6 +114,7 @@ module_param(clock, int, 0444); > struct max6650_data { > struct i2c_client *client; > const struct attribute_group *groups[3]; > + struct thermal_cooling_device *cooling_dev; > struct mutex update_lock; > int nr_fans; > char valid; /* zero until following fields are valid */ > @@ -125,6 +127,7 @@ struct max6650_data { > u8 count; > u8 dac; > u8 alarm; > + unsigned long cooling_dev_state; > }; > > static const u8 tach_reg[] = { > @@ -694,6 +697,63 @@ static int max6650_init_client(struct max6650_data *data, > return 0; > } > > +#if IS_ENABLED(CONFIG_THERMAL) > +/* thermal cooling device callbacks */ > +static int max6650_get_max_state(struct thermal_cooling_device *cdev, > + unsigned long *state) > +{ > + *state = 255; > + > + return 0; > +} > + > +static int max6650_get_cur_state(struct thermal_cooling_device *cdev, > + unsigned long *state) > +{ > + struct max6650_data *data = cdev->devdata; > + > + *state = data->cooling_dev_state; > + > + return 0; > +} > + > +static int > +max6650_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state) > +{ > + struct max6650_data *data = cdev->devdata; > + struct i2c_client *client = data->client; > + int err; > + > + state = clamp_val(state, 0, 255); > + > + mutex_lock(&data->update_lock); > + > + if (data->config & MAX6650_CFG_V12) > + data->dac = 180 - (180 * state)/255; > + else > + data->dac = 76 - (76 * state)/255; > + > + err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac); > + > + if (!err) { > + max6650_set_operating_mode(data, state ? > + MAX6650_CFG_MODE_OPEN_LOOP : > + MAX6650_CFG_MODE_OFF); > + data->cooling_dev_state = state; > + } > + > + mutex_unlock(&data->update_lock); > + > + return err < 0 ? err : 0; > +} > + > +static const struct thermal_cooling_device_ops max6650_cooling_ops = { > + .get_max_state = max6650_get_max_state, > + .get_cur_state = max6650_get_cur_state, > + .set_cur_state = max6650_set_cur_state, > +}; > +#endif > + > static int max6650_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -709,6 +769,7 @@ static int max6650_probe(struct i2c_client *client, > return -ENOMEM; > > data->client = client; > + i2c_set_clientdata(client, data); > mutex_init(&data->update_lock); > data->nr_fans = of_id ? (int)(uintptr_t)of_id->data : id->driver_data; > > @@ -727,7 +788,34 @@ static int max6650_probe(struct i2c_client *client, > hwmon_dev = devm_hwmon_device_register_with_groups(dev, > client->name, data, > data->groups); > - return PTR_ERR_OR_ZERO(hwmon_dev); > + err = PTR_ERR_OR_ZERO(hwmon_dev); > + if (err) > + return err; > + > +#if IS_ENABLED(CONFIG_THERMAL) > + data->cooling_dev = > + thermal_of_cooling_device_register(client->dev.of_node, > + id->name, data, > + &max6650_cooling_ops); > + if (IS_ERR(data->cooling_dev)) { > + err = PTR_ERR(data->cooling_dev); > + dev_err(&client->dev, > + "Failed to register as cooling device (%d)\n", err); As mentioned in my other mail, this message adds zero value. Please drop. > + return err; Sorry, I won't accept this. Guenter > + } > + > + thermal_cdev_update(data->cooling_dev); > +#endif > + return 0; > +} > + > +static int max6650_remove(struct i2c_client *client) > +{ > + struct max6650_data *data = i2c_get_clientdata(client); > + > + thermal_cooling_device_unregister(data->cooling_dev); > + > + return 0; > } > > static const struct i2c_device_id max6650_id[] = { > @@ -743,6 +831,7 @@ static struct i2c_driver max6650_driver = { > .of_match_table = of_match_ptr(max6650_dt_match), > }, > .probe = max6650_probe, > + .remove = max6650_remove, > .id_table = max6650_id, > }; > > -- > 2.11.0 >
> On Apr 18, 2019, at 16:13, Guenter Roeck <linux@roeck-us.net> wrote: > >> + if (IS_ERR(data->cooling_dev)) { >> + err = PTR_ERR(data->cooling_dev); >> + dev_err(&client->dev, >> + "Failed to register as cooling device (%d)\n", err); > > As mentioned in my other mail, this message adds zero value. Please drop. How does one distinguish the different failures which can occur within a probe function then. I know it is only useful for system integrators while debugging DTS for example. But I find my self always having to insert these kind of messages at each "return" statements of a failing probe implementation. I just think it's nice for developers which can use the info to quickly troubleshoot the problem. I won't fight this though. You are maintainer of this subsys so... > >> + return err; > > Sorry, I won't accept this. You mean, you won't accept v3, but a v4 without the dev_err? Perhaps also, we should wait a v4 which would use your new devm_register... ?
On Thu, Apr 18, 2019 at 04:45:00PM -0400, Jean-Francois Dagenais wrote: > > > On Apr 18, 2019, at 16:13, Guenter Roeck <linux@roeck-us.net> wrote: > > > >> + if (IS_ERR(data->cooling_dev)) { > >> + err = PTR_ERR(data->cooling_dev); > >> + dev_err(&client->dev, > >> + "Failed to register as cooling device (%d)\n", err); > > > > As mentioned in my other mail, this message adds zero value. Please drop. > > How does one distinguish the different failures which can occur within a probe > function then. I know it is only useful for system integrators while debugging > DTS for example. But I find my self always having to insert these kind of messages > at each "return" statements of a failing probe implementation. I just think it's > nice for developers which can use the info to quickly troubleshoot the problem. > > I won't fight this though. You are maintainer of this subsys so... > Yes, I know there are peple who like a lot of messages. I don't - it happend to me too often that the actual important message(s) get lost in the noise. Anyway, see below. > > > >> + return err; > > > > Sorry, I won't accept this. > > You mean, you won't accept v3, but a v4 without the dev_err? > I won't accept v3 due to the error return, which may be considered a regression for those who don't care about using the driver with the thermal subsystem. If you insist on an extra message, I'll accept a dev_warn. > Perhaps also, we should wait a v4 which would use your new devm_register... ? Your call, but there is no guarantee that this series will ever be accepted. The thermal maintainers may object to the new devm function. Guenter
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index d0f1dfe2bcbb..46d69fcdd48b 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -898,6 +898,7 @@ config SENSORS_MAX6642 config SENSORS_MAX6650 tristate "Maxim MAX6650 sensor chip" depends on I2C + depends on THERMAL || THERMAL=n help If you say yes here you get support for the MAX6650 / MAX6651 sensor chips. diff --git a/drivers/hwmon/max6650.c b/drivers/hwmon/max6650.c index 61135a2d0cff..f1cbbaaa1206 100644 --- a/drivers/hwmon/max6650.c +++ b/drivers/hwmon/max6650.c @@ -40,6 +40,7 @@ #include <linux/hwmon-sysfs.h> #include <linux/err.h> #include <linux/of_device.h> +#include <linux/thermal.h> /* * Insmod parameters @@ -113,6 +114,7 @@ module_param(clock, int, 0444); struct max6650_data { struct i2c_client *client; const struct attribute_group *groups[3]; + struct thermal_cooling_device *cooling_dev; struct mutex update_lock; int nr_fans; char valid; /* zero until following fields are valid */ @@ -125,6 +127,7 @@ struct max6650_data { u8 count; u8 dac; u8 alarm; + unsigned long cooling_dev_state; }; static const u8 tach_reg[] = { @@ -694,6 +697,63 @@ static int max6650_init_client(struct max6650_data *data, return 0; } +#if IS_ENABLED(CONFIG_THERMAL) +/* thermal cooling device callbacks */ +static int max6650_get_max_state(struct thermal_cooling_device *cdev, + unsigned long *state) +{ + *state = 255; + + return 0; +} + +static int max6650_get_cur_state(struct thermal_cooling_device *cdev, + unsigned long *state) +{ + struct max6650_data *data = cdev->devdata; + + *state = data->cooling_dev_state; + + return 0; +} + +static int +max6650_set_cur_state(struct thermal_cooling_device *cdev, unsigned long state) +{ + struct max6650_data *data = cdev->devdata; + struct i2c_client *client = data->client; + int err; + + state = clamp_val(state, 0, 255); + + mutex_lock(&data->update_lock); + + if (data->config & MAX6650_CFG_V12) + data->dac = 180 - (180 * state)/255; + else + data->dac = 76 - (76 * state)/255; + + err = i2c_smbus_write_byte_data(client, MAX6650_REG_DAC, data->dac); + + if (!err) { + max6650_set_operating_mode(data, state ? + MAX6650_CFG_MODE_OPEN_LOOP : + MAX6650_CFG_MODE_OFF); + data->cooling_dev_state = state; + } + + mutex_unlock(&data->update_lock); + + return err < 0 ? err : 0; +} + +static const struct thermal_cooling_device_ops max6650_cooling_ops = { + .get_max_state = max6650_get_max_state, + .get_cur_state = max6650_get_cur_state, + .set_cur_state = max6650_set_cur_state, +}; +#endif + static int max6650_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -709,6 +769,7 @@ static int max6650_probe(struct i2c_client *client, return -ENOMEM; data->client = client; + i2c_set_clientdata(client, data); mutex_init(&data->update_lock); data->nr_fans = of_id ? (int)(uintptr_t)of_id->data : id->driver_data; @@ -727,7 +788,34 @@ static int max6650_probe(struct i2c_client *client, hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name, data, data->groups); - return PTR_ERR_OR_ZERO(hwmon_dev); + err = PTR_ERR_OR_ZERO(hwmon_dev); + if (err) + return err; + +#if IS_ENABLED(CONFIG_THERMAL) + data->cooling_dev = + thermal_of_cooling_device_register(client->dev.of_node, + id->name, data, + &max6650_cooling_ops); + if (IS_ERR(data->cooling_dev)) { + err = PTR_ERR(data->cooling_dev); + dev_err(&client->dev, + "Failed to register as cooling device (%d)\n", err); + return err; + } + + thermal_cdev_update(data->cooling_dev); +#endif + return 0; +} + +static int max6650_remove(struct i2c_client *client) +{ + struct max6650_data *data = i2c_get_clientdata(client); + + thermal_cooling_device_unregister(data->cooling_dev); + + return 0; } static const struct i2c_device_id max6650_id[] = { @@ -743,6 +831,7 @@ static struct i2c_driver max6650_driver = { .of_match_table = of_match_ptr(max6650_dt_match), }, .probe = max6650_probe, + .remove = max6650_remove, .id_table = max6650_id, };
This allows max6650 devices to be referenced in dts as a cooling device. The pwm value seems duplicated in cooling_dev_state but since pwm goes through rounding logic into data->dac, it is modified and messes with the thermal zone state algorithms. It's also better to serve a cache value, thus avoiding periodic actual i2c traffic. Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com> --- Changes in v2: Removed left-over debug printk. Changes in v3: Add missing dependency in Kconfig drivers/hwmon/Kconfig | 1 + drivers/hwmon/max6650.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 1 deletion(-)