MIME-Version: 1.0
References: <20230118181622.33335-1-daniel.lezcano@linaro.org>
In-Reply-To: <20230118181622.33335-1-daniel.lezcano@linaro.org>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 26 Jan 2023 15:15:58 +0100
Message-ID: <CAJZ5v0icjsLBNkDqm49az=GixfEoLHAtCm7H13uOUv7Hr6yO2Q@mail.gmail.com>
Subject: Re: [PATCH 1/3] thermal/drivers/intel: Use generic trip points for quark_dts
To:     Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     rafael@kernel.org, srinivas.pandruvada@linux.intel.com,
        linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
        rui.zhang@intel.com, Amit Kucheria <amitk@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 18, 2023 at 7:16 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> 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>
> ---
>  .../thermal/intel/intel_quark_dts_thermal.c   | 56 +++++++++----------
>  1 file changed, 25 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/thermal/intel/intel_quark_dts_thermal.c b/drivers/thermal/intel/intel_quark_dts_thermal.c
> index 3eafc6b0e6c3..4e1d1799ec22 100644
> --- a/drivers/thermal/intel/intel_quark_dts_thermal.c
> +++ b/drivers/thermal/intel/intel_quark_dts_thermal.c
> @@ -84,6 +84,7 @@
>  #define QRK_DTS_MASK_TP_THRES          0xFF
>  #define QRK_DTS_SHIFT_TP               8
>  #define QRK_DTS_ID_TP_CRITICAL         0
> +#define QRK_DTS_ID_TP_HOT              1
>  #define QRK_DTS_SAFE_TP_THRES          105
>
>  /* Thermal Sensor Register Lock */
> @@ -104,6 +105,7 @@ struct soc_sensor_entry {
>         u32 store_ptps;
>         u32 store_dts_enable;
>         struct thermal_zone_device *tzone;
> +       struct thermal_trip trips[QRK_MAX_DTS_TRIPS];
>  };
>
>  static struct soc_sensor_entry *soc_dts;
> @@ -172,7 +174,7 @@ static int soc_dts_disable(struct thermal_zone_device *tzd)
>         return ret;
>  }
>
> -static int _get_trip_temp(int trip, int *temp)
> +static int get_trip_temp(int trip, int *temp)
>  {
>         int status;
>         u32 out;
> @@ -197,17 +199,6 @@ static int _get_trip_temp(int trip, int *temp)
>         return 0;
>  }
>
> -static inline int sys_get_trip_temp(struct thermal_zone_device *tzd,
> -                               int trip, int *temp)
> -{
> -       return _get_trip_temp(trip, temp);
> -}
> -
> -static inline int sys_get_crit_temp(struct thermal_zone_device *tzd, int *temp)
> -{
> -       return _get_trip_temp(QRK_DTS_ID_TP_CRITICAL, temp);
> -}
> -
>  static int update_trip_temp(struct soc_sensor_entry *aux_entry,
>                                 int trip, int temp)
>  {
> @@ -262,17 +253,6 @@ static inline int sys_set_trip_temp(struct thermal_zone_device *tzd, int trip,
>         return update_trip_temp(tzd->devdata, trip, temp);
>  }
>
> -static int sys_get_trip_type(struct thermal_zone_device *thermal,
> -               int trip, enum thermal_trip_type *type)
> -{
> -       if (trip)
> -               *type = THERMAL_TRIP_HOT;
> -       else
> -               *type = THERMAL_TRIP_CRITICAL;
> -
> -       return 0;
> -}
> -
>  static int sys_get_curr_temp(struct thermal_zone_device *tzd,
>                                 int *temp)
>  {
> @@ -315,10 +295,7 @@ static int sys_change_mode(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,
> -       .get_crit_temp = sys_get_crit_temp,
>         .change_mode = sys_change_mode,
>  };
>
> @@ -344,7 +321,7 @@ static void free_soc_dts(struct soc_sensor_entry *aux_entry)
>  static struct soc_sensor_entry *alloc_soc_dts(void)
>  {
>         struct soc_sensor_entry *aux_entry;
> -       int err;
> +       int err, temperature;
>         u32 out;
>         int wr_mask;
>
> @@ -385,10 +362,27 @@ static struct soc_sensor_entry *alloc_soc_dts(void)
>                         goto err_ret;
>         }
>
> -       aux_entry->tzone = thermal_zone_device_register("quark_dts",
> -                       QRK_MAX_DTS_TRIPS,
> -                       wr_mask,
> -                       aux_entry, &tzone_ops, NULL, 0, polling_delay);
> +       err = get_trip_temp(QRK_DTS_ID_TP_CRITICAL, &temperature);
> +       if (err)
> +               goto err_ret;
> +
> +       aux_entry->trips[QRK_DTS_ID_TP_CRITICAL].temperature = temperature;
> +       aux_entry->trips[QRK_DTS_ID_TP_CRITICAL].type = THERMAL_TRIP_CRITICAL;
> +
> +       err = get_trip_temp(QRK_DTS_ID_TP_HOT, &temperature);
> +       if (err)
> +               goto err_ret;

If I'm not mistaken, this won't even try to register the thermal zone
if at least one trip cannot be initialized, but previously it was
registered in that case, but the trips that failed to respond were
disabled.

This is a change in behavior that would at least need to be documented
in the changelog, but it isn't.

I'm not sure if it is safe to make even, however.

> +
> +       aux_entry->trips[QRK_DTS_ID_TP_HOT].temperature = temperature;
> +       aux_entry->trips[QRK_DTS_ID_TP_HOT].type = THERMAL_TRIP_HOT;
> +
> +       aux_entry->tzone =
> +               thermal_zone_device_register_with_trips("quark_dts",
> +                                                       aux_entry->trips,
> +                                                       QRK_MAX_DTS_TRIPS,
> +                                                       wr_mask,
> +                                                       aux_entry, &tzone_ops,
> +                                                       NULL, 0, polling_delay);
>         if (IS_ERR(aux_entry->tzone)) {
>                 err = PTR_ERR(aux_entry->tzone);
>                 goto err_ret;
> --
> 2.34.1
>