| Message ID | 20200417162020.19980-2-andrzej.p@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v3,1/2] thermal: core: Let thermal zone device's mode be stored in its struct | expand |
On Fri, Apr 17, 2020 at 7:20 PM Andrzej Pietrasiewicz <andrzej.p@collabora.com> wrote: > > Thermal zone devices' mode is stored in individual drivers. This patch > changes it so that mode is stored in struct thermal_zone_device instead. > > As a result all driver-specific variables storing the mode are not needed > and are removed. Consequently, the get_mode() implementations have nothing > to operate on and need to be removed, too. > > Some thermal framework specific functions are introduced: > > thermal_zone_device_get_mode() > thermal_zone_device_set_mode() > thermal_zone_device_enable() > thermal_zone_device_disable() > > thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock > and the "set" calls driver's set_mode() if provided, so the latter must > not take this lock again. At the end of the "set" > thermal_zone_device_update() is called so drivers don't need to repeat this > invocation in their specific set_mode() implementations. > > The scope of the above 4 functions is purposedly limited to the thermal > framework and drivers are not supposed to call them. This encapsulation > does not fully work at the moment for some drivers, though: > > - platform/x86/acerhdf.c Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com> for PDx86 bits. > - drivers/thermal/imx_thermal.c > - drivers/thermal/intel/intel_quark_dts_thermal.c > - drivers/thermal/of-thermal.c > > and they manipulate struct thermal_zone_device's members directly. > > struct thermal_zone_params gains a new member called initial_mode, which > is used to set tzd's mode at registration time. > > The sysfs "mode" attribute is always exposed from now on, because all > thermal zone devices now have their get_mode() implemented at the generic > level and it is always available. Exposing "mode" doesn't hurt the drivers > which don't provide their own set_mode(), because writing to "mode" will > result in -EPERM, as expected. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/acpi/thermal.c | 46 +++++---------- > .../ethernet/mellanox/mlxsw/core_thermal.c | 57 ++++--------------- > drivers/platform/x86/acerhdf.c | 17 +----- > drivers/thermal/da9062-thermal.c | 16 ++---- > drivers/thermal/imx_thermal.c | 29 +++------- > .../intel/int340x_thermal/int3400_thermal.c | 30 ++-------- > .../thermal/intel/intel_quark_dts_thermal.c | 22 ++----- > drivers/thermal/of-thermal.c | 30 +++------- > drivers/thermal/thermal_core.c | 44 +++++++++++++- > drivers/thermal/thermal_core.h | 16 ++++++ > drivers/thermal/thermal_sysfs.c | 29 +--------- > include/linux/thermal.h | 7 ++- > 12 files changed, 125 insertions(+), 218 deletions(-) > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c > index 19067a5e5293..67bc263332a5 100644 > --- a/drivers/acpi/thermal.c > +++ b/drivers/acpi/thermal.c > @@ -172,7 +172,6 @@ struct acpi_thermal { > struct acpi_thermal_trips trips; > struct acpi_handle_list devices; > struct thermal_zone_device *thermal_zone; > - int tz_enabled; > int kelvin_offset; /* in millidegrees */ > struct work_struct thermal_check_work; > }; > @@ -500,7 +499,7 @@ static void acpi_thermal_check(void *data) > { > struct acpi_thermal *tz = data; > > - if (!tz->tz_enabled) > + if (tz->thermal_zone->mode != THERMAL_DEVICE_ENABLED) > return; > > thermal_zone_device_update(tz->thermal_zone, > @@ -526,46 +525,29 @@ static int thermal_get_temp(struct thermal_zone_device *thermal, int *temp) > return 0; > } > > -static int thermal_get_mode(struct thermal_zone_device *thermal, > - enum thermal_device_mode *mode) > -{ > - struct acpi_thermal *tz = thermal->devdata; > - > - if (!tz) > - return -EINVAL; > - > - *mode = tz->tz_enabled ? THERMAL_DEVICE_ENABLED : > - THERMAL_DEVICE_DISABLED; > - > - return 0; > -} > - > static int thermal_set_mode(struct thermal_zone_device *thermal, > enum thermal_device_mode mode) > { > struct acpi_thermal *tz = thermal->devdata; > - int enable; > > if (!tz) > return -EINVAL; > > + if (mode != THERMAL_DEVICE_DISABLED && > + mode != THERMAL_DEVICE_ENABLED) > + return -EINVAL; > + > /* > * enable/disable thermal management from ACPI thermal driver > */ > - if (mode == THERMAL_DEVICE_ENABLED) > - enable = 1; > - else if (mode == THERMAL_DEVICE_DISABLED) { > - enable = 0; > + if (mode == THERMAL_DEVICE_DISABLED) > pr_warn("thermal zone will be disabled\n"); > - } else > - return -EINVAL; > > - if (enable != tz->tz_enabled) { > - tz->tz_enabled = enable; > + if (mode != thermal->mode) { > ACPI_DEBUG_PRINT((ACPI_DB_INFO, > "%s kernel ACPI thermal control\n", > - tz->tz_enabled ? "Enable" : "Disable")); > - acpi_thermal_check(tz); > + mode == THERMAL_DEVICE_ENABLED ? > + "Enable" : "Disable")); > } > return 0; > } > @@ -856,7 +838,6 @@ static struct thermal_zone_device_ops acpi_thermal_zone_ops = { > .bind = acpi_thermal_bind_cooling_device, > .unbind = acpi_thermal_unbind_cooling_device, > .get_temp = thermal_get_temp, > - .get_mode = thermal_get_mode, > .set_mode = thermal_set_mode, > .get_trip_type = thermal_get_trip_type, > .get_trip_temp = thermal_get_trip_temp, > @@ -870,6 +851,9 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) > int trips = 0; > int result; > acpi_status status; > + struct thermal_zone_params prms = { > + .initial_mode = THERMAL_DEVICE_ENABLED, > + }; > int i; > > if (tz->trips.critical.flags.valid) > @@ -887,13 +871,13 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) > if (tz->trips.passive.flags.valid) > tz->thermal_zone = > thermal_zone_device_register("acpitz", trips, 0, tz, > - &acpi_thermal_zone_ops, NULL, > + &acpi_thermal_zone_ops, &prms, > tz->trips.passive.tsp*100, > tz->polling_frequency*100); > else > tz->thermal_zone = > thermal_zone_device_register("acpitz", trips, 0, tz, > - &acpi_thermal_zone_ops, NULL, > + &acpi_thermal_zone_ops, &prms, > 0, tz->polling_frequency*100); > if (IS_ERR(tz->thermal_zone)) > return -ENODEV; > @@ -913,8 +897,6 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) > if (ACPI_FAILURE(status)) > return -ENODEV; > > - tz->tz_enabled = 1; > - > dev_info(&tz->device->dev, "registered as thermal_zone%d\n", > tz->thermal_zone->id); > return 0; > diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c > index ce0a6837daa3..50518048b86d 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c > @@ -98,7 +98,6 @@ struct mlxsw_thermal_module { > struct mlxsw_thermal *parent; > struct thermal_zone_device *tzdev; > struct mlxsw_thermal_trip trips[MLXSW_THERMAL_NUM_TRIPS]; > - enum thermal_device_mode mode; > int module; /* Module or gearbox number */ > }; > > @@ -110,7 +109,6 @@ struct mlxsw_thermal { > struct thermal_cooling_device *cdevs[MLXSW_MFCR_PWMS_MAX]; > u8 cooling_levels[MLXSW_THERMAL_MAX_STATE + 1]; > struct mlxsw_thermal_trip trips[MLXSW_THERMAL_NUM_TRIPS]; > - enum thermal_device_mode mode; > struct mlxsw_thermal_module *tz_module_arr; > u8 tz_module_num; > struct mlxsw_thermal_module *tz_gearbox_arr; > @@ -277,33 +275,16 @@ static int mlxsw_thermal_unbind(struct thermal_zone_device *tzdev, > return 0; > } > > -static int mlxsw_thermal_get_mode(struct thermal_zone_device *tzdev, > - enum thermal_device_mode *mode) > -{ > - struct mlxsw_thermal *thermal = tzdev->devdata; > - > - *mode = thermal->mode; > - > - return 0; > -} > - > static int mlxsw_thermal_set_mode(struct thermal_zone_device *tzdev, > enum thermal_device_mode mode) > { > struct mlxsw_thermal *thermal = tzdev->devdata; > > - mutex_lock(&tzdev->lock); > - > if (mode == THERMAL_DEVICE_ENABLED) > tzdev->polling_delay = thermal->polling_delay; > else > tzdev->polling_delay = 0; > > - mutex_unlock(&tzdev->lock); > - > - thermal->mode = mode; > - thermal_zone_device_update(tzdev, THERMAL_EVENT_UNSPECIFIED); > - > return 0; > } > > @@ -407,7 +388,6 @@ static int mlxsw_thermal_trend_get(struct thermal_zone_device *tzdev, > static struct thermal_zone_device_ops mlxsw_thermal_ops = { > .bind = mlxsw_thermal_bind, > .unbind = mlxsw_thermal_unbind, > - .get_mode = mlxsw_thermal_get_mode, > .set_mode = mlxsw_thermal_set_mode, > .get_temp = mlxsw_thermal_get_temp, > .get_trip_type = mlxsw_thermal_get_trip_type, > @@ -466,34 +446,17 @@ static int mlxsw_thermal_module_unbind(struct thermal_zone_device *tzdev, > return err; > } > > -static int mlxsw_thermal_module_mode_get(struct thermal_zone_device *tzdev, > - enum thermal_device_mode *mode) > -{ > - struct mlxsw_thermal_module *tz = tzdev->devdata; > - > - *mode = tz->mode; > - > - return 0; > -} > - > static int mlxsw_thermal_module_mode_set(struct thermal_zone_device *tzdev, > enum thermal_device_mode mode) > { > struct mlxsw_thermal_module *tz = tzdev->devdata; > struct mlxsw_thermal *thermal = tz->parent; > > - mutex_lock(&tzdev->lock); > - > if (mode == THERMAL_DEVICE_ENABLED) > tzdev->polling_delay = thermal->polling_delay; > else > tzdev->polling_delay = 0; > > - mutex_unlock(&tzdev->lock); > - > - tz->mode = mode; > - thermal_zone_device_update(tzdev, THERMAL_EVENT_UNSPECIFIED); > - > return 0; > } > > @@ -596,7 +559,6 @@ mlxsw_thermal_module_trip_hyst_set(struct thermal_zone_device *tzdev, int trip, > static struct thermal_zone_device_ops mlxsw_thermal_module_ops = { > .bind = mlxsw_thermal_module_bind, > .unbind = mlxsw_thermal_module_unbind, > - .get_mode = mlxsw_thermal_module_mode_get, > .set_mode = mlxsw_thermal_module_mode_set, > .get_temp = mlxsw_thermal_module_temp_get, > .get_trip_type = mlxsw_thermal_module_trip_type_get, > @@ -635,7 +597,6 @@ static int mlxsw_thermal_gearbox_temp_get(struct thermal_zone_device *tzdev, > static struct thermal_zone_device_ops mlxsw_thermal_gearbox_ops = { > .bind = mlxsw_thermal_module_bind, > .unbind = mlxsw_thermal_module_unbind, > - .get_mode = mlxsw_thermal_module_mode_get, > .set_mode = mlxsw_thermal_module_mode_set, > .get_temp = mlxsw_thermal_gearbox_temp_get, > .get_trip_type = mlxsw_thermal_module_trip_type_get, > @@ -749,6 +710,9 @@ static const struct thermal_cooling_device_ops mlxsw_cooling_ops = { > static int > mlxsw_thermal_module_tz_init(struct mlxsw_thermal_module *module_tz) > { > + struct thermal_zone_params tzp = { > + .initial_mode = THERMAL_DEVICE_ENABLED, > + }; > char tz_name[MLXSW_THERMAL_ZONE_MAX_NAME]; > int err; > > @@ -759,13 +723,12 @@ mlxsw_thermal_module_tz_init(struct mlxsw_thermal_module *module_tz) > MLXSW_THERMAL_TRIP_MASK, > module_tz, > &mlxsw_thermal_module_ops, > - NULL, 0, 0); > + &tzp, 0, 0); > if (IS_ERR(module_tz->tzdev)) { > err = PTR_ERR(module_tz->tzdev); > return err; > } > > - module_tz->mode = THERMAL_DEVICE_ENABLED; > return 0; > } > > @@ -868,6 +831,9 @@ mlxsw_thermal_modules_fini(struct mlxsw_thermal *thermal) > static int > mlxsw_thermal_gearbox_tz_init(struct mlxsw_thermal_module *gearbox_tz) > { > + struct thermal_zone_params tzp = { > + .initial_mode = THERMAL_DEVICE_ENABLED, > + }; > char tz_name[MLXSW_THERMAL_ZONE_MAX_NAME]; > > snprintf(tz_name, sizeof(tz_name), "mlxsw-gearbox%d", > @@ -877,11 +843,10 @@ mlxsw_thermal_gearbox_tz_init(struct mlxsw_thermal_module *gearbox_tz) > MLXSW_THERMAL_TRIP_MASK, > gearbox_tz, > &mlxsw_thermal_gearbox_ops, > - NULL, 0, 0); > + &tzp, 0, 0); > if (IS_ERR(gearbox_tz->tzdev)) > return PTR_ERR(gearbox_tz->tzdev); > > - gearbox_tz->mode = THERMAL_DEVICE_ENABLED; > return 0; > } > > @@ -960,6 +925,9 @@ int mlxsw_thermal_init(struct mlxsw_core *core, > const struct mlxsw_bus_info *bus_info, > struct mlxsw_thermal **p_thermal) > { > + struct thermal_zone_params tzp = { > + .initial_mode = THERMAL_DEVICE_ENABLED, > + }; > char mfcr_pl[MLXSW_REG_MFCR_LEN] = { 0 }; > enum mlxsw_reg_mfcr_pwm_frequency freq; > struct device *dev = bus_info->dev; > @@ -1034,7 +1002,7 @@ int mlxsw_thermal_init(struct mlxsw_core *core, > MLXSW_THERMAL_TRIP_MASK, > thermal, > &mlxsw_thermal_ops, > - NULL, 0, > + &tzp, 0, > thermal->polling_delay); > if (IS_ERR(thermal->tzdev)) { > err = PTR_ERR(thermal->tzdev); > @@ -1050,7 +1018,6 @@ int mlxsw_thermal_init(struct mlxsw_core *core, > if (err) > goto err_unreg_modules_tzdev; > > - thermal->mode = THERMAL_DEVICE_ENABLED; > *p_thermal = thermal; > return 0; > > diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c > index 8cc86f4e3ac1..aaf8b845be90 100644 > --- a/drivers/platform/x86/acerhdf.c > +++ b/drivers/platform/x86/acerhdf.c > @@ -406,22 +406,9 @@ static inline void acerhdf_enable_kernelmode(void) > kernelmode = 1; > > thz_dev->polling_delay = interval*1000; > - thermal_zone_device_update(thz_dev, THERMAL_EVENT_UNSPECIFIED); > pr_notice("kernel mode fan control ON\n"); > } > > -static int acerhdf_get_mode(struct thermal_zone_device *thermal, > - enum thermal_device_mode *mode) > -{ > - if (verbose) > - pr_notice("kernel mode fan control %d\n", kernelmode); > - > - *mode = (kernelmode) ? THERMAL_DEVICE_ENABLED > - : THERMAL_DEVICE_DISABLED; > - > - return 0; > -} > - > /* > * set operation mode; > * enabled: the thermal layer of the kernel takes care about > @@ -488,7 +475,6 @@ static struct thermal_zone_device_ops acerhdf_dev_ops = { > .bind = acerhdf_bind, > .unbind = acerhdf_unbind, > .get_temp = acerhdf_get_ec_temp, > - .get_mode = acerhdf_get_mode, > .set_mode = acerhdf_set_mode, > .get_trip_type = acerhdf_get_trip_type, > .get_trip_hyst = acerhdf_get_trip_hyst, > @@ -554,6 +540,7 @@ static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev, > > err_out: > acerhdf_revert_to_bios_mode(); > + thz_dev->mode = THERMAL_DEVICE_DISABLED; > return -EINVAL; > } > > @@ -739,6 +726,8 @@ static int __init acerhdf_register_thermal(void) > if (IS_ERR(cl_dev)) > return -EINVAL; > > + acerhdf_zone_params.initial_mode = > + kernelmode ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED; > thz_dev = thermal_zone_device_register("acerhdf", 2, 0, NULL, > &acerhdf_dev_ops, > &acerhdf_zone_params, 0, > diff --git a/drivers/thermal/da9062-thermal.c b/drivers/thermal/da9062-thermal.c > index c32709badeda..4bdb6f9621c1 100644 > --- a/drivers/thermal/da9062-thermal.c > +++ b/drivers/thermal/da9062-thermal.c > @@ -49,7 +49,6 @@ struct da9062_thermal { > struct da9062 *hw; > struct delayed_work work; > struct thermal_zone_device *zone; > - enum thermal_device_mode mode; > struct mutex lock; /* protection for da9062_thermal temperature */ > int temperature; > int irq; > @@ -121,14 +120,6 @@ static irqreturn_t da9062_thermal_irq_handler(int irq, void *data) > return IRQ_HANDLED; > } > > -static int da9062_thermal_get_mode(struct thermal_zone_device *z, > - enum thermal_device_mode *mode) > -{ > - struct da9062_thermal *thermal = z->devdata; > - *mode = thermal->mode; > - return 0; > -} > - > static int da9062_thermal_get_trip_type(struct thermal_zone_device *z, > int trip, > enum thermal_trip_type *type) > @@ -181,7 +172,6 @@ static int da9062_thermal_get_temp(struct thermal_zone_device *z, > > static struct thermal_zone_device_ops da9062_thermal_ops = { > .get_temp = da9062_thermal_get_temp, > - .get_mode = da9062_thermal_get_mode, > .get_trip_type = da9062_thermal_get_trip_type, > .get_trip_temp = da9062_thermal_get_trip_temp, > }; > @@ -199,6 +189,9 @@ MODULE_DEVICE_TABLE(of, da9062_compatible_reg_id_table); > > static int da9062_thermal_probe(struct platform_device *pdev) > { > + struct thermal_zone_params tzp = { > + .initial_mode = THERMAL_DEVICE_ENABLED, > + }; > struct da9062 *chip = dev_get_drvdata(pdev->dev.parent); > struct da9062_thermal *thermal; > unsigned int pp_tmp = DA9062_DEFAULT_POLLING_MS_PERIOD; > @@ -233,7 +226,6 @@ static int da9062_thermal_probe(struct platform_device *pdev) > > thermal->config = match->data; > thermal->hw = chip; > - thermal->mode = THERMAL_DEVICE_ENABLED; > thermal->dev = &pdev->dev; > > INIT_DELAYED_WORK(&thermal->work, da9062_thermal_poll_on); > @@ -241,7 +233,7 @@ static int da9062_thermal_probe(struct platform_device *pdev) > > thermal->zone = thermal_zone_device_register(thermal->config->name, > 1, 0, thermal, > - &da9062_thermal_ops, NULL, pp_tmp, > + &da9062_thermal_ops, &tzp, pp_tmp, > 0); > if (IS_ERR(thermal->zone)) { > dev_err(&pdev->dev, "Cannot register thermal zone device\n"); > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c > index e761c9b42217..3e02323c938b 100644 > --- a/drivers/thermal/imx_thermal.c > +++ b/drivers/thermal/imx_thermal.c > @@ -197,7 +197,6 @@ struct imx_thermal_data { > struct cpufreq_policy *policy; > struct thermal_zone_device *tz; > struct thermal_cooling_device *cdev; > - enum thermal_device_mode mode; > struct regmap *tempmon; > u32 c1, c2; /* See formula in imx_init_calib() */ > int temp_passive; > @@ -256,7 +255,7 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) > bool wait; > u32 val; > > - if (data->mode == THERMAL_DEVICE_ENABLED) { > + if (tz->mode == THERMAL_DEVICE_ENABLED) { > /* Check if a measurement is currently in progress */ > regmap_read(map, soc_data->temp_data, &val); > wait = !(val & soc_data->temp_valid_mask); > @@ -283,7 +282,7 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) > > regmap_read(map, soc_data->temp_data, &val); > > - if (data->mode != THERMAL_DEVICE_ENABLED) { > + if (tz->mode != THERMAL_DEVICE_ENABLED) { > regmap_write(map, soc_data->sensor_ctrl + REG_CLR, > soc_data->measure_temp_mask); > regmap_write(map, soc_data->sensor_ctrl + REG_SET, > @@ -331,16 +330,6 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) > return 0; > } > > -static int imx_get_mode(struct thermal_zone_device *tz, > - enum thermal_device_mode *mode) > -{ > - struct imx_thermal_data *data = tz->devdata; > - > - *mode = data->mode; > - > - return 0; > -} > - > static int imx_set_mode(struct thermal_zone_device *tz, > enum thermal_device_mode mode) > { > @@ -376,9 +365,6 @@ static int imx_set_mode(struct thermal_zone_device *tz, > } > } > > - data->mode = mode; > - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > - > return 0; > } > > @@ -467,7 +453,6 @@ static struct thermal_zone_device_ops imx_tz_ops = { > .bind = imx_bind, > .unbind = imx_unbind, > .get_temp = imx_get_temp, > - .get_mode = imx_get_mode, > .set_mode = imx_set_mode, > .get_trip_type = imx_get_trip_type, > .get_trip_temp = imx_get_trip_temp, > @@ -691,6 +676,9 @@ static inline void imx_thermal_unregister_legacy_cooling(struct imx_thermal_data > > static int imx_thermal_probe(struct platform_device *pdev) > { > + struct thermal_zone_params tzp = { > + .initial_mode = THERMAL_DEVICE_ENABLED, > + }; > struct imx_thermal_data *data; > struct regmap *map; > int measure_freq; > @@ -799,7 +787,7 @@ static int imx_thermal_probe(struct platform_device *pdev) > data->tz = thermal_zone_device_register("imx_thermal_zone", > IMX_TRIP_NUM, > BIT(IMX_TRIP_PASSIVE), data, > - &imx_tz_ops, NULL, > + &imx_tz_ops, &tzp, > IMX_PASSIVE_DELAY, > IMX_POLLING_DELAY); > if (IS_ERR(data->tz)) { > @@ -831,7 +819,6 @@ static int imx_thermal_probe(struct platform_device *pdev) > data->socdata->measure_temp_mask); > > data->irq_enabled = true; > - data->mode = THERMAL_DEVICE_ENABLED; > > ret = devm_request_threaded_irq(&pdev->dev, data->irq, > imx_thermal_alarm_irq, imx_thermal_alarm_irq_thread, > @@ -885,7 +872,7 @@ static int __maybe_unused imx_thermal_suspend(struct device *dev) > data->socdata->measure_temp_mask); > regmap_write(map, data->socdata->sensor_ctrl + REG_SET, > data->socdata->power_down_mask); > - data->mode = THERMAL_DEVICE_DISABLED; > + data->tz->mode = THERMAL_DEVICE_DISABLED; > clk_disable_unprepare(data->thermal_clk); > > return 0; > @@ -905,7 +892,7 @@ static int __maybe_unused imx_thermal_resume(struct device *dev) > data->socdata->power_down_mask); > regmap_write(map, data->socdata->sensor_ctrl + REG_SET, > data->socdata->measure_temp_mask); > - data->mode = THERMAL_DEVICE_ENABLED; > + data->tz->mode = THERMAL_DEVICE_ENABLED; > > return 0; > } > diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > index e802922a13cf..86a00598ed09 100644 > --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > @@ -44,7 +44,6 @@ static char *int3400_thermal_uuids[INT3400_THERMAL_MAXIMUM_UUID] = { > struct int3400_thermal_priv { > struct acpi_device *adev; > struct thermal_zone_device *thermal; > - int mode; > int art_count; > struct art *arts; > int trt_count; > @@ -230,48 +229,29 @@ static int int3400_thermal_get_temp(struct thermal_zone_device *thermal, > return 0; > } > > -static int int3400_thermal_get_mode(struct thermal_zone_device *thermal, > - enum thermal_device_mode *mode) > -{ > - struct int3400_thermal_priv *priv = thermal->devdata; > - > - if (!priv) > - return -EINVAL; > - > - *mode = priv->mode; > - > - return 0; > -} > - > static int int3400_thermal_set_mode(struct thermal_zone_device *thermal, > enum thermal_device_mode mode) > { > struct int3400_thermal_priv *priv = thermal->devdata; > - bool enable; > int result = 0; > > if (!priv) > return -EINVAL; > > - if (mode == THERMAL_DEVICE_ENABLED) > - enable = true; > - else if (mode == THERMAL_DEVICE_DISABLED) > - enable = false; > - else > + if (mode != THERMAL_DEVICE_ENABLED && > + mode != THERMAL_DEVICE_DISABLED) > return -EINVAL; > > - if (enable != priv->mode) { > - priv->mode = enable; > + if (mode != thermal->mode) { > result = int3400_thermal_run_osc(priv->adev->handle, > - priv->current_uuid_index, > - enable); > + priv->current_uuid_index, > + mode == THERMAL_DEVICE_ENABLED); > } > return result; > } > > static struct thermal_zone_device_ops int3400_thermal_ops = { > .get_temp = int3400_thermal_get_temp, > - .get_mode = int3400_thermal_get_mode, > .set_mode = int3400_thermal_set_mode, > }; > > diff --git a/drivers/thermal/intel/intel_quark_dts_thermal.c b/drivers/thermal/intel/intel_quark_dts_thermal.c > index d704fc104cfd..c4879b4bfbf1 100644 > --- a/drivers/thermal/intel/intel_quark_dts_thermal.c > +++ b/drivers/thermal/intel/intel_quark_dts_thermal.c > @@ -103,7 +103,6 @@ struct soc_sensor_entry { > bool locked; > u32 store_ptps; > u32 store_dts_enable; > - enum thermal_device_mode mode; > struct thermal_zone_device *tzone; > }; > > @@ -128,7 +127,7 @@ static int soc_dts_enable(struct thermal_zone_device *tzd) > return ret; > > if (out & QRK_DTS_ENABLE_BIT) { > - aux_entry->mode = THERMAL_DEVICE_ENABLED; > + tzd->mode = THERMAL_DEVICE_ENABLED; > return 0; > } > > @@ -139,9 +138,9 @@ static int soc_dts_enable(struct thermal_zone_device *tzd) > if (ret) > return ret; > > - aux_entry->mode = THERMAL_DEVICE_ENABLED; > + tzd->mode = THERMAL_DEVICE_ENABLED; > } else { > - aux_entry->mode = THERMAL_DEVICE_DISABLED; > + tzd->mode = THERMAL_DEVICE_DISABLED; > pr_info("DTS is locked. Cannot enable DTS\n"); > ret = -EPERM; > } > @@ -161,7 +160,7 @@ static int soc_dts_disable(struct thermal_zone_device *tzd) > return ret; > > if (!(out & QRK_DTS_ENABLE_BIT)) { > - aux_entry->mode = THERMAL_DEVICE_DISABLED; > + tzd->mode = THERMAL_DEVICE_DISABLED; > return 0; > } > > @@ -173,9 +172,9 @@ static int soc_dts_disable(struct thermal_zone_device *tzd) > if (ret) > return ret; > > - aux_entry->mode = THERMAL_DEVICE_DISABLED; > + tzd->mode = THERMAL_DEVICE_DISABLED; > } else { > - aux_entry->mode = THERMAL_DEVICE_ENABLED; > + tzd->mode = THERMAL_DEVICE_ENABLED; > pr_info("DTS is locked. Cannot disable DTS\n"); > ret = -EPERM; > } > @@ -309,14 +308,6 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd, > return 0; > } > > -static int sys_get_mode(struct thermal_zone_device *tzd, > - enum thermal_device_mode *mode) > -{ > - struct soc_sensor_entry *aux_entry = tzd->devdata; > - *mode = aux_entry->mode; > - return 0; > -} > - > static int sys_set_mode(struct thermal_zone_device *tzd, > enum thermal_device_mode mode) > { > @@ -338,7 +329,6 @@ static struct thermal_zone_device_ops tzone_ops = { > .get_trip_type = sys_get_trip_type, > .set_trip_temp = sys_set_trip_temp, > .get_crit_temp = sys_get_crit_temp, > - .get_mode = sys_get_mode, > .set_mode = sys_set_mode, > }; > > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > index 874a47d6923f..863b89546f81 100644 > --- a/drivers/thermal/of-thermal.c > +++ b/drivers/thermal/of-thermal.c > @@ -51,7 +51,6 @@ struct __thermal_bind_params { > > /** > * struct __thermal_zone - internal representation of a thermal zone > - * @mode: current thermal zone device mode (enabled/disabled) > * @passive_delay: polling interval while passive cooling is activated > * @polling_delay: zone polling interval > * @slope: slope of the temperature adjustment curve > @@ -65,7 +64,6 @@ struct __thermal_bind_params { > */ > > struct __thermal_zone { > - enum thermal_device_mode mode; > int passive_delay; > int polling_delay; > int slope; > @@ -269,23 +267,11 @@ static int of_thermal_unbind(struct thermal_zone_device *thermal, > return 0; > } > > -static int of_thermal_get_mode(struct thermal_zone_device *tz, > - enum thermal_device_mode *mode) > -{ > - struct __thermal_zone *data = tz->devdata; > - > - *mode = data->mode; > - > - return 0; > -} > - > static int of_thermal_set_mode(struct thermal_zone_device *tz, > enum thermal_device_mode mode) > { > struct __thermal_zone *data = tz->devdata; > > - mutex_lock(&tz->lock); > - > if (mode == THERMAL_DEVICE_ENABLED) { > tz->polling_delay = data->polling_delay; > tz->passive_delay = data->passive_delay; > @@ -294,11 +280,6 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz, > tz->passive_delay = 0; > } > > - mutex_unlock(&tz->lock); > - > - data->mode = mode; > - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > - > return 0; > } > > @@ -393,7 +374,6 @@ static int of_thermal_get_crit_temp(struct thermal_zone_device *tz, > } > > static struct thermal_zone_device_ops of_thermal_ops = { > - .get_mode = of_thermal_get_mode, > .set_mode = of_thermal_set_mode, > > .get_trip_type = of_thermal_get_trip_type, > @@ -553,8 +533,14 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, > if (id == sensor_id) { > tzd = thermal_zone_of_add_sensor(child, sensor_np, > data, ops); > - if (!IS_ERR(tzd)) > + if (!IS_ERR(tzd)) { > + mutex_lock(&tzd->lock); > tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED); > + tzd->mode = THERMAL_DEVICE_ENABLED; > + mutex_unlock(&tzd->lock); > + thermal_zone_device_update(tzd, > + THERMAL_EVENT_UNSPECIFIED); > + } > > of_node_put(child); > goto exit; > @@ -979,7 +965,6 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) > > finish: > of_node_put(child); > - tz->mode = THERMAL_DEVICE_DISABLED; > > return tz; > > @@ -1120,6 +1105,7 @@ int __init of_parse_thermal_zones(void) > /* these two are left for temperature drivers to use */ > tzp->slope = tz->slope; > tzp->offset = tz->offset; > + tzp->initial_mode = THERMAL_DEVICE_DISABLED; > > zone = thermal_zone_device_register(child->name, tz->ntrips, > mask, tz, > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index c06550930979..5ff98fcc0f6a 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -463,6 +463,43 @@ static void thermal_zone_device_reset(struct thermal_zone_device *tz) > thermal_zone_device_init(tz); > } > > +enum thermal_device_mode > +thermal_zone_device_get_mode(struct thermal_zone_device *tz) > +{ > + enum thermal_device_mode mode; > + > + mutex_lock(&tz->lock); > + > + mode = tz->mode; > + > + mutex_unlock(&tz->lock); > + > + return mode; > +} > + > +int thermal_zone_device_set_mode(struct thermal_zone_device *tz, > + enum thermal_device_mode mode) > +{ > + int ret = 0; > + > + if (mode != THERMAL_DEVICE_DISABLED && > + mode != THERMAL_DEVICE_ENABLED) > + return -EINVAL; > + > + mutex_lock(&tz->lock); > + > + if (tz->ops->set_mode) > + ret = tz->ops->set_mode(tz, mode); > + > + tz->mode = mode; > + > + mutex_unlock(&tz->lock); > + > + thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > + > + return ret; > +} > + > void thermal_zone_device_update(struct thermal_zone_device *tz, > enum thermal_notify_event event) > { > @@ -1344,6 +1381,9 @@ thermal_zone_device_register(const char *type, int trips, int mask, > if (atomic_cmpxchg(&tz->need_update, 1, 0)) > thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > > + if (tzp) > + thermal_zone_device_set_mode(tz, tzp->initial_mode); > + > return tz; > > unregister: > @@ -1473,9 +1513,7 @@ static int thermal_pm_notify(struct notifier_block *nb, > case PM_POST_SUSPEND: > atomic_set(&in_suspend, 0); > list_for_each_entry(tz, &thermal_tz_list, node) { > - tz_mode = THERMAL_DEVICE_ENABLED; > - if (tz->ops->get_mode) > - tz->ops->get_mode(tz, &tz_mode); > + tz_mode = thermal_zone_device_get_mode(tz); > > if (tz_mode == THERMAL_DEVICE_DISABLED) > continue; > diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h > index c95689586e19..8e561bac3133 100644 > --- a/drivers/thermal/thermal_core.h > +++ b/drivers/thermal/thermal_core.h > @@ -141,6 +141,22 @@ thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev, > unsigned long new_state) {} > #endif /* CONFIG_THERMAL_STATISTICS */ > > +enum thermal_device_mode > +thermal_zone_device_get_mode(struct thermal_zone_device *tz); > + > +int thermal_zone_device_set_mode(struct thermal_zone_device *tz, > + enum thermal_device_mode mode); > + > +static inline int thermal_zone_device_enable(struct thermal_zone_device *tz) > +{ > + return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_ENABLED); > +} > + > +static inline int thermal_zone_device_disable(struct thermal_zone_device *tz) > +{ > + return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_DISABLED); > +} > + > /* device tree support */ > #ifdef CONFIG_THERMAL_OF > int of_parse_thermal_zones(void); > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c > index aa99edb4dff7..cbb27b3c96d2 100644 > --- a/drivers/thermal/thermal_sysfs.c > +++ b/drivers/thermal/thermal_sysfs.c > @@ -50,14 +50,8 @@ mode_show(struct device *dev, struct device_attribute *attr, char *buf) > { > struct thermal_zone_device *tz = to_thermal_zone(dev); > enum thermal_device_mode mode; > - int result; > - > - if (!tz->ops->get_mode) > - return -EPERM; > > - result = tz->ops->get_mode(tz, &mode); > - if (result) > - return result; > + mode = thermal_zone_device_get_mode(tz); > > return sprintf(buf, "%s\n", mode == THERMAL_DEVICE_ENABLED ? "enabled" > : "disabled"); > @@ -74,9 +68,9 @@ mode_store(struct device *dev, struct device_attribute *attr, > return -EPERM; > > if (!strncmp(buf, "enabled", sizeof("enabled") - 1)) > - result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED); > + result = thermal_zone_device_enable(tz); > else if (!strncmp(buf, "disabled", sizeof("disabled") - 1)) > - result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED); > + result = thermal_zone_device_disable(tz); > else > result = -EINVAL; > > @@ -428,30 +422,13 @@ static struct attribute_group thermal_zone_attribute_group = { > .attrs = thermal_zone_dev_attrs, > }; > > -/* We expose mode only if .get_mode is present */ > static struct attribute *thermal_zone_mode_attrs[] = { > &dev_attr_mode.attr, > NULL, > }; > > -static umode_t thermal_zone_mode_is_visible(struct kobject *kobj, > - struct attribute *attr, > - int attrno) > -{ > - struct device *dev = container_of(kobj, struct device, kobj); > - struct thermal_zone_device *tz; > - > - tz = container_of(dev, struct thermal_zone_device, device); > - > - if (tz->ops->get_mode) > - return attr->mode; > - > - return 0; > -} > - > static struct attribute_group thermal_zone_mode_attribute_group = { > .attrs = thermal_zone_mode_attrs, > - .is_visible = thermal_zone_mode_is_visible, > }; > > /* We expose passive only if passive trips are present */ > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > index 216185bb3014..da4141697e2e 100644 > --- a/include/linux/thermal.h > +++ b/include/linux/thermal.h > @@ -76,8 +76,6 @@ struct thermal_zone_device_ops { > struct thermal_cooling_device *); > int (*get_temp) (struct thermal_zone_device *, int *); > int (*set_trips) (struct thermal_zone_device *, int, int); > - int (*get_mode) (struct thermal_zone_device *, > - enum thermal_device_mode *); > int (*set_mode) (struct thermal_zone_device *, > enum thermal_device_mode); > int (*get_trip_type) (struct thermal_zone_device *, int, > @@ -128,6 +126,7 @@ struct thermal_cooling_device { > * @trip_temp_attrs: attributes for trip points for sysfs: trip temperature > * @trip_type_attrs: attributes for trip points for sysfs: trip type > * @trip_hyst_attrs: attributes for trip points for sysfs: trip hysteresis > + * @mode: current mode of this thermal zone > * @devdata: private pointer for device private data > * @trips: number of trip points the thermal zone supports > * @trips_disabled; bitmap for disabled trips > @@ -170,6 +169,7 @@ struct thermal_zone_device { > struct thermal_attr *trip_temp_attrs; > struct thermal_attr *trip_type_attrs; > struct thermal_attr *trip_hyst_attrs; > + enum thermal_device_mode mode; > void *devdata; > int trips; > unsigned long trips_disabled; /* bitmap for disabled trips */ > @@ -264,6 +264,9 @@ struct thermal_zone_params { > int num_tbps; /* Number of tbp entries */ > struct thermal_bind_params *tbp; > > + /* Initial mode of this thermal zone device */ > + enum thermal_device_mode initial_mode; > + > /* > * Sustainable power (heat) that this thermal zone can dissipate in > * mW > -- > 2.17.1 >
Hi Andrzej, On 4/17/20 6:20 PM, Andrzej Pietrasiewicz wrote: > Thermal zone devices' mode is stored in individual drivers. This patch > changes it so that mode is stored in struct thermal_zone_device instead. > > As a result all driver-specific variables storing the mode are not needed > and are removed. Consequently, the get_mode() implementations have nothing > to operate on and need to be removed, too. > > Some thermal framework specific functions are introduced: > > thermal_zone_device_get_mode() > thermal_zone_device_set_mode() > thermal_zone_device_enable() > thermal_zone_device_disable() > > thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock > and the "set" calls driver's set_mode() if provided, so the latter must > not take this lock again. At the end of the "set" > thermal_zone_device_update() is called so drivers don't need to repeat this > invocation in their specific set_mode() implementations. > > The scope of the above 4 functions is purposedly limited to the thermal > framework and drivers are not supposed to call them. This encapsulation This should be true only for thermal_zone_device_{get,set}_mode(). thermal_zone_device_{en,dis}able() should be available for device drivers: * of/thermal device drivers need to enable thermal device itself (please refer to my patchset for details) * device drivers need to call them on ->suspend and ->resume operations > does not fully work at the moment for some drivers, though: > > - platform/x86/acerhdf.c > - drivers/thermal/imx_thermal.c > - drivers/thermal/intel/intel_quark_dts_thermal.c > - drivers/thermal/of-thermal.c > > and they manipulate struct thermal_zone_device's members directly. > > struct thermal_zone_params gains a new member called initial_mode, which > is used to set tzd's mode at registration time. > > The sysfs "mode" attribute is always exposed from now on, because all > thermal zone devices now have their get_mode() implemented at the generic > level and it is always available. Exposing "mode" doesn't hurt the drivers > which don't provide their own set_mode(), because writing to "mode" will > result in -EPERM, as expected. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/acpi/thermal.c | 46 +++++---------- > .../ethernet/mellanox/mlxsw/core_thermal.c | 57 ++++--------------- > drivers/platform/x86/acerhdf.c | 17 +----- > drivers/thermal/da9062-thermal.c | 16 ++---- > drivers/thermal/imx_thermal.c | 29 +++------- > .../intel/int340x_thermal/int3400_thermal.c | 30 ++-------- > .../thermal/intel/intel_quark_dts_thermal.c | 22 ++----- > drivers/thermal/of-thermal.c | 30 +++------- > drivers/thermal/thermal_core.c | 44 +++++++++++++- > drivers/thermal/thermal_core.h | 16 ++++++ > drivers/thermal/thermal_sysfs.c | 29 +--------- > include/linux/thermal.h | 7 ++- > 12 files changed, 125 insertions(+), 218 deletions(-) > > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c > index 19067a5e5293..67bc263332a5 100644 > --- a/drivers/acpi/thermal.c > +++ b/drivers/acpi/thermal.c > @@ -172,7 +172,6 @@ struct acpi_thermal { > struct acpi_thermal_trips trips; > struct acpi_handle_list devices; > struct thermal_zone_device *thermal_zone; > - int tz_enabled; > int kelvin_offset; /* in millidegrees */ > struct work_struct thermal_check_work; > }; > @@ -500,7 +499,7 @@ static void acpi_thermal_check(void *data) > { > struct acpi_thermal *tz = data; > > - if (!tz->tz_enabled) > + if (tz->thermal_zone->mode != THERMAL_DEVICE_ENABLED) > return; > > thermal_zone_device_update(tz->thermal_zone, > @@ -526,46 +525,29 @@ static int thermal_get_temp(struct thermal_zone_device *thermal, int *temp) > return 0; > } > > -static int thermal_get_mode(struct thermal_zone_device *thermal, > - enum thermal_device_mode *mode) > -{ > - struct acpi_thermal *tz = thermal->devdata; > - > - if (!tz) > - return -EINVAL; > - > - *mode = tz->tz_enabled ? THERMAL_DEVICE_ENABLED : > - THERMAL_DEVICE_DISABLED; > - > - return 0; > -} > - > static int thermal_set_mode(struct thermal_zone_device *thermal, > enum thermal_device_mode mode) > { > struct acpi_thermal *tz = thermal->devdata; > - int enable; > > if (!tz) > return -EINVAL; > > + if (mode != THERMAL_DEVICE_DISABLED && > + mode != THERMAL_DEVICE_ENABLED) > + return -EINVAL; These checks should be removed as they are already done in the caller of ->set_mode method (thermal_zone_device_set_mode()). > + > /* > * enable/disable thermal management from ACPI thermal driver > */ > - if (mode == THERMAL_DEVICE_ENABLED) > - enable = 1; > - else if (mode == THERMAL_DEVICE_DISABLED) { > - enable = 0; > + if (mode == THERMAL_DEVICE_DISABLED) > pr_warn("thermal zone will be disabled\n"); > - } else > - return -EINVAL; > > - if (enable != tz->tz_enabled) { > - tz->tz_enabled = enable; > + if (mode != thermal->mode) { > ACPI_DEBUG_PRINT((ACPI_DB_INFO, > "%s kernel ACPI thermal control\n", > - tz->tz_enabled ? "Enable" : "Disable")); > - acpi_thermal_check(tz); > + mode == THERMAL_DEVICE_ENABLED ? > + "Enable" : "Disable")); > } > return 0; > } > @@ -856,7 +838,6 @@ static struct thermal_zone_device_ops acpi_thermal_zone_ops = { > .bind = acpi_thermal_bind_cooling_device, > .unbind = acpi_thermal_unbind_cooling_device, > .get_temp = thermal_get_temp, > - .get_mode = thermal_get_mode, > .set_mode = thermal_set_mode, > .get_trip_type = thermal_get_trip_type, > .get_trip_temp = thermal_get_trip_temp, > @@ -870,6 +851,9 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) > int trips = 0; > int result; > acpi_status status; > + struct thermal_zone_params prms = { > + .initial_mode = THERMAL_DEVICE_ENABLED, > + }; > int i; > > if (tz->trips.critical.flags.valid) > @@ -887,13 +871,13 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) > if (tz->trips.passive.flags.valid) > tz->thermal_zone = > thermal_zone_device_register("acpitz", trips, 0, tz, > - &acpi_thermal_zone_ops, NULL, > + &acpi_thermal_zone_ops, &prms, > tz->trips.passive.tsp*100, > tz->polling_frequency*100); > else > tz->thermal_zone = > thermal_zone_device_register("acpitz", trips, 0, tz, > - &acpi_thermal_zone_ops, NULL, > + &acpi_thermal_zone_ops, &prms, > 0, tz->polling_frequency*100); > if (IS_ERR(tz->thermal_zone)) > return -ENODEV; > @@ -913,8 +897,6 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) > if (ACPI_FAILURE(status)) > return -ENODEV; > > - tz->tz_enabled = 1; > - > dev_info(&tz->device->dev, "registered as thermal_zone%d\n", > tz->thermal_zone->id); > return 0; > diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c > index ce0a6837daa3..50518048b86d 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c > @@ -98,7 +98,6 @@ struct mlxsw_thermal_module { > struct mlxsw_thermal *parent; > struct thermal_zone_device *tzdev; > struct mlxsw_thermal_trip trips[MLXSW_THERMAL_NUM_TRIPS]; > - enum thermal_device_mode mode; > int module; /* Module or gearbox number */ > }; > > @@ -110,7 +109,6 @@ struct mlxsw_thermal { > struct thermal_cooling_device *cdevs[MLXSW_MFCR_PWMS_MAX]; > u8 cooling_levels[MLXSW_THERMAL_MAX_STATE + 1]; > struct mlxsw_thermal_trip trips[MLXSW_THERMAL_NUM_TRIPS]; > - enum thermal_device_mode mode; > struct mlxsw_thermal_module *tz_module_arr; > u8 tz_module_num; > struct mlxsw_thermal_module *tz_gearbox_arr; > @@ -277,33 +275,16 @@ static int mlxsw_thermal_unbind(struct thermal_zone_device *tzdev, > return 0; > } > > -static int mlxsw_thermal_get_mode(struct thermal_zone_device *tzdev, > - enum thermal_device_mode *mode) > -{ > - struct mlxsw_thermal *thermal = tzdev->devdata; > - > - *mode = thermal->mode; > - > - return 0; > -} > - > static int mlxsw_thermal_set_mode(struct thermal_zone_device *tzdev, > enum thermal_device_mode mode) > { > struct mlxsw_thermal *thermal = tzdev->devdata; > > - mutex_lock(&tzdev->lock); > - > if (mode == THERMAL_DEVICE_ENABLED) > tzdev->polling_delay = thermal->polling_delay; > else > tzdev->polling_delay = 0; > > - mutex_unlock(&tzdev->lock); > - > - thermal->mode = mode; > - thermal_zone_device_update(tzdev, THERMAL_EVENT_UNSPECIFIED); > - > return 0; > } > > @@ -407,7 +388,6 @@ static int mlxsw_thermal_trend_get(struct thermal_zone_device *tzdev, > static struct thermal_zone_device_ops mlxsw_thermal_ops = { > .bind = mlxsw_thermal_bind, > .unbind = mlxsw_thermal_unbind, > - .get_mode = mlxsw_thermal_get_mode, > .set_mode = mlxsw_thermal_set_mode, > .get_temp = mlxsw_thermal_get_temp, > .get_trip_type = mlxsw_thermal_get_trip_type, > @@ -466,34 +446,17 @@ static int mlxsw_thermal_module_unbind(struct thermal_zone_device *tzdev, > return err; > } > > -static int mlxsw_thermal_module_mode_get(struct thermal_zone_device *tzdev, > - enum thermal_device_mode *mode) > -{ > - struct mlxsw_thermal_module *tz = tzdev->devdata; > - > - *mode = tz->mode; > - > - return 0; > -} > - > static int mlxsw_thermal_module_mode_set(struct thermal_zone_device *tzdev, > enum thermal_device_mode mode) > { > struct mlxsw_thermal_module *tz = tzdev->devdata; > struct mlxsw_thermal *thermal = tz->parent; > > - mutex_lock(&tzdev->lock); > - > if (mode == THERMAL_DEVICE_ENABLED) > tzdev->polling_delay = thermal->polling_delay; > else > tzdev->polling_delay = 0; > > - mutex_unlock(&tzdev->lock); > - > - tz->mode = mode; > - thermal_zone_device_update(tzdev, THERMAL_EVENT_UNSPECIFIED); > - > return 0; > } > > @@ -596,7 +559,6 @@ mlxsw_thermal_module_trip_hyst_set(struct thermal_zone_device *tzdev, int trip, > static struct thermal_zone_device_ops mlxsw_thermal_module_ops = { > .bind = mlxsw_thermal_module_bind, > .unbind = mlxsw_thermal_module_unbind, > - .get_mode = mlxsw_thermal_module_mode_get, > .set_mode = mlxsw_thermal_module_mode_set, > .get_temp = mlxsw_thermal_module_temp_get, > .get_trip_type = mlxsw_thermal_module_trip_type_get, > @@ -635,7 +597,6 @@ static int mlxsw_thermal_gearbox_temp_get(struct thermal_zone_device *tzdev, > static struct thermal_zone_device_ops mlxsw_thermal_gearbox_ops = { > .bind = mlxsw_thermal_module_bind, > .unbind = mlxsw_thermal_module_unbind, > - .get_mode = mlxsw_thermal_module_mode_get, > .set_mode = mlxsw_thermal_module_mode_set, > .get_temp = mlxsw_thermal_gearbox_temp_get, > .get_trip_type = mlxsw_thermal_module_trip_type_get, > @@ -749,6 +710,9 @@ static const struct thermal_cooling_device_ops mlxsw_cooling_ops = { > static int > mlxsw_thermal_module_tz_init(struct mlxsw_thermal_module *module_tz) > { > + struct thermal_zone_params tzp = { > + .initial_mode = THERMAL_DEVICE_ENABLED, > + }; > char tz_name[MLXSW_THERMAL_ZONE_MAX_NAME]; > int err; > > @@ -759,13 +723,12 @@ mlxsw_thermal_module_tz_init(struct mlxsw_thermal_module *module_tz) > MLXSW_THERMAL_TRIP_MASK, > module_tz, > &mlxsw_thermal_module_ops, > - NULL, 0, 0); > + &tzp, 0, 0); > if (IS_ERR(module_tz->tzdev)) { > err = PTR_ERR(module_tz->tzdev); > return err; > } > > - module_tz->mode = THERMAL_DEVICE_ENABLED; > return 0; > } > > @@ -868,6 +831,9 @@ mlxsw_thermal_modules_fini(struct mlxsw_thermal *thermal) > static int > mlxsw_thermal_gearbox_tz_init(struct mlxsw_thermal_module *gearbox_tz) > { > + struct thermal_zone_params tzp = { > + .initial_mode = THERMAL_DEVICE_ENABLED, > + }; > char tz_name[MLXSW_THERMAL_ZONE_MAX_NAME]; > > snprintf(tz_name, sizeof(tz_name), "mlxsw-gearbox%d", > @@ -877,11 +843,10 @@ mlxsw_thermal_gearbox_tz_init(struct mlxsw_thermal_module *gearbox_tz) > MLXSW_THERMAL_TRIP_MASK, > gearbox_tz, > &mlxsw_thermal_gearbox_ops, > - NULL, 0, 0); > + &tzp, 0, 0); > if (IS_ERR(gearbox_tz->tzdev)) > return PTR_ERR(gearbox_tz->tzdev); > > - gearbox_tz->mode = THERMAL_DEVICE_ENABLED; > return 0; > } > > @@ -960,6 +925,9 @@ int mlxsw_thermal_init(struct mlxsw_core *core, > const struct mlxsw_bus_info *bus_info, > struct mlxsw_thermal **p_thermal) > { > + struct thermal_zone_params tzp = { > + .initial_mode = THERMAL_DEVICE_ENABLED, > + }; > char mfcr_pl[MLXSW_REG_MFCR_LEN] = { 0 }; > enum mlxsw_reg_mfcr_pwm_frequency freq; > struct device *dev = bus_info->dev; > @@ -1034,7 +1002,7 @@ int mlxsw_thermal_init(struct mlxsw_core *core, > MLXSW_THERMAL_TRIP_MASK, > thermal, > &mlxsw_thermal_ops, > - NULL, 0, > + &tzp, 0, > thermal->polling_delay); > if (IS_ERR(thermal->tzdev)) { > err = PTR_ERR(thermal->tzdev); > @@ -1050,7 +1018,6 @@ int mlxsw_thermal_init(struct mlxsw_core *core, > if (err) > goto err_unreg_modules_tzdev; > > - thermal->mode = THERMAL_DEVICE_ENABLED; > *p_thermal = thermal; > return 0; > > diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c > index 8cc86f4e3ac1..aaf8b845be90 100644 > --- a/drivers/platform/x86/acerhdf.c > +++ b/drivers/platform/x86/acerhdf.c > @@ -406,22 +406,9 @@ static inline void acerhdf_enable_kernelmode(void) > kernelmode = 1; > > thz_dev->polling_delay = interval*1000; > - thermal_zone_device_update(thz_dev, THERMAL_EVENT_UNSPECIFIED); > pr_notice("kernel mode fan control ON\n"); > } > > -static int acerhdf_get_mode(struct thermal_zone_device *thermal, > - enum thermal_device_mode *mode) > -{ > - if (verbose) > - pr_notice("kernel mode fan control %d\n", kernelmode); > - > - *mode = (kernelmode) ? THERMAL_DEVICE_ENABLED > - : THERMAL_DEVICE_DISABLED; > - > - return 0; > -} > - > /* > * set operation mode; > * enabled: the thermal layer of the kernel takes care about > @@ -488,7 +475,6 @@ static struct thermal_zone_device_ops acerhdf_dev_ops = { > .bind = acerhdf_bind, > .unbind = acerhdf_unbind, > .get_temp = acerhdf_get_ec_temp, > - .get_mode = acerhdf_get_mode, > .set_mode = acerhdf_set_mode, > .get_trip_type = acerhdf_get_trip_type, > .get_trip_hyst = acerhdf_get_trip_hyst, > @@ -554,6 +540,7 @@ static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev, > > err_out: > acerhdf_revert_to_bios_mode(); > + thz_dev->mode = THERMAL_DEVICE_DISABLED; > return -EINVAL; > } > > @@ -739,6 +726,8 @@ static int __init acerhdf_register_thermal(void) > if (IS_ERR(cl_dev)) > return -EINVAL; > > + acerhdf_zone_params.initial_mode = > + kernelmode ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED; > thz_dev = thermal_zone_device_register("acerhdf", 2, 0, NULL, > &acerhdf_dev_ops, > &acerhdf_zone_params, 0, > diff --git a/drivers/thermal/da9062-thermal.c b/drivers/thermal/da9062-thermal.c > index c32709badeda..4bdb6f9621c1 100644 > --- a/drivers/thermal/da9062-thermal.c > +++ b/drivers/thermal/da9062-thermal.c > @@ -49,7 +49,6 @@ struct da9062_thermal { > struct da9062 *hw; > struct delayed_work work; > struct thermal_zone_device *zone; > - enum thermal_device_mode mode; > struct mutex lock; /* protection for da9062_thermal temperature */ > int temperature; > int irq; > @@ -121,14 +120,6 @@ static irqreturn_t da9062_thermal_irq_handler(int irq, void *data) > return IRQ_HANDLED; > } > > -static int da9062_thermal_get_mode(struct thermal_zone_device *z, > - enum thermal_device_mode *mode) > -{ > - struct da9062_thermal *thermal = z->devdata; > - *mode = thermal->mode; > - return 0; > -} > - > static int da9062_thermal_get_trip_type(struct thermal_zone_device *z, > int trip, > enum thermal_trip_type *type) > @@ -181,7 +172,6 @@ static int da9062_thermal_get_temp(struct thermal_zone_device *z, > > static struct thermal_zone_device_ops da9062_thermal_ops = { > .get_temp = da9062_thermal_get_temp, > - .get_mode = da9062_thermal_get_mode, > .get_trip_type = da9062_thermal_get_trip_type, > .get_trip_temp = da9062_thermal_get_trip_temp, > }; > @@ -199,6 +189,9 @@ MODULE_DEVICE_TABLE(of, da9062_compatible_reg_id_table); > > static int da9062_thermal_probe(struct platform_device *pdev) > { > + struct thermal_zone_params tzp = { > + .initial_mode = THERMAL_DEVICE_ENABLED, > + }; > struct da9062 *chip = dev_get_drvdata(pdev->dev.parent); > struct da9062_thermal *thermal; > unsigned int pp_tmp = DA9062_DEFAULT_POLLING_MS_PERIOD; > @@ -233,7 +226,6 @@ static int da9062_thermal_probe(struct platform_device *pdev) > > thermal->config = match->data; > thermal->hw = chip; > - thermal->mode = THERMAL_DEVICE_ENABLED; > thermal->dev = &pdev->dev; > > INIT_DELAYED_WORK(&thermal->work, da9062_thermal_poll_on); > @@ -241,7 +233,7 @@ static int da9062_thermal_probe(struct platform_device *pdev) > > thermal->zone = thermal_zone_device_register(thermal->config->name, > 1, 0, thermal, > - &da9062_thermal_ops, NULL, pp_tmp, > + &da9062_thermal_ops, &tzp, pp_tmp, > 0); > if (IS_ERR(thermal->zone)) { > dev_err(&pdev->dev, "Cannot register thermal zone device\n"); > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c > index e761c9b42217..3e02323c938b 100644 > --- a/drivers/thermal/imx_thermal.c > +++ b/drivers/thermal/imx_thermal.c > @@ -197,7 +197,6 @@ struct imx_thermal_data { > struct cpufreq_policy *policy; > struct thermal_zone_device *tz; > struct thermal_cooling_device *cdev; > - enum thermal_device_mode mode; > struct regmap *tempmon; > u32 c1, c2; /* See formula in imx_init_calib() */ > int temp_passive; > @@ -256,7 +255,7 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) > bool wait; > u32 val; > > - if (data->mode == THERMAL_DEVICE_ENABLED) { > + if (tz->mode == THERMAL_DEVICE_ENABLED) { > /* Check if a measurement is currently in progress */ > regmap_read(map, soc_data->temp_data, &val); > wait = !(val & soc_data->temp_valid_mask); > @@ -283,7 +282,7 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) > > regmap_read(map, soc_data->temp_data, &val); > > - if (data->mode != THERMAL_DEVICE_ENABLED) { > + if (tz->mode != THERMAL_DEVICE_ENABLED) { > regmap_write(map, soc_data->sensor_ctrl + REG_CLR, > soc_data->measure_temp_mask); > regmap_write(map, soc_data->sensor_ctrl + REG_SET, > @@ -331,16 +330,6 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) > return 0; > } > > -static int imx_get_mode(struct thermal_zone_device *tz, > - enum thermal_device_mode *mode) > -{ > - struct imx_thermal_data *data = tz->devdata; > - > - *mode = data->mode; > - > - return 0; > -} > - > static int imx_set_mode(struct thermal_zone_device *tz, > enum thermal_device_mode mode) > { > @@ -376,9 +365,6 @@ static int imx_set_mode(struct thermal_zone_device *tz, > } > } > > - data->mode = mode; > - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > - > return 0; > } > > @@ -467,7 +453,6 @@ static struct thermal_zone_device_ops imx_tz_ops = { > .bind = imx_bind, > .unbind = imx_unbind, > .get_temp = imx_get_temp, > - .get_mode = imx_get_mode, > .set_mode = imx_set_mode, > .get_trip_type = imx_get_trip_type, > .get_trip_temp = imx_get_trip_temp, > @@ -691,6 +676,9 @@ static inline void imx_thermal_unregister_legacy_cooling(struct imx_thermal_data > > static int imx_thermal_probe(struct platform_device *pdev) > { > + struct thermal_zone_params tzp = { > + .initial_mode = THERMAL_DEVICE_ENABLED, > + }; > struct imx_thermal_data *data; > struct regmap *map; > int measure_freq; > @@ -799,7 +787,7 @@ static int imx_thermal_probe(struct platform_device *pdev) > data->tz = thermal_zone_device_register("imx_thermal_zone", > IMX_TRIP_NUM, > BIT(IMX_TRIP_PASSIVE), data, > - &imx_tz_ops, NULL, > + &imx_tz_ops, &tzp, > IMX_PASSIVE_DELAY, > IMX_POLLING_DELAY); > if (IS_ERR(data->tz)) { > @@ -831,7 +819,6 @@ static int imx_thermal_probe(struct platform_device *pdev) > data->socdata->measure_temp_mask); > > data->irq_enabled = true; > - data->mode = THERMAL_DEVICE_ENABLED; > > ret = devm_request_threaded_irq(&pdev->dev, data->irq, > imx_thermal_alarm_irq, imx_thermal_alarm_irq_thread, > @@ -885,7 +872,7 @@ static int __maybe_unused imx_thermal_suspend(struct device *dev) > data->socdata->measure_temp_mask); > regmap_write(map, data->socdata->sensor_ctrl + REG_SET, > data->socdata->power_down_mask); > - data->mode = THERMAL_DEVICE_DISABLED; > + data->tz->mode = THERMAL_DEVICE_DISABLED; > clk_disable_unprepare(data->thermal_clk); > > return 0; > @@ -905,7 +892,7 @@ static int __maybe_unused imx_thermal_resume(struct device *dev) > data->socdata->power_down_mask); > regmap_write(map, data->socdata->sensor_ctrl + REG_SET, > data->socdata->measure_temp_mask); > - data->mode = THERMAL_DEVICE_ENABLED; > + data->tz->mode = THERMAL_DEVICE_ENABLED; > > return 0; > } > diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > index e802922a13cf..86a00598ed09 100644 > --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c > @@ -44,7 +44,6 @@ static char *int3400_thermal_uuids[INT3400_THERMAL_MAXIMUM_UUID] = { > struct int3400_thermal_priv { > struct acpi_device *adev; > struct thermal_zone_device *thermal; > - int mode; > int art_count; > struct art *arts; > int trt_count; > @@ -230,48 +229,29 @@ static int int3400_thermal_get_temp(struct thermal_zone_device *thermal, > return 0; > } > > -static int int3400_thermal_get_mode(struct thermal_zone_device *thermal, > - enum thermal_device_mode *mode) > -{ > - struct int3400_thermal_priv *priv = thermal->devdata; > - > - if (!priv) > - return -EINVAL; > - > - *mode = priv->mode; > - > - return 0; > -} > - > static int int3400_thermal_set_mode(struct thermal_zone_device *thermal, > enum thermal_device_mode mode) > { > struct int3400_thermal_priv *priv = thermal->devdata; > - bool enable; > int result = 0; > > if (!priv) > return -EINVAL; > > - if (mode == THERMAL_DEVICE_ENABLED) > - enable = true; > - else if (mode == THERMAL_DEVICE_DISABLED) > - enable = false; > - else > + if (mode != THERMAL_DEVICE_ENABLED && > + mode != THERMAL_DEVICE_DISABLED) > return -EINVAL; These checks should be removed as they are already done in the caller of ->set_mode method (thermal_zone_device_set_mode()). > - if (enable != priv->mode) { > - priv->mode = enable; > + if (mode != thermal->mode) { > result = int3400_thermal_run_osc(priv->adev->handle, > - priv->current_uuid_index, > - enable); > + priv->current_uuid_index, > + mode == THERMAL_DEVICE_ENABLED); > } > return result; > } > > static struct thermal_zone_device_ops int3400_thermal_ops = { > .get_temp = int3400_thermal_get_temp, > - .get_mode = int3400_thermal_get_mode, > .set_mode = int3400_thermal_set_mode, > }; > > diff --git a/drivers/thermal/intel/intel_quark_dts_thermal.c b/drivers/thermal/intel/intel_quark_dts_thermal.c > index d704fc104cfd..c4879b4bfbf1 100644 > --- a/drivers/thermal/intel/intel_quark_dts_thermal.c > +++ b/drivers/thermal/intel/intel_quark_dts_thermal.c > @@ -103,7 +103,6 @@ struct soc_sensor_entry { > bool locked; > u32 store_ptps; > u32 store_dts_enable; > - enum thermal_device_mode mode; > struct thermal_zone_device *tzone; > }; > > @@ -128,7 +127,7 @@ static int soc_dts_enable(struct thermal_zone_device *tzd) > return ret; > > if (out & QRK_DTS_ENABLE_BIT) { > - aux_entry->mode = THERMAL_DEVICE_ENABLED; > + tzd->mode = THERMAL_DEVICE_ENABLED; > return 0; > } > > @@ -139,9 +138,9 @@ static int soc_dts_enable(struct thermal_zone_device *tzd) > if (ret) > return ret; > > - aux_entry->mode = THERMAL_DEVICE_ENABLED; > + tzd->mode = THERMAL_DEVICE_ENABLED; > } else { > - aux_entry->mode = THERMAL_DEVICE_DISABLED; > + tzd->mode = THERMAL_DEVICE_DISABLED; > pr_info("DTS is locked. Cannot enable DTS\n"); > ret = -EPERM; > } > @@ -161,7 +160,7 @@ static int soc_dts_disable(struct thermal_zone_device *tzd) > return ret; > > if (!(out & QRK_DTS_ENABLE_BIT)) { > - aux_entry->mode = THERMAL_DEVICE_DISABLED; > + tzd->mode = THERMAL_DEVICE_DISABLED; > return 0; > } > > @@ -173,9 +172,9 @@ static int soc_dts_disable(struct thermal_zone_device *tzd) > if (ret) > return ret; > > - aux_entry->mode = THERMAL_DEVICE_DISABLED; > + tzd->mode = THERMAL_DEVICE_DISABLED; > } else { > - aux_entry->mode = THERMAL_DEVICE_ENABLED; > + tzd->mode = THERMAL_DEVICE_ENABLED; > pr_info("DTS is locked. Cannot disable DTS\n"); > ret = -EPERM; > } > @@ -309,14 +308,6 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd, > return 0; > } > > -static int sys_get_mode(struct thermal_zone_device *tzd, > - enum thermal_device_mode *mode) > -{ > - struct soc_sensor_entry *aux_entry = tzd->devdata; > - *mode = aux_entry->mode; > - return 0; > -} > - > static int sys_set_mode(struct thermal_zone_device *tzd, > enum thermal_device_mode mode) > { > @@ -338,7 +329,6 @@ static struct thermal_zone_device_ops tzone_ops = { > .get_trip_type = sys_get_trip_type, > .set_trip_temp = sys_set_trip_temp, > .get_crit_temp = sys_get_crit_temp, > - .get_mode = sys_get_mode, > .set_mode = sys_set_mode, > }; > > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > index 874a47d6923f..863b89546f81 100644 > --- a/drivers/thermal/of-thermal.c > +++ b/drivers/thermal/of-thermal.c > @@ -51,7 +51,6 @@ struct __thermal_bind_params { > > /** > * struct __thermal_zone - internal representation of a thermal zone > - * @mode: current thermal zone device mode (enabled/disabled) > * @passive_delay: polling interval while passive cooling is activated > * @polling_delay: zone polling interval > * @slope: slope of the temperature adjustment curve > @@ -65,7 +64,6 @@ struct __thermal_bind_params { > */ > > struct __thermal_zone { > - enum thermal_device_mode mode; > int passive_delay; > int polling_delay; > int slope; > @@ -269,23 +267,11 @@ static int of_thermal_unbind(struct thermal_zone_device *thermal, > return 0; > } > > -static int of_thermal_get_mode(struct thermal_zone_device *tz, > - enum thermal_device_mode *mode) > -{ > - struct __thermal_zone *data = tz->devdata; > - > - *mode = data->mode; > - > - return 0; > -} > - > static int of_thermal_set_mode(struct thermal_zone_device *tz, > enum thermal_device_mode mode) > { > struct __thermal_zone *data = tz->devdata; > > - mutex_lock(&tz->lock); > - > if (mode == THERMAL_DEVICE_ENABLED) { > tz->polling_delay = data->polling_delay; > tz->passive_delay = data->passive_delay; > @@ -294,11 +280,6 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz, > tz->passive_delay = 0; > } > > - mutex_unlock(&tz->lock); > - > - data->mode = mode; > - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > - > return 0; > } > > @@ -393,7 +374,6 @@ static int of_thermal_get_crit_temp(struct thermal_zone_device *tz, > } > > static struct thermal_zone_device_ops of_thermal_ops = { > - .get_mode = of_thermal_get_mode, > .set_mode = of_thermal_set_mode, > > .get_trip_type = of_thermal_get_trip_type, > @@ -553,8 +533,14 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, > if (id == sensor_id) { > tzd = thermal_zone_of_add_sensor(child, sensor_np, > data, ops); > - if (!IS_ERR(tzd)) > + if (!IS_ERR(tzd)) { > + mutex_lock(&tzd->lock); > tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED); > + tzd->mode = THERMAL_DEVICE_ENABLED; > + mutex_unlock(&tzd->lock); > + thermal_zone_device_update(tzd, > + THERMAL_EVENT_UNSPECIFIED); > + } This should use thermal_zone_device_enable() instead of open-coding it: if (!IS_ERR(tzd)) thermal_zone_device_enable(tzd); Also because of of_thermal_set_mode() modifications following of/thermal device drivers (which use ->set_mode directly): drivers/thermal/hisi_thermal.c: tzd->ops->set_mode(tzd, drivers/thermal/rockchip_thermal.c: tzd->ops->set_mode(tzd, drivers/thermal/sprd_thermal.c: tzd->ops->set_mode(tzd, need to be converted to use thermal_zone_device_{en,dis}able() instead. > of_node_put(child); > goto exit; > @@ -979,7 +965,6 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) > > finish: > of_node_put(child); > - tz->mode = THERMAL_DEVICE_DISABLED; > > return tz; > > @@ -1120,6 +1105,7 @@ int __init of_parse_thermal_zones(void) > /* these two are left for temperature drivers to use */ > tzp->slope = tz->slope; > tzp->offset = tz->offset; > + tzp->initial_mode = THERMAL_DEVICE_DISABLED; > > zone = thermal_zone_device_register(child->name, tz->ntrips, > mask, tz, > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index c06550930979..5ff98fcc0f6a 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -463,6 +463,43 @@ static void thermal_zone_device_reset(struct thermal_zone_device *tz) > thermal_zone_device_init(tz); > } > > +enum thermal_device_mode > +thermal_zone_device_get_mode(struct thermal_zone_device *tz) > +{ > + enum thermal_device_mode mode; > + > + mutex_lock(&tz->lock); > + > + mode = tz->mode; > + > + mutex_unlock(&tz->lock); > + > + return mode; > +} > + > +int thermal_zone_device_set_mode(struct thermal_zone_device *tz, > + enum thermal_device_mode mode) > +{ > + int ret = 0; > + > + if (mode != THERMAL_DEVICE_DISABLED && > + mode != THERMAL_DEVICE_ENABLED) > + return -EINVAL; No need for these checks as enum thermal_device_mode has only two possible values (we don't do "defensive coding" in the kernel). > + mutex_lock(&tz->lock); > + > + if (tz->ops->set_mode) > + ret = tz->ops->set_mode(tz, mode); > + > + tz->mode = mode; > + > + mutex_unlock(&tz->lock); > + > + thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > + > + return ret; > +} Please add: EXPORT_SYMBOL_GPL(thermal_zone_device_set_mode); here so device drivers can use thermal_zone_device_{en,dis}able(). > void thermal_zone_device_update(struct thermal_zone_device *tz, > enum thermal_notify_event event) > { > @@ -1344,6 +1381,9 @@ thermal_zone_device_register(const char *type, int trips, int mask, > if (atomic_cmpxchg(&tz->need_update, 1, 0)) > thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > > + if (tzp) > + thermal_zone_device_set_mode(tz, tzp->initial_mode); Please note that currently some device drivers don't access ->mode at all so their default ->mode value is THERMAL_DEVICE_DISABLED but in reality they are enabled (IOW after changes in this patch they should default to THERMAL_DEVICE_ENABLED to reflect real device state in "mode" sysfs attribue and not break after applying patch #2). Therefore we should always call thermal_zone_device_set_mode() and use the default value of THERMAL_DEVICE_ENABLED if tzp is NULL: mode = tzp ? tzp->initial_mode : THERMAL_DEVICE_ENABLED; thermal_zone_device_set_mode(tz, mode); This would also simplify the patch further: * setting tz->need_update and calling thermal_zone_device_update() directly can be removed from thermal_zone_device_register() * tzp-s with only .initial_mode = THERMAL_DEVICE_ENABLED set can be removed from: drivers/thermal/acpi.c drivers/net/ethernet/mellanox/mlxsw/core_thermal.c drivers/thermal/da9062-thermal.c drivers/thermal/imx_thermal.c The rest of the patch looks fine, thank you for working on this! Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics > + > return tz; > > unregister: > @@ -1473,9 +1513,7 @@ static int thermal_pm_notify(struct notifier_block *nb, > case PM_POST_SUSPEND: > atomic_set(&in_suspend, 0); > list_for_each_entry(tz, &thermal_tz_list, node) { > - tz_mode = THERMAL_DEVICE_ENABLED; > - if (tz->ops->get_mode) > - tz->ops->get_mode(tz, &tz_mode); > + tz_mode = thermal_zone_device_get_mode(tz); > > if (tz_mode == THERMAL_DEVICE_DISABLED) > continue; > diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h > index c95689586e19..8e561bac3133 100644 > --- a/drivers/thermal/thermal_core.h > +++ b/drivers/thermal/thermal_core.h > @@ -141,6 +141,22 @@ thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev, > unsigned long new_state) {} > #endif /* CONFIG_THERMAL_STATISTICS */ > > +enum thermal_device_mode > +thermal_zone_device_get_mode(struct thermal_zone_device *tz); > + > +int thermal_zone_device_set_mode(struct thermal_zone_device *tz, > + enum thermal_device_mode mode); > + > +static inline int thermal_zone_device_enable(struct thermal_zone_device *tz) > +{ > + return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_ENABLED); > +} > + > +static inline int thermal_zone_device_disable(struct thermal_zone_device *tz) > +{ > + return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_DISABLED); > +} > + > /* device tree support */ > #ifdef CONFIG_THERMAL_OF > int of_parse_thermal_zones(void); > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c > index aa99edb4dff7..cbb27b3c96d2 100644 > --- a/drivers/thermal/thermal_sysfs.c > +++ b/drivers/thermal/thermal_sysfs.c > @@ -50,14 +50,8 @@ mode_show(struct device *dev, struct device_attribute *attr, char *buf) > { > struct thermal_zone_device *tz = to_thermal_zone(dev); > enum thermal_device_mode mode; > - int result; > - > - if (!tz->ops->get_mode) > - return -EPERM; > > - result = tz->ops->get_mode(tz, &mode); > - if (result) > - return result; > + mode = thermal_zone_device_get_mode(tz); > > return sprintf(buf, "%s\n", mode == THERMAL_DEVICE_ENABLED ? "enabled" > : "disabled"); > @@ -74,9 +68,9 @@ mode_store(struct device *dev, struct device_attribute *attr, > return -EPERM; > > if (!strncmp(buf, "enabled", sizeof("enabled") - 1)) > - result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED); > + result = thermal_zone_device_enable(tz); > else if (!strncmp(buf, "disabled", sizeof("disabled") - 1)) > - result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED); > + result = thermal_zone_device_disable(tz); > else > result = -EINVAL; > > @@ -428,30 +422,13 @@ static struct attribute_group thermal_zone_attribute_group = { > .attrs = thermal_zone_dev_attrs, > }; > > -/* We expose mode only if .get_mode is present */ > static struct attribute *thermal_zone_mode_attrs[] = { > &dev_attr_mode.attr, > NULL, > }; > > -static umode_t thermal_zone_mode_is_visible(struct kobject *kobj, > - struct attribute *attr, > - int attrno) > -{ > - struct device *dev = container_of(kobj, struct device, kobj); > - struct thermal_zone_device *tz; > - > - tz = container_of(dev, struct thermal_zone_device, device); > - > - if (tz->ops->get_mode) > - return attr->mode; > - > - return 0; > -} > - > static struct attribute_group thermal_zone_mode_attribute_group = { > .attrs = thermal_zone_mode_attrs, > - .is_visible = thermal_zone_mode_is_visible, > }; > > /* We expose passive only if passive trips are present */ > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > index 216185bb3014..da4141697e2e 100644 > --- a/include/linux/thermal.h > +++ b/include/linux/thermal.h > @@ -76,8 +76,6 @@ struct thermal_zone_device_ops { > struct thermal_cooling_device *); > int (*get_temp) (struct thermal_zone_device *, int *); > int (*set_trips) (struct thermal_zone_device *, int, int); > - int (*get_mode) (struct thermal_zone_device *, > - enum thermal_device_mode *); > int (*set_mode) (struct thermal_zone_device *, > enum thermal_device_mode); > int (*get_trip_type) (struct thermal_zone_device *, int, > @@ -128,6 +126,7 @@ struct thermal_cooling_device { > * @trip_temp_attrs: attributes for trip points for sysfs: trip temperature > * @trip_type_attrs: attributes for trip points for sysfs: trip type > * @trip_hyst_attrs: attributes for trip points for sysfs: trip hysteresis > + * @mode: current mode of this thermal zone > * @devdata: private pointer for device private data > * @trips: number of trip points the thermal zone supports > * @trips_disabled; bitmap for disabled trips > @@ -170,6 +169,7 @@ struct thermal_zone_device { > struct thermal_attr *trip_temp_attrs; > struct thermal_attr *trip_type_attrs; > struct thermal_attr *trip_hyst_attrs; > + enum thermal_device_mode mode; > void *devdata; > int trips; > unsigned long trips_disabled; /* bitmap for disabled trips */ > @@ -264,6 +264,9 @@ struct thermal_zone_params { > int num_tbps; /* Number of tbp entries */ > struct thermal_bind_params *tbp; > > + /* Initial mode of this thermal zone device */ > + enum thermal_device_mode initial_mode; > + > /* > * Sustainable power (heat) that this thermal zone can dissipate in > * mW >
On 4/19/20 1:38 PM, Bartlomiej Zolnierkiewicz wrote: > > Hi Andrzej, > > On 4/17/20 6:20 PM, Andrzej Pietrasiewicz wrote: >> Thermal zone devices' mode is stored in individual drivers. This patch >> changes it so that mode is stored in struct thermal_zone_device instead. >> >> As a result all driver-specific variables storing the mode are not needed >> and are removed. Consequently, the get_mode() implementations have nothing >> to operate on and need to be removed, too. >> >> Some thermal framework specific functions are introduced: >> >> thermal_zone_device_get_mode() >> thermal_zone_device_set_mode() >> thermal_zone_device_enable() >> thermal_zone_device_disable() >> >> thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock >> and the "set" calls driver's set_mode() if provided, so the latter must >> not take this lock again. At the end of the "set" >> thermal_zone_device_update() is called so drivers don't need to repeat this >> invocation in their specific set_mode() implementations. >> >> The scope of the above 4 functions is purposedly limited to the thermal >> framework and drivers are not supposed to call them. This encapsulation > > This should be true only for thermal_zone_device_{get,set}_mode(). > > thermal_zone_device_{en,dis}able() should be available for device drivers: > > * of/thermal device drivers need to enable thermal device itself > (please refer to my patchset for details) > > * device drivers need to call them on ->suspend and ->resume operations > >> does not fully work at the moment for some drivers, though: >> >> - platform/x86/acerhdf.c >> - drivers/thermal/imx_thermal.c >> - drivers/thermal/intel/intel_quark_dts_thermal.c >> - drivers/thermal/of-thermal.c >> >> and they manipulate struct thermal_zone_device's members directly. >> >> struct thermal_zone_params gains a new member called initial_mode, which >> is used to set tzd's mode at registration time. >> >> The sysfs "mode" attribute is always exposed from now on, because all >> thermal zone devices now have their get_mode() implemented at the generic >> level and it is always available. Exposing "mode" doesn't hurt the drivers >> which don't provide their own set_mode(), because writing to "mode" will >> result in -EPERM, as expected. >> >> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> >> --- >> drivers/acpi/thermal.c | 46 +++++---------- >> .../ethernet/mellanox/mlxsw/core_thermal.c | 57 ++++--------------- >> drivers/platform/x86/acerhdf.c | 17 +----- >> drivers/thermal/da9062-thermal.c | 16 ++---- >> drivers/thermal/imx_thermal.c | 29 +++------- >> .../intel/int340x_thermal/int3400_thermal.c | 30 ++-------- >> .../thermal/intel/intel_quark_dts_thermal.c | 22 ++----- >> drivers/thermal/of-thermal.c | 30 +++------- >> drivers/thermal/thermal_core.c | 44 +++++++++++++- >> drivers/thermal/thermal_core.h | 16 ++++++ >> drivers/thermal/thermal_sysfs.c | 29 +--------- >> include/linux/thermal.h | 7 ++- >> 12 files changed, 125 insertions(+), 218 deletions(-) >> >> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c >> index 19067a5e5293..67bc263332a5 100644 >> --- a/drivers/acpi/thermal.c >> +++ b/drivers/acpi/thermal.c >> @@ -172,7 +172,6 @@ struct acpi_thermal { >> struct acpi_thermal_trips trips; >> struct acpi_handle_list devices; >> struct thermal_zone_device *thermal_zone; >> - int tz_enabled; >> int kelvin_offset; /* in millidegrees */ >> struct work_struct thermal_check_work; >> }; >> @@ -500,7 +499,7 @@ static void acpi_thermal_check(void *data) >> { >> struct acpi_thermal *tz = data; >> >> - if (!tz->tz_enabled) >> + if (tz->thermal_zone->mode != THERMAL_DEVICE_ENABLED) >> return; >> >> thermal_zone_device_update(tz->thermal_zone, >> @@ -526,46 +525,29 @@ static int thermal_get_temp(struct thermal_zone_device *thermal, int *temp) >> return 0; >> } >> >> -static int thermal_get_mode(struct thermal_zone_device *thermal, >> - enum thermal_device_mode *mode) >> -{ >> - struct acpi_thermal *tz = thermal->devdata; >> - >> - if (!tz) >> - return -EINVAL; >> - >> - *mode = tz->tz_enabled ? THERMAL_DEVICE_ENABLED : >> - THERMAL_DEVICE_DISABLED; >> - >> - return 0; >> -} >> - >> static int thermal_set_mode(struct thermal_zone_device *thermal, >> enum thermal_device_mode mode) >> { >> struct acpi_thermal *tz = thermal->devdata; >> - int enable; >> >> if (!tz) >> return -EINVAL; >> >> + if (mode != THERMAL_DEVICE_DISABLED && >> + mode != THERMAL_DEVICE_ENABLED) >> + return -EINVAL; > > These checks should be removed as they are already done in the caller > of ->set_mode method (thermal_zone_device_set_mode()). > >> + >> /* >> * enable/disable thermal management from ACPI thermal driver >> */ >> - if (mode == THERMAL_DEVICE_ENABLED) >> - enable = 1; >> - else if (mode == THERMAL_DEVICE_DISABLED) { >> - enable = 0; >> + if (mode == THERMAL_DEVICE_DISABLED) >> pr_warn("thermal zone will be disabled\n"); >> - } else >> - return -EINVAL; >> >> - if (enable != tz->tz_enabled) { >> - tz->tz_enabled = enable; >> + if (mode != thermal->mode) { >> ACPI_DEBUG_PRINT((ACPI_DB_INFO, >> "%s kernel ACPI thermal control\n", >> - tz->tz_enabled ? "Enable" : "Disable")); >> - acpi_thermal_check(tz); >> + mode == THERMAL_DEVICE_ENABLED ? >> + "Enable" : "Disable")); >> } >> return 0; >> } >> @@ -856,7 +838,6 @@ static struct thermal_zone_device_ops acpi_thermal_zone_ops = { >> .bind = acpi_thermal_bind_cooling_device, >> .unbind = acpi_thermal_unbind_cooling_device, >> .get_temp = thermal_get_temp, >> - .get_mode = thermal_get_mode, >> .set_mode = thermal_set_mode, >> .get_trip_type = thermal_get_trip_type, >> .get_trip_temp = thermal_get_trip_temp, >> @@ -870,6 +851,9 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) >> int trips = 0; >> int result; >> acpi_status status; >> + struct thermal_zone_params prms = { >> + .initial_mode = THERMAL_DEVICE_ENABLED, >> + }; >> int i; >> >> if (tz->trips.critical.flags.valid) >> @@ -887,13 +871,13 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) >> if (tz->trips.passive.flags.valid) >> tz->thermal_zone = >> thermal_zone_device_register("acpitz", trips, 0, tz, >> - &acpi_thermal_zone_ops, NULL, >> + &acpi_thermal_zone_ops, &prms, >> tz->trips.passive.tsp*100, >> tz->polling_frequency*100); >> else >> tz->thermal_zone = >> thermal_zone_device_register("acpitz", trips, 0, tz, >> - &acpi_thermal_zone_ops, NULL, >> + &acpi_thermal_zone_ops, &prms, >> 0, tz->polling_frequency*100); >> if (IS_ERR(tz->thermal_zone)) >> return -ENODEV; >> @@ -913,8 +897,6 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) >> if (ACPI_FAILURE(status)) >> return -ENODEV; >> >> - tz->tz_enabled = 1; >> - >> dev_info(&tz->device->dev, "registered as thermal_zone%d\n", >> tz->thermal_zone->id); >> return 0; >> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c >> index ce0a6837daa3..50518048b86d 100644 >> --- a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c >> +++ b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c >> @@ -98,7 +98,6 @@ struct mlxsw_thermal_module { >> struct mlxsw_thermal *parent; >> struct thermal_zone_device *tzdev; >> struct mlxsw_thermal_trip trips[MLXSW_THERMAL_NUM_TRIPS]; >> - enum thermal_device_mode mode; >> int module; /* Module or gearbox number */ >> }; >> >> @@ -110,7 +109,6 @@ struct mlxsw_thermal { >> struct thermal_cooling_device *cdevs[MLXSW_MFCR_PWMS_MAX]; >> u8 cooling_levels[MLXSW_THERMAL_MAX_STATE + 1]; >> struct mlxsw_thermal_trip trips[MLXSW_THERMAL_NUM_TRIPS]; >> - enum thermal_device_mode mode; >> struct mlxsw_thermal_module *tz_module_arr; >> u8 tz_module_num; >> struct mlxsw_thermal_module *tz_gearbox_arr; >> @@ -277,33 +275,16 @@ static int mlxsw_thermal_unbind(struct thermal_zone_device *tzdev, >> return 0; >> } >> >> -static int mlxsw_thermal_get_mode(struct thermal_zone_device *tzdev, >> - enum thermal_device_mode *mode) >> -{ >> - struct mlxsw_thermal *thermal = tzdev->devdata; >> - >> - *mode = thermal->mode; >> - >> - return 0; >> -} >> - >> static int mlxsw_thermal_set_mode(struct thermal_zone_device *tzdev, >> enum thermal_device_mode mode) >> { >> struct mlxsw_thermal *thermal = tzdev->devdata; >> >> - mutex_lock(&tzdev->lock); >> - >> if (mode == THERMAL_DEVICE_ENABLED) >> tzdev->polling_delay = thermal->polling_delay; >> else >> tzdev->polling_delay = 0; >> >> - mutex_unlock(&tzdev->lock); >> - >> - thermal->mode = mode; >> - thermal_zone_device_update(tzdev, THERMAL_EVENT_UNSPECIFIED); >> - >> return 0; >> } >> >> @@ -407,7 +388,6 @@ static int mlxsw_thermal_trend_get(struct thermal_zone_device *tzdev, >> static struct thermal_zone_device_ops mlxsw_thermal_ops = { >> .bind = mlxsw_thermal_bind, >> .unbind = mlxsw_thermal_unbind, >> - .get_mode = mlxsw_thermal_get_mode, >> .set_mode = mlxsw_thermal_set_mode, >> .get_temp = mlxsw_thermal_get_temp, >> .get_trip_type = mlxsw_thermal_get_trip_type, >> @@ -466,34 +446,17 @@ static int mlxsw_thermal_module_unbind(struct thermal_zone_device *tzdev, >> return err; >> } >> >> -static int mlxsw_thermal_module_mode_get(struct thermal_zone_device *tzdev, >> - enum thermal_device_mode *mode) >> -{ >> - struct mlxsw_thermal_module *tz = tzdev->devdata; >> - >> - *mode = tz->mode; >> - >> - return 0; >> -} >> - >> static int mlxsw_thermal_module_mode_set(struct thermal_zone_device *tzdev, >> enum thermal_device_mode mode) >> { >> struct mlxsw_thermal_module *tz = tzdev->devdata; >> struct mlxsw_thermal *thermal = tz->parent; >> >> - mutex_lock(&tzdev->lock); >> - >> if (mode == THERMAL_DEVICE_ENABLED) >> tzdev->polling_delay = thermal->polling_delay; >> else >> tzdev->polling_delay = 0; >> >> - mutex_unlock(&tzdev->lock); >> - >> - tz->mode = mode; >> - thermal_zone_device_update(tzdev, THERMAL_EVENT_UNSPECIFIED); >> - >> return 0; >> } >> >> @@ -596,7 +559,6 @@ mlxsw_thermal_module_trip_hyst_set(struct thermal_zone_device *tzdev, int trip, >> static struct thermal_zone_device_ops mlxsw_thermal_module_ops = { >> .bind = mlxsw_thermal_module_bind, >> .unbind = mlxsw_thermal_module_unbind, >> - .get_mode = mlxsw_thermal_module_mode_get, >> .set_mode = mlxsw_thermal_module_mode_set, >> .get_temp = mlxsw_thermal_module_temp_get, >> .get_trip_type = mlxsw_thermal_module_trip_type_get, >> @@ -635,7 +597,6 @@ static int mlxsw_thermal_gearbox_temp_get(struct thermal_zone_device *tzdev, >> static struct thermal_zone_device_ops mlxsw_thermal_gearbox_ops = { >> .bind = mlxsw_thermal_module_bind, >> .unbind = mlxsw_thermal_module_unbind, >> - .get_mode = mlxsw_thermal_module_mode_get, >> .set_mode = mlxsw_thermal_module_mode_set, >> .get_temp = mlxsw_thermal_gearbox_temp_get, >> .get_trip_type = mlxsw_thermal_module_trip_type_get, >> @@ -749,6 +710,9 @@ static const struct thermal_cooling_device_ops mlxsw_cooling_ops = { >> static int >> mlxsw_thermal_module_tz_init(struct mlxsw_thermal_module *module_tz) >> { >> + struct thermal_zone_params tzp = { >> + .initial_mode = THERMAL_DEVICE_ENABLED, >> + }; >> char tz_name[MLXSW_THERMAL_ZONE_MAX_NAME]; >> int err; >> >> @@ -759,13 +723,12 @@ mlxsw_thermal_module_tz_init(struct mlxsw_thermal_module *module_tz) >> MLXSW_THERMAL_TRIP_MASK, >> module_tz, >> &mlxsw_thermal_module_ops, >> - NULL, 0, 0); >> + &tzp, 0, 0); >> if (IS_ERR(module_tz->tzdev)) { >> err = PTR_ERR(module_tz->tzdev); >> return err; >> } >> >> - module_tz->mode = THERMAL_DEVICE_ENABLED; >> return 0; >> } >> >> @@ -868,6 +831,9 @@ mlxsw_thermal_modules_fini(struct mlxsw_thermal *thermal) >> static int >> mlxsw_thermal_gearbox_tz_init(struct mlxsw_thermal_module *gearbox_tz) >> { >> + struct thermal_zone_params tzp = { >> + .initial_mode = THERMAL_DEVICE_ENABLED, >> + }; >> char tz_name[MLXSW_THERMAL_ZONE_MAX_NAME]; >> >> snprintf(tz_name, sizeof(tz_name), "mlxsw-gearbox%d", >> @@ -877,11 +843,10 @@ mlxsw_thermal_gearbox_tz_init(struct mlxsw_thermal_module *gearbox_tz) >> MLXSW_THERMAL_TRIP_MASK, >> gearbox_tz, >> &mlxsw_thermal_gearbox_ops, >> - NULL, 0, 0); >> + &tzp, 0, 0); >> if (IS_ERR(gearbox_tz->tzdev)) >> return PTR_ERR(gearbox_tz->tzdev); >> >> - gearbox_tz->mode = THERMAL_DEVICE_ENABLED; >> return 0; >> } >> >> @@ -960,6 +925,9 @@ int mlxsw_thermal_init(struct mlxsw_core *core, >> const struct mlxsw_bus_info *bus_info, >> struct mlxsw_thermal **p_thermal) >> { >> + struct thermal_zone_params tzp = { >> + .initial_mode = THERMAL_DEVICE_ENABLED, >> + }; >> char mfcr_pl[MLXSW_REG_MFCR_LEN] = { 0 }; >> enum mlxsw_reg_mfcr_pwm_frequency freq; >> struct device *dev = bus_info->dev; >> @@ -1034,7 +1002,7 @@ int mlxsw_thermal_init(struct mlxsw_core *core, >> MLXSW_THERMAL_TRIP_MASK, >> thermal, >> &mlxsw_thermal_ops, >> - NULL, 0, >> + &tzp, 0, >> thermal->polling_delay); >> if (IS_ERR(thermal->tzdev)) { >> err = PTR_ERR(thermal->tzdev); >> @@ -1050,7 +1018,6 @@ int mlxsw_thermal_init(struct mlxsw_core *core, >> if (err) >> goto err_unreg_modules_tzdev; >> >> - thermal->mode = THERMAL_DEVICE_ENABLED; >> *p_thermal = thermal; >> return 0; >> >> diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c >> index 8cc86f4e3ac1..aaf8b845be90 100644 >> --- a/drivers/platform/x86/acerhdf.c >> +++ b/drivers/platform/x86/acerhdf.c >> @@ -406,22 +406,9 @@ static inline void acerhdf_enable_kernelmode(void) >> kernelmode = 1; >> >> thz_dev->polling_delay = interval*1000; >> - thermal_zone_device_update(thz_dev, THERMAL_EVENT_UNSPECIFIED); >> pr_notice("kernel mode fan control ON\n"); >> } >> >> -static int acerhdf_get_mode(struct thermal_zone_device *thermal, >> - enum thermal_device_mode *mode) >> -{ >> - if (verbose) >> - pr_notice("kernel mode fan control %d\n", kernelmode); >> - >> - *mode = (kernelmode) ? THERMAL_DEVICE_ENABLED >> - : THERMAL_DEVICE_DISABLED; >> - >> - return 0; >> -} >> - >> /* >> * set operation mode; >> * enabled: the thermal layer of the kernel takes care about >> @@ -488,7 +475,6 @@ static struct thermal_zone_device_ops acerhdf_dev_ops = { >> .bind = acerhdf_bind, >> .unbind = acerhdf_unbind, >> .get_temp = acerhdf_get_ec_temp, >> - .get_mode = acerhdf_get_mode, >> .set_mode = acerhdf_set_mode, >> .get_trip_type = acerhdf_get_trip_type, >> .get_trip_hyst = acerhdf_get_trip_hyst, >> @@ -554,6 +540,7 @@ static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev, >> >> err_out: >> acerhdf_revert_to_bios_mode(); >> + thz_dev->mode = THERMAL_DEVICE_DISABLED; >> return -EINVAL; >> } >> >> @@ -739,6 +726,8 @@ static int __init acerhdf_register_thermal(void) >> if (IS_ERR(cl_dev)) >> return -EINVAL; >> >> + acerhdf_zone_params.initial_mode = >> + kernelmode ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED; >> thz_dev = thermal_zone_device_register("acerhdf", 2, 0, NULL, >> &acerhdf_dev_ops, >> &acerhdf_zone_params, 0, >> diff --git a/drivers/thermal/da9062-thermal.c b/drivers/thermal/da9062-thermal.c >> index c32709badeda..4bdb6f9621c1 100644 >> --- a/drivers/thermal/da9062-thermal.c >> +++ b/drivers/thermal/da9062-thermal.c >> @@ -49,7 +49,6 @@ struct da9062_thermal { >> struct da9062 *hw; >> struct delayed_work work; >> struct thermal_zone_device *zone; >> - enum thermal_device_mode mode; >> struct mutex lock; /* protection for da9062_thermal temperature */ >> int temperature; >> int irq; >> @@ -121,14 +120,6 @@ static irqreturn_t da9062_thermal_irq_handler(int irq, void *data) >> return IRQ_HANDLED; >> } >> >> -static int da9062_thermal_get_mode(struct thermal_zone_device *z, >> - enum thermal_device_mode *mode) >> -{ >> - struct da9062_thermal *thermal = z->devdata; >> - *mode = thermal->mode; >> - return 0; >> -} >> - >> static int da9062_thermal_get_trip_type(struct thermal_zone_device *z, >> int trip, >> enum thermal_trip_type *type) >> @@ -181,7 +172,6 @@ static int da9062_thermal_get_temp(struct thermal_zone_device *z, >> >> static struct thermal_zone_device_ops da9062_thermal_ops = { >> .get_temp = da9062_thermal_get_temp, >> - .get_mode = da9062_thermal_get_mode, >> .get_trip_type = da9062_thermal_get_trip_type, >> .get_trip_temp = da9062_thermal_get_trip_temp, >> }; >> @@ -199,6 +189,9 @@ MODULE_DEVICE_TABLE(of, da9062_compatible_reg_id_table); >> >> static int da9062_thermal_probe(struct platform_device *pdev) >> { >> + struct thermal_zone_params tzp = { >> + .initial_mode = THERMAL_DEVICE_ENABLED, >> + }; >> struct da9062 *chip = dev_get_drvdata(pdev->dev.parent); >> struct da9062_thermal *thermal; >> unsigned int pp_tmp = DA9062_DEFAULT_POLLING_MS_PERIOD; >> @@ -233,7 +226,6 @@ static int da9062_thermal_probe(struct platform_device *pdev) >> >> thermal->config = match->data; >> thermal->hw = chip; >> - thermal->mode = THERMAL_DEVICE_ENABLED; >> thermal->dev = &pdev->dev; >> >> INIT_DELAYED_WORK(&thermal->work, da9062_thermal_poll_on); >> @@ -241,7 +233,7 @@ static int da9062_thermal_probe(struct platform_device *pdev) >> >> thermal->zone = thermal_zone_device_register(thermal->config->name, >> 1, 0, thermal, >> - &da9062_thermal_ops, NULL, pp_tmp, >> + &da9062_thermal_ops, &tzp, pp_tmp, >> 0); >> if (IS_ERR(thermal->zone)) { >> dev_err(&pdev->dev, "Cannot register thermal zone device\n"); >> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c >> index e761c9b42217..3e02323c938b 100644 >> --- a/drivers/thermal/imx_thermal.c >> +++ b/drivers/thermal/imx_thermal.c >> @@ -197,7 +197,6 @@ struct imx_thermal_data { >> struct cpufreq_policy *policy; >> struct thermal_zone_device *tz; >> struct thermal_cooling_device *cdev; >> - enum thermal_device_mode mode; >> struct regmap *tempmon; >> u32 c1, c2; /* See formula in imx_init_calib() */ >> int temp_passive; >> @@ -256,7 +255,7 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) >> bool wait; >> u32 val; >> >> - if (data->mode == THERMAL_DEVICE_ENABLED) { >> + if (tz->mode == THERMAL_DEVICE_ENABLED) { >> /* Check if a measurement is currently in progress */ >> regmap_read(map, soc_data->temp_data, &val); >> wait = !(val & soc_data->temp_valid_mask); >> @@ -283,7 +282,7 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) >> >> regmap_read(map, soc_data->temp_data, &val); >> >> - if (data->mode != THERMAL_DEVICE_ENABLED) { >> + if (tz->mode != THERMAL_DEVICE_ENABLED) { >> regmap_write(map, soc_data->sensor_ctrl + REG_CLR, >> soc_data->measure_temp_mask); >> regmap_write(map, soc_data->sensor_ctrl + REG_SET, >> @@ -331,16 +330,6 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) >> return 0; >> } >> >> -static int imx_get_mode(struct thermal_zone_device *tz, >> - enum thermal_device_mode *mode) >> -{ >> - struct imx_thermal_data *data = tz->devdata; >> - >> - *mode = data->mode; >> - >> - return 0; >> -} >> - >> static int imx_set_mode(struct thermal_zone_device *tz, >> enum thermal_device_mode mode) >> { >> @@ -376,9 +365,6 @@ static int imx_set_mode(struct thermal_zone_device *tz, >> } >> } >> >> - data->mode = mode; >> - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); >> - >> return 0; >> } >> >> @@ -467,7 +453,6 @@ static struct thermal_zone_device_ops imx_tz_ops = { >> .bind = imx_bind, >> .unbind = imx_unbind, >> .get_temp = imx_get_temp, >> - .get_mode = imx_get_mode, >> .set_mode = imx_set_mode, >> .get_trip_type = imx_get_trip_type, >> .get_trip_temp = imx_get_trip_temp, >> @@ -691,6 +676,9 @@ static inline void imx_thermal_unregister_legacy_cooling(struct imx_thermal_data >> >> static int imx_thermal_probe(struct platform_device *pdev) >> { >> + struct thermal_zone_params tzp = { >> + .initial_mode = THERMAL_DEVICE_ENABLED, >> + }; >> struct imx_thermal_data *data; >> struct regmap *map; >> int measure_freq; >> @@ -799,7 +787,7 @@ static int imx_thermal_probe(struct platform_device *pdev) >> data->tz = thermal_zone_device_register("imx_thermal_zone", >> IMX_TRIP_NUM, >> BIT(IMX_TRIP_PASSIVE), data, >> - &imx_tz_ops, NULL, >> + &imx_tz_ops, &tzp, >> IMX_PASSIVE_DELAY, >> IMX_POLLING_DELAY); >> if (IS_ERR(data->tz)) { >> @@ -831,7 +819,6 @@ static int imx_thermal_probe(struct platform_device *pdev) >> data->socdata->measure_temp_mask); >> >> data->irq_enabled = true; >> - data->mode = THERMAL_DEVICE_ENABLED; >> >> ret = devm_request_threaded_irq(&pdev->dev, data->irq, >> imx_thermal_alarm_irq, imx_thermal_alarm_irq_thread, >> @@ -885,7 +872,7 @@ static int __maybe_unused imx_thermal_suspend(struct device *dev) >> data->socdata->measure_temp_mask); >> regmap_write(map, data->socdata->sensor_ctrl + REG_SET, >> data->socdata->power_down_mask); >> - data->mode = THERMAL_DEVICE_DISABLED; >> + data->tz->mode = THERMAL_DEVICE_DISABLED; >> clk_disable_unprepare(data->thermal_clk); >> >> return 0; >> @@ -905,7 +892,7 @@ static int __maybe_unused imx_thermal_resume(struct device *dev) >> data->socdata->power_down_mask); >> regmap_write(map, data->socdata->sensor_ctrl + REG_SET, >> data->socdata->measure_temp_mask); >> - data->mode = THERMAL_DEVICE_ENABLED; >> + data->tz->mode = THERMAL_DEVICE_ENABLED; >> >> return 0; >> } >> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c >> index e802922a13cf..86a00598ed09 100644 >> --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c >> +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c >> @@ -44,7 +44,6 @@ static char *int3400_thermal_uuids[INT3400_THERMAL_MAXIMUM_UUID] = { >> struct int3400_thermal_priv { >> struct acpi_device *adev; >> struct thermal_zone_device *thermal; >> - int mode; >> int art_count; >> struct art *arts; >> int trt_count; >> @@ -230,48 +229,29 @@ static int int3400_thermal_get_temp(struct thermal_zone_device *thermal, >> return 0; >> } >> >> -static int int3400_thermal_get_mode(struct thermal_zone_device *thermal, >> - enum thermal_device_mode *mode) >> -{ >> - struct int3400_thermal_priv *priv = thermal->devdata; >> - >> - if (!priv) >> - return -EINVAL; >> - >> - *mode = priv->mode; >> - >> - return 0; >> -} >> - >> static int int3400_thermal_set_mode(struct thermal_zone_device *thermal, >> enum thermal_device_mode mode) >> { >> struct int3400_thermal_priv *priv = thermal->devdata; >> - bool enable; >> int result = 0; >> >> if (!priv) >> return -EINVAL; >> >> - if (mode == THERMAL_DEVICE_ENABLED) >> - enable = true; >> - else if (mode == THERMAL_DEVICE_DISABLED) >> - enable = false; >> - else >> + if (mode != THERMAL_DEVICE_ENABLED && >> + mode != THERMAL_DEVICE_DISABLED) >> return -EINVAL; > > These checks should be removed as they are already done in the caller > of ->set_mode method (thermal_zone_device_set_mode()). > >> - if (enable != priv->mode) { >> - priv->mode = enable; >> + if (mode != thermal->mode) { >> result = int3400_thermal_run_osc(priv->adev->handle, >> - priv->current_uuid_index, >> - enable); >> + priv->current_uuid_index, >> + mode == THERMAL_DEVICE_ENABLED); >> } >> return result; >> } >> >> static struct thermal_zone_device_ops int3400_thermal_ops = { >> .get_temp = int3400_thermal_get_temp, >> - .get_mode = int3400_thermal_get_mode, >> .set_mode = int3400_thermal_set_mode, >> }; >> >> diff --git a/drivers/thermal/intel/intel_quark_dts_thermal.c b/drivers/thermal/intel/intel_quark_dts_thermal.c >> index d704fc104cfd..c4879b4bfbf1 100644 >> --- a/drivers/thermal/intel/intel_quark_dts_thermal.c >> +++ b/drivers/thermal/intel/intel_quark_dts_thermal.c >> @@ -103,7 +103,6 @@ struct soc_sensor_entry { >> bool locked; >> u32 store_ptps; >> u32 store_dts_enable; >> - enum thermal_device_mode mode; >> struct thermal_zone_device *tzone; >> }; >> >> @@ -128,7 +127,7 @@ static int soc_dts_enable(struct thermal_zone_device *tzd) >> return ret; >> >> if (out & QRK_DTS_ENABLE_BIT) { >> - aux_entry->mode = THERMAL_DEVICE_ENABLED; >> + tzd->mode = THERMAL_DEVICE_ENABLED; >> return 0; >> } >> >> @@ -139,9 +138,9 @@ static int soc_dts_enable(struct thermal_zone_device *tzd) >> if (ret) >> return ret; >> >> - aux_entry->mode = THERMAL_DEVICE_ENABLED; >> + tzd->mode = THERMAL_DEVICE_ENABLED; >> } else { >> - aux_entry->mode = THERMAL_DEVICE_DISABLED; >> + tzd->mode = THERMAL_DEVICE_DISABLED; >> pr_info("DTS is locked. Cannot enable DTS\n"); >> ret = -EPERM; >> } >> @@ -161,7 +160,7 @@ static int soc_dts_disable(struct thermal_zone_device *tzd) >> return ret; >> >> if (!(out & QRK_DTS_ENABLE_BIT)) { >> - aux_entry->mode = THERMAL_DEVICE_DISABLED; >> + tzd->mode = THERMAL_DEVICE_DISABLED; >> return 0; >> } >> >> @@ -173,9 +172,9 @@ static int soc_dts_disable(struct thermal_zone_device *tzd) >> if (ret) >> return ret; >> >> - aux_entry->mode = THERMAL_DEVICE_DISABLED; >> + tzd->mode = THERMAL_DEVICE_DISABLED; >> } else { >> - aux_entry->mode = THERMAL_DEVICE_ENABLED; >> + tzd->mode = THERMAL_DEVICE_ENABLED; >> pr_info("DTS is locked. Cannot disable DTS\n"); >> ret = -EPERM; >> } >> @@ -309,14 +308,6 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd, >> return 0; >> } >> >> -static int sys_get_mode(struct thermal_zone_device *tzd, >> - enum thermal_device_mode *mode) >> -{ >> - struct soc_sensor_entry *aux_entry = tzd->devdata; >> - *mode = aux_entry->mode; >> - return 0; >> -} >> - >> static int sys_set_mode(struct thermal_zone_device *tzd, >> enum thermal_device_mode mode) >> { >> @@ -338,7 +329,6 @@ static struct thermal_zone_device_ops tzone_ops = { >> .get_trip_type = sys_get_trip_type, >> .set_trip_temp = sys_set_trip_temp, >> .get_crit_temp = sys_get_crit_temp, >> - .get_mode = sys_get_mode, >> .set_mode = sys_set_mode, >> }; >> >> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c >> index 874a47d6923f..863b89546f81 100644 >> --- a/drivers/thermal/of-thermal.c >> +++ b/drivers/thermal/of-thermal.c >> @@ -51,7 +51,6 @@ struct __thermal_bind_params { >> >> /** >> * struct __thermal_zone - internal representation of a thermal zone >> - * @mode: current thermal zone device mode (enabled/disabled) >> * @passive_delay: polling interval while passive cooling is activated >> * @polling_delay: zone polling interval >> * @slope: slope of the temperature adjustment curve >> @@ -65,7 +64,6 @@ struct __thermal_bind_params { >> */ >> >> struct __thermal_zone { >> - enum thermal_device_mode mode; >> int passive_delay; >> int polling_delay; >> int slope; >> @@ -269,23 +267,11 @@ static int of_thermal_unbind(struct thermal_zone_device *thermal, >> return 0; >> } >> >> -static int of_thermal_get_mode(struct thermal_zone_device *tz, >> - enum thermal_device_mode *mode) >> -{ >> - struct __thermal_zone *data = tz->devdata; >> - >> - *mode = data->mode; >> - >> - return 0; >> -} >> - >> static int of_thermal_set_mode(struct thermal_zone_device *tz, >> enum thermal_device_mode mode) >> { >> struct __thermal_zone *data = tz->devdata; >> >> - mutex_lock(&tz->lock); >> - >> if (mode == THERMAL_DEVICE_ENABLED) { >> tz->polling_delay = data->polling_delay; >> tz->passive_delay = data->passive_delay; >> @@ -294,11 +280,6 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz, >> tz->passive_delay = 0; >> } >> >> - mutex_unlock(&tz->lock); >> - >> - data->mode = mode; >> - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); >> - >> return 0; >> } >> >> @@ -393,7 +374,6 @@ static int of_thermal_get_crit_temp(struct thermal_zone_device *tz, >> } >> >> static struct thermal_zone_device_ops of_thermal_ops = { >> - .get_mode = of_thermal_get_mode, >> .set_mode = of_thermal_set_mode, >> >> .get_trip_type = of_thermal_get_trip_type, >> @@ -553,8 +533,14 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, >> if (id == sensor_id) { >> tzd = thermal_zone_of_add_sensor(child, sensor_np, >> data, ops); >> - if (!IS_ERR(tzd)) >> + if (!IS_ERR(tzd)) { >> + mutex_lock(&tzd->lock); >> tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED); >> + tzd->mode = THERMAL_DEVICE_ENABLED; >> + mutex_unlock(&tzd->lock); >> + thermal_zone_device_update(tzd, >> + THERMAL_EVENT_UNSPECIFIED); >> + } > > This should use thermal_zone_device_enable() instead of open-coding it: > > if (!IS_ERR(tzd)) > thermal_zone_device_enable(tzd); > > Also because of of_thermal_set_mode() modifications following of/thermal > device drivers (which use ->set_mode directly): > > drivers/thermal/hisi_thermal.c: tzd->ops->set_mode(tzd, > drivers/thermal/rockchip_thermal.c: tzd->ops->set_mode(tzd, > drivers/thermal/sprd_thermal.c: tzd->ops->set_mode(tzd, > > need to be converted to use thermal_zone_device_{en,dis}able() instead. > >> of_node_put(child); >> goto exit; >> @@ -979,7 +965,6 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) >> >> finish: >> of_node_put(child); >> - tz->mode = THERMAL_DEVICE_DISABLED; >> >> return tz; >> >> @@ -1120,6 +1105,7 @@ int __init of_parse_thermal_zones(void) >> /* these two are left for temperature drivers to use */ >> tzp->slope = tz->slope; >> tzp->offset = tz->offset; >> + tzp->initial_mode = THERMAL_DEVICE_DISABLED; >> >> zone = thermal_zone_device_register(child->name, tz->ntrips, >> mask, tz, >> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c >> index c06550930979..5ff98fcc0f6a 100644 >> --- a/drivers/thermal/thermal_core.c >> +++ b/drivers/thermal/thermal_core.c >> @@ -463,6 +463,43 @@ static void thermal_zone_device_reset(struct thermal_zone_device *tz) >> thermal_zone_device_init(tz); >> } >> >> +enum thermal_device_mode >> +thermal_zone_device_get_mode(struct thermal_zone_device *tz) >> +{ >> + enum thermal_device_mode mode; >> + >> + mutex_lock(&tz->lock); >> + >> + mode = tz->mode; >> + >> + mutex_unlock(&tz->lock); >> + >> + return mode; >> +} >> + >> +int thermal_zone_device_set_mode(struct thermal_zone_device *tz, >> + enum thermal_device_mode mode) >> +{ >> + int ret = 0; >> + >> + if (mode != THERMAL_DEVICE_DISABLED && >> + mode != THERMAL_DEVICE_ENABLED) >> + return -EINVAL; > > No need for these checks as enum thermal_device_mode has only two > possible values (we don't do "defensive coding" in the kernel). > >> + mutex_lock(&tz->lock); >> + >> + if (tz->ops->set_mode) >> + ret = tz->ops->set_mode(tz, mode); >> + >> + tz->mode = mode; >> + >> + mutex_unlock(&tz->lock); >> + >> + thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); >> + >> + return ret; >> +} > > Please add: > > EXPORT_SYMBOL_GPL(thermal_zone_device_set_mode); > > here so device drivers can use thermal_zone_device_{en,dis}able(). > >> void thermal_zone_device_update(struct thermal_zone_device *tz, >> enum thermal_notify_event event) >> { >> @@ -1344,6 +1381,9 @@ thermal_zone_device_register(const char *type, int trips, int mask, >> if (atomic_cmpxchg(&tz->need_update, 1, 0)) >> thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); >> >> + if (tzp) >> + thermal_zone_device_set_mode(tz, tzp->initial_mode); > > Please note that currently some device drivers don't access ->mode > at all so their default ->mode value is THERMAL_DEVICE_DISABLED but > in reality they are enabled (IOW after changes in this patch they > should default to THERMAL_DEVICE_ENABLED to reflect real device state > in "mode" sysfs attribue and not break after applying patch #2). > > Therefore we should always call thermal_zone_device_set_mode() and > use the default value of THERMAL_DEVICE_ENABLED if tzp is NULL: > > mode = tzp ? tzp->initial_mode : THERMAL_DEVICE_ENABLED; > thermal_zone_device_set_mode(tz, mode); > > This would also simplify the patch further: > > * setting tz->need_update and calling thermal_zone_device_update() > directly can be removed from thermal_zone_device_register() > > * tzp-s with only .initial_mode = THERMAL_DEVICE_ENABLED set can > be removed from: > > drivers/thermal/acpi.c > drivers/net/ethernet/mellanox/mlxsw/core_thermal.c > drivers/thermal/da9062-thermal.c > drivers/thermal/imx_thermal.c > > The rest of the patch looks fine, thank you for working on this! Oops, one more thing, I've audited all existing users of thermal_zone_device_register() with non-NULL tzp argument: * following drivers should be updated to set .initial_mode to THERMAL_DEVICE_ENABLED in their tzp instance (no ->set_mode method and ->mode not set currently during initialization): drivers/thermal/intel/int340x_thermal/int340x_thermal_zone.c drivers/thermal/intel/x86_pkg_temp_thermal.c * following driver may be updated to explicitly set .initial_mode to THERMAL_DEVICE_DISABLED in its tzp instance (has ->set_mode method but ->mode not set currently during initialization): drivers/thermal/intel/int340x_thermal/int3400_thermal.c (THERMAL_DEVICE_DISABLED == 0 is the default currently but lets document the behavior and prepare driver for potentially changing the default into more sane THERMAL_DEVICE_ENABLED in the future) Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics > Best regards, > -- > Bartlomiej Zolnierkiewicz > Samsung R&D Institute Poland > Samsung Electronics > >> + >> return tz; >> >> unregister: >> @@ -1473,9 +1513,7 @@ static int thermal_pm_notify(struct notifier_block *nb, >> case PM_POST_SUSPEND: >> atomic_set(&in_suspend, 0); >> list_for_each_entry(tz, &thermal_tz_list, node) { >> - tz_mode = THERMAL_DEVICE_ENABLED; >> - if (tz->ops->get_mode) >> - tz->ops->get_mode(tz, &tz_mode); >> + tz_mode = thermal_zone_device_get_mode(tz); >> >> if (tz_mode == THERMAL_DEVICE_DISABLED) >> continue; >> diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h >> index c95689586e19..8e561bac3133 100644 >> --- a/drivers/thermal/thermal_core.h >> +++ b/drivers/thermal/thermal_core.h >> @@ -141,6 +141,22 @@ thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev, >> unsigned long new_state) {} >> #endif /* CONFIG_THERMAL_STATISTICS */ >> >> +enum thermal_device_mode >> +thermal_zone_device_get_mode(struct thermal_zone_device *tz); >> + >> +int thermal_zone_device_set_mode(struct thermal_zone_device *tz, >> + enum thermal_device_mode mode); >> + >> +static inline int thermal_zone_device_enable(struct thermal_zone_device *tz) >> +{ >> + return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_ENABLED); >> +} >> + >> +static inline int thermal_zone_device_disable(struct thermal_zone_device *tz) >> +{ >> + return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_DISABLED); >> +} >> + >> /* device tree support */ >> #ifdef CONFIG_THERMAL_OF >> int of_parse_thermal_zones(void); >> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c >> index aa99edb4dff7..cbb27b3c96d2 100644 >> --- a/drivers/thermal/thermal_sysfs.c >> +++ b/drivers/thermal/thermal_sysfs.c >> @@ -50,14 +50,8 @@ mode_show(struct device *dev, struct device_attribute *attr, char *buf) >> { >> struct thermal_zone_device *tz = to_thermal_zone(dev); >> enum thermal_device_mode mode; >> - int result; >> - >> - if (!tz->ops->get_mode) >> - return -EPERM; >> >> - result = tz->ops->get_mode(tz, &mode); >> - if (result) >> - return result; >> + mode = thermal_zone_device_get_mode(tz); >> >> return sprintf(buf, "%s\n", mode == THERMAL_DEVICE_ENABLED ? "enabled" >> : "disabled"); >> @@ -74,9 +68,9 @@ mode_store(struct device *dev, struct device_attribute *attr, >> return -EPERM; >> >> if (!strncmp(buf, "enabled", sizeof("enabled") - 1)) >> - result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED); >> + result = thermal_zone_device_enable(tz); >> else if (!strncmp(buf, "disabled", sizeof("disabled") - 1)) >> - result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED); >> + result = thermal_zone_device_disable(tz); >> else >> result = -EINVAL; >> >> @@ -428,30 +422,13 @@ static struct attribute_group thermal_zone_attribute_group = { >> .attrs = thermal_zone_dev_attrs, >> }; >> >> -/* We expose mode only if .get_mode is present */ >> static struct attribute *thermal_zone_mode_attrs[] = { >> &dev_attr_mode.attr, >> NULL, >> }; >> >> -static umode_t thermal_zone_mode_is_visible(struct kobject *kobj, >> - struct attribute *attr, >> - int attrno) >> -{ >> - struct device *dev = container_of(kobj, struct device, kobj); >> - struct thermal_zone_device *tz; >> - >> - tz = container_of(dev, struct thermal_zone_device, device); >> - >> - if (tz->ops->get_mode) >> - return attr->mode; >> - >> - return 0; >> -} >> - >> static struct attribute_group thermal_zone_mode_attribute_group = { >> .attrs = thermal_zone_mode_attrs, >> - .is_visible = thermal_zone_mode_is_visible, >> }; >> >> /* We expose passive only if passive trips are present */ >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h >> index 216185bb3014..da4141697e2e 100644 >> --- a/include/linux/thermal.h >> +++ b/include/linux/thermal.h >> @@ -76,8 +76,6 @@ struct thermal_zone_device_ops { >> struct thermal_cooling_device *); >> int (*get_temp) (struct thermal_zone_device *, int *); >> int (*set_trips) (struct thermal_zone_device *, int, int); >> - int (*get_mode) (struct thermal_zone_device *, >> - enum thermal_device_mode *); >> int (*set_mode) (struct thermal_zone_device *, >> enum thermal_device_mode); >> int (*get_trip_type) (struct thermal_zone_device *, int, >> @@ -128,6 +126,7 @@ struct thermal_cooling_device { >> * @trip_temp_attrs: attributes for trip points for sysfs: trip temperature >> * @trip_type_attrs: attributes for trip points for sysfs: trip type >> * @trip_hyst_attrs: attributes for trip points for sysfs: trip hysteresis >> + * @mode: current mode of this thermal zone >> * @devdata: private pointer for device private data >> * @trips: number of trip points the thermal zone supports >> * @trips_disabled; bitmap for disabled trips >> @@ -170,6 +169,7 @@ struct thermal_zone_device { >> struct thermal_attr *trip_temp_attrs; >> struct thermal_attr *trip_type_attrs; >> struct thermal_attr *trip_hyst_attrs; >> + enum thermal_device_mode mode; >> void *devdata; >> int trips; >> unsigned long trips_disabled; /* bitmap for disabled trips */ >> @@ -264,6 +264,9 @@ struct thermal_zone_params { >> int num_tbps; /* Number of tbp entries */ >> struct thermal_bind_params *tbp; >> >> + /* Initial mode of this thermal zone device */ >> + enum thermal_device_mode initial_mode; >> + >> /* >> * Sustainable power (heat) that this thermal zone can dissipate in >> * mW >>
17. April 2020 18:20, "Andrzej Pietrasiewicz" <andrzej.p@collabora.com> schrieb: > Thermal zone devices' mode is stored in individual drivers. This patch > changes it so that mode is stored in struct thermal_zone_device instead. > > As a result all driver-specific variables storing the mode are not needed > and are removed. Consequently, the get_mode() implementations have nothing > to operate on and need to be removed, too. > > Some thermal framework specific functions are introduced: > > thermal_zone_device_get_mode() > thermal_zone_device_set_mode() > thermal_zone_device_enable() > thermal_zone_device_disable() > > thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock > and the "set" calls driver's set_mode() if provided, so the latter must > not take this lock again. At the end of the "set" > thermal_zone_device_update() is called so drivers don't need to repeat this > invocation in their specific set_mode() implementations. > > The scope of the above 4 functions is purposedly limited to the thermal > framework and drivers are not supposed to call them. This encapsulation > does not fully work at the moment for some drivers, though: > > - platform/x86/acerhdf.c Acked-by: Peter Kaestle <peter@piie.net> [...]
Hi Barlomiej, Thanks for looking into the series. @Daniel can you see below? W dniu 19.04.2020 o 13:38, Bartlomiej Zolnierkiewicz pisze: > > Hi Andrzej, > > On 4/17/20 6:20 PM, Andrzej Pietrasiewicz wrote: >> Thermal zone devices' mode is stored in individual drivers. This patch >> changes it so that mode is stored in struct thermal_zone_device instead. >> >> As a result all driver-specific variables storing the mode are not needed >> and are removed. Consequently, the get_mode() implementations have nothing >> to operate on and need to be removed, too. >> >> Some thermal framework specific functions are introduced: >> >> thermal_zone_device_get_mode() >> thermal_zone_device_set_mode() >> thermal_zone_device_enable() >> thermal_zone_device_disable() >> >> thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock >> and the "set" calls driver's set_mode() if provided, so the latter must >> not take this lock again. At the end of the "set" >> thermal_zone_device_update() is called so drivers don't need to repeat this >> invocation in their specific set_mode() implementations. >> >> The scope of the above 4 functions is purposedly limited to the thermal >> framework and drivers are not supposed to call them. This encapsulation > > This should be true only for thermal_zone_device_{get,set}_mode(). > > thermal_zone_device_{en,dis}able() should be available for device drivers: > > * of/thermal device drivers need to enable thermal device itself > (please refer to my patchset for details) > > * device drivers need to call them on ->suspend and ->resume operations > @Daniel: How does this compare to "Just: thermal_zone_device_get_mode() thermal_zone_device_set_mode() thermal_zone_device_disable() thermal_zone_device_enable() And all of them in drivers/thermal/thermal_core.h". Did I understand you correctly? Andrzej
After 3 revisions of an RFC I'm sending this as a PATCH series.
The first patch makes all the drivers store their mode in struct
thermal_zone_device. Such a move has consequences: driver-specific
variables for storing mode are not necessary. Consequently get_mode()
methods become obsolete. Then sysfs "mode" attribute stops depending
on get_mode() being provided, because it is always provided from now on.
The first patch also introduces the initial mode to be optionally passed
to thermal_zone_device_register().
Given all the groundwork done in patch 1/2 patch 2/2 becomes very simple.
Compared to RFC v3 this series addresses comments from Bartlomiej,
thank you Bartlomiej for your review!
RFCv3..this PATCH:
- export thermal_zone_device_{enable|disable}() for drivers
- don't check provided enum values in acpi's thermal_zet_mode()
and in int3400_thermal_set_mode()
- use thermal_zone_device_enable() in of_thermal instead of open coding it
- use thermal_zone_device_{enable|disable}() in hisi_thermal, rockchip_thermal
and sprd_thermal
- assume THERMAL_DEVICE_ENABLED is thermal_zone_params not provided at
tzd's register time
- eliminated tzp-s which contain only .initial_mode = THERMAL_DEVICE_ENABLED,
- don't set tz->need_update and don't call thermal_zone_device_update()
at the end of thermal_zone_device_register()
- used .initial_mode in int340x_thermal_zone, x86_pkg_temp_thermal and
int3400_thermal
Andrzej Pietrasiewicz (2):
thermal: core: Let thermal zone device's mode be stored in its struct
thermal: core: Stop polling DISABLED thermal devices
drivers/acpi/thermal.c | 35 ++--------
.../ethernet/mellanox/mlxsw/core_thermal.c | 42 ------------
drivers/platform/x86/acerhdf.c | 17 +----
drivers/thermal/da9062-thermal.c | 11 ----
drivers/thermal/hisi_thermal.c | 6 +-
drivers/thermal/imx_thermal.c | 24 ++-----
.../intel/int340x_thermal/int3400_thermal.c | 31 ++-------
.../int340x_thermal/int340x_thermal_zone.c | 1 +
.../thermal/intel/intel_quark_dts_thermal.c | 22 ++-----
drivers/thermal/intel/x86_pkg_temp_thermal.c | 1 +
drivers/thermal/of-thermal.c | 24 +------
drivers/thermal/rockchip_thermal.c | 6 +-
drivers/thermal/sprd_thermal.c | 6 +-
drivers/thermal/thermal_core.c | 65 ++++++++++++++++---
drivers/thermal/thermal_core.h | 3 +
drivers/thermal/thermal_sysfs.c | 29 +--------
include/linux/thermal.h | 22 ++++++-
17 files changed, 121 insertions(+), 224 deletions(-)
base-commit: 79799562bf087b30d9dd0fddf5bed2d3b038be08
@Daniel please see below W dniu 20.04.2020 o 13:03, Andrzej Pietrasiewicz pisze: > Hi Barlomiej, > > Thanks for looking into the series. > > @Daniel can you see below? > > W dniu 19.04.2020 o 13:38, Bartlomiej Zolnierkiewicz pisze: >> >> Hi Andrzej, >> >> On 4/17/20 6:20 PM, Andrzej Pietrasiewicz wrote: >>> Thermal zone devices' mode is stored in individual drivers. This patch >>> changes it so that mode is stored in struct thermal_zone_device instead. >>> >>> As a result all driver-specific variables storing the mode are not needed >>> and are removed. Consequently, the get_mode() implementations have nothing >>> to operate on and need to be removed, too. >>> >>> Some thermal framework specific functions are introduced: >>> >>> thermal_zone_device_get_mode() >>> thermal_zone_device_set_mode() >>> thermal_zone_device_enable() >>> thermal_zone_device_disable() >>> >>> thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock >>> and the "set" calls driver's set_mode() if provided, so the latter must >>> not take this lock again. At the end of the "set" >>> thermal_zone_device_update() is called so drivers don't need to repeat this >>> invocation in their specific set_mode() implementations. >>> >>> The scope of the above 4 functions is purposedly limited to the thermal >>> framework and drivers are not supposed to call them. This encapsulation >> >> This should be true only for thermal_zone_device_{get,set}_mode(). >> >> thermal_zone_device_{en,dis}able() should be available for device drivers: >> >> * of/thermal device drivers need to enable thermal device itself >> (please refer to my patchset for details) >> >> * device drivers need to call them on ->suspend and ->resume operations >> > > @Daniel: > > How does this compare to > > "Just: > > thermal_zone_device_get_mode() > thermal_zone_device_set_mode() > thermal_zone_device_disable() > thermal_zone_device_enable() > > And all of them in drivers/thermal/thermal_core.h". Did I understand > you correctly? > I sent a PATCH series (rather than next iteration of RFC) addressing Bartlomiej's comments. They make sense to me. Regards, Andrzej
Hi Andrzej, On 17/04/2020 18:20, Andrzej Pietrasiewicz wrote: > Thermal zone devices' mode is stored in individual drivers. This patch > changes it so that mode is stored in struct thermal_zone_device instead. > > As a result all driver-specific variables storing the mode are not needed > and are removed. Consequently, the get_mode() implementations have nothing > to operate on and need to be removed, too. > > Some thermal framework specific functions are introduced: > > thermal_zone_device_get_mode() > thermal_zone_device_set_mode() > thermal_zone_device_enable() > thermal_zone_device_disable() > > thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock > and the "set" calls driver's set_mode() if provided, so the latter must > not take this lock again. At the end of the "set" > thermal_zone_device_update() is called so drivers don't need to repeat this > invocation in their specific set_mode() implementations. > > The scope of the above 4 functions is purposedly limited to the thermal > framework and drivers are not supposed to call them. This encapsulation > does not fully work at the moment for some drivers, though: > > - platform/x86/acerhdf.c > - drivers/thermal/imx_thermal.c > - drivers/thermal/intel/intel_quark_dts_thermal.c > - drivers/thermal/of-thermal.c > > and they manipulate struct thermal_zone_device's members directly. > > struct thermal_zone_params gains a new member called initial_mode, which > is used to set tzd's mode at registration time. > > The sysfs "mode" attribute is always exposed from now on, because all > thermal zone devices now have their get_mode() implemented at the generic > level and it is always available. Exposing "mode" doesn't hurt the drivers > which don't provide their own set_mode(), because writing to "mode" will > result in -EPERM, as expected. The result is great, that is a nice cleanup of the thermal framework. After review it appears there are still problems IMO, especially with the suspend / resume path. The patch is big, it is a bit complex to comment. I suggest to re-org the changes as following: - patch 1 : Add the four functions: * thermal_zone_device_set_mode() * thermal_zone_device_enable() * thermal_zone_device_disable() * thermal_zone_device_is_enabled() *but* do not export thermal_zone_device_set_mode(), it must stay private to the thermal framework ATM. - patch 2 : Add the mode THERMAL_DEVICE_SUSPENDED In the thermal_pm_notify() in the: - PM_SUSPEND_PREPARE case, set the mode to THERMAL_DEVICE_SUSPENDED if the mode is THERMAL_DEVICE_ENABLED - PM_POST_SUSPEND case, set the mode to THERMAL_DEVICE_ENABLED, if the mode is THERMAL_DEVICE_SUSPENDED - patch 3 : Change the monitor function Change monitor_thermal_zone() function to set the polling to zero if the mode is THERMAL_DEVICE_DISABLED - patch 4 : Do the changes to remove get_mode() ops Make sure there is no access to tz->mode from the drivers anymore but use of the functions of patch 1. IMO, this is the tricky part because a part of the drivers are not calling the update after setting the mode while the function thermal_zone_device_enable()/disable() call update via the thermal_zone_device_set_mode(), so we must be sure to not break anything. - patch 5 : Do the changes to remove set_mode() ops users As the patch 3 sets the polling to zero, the routine in the driver setting the polling to zero is no longer needed (eg. in the mellanox driver). I expect int300 to be the last user of this ops, hopefully we can find a way to get rid of the specific call done inside and then remove the ops. The initial_mode approach looks hackish, I suggest to make the default the thermal zone disabled after creating and then explicitly enable it. Note that is what do a lot of drivers already. Hopefully, these changes are git-bisect safe. Does it make sense ?
Hi Daniel, W dniu 23.05.2020 o 23:24, Daniel Lezcano pisze: > Hi Andrzej, > > On 17/04/2020 18:20, Andrzej Pietrasiewicz wrote: >> Thermal zone devices' mode is stored in individual drivers. This patch >> changes it so that mode is stored in struct thermal_zone_device instead. >> >> As a result all driver-specific variables storing the mode are not needed >> and are removed. Consequently, the get_mode() implementations have nothing >> to operate on and need to be removed, too. >> >> Some thermal framework specific functions are introduced: >> >> thermal_zone_device_get_mode() >> thermal_zone_device_set_mode() >> thermal_zone_device_enable() >> thermal_zone_device_disable() >> >> thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock >> and the "set" calls driver's set_mode() if provided, so the latter must >> not take this lock again. At the end of the "set" >> thermal_zone_device_update() is called so drivers don't need to repeat this >> invocation in their specific set_mode() implementations. >> >> The scope of the above 4 functions is purposedly limited to the thermal >> framework and drivers are not supposed to call them. This encapsulation >> does not fully work at the moment for some drivers, though: >> >> - platform/x86/acerhdf.c >> - drivers/thermal/imx_thermal.c >> - drivers/thermal/intel/intel_quark_dts_thermal.c >> - drivers/thermal/of-thermal.c >> >> and they manipulate struct thermal_zone_device's members directly. >> >> struct thermal_zone_params gains a new member called initial_mode, which >> is used to set tzd's mode at registration time. >> >> The sysfs "mode" attribute is always exposed from now on, because all >> thermal zone devices now have their get_mode() implemented at the generic >> level and it is always available. Exposing "mode" doesn't hurt the drivers >> which don't provide their own set_mode(), because writing to "mode" will >> result in -EPERM, as expected. > > The result is great, that is a nice cleanup of the thermal framework. > > After review it appears there are still problems IMO, especially with > the suspend / resume path. The patch is big, it is a bit complex to > comment. I suggest to re-org the changes as following: > > - patch 1 : Add the four functions: > > * thermal_zone_device_set_mode() > * thermal_zone_device_enable() > * thermal_zone_device_disable() > * thermal_zone_device_is_enabled() > > *but* do not export thermal_zone_device_set_mode(), it must stay private > to the thermal framework ATM. Not exporting thermal_zone_device_set_mode() implies not exporting thermal_zone_device_enable()/thermal_zone_device_disable() because they are implemented in terms of the former. Or do you have a different idea? > > - patch 2 : Add the mode THERMAL_DEVICE_SUSPENDED > > In the thermal_pm_notify() in the: > > - PM_SUSPEND_PREPARE case, set the mode to THERMAL_DEVICE_SUSPENDED if > the mode is THERMAL_DEVICE_ENABLED > > - PM_POST_SUSPEND case, set the mode to THERMAL_DEVICE_ENABLED, if the > mode is THERMAL_DEVICE_SUSPENDED > > - patch 3 : Change the monitor function > > Change monitor_thermal_zone() function to set the polling to zero if the > mode is THERMAL_DEVICE_DISABLED So we assume this: if a driver creates a tz which is initially ENABLED, it will be polled. If a driver creates a tz which is initially DISABLED (which is what you suggest the drivers should be doing, but not all of them do), it won't be polled unless the driver explicitly enables its tz. Am I concluding right that a suspended device will remain polled? Is it ok? > > - patch 4 : Do the changes to remove get_mode() ops > > Make sure there is no access to tz->mode from the drivers anymore but > use of the functions of patch 1. IMO, this is the tricky part because a > part of the drivers are not calling the update after setting the mode > while the function thermal_zone_device_enable()/disable() call update > via the thermal_zone_device_set_mode(), so we must be sure to not break > anything. Ah, I guess now is the time to make the functions from patch 1 exported? Ensuring no driver accesses tz->mode directly might be tricky, indeed. If it can be shown that calling the update doesn't hurt a particular driver, it can be converted to use the helpers instead of manipulating tz->mode directly. If, however, it does make a difference then it all depends and getting rid of accessing tz->mode directly might require help from the respective maintainers. This also calls for storing tz's mode in struct thermal_zone_device rather than in individual drivers. In fact it seems the purpose of ->get_mode() is to produce the state stored internally in drivers. Removing ->get_mode() requires changing the place where the state is stored. struct thermal_zone_device seems most appropriate. So this patch is not going to be small. Once we start storing tz's state in struct thermal_zone_device the ->set_mode() implementations need to be changed, too. To me it seems awkward to split this change in two patches: if we keep the changes split then in patch 4 we need to introduce code which implements ->set_mode() in terms of the new state location, only to remove it in the very next patch. While we are at it some drivers, namely acpi/thermal and int3400 store their mode in an int rather than enum thermal_device_mode. So maybe changing this should go even before patch 4? acerhdf does not store its mode at all and on top of it it wants to manipulate the polling delay directly and it has a module parameter which specifies it. > > - patch 5 : Do the changes to remove set_mode() ops users > > As the patch 3 sets the polling to zero, the routine in the driver > setting the polling to zero is no longer needed (eg. in the mellanox > driver). I expect int300 to be the last user of this ops, hopefully we > can find a way to get rid of the specific call done inside and then > remove the ops. acerhdf wants ->set_mode() desperately. > > The initial_mode approach looks hackish, I suggest to make the default > the thermal zone disabled after creating and then explicitly enable it. Is it needed in drivers which create their thermal zone enabled? Regards, Andrzej
On 25/05/2020 21:35, Andrzej Pietrasiewicz wrote: > Hi Daniel, > > W dniu 23.05.2020 o 23:24, Daniel Lezcano pisze: >> Hi Andrzej, >> >> On 17/04/2020 18:20, Andrzej Pietrasiewicz wrote: >>> Thermal zone devices' mode is stored in individual drivers. This patch >>> changes it so that mode is stored in struct thermal_zone_device instead. >>> >>> As a result all driver-specific variables storing the mode are not >>> needed >>> and are removed. Consequently, the get_mode() implementations have >>> nothing >>> to operate on and need to be removed, too. >>> >>> Some thermal framework specific functions are introduced: >>> >>> thermal_zone_device_get_mode() >>> thermal_zone_device_set_mode() >>> thermal_zone_device_enable() >>> thermal_zone_device_disable() >>> >>> thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock >>> and the "set" calls driver's set_mode() if provided, so the latter must >>> not take this lock again. At the end of the "set" >>> thermal_zone_device_update() is called so drivers don't need to >>> repeat this >>> invocation in their specific set_mode() implementations. >>> >>> The scope of the above 4 functions is purposedly limited to the thermal >>> framework and drivers are not supposed to call them. This encapsulation >>> does not fully work at the moment for some drivers, though: >>> >>> - platform/x86/acerhdf.c >>> - drivers/thermal/imx_thermal.c >>> - drivers/thermal/intel/intel_quark_dts_thermal.c >>> - drivers/thermal/of-thermal.c >>> >>> and they manipulate struct thermal_zone_device's members directly. >>> >>> struct thermal_zone_params gains a new member called initial_mode, which >>> is used to set tzd's mode at registration time. >>> >>> The sysfs "mode" attribute is always exposed from now on, because all >>> thermal zone devices now have their get_mode() implemented at the >>> generic >>> level and it is always available. Exposing "mode" doesn't hurt the >>> drivers >>> which don't provide their own set_mode(), because writing to "mode" will >>> result in -EPERM, as expected. >> >> The result is great, that is a nice cleanup of the thermal framework. >> >> After review it appears there are still problems IMO, especially with >> the suspend / resume path. The patch is big, it is a bit complex to >> comment. I suggest to re-org the changes as following: >> >> - patch 1 : Add the four functions: >> >> * thermal_zone_device_set_mode() >> * thermal_zone_device_enable() >> * thermal_zone_device_disable() >> * thermal_zone_device_is_enabled() >> >> *but* do not export thermal_zone_device_set_mode(), it must stay private >> to the thermal framework ATM. > > Not exporting thermal_zone_device_set_mode() implies not exporting > thermal_zone_device_enable()/thermal_zone_device_disable() because they > are implemented in terms of the former. Or do you have a different idea? I meant no inline for them but as below: in .h extern int thermal_zone_device_enable(); extern int thermal_zone_device_disable(); extern int thermal_zone_device_is_enabled(); in .c static int thermal_zone_device_set_mode() { ... } int thermal_zone_device_enable() { thermal_zone_device_set_mode(); } EXPORT_SYMBOL_GPL(thermal_zone_device_enable); >> - patch 2 : Add the mode THERMAL_DEVICE_SUSPENDED >> >> In the thermal_pm_notify() in the: >> >> - PM_SUSPEND_PREPARE case, set the mode to THERMAL_DEVICE_SUSPENDED if >> the mode is THERMAL_DEVICE_ENABLED >> >> - PM_POST_SUSPEND case, set the mode to THERMAL_DEVICE_ENABLED, if the >> mode is THERMAL_DEVICE_SUSPENDED >> >> - patch 3 : Change the monitor function >> >> Change monitor_thermal_zone() function to set the polling to zero if the >> mode is THERMAL_DEVICE_DISABLED > > So we assume this: if a driver creates a tz which is initially ENABLED, > it will be polled. If a driver creates a tz which is initially DISABLED > (which is what you suggest the drivers should be doing, but not all of them > do), it won't be polled unless the driver explicitly enables its tz. Yes. > Am I concluding right that a suspended device will remain polled? Is it ok? Actually it is not ok but AFAICT, it is the current behavior. The polling do not stop but the 'in_suspend' prevent an update. I thought we can post-pone the suspend case later when the ENABLED/DISABLED changes are consolidated, so SUSPENDED will act as DISABLED. >> - patch 4 : Do the changes to remove get_mode() ops >> >> Make sure there is no access to tz->mode from the drivers anymore but >> use of the functions of patch 1. IMO, this is the tricky part because a >> part of the drivers are not calling the update after setting the mode >> while the function thermal_zone_device_enable()/disable() call update >> via the thermal_zone_device_set_mode(), so we must be sure to not break >> anything. > > Ah, I guess now is the time to make the functions from patch 1 exported? Yes :) > Ensuring no driver accesses tz->mode directly might be tricky, indeed. > If it can be shown that calling the update doesn't hurt a particular > driver, > it can be converted to use the helpers instead of manipulating tz->mode > directly. If, however, it does make a difference then it all depends and > getting rid of accessing tz->mode directly might require help from the > respective maintainers. Agree. > This also calls for storing tz's mode in struct thermal_zone_device > rather than in individual drivers. In fact it seems the purpose > of ->get_mode() is to produce the state stored internally in drivers. > Removing ->get_mode() requires changing the place where the state is > stored. struct thermal_zone_device seems most appropriate. So this patch > is not going to be small. Yes, the patch can be big. It is fine if the changes are only to replace tz->mode by their respective disable/enable/is_enabled functions. More complex changes can be separate. > Once we start storing tz's state in struct thermal_zone_device the > ->set_mode() implementations need to be changed, too. To me it seems > awkward to split this change in two patches: if we keep the changes > split then in patch 4 we need to introduce code which implements > ->set_mode() in terms of the new state location, only to remove it > in the very next patch. Yes, it is a valid point. May be you can do the changes in two patches to see the results in terms of complexity for the review process, then decide if it is worth to merge them before sending. > While we are at it some drivers, namely acpi/thermal and int3400 store > their mode in an int rather than enum thermal_device_mode. So maybe > changing this should go even before patch 4? I agree. > acerhdf does not store > its mode at all and on top of it it wants to manipulate the polling > delay directly and it has a module parameter which specifies it. >> - patch 5 : Do the changes to remove set_mode() ops users >> >> As the patch 3 sets the polling to zero, the routine in the driver >> setting the polling to zero is no longer needed (eg. in the mellanox >> driver). I expect int300 to be the last user of this ops, hopefully we >> can find a way to get rid of the specific call done inside and then >> remove the ops. > > acerhdf wants ->set_mode() desperately. Yes, there is a couple of drivers which requires for the moment to keep the ops->set_mode to be called: int3400 and acerhdf. Both of them will be greatly simplified with the DISABLED / ENABLED changes. >> The initial_mode approach looks hackish, I suggest to make the default >> the thermal zone disabled after creating and then explicitly enable it. > > Is it needed in drivers which create their thermal zone enabled? IMO, yes. We are doing changes with a code prone to issues, so making the steps: creation + enable will make things more clear. For instance, the clk framework do the same.
Hi Daniel, W dniu 26.05.2020 o 00:08, Daniel Lezcano pisze: > On 25/05/2020 21:35, Andrzej Pietrasiewicz wrote: >> Hi Daniel, >> >> W dniu 23.05.2020 o 23:24, Daniel Lezcano pisze: >>> Hi Andrzej, >>> >>> On 17/04/2020 18:20, Andrzej Pietrasiewicz wrote: >>>> Thermal zone devices' mode is stored in individual drivers. This patch >>>> changes it so that mode is stored in struct thermal_zone_device instead. >>>> >>>> As a result all driver-specific variables storing the mode are not >>>> needed >>>> and are removed. Consequently, the get_mode() implementations have >>>> nothing >>>> to operate on and need to be removed, too. >>>> >>>> Some thermal framework specific functions are introduced: >>>> >>>> thermal_zone_device_get_mode() >>>> thermal_zone_device_set_mode() >>>> thermal_zone_device_enable() >>>> thermal_zone_device_disable() >>>> >>>> thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock >>>> and the "set" calls driver's set_mode() if provided, so the latter must >>>> not take this lock again. At the end of the "set" >>>> thermal_zone_device_update() is called so drivers don't need to >>>> repeat this >>>> invocation in their specific set_mode() implementations. >>>> >>>> The scope of the above 4 functions is purposedly limited to the thermal >>>> framework and drivers are not supposed to call them. This encapsulation >>>> does not fully work at the moment for some drivers, though: >>>> >>>> - platform/x86/acerhdf.c >>>> - drivers/thermal/imx_thermal.c >>>> - drivers/thermal/intel/intel_quark_dts_thermal.c >>>> - drivers/thermal/of-thermal.c >>>> >>>> and they manipulate struct thermal_zone_device's members directly. >>>> >>>> struct thermal_zone_params gains a new member called initial_mode, which >>>> is used to set tzd's mode at registration time. >>>> >>>> The sysfs "mode" attribute is always exposed from now on, because all >>>> thermal zone devices now have their get_mode() implemented at the >>>> generic >>>> level and it is always available. Exposing "mode" doesn't hurt the >>>> drivers >>>> which don't provide their own set_mode(), because writing to "mode" will >>>> result in -EPERM, as expected. >>> >>> The result is great, that is a nice cleanup of the thermal framework. >>> >>> After review it appears there are still problems IMO, especially with >>> the suspend / resume path. The patch is big, it is a bit complex to >>> comment. I suggest to re-org the changes as following: >>> >>> - patch 1 : Add the four functions: >>> >>> * thermal_zone_device_set_mode() >>> * thermal_zone_device_enable() >>> * thermal_zone_device_disable() >>> * thermal_zone_device_is_enabled() >>> >>> *but* do not export thermal_zone_device_set_mode(), it must stay private >>> to the thermal framework ATM. >> >> Not exporting thermal_zone_device_set_mode() implies not exporting >> thermal_zone_device_enable()/thermal_zone_device_disable() because they >> are implemented in terms of the former. Or do you have a different idea? > > I meant no inline for them but as below: > > in .h > > extern int thermal_zone_device_enable(); > extern int thermal_zone_device_disable(); > extern int thermal_zone_device_is_enabled(); > > in .c > > static int thermal_zone_device_set_mode() > { > ... > } > > int thermal_zone_device_enable() > { > thermal_zone_device_set_mode(); > } > EXPORT_SYMBOL_GPL(thermal_zone_device_enable); > Hmm. I'm trying to proceed according to what you outline, but it doesn't feel the right approach. Let me show you patch 1: drivers/thermal/thermal_core.c: +int thermal_zone_device_set_mode(struct thermal_zone_device *tz, + enum thermal_device_mode mode) +{ + int ret = 0; + + mutex_lock(&tz->lock); + + if (tz->ops->set_mode) + ret = tz->ops->set_mode(tz, mode); + + mutex_unlock(&tz->lock); + + thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); + + return ret; +} + +int thermal_zone_device_enable(struct thermal_zone_device *tz) +{ + return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_ENABLED); +} +EXPORT_SYMBOL(thermal_zone_device_enable); + +int thermal_zone_device_disable(struct thermal_zone_device *tz) +{ + return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_DISABLED); +} +EXPORT_SYMBOL(thermal_zone_device_disable); + +int thermal_zone_device_is_enabled(struct thermal_zone_device *tz) +{ + enum thermal_device_mode mode = THERMAL_DEVICE_ENABLED; + int ret; + + mutex_lock(&tz->lock); + + if (tz->ops->get_mode) + ret = tz->ops->get_mode(tz, &mode); + + mutex_unlock(&tz->lock); + + return mode == THERMAL_DEVICE_ENABLED; +} +EXPORT_SYMBOL(thermal_zone_device_is_enabled); + plus prototypes of all except set_mode in include/linux/thermal.h I can see 2 problems: 1) we add unused code to the kernel (perhaps this is ok if patches in this series will start using it) 2) tzd *does not* store mode. These are drivers that provide mode access with get_mode()/set_mode() - if they implement them. What if they don't? If thermal_zone_device_set_mode() is introduced now its effect depends on whether a driver implements set_mode() or not. If it doesn't then despite thermal_zone_device_set_mode() being invoked the mode is not changed. Then what does thermal_zone_device_is_enabled() mean? We don't know, because we don't know what the effect of thermal_zone_device_set_mode() is in the first place. And if the driver does not provide get_mode() then thermal_zone_device_is_enabled() always returns "enabled". Adding these functions now without mode being stored in tzd seems awkward to me. Consequently, IMO the first thing to do is make tzd store device's mode and this way be independent from whether drivers implement or not implement get_mode()/set_mode(). > >>> - patch 2 : Add the mode THERMAL_DEVICE_SUSPENDED >>> >>> In the thermal_pm_notify() in the: >>> >>> - PM_SUSPEND_PREPARE case, set the mode to THERMAL_DEVICE_SUSPENDED if >>> the mode is THERMAL_DEVICE_ENABLED >>> >>> - PM_POST_SUSPEND case, set the mode to THERMAL_DEVICE_ENABLED, if the >>> mode is THERMAL_DEVICE_SUSPENDED >>> drivers/thermal/thermal_core.c: case PM_HIBERNATION_PREPARE: case PM_RESTORE_PREPARE: case PM_SUSPEND_PREPARE: + list_for_each_entry(tz, &thermal_tz_list, node) { + tz_mode = THERMAL_DEVICE_ENABLED; + if (tz->ops->get_mode) + tz->ops->get_mode(tz, &tz_mode); + if (tz_mode == THERMAL_DEVICE_ENABLED) + thermal_zone_device_set_mode(tz, + THERMAL_DEVICE_SUSPENDED); + } atomic_set(&in_suspend, 1); break; case PM_POST_HIBERNATION: @@ -1530,6 +1538,9 @@ static int thermal_pm_notify(struct notifier_block *nb, tz_mode = THERMAL_DEVICE_ENABLED; if (tz->ops->get_mode) tz->ops->get_mode(tz, &tz_mode); if (tz_mode == THERMAL_DEVICE_DISABLED) continue; + if (tz_mode == THERMAL_DEVICE_SUSPENDED) + thermal_zone_device_set_mode(tz, + THERMAL_DEVICE_ENABLED); include/linux/thermal.h: enum thermal_device_mode { THERMAL_DEVICE_DISABLED = 0, THERMAL_DEVICE_ENABLED, + THERMAL_DEVICE_SUSPENDED, We don't know if set_mode() was effective in PM_SUSPEND_PREPARE_PATH. If it wasn't then instead of being SUSPENDED the device is still ENABLED. If still enabled it doesn't need enabling so the last "if" does the right thing, but does not feel right. >>> - patch 3 : Change the monitor function >>> >>> Change monitor_thermal_zone() function to set the polling to zero if the >>> mode is THERMAL_DEVICE_DISABLED >> >> So we assume this: if a driver creates a tz which is initially ENABLED, >> it will be polled. If a driver creates a tz which is initially DISABLED >> (which is what you suggest the drivers should be doing, but not all of them >> do), it won't be polled unless the driver explicitly enables its tz. > > Yes. > >> Am I concluding right that a suspended device will remain polled? Is it ok? > > Actually it is not ok but AFAICT, it is the current behavior. The > polling do not stop but the 'in_suspend' prevent an update. I thought we > can post-pone the suspend case later when the ENABLED/DISABLED changes > are consolidated, so SUSPENDED will act as DISABLED. > drivers/thermal/thermal_core.c: static void monitor_thermal_zone(struct thermal_zone_device *tz) { + enum thermal_device_mode tz_mode = THERMAL_DEVICE_ENABLED; + mutex_lock(&tz->lock); - if (tz->passive) + if (tz->ops->get_mode) + tz->ops->get_mode(tz, &tz_mode); + + if (tz->passive && mode != THERMAL_DEVICE_DISABLED) thermal_zone_device_set_polling(tz, tz->passive_delay); - else if (tz->polling_delay) + else if (tz->polling_delay && mode != THERMAL_DEVICE_DISABLED) thermal_zone_device_set_polling(tz, tz->polling_delay); else thermal_zone_device_set_polling(tz, 0); If the driver does not implement get_mode() then we assume ENABLED. What if it is actually DISABLED? How does this depend on patch 1 or patch 2? >>> - patch 4 : Do the changes to remove get_mode() ops >>> >>> Make sure there is no access to tz->mode from the drivers anymore but >>> use of the functions of patch 1. IMO, this is the tricky part because a >>> part of the drivers are not calling the update after setting the mode >>> while the function thermal_zone_device_enable()/disable() call update >>> via the thermal_zone_device_set_mode(), so we must be sure to not break >>> anything. >> I haven't started this yet, but again it seems to me that drivers need to start storing their mode in tzd->mode in the first place So what I envision is this: 1) Make all drivers store their state still locally, but using the enum (not all of them do) 2) Once all drivers store their state in the enum, store the enum in struct tzd instead of locally in drivers. This makes get_mode() driver op redundant, but if you prefer more granularity removing it might be done in a separate patch (at the expense of modifying it now to use tzd's member instead of driver-local variable). This also impacts set_mode() ops, because they need to actually change tzd's member instead of some driver-level variable. Changing set_mode() IMO needs to be done in one go. 3) Remove get_mode() driver op altogether, as the mode is stored in struct tzd. 4) patch 1 you outlined - set_mode() and is_enabled() will now operate on tzd's members, so their effect does not depend on drivers implementing or not implementing set_mode(). These effects don't depend on get_mode() any more because of 3). set_mode() would still be calling the set_mode() op in drivers before modifying tzd->mode. 5) patch 2 you outlined - but it can't use thermal_zone_device_set_mode(), because if it gets and then changes tzd's mode, it must do so under tzd->lock. This is ok as this is the very implementation of thermal_core, so accessing "private" members of tzd is a valid approach here. 6) patch 3 you outlined - now it makes much more sense to query tzd's member for mode instead of relying on drivers implementing get_mode(). 7) patch 4 you outlined but under a different name, because get_mode() is already gone. The guts of the patch should be doing what you wrote, though, which is use the helpers instead of directly modifying tzd's members in drivers. 8) patch 5 you outlined - perhaps under a different name, but still doing the same thing: removing portions of code which set polling time to zero in drivers, as that is already being done at tzd's level. This would hopefully make at least some set_mode() implementations no-ops and, consequently, redundant. I can see these drivers: mellanox, part of acerhdf (this driver wants to know when it is becoming enabled/disabled, but the part setting polling can be removed), part of imx (similar situation to that of acerhdf) and of-thermal. After 8) there would be 5 non-empty set_mode() implementations: acpi - but apparently only to print some debug, maybe can be removed acerhdf - no idea if it can live without knowing when the mode changes imx - ditto int3400 - ditto quark - ditto Perhaps after 8) instead of removing set_mode() maybe we should change its name to better reflect its purpose: mode_changing() ? Or maybe even such a change should be a part of 4)? Regards, Andrzej
Hi Daniel, On 5/23/20 11:24 PM, Daniel Lezcano wrote: > Hi Andrzej, > > On 17/04/2020 18:20, Andrzej Pietrasiewicz wrote: >> Thermal zone devices' mode is stored in individual drivers. This patch >> changes it so that mode is stored in struct thermal_zone_device instead. >> >> As a result all driver-specific variables storing the mode are not needed >> and are removed. Consequently, the get_mode() implementations have nothing >> to operate on and need to be removed, too. >> >> Some thermal framework specific functions are introduced: >> >> thermal_zone_device_get_mode() >> thermal_zone_device_set_mode() >> thermal_zone_device_enable() >> thermal_zone_device_disable() >> >> thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock >> and the "set" calls driver's set_mode() if provided, so the latter must >> not take this lock again. At the end of the "set" >> thermal_zone_device_update() is called so drivers don't need to repeat this >> invocation in their specific set_mode() implementations. >> >> The scope of the above 4 functions is purposedly limited to the thermal >> framework and drivers are not supposed to call them. This encapsulation >> does not fully work at the moment for some drivers, though: >> >> - platform/x86/acerhdf.c >> - drivers/thermal/imx_thermal.c >> - drivers/thermal/intel/intel_quark_dts_thermal.c >> - drivers/thermal/of-thermal.c >> >> and they manipulate struct thermal_zone_device's members directly. >> >> struct thermal_zone_params gains a new member called initial_mode, which >> is used to set tzd's mode at registration time. >> >> The sysfs "mode" attribute is always exposed from now on, because all >> thermal zone devices now have their get_mode() implemented at the generic >> level and it is always available. Exposing "mode" doesn't hurt the drivers >> which don't provide their own set_mode(), because writing to "mode" will >> result in -EPERM, as expected. > > The result is great, that is a nice cleanup of the thermal framework. > > After review it appears there are still problems IMO, especially with > the suspend / resume path. The patch is big, it is a bit complex to > comment. I suggest to re-org the changes as following: There are still issues with the related existing thermal code but this patch seems to be a step in the right direction. For the latest version posted ("v3" one, your mail was replied to the older "RFC v3" one): https://lore.kernel.org/linux-pm/20200423165705.13585-2-andrzej.p@collabora.com/ I couldn't find the problems with the patch itself (no new issues being introduced, all changes seem to be improvements over the current situation). Also the patch is not small but it also not that big and it mostly removes the code: 17 files changed, 105 insertions(+), 244 deletions(-) I worry that since the original code is intertwined in the interesting ways the cost of work on splitting the patch on smaller changes may be higher than its benefits. > - patch 1 : Add the four functions: > > * thermal_zone_device_set_mode() > * thermal_zone_device_enable() > * thermal_zone_device_disable() > * thermal_zone_device_is_enabled() > > *but* do not export thermal_zone_device_set_mode(), it must stay private > to the thermal framework ATM. > > - patch 2 : Add the mode THERMAL_DEVICE_SUSPENDED > > In the thermal_pm_notify() in the: > > - PM_SUSPEND_PREPARE case, set the mode to THERMAL_DEVICE_SUSPENDED if > the mode is THERMAL_DEVICE_ENABLED > > - PM_POST_SUSPEND case, set the mode to THERMAL_DEVICE_ENABLED, if the > mode is THERMAL_DEVICE_SUSPENDED > > - patch 3 : Change the monitor function > > Change monitor_thermal_zone() function to set the polling to zero if the > mode is THERMAL_DEVICE_DISABLED > > - patch 4 : Do the changes to remove get_mode() ops > > Make sure there is no access to tz->mode from the drivers anymore but > use of the functions of patch 1. IMO, this is the tricky part because a > part of the drivers are not calling the update after setting the mode > while the function thermal_zone_device_enable()/disable() call update > via the thermal_zone_device_set_mode(), so we must be sure to not break > anything. > > - patch 5 : Do the changes to remove set_mode() ops users > > As the patch 3 sets the polling to zero, the routine in the driver > setting the polling to zero is no longer needed (eg. in the mellanox > driver). I expect int300 to be the last user of this ops, hopefully we > can find a way to get rid of the specific call done inside and then > remove the ops. > > The initial_mode approach looks hackish, I suggest to make the default > the thermal zone disabled after creating and then explicitly enable it. > Note that is what do a lot of drivers already. > > Hopefully, these changes are git-bisect safe. > > Does it make sense ? Besides the requirement to split the patch it seems that the above list contains a lot of problematic areas with the existing thermal code yet to be addressed.. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
There is already a reviewed v3 (not to be confused with RFC v3), which can be considered for merging: https://lore.kernel.org/linux-pm/20200423165705.13585-2-andrzej.p@collabora.com/ Let me cite Bartlomiej Zolnierkiewicz: "I couldn't find the problems with the patch itself (no new issues being introduced, all changes seem to be improvements over the current situation). Also the patch is not small but it also not that big and it mostly removes the code: 17 files changed, 105 insertions(+), 244 deletions(-)" There have been raised some concerns about bisectability and about introducing "initial_mode" member in struct thermal_zone_params. This v4 series addresses those concerns: it takes a more gradual approach and uses explicit tzd state initialization, hence there are more insertions than in v3, and the net effect is -63 lines versus -139 lines in v3. Patch 2/11 converts the 3 drivers which don't store their mode in enum thermal_device_mode to do so. Once that is cleared, struct thermal_zone_device gains its "mode" member (patch 3/11) and then all interested drivers change the location where they store their state: instead of storing it in some variable in a driver, they store it in struct thermal_zone_device (patch 4/11). Patch 4/11 does not introduce other changes. Then get_mode() driver method becomes redundant, and so it is removed (patch 5/11). This is the first part of the groundwork. The second part of the groundwork is to add (patch 6/11) and use (patch 7/11) helpers for accessing tzd's state from drivers. From this moment on the drivers don't access tzd->mode directly. Please note that after patch 4/11 all thermal zone devices have their mode implicitly initialized to DISABLED, as a result of kzalloc and THERMAL_DEVICE_DISABLED == 0. This is not a problem from the point of view of polling them, because their state is not considered when deciding to poll or to cease polling. In preparation for considering tzd's state when deciding to poll or to cease polling it ensured (patch 8/11 and some in patch 7/11) that all the drivers are explicitly initialized with regard to their state. With all that groundwork in place now it makes sense to modify thermal_core so that it stops polling DISABLED devices (patch 9/11), which is the ultimate purpose of this work. While at it, some set_mode() implementations only change the polling variables to make the core stop polling their drivers, but that is now unnecessary and those set_mode() implementations are removed. In other implementations polling variables modifications are removed. Some other set_mode() implementations are simplified or removed (patch 10/11). set_mode() is now only called when tzd's mode is about to change. Actual setting is performed in thermal_core, in thermal_zone_device_set_mode(). The meaning of set_mode() callback is actually to notify the driver about the mode being changed and giving the driver a chance to oppose such a change. To better reflect the purpose of the method it is renamed to change_mode() (patch 11/11). Andrzej Pietrasiewicz (11): acpi: thermal: Fix error handling in the register function thermal: Store thermal mode in a dedicated enum thermal: Add current mode to thermal zone device thermal: Store device mode in struct thermal_zone_device thermal: remove get_mode() operation of drivers thermal: Add mode helpers thermal: Use mode helpers in drivers thermal: Explicitly enable non-changing thermal zone devices thermal: core: Stop polling DISABLED thermal devices thermal: Simplify or eliminate unnecessary set_mode() methods thermal: Rename set_mode() to change_mode() drivers/acpi/thermal.c | 75 +++++---------- .../ethernet/chelsio/cxgb4/cxgb4_thermal.c | 8 ++ .../ethernet/mellanox/mlxsw/core_thermal.c | 91 ++++--------------- drivers/net/wireless/intel/iwlwifi/mvm/tt.c | 9 +- drivers/platform/x86/acerhdf.c | 33 +++---- drivers/platform/x86/intel_mid_thermal.c | 6 ++ drivers/power/supply/power_supply_core.c | 9 +- drivers/thermal/armada_thermal.c | 6 ++ drivers/thermal/da9062-thermal.c | 16 +--- drivers/thermal/dove_thermal.c | 6 ++ drivers/thermal/hisi_thermal.c | 6 +- drivers/thermal/imx_thermal.c | 57 ++++-------- .../intel/int340x_thermal/int3400_thermal.c | 43 +++------ .../int340x_thermal/int340x_thermal_zone.c | 5 + drivers/thermal/intel/intel_pch_thermal.c | 5 + .../thermal/intel/intel_quark_dts_thermal.c | 34 ++----- drivers/thermal/intel/intel_soc_dts_iosf.c | 3 + drivers/thermal/intel/x86_pkg_temp_thermal.c | 6 ++ drivers/thermal/kirkwood_thermal.c | 7 ++ drivers/thermal/rcar_thermal.c | 9 +- drivers/thermal/rockchip_thermal.c | 6 +- drivers/thermal/spear_thermal.c | 7 ++ drivers/thermal/sprd_thermal.c | 6 +- drivers/thermal/st/st_thermal.c | 5 + drivers/thermal/thermal_core.c | 76 ++++++++++++++-- drivers/thermal/thermal_of.c | 51 ++--------- drivers/thermal/thermal_sysfs.c | 37 +------- include/linux/thermal.h | 19 +++- 28 files changed, 289 insertions(+), 352 deletions(-) base-commit: 351f4911a477ae01239c42f771f621d85b06ea10
Hi, 28. Mai 2020 21:21, "Andrzej Pietrasiewicz" <andrzej.p@collabora.com> schrieb: [...] > This v4 series addresses those concerns: it takes a more gradual > approach and uses explicit tzd state initialization, hence there are more > insertions than in v3, and the net effect is -63 lines versus -139 lines > in v3. I'd like to test it. Which git repo / branch do you base this series of patches on? [...] > base-commit: 351f4911a477ae01239c42f771f621d85b06ea10 Can't find this hashref anywhere.
W dniu 01.06.2020 o 12:02, Peter Kästle pisze: > Hi, > > 28. Mai 2020 21:21, "Andrzej Pietrasiewicz" <andrzej.p@collabora.com> schrieb: > > [...] > >> This v4 series addresses those concerns: it takes a more gradual >> approach and uses explicit tzd state initialization, hence there are more >> insertions than in v3, and the net effect is -63 lines versus -139 lines >> in v3. > > I'd like to test it. Which git repo / branch do you base this series of patches on? > > [...] > >> base-commit: 351f4911a477ae01239c42f771f621d85b06ea10 > > Can't find this hashref anywhere. > git://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git, branch "testing". base-commit: 351f4911a477ae01239c42f771f621d85b06ea10
Hi Andrzej, On 28/05/2020 21:20, Andrzej Pietrasiewicz wrote: > There is already a reviewed v3 (not to be confused with RFC v3), which can > be considered for merging: > > https://lore.kernel.org/linux-pm/20200423165705.13585-2-andrzej.p@collabora.com/ > > Let me cite Bartlomiej Zolnierkiewicz: > > "I couldn't find the problems with the patch itself (no new issues > being introduced, all changes seem to be improvements over the current > situation). > > Also the patch is not small but it also not that big and it mostly > removes the code: > > 17 files changed, 105 insertions(+), 244 deletions(-)" Thanks for this nice cleanup. Given the series was tested, reviewed and acked, I would like to merge it as soon as possible. Can you send the V5 with the EXPORT_SYMBOL_GPL fixed ? So the series can enter the integration loop. Thanks -- Daniel
A respin of v4 with these changes: v4..v5: - EXPORT_SYMBOL -> EXPORT_SYMBOL_GPL (Daniel) - dropped unnecessary thermal_zone_device_enable() in int3400_thermal.c and in thermal_of.c (Bartlomiej) Andrzej Pietrasiewicz (11): acpi: thermal: Fix error handling in the register function thermal: Store thermal mode in a dedicated enum thermal: Add current mode to thermal zone device thermal: Store device mode in struct thermal_zone_device thermal: remove get_mode() operation of drivers thermal: Add mode helpers thermal: Use mode helpers in drivers thermal: Explicitly enable non-changing thermal zone devices thermal: core: Stop polling DISABLED thermal devices thermal: Simplify or eliminate unnecessary set_mode() methods thermal: Rename set_mode() to change_mode() drivers/acpi/thermal.c | 75 +++++---------- .../ethernet/chelsio/cxgb4/cxgb4_thermal.c | 8 ++ .../ethernet/mellanox/mlxsw/core_thermal.c | 91 ++++--------------- drivers/net/wireless/intel/iwlwifi/mvm/tt.c | 9 +- drivers/platform/x86/acerhdf.c | 33 +++---- drivers/platform/x86/intel_mid_thermal.c | 6 ++ drivers/power/supply/power_supply_core.c | 9 +- drivers/thermal/armada_thermal.c | 6 ++ drivers/thermal/da9062-thermal.c | 16 +--- drivers/thermal/dove_thermal.c | 6 ++ drivers/thermal/hisi_thermal.c | 6 +- drivers/thermal/imx_thermal.c | 57 ++++-------- .../intel/int340x_thermal/int3400_thermal.c | 38 ++------ .../int340x_thermal/int340x_thermal_zone.c | 5 + drivers/thermal/intel/intel_pch_thermal.c | 5 + .../thermal/intel/intel_quark_dts_thermal.c | 34 ++----- drivers/thermal/intel/intel_soc_dts_iosf.c | 3 + drivers/thermal/intel/x86_pkg_temp_thermal.c | 6 ++ drivers/thermal/kirkwood_thermal.c | 7 ++ drivers/thermal/rcar_thermal.c | 9 +- drivers/thermal/rockchip_thermal.c | 6 +- drivers/thermal/spear_thermal.c | 7 ++ drivers/thermal/sprd_thermal.c | 6 +- drivers/thermal/st/st_thermal.c | 5 + drivers/thermal/thermal_core.c | 76 ++++++++++++++-- drivers/thermal/thermal_of.c | 41 +-------- drivers/thermal/thermal_sysfs.c | 37 +------- include/linux/thermal.h | 19 +++- 28 files changed, 275 insertions(+), 351 deletions(-) base-commit: 48778464bb7d346b47157d21ffde2af6b2d39110
diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c index 19067a5e5293..67bc263332a5 100644 --- a/drivers/acpi/thermal.c +++ b/drivers/acpi/thermal.c @@ -172,7 +172,6 @@ struct acpi_thermal { struct acpi_thermal_trips trips; struct acpi_handle_list devices; struct thermal_zone_device *thermal_zone; - int tz_enabled; int kelvin_offset; /* in millidegrees */ struct work_struct thermal_check_work; }; @@ -500,7 +499,7 @@ static void acpi_thermal_check(void *data) { struct acpi_thermal *tz = data; - if (!tz->tz_enabled) + if (tz->thermal_zone->mode != THERMAL_DEVICE_ENABLED) return; thermal_zone_device_update(tz->thermal_zone, @@ -526,46 +525,29 @@ static int thermal_get_temp(struct thermal_zone_device *thermal, int *temp) return 0; } -static int thermal_get_mode(struct thermal_zone_device *thermal, - enum thermal_device_mode *mode) -{ - struct acpi_thermal *tz = thermal->devdata; - - if (!tz) - return -EINVAL; - - *mode = tz->tz_enabled ? THERMAL_DEVICE_ENABLED : - THERMAL_DEVICE_DISABLED; - - return 0; -} - static int thermal_set_mode(struct thermal_zone_device *thermal, enum thermal_device_mode mode) { struct acpi_thermal *tz = thermal->devdata; - int enable; if (!tz) return -EINVAL; + if (mode != THERMAL_DEVICE_DISABLED && + mode != THERMAL_DEVICE_ENABLED) + return -EINVAL; + /* * enable/disable thermal management from ACPI thermal driver */ - if (mode == THERMAL_DEVICE_ENABLED) - enable = 1; - else if (mode == THERMAL_DEVICE_DISABLED) { - enable = 0; + if (mode == THERMAL_DEVICE_DISABLED) pr_warn("thermal zone will be disabled\n"); - } else - return -EINVAL; - if (enable != tz->tz_enabled) { - tz->tz_enabled = enable; + if (mode != thermal->mode) { ACPI_DEBUG_PRINT((ACPI_DB_INFO, "%s kernel ACPI thermal control\n", - tz->tz_enabled ? "Enable" : "Disable")); - acpi_thermal_check(tz); + mode == THERMAL_DEVICE_ENABLED ? + "Enable" : "Disable")); } return 0; } @@ -856,7 +838,6 @@ static struct thermal_zone_device_ops acpi_thermal_zone_ops = { .bind = acpi_thermal_bind_cooling_device, .unbind = acpi_thermal_unbind_cooling_device, .get_temp = thermal_get_temp, - .get_mode = thermal_get_mode, .set_mode = thermal_set_mode, .get_trip_type = thermal_get_trip_type, .get_trip_temp = thermal_get_trip_temp, @@ -870,6 +851,9 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) int trips = 0; int result; acpi_status status; + struct thermal_zone_params prms = { + .initial_mode = THERMAL_DEVICE_ENABLED, + }; int i; if (tz->trips.critical.flags.valid) @@ -887,13 +871,13 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) if (tz->trips.passive.flags.valid) tz->thermal_zone = thermal_zone_device_register("acpitz", trips, 0, tz, - &acpi_thermal_zone_ops, NULL, + &acpi_thermal_zone_ops, &prms, tz->trips.passive.tsp*100, tz->polling_frequency*100); else tz->thermal_zone = thermal_zone_device_register("acpitz", trips, 0, tz, - &acpi_thermal_zone_ops, NULL, + &acpi_thermal_zone_ops, &prms, 0, tz->polling_frequency*100); if (IS_ERR(tz->thermal_zone)) return -ENODEV; @@ -913,8 +897,6 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz) if (ACPI_FAILURE(status)) return -ENODEV; - tz->tz_enabled = 1; - dev_info(&tz->device->dev, "registered as thermal_zone%d\n", tz->thermal_zone->id); return 0; diff --git a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c index ce0a6837daa3..50518048b86d 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c +++ b/drivers/net/ethernet/mellanox/mlxsw/core_thermal.c @@ -98,7 +98,6 @@ struct mlxsw_thermal_module { struct mlxsw_thermal *parent; struct thermal_zone_device *tzdev; struct mlxsw_thermal_trip trips[MLXSW_THERMAL_NUM_TRIPS]; - enum thermal_device_mode mode; int module; /* Module or gearbox number */ }; @@ -110,7 +109,6 @@ struct mlxsw_thermal { struct thermal_cooling_device *cdevs[MLXSW_MFCR_PWMS_MAX]; u8 cooling_levels[MLXSW_THERMAL_MAX_STATE + 1]; struct mlxsw_thermal_trip trips[MLXSW_THERMAL_NUM_TRIPS]; - enum thermal_device_mode mode; struct mlxsw_thermal_module *tz_module_arr; u8 tz_module_num; struct mlxsw_thermal_module *tz_gearbox_arr; @@ -277,33 +275,16 @@ static int mlxsw_thermal_unbind(struct thermal_zone_device *tzdev, return 0; } -static int mlxsw_thermal_get_mode(struct thermal_zone_device *tzdev, - enum thermal_device_mode *mode) -{ - struct mlxsw_thermal *thermal = tzdev->devdata; - - *mode = thermal->mode; - - return 0; -} - static int mlxsw_thermal_set_mode(struct thermal_zone_device *tzdev, enum thermal_device_mode mode) { struct mlxsw_thermal *thermal = tzdev->devdata; - mutex_lock(&tzdev->lock); - if (mode == THERMAL_DEVICE_ENABLED) tzdev->polling_delay = thermal->polling_delay; else tzdev->polling_delay = 0; - mutex_unlock(&tzdev->lock); - - thermal->mode = mode; - thermal_zone_device_update(tzdev, THERMAL_EVENT_UNSPECIFIED); - return 0; } @@ -407,7 +388,6 @@ static int mlxsw_thermal_trend_get(struct thermal_zone_device *tzdev, static struct thermal_zone_device_ops mlxsw_thermal_ops = { .bind = mlxsw_thermal_bind, .unbind = mlxsw_thermal_unbind, - .get_mode = mlxsw_thermal_get_mode, .set_mode = mlxsw_thermal_set_mode, .get_temp = mlxsw_thermal_get_temp, .get_trip_type = mlxsw_thermal_get_trip_type, @@ -466,34 +446,17 @@ static int mlxsw_thermal_module_unbind(struct thermal_zone_device *tzdev, return err; } -static int mlxsw_thermal_module_mode_get(struct thermal_zone_device *tzdev, - enum thermal_device_mode *mode) -{ - struct mlxsw_thermal_module *tz = tzdev->devdata; - - *mode = tz->mode; - - return 0; -} - static int mlxsw_thermal_module_mode_set(struct thermal_zone_device *tzdev, enum thermal_device_mode mode) { struct mlxsw_thermal_module *tz = tzdev->devdata; struct mlxsw_thermal *thermal = tz->parent; - mutex_lock(&tzdev->lock); - if (mode == THERMAL_DEVICE_ENABLED) tzdev->polling_delay = thermal->polling_delay; else tzdev->polling_delay = 0; - mutex_unlock(&tzdev->lock); - - tz->mode = mode; - thermal_zone_device_update(tzdev, THERMAL_EVENT_UNSPECIFIED); - return 0; } @@ -596,7 +559,6 @@ mlxsw_thermal_module_trip_hyst_set(struct thermal_zone_device *tzdev, int trip, static struct thermal_zone_device_ops mlxsw_thermal_module_ops = { .bind = mlxsw_thermal_module_bind, .unbind = mlxsw_thermal_module_unbind, - .get_mode = mlxsw_thermal_module_mode_get, .set_mode = mlxsw_thermal_module_mode_set, .get_temp = mlxsw_thermal_module_temp_get, .get_trip_type = mlxsw_thermal_module_trip_type_get, @@ -635,7 +597,6 @@ static int mlxsw_thermal_gearbox_temp_get(struct thermal_zone_device *tzdev, static struct thermal_zone_device_ops mlxsw_thermal_gearbox_ops = { .bind = mlxsw_thermal_module_bind, .unbind = mlxsw_thermal_module_unbind, - .get_mode = mlxsw_thermal_module_mode_get, .set_mode = mlxsw_thermal_module_mode_set, .get_temp = mlxsw_thermal_gearbox_temp_get, .get_trip_type = mlxsw_thermal_module_trip_type_get, @@ -749,6 +710,9 @@ static const struct thermal_cooling_device_ops mlxsw_cooling_ops = { static int mlxsw_thermal_module_tz_init(struct mlxsw_thermal_module *module_tz) { + struct thermal_zone_params tzp = { + .initial_mode = THERMAL_DEVICE_ENABLED, + }; char tz_name[MLXSW_THERMAL_ZONE_MAX_NAME]; int err; @@ -759,13 +723,12 @@ mlxsw_thermal_module_tz_init(struct mlxsw_thermal_module *module_tz) MLXSW_THERMAL_TRIP_MASK, module_tz, &mlxsw_thermal_module_ops, - NULL, 0, 0); + &tzp, 0, 0); if (IS_ERR(module_tz->tzdev)) { err = PTR_ERR(module_tz->tzdev); return err; } - module_tz->mode = THERMAL_DEVICE_ENABLED; return 0; } @@ -868,6 +831,9 @@ mlxsw_thermal_modules_fini(struct mlxsw_thermal *thermal) static int mlxsw_thermal_gearbox_tz_init(struct mlxsw_thermal_module *gearbox_tz) { + struct thermal_zone_params tzp = { + .initial_mode = THERMAL_DEVICE_ENABLED, + }; char tz_name[MLXSW_THERMAL_ZONE_MAX_NAME]; snprintf(tz_name, sizeof(tz_name), "mlxsw-gearbox%d", @@ -877,11 +843,10 @@ mlxsw_thermal_gearbox_tz_init(struct mlxsw_thermal_module *gearbox_tz) MLXSW_THERMAL_TRIP_MASK, gearbox_tz, &mlxsw_thermal_gearbox_ops, - NULL, 0, 0); + &tzp, 0, 0); if (IS_ERR(gearbox_tz->tzdev)) return PTR_ERR(gearbox_tz->tzdev); - gearbox_tz->mode = THERMAL_DEVICE_ENABLED; return 0; } @@ -960,6 +925,9 @@ int mlxsw_thermal_init(struct mlxsw_core *core, const struct mlxsw_bus_info *bus_info, struct mlxsw_thermal **p_thermal) { + struct thermal_zone_params tzp = { + .initial_mode = THERMAL_DEVICE_ENABLED, + }; char mfcr_pl[MLXSW_REG_MFCR_LEN] = { 0 }; enum mlxsw_reg_mfcr_pwm_frequency freq; struct device *dev = bus_info->dev; @@ -1034,7 +1002,7 @@ int mlxsw_thermal_init(struct mlxsw_core *core, MLXSW_THERMAL_TRIP_MASK, thermal, &mlxsw_thermal_ops, - NULL, 0, + &tzp, 0, thermal->polling_delay); if (IS_ERR(thermal->tzdev)) { err = PTR_ERR(thermal->tzdev); @@ -1050,7 +1018,6 @@ int mlxsw_thermal_init(struct mlxsw_core *core, if (err) goto err_unreg_modules_tzdev; - thermal->mode = THERMAL_DEVICE_ENABLED; *p_thermal = thermal; return 0; diff --git a/drivers/platform/x86/acerhdf.c b/drivers/platform/x86/acerhdf.c index 8cc86f4e3ac1..aaf8b845be90 100644 --- a/drivers/platform/x86/acerhdf.c +++ b/drivers/platform/x86/acerhdf.c @@ -406,22 +406,9 @@ static inline void acerhdf_enable_kernelmode(void) kernelmode = 1; thz_dev->polling_delay = interval*1000; - thermal_zone_device_update(thz_dev, THERMAL_EVENT_UNSPECIFIED); pr_notice("kernel mode fan control ON\n"); } -static int acerhdf_get_mode(struct thermal_zone_device *thermal, - enum thermal_device_mode *mode) -{ - if (verbose) - pr_notice("kernel mode fan control %d\n", kernelmode); - - *mode = (kernelmode) ? THERMAL_DEVICE_ENABLED - : THERMAL_DEVICE_DISABLED; - - return 0; -} - /* * set operation mode; * enabled: the thermal layer of the kernel takes care about @@ -488,7 +475,6 @@ static struct thermal_zone_device_ops acerhdf_dev_ops = { .bind = acerhdf_bind, .unbind = acerhdf_unbind, .get_temp = acerhdf_get_ec_temp, - .get_mode = acerhdf_get_mode, .set_mode = acerhdf_set_mode, .get_trip_type = acerhdf_get_trip_type, .get_trip_hyst = acerhdf_get_trip_hyst, @@ -554,6 +540,7 @@ static int acerhdf_set_cur_state(struct thermal_cooling_device *cdev, err_out: acerhdf_revert_to_bios_mode(); + thz_dev->mode = THERMAL_DEVICE_DISABLED; return -EINVAL; } @@ -739,6 +726,8 @@ static int __init acerhdf_register_thermal(void) if (IS_ERR(cl_dev)) return -EINVAL; + acerhdf_zone_params.initial_mode = + kernelmode ? THERMAL_DEVICE_ENABLED : THERMAL_DEVICE_DISABLED; thz_dev = thermal_zone_device_register("acerhdf", 2, 0, NULL, &acerhdf_dev_ops, &acerhdf_zone_params, 0, diff --git a/drivers/thermal/da9062-thermal.c b/drivers/thermal/da9062-thermal.c index c32709badeda..4bdb6f9621c1 100644 --- a/drivers/thermal/da9062-thermal.c +++ b/drivers/thermal/da9062-thermal.c @@ -49,7 +49,6 @@ struct da9062_thermal { struct da9062 *hw; struct delayed_work work; struct thermal_zone_device *zone; - enum thermal_device_mode mode; struct mutex lock; /* protection for da9062_thermal temperature */ int temperature; int irq; @@ -121,14 +120,6 @@ static irqreturn_t da9062_thermal_irq_handler(int irq, void *data) return IRQ_HANDLED; } -static int da9062_thermal_get_mode(struct thermal_zone_device *z, - enum thermal_device_mode *mode) -{ - struct da9062_thermal *thermal = z->devdata; - *mode = thermal->mode; - return 0; -} - static int da9062_thermal_get_trip_type(struct thermal_zone_device *z, int trip, enum thermal_trip_type *type) @@ -181,7 +172,6 @@ static int da9062_thermal_get_temp(struct thermal_zone_device *z, static struct thermal_zone_device_ops da9062_thermal_ops = { .get_temp = da9062_thermal_get_temp, - .get_mode = da9062_thermal_get_mode, .get_trip_type = da9062_thermal_get_trip_type, .get_trip_temp = da9062_thermal_get_trip_temp, }; @@ -199,6 +189,9 @@ MODULE_DEVICE_TABLE(of, da9062_compatible_reg_id_table); static int da9062_thermal_probe(struct platform_device *pdev) { + struct thermal_zone_params tzp = { + .initial_mode = THERMAL_DEVICE_ENABLED, + }; struct da9062 *chip = dev_get_drvdata(pdev->dev.parent); struct da9062_thermal *thermal; unsigned int pp_tmp = DA9062_DEFAULT_POLLING_MS_PERIOD; @@ -233,7 +226,6 @@ static int da9062_thermal_probe(struct platform_device *pdev) thermal->config = match->data; thermal->hw = chip; - thermal->mode = THERMAL_DEVICE_ENABLED; thermal->dev = &pdev->dev; INIT_DELAYED_WORK(&thermal->work, da9062_thermal_poll_on); @@ -241,7 +233,7 @@ static int da9062_thermal_probe(struct platform_device *pdev) thermal->zone = thermal_zone_device_register(thermal->config->name, 1, 0, thermal, - &da9062_thermal_ops, NULL, pp_tmp, + &da9062_thermal_ops, &tzp, pp_tmp, 0); if (IS_ERR(thermal->zone)) { dev_err(&pdev->dev, "Cannot register thermal zone device\n"); diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index e761c9b42217..3e02323c938b 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -197,7 +197,6 @@ struct imx_thermal_data { struct cpufreq_policy *policy; struct thermal_zone_device *tz; struct thermal_cooling_device *cdev; - enum thermal_device_mode mode; struct regmap *tempmon; u32 c1, c2; /* See formula in imx_init_calib() */ int temp_passive; @@ -256,7 +255,7 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) bool wait; u32 val; - if (data->mode == THERMAL_DEVICE_ENABLED) { + if (tz->mode == THERMAL_DEVICE_ENABLED) { /* Check if a measurement is currently in progress */ regmap_read(map, soc_data->temp_data, &val); wait = !(val & soc_data->temp_valid_mask); @@ -283,7 +282,7 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) regmap_read(map, soc_data->temp_data, &val); - if (data->mode != THERMAL_DEVICE_ENABLED) { + if (tz->mode != THERMAL_DEVICE_ENABLED) { regmap_write(map, soc_data->sensor_ctrl + REG_CLR, soc_data->measure_temp_mask); regmap_write(map, soc_data->sensor_ctrl + REG_SET, @@ -331,16 +330,6 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) return 0; } -static int imx_get_mode(struct thermal_zone_device *tz, - enum thermal_device_mode *mode) -{ - struct imx_thermal_data *data = tz->devdata; - - *mode = data->mode; - - return 0; -} - static int imx_set_mode(struct thermal_zone_device *tz, enum thermal_device_mode mode) { @@ -376,9 +365,6 @@ static int imx_set_mode(struct thermal_zone_device *tz, } } - data->mode = mode; - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); - return 0; } @@ -467,7 +453,6 @@ static struct thermal_zone_device_ops imx_tz_ops = { .bind = imx_bind, .unbind = imx_unbind, .get_temp = imx_get_temp, - .get_mode = imx_get_mode, .set_mode = imx_set_mode, .get_trip_type = imx_get_trip_type, .get_trip_temp = imx_get_trip_temp, @@ -691,6 +676,9 @@ static inline void imx_thermal_unregister_legacy_cooling(struct imx_thermal_data static int imx_thermal_probe(struct platform_device *pdev) { + struct thermal_zone_params tzp = { + .initial_mode = THERMAL_DEVICE_ENABLED, + }; struct imx_thermal_data *data; struct regmap *map; int measure_freq; @@ -799,7 +787,7 @@ static int imx_thermal_probe(struct platform_device *pdev) data->tz = thermal_zone_device_register("imx_thermal_zone", IMX_TRIP_NUM, BIT(IMX_TRIP_PASSIVE), data, - &imx_tz_ops, NULL, + &imx_tz_ops, &tzp, IMX_PASSIVE_DELAY, IMX_POLLING_DELAY); if (IS_ERR(data->tz)) { @@ -831,7 +819,6 @@ static int imx_thermal_probe(struct platform_device *pdev) data->socdata->measure_temp_mask); data->irq_enabled = true; - data->mode = THERMAL_DEVICE_ENABLED; ret = devm_request_threaded_irq(&pdev->dev, data->irq, imx_thermal_alarm_irq, imx_thermal_alarm_irq_thread, @@ -885,7 +872,7 @@ static int __maybe_unused imx_thermal_suspend(struct device *dev) data->socdata->measure_temp_mask); regmap_write(map, data->socdata->sensor_ctrl + REG_SET, data->socdata->power_down_mask); - data->mode = THERMAL_DEVICE_DISABLED; + data->tz->mode = THERMAL_DEVICE_DISABLED; clk_disable_unprepare(data->thermal_clk); return 0; @@ -905,7 +892,7 @@ static int __maybe_unused imx_thermal_resume(struct device *dev) data->socdata->power_down_mask); regmap_write(map, data->socdata->sensor_ctrl + REG_SET, data->socdata->measure_temp_mask); - data->mode = THERMAL_DEVICE_ENABLED; + data->tz->mode = THERMAL_DEVICE_ENABLED; return 0; } diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c index e802922a13cf..86a00598ed09 100644 --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c @@ -44,7 +44,6 @@ static char *int3400_thermal_uuids[INT3400_THERMAL_MAXIMUM_UUID] = { struct int3400_thermal_priv { struct acpi_device *adev; struct thermal_zone_device *thermal; - int mode; int art_count; struct art *arts; int trt_count; @@ -230,48 +229,29 @@ static int int3400_thermal_get_temp(struct thermal_zone_device *thermal, return 0; } -static int int3400_thermal_get_mode(struct thermal_zone_device *thermal, - enum thermal_device_mode *mode) -{ - struct int3400_thermal_priv *priv = thermal->devdata; - - if (!priv) - return -EINVAL; - - *mode = priv->mode; - - return 0; -} - static int int3400_thermal_set_mode(struct thermal_zone_device *thermal, enum thermal_device_mode mode) { struct int3400_thermal_priv *priv = thermal->devdata; - bool enable; int result = 0; if (!priv) return -EINVAL; - if (mode == THERMAL_DEVICE_ENABLED) - enable = true; - else if (mode == THERMAL_DEVICE_DISABLED) - enable = false; - else + if (mode != THERMAL_DEVICE_ENABLED && + mode != THERMAL_DEVICE_DISABLED) return -EINVAL; - if (enable != priv->mode) { - priv->mode = enable; + if (mode != thermal->mode) { result = int3400_thermal_run_osc(priv->adev->handle, - priv->current_uuid_index, - enable); + priv->current_uuid_index, + mode == THERMAL_DEVICE_ENABLED); } return result; } static struct thermal_zone_device_ops int3400_thermal_ops = { .get_temp = int3400_thermal_get_temp, - .get_mode = int3400_thermal_get_mode, .set_mode = int3400_thermal_set_mode, }; diff --git a/drivers/thermal/intel/intel_quark_dts_thermal.c b/drivers/thermal/intel/intel_quark_dts_thermal.c index d704fc104cfd..c4879b4bfbf1 100644 --- a/drivers/thermal/intel/intel_quark_dts_thermal.c +++ b/drivers/thermal/intel/intel_quark_dts_thermal.c @@ -103,7 +103,6 @@ struct soc_sensor_entry { bool locked; u32 store_ptps; u32 store_dts_enable; - enum thermal_device_mode mode; struct thermal_zone_device *tzone; }; @@ -128,7 +127,7 @@ static int soc_dts_enable(struct thermal_zone_device *tzd) return ret; if (out & QRK_DTS_ENABLE_BIT) { - aux_entry->mode = THERMAL_DEVICE_ENABLED; + tzd->mode = THERMAL_DEVICE_ENABLED; return 0; } @@ -139,9 +138,9 @@ static int soc_dts_enable(struct thermal_zone_device *tzd) if (ret) return ret; - aux_entry->mode = THERMAL_DEVICE_ENABLED; + tzd->mode = THERMAL_DEVICE_ENABLED; } else { - aux_entry->mode = THERMAL_DEVICE_DISABLED; + tzd->mode = THERMAL_DEVICE_DISABLED; pr_info("DTS is locked. Cannot enable DTS\n"); ret = -EPERM; } @@ -161,7 +160,7 @@ static int soc_dts_disable(struct thermal_zone_device *tzd) return ret; if (!(out & QRK_DTS_ENABLE_BIT)) { - aux_entry->mode = THERMAL_DEVICE_DISABLED; + tzd->mode = THERMAL_DEVICE_DISABLED; return 0; } @@ -173,9 +172,9 @@ static int soc_dts_disable(struct thermal_zone_device *tzd) if (ret) return ret; - aux_entry->mode = THERMAL_DEVICE_DISABLED; + tzd->mode = THERMAL_DEVICE_DISABLED; } else { - aux_entry->mode = THERMAL_DEVICE_ENABLED; + tzd->mode = THERMAL_DEVICE_ENABLED; pr_info("DTS is locked. Cannot disable DTS\n"); ret = -EPERM; } @@ -309,14 +308,6 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd, return 0; } -static int sys_get_mode(struct thermal_zone_device *tzd, - enum thermal_device_mode *mode) -{ - struct soc_sensor_entry *aux_entry = tzd->devdata; - *mode = aux_entry->mode; - return 0; -} - static int sys_set_mode(struct thermal_zone_device *tzd, enum thermal_device_mode mode) { @@ -338,7 +329,6 @@ static struct thermal_zone_device_ops tzone_ops = { .get_trip_type = sys_get_trip_type, .set_trip_temp = sys_set_trip_temp, .get_crit_temp = sys_get_crit_temp, - .get_mode = sys_get_mode, .set_mode = sys_set_mode, }; diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c index 874a47d6923f..863b89546f81 100644 --- a/drivers/thermal/of-thermal.c +++ b/drivers/thermal/of-thermal.c @@ -51,7 +51,6 @@ struct __thermal_bind_params { /** * struct __thermal_zone - internal representation of a thermal zone - * @mode: current thermal zone device mode (enabled/disabled) * @passive_delay: polling interval while passive cooling is activated * @polling_delay: zone polling interval * @slope: slope of the temperature adjustment curve @@ -65,7 +64,6 @@ struct __thermal_bind_params { */ struct __thermal_zone { - enum thermal_device_mode mode; int passive_delay; int polling_delay; int slope; @@ -269,23 +267,11 @@ static int of_thermal_unbind(struct thermal_zone_device *thermal, return 0; } -static int of_thermal_get_mode(struct thermal_zone_device *tz, - enum thermal_device_mode *mode) -{ - struct __thermal_zone *data = tz->devdata; - - *mode = data->mode; - - return 0; -} - static int of_thermal_set_mode(struct thermal_zone_device *tz, enum thermal_device_mode mode) { struct __thermal_zone *data = tz->devdata; - mutex_lock(&tz->lock); - if (mode == THERMAL_DEVICE_ENABLED) { tz->polling_delay = data->polling_delay; tz->passive_delay = data->passive_delay; @@ -294,11 +280,6 @@ static int of_thermal_set_mode(struct thermal_zone_device *tz, tz->passive_delay = 0; } - mutex_unlock(&tz->lock); - - data->mode = mode; - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); - return 0; } @@ -393,7 +374,6 @@ static int of_thermal_get_crit_temp(struct thermal_zone_device *tz, } static struct thermal_zone_device_ops of_thermal_ops = { - .get_mode = of_thermal_get_mode, .set_mode = of_thermal_set_mode, .get_trip_type = of_thermal_get_trip_type, @@ -553,8 +533,14 @@ thermal_zone_of_sensor_register(struct device *dev, int sensor_id, void *data, if (id == sensor_id) { tzd = thermal_zone_of_add_sensor(child, sensor_np, data, ops); - if (!IS_ERR(tzd)) + if (!IS_ERR(tzd)) { + mutex_lock(&tzd->lock); tzd->ops->set_mode(tzd, THERMAL_DEVICE_ENABLED); + tzd->mode = THERMAL_DEVICE_ENABLED; + mutex_unlock(&tzd->lock); + thermal_zone_device_update(tzd, + THERMAL_EVENT_UNSPECIFIED); + } of_node_put(child); goto exit; @@ -979,7 +965,6 @@ __init *thermal_of_build_thermal_zone(struct device_node *np) finish: of_node_put(child); - tz->mode = THERMAL_DEVICE_DISABLED; return tz; @@ -1120,6 +1105,7 @@ int __init of_parse_thermal_zones(void) /* these two are left for temperature drivers to use */ tzp->slope = tz->slope; tzp->offset = tz->offset; + tzp->initial_mode = THERMAL_DEVICE_DISABLED; zone = thermal_zone_device_register(child->name, tz->ntrips, mask, tz, diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index c06550930979..5ff98fcc0f6a 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -463,6 +463,43 @@ static void thermal_zone_device_reset(struct thermal_zone_device *tz) thermal_zone_device_init(tz); } +enum thermal_device_mode +thermal_zone_device_get_mode(struct thermal_zone_device *tz) +{ + enum thermal_device_mode mode; + + mutex_lock(&tz->lock); + + mode = tz->mode; + + mutex_unlock(&tz->lock); + + return mode; +} + +int thermal_zone_device_set_mode(struct thermal_zone_device *tz, + enum thermal_device_mode mode) +{ + int ret = 0; + + if (mode != THERMAL_DEVICE_DISABLED && + mode != THERMAL_DEVICE_ENABLED) + return -EINVAL; + + mutex_lock(&tz->lock); + + if (tz->ops->set_mode) + ret = tz->ops->set_mode(tz, mode); + + tz->mode = mode; + + mutex_unlock(&tz->lock); + + thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); + + return ret; +} + void thermal_zone_device_update(struct thermal_zone_device *tz, enum thermal_notify_event event) { @@ -1344,6 +1381,9 @@ thermal_zone_device_register(const char *type, int trips, int mask, if (atomic_cmpxchg(&tz->need_update, 1, 0)) thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); + if (tzp) + thermal_zone_device_set_mode(tz, tzp->initial_mode); + return tz; unregister: @@ -1473,9 +1513,7 @@ static int thermal_pm_notify(struct notifier_block *nb, case PM_POST_SUSPEND: atomic_set(&in_suspend, 0); list_for_each_entry(tz, &thermal_tz_list, node) { - tz_mode = THERMAL_DEVICE_ENABLED; - if (tz->ops->get_mode) - tz->ops->get_mode(tz, &tz_mode); + tz_mode = thermal_zone_device_get_mode(tz); if (tz_mode == THERMAL_DEVICE_DISABLED) continue; diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h index c95689586e19..8e561bac3133 100644 --- a/drivers/thermal/thermal_core.h +++ b/drivers/thermal/thermal_core.h @@ -141,6 +141,22 @@ thermal_cooling_device_stats_update(struct thermal_cooling_device *cdev, unsigned long new_state) {} #endif /* CONFIG_THERMAL_STATISTICS */ +enum thermal_device_mode +thermal_zone_device_get_mode(struct thermal_zone_device *tz); + +int thermal_zone_device_set_mode(struct thermal_zone_device *tz, + enum thermal_device_mode mode); + +static inline int thermal_zone_device_enable(struct thermal_zone_device *tz) +{ + return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_ENABLED); +} + +static inline int thermal_zone_device_disable(struct thermal_zone_device *tz) +{ + return thermal_zone_device_set_mode(tz, THERMAL_DEVICE_DISABLED); +} + /* device tree support */ #ifdef CONFIG_THERMAL_OF int of_parse_thermal_zones(void); diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c index aa99edb4dff7..cbb27b3c96d2 100644 --- a/drivers/thermal/thermal_sysfs.c +++ b/drivers/thermal/thermal_sysfs.c @@ -50,14 +50,8 @@ mode_show(struct device *dev, struct device_attribute *attr, char *buf) { struct thermal_zone_device *tz = to_thermal_zone(dev); enum thermal_device_mode mode; - int result; - - if (!tz->ops->get_mode) - return -EPERM; - result = tz->ops->get_mode(tz, &mode); - if (result) - return result; + mode = thermal_zone_device_get_mode(tz); return sprintf(buf, "%s\n", mode == THERMAL_DEVICE_ENABLED ? "enabled" : "disabled"); @@ -74,9 +68,9 @@ mode_store(struct device *dev, struct device_attribute *attr, return -EPERM; if (!strncmp(buf, "enabled", sizeof("enabled") - 1)) - result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED); + result = thermal_zone_device_enable(tz); else if (!strncmp(buf, "disabled", sizeof("disabled") - 1)) - result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED); + result = thermal_zone_device_disable(tz); else result = -EINVAL; @@ -428,30 +422,13 @@ static struct attribute_group thermal_zone_attribute_group = { .attrs = thermal_zone_dev_attrs, }; -/* We expose mode only if .get_mode is present */ static struct attribute *thermal_zone_mode_attrs[] = { &dev_attr_mode.attr, NULL, }; -static umode_t thermal_zone_mode_is_visible(struct kobject *kobj, - struct attribute *attr, - int attrno) -{ - struct device *dev = container_of(kobj, struct device, kobj); - struct thermal_zone_device *tz; - - tz = container_of(dev, struct thermal_zone_device, device); - - if (tz->ops->get_mode) - return attr->mode; - - return 0; -} - static struct attribute_group thermal_zone_mode_attribute_group = { .attrs = thermal_zone_mode_attrs, - .is_visible = thermal_zone_mode_is_visible, }; /* We expose passive only if passive trips are present */ diff --git a/include/linux/thermal.h b/include/linux/thermal.h index 216185bb3014..da4141697e2e 100644 --- a/include/linux/thermal.h +++ b/include/linux/thermal.h @@ -76,8 +76,6 @@ struct thermal_zone_device_ops { struct thermal_cooling_device *); int (*get_temp) (struct thermal_zone_device *, int *); int (*set_trips) (struct thermal_zone_device *, int, int); - int (*get_mode) (struct thermal_zone_device *, - enum thermal_device_mode *); int (*set_mode) (struct thermal_zone_device *, enum thermal_device_mode); int (*get_trip_type) (struct thermal_zone_device *, int, @@ -128,6 +126,7 @@ struct thermal_cooling_device { * @trip_temp_attrs: attributes for trip points for sysfs: trip temperature * @trip_type_attrs: attributes for trip points for sysfs: trip type * @trip_hyst_attrs: attributes for trip points for sysfs: trip hysteresis + * @mode: current mode of this thermal zone * @devdata: private pointer for device private data * @trips: number of trip points the thermal zone supports * @trips_disabled; bitmap for disabled trips @@ -170,6 +169,7 @@ struct thermal_zone_device { struct thermal_attr *trip_temp_attrs; struct thermal_attr *trip_type_attrs; struct thermal_attr *trip_hyst_attrs; + enum thermal_device_mode mode; void *devdata; int trips; unsigned long trips_disabled; /* bitmap for disabled trips */ @@ -264,6 +264,9 @@ struct thermal_zone_params { int num_tbps; /* Number of tbp entries */ struct thermal_bind_params *tbp; + /* Initial mode of this thermal zone device */ + enum thermal_device_mode initial_mode; + /* * Sustainable power (heat) that this thermal zone can dissipate in * mW
Thermal zone devices' mode is stored in individual drivers. This patch changes it so that mode is stored in struct thermal_zone_device instead. As a result all driver-specific variables storing the mode are not needed and are removed. Consequently, the get_mode() implementations have nothing to operate on and need to be removed, too. Some thermal framework specific functions are introduced: thermal_zone_device_get_mode() thermal_zone_device_set_mode() thermal_zone_device_enable() thermal_zone_device_disable() thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock and the "set" calls driver's set_mode() if provided, so the latter must not take this lock again. At the end of the "set" thermal_zone_device_update() is called so drivers don't need to repeat this invocation in their specific set_mode() implementations. The scope of the above 4 functions is purposedly limited to the thermal framework and drivers are not supposed to call them. This encapsulation does not fully work at the moment for some drivers, though: - platform/x86/acerhdf.c - drivers/thermal/imx_thermal.c - drivers/thermal/intel/intel_quark_dts_thermal.c - drivers/thermal/of-thermal.c and they manipulate struct thermal_zone_device's members directly. struct thermal_zone_params gains a new member called initial_mode, which is used to set tzd's mode at registration time. The sysfs "mode" attribute is always exposed from now on, because all thermal zone devices now have their get_mode() implemented at the generic level and it is always available. Exposing "mode" doesn't hurt the drivers which don't provide their own set_mode(), because writing to "mode" will result in -EPERM, as expected. Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> --- drivers/acpi/thermal.c | 46 +++++---------- .../ethernet/mellanox/mlxsw/core_thermal.c | 57 ++++--------------- drivers/platform/x86/acerhdf.c | 17 +----- drivers/thermal/da9062-thermal.c | 16 ++---- drivers/thermal/imx_thermal.c | 29 +++------- .../intel/int340x_thermal/int3400_thermal.c | 30 ++-------- .../thermal/intel/intel_quark_dts_thermal.c | 22 ++----- drivers/thermal/of-thermal.c | 30 +++------- drivers/thermal/thermal_core.c | 44 +++++++++++++- drivers/thermal/thermal_core.h | 16 ++++++ drivers/thermal/thermal_sysfs.c | 29 +--------- include/linux/thermal.h | 7 ++- 12 files changed, 125 insertions(+), 218 deletions(-)