diff mbox series

[3/3] thermal/drivers/intel: Use generic trip points for intel_soc_dts_iosf

Message ID 20230118181622.33335-3-daniel.lezcano@linaro.org
State New
Headers
Series [1/3] thermal/drivers/intel: Use generic trip points for quark_dts |

Commit Message

Daniel Lezcano Jan. 18, 2023, 6:16 p.m. UTC 
  From: Daniel Lezcano <daniel.lezcano@kernel.org>

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 <daniel.lezcano@linaro.org>
---
 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(-)

Comments

Rafael J. Wysocki Jan. 19, 2023, 8:04 p.m. UTC | #1 
On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> From: Daniel Lezcano <daniel.lezcano@kernel.org>
>
> 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 <daniel.lezcano@linaro.org>
> ---
>  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;
>  };
> --
Daniel Lezcano Jan. 23, 2023, 6:09 p.m. UTC | #2 
Hi Srinivas,

On 19/01/2023 21:04, Rafael J. Wysocki wrote:
> On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> From: Daniel Lezcano <daniel.lezcano@kernel.org>
>>
>> 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 <daniel.lezcano@linaro.org>
>> ---

[ ... ]

>>
>> @@ -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;
>> +       }
>> +
Rafael J. Wysocki Jan. 23, 2023, 8:19 p.m. UTC | #3 
On Mon, Jan 23, 2023 at 7:09 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi Srinivas,
>
> On 19/01/2023 21:04, Rafael J. Wysocki wrote:
> > On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> From: Daniel Lezcano <daniel.lezcano@kernel.org>
> >>
> >> 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 <daniel.lezcano@linaro.org>
> >> ---
>
> [ ... ]
>
> >>
> >> @@ -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;
> >> +       }
> >> +
>
> --
Daniel Lezcano Jan. 24, 2023, 10:28 a.m. UTC | #4 
On 23/01/2023 21:19, Rafael J. Wysocki wrote:
> On Mon, Jan 23, 2023 at 7:09 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>>
>> Hi Srinivas,
>>
>> On 19/01/2023 21:04, Rafael J. Wysocki wrote:
>>> On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> From: Daniel Lezcano <daniel.lezcano@kernel.org>
>>>>
>>>> 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 <daniel.lezcano@linaro.org>
>>>> ---
>>
>> [ ... ]
>>
>>>>
>>>> @@ -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 ?
Rafael J. Wysocki Jan. 24, 2023, 2:09 p.m. UTC | #5 
On Tue, Jan 24, 2023 at 11:28 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 23/01/2023 21:19, Rafael J. Wysocki wrote:
> > On Mon, Jan 23, 2023 at 7:09 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >>
> >> Hi Srinivas,
> >>
> >> On 19/01/2023 21:04, Rafael J. Wysocki wrote:
> >>> On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>> From: Daniel Lezcano <daniel.lezcano@kernel.org>
> >>>>
> >>>> 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 <daniel.lezcano@linaro.org>
> >>>> ---
> >>
> >> [ ... ]
> >>
> >>>>
> >>>> @@ -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.
Rafael J. Wysocki Jan. 26, 2023, 4:47 p.m. UTC | #6 
On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> From: Daniel Lezcano <daniel.lezcano@kernel.org>
>
> 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 <daniel.lezcano@linaro.org>
> ---
>  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
>
Daniel Lezcano Jan. 31, 2023, 5:03 p.m. UTC | #7 
Hi Rafael,


On 26/01/2023 17:47, Rafael J. Wysocki wrote:
> On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> From: Daniel Lezcano <daniel.lezcano@kernel.org>
>>
>> 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 <daniel.lezcano@linaro.org>
>> ---

[ ... ]

>> @@ -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
Rafael J. Wysocki Jan. 31, 2023, 7:17 p.m. UTC | #8 
On Tue, Jan 31, 2023 at 6:03 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi Rafael,
>
>
> On 26/01/2023 17:47, Rafael J. Wysocki wrote:
> > On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> From: Daniel Lezcano <daniel.lezcano@kernel.org>
> >>
> >> 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 <daniel.lezcano@linaro.org>
> >> ---
>
> [ ... ]
>
> >> @@ -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.
Daniel Lezcano Feb. 2, 2023, 2:36 p.m. UTC | #9 
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 ?
Rafael J. Wysocki Feb. 2, 2023, 2:43 p.m. UTC | #10 
On Thu, Feb 2, 2023 at 3:36 PM Daniel Lezcano <daniel.lezcano@linaro.org> 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.
Daniel Lezcano Feb. 2, 2023, 3:54 p.m. UTC | #11 
On 02/02/2023 15:43, Rafael J. Wysocki wrote:
> On Thu, Feb 2, 2023 at 3:36 PM Daniel Lezcano <daniel.lezcano@linaro.org> 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
diff mbox series

Patch

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;
 };