| Message ID | 20240130111250.185718-1-angelogioacchino.delregno@collabora.com |
|---|---|
| Headers | show |
| Series | Thermal: Part 1 - Introduce new structs and registration | expand |
On Tue, Jan 30, 2024 at 12:13 PM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > This series is a preparation for a bigger cleanup that will be split in > three parts in order to avoid immutable branches on multiple subsystems, > as in other parts of this series there will be changes in: > - drivers/acpi > - drivers/platform/x86 > - drivers/power/supply > - drivers/net/wireless > - drivers/net/ethernet > > This is the first part which adds new structures and starts reorganizing > struct members around, plus, this migrates all of the thermal drivers > found in drivers/thermal/ to the new thermal_zone_device_register() > function, and advertises that the old registration functions are > deprecated and will be removed. > > The reorganization is supposed to be complete (or mostly) but... > - struct thermal_zone_platform_params has a temporary name: > this is done in order to avoid compile time failures for > the drivers outside of drivers/thermal before migrating > them to thermal_zone_device_params/thermal_zone_device_register(); > - struct thermal_zone_params temporarily has two duplicated members, > governor_name and no_hwmon; > > Part 2 of this topic will migrate all drivers that are external to > drivers/thermal to thermal_zone_device_register(); I will send that > part only after part 1 is confirmed to be acceptable, as otherwise > I'd be spamming people for no reason :-) > > After all drivers will be migrated to thermal_zone_device_register(), > we won't have to care about changing anything outside of drivers/thermal > to finish this set of cleanups/changes (and no immutable branches needed) > and this means that... > Part 3 of this topic will contain the following changes: > - thermal_zone_device_register_with_trips() will be removed > - thermal_tripless_zone_device_register() will be removed > - thermal_zone_params will be renamed to thermal_governor_ipa_params > - governor_name, no_hwmon members will be removed > - thermal_zone_platform_params will be renamed to thermal_zone_params > - Removal of the THERMAL_NAME_LENGTH limitation for `type` > > More scheduled changes, which should end up in part 3 (at least that's > my intention), or eventually entirely after this cleanup topic, include: > - Introduction of Thermal Zone names > - Disambiguation of TZ name and type > - Addition of `thermal-zones`, `thermal-zone-names` parsing for > devicetrees You really should start with this, because that's your goal. It is quite arguable that it can be achieved without making all of the changes mentioned above. > > ... Summarizing ... > > Part 1: > - Reorganize structures (some temporary names/leftovers) > - New registration function, deprecation of old ones > - Migration of drivers/thermal drivers to new registration I kind of see where this is going, but I don't agree with some of the changes made. Let me comment on individual patches (which is not necessarily going to happen today or even tomorrow, so let me go through the entire series before deciding what to do next). Thanks!
Il 01/02/24 19:35, Rafael J. Wysocki ha scritto: > On Tue, Jan 30, 2024 at 12:13 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: >> >> This series is a preparation for a bigger cleanup that will be split in >> three parts in order to avoid immutable branches on multiple subsystems, >> as in other parts of this series there will be changes in: >> - drivers/acpi >> - drivers/platform/x86 >> - drivers/power/supply >> - drivers/net/wireless >> - drivers/net/ethernet >> >> This is the first part which adds new structures and starts reorganizing >> struct members around, plus, this migrates all of the thermal drivers >> found in drivers/thermal/ to the new thermal_zone_device_register() >> function, and advertises that the old registration functions are >> deprecated and will be removed. >> >> The reorganization is supposed to be complete (or mostly) but... >> - struct thermal_zone_platform_params has a temporary name: >> this is done in order to avoid compile time failures for >> the drivers outside of drivers/thermal before migrating >> them to thermal_zone_device_params/thermal_zone_device_register(); >> - struct thermal_zone_params temporarily has two duplicated members, >> governor_name and no_hwmon; >> >> Part 2 of this topic will migrate all drivers that are external to >> drivers/thermal to thermal_zone_device_register(); I will send that >> part only after part 1 is confirmed to be acceptable, as otherwise >> I'd be spamming people for no reason :-) >> >> After all drivers will be migrated to thermal_zone_device_register(), >> we won't have to care about changing anything outside of drivers/thermal >> to finish this set of cleanups/changes (and no immutable branches needed) >> and this means that... >> Part 3 of this topic will contain the following changes: >> - thermal_zone_device_register_with_trips() will be removed >> - thermal_tripless_zone_device_register() will be removed >> - thermal_zone_params will be renamed to thermal_governor_ipa_params >> - governor_name, no_hwmon members will be removed >> - thermal_zone_platform_params will be renamed to thermal_zone_params >> - Removal of the THERMAL_NAME_LENGTH limitation for `type` >> >> More scheduled changes, which should end up in part 3 (at least that's >> my intention), or eventually entirely after this cleanup topic, include: >> - Introduction of Thermal Zone names >> - Disambiguation of TZ name and type >> - Addition of `thermal-zones`, `thermal-zone-names` parsing for >> devicetrees > > You really should start with this, because that's your goal. > > It is quite arguable that it can be achieved without making all of the > changes mentioned above. > Actually, my initial idea was exactly that... but then there were some discussions around it, please check [1] for the thermal-zone-names, and [2] for the rest of the discussion. [1]: https://lore.kernel.org/all/8d42e0f5-b2d2-471b-ada9-79f76c637abe@collabora.com/ [2]: https://lore.kernel.org/all/4dd4ac79-e8bc-4d88-92d6-6061dae42092@collabora.com/ >> >> ... Summarizing ... >> >> Part 1: >> - Reorganize structures (some temporary names/leftovers) >> - New registration function, deprecation of old ones >> - Migration of drivers/thermal drivers to new registration > > I kind of see where this is going, but I don't agree with some of the > changes made. > > Let me comment on individual patches (which is not necessarily going > to happen today or even tomorrow, so let me go through the entire > series before deciding what to do next). > Sure, thanks for your feedback! Cheers, Angelo
Il 01/02/24 19:37, Rafael J. Wysocki ha scritto: > On Tue, Jan 30, 2024 at 12:13 PM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: >> >> All users are already assigning a const char * to the `governor_name` >> member of struct thermal_zone_params and to the `name` member of >> struct thermal_governor. >> Even if users are technically wrong, it just makes more sense to change >> this member to be a const char pointer instead of doing the other way >> around. >> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > Acked-by: Rafael J. Wysocki <rafael@kernel.org> > > or I can pick it up right away if you want me to do that. > I appreciate having less patches to carry over with new series versions. Whatever you can take, please feel free to pick directly :-) Thanks, Angelo >> --- >> include/linux/thermal.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h >> index b7a3deb372fd..65d8f92a9a0d 100644 >> --- a/include/linux/thermal.h >> +++ b/include/linux/thermal.h >> @@ -214,7 +214,7 @@ struct thermal_zone_device { >> * @governor_list: node in thermal_governor_list (in thermal_core.c) >> */ >> struct thermal_governor { >> - char name[THERMAL_NAME_LENGTH]; >> + const char *name; >> int (*bind_to_tz)(struct thermal_zone_device *tz); >> void (*unbind_from_tz)(struct thermal_zone_device *tz); >> int (*throttle)(struct thermal_zone_device *tz, >> @@ -226,7 +226,7 @@ struct thermal_governor { >> >> /* Structure to define Thermal Zone parameters */ >> struct thermal_zone_params { >> - char governor_name[THERMAL_NAME_LENGTH]; >> + const char *governor_name; >> >> /* >> * a boolean to indicate if the thermal to hwmon sysfs interface >> --
On Fri, Feb 2, 2024 at 9:47 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Thu, Feb 01, 2024 at 08:24:15PM +0100, Rafael J. Wysocki wrote: > > On Tue, Jan 30, 2024 at 12:13 PM AngeloGioacchino Del Regno > > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > > > index 65d8f92a9a0d..7a540b746703 100644 > > > --- a/include/linux/thermal.h > > > +++ b/include/linux/thermal.h > > > @@ -149,7 +149,8 @@ struct thermal_cooling_device { > > > passive trip point. > > > * @need_update: if equals 1, thermal_zone_device_update needs to be invoked. > > > * @ops: operations this &thermal_zone_device supports > > > - * @tzp: thermal zone parameters > > > + * @tzp: Thermal zone parameters > > > + * @tgp: Thermal zone governor parameters > > > * @governor: pointer to the governor for this thermal zone > > > * @governor_data: private pointer for governor data > > > * @thermal_instances: list of &struct thermal_instance of this thermal zone > > > @@ -184,7 +185,8 @@ struct thermal_zone_device { > > > int prev_high_trip; > > > atomic_t need_update; > > > struct thermal_zone_device_ops *ops; > > > - struct thermal_zone_params *tzp; > > > + struct thermal_zone_platform_params *tzp; > > > + struct thermal_governor_params *tgp; > > > > I agree with doing a split here, but I'm not sure about moving items > > from the arg list to struct thermal_zone_platform_params (as mentioned > > above). > > > > Also the naming is quite inconsistent. IMO it would be better to call > > the first pointer "tzpp", rename struct thermal_governor_params to > > struct thermal_zone_governor_params and call the second pointer > > "tzgp". > > > > The names "tzgp" and "tzpp" look almost identical at first glance. > Could we increase the hamming distance somehow? Good point. They may as well be gov_params and platform_params AFAIAC.
On Fri, Feb 2, 2024 at 11:01 AM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 01/02/24 19:37, Rafael J. Wysocki ha scritto: > > On Tue, Jan 30, 2024 at 12:13 PM AngeloGioacchino Del Regno > > <angelogioacchino.delregno@collabora.com> wrote: > >> > >> All users are already assigning a const char * to the `governor_name` > >> member of struct thermal_zone_params and to the `name` member of > >> struct thermal_governor. > >> Even if users are technically wrong, it just makes more sense to change > >> this member to be a const char pointer instead of doing the other way > >> around. > >> > >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > > > Acked-by: Rafael J. Wysocki <rafael@kernel.org> > > > > or I can pick it up right away if you want me to do that. > > > > I appreciate having less patches to carry over with new series versions. > > Whatever you can take, please feel free to pick directly :-) OK, applied (as 6.9 material). Thanks!
Il 02/02/24 18:13, Rafael J. Wysocki ha scritto: > On Fri, Feb 2, 2024 at 9:47 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: >> >> On Thu, Feb 01, 2024 at 08:24:15PM +0100, Rafael J. Wysocki wrote: >>> On Tue, Jan 30, 2024 at 12:13 PM AngeloGioacchino Del Regno >>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h >>>> index 65d8f92a9a0d..7a540b746703 100644 >>>> --- a/include/linux/thermal.h >>>> +++ b/include/linux/thermal.h >>>> @@ -149,7 +149,8 @@ struct thermal_cooling_device { >>>> passive trip point. >>>> * @need_update: if equals 1, thermal_zone_device_update needs to be invoked. >>>> * @ops: operations this &thermal_zone_device supports >>>> - * @tzp: thermal zone parameters >>>> + * @tzp: Thermal zone parameters >>>> + * @tgp: Thermal zone governor parameters >>>> * @governor: pointer to the governor for this thermal zone >>>> * @governor_data: private pointer for governor data >>>> * @thermal_instances: list of &struct thermal_instance of this thermal zone >>>> @@ -184,7 +185,8 @@ struct thermal_zone_device { >>>> int prev_high_trip; >>>> atomic_t need_update; >>>> struct thermal_zone_device_ops *ops; >>>> - struct thermal_zone_params *tzp; >>>> + struct thermal_zone_platform_params *tzp; >>>> + struct thermal_governor_params *tgp; >>> >>> I agree with doing a split here, but I'm not sure about moving items >>> from the arg list to struct thermal_zone_platform_params (as mentioned >>> above). >>> >>> Also the naming is quite inconsistent. IMO it would be better to call >>> the first pointer "tzpp", rename struct thermal_governor_params to >>> struct thermal_zone_governor_params and call the second pointer >>> "tzgp". >>> >> >> The names "tzgp" and "tzpp" look almost identical at first glance. >> Could we increase the hamming distance somehow? > > Good point. > > They may as well be gov_params and platform_params AFAIAC. I'm more for gov_params and zone_params, as the thermal_zone_platform_params is supposed to get renamed to struct thermal_zone_params in part 2. Anything against that? Cheers, Angelo
On Mon, Feb 12, 2024 at 11:41 AM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 02/02/24 18:13, Rafael J. Wysocki ha scritto: > > On Fri, Feb 2, 2024 at 9:47 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > >> > >> On Thu, Feb 01, 2024 at 08:24:15PM +0100, Rafael J. Wysocki wrote: > >>> On Tue, Jan 30, 2024 at 12:13 PM AngeloGioacchino Del Regno > >>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h > >>>> index 65d8f92a9a0d..7a540b746703 100644 > >>>> --- a/include/linux/thermal.h > >>>> +++ b/include/linux/thermal.h > >>>> @@ -149,7 +149,8 @@ struct thermal_cooling_device { > >>>> passive trip point. > >>>> * @need_update: if equals 1, thermal_zone_device_update needs to be invoked. > >>>> * @ops: operations this &thermal_zone_device supports > >>>> - * @tzp: thermal zone parameters > >>>> + * @tzp: Thermal zone parameters > >>>> + * @tgp: Thermal zone governor parameters > >>>> * @governor: pointer to the governor for this thermal zone > >>>> * @governor_data: private pointer for governor data > >>>> * @thermal_instances: list of &struct thermal_instance of this thermal zone > >>>> @@ -184,7 +185,8 @@ struct thermal_zone_device { > >>>> int prev_high_trip; > >>>> atomic_t need_update; > >>>> struct thermal_zone_device_ops *ops; > >>>> - struct thermal_zone_params *tzp; > >>>> + struct thermal_zone_platform_params *tzp; > >>>> + struct thermal_governor_params *tgp; > >>> > >>> I agree with doing a split here, but I'm not sure about moving items > >>> from the arg list to struct thermal_zone_platform_params (as mentioned > >>> above). > >>> > >>> Also the naming is quite inconsistent. IMO it would be better to call > >>> the first pointer "tzpp", rename struct thermal_governor_params to > >>> struct thermal_zone_governor_params and call the second pointer > >>> "tzgp". > >>> > >> > >> The names "tzgp" and "tzpp" look almost identical at first glance. > >> Could we increase the hamming distance somehow? > > > > Good point. > > > > They may as well be gov_params and platform_params AFAIAC. > > I'm more for gov_params and zone_params, as the thermal_zone_platform_params is > supposed to get renamed to struct thermal_zone_params in part 2. > > Anything against that? Nope, sounds good!
This series is a preparation for a bigger cleanup that will be split in three parts in order to avoid immutable branches on multiple subsystems, as in other parts of this series there will be changes in: - drivers/acpi - drivers/platform/x86 - drivers/power/supply - drivers/net/wireless - drivers/net/ethernet This is the first part which adds new structures and starts reorganizing struct members around, plus, this migrates all of the thermal drivers found in drivers/thermal/ to the new thermal_zone_device_register() function, and advertises that the old registration functions are deprecated and will be removed. The reorganization is supposed to be complete (or mostly) but... - struct thermal_zone_platform_params has a temporary name: this is done in order to avoid compile time failures for the drivers outside of drivers/thermal before migrating them to thermal_zone_device_params/thermal_zone_device_register(); - struct thermal_zone_params temporarily has two duplicated members, governor_name and no_hwmon; Part 2 of this topic will migrate all drivers that are external to drivers/thermal to thermal_zone_device_register(); I will send that part only after part 1 is confirmed to be acceptable, as otherwise I'd be spamming people for no reason :-) After all drivers will be migrated to thermal_zone_device_register(), we won't have to care about changing anything outside of drivers/thermal to finish this set of cleanups/changes (and no immutable branches needed) and this means that... Part 3 of this topic will contain the following changes: - thermal_zone_device_register_with_trips() will be removed - thermal_tripless_zone_device_register() will be removed - thermal_zone_params will be renamed to thermal_governor_ipa_params - governor_name, no_hwmon members will be removed - thermal_zone_platform_params will be renamed to thermal_zone_params - Removal of the THERMAL_NAME_LENGTH limitation for `type` More scheduled changes, which should end up in part 3 (at least that's my intention), or eventually entirely after this cleanup topic, include: - Introduction of Thermal Zone names - Disambiguation of TZ name and type - Addition of `thermal-zones`, `thermal-zone-names` parsing for devicetrees ... Summarizing ... Part 1: - Reorganize structures (some temporary names/leftovers) - New registration function, deprecation of old ones - Migration of drivers/thermal drivers to new registration Part 2: - Migration of drivers in other subsystems to new thermal registration Part 3: - Remove the two leftovers in thermal.h - Rename structures with proper, final names - Everything else, anyway. Cheers, Angelo AngeloGioacchino Del Regno (18): thermal: core: Change governor name to const char pointer thermal: Add new structures and thermal_zone_device_register() thermal: Directly use thermal_zone_platform_params for thermal_zone_device thermal/drivers/da9062: Migrate to thermal_zone_device_register() thermal/drivers/imx: Migrate to thermal_zone_device_register() thermal/drivers/rcar: Migrate to thermal_zone_device_register() thermal/drivers/st: Migrate to thermal_zone_device_register() thermal: intel: pch_thermal: Migrate to thermal_zone_device_register() thermal: intel: quark_dts: Migrate to thermal_zone_device_register() thermal: intel: soc_dts_iosf: Migrate to thermal_zone_device_register() thermal: intel: int340x: Migrate to thermal_zone_device_register() thermal: int340x: processor: Migrate to thermal_zone_device_register() thermal: intel: x86_pkg_temp: Migrate to thermal_zone_device_register() thermal/drivers/armada: Migrate to thermal_zone_device_register() thermal/drivers/dove: Migrate to thermal_zone_device_register() thermal/drivers/kirkwood: Migrate to thermal_zone_device_register() thermal/drivers/spear: Migrate to thermal_zone_device_register() thermal/drivers/int340x: Migrate to thermal_zone_device_register() drivers/thermal/armada_thermal.c | 12 +- drivers/thermal/da9062-thermal.c | 16 +- drivers/thermal/dove_thermal.c | 10 +- drivers/thermal/gov_power_allocator.c | 38 ++-- drivers/thermal/gov_user_space.c | 2 +- drivers/thermal/imx_thermal.c | 21 +- .../intel/int340x_thermal/int3400_thermal.c | 20 +- .../int340x_thermal/int340x_thermal_zone.c | 28 +-- .../processor_thermal_device_pci.c | 26 ++- drivers/thermal/intel/intel_pch_thermal.c | 12 +- .../thermal/intel/intel_quark_dts_thermal.c | 23 +- drivers/thermal/intel/intel_soc_dts_iosf.c | 24 ++- drivers/thermal/intel/x86_pkg_temp_thermal.c | 22 +- drivers/thermal/kirkwood_thermal.c | 10 +- drivers/thermal/qcom/tsens.c | 4 +- drivers/thermal/rcar_thermal.c | 15 +- drivers/thermal/spear_thermal.c | 10 +- drivers/thermal/st/st_thermal.c | 15 +- drivers/thermal/thermal_core.c | 201 +++++++++++------- drivers/thermal/thermal_core.h | 6 +- drivers/thermal/thermal_helpers.c | 22 +- drivers/thermal/thermal_hwmon.c | 8 +- drivers/thermal/thermal_of.c | 12 +- drivers/thermal/thermal_sysfs.c | 64 +++--- drivers/thermal/thermal_trace.h | 8 +- drivers/thermal/thermal_trip.c | 14 +- include/linux/thermal.h | 87 ++++++-- 27 files changed, 461 insertions(+), 269 deletions(-)