From: Daniel Lezcano <[email protected]>
The thermal framework gives the possibility to register the trip
points with the thermal zone. When that is done, no get_trip_* ops are
needed and they can be removed.
Convert ops content logic into generic trip points and register them with the
thermal zone.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/intel/intel_soc_dts_iosf.c | 58 ++++++++--------------
drivers/thermal/intel/intel_soc_dts_iosf.h | 2 +-
2 files changed, 23 insertions(+), 37 deletions(-)
diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c
index 342b0bb5a56d..130c416ec601 100644
--- a/drivers/thermal/intel/intel_soc_dts_iosf.c
+++ b/drivers/thermal/intel/intel_soc_dts_iosf.c
@@ -71,20 +71,13 @@ static int get_tj_max(u32 *tj_max)
return err;
}
-static int sys_get_trip_temp(struct thermal_zone_device *tzd, int trip,
- int *temp)
+static int get_trip_temp(struct intel_soc_dts_sensors *sensors, int trip, int *temp)
{
int status;
u32 out;
- struct intel_soc_dts_sensor_entry *dts;
- struct intel_soc_dts_sensors *sensors;
- dts = tzd->devdata;
- sensors = dts->sensors;
- mutex_lock(&sensors->dts_update_lock);
status = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
SOC_DTS_OFFSET_PTPS, &out);
- mutex_unlock(&sensors->dts_update_lock);
if (status)
return status;
@@ -173,8 +166,13 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
if (status)
goto err_restore_te_out;
- dts->trip_types[thres_index] = trip_type;
-
+ status = get_trip_temp(sensors, thres_index, &temp);
+ if (status)
+ goto err_restore_te_out;
+
+ dts->trips[thres_index].type = trip_type;
+ dts->trips[thres_index].temperature = temp;
+
return 0;
err_restore_te_out:
iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
@@ -202,24 +200,12 @@ static int sys_set_trip_temp(struct thermal_zone_device *tzd, int trip,
mutex_lock(&sensors->dts_update_lock);
status = update_trip_temp(tzd->devdata, trip, temp,
- dts->trip_types[trip]);
+ dts->trips[trip].type);
mutex_unlock(&sensors->dts_update_lock);
return status;
}
-static int sys_get_trip_type(struct thermal_zone_device *tzd,
- int trip, enum thermal_trip_type *type)
-{
- struct intel_soc_dts_sensor_entry *dts;
-
- dts = tzd->devdata;
-
- *type = dts->trip_types[trip];
-
- return 0;
-}
-
static int sys_get_curr_temp(struct thermal_zone_device *tzd,
int *temp)
{
@@ -245,8 +231,6 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
static struct thermal_zone_device_ops tzone_ops = {
.get_temp = sys_get_curr_temp,
- .get_trip_temp = sys_get_trip_temp,
- .get_trip_type = sys_get_trip_type,
.set_trip_temp = sys_set_trip_temp,
};
@@ -320,7 +304,8 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
dts->trip_mask = trip_mask;
dts->trip_count = trip_count;
snprintf(name, sizeof(name), "soc_dts%d", id);
- dts->tzone = thermal_zone_device_register(name,
+ dts->tzone = thermal_zone_device_register_with_trips(name,
+ dts->trips,
trip_count,
trip_mask,
dts, &tzone_ops,
@@ -430,27 +415,28 @@ struct intel_soc_dts_sensors *intel_soc_dts_iosf_init(
notification = false;
else
notification = true;
- for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
- sensors->soc_dts[i].sensors = sensors;
- ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
- notification, trip_count,
- read_only_trip_count);
- if (ret)
- goto err_free;
- }
for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
ret = update_trip_temp(&sensors->soc_dts[i], 0, 0,
THERMAL_TRIP_PASSIVE);
if (ret)
- goto err_remove_zone;
+ goto err_free;
ret = update_trip_temp(&sensors->soc_dts[i], 1, 0,
THERMAL_TRIP_PASSIVE);
if (ret)
- goto err_remove_zone;
+ goto err_free;
}
+ for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
+ sensors->soc_dts[i].sensors = sensors;
+ ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
+ notification, trip_count,
+ read_only_trip_count);
+ if (ret)
+ goto err_remove_zone;
+ }
+
return sensors;
err_remove_zone:
for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i)
diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.h b/drivers/thermal/intel/intel_soc_dts_iosf.h
index c54945748200..ee0a39e3edd3 100644
--- a/drivers/thermal/intel/intel_soc_dts_iosf.h
+++ b/drivers/thermal/intel/intel_soc_dts_iosf.h
@@ -27,7 +27,7 @@ struct intel_soc_dts_sensor_entry {
u32 store_status;
u32 trip_mask;
u32 trip_count;
- enum thermal_trip_type trip_types[2];
+ struct thermal_trip trips[2];
struct thermal_zone_device *tzone;
struct intel_soc_dts_sensors *sensors;
};
--
2.34.1
On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
<[email protected]> wrote:
>
> From: Daniel Lezcano <[email protected]>
>
> The thermal framework gives the possibility to register the trip
> points with the thermal zone. When that is done, no get_trip_* ops are
> needed and they can be removed.
>
> Convert ops content logic into generic trip points and register them with the
> thermal zone.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/intel/intel_soc_dts_iosf.c | 58 ++++++++--------------
> drivers/thermal/intel/intel_soc_dts_iosf.h | 2 +-
> 2 files changed, 23 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c
> index 342b0bb5a56d..130c416ec601 100644
> --- a/drivers/thermal/intel/intel_soc_dts_iosf.c
> +++ b/drivers/thermal/intel/intel_soc_dts_iosf.c
> @@ -71,20 +71,13 @@ static int get_tj_max(u32 *tj_max)
> return err;
> }
>
> -static int sys_get_trip_temp(struct thermal_zone_device *tzd, int trip,
> - int *temp)
> +static int get_trip_temp(struct intel_soc_dts_sensors *sensors, int trip, int *temp)
> {
> int status;
> u32 out;
> - struct intel_soc_dts_sensor_entry *dts;
> - struct intel_soc_dts_sensors *sensors;
>
> - dts = tzd->devdata;
> - sensors = dts->sensors;
> - mutex_lock(&sensors->dts_update_lock);
> status = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> SOC_DTS_OFFSET_PTPS, &out);
> - mutex_unlock(&sensors->dts_update_lock);
> if (status)
> return status;
>
> @@ -173,8 +166,13 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
> if (status)
> goto err_restore_te_out;
>
> - dts->trip_types[thres_index] = trip_type;
> -
> + status = get_trip_temp(sensors, thres_index, &temp);
> + if (status)
> + goto err_restore_te_out;
> +
> + dts->trips[thres_index].type = trip_type;
> + dts->trips[thres_index].temperature = temp;
> +
> return 0;
> err_restore_te_out:
> iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> @@ -202,24 +200,12 @@ static int sys_set_trip_temp(struct thermal_zone_device *tzd, int trip,
>
> mutex_lock(&sensors->dts_update_lock);
> status = update_trip_temp(tzd->devdata, trip, temp,
> - dts->trip_types[trip]);
> + dts->trips[trip].type);
> mutex_unlock(&sensors->dts_update_lock);
>
> return status;
> }
>
> -static int sys_get_trip_type(struct thermal_zone_device *tzd,
> - int trip, enum thermal_trip_type *type)
> -{
> - struct intel_soc_dts_sensor_entry *dts;
> -
> - dts = tzd->devdata;
> -
> - *type = dts->trip_types[trip];
> -
> - return 0;
> -}
> -
> static int sys_get_curr_temp(struct thermal_zone_device *tzd,
> int *temp)
> {
> @@ -245,8 +231,6 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
>
> static struct thermal_zone_device_ops tzone_ops = {
> .get_temp = sys_get_curr_temp,
> - .get_trip_temp = sys_get_trip_temp,
> - .get_trip_type = sys_get_trip_type,
> .set_trip_temp = sys_set_trip_temp,
> };
>
> @@ -320,7 +304,8 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
> dts->trip_mask = trip_mask;
> dts->trip_count = trip_count;
> snprintf(name, sizeof(name), "soc_dts%d", id);
> - dts->tzone = thermal_zone_device_register(name,
> + dts->tzone = thermal_zone_device_register_with_trips(name,
> + dts->trips,
> trip_count,
> trip_mask,
> dts, &tzone_ops,
> @@ -430,27 +415,28 @@ struct intel_soc_dts_sensors *intel_soc_dts_iosf_init(
> notification = false;
> else
> notification = true;
> - for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
> - sensors->soc_dts[i].sensors = sensors;
> - ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
> - notification, trip_count,
> - read_only_trip_count);
> - if (ret)
> - goto err_free;
> - }
How is this change related to the rest of the patch?
>
> for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
> ret = update_trip_temp(&sensors->soc_dts[i], 0, 0,
> THERMAL_TRIP_PASSIVE);
> if (ret)
> - goto err_remove_zone;
> + goto err_free;
>
> ret = update_trip_temp(&sensors->soc_dts[i], 1, 0,
> THERMAL_TRIP_PASSIVE);
> if (ret)
> - goto err_remove_zone;
> + goto err_free;
> }
>
> + for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
> + sensors->soc_dts[i].sensors = sensors;
> + ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
> + notification, trip_count,
> + read_only_trip_count);
> + if (ret)
> + goto err_remove_zone;
> + }
> +
> return sensors;
> err_remove_zone:
> for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i)
> diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.h b/drivers/thermal/intel/intel_soc_dts_iosf.h
> index c54945748200..ee0a39e3edd3 100644
> --- a/drivers/thermal/intel/intel_soc_dts_iosf.h
> +++ b/drivers/thermal/intel/intel_soc_dts_iosf.h
> @@ -27,7 +27,7 @@ struct intel_soc_dts_sensor_entry {
> u32 store_status;
> u32 trip_mask;
> u32 trip_count;
> - enum thermal_trip_type trip_types[2];
> + struct thermal_trip trips[2];
> struct thermal_zone_device *tzone;
> struct intel_soc_dts_sensors *sensors;
> };
> --
Hi Srinivas,
On 19/01/2023 21:04, Rafael J. Wysocki wrote:
> On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>> From: Daniel Lezcano <[email protected]>
>>
>> The thermal framework gives the possibility to register the trip
>> points with the thermal zone. When that is done, no get_trip_* ops are
>> needed and they can be removed.
>>
>> Convert ops content logic into generic trip points and register them with the
>> thermal zone.
>>
>> Signed-off-by: Daniel Lezcano <[email protected]>
>> ---
[ ... ]
>>
>> @@ -320,7 +304,8 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
>> dts->trip_mask = trip_mask;
>> dts->trip_count = trip_count;
>> snprintf(name, sizeof(name), "soc_dts%d", id);
>> - dts->tzone = thermal_zone_device_register(name,
>> + dts->tzone = thermal_zone_device_register_with_trips(name,
>> + dts->trips,
>> trip_count,
>> trip_mask,
>> dts, &tzone_ops,
>> @@ -430,27 +415,28 @@ struct intel_soc_dts_sensors *intel_soc_dts_iosf_init(
>> notification = false;
>> else
>> notification = true;
>> - for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
>> - sensors->soc_dts[i].sensors = sensors;
>> - ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
>> - notification, trip_count,
>> - read_only_trip_count);
>> - if (ret)
>> - goto err_free;
>> - }
>
> How is this change related to the rest of the patch?
We want to register the thermal zone with the trip points.
thermal_zone_device_register() becomes
thermal_zone_device_register_with_trips()
But in the current code, the trip points are updated after the thermal
zones are created (and strictly speaking it is wrong as get_trip_temp
can be invoked before the trip points are updated).
So the change inverts the initialization where we update the trip points
and then we register the thermal zones.
>>
>> for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
>> ret = update_trip_temp(&sensors->soc_dts[i], 0, 0,
>> THERMAL_TRIP_PASSIVE);
>> if (ret)
>> - goto err_remove_zone;
>> + goto err_free;
>>
>> ret = update_trip_temp(&sensors->soc_dts[i], 1, 0,
>> THERMAL_TRIP_PASSIVE);
>> if (ret)
>> - goto err_remove_zone;
>> + goto err_free;
>> }
>>
>> + for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
>> + sensors->soc_dts[i].sensors = sensors;
>> + ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
>> + notification, trip_count,
>> + read_only_trip_count);
>> + if (ret)
>> + goto err_remove_zone;
>> + }
>> +
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Mon, Jan 23, 2023 at 7:09 PM Daniel Lezcano
<[email protected]> wrote:
>
>
> Hi Srinivas,
>
> On 19/01/2023 21:04, Rafael J. Wysocki wrote:
> > On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
> > <[email protected]> wrote:
> >>
> >> From: Daniel Lezcano <[email protected]>
> >>
> >> The thermal framework gives the possibility to register the trip
> >> points with the thermal zone. When that is done, no get_trip_* ops are
> >> needed and they can be removed.
> >>
> >> Convert ops content logic into generic trip points and register them with the
> >> thermal zone.
> >>
> >> Signed-off-by: Daniel Lezcano <[email protected]>
> >> ---
>
> [ ... ]
>
> >>
> >> @@ -320,7 +304,8 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
> >> dts->trip_mask = trip_mask;
> >> dts->trip_count = trip_count;
> >> snprintf(name, sizeof(name), "soc_dts%d", id);
> >> - dts->tzone = thermal_zone_device_register(name,
> >> + dts->tzone = thermal_zone_device_register_with_trips(name,
> >> + dts->trips,
> >> trip_count,
> >> trip_mask,
> >> dts, &tzone_ops,
> >> @@ -430,27 +415,28 @@ struct intel_soc_dts_sensors *intel_soc_dts_iosf_init(
> >> notification = false;
> >> else
> >> notification = true;
> >> - for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
> >> - sensors->soc_dts[i].sensors = sensors;
> >> - ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
> >> - notification, trip_count,
> >> - read_only_trip_count);
> >> - if (ret)
> >> - goto err_free;
> >> - }
> >
> > How is this change related to the rest of the patch?
>
> We want to register the thermal zone with the trip points.
>
> thermal_zone_device_register() becomes
>
> thermal_zone_device_register_with_trips()
>
> But in the current code, the trip points are updated after the thermal
> zones are created (and strictly speaking it is wrong as get_trip_temp
> can be invoked before the trip points are updated).
>
> So the change inverts the initialization where we update the trip points
> and then we register the thermal zones.
It would be nice to write this in the changelog too.
> >>
> >> for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
> >> ret = update_trip_temp(&sensors->soc_dts[i], 0, 0,
> >> THERMAL_TRIP_PASSIVE);
> >> if (ret)
> >> - goto err_remove_zone;
> >> + goto err_free;
> >>
> >> ret = update_trip_temp(&sensors->soc_dts[i], 1, 0,
> >> THERMAL_TRIP_PASSIVE);
> >> if (ret)
> >> - goto err_remove_zone;
> >> + goto err_free;
> >> }
> >>
> >> + for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
> >> + sensors->soc_dts[i].sensors = sensors;
> >> + ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
> >> + notification, trip_count,
> >> + read_only_trip_count);
> >> + if (ret)
> >> + goto err_remove_zone;
> >> + }
> >> +
>
> --
On 23/01/2023 21:19, Rafael J. Wysocki wrote:
> On Mon, Jan 23, 2023 at 7:09 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>>
>> Hi Srinivas,
>>
>> On 19/01/2023 21:04, Rafael J. Wysocki wrote:
>>> On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
>>> <[email protected]> wrote:
>>>>
>>>> From: Daniel Lezcano <[email protected]>
>>>>
>>>> The thermal framework gives the possibility to register the trip
>>>> points with the thermal zone. When that is done, no get_trip_* ops are
>>>> needed and they can be removed.
>>>>
>>>> Convert ops content logic into generic trip points and register them with the
>>>> thermal zone.
>>>>
>>>> Signed-off-by: Daniel Lezcano <[email protected]>
>>>> ---
>>
>> [ ... ]
>>
>>>>
>>>> @@ -320,7 +304,8 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
>>>> dts->trip_mask = trip_mask;
>>>> dts->trip_count = trip_count;
>>>> snprintf(name, sizeof(name), "soc_dts%d", id);
>>>> - dts->tzone = thermal_zone_device_register(name,
>>>> + dts->tzone = thermal_zone_device_register_with_trips(name,
>>>> + dts->trips,
>>>> trip_count,
>>>> trip_mask,
>>>> dts, &tzone_ops,
>>>> @@ -430,27 +415,28 @@ struct intel_soc_dts_sensors *intel_soc_dts_iosf_init(
>>>> notification = false;
>>>> else
>>>> notification = true;
>>>> - for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
>>>> - sensors->soc_dts[i].sensors = sensors;
>>>> - ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
>>>> - notification, trip_count,
>>>> - read_only_trip_count);
>>>> - if (ret)
>>>> - goto err_free;
>>>> - }
>>>
>>> How is this change related to the rest of the patch?
>>
>> We want to register the thermal zone with the trip points.
>>
>> thermal_zone_device_register() becomes
>>
>> thermal_zone_device_register_with_trips()
>>
>> But in the current code, the trip points are updated after the thermal
>> zones are created (and strictly speaking it is wrong as get_trip_temp
>> can be invoked before the trip points are updated).
>>
>> So the change inverts the initialization where we update the trip points
>> and then we register the thermal zones.
>
> It would be nice to write this in the changelog too.
Srinivasn, are you fine with the changes ?
Rafael, if the patches are ok, shall I resend a new version with the
changelog updated or can you pick them amending the changelog ?
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Tue, Jan 24, 2023 at 11:28 AM Daniel Lezcano
<[email protected]> wrote:
>
> On 23/01/2023 21:19, Rafael J. Wysocki wrote:
> > On Mon, Jan 23, 2023 at 7:09 PM Daniel Lezcano
> > <[email protected]> wrote:
> >>
> >>
> >> Hi Srinivas,
> >>
> >> On 19/01/2023 21:04, Rafael J. Wysocki wrote:
> >>> On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
> >>> <[email protected]> wrote:
> >>>>
> >>>> From: Daniel Lezcano <[email protected]>
> >>>>
> >>>> The thermal framework gives the possibility to register the trip
> >>>> points with the thermal zone. When that is done, no get_trip_* ops are
> >>>> needed and they can be removed.
> >>>>
> >>>> Convert ops content logic into generic trip points and register them with the
> >>>> thermal zone.
> >>>>
> >>>> Signed-off-by: Daniel Lezcano <[email protected]>
> >>>> ---
> >>
> >> [ ... ]
> >>
> >>>>
> >>>> @@ -320,7 +304,8 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
> >>>> dts->trip_mask = trip_mask;
> >>>> dts->trip_count = trip_count;
> >>>> snprintf(name, sizeof(name), "soc_dts%d", id);
> >>>> - dts->tzone = thermal_zone_device_register(name,
> >>>> + dts->tzone = thermal_zone_device_register_with_trips(name,
> >>>> + dts->trips,
> >>>> trip_count,
> >>>> trip_mask,
> >>>> dts, &tzone_ops,
> >>>> @@ -430,27 +415,28 @@ struct intel_soc_dts_sensors *intel_soc_dts_iosf_init(
> >>>> notification = false;
> >>>> else
> >>>> notification = true;
> >>>> - for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
> >>>> - sensors->soc_dts[i].sensors = sensors;
> >>>> - ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
> >>>> - notification, trip_count,
> >>>> - read_only_trip_count);
> >>>> - if (ret)
> >>>> - goto err_free;
> >>>> - }
> >>>
> >>> How is this change related to the rest of the patch?
> >>
> >> We want to register the thermal zone with the trip points.
> >>
> >> thermal_zone_device_register() becomes
> >>
> >> thermal_zone_device_register_with_trips()
> >>
> >> But in the current code, the trip points are updated after the thermal
> >> zones are created (and strictly speaking it is wrong as get_trip_temp
> >> can be invoked before the trip points are updated).
> >>
> >> So the change inverts the initialization where we update the trip points
> >> and then we register the thermal zones.
> >
> > It would be nice to write this in the changelog too.
>
> Srinivasn, are you fine with the changes ?
>
> Rafael, if the patches are ok, shall I resend a new version with the
> changelog updated or can you pick them amending the changelog ?
I can pick them up, but I would like to hear from Srinivas first.
On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
<[email protected]> wrote:
>
> From: Daniel Lezcano <[email protected]>
>
> The thermal framework gives the possibility to register the trip
> points with the thermal zone. When that is done, no get_trip_* ops are
> needed and they can be removed.
>
> Convert ops content logic into generic trip points and register them with the
> thermal zone.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/intel/intel_soc_dts_iosf.c | 58 ++++++++--------------
> drivers/thermal/intel/intel_soc_dts_iosf.h | 2 +-
> 2 files changed, 23 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.c b/drivers/thermal/intel/intel_soc_dts_iosf.c
> index 342b0bb5a56d..130c416ec601 100644
> --- a/drivers/thermal/intel/intel_soc_dts_iosf.c
> +++ b/drivers/thermal/intel/intel_soc_dts_iosf.c
> @@ -71,20 +71,13 @@ static int get_tj_max(u32 *tj_max)
> return err;
> }
>
> -static int sys_get_trip_temp(struct thermal_zone_device *tzd, int trip,
> - int *temp)
> +static int get_trip_temp(struct intel_soc_dts_sensors *sensors, int trip, int *temp)
> {
> int status;
> u32 out;
> - struct intel_soc_dts_sensor_entry *dts;
> - struct intel_soc_dts_sensors *sensors;
>
> - dts = tzd->devdata;
> - sensors = dts->sensors;
> - mutex_lock(&sensors->dts_update_lock);
> status = iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> SOC_DTS_OFFSET_PTPS, &out);
> - mutex_unlock(&sensors->dts_update_lock);
> if (status)
> return status;
>
> @@ -173,8 +166,13 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
> if (status)
> goto err_restore_te_out;
>
> - dts->trip_types[thres_index] = trip_type;
> -
> + status = get_trip_temp(sensors, thres_index, &temp);
> + if (status)
> + goto err_restore_te_out;
> +
> + dts->trips[thres_index].type = trip_type;
> + dts->trips[thres_index].temperature = temp;
This change doesn't look correct to me, because this function takes
temp as an argument and it is used to populate the trip with it at
least in some cases.
Why should temp be overwritten here?
> +
> return 0;
> err_restore_te_out:
> iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> @@ -202,24 +200,12 @@ static int sys_set_trip_temp(struct thermal_zone_device *tzd, int trip,
>
> mutex_lock(&sensors->dts_update_lock);
> status = update_trip_temp(tzd->devdata, trip, temp,
> - dts->trip_types[trip]);
> + dts->trips[trip].type);
> mutex_unlock(&sensors->dts_update_lock);
>
> return status;
> }
>
> -static int sys_get_trip_type(struct thermal_zone_device *tzd,
> - int trip, enum thermal_trip_type *type)
> -{
> - struct intel_soc_dts_sensor_entry *dts;
> -
> - dts = tzd->devdata;
> -
> - *type = dts->trip_types[trip];
> -
> - return 0;
> -}
> -
> static int sys_get_curr_temp(struct thermal_zone_device *tzd,
> int *temp)
> {
> @@ -245,8 +231,6 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd,
>
> static struct thermal_zone_device_ops tzone_ops = {
> .get_temp = sys_get_curr_temp,
> - .get_trip_temp = sys_get_trip_temp,
> - .get_trip_type = sys_get_trip_type,
> .set_trip_temp = sys_set_trip_temp,
> };
>
> @@ -320,7 +304,8 @@ static int add_dts_thermal_zone(int id, struct intel_soc_dts_sensor_entry *dts,
> dts->trip_mask = trip_mask;
> dts->trip_count = trip_count;
> snprintf(name, sizeof(name), "soc_dts%d", id);
> - dts->tzone = thermal_zone_device_register(name,
> + dts->tzone = thermal_zone_device_register_with_trips(name,
> + dts->trips,
> trip_count,
> trip_mask,
> dts, &tzone_ops,
> @@ -430,27 +415,28 @@ struct intel_soc_dts_sensors *intel_soc_dts_iosf_init(
> notification = false;
> else
> notification = true;
> - for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
> - sensors->soc_dts[i].sensors = sensors;
> - ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
> - notification, trip_count,
> - read_only_trip_count);
> - if (ret)
> - goto err_free;
> - }
>
> for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
> ret = update_trip_temp(&sensors->soc_dts[i], 0, 0,
> THERMAL_TRIP_PASSIVE);
> if (ret)
> - goto err_remove_zone;
> + goto err_free;
>
> ret = update_trip_temp(&sensors->soc_dts[i], 1, 0,
> THERMAL_TRIP_PASSIVE);
> if (ret)
> - goto err_remove_zone;
> + goto err_free;
> }
>
> + for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i) {
> + sensors->soc_dts[i].sensors = sensors;
> + ret = add_dts_thermal_zone(i, &sensors->soc_dts[i],
> + notification, trip_count,
> + read_only_trip_count);
> + if (ret)
> + goto err_remove_zone;
> + }
> +
> return sensors;
> err_remove_zone:
> for (i = 0; i < SOC_MAX_DTS_SENSORS; ++i)
> diff --git a/drivers/thermal/intel/intel_soc_dts_iosf.h b/drivers/thermal/intel/intel_soc_dts_iosf.h
> index c54945748200..ee0a39e3edd3 100644
> --- a/drivers/thermal/intel/intel_soc_dts_iosf.h
> +++ b/drivers/thermal/intel/intel_soc_dts_iosf.h
> @@ -27,7 +27,7 @@ struct intel_soc_dts_sensor_entry {
> u32 store_status;
> u32 trip_mask;
> u32 trip_count;
> - enum thermal_trip_type trip_types[2];
> + struct thermal_trip trips[2];
> struct thermal_zone_device *tzone;
> struct intel_soc_dts_sensors *sensors;
> };
> --
> 2.34.1
>
Hi Rafael,
On 26/01/2023 17:47, Rafael J. Wysocki wrote:
> On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>> From: Daniel Lezcano <[email protected]>
>>
>> The thermal framework gives the possibility to register the trip
>> points with the thermal zone. When that is done, no get_trip_* ops are
>> needed and they can be removed.
>>
>> Convert ops content logic into generic trip points and register them with the
>> thermal zone.
>>
>> Signed-off-by: Daniel Lezcano <[email protected]>
>> ---
[ ... ]
>> @@ -173,8 +166,13 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
>> if (status)
>> goto err_restore_te_out;
>>
>> - dts->trip_types[thres_index] = trip_type;
>> -
>> + status = get_trip_temp(sensors, thres_index, &temp);
>> + if (status)
>> + goto err_restore_te_out;
>> +
>> + dts->trips[thres_index].type = trip_type;
>> + dts->trips[thres_index].temperature = temp;
>
> This change doesn't look correct to me, because this function takes
> temp as an argument and it is used to populate the trip with it at
> least in some cases.
>
> Why should temp be overwritten here?
You are correct. This is wrong.
I think we should call get_trip_temp() before calling update_trip_temp()
instead of passing a zero temperature parameter
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Tue, Jan 31, 2023 at 6:03 PM Daniel Lezcano
<[email protected]> wrote:
>
>
> Hi Rafael,
>
>
> On 26/01/2023 17:47, Rafael J. Wysocki wrote:
> > On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
> > <[email protected]> wrote:
> >>
> >> From: Daniel Lezcano <[email protected]>
> >>
> >> The thermal framework gives the possibility to register the trip
> >> points with the thermal zone. When that is done, no get_trip_* ops are
> >> needed and they can be removed.
> >>
> >> Convert ops content logic into generic trip points and register them with the
> >> thermal zone.
> >>
> >> Signed-off-by: Daniel Lezcano <[email protected]>
> >> ---
>
> [ ... ]
>
> >> @@ -173,8 +166,13 @@ static int update_trip_temp(struct intel_soc_dts_sensor_entry *dts,
> >> if (status)
> >> goto err_restore_te_out;
> >>
> >> - dts->trip_types[thres_index] = trip_type;
> >> -
> >> + status = get_trip_temp(sensors, thres_index, &temp);
> >> + if (status)
> >> + goto err_restore_te_out;
> >> +
> >> + dts->trips[thres_index].type = trip_type;
> >> + dts->trips[thres_index].temperature = temp;
> >
> > This change doesn't look correct to me, because this function takes
> > temp as an argument and it is used to populate the trip with it at
> > least in some cases.
> >
> > Why should temp be overwritten here?
>
> You are correct. This is wrong.
>
> I think we should call get_trip_temp() before calling update_trip_temp()
> instead of passing a zero temperature parameter
update_trip_temp() is sort of a misnomer, because it is used for
initializing a trip point for example in
intel_soc_dts_iosf_add_read_only_critical_trip() and in this
particular case get_trip_temp() need not be called before it.
This driver seems to be in need of a cleanup.
Hi Rafael,
On 31/01/2023 20:17, Rafael J. Wysocki wrote:
[ ... ]
>>> Why should temp be overwritten here?
>>
>> You are correct. This is wrong.
>>
>> I think we should call get_trip_temp() before calling update_trip_temp()
>> instead of passing a zero temperature parameter
>
> update_trip_temp() is sort of a misnomer, because it is used for
> initializing a trip point for example in
> intel_soc_dts_iosf_add_read_only_critical_trip() and in this
> particular case get_trip_temp() need not be called before it.
>
> This driver seems to be in need of a cleanup.
Will you take care of this cleanup ?
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Thu, Feb 2, 2023 at 3:36 PM Daniel Lezcano <[email protected]> wrote:
>
>
> Hi Rafael,
>
> On 31/01/2023 20:17, Rafael J. Wysocki wrote:
>
> [ ... ]
>
> >>> Why should temp be overwritten here?
> >>
> >> You are correct. This is wrong.
> >>
> >> I think we should call get_trip_temp() before calling update_trip_temp()
> >> instead of passing a zero temperature parameter
> >
> > update_trip_temp() is sort of a misnomer, because it is used for
> > initializing a trip point for example in
> > intel_soc_dts_iosf_add_read_only_critical_trip() and in this
> > particular case get_trip_temp() need not be called before it.
> >
> > This driver seems to be in need of a cleanup.
>
> Will you take care of this cleanup ?
I think I can do that, but I'm not sure how much time I will be able
to allocate for that. Let me try though.
On 02/02/2023 15:43, Rafael J. Wysocki wrote:
> On Thu, Feb 2, 2023 at 3:36 PM Daniel Lezcano <[email protected]> wrote:
>>
>>
>> Hi Rafael,
>>
>> On 31/01/2023 20:17, Rafael J. Wysocki wrote:
>>
>> [ ... ]
>>
>>>>> Why should temp be overwritten here?
>>>>
>>>> You are correct. This is wrong.
>>>>
>>>> I think we should call get_trip_temp() before calling update_trip_temp()
>>>> instead of passing a zero temperature parameter
>>>
>>> update_trip_temp() is sort of a misnomer, because it is used for
>>> initializing a trip point for example in
>>> intel_soc_dts_iosf_add_read_only_critical_trip() and in this
>>> particular case get_trip_temp() need not be called before it.
>>>
>>> This driver seems to be in need of a cleanup.
>>
>> Will you take care of this cleanup ?
>
> I think I can do that, but I'm not sure how much time I will be able
> to allocate for that. Let me try though.
Great, thanks for your help
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog