| Message ID | 20220703183059.4133659-1-daniel.lezcano@linexp.org |
|---|---|
| Headers | show |
| Series | thermal OF rework | expand |
On 7/3/22 19:30, Daniel Lezcano wrote: > As the thermal zone contains the trip point, we can store them > directly in the when registering the thermal zone. That will allow 'directly in the' is there something missing? maybe: in the thermal zone when registering > another step forward to remove the duplicate thermal zone structure we > find in the thermal_of code. > > Cc: Alexandre Bailon <abailon@baylibre.com> > Cc: Kevin Hilman <khilman@baylibre.com> > Cc; Eduardo Valentin <eduval@amazon.com> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linexp.org> > --- > drivers/thermal/thermal_of.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c > index 16eb18c24430..16b6b90a2390 100644 > --- a/drivers/thermal/thermal_of.c > +++ b/drivers/thermal/thermal_of.c > @@ -1117,11 +1117,9 @@ int __init of_parse_thermal_zones(void) > tzp->slope = tz->slope; > tzp->offset = tz->offset; > > - zone = thermal_zone_device_register(child->name, tz->ntrips, > - mask, tz, > - ops, tzp, > - tz->passive_delay, > - tz->polling_delay); > + zone = thermal_zone_device_register_with_trips(child->name, tz->trips, tz->ntrips, > + mask, tz, ops, tzp, tz->passive_delay, > + tz->polling_delay); > if (IS_ERR(zone)) { > pr_err("Failed to build %pOFn zone %ld\n", child, > PTR_ERR(zone)); 'num_trips' ? Apart from that LGTM
On Sun, 2022-07-03 at 20:30 +0200, Daniel Lezcano wrote: > Now that we have the thermal trip stored in the thermal zone in a > generic way, we can rely on them and remove one indirection we found > in the thermal_of code and do one more step forward the removal of > the > duplicated structures. > > Cc: Alexandre Bailon <abailon@baylibre.com> > Cc: Kevin Hilman <khilman@baylibre.com> > Cc; Eduardo Valentin <eduval@amazon.com> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linexp.org> > --- > drivers/thermal/thermal_of.c | 53 +++++++++++----------------------- > -- > 1 file changed, 16 insertions(+), 37 deletions(-) > > diff --git a/drivers/thermal/thermal_of.c > b/drivers/thermal/thermal_of.c > index 16b6b90a2390..bc885729bf23 100644 > --- a/drivers/thermal/thermal_of.c > +++ b/drivers/thermal/thermal_of.c > @@ -118,12 +118,7 @@ static int of_thermal_set_trips(struct > thermal_zone_device *tz, > */ > int of_thermal_get_ntrips(struct thermal_zone_device *tz) > { > - struct __thermal_zone *data = tz->devdata; > - > - if (!data || IS_ERR(data)) > - return -ENODEV; > - > - return data->ntrips; > + return tz->ntrips; > } > EXPORT_SYMBOL_GPL(of_thermal_get_ntrips); > > @@ -139,9 +134,7 @@ EXPORT_SYMBOL_GPL(of_thermal_get_ntrips); > */ > bool of_thermal_is_trip_valid(struct thermal_zone_device *tz, int > trip) > { > - struct __thermal_zone *data = tz->devdata; > - > - if (!data || trip >= data->ntrips || trip < 0) > + if (trip >= tz->ntrips || trip < 0) > return false; > > return true; > @@ -161,12 +154,7 @@ EXPORT_SYMBOL_GPL(of_thermal_is_trip_valid); > const struct thermal_trip * > of_thermal_get_trip_points(struct thermal_zone_device *tz) > { > - struct __thermal_zone *data = tz->devdata; > - > - if (!data) > - return NULL; > - > - return data->trips; > + return tz->trips; > } > EXPORT_SYMBOL_GPL(of_thermal_get_trip_points); what is the difference between of_thermal_get_ntrips/of_thermal_get_trip_points and thermal_zone_get_ntrips/thermal_zone_get_trips as introduced in this patch series? we need to remove the duplications. thanks, rui > > @@ -281,12 +269,10 @@ static int of_thermal_unbind(struct > thermal_zone_device *thermal, > static int of_thermal_get_trip_type(struct thermal_zone_device *tz, > int trip, > enum thermal_trip_type *type) > { > - struct __thermal_zone *data = tz->devdata; > - > - if (trip >= data->ntrips || trip < 0) > + if (trip >= tz->ntrips || trip < 0) > return -EDOM; > > - *type = data->trips[trip].type; > + *type = tz->trips[trip].type; > > return 0; > } > @@ -294,12 +280,10 @@ static int of_thermal_get_trip_type(struct > thermal_zone_device *tz, int trip, > static int of_thermal_get_trip_temp(struct thermal_zone_device *tz, > int trip, > int *temp) > { > - struct __thermal_zone *data = tz->devdata; > - > - if (trip >= data->ntrips || trip < 0) > + if (trip >= tz->ntrips || trip < 0) > return -EDOM; > > - *temp = data->trips[trip].temperature; > + *temp = tz->trips[trip].temperature; > > return 0; > } > @@ -309,7 +293,7 @@ static int of_thermal_set_trip_temp(struct > thermal_zone_device *tz, int trip, > { > struct __thermal_zone *data = tz->devdata; > > - if (trip >= data->ntrips || trip < 0) > + if (trip >= tz->ntrips || trip < 0) > return -EDOM; > > if (data->ops && data->ops->set_trip_temp) { > @@ -321,7 +305,7 @@ static int of_thermal_set_trip_temp(struct > thermal_zone_device *tz, int trip, > } > > /* thermal framework should take care of data->mask & (1 << > trip) */ > - data->trips[trip].temperature = temp; > + tz->trips[trip].temperature = temp; > > return 0; > } > @@ -329,12 +313,10 @@ static int of_thermal_set_trip_temp(struct > thermal_zone_device *tz, int trip, > static int of_thermal_get_trip_hyst(struct thermal_zone_device *tz, > int trip, > int *hyst) > { > - struct __thermal_zone *data = tz->devdata; > - > - if (trip >= data->ntrips || trip < 0) > + if (trip >= tz->ntrips || trip < 0) > return -EDOM; > > - *hyst = data->trips[trip].hysteresis; > + *hyst = tz->trips[trip].hysteresis; > > return 0; > } > @@ -342,13 +324,11 @@ static int of_thermal_get_trip_hyst(struct > thermal_zone_device *tz, int trip, > static int of_thermal_set_trip_hyst(struct thermal_zone_device *tz, > int trip, > int hyst) > { > - struct __thermal_zone *data = tz->devdata; > - > - if (trip >= data->ntrips || trip < 0) > + if (trip >= tz->ntrips || trip < 0) > return -EDOM; > > /* thermal framework should take care of data->mask & (1 << > trip) */ > - data->trips[trip].hysteresis = hyst; > + tz->trips[trip].hysteresis = hyst; > > return 0; > } > @@ -356,12 +336,11 @@ static int of_thermal_set_trip_hyst(struct > thermal_zone_device *tz, int trip, > static int of_thermal_get_crit_temp(struct thermal_zone_device *tz, > int *temp) > { > - struct __thermal_zone *data = tz->devdata; > int i; > > - for (i = 0; i < data->ntrips; i++) > - if (data->trips[i].type == THERMAL_TRIP_CRITICAL) { > - *temp = data->trips[i].temperature; > + for (i = 0; i < tz->ntrips; i++) > + if (tz->trips[i].type == THERMAL_TRIP_CRITICAL) { > + *temp = tz->trips[i].temperature; > return 0; > } >
On 04/07/2022 10:24, Lukasz Luba wrote: > > > On 7/3/22 19:30, Daniel Lezcano wrote: >> In order to use thermal trips defined in the thermal structure, rename >> the 'trips' field to 'ntrips' to have the 'trips' field containing the >> thermal trip points. >> >> Cc: Alexandre Bailon <abailon@baylibre.com> >> Cc: Kevin Hilman <khilman@baylibre.com> >> Cc; Eduardo Valentin <eduval@amazon.com> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linexp.org> >> --- >> drivers/thermal/gov_fair_share.c | 6 +++--- >> drivers/thermal/gov_power_allocator.c | 4 ++-- >> drivers/thermal/tegra/tegra30-tsensor.c | 2 +- >> drivers/thermal/thermal_core.c | 20 ++++++++++---------- >> drivers/thermal/thermal_helpers.c | 4 ++-- >> drivers/thermal/thermal_netlink.c | 2 +- >> drivers/thermal/thermal_sysfs.c | 22 +++++++++++----------- >> include/linux/thermal.h | 2 +- >> 8 files changed, 31 insertions(+), 31 deletions(-) > > > [snip] > >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h >> index 6289b0bb1c97..3a57878a2a6c 100644 >> --- a/include/linux/thermal.h >> +++ b/include/linux/thermal.h > > Missing updated ne name in comment here: > * @trips: number of trip points the thermal zone supports > > >> @@ -165,7 +165,7 @@ struct thermal_zone_device { >> struct thermal_attr *trip_hyst_attrs; >> enum thermal_device_mode mode; >> void *devdata; >> - int trips; >> + int ntrips; >> unsigned long trips_disabled; /* bitmap for disabled trips */ >> unsigned long passive_delay_jiffies; >> unsigned long polling_delay_jiffies; > > Maybe this is only my bias, but this new name 'ntrips' looks > like negation in electronics. > > We have examples like: num_cpus, num_pins, num_leds, num_groups, > num_locks, num_buffers, num_phys, etc... > > Could we have 'num_trips' and follow to this convention here as well? Sure, I'll do the changes accordingly
On 04/07/2022 16:14, Zhang Rui wrote: > On Sun, 2022-07-03 at 20:30 +0200, Daniel Lezcano wrote: >> Now that we have the thermal trip stored in the thermal zone in a >> generic way, we can rely on them and remove one indirection we found >> in the thermal_of code and do one more step forward the removal of >> the >> duplicated structures. >> >> Cc: Alexandre Bailon <abailon@baylibre.com> >> Cc: Kevin Hilman <khilman@baylibre.com> >> Cc; Eduardo Valentin <eduval@amazon.com> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linexp.org> >> --- [ ... ] >> EXPORT_SYMBOL_GPL(of_thermal_get_ntrips); >> >> @@ -139,9 +134,7 @@ EXPORT_SYMBOL_GPL(of_thermal_get_ntrips); >> */ >> bool of_thermal_is_trip_valid(struct thermal_zone_device *tz, int >> trip) >> { >> - struct __thermal_zone *data = tz->devdata; >> - >> - if (!data || trip >= data->ntrips || trip < 0) >> + if (trip >= tz->ntrips || trip < 0) >> return false; >> >> return true; >> @@ -161,12 +154,7 @@ EXPORT_SYMBOL_GPL(of_thermal_is_trip_valid); >> const struct thermal_trip * >> of_thermal_get_trip_points(struct thermal_zone_device *tz) >> { >> - struct __thermal_zone *data = tz->devdata; >> - >> - if (!data) >> - return NULL; >> - >> - return data->trips; >> + return tz->trips; >> } >> EXPORT_SYMBOL_GPL(of_thermal_get_trip_points); > > what is the difference between > of_thermal_get_ntrips/of_thermal_get_trip_points and > thermal_zone_get_ntrips/thermal_zone_get_trips as introduced in this > patch series? > > we need to remove the duplications. There is no difference between those functions. There are 34 more patches in the pipe to be sent after this series to do more cleanups and remove code duplication.
On Mon, 2022-07-04 at 23:24 +0200, Daniel Lezcano wrote: > On 04/07/2022 16:14, Zhang Rui wrote: > > On Sun, 2022-07-03 at 20:30 +0200, Daniel Lezcano wrote: > > > Now that we have the thermal trip stored in the thermal zone in a > > > generic way, we can rely on them and remove one indirection we > > > found > > > in the thermal_of code and do one more step forward the removal > > > of > > > the > > > duplicated structures. > > > > > > Cc: Alexandre Bailon <abailon@baylibre.com> > > > Cc: Kevin Hilman <khilman@baylibre.com> > > > Cc; Eduardo Valentin <eduval@amazon.com> > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linexp.org> > > > --- > > [ ... ] > > > > EXPORT_SYMBOL_GPL(of_thermal_get_ntrips); > > > > > > @@ -139,9 +134,7 @@ EXPORT_SYMBOL_GPL(of_thermal_get_ntrips); > > > */ > > > bool of_thermal_is_trip_valid(struct thermal_zone_device *tz, > > > int > > > trip) > > > { > > > - struct __thermal_zone *data = tz->devdata; > > > - > > > - if (!data || trip >= data->ntrips || trip < 0) > > > + if (trip >= tz->ntrips || trip < 0) > > > return false; > > > > > > return true; > > > @@ -161,12 +154,7 @@ EXPORT_SYMBOL_GPL(of_thermal_is_trip_valid); > > > const struct thermal_trip * > > > of_thermal_get_trip_points(struct thermal_zone_device *tz) > > > { > > > - struct __thermal_zone *data = tz->devdata; > > > - > > > - if (!data) > > > - return NULL; > > > - > > > - return data->trips; > > > + return tz->trips; > > > } > > > EXPORT_SYMBOL_GPL(of_thermal_get_trip_points); > > > > what is the difference between > > of_thermal_get_ntrips/of_thermal_get_trip_points and > > thermal_zone_get_ntrips/thermal_zone_get_trips as introduced in > > this > > patch series? > > > > we need to remove the duplications. > > There is no difference between those functions. There are 34 more > patches in the pipe to be sent after this series to do more cleanups > and > remove code duplication. > Good to know. It would be nice to have a cover letter to describe the whole picture, including this patch series and the following patches in your queue. thanks, rui
On 05/07/2022 03:20, Zhang Rui wrote: [ ... ] >> There is no difference between those functions. There are 34 more >> patches in the pipe to be sent after this series to do more cleanups >> and >> remove code duplication. >> > Good to know. > > It would be nice to have a cover letter to describe the whole picture, > including this patch series and the following patches in your queue. https://lore.kernel.org/lkml/20220703183059.4133659-4-daniel.lezcano@linexp.org/T/ You will Cc'ed next time ;)
On Tue, 2022-07-05 at 08:44 +0200, Daniel Lezcano wrote: > On 05/07/2022 03:20, Zhang Rui wrote: > > [ ... ] > > > > There is no difference between those functions. There are 34 more > > > patches in the pipe to be sent after this series to do more > > > cleanups > > > and > > > remove code duplication. > > > > > > > Good to know. > > > > It would be nice to have a cover letter to describe the whole > > picture, > > including this patch series and the following patches in your > > queue. > > https://lore.kernel.org/lkml/20220703183059.4133659-4-daniel.lezcano@linexp.org/T/ > > You will Cc'ed next time ;) > Cool, thanks! -rui
The thermal framework initialization with the device tree appears to be complicated and hard to make it to evolve. It contains duplication of almost the same thermal generic structures and has an assymetric initialization making hard any kind of serious changes for more complex features. One of them is the multiple sensors support per thermal zone. In order to set the scene for the aforementioned feature with generic code, we need to cleanup and rework the device tree initialization. However this rework is not obvious because of the multiple components entering in the composition of a thermal zone and being initialized at different moments. For instance, a cooling device can be initialized before a sensor, so the thermal zones must exist before the cooling device as well as the sensor. This asynchronous initialization forces the thermal zone to be created with fake ops because they are mandotory and build a list of cooling devices which is used to lookup afterwards when the cooling device driver is registering itself. As there could be a large number of changes, this first series provide some steps forward for a simpler device tree initialization. Changelog: - V3: - Removed patch 1 and 2 from the V2 which consist in renaming the thermal_zone_device_ops to thermal_sensor_ops and separating the structure. I'll do a separate proposal for that after the incoming cleanups - V2: - Drop patch 1/15 which contains too many changes for a simple structure renaming. This could be addressed in a separate series as it is not necessary for the OF rework - Fixed of_node_put with gchild not initialized as reported by kbuild and Dan Carpenter - V1: - Initial post Daniel Lezcano (12): thermal/core: Remove duplicate information when an error occurs thermal/of: Replace device node match with device node search thermal/of: Remove the device node pointer for thermal_trip thermal/of: Move thermal_trip structure to thermal.h thermal/core: Remove unneeded EXPORT_SYMBOLS thermal/core: Move thermal_set_delay_jiffies to static thermal/core: Rename trips to ntrips thermal/core: Add thermal_trip in thermal_zone thermal/core: Register with the trip points thermal/of: Store the trips in the thermal zone thermal/of: Use thermal trips stored in the thermal zone thermal/of: Initialize trip points separately drivers/thermal/gov_fair_share.c | 6 +- drivers/thermal/gov_power_allocator.c | 4 +- drivers/thermal/tegra/tegra30-tsensor.c | 2 +- drivers/thermal/thermal_core.c | 53 +++++-- drivers/thermal/thermal_core.h | 25 ++- drivers/thermal/thermal_helpers.c | 13 +- drivers/thermal/thermal_netlink.c | 2 +- drivers/thermal/thermal_of.c | 201 +++++++++++++----------- drivers/thermal/thermal_sysfs.c | 22 +-- include/linux/thermal.h | 21 ++- 10 files changed, 197 insertions(+), 152 deletions(-)