[PATCH 1/3] thermal/drivers/intel: Use generic trip points for quark_dts

[PATCH 1/3] thermal/drivers/intel: Use generic trip points for quark_dts

[Daniel Lezcano]

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

[PATCH 2/3] thermal/drivers/intel: Use generic trip points for processor_thermal_device_pci

[Daniel Lezcano]

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>
---
 .../processor_thermal_device_pci.c            | 53 ++++++++-----------
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
index bf1b1cdfade4..c7d50862bf56 100644
--- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
+++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
@@ -144,34 +144,6 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd, int *temp)
 	return 0;
 }
 
-static int sys_get_trip_temp(struct thermal_zone_device *tzd,
-			     int trip, int *temp)
-{
-	struct proc_thermal_pci *pci_info = tzd->devdata;
-	u32 _temp;
-
-	proc_thermal_mmio_read(pci_info, PROC_THERMAL_MMIO_THRES_0, &_temp);
-	if (!_temp) {
-		*temp = THERMAL_TEMP_INVALID;
-	} else {
-		int tjmax;
-
-		proc_thermal_mmio_read(pci_info, PROC_THERMAL_MMIO_TJMAX, &tjmax);
-		_temp = tjmax - _temp;
-		*temp = (unsigned long)_temp * 1000;
-	}
-
-	return 0;
-}
-
-static int sys_get_trip_type(struct thermal_zone_device *tzd, int trip,
-			      enum thermal_trip_type *type)
-{
-	*type = THERMAL_TRIP_PASSIVE;
-
-	return 0;
-}
-
 static int sys_set_trip_temp(struct thermal_zone_device *tzd, int trip, int temp)
 {
 	struct proc_thermal_pci *pci_info = tzd->devdata;
@@ -200,10 +172,26 @@ static int sys_set_trip_temp(struct thermal_zone_device *tzd, int trip, int temp
 	return 0;
 }
 
+static int get_trip_temp(struct proc_thermal_pci *pci_info)
+{
+	int temp, tjmax;
+
+	proc_thermal_mmio_read(pci_info, PROC_THERMAL_MMIO_THRES_0, &temp);
+	if (!temp)
+		return THERMAL_TEMP_INVALID;
+
+	proc_thermal_mmio_read(pci_info, PROC_THERMAL_MMIO_TJMAX, &tjmax);
+	temp = (tjmax - temp) * 1000;
+
+	return temp;
+}
+
+static struct thermal_trip psv_trip = {
+	.type = THERMAL_TRIP_PASSIVE,
+};
+
 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,
 };
 
@@ -251,7 +239,10 @@ static int proc_thermal_pci_probe(struct pci_dev *pdev, const struct pci_device_
 	if (ret)
 		goto err_ret_thermal;
 
-	pci_info->tzone = thermal_zone_device_register("TCPU_PCI", 1, 1, pci_info,
+	psv_trip.temperature = get_trip_temp(pci_info);
+	
+	pci_info->tzone = thermal_zone_device_register_with_trips("TCPU_PCI", &psv_trip,
+							1, 1, pci_info,
 							&tzone_ops,
 							&tzone_params, 0, 0);
 	if (IS_ERR(pci_info->tzone)) {
-- 
2.34.1

Re: [PATCH 2/3] thermal/drivers/intel: Use generic trip points for processor_thermal_device_pci

[srinivas pandruvada]

On Wed, 2023-01-18 at 19:16 +0100, Daniel Lezcano 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.
> 
In this scheme is the assumption is that trip point temperature never
changes? If firmware updated the trip temperature, what needs to be
done?

Thanks,
Srinivas


> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  .../processor_thermal_device_pci.c            | 53 ++++++++---------
> --
>  1 file changed, 22 insertions(+), 31 deletions(-)
> 
> diff --git
> a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.
> c
> b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.
> c
> index bf1b1cdfade4..c7d50862bf56 100644
> ---
> a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.
> c
> +++
> b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.
> c
> @@ -144,34 +144,6 @@ static int sys_get_curr_temp(struct
> thermal_zone_device *tzd, int *temp)
>         return 0;
>  }
>  
> -static int sys_get_trip_temp(struct thermal_zone_device *tzd,
> -                            int trip, int *temp)
> -{
> -       struct proc_thermal_pci *pci_info = tzd->devdata;
> -       u32 _temp;
> -
> -       proc_thermal_mmio_read(pci_info, PROC_THERMAL_MMIO_THRES_0,
> &_temp);
> -       if (!_temp) {
> -               *temp = THERMAL_TEMP_INVALID;
> -       } else {
> -               int tjmax;
> -
> -               proc_thermal_mmio_read(pci_info,
> PROC_THERMAL_MMIO_TJMAX, &tjmax);
> -               _temp = tjmax - _temp;
> -               *temp = (unsigned long)_temp * 1000;
> -       }
> -
> -       return 0;
> -}
> -
> -static int sys_get_trip_type(struct thermal_zone_device *tzd, int
> trip,
> -                             enum thermal_trip_type *type)
> -{
> -       *type = THERMAL_TRIP_PASSIVE;
> -
> -       return 0;
> -}
> -
>  static int sys_set_trip_temp(struct thermal_zone_device *tzd, int
> trip, int temp)
>  {
>         struct proc_thermal_pci *pci_info = tzd->devdata;
> @@ -200,10 +172,26 @@ static int sys_set_trip_temp(struct
> thermal_zone_device *tzd, int trip, int temp
>         return 0;
>  }
>  
> +static int get_trip_temp(struct proc_thermal_pci *pci_info)
> +{
> +       int temp, tjmax;
> +
> +       proc_thermal_mmio_read(pci_info, PROC_THERMAL_MMIO_THRES_0,
> &temp);
> +       if (!temp)
> +               return THERMAL_TEMP_INVALID;
> +
> +       proc_thermal_mmio_read(pci_info, PROC_THERMAL_MMIO_TJMAX,
> &tjmax);
> +       temp = (tjmax - temp) * 1000;
> +
> +       return temp;
> +}
> +
> +static struct thermal_trip psv_trip = {
> +       .type = THERMAL_TRIP_PASSIVE,
> +};
> +
>  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,
>  };
>  
> @@ -251,7 +239,10 @@ static int proc_thermal_pci_probe(struct pci_dev
> *pdev, const struct pci_device_
>         if (ret)
>                 goto err_ret_thermal;
>  
> -       pci_info->tzone = thermal_zone_device_register("TCPU_PCI", 1,
> 1, pci_info,
> +       psv_trip.temperature = get_trip_temp(pci_info);
> +       
> +       pci_info->tzone =
> thermal_zone_device_register_with_trips("TCPU_PCI", &psv_trip,
> +                                                       1, 1,
> pci_info,
>                                                         &tzone_ops,
>                                                         &tzone_params
> , 0, 0);
>         if (IS_ERR(pci_info->tzone)) {

Re: [PATCH 2/3] thermal/drivers/intel: Use generic trip points for processor_thermal_device_pci

[Daniel Lezcano]

Hi Srinivas,


On 18/01/2023 20:09, srinivas pandruvada wrote:
> On Wed, 2023-01-18 at 19:16 +0100, Daniel Lezcano 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.
>>
> In this scheme is the assumption is that trip point temperature never
> changes? If firmware updated the trip temperature, what needs to be
> done?

I'm a bit confused about the situation where the firmware can change the 
trip point in the back of the OSPM.

Does the firmware send a notification about the trip change? Or does it 
assume the OSPM will be reading the trip point while monitoring/polling 
the thermal zone ?

Is the question for this particular driver?

If the trip point is changed by the userspace (via sysfs), 
thermal_zone_set_trip() is used which in turn changes the thermal trip 
temperature directly in the generic structure and then calls the back 
set_trip_temp.


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

Re: [PATCH 2/3] thermal/drivers/intel: Use generic trip points for processor_thermal_device_pci

[srinivas pandruvada]

Hi Daniel,

On Mon, 2023-01-23 at 19:02 +0100, Daniel Lezcano wrote:
> 
> Hi Srinivas,
> 
> 
> On 18/01/2023 20:09, srinivas pandruvada wrote:
> > On Wed, 2023-01-18 at 19:16 +0100, Daniel Lezcano 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.
> > > 
> > In this scheme is the assumption is that trip point temperature
> > never
> > changes? If firmware updated the trip temperature, what needs to be
> > done?
> 
> I'm a bit confused about the situation where the firmware can change
> the 
> trip point in the back of the OSPM.
> 
> Does the firmware send a notification about the trip change? Or does
> it 
> assume the OSPM will be reading the trip point while
> monitoring/polling 
> the thermal zone ?
Firmware sends an ACPI notification. For example INT3403.

https://elixir.bootlin.com/linux/latest/C/ident/INT3403_PERF_TRIP_POINT_CHANGED


> 
> Is the question for this particular driver?
This PCH driver trips are not changed by firmware hence we don't have
to worry about here.

Thanks,
Srinivas

> 
> If the trip point is changed by the userspace (via sysfs), 
> thermal_zone_set_trip() is used which in turn changes the thermal
> trip 
> temperature directly in the generic structure and then calls the back
> set_trip_temp.
> 

Re: [PATCH 2/3] thermal/drivers/intel: Use generic trip points for processor_thermal_device_pci

[Rafael J. Wysocki]

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>
> ---
>  .../processor_thermal_device_pci.c            | 53 ++++++++-----------
>  1 file changed, 22 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
> index bf1b1cdfade4..c7d50862bf56 100644
> --- a/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
> +++ b/drivers/thermal/intel/int340x_thermal/processor_thermal_device_pci.c
> @@ -144,34 +144,6 @@ static int sys_get_curr_temp(struct thermal_zone_device *tzd, int *temp)
>         return 0;
>  }
>
> -static int sys_get_trip_temp(struct thermal_zone_device *tzd,
> -                            int trip, int *temp)
> -{
> -       struct proc_thermal_pci *pci_info = tzd->devdata;
> -       u32 _temp;
> -
> -       proc_thermal_mmio_read(pci_info, PROC_THERMAL_MMIO_THRES_0, &_temp);
> -       if (!_temp) {
> -               *temp = THERMAL_TEMP_INVALID;
> -       } else {
> -               int tjmax;
> -
> -               proc_thermal_mmio_read(pci_info, PROC_THERMAL_MMIO_TJMAX, &tjmax);
> -               _temp = tjmax - _temp;
> -               *temp = (unsigned long)_temp * 1000;
> -       }
> -
> -       return 0;
> -}
> -
> -static int sys_get_trip_type(struct thermal_zone_device *tzd, int trip,
> -                             enum thermal_trip_type *type)
> -{
> -       *type = THERMAL_TRIP_PASSIVE;
> -
> -       return 0;
> -}
> -
>  static int sys_set_trip_temp(struct thermal_zone_device *tzd, int trip, int temp)
>  {
>         struct proc_thermal_pci *pci_info = tzd->devdata;
> @@ -200,10 +172,26 @@ static int sys_set_trip_temp(struct thermal_zone_device *tzd, int trip, int temp
>         return 0;
>  }
>
> +static int get_trip_temp(struct proc_thermal_pci *pci_info)
> +{
> +       int temp, tjmax;
> +
> +       proc_thermal_mmio_read(pci_info, PROC_THERMAL_MMIO_THRES_0, &temp);
> +       if (!temp)
> +               return THERMAL_TEMP_INVALID;
> +
> +       proc_thermal_mmio_read(pci_info, PROC_THERMAL_MMIO_TJMAX, &tjmax);
> +       temp = (tjmax - temp) * 1000;
> +
> +       return temp;
> +}
> +
> +static struct thermal_trip psv_trip = {
> +       .type = THERMAL_TRIP_PASSIVE,
> +};
> +
>  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,
>  };
>
> @@ -251,7 +239,10 @@ static int proc_thermal_pci_probe(struct pci_dev *pdev, const struct pci_device_
>         if (ret)
>                 goto err_ret_thermal;
>
> -       pci_info->tzone = thermal_zone_device_register("TCPU_PCI", 1, 1, pci_info,
> +       psv_trip.temperature = get_trip_temp(pci_info);
> +
> +       pci_info->tzone = thermal_zone_device_register_with_trips("TCPU_PCI", &psv_trip,
> +                                                       1, 1, pci_info,
>                                                         &tzone_ops,
>                                                         &tzone_params, 0, 0);
>         if (IS_ERR(pci_info->tzone)) {
> --

Applied as 6.3 material with edits in the subject and changelog, thanks!

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

[Daniel Lezcano]

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

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

[Rafael J. Wysocki]

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

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

[Daniel Lezcano]

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

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

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

[Rafael J. Wysocki]

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

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

[Daniel Lezcano]

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 ?



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

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

[Rafael J. Wysocki]

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.

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

[Rafael J. Wysocki]

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
>

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

[Daniel Lezcano]

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


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

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

[Rafael J. Wysocki]

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.

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

[Daniel Lezcano]

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

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

[Rafael J. Wysocki]

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.

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

[Daniel Lezcano]

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


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

Re: [PATCH 1/3] thermal/drivers/intel: Use generic trip points for quark_dts

[Rafael J. Wysocki]

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
>

Re: [PATCH 1/3] thermal/drivers/intel: Use generic trip points for quark_dts

[Daniel Lezcano]

On 26/01/2023 15:15, Rafael J. Wysocki wrote:
> 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>
>> ---

[ ... ]

>> -       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.

Thanks for catching this.

Two solutions:

1. Set the temperature to THERMAL_TEMP_INVALID and change 
get_thermal_trip() to return -EINVAL or -ERANGE if the temperature is 
THERMAL_TEMP_INVALID

2. Register only the valid trip points.

What would be the preferable way ?


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

Re: [PATCH 1/3] thermal/drivers/intel: Use generic trip points for quark_dts

[Rafael J. Wysocki]

On Tue, Jan 31, 2023 at 5:41 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 26/01/2023 15:15, Rafael J. Wysocki wrote:
> > 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>
> >> ---
>
> [ ... ]
>
> >> -       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.
>
> Thanks for catching this.
>
> Two solutions:
>
> 1. Set the temperature to THERMAL_TEMP_INVALID and change
> get_thermal_trip() to return -EINVAL or -ERANGE if the temperature is
> THERMAL_TEMP_INVALID
>
> 2. Register only the valid trip points.
>
> What would be the preferable way ?

I think that the trip points that are registered currently need to
still be registered after the change.

Does registering a trip point with the temperature set to
THERMAL_TEMP_INVALID cause it to be effectively disabled?

Re: [PATCH 1/3] thermal/drivers/intel: Use generic trip points for quark_dts

[Daniel Lezcano]

On 31/01/2023 20:11, Rafael J. Wysocki wrote:
> On Tue, Jan 31, 2023 at 5:41 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 26/01/2023 15:15, Rafael J. Wysocki wrote:
>>> 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>
>>>> ---
>>
>> [ ... ]
>>
>>>> -       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.
>>
>> Thanks for catching this.
>>
>> Two solutions:
>>
>> 1. Set the temperature to THERMAL_TEMP_INVALID and change
>> get_thermal_trip() to return -EINVAL or -ERANGE if the temperature is
>> THERMAL_TEMP_INVALID
>>
>> 2. Register only the valid trip points.
>>
>> What would be the preferable way ?
> 
> I think that the trip points that are registered currently need to
> still be registered after the change.
> 
> Does registering a trip point with the temperature set to
> THERMAL_TEMP_INVALID cause it to be effectively disabled?

No but if we have thermal_zone_get_trip() returning -EINVAL if 
THERMAL_TEMP_INVALID is set for the specified trip id. Then the 
registering will set the disabled flag.

https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/thermal/thermal_core.c?h=thermal/bleeding-edge#n1395




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

Re: [PATCH 1/3] thermal/drivers/intel: Use generic trip points for quark_dts

[Daniel Lezcano]

On 31/01/2023 20:11, Rafael J. Wysocki wrote:
> On Tue, Jan 31, 2023 at 5:41 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 26/01/2023 15:15, Rafael J. Wysocki wrote:
>>> 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>
>>>> ---
>>
>> [ ... ]
>>
>>>> -       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.
>>
>> Thanks for catching this.
>>
>> Two solutions:
>>
>> 1. Set the temperature to THERMAL_TEMP_INVALID and change
>> get_thermal_trip() to return -EINVAL or -ERANGE if the temperature is
>> THERMAL_TEMP_INVALID
>>
>> 2. Register only the valid trip points.
>>
>> What would be the preferable way ?
> 
> I think that the trip points that are registered currently need to
> still be registered after the change.
> 
> Does registering a trip point with the temperature set to
> THERMAL_TEMP_INVALID cause it to be effectively disabled?

The initial behavior before the changes is:

The function thermal_zone_device_register() will go through all the trip 
points and call thermal_zone_get_trip(), resulting in a call to 
ops->get_trip_temp(). If the call fails, the trip point is tagged as 
disabled and will stay in this state forever, so discarded in the trip 
point crossed detection.

That does not report an error and the trip point is showed in sysfs but 
in a inconsistent state as it is actually disabled. Reading the trip 
point will return an error or not, but it is in any case disabled in the 
thermal framework. The userspace does not have the information about the 
trip point being disabled, so showing it up regardless its state is 
pointless and prone to confusion for the userspace.

IMO, it would be more sane to register the trip points which are 
actually valid, so invalid trip points are not showed up and does 
prevent extra complexity in the thermal core to handle them.





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

Re: [PATCH 1/3] thermal/drivers/intel: Use generic trip points for quark_dts

[Rafael J. Wysocki]

On Wed, Feb 1, 2023 at 11:42 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 31/01/2023 20:11, Rafael J. Wysocki wrote:
> > On Tue, Jan 31, 2023 at 5:41 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 26/01/2023 15:15, Rafael J. Wysocki wrote:
> >>> 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>
> >>>> ---
> >>
> >> [ ... ]
> >>
> >>>> -       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.
> >>
> >> Thanks for catching this.
> >>
> >> Two solutions:
> >>
> >> 1. Set the temperature to THERMAL_TEMP_INVALID and change
> >> get_thermal_trip() to return -EINVAL or -ERANGE if the temperature is
> >> THERMAL_TEMP_INVALID
> >>
> >> 2. Register only the valid trip points.
> >>
> >> What would be the preferable way ?
> >
> > I think that the trip points that are registered currently need to
> > still be registered after the change.
> >
> > Does registering a trip point with the temperature set to
> > THERMAL_TEMP_INVALID cause it to be effectively disabled?
>
> The initial behavior before the changes is:
>
> The function thermal_zone_device_register() will go through all the trip
> points and call thermal_zone_get_trip(), resulting in a call to
> ops->get_trip_temp(). If the call fails, the trip point is tagged as
> disabled and will stay in this state forever, so discarded in the trip
> point crossed detection.
>
> That does not report an error and the trip point is showed in sysfs but
> in a inconsistent state as it is actually disabled. Reading the trip
> point will return an error or not, but it is in any case disabled in the
> thermal framework. The userspace does not have the information about the
> trip point being disabled, so showing it up regardless its state is
> pointless and prone to confusion for the userspace.
>
> IMO, it would be more sane to register the trip points which are
> actually valid, so invalid trip points are not showed up and does
> prevent extra complexity in the thermal core to handle them.

Except when the trip point can be updated to become a valid one later,
for example in response to a system configuration change.  That can
happen to ACPI-provided trip points, for example.

I don't think that this is an issue for this particular driver, but
the core needs to handle that case anyway.

Moreover, there is the case when trip points only become relevant when
their temperatures are set via ops->set_trip_temp() and they are
THERMAL_TEMP_INVALID initially, which needs to be handled by the core
either.

When the driver has no way to update trip point temperatures, either
through a firmware notification or via ops->set_trip_temp(), then I
agree that registering them is not very useful if their temperatures
cannot be determined.

Re: [PATCH 1/3] thermal/drivers/intel: Use generic trip points for quark_dts

[Daniel Lezcano]

On 01/02/2023 19:47, Rafael J. Wysocki wrote:
> On Wed, Feb 1, 2023 at 11:42 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 31/01/2023 20:11, Rafael J. Wysocki wrote:
>>> On Tue, Jan 31, 2023 at 5:41 PM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 26/01/2023 15:15, Rafael J. Wysocki wrote:
>>>>> 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>
>>>>>> ---
>>>>
>>>> [ ... ]
>>>>
>>>>>> -       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.
>>>>
>>>> Thanks for catching this.
>>>>
>>>> Two solutions:
>>>>
>>>> 1. Set the temperature to THERMAL_TEMP_INVALID and change
>>>> get_thermal_trip() to return -EINVAL or -ERANGE if the temperature is
>>>> THERMAL_TEMP_INVALID
>>>>
>>>> 2. Register only the valid trip points.
>>>>
>>>> What would be the preferable way ?
>>>
>>> I think that the trip points that are registered currently need to
>>> still be registered after the change.
>>>
>>> Does registering a trip point with the temperature set to
>>> THERMAL_TEMP_INVALID cause it to be effectively disabled?
>>
>> The initial behavior before the changes is:
>>
>> The function thermal_zone_device_register() will go through all the trip
>> points and call thermal_zone_get_trip(), resulting in a call to
>> ops->get_trip_temp(). If the call fails, the trip point is tagged as
>> disabled and will stay in this state forever, so discarded in the trip
>> point crossed detection.
>>
>> That does not report an error and the trip point is showed in sysfs but
>> in a inconsistent state as it is actually disabled. Reading the trip
>> point will return an error or not, but it is in any case disabled in the
>> thermal framework. The userspace does not have the information about the
>> trip point being disabled, so showing it up regardless its state is
>> pointless and prone to confusion for the userspace.
>>
>> IMO, it would be more sane to register the trip points which are
>> actually valid, so invalid trip points are not showed up and does
>> prevent extra complexity in the thermal core to handle them.
> 
> Except when the trip point can be updated to become a valid one later,
> for example in response to a system configuration change.  That can
> happen to ACPI-provided trip points, for example.
> 
> I don't think that this is an issue for this particular driver, but
> the core needs to handle that case anyway.

Yes, but the point is the core code never handled that case.

If the trip point fails when registering the thermal zone (and this is 
not related to our changes), the trip point is added to the disabled 
trips bitmap and then whatever the action to validate the trip point, it 
remains disabled for the thermal framework. There is no action to enable 
it (except I missed something).

> Moreover, there is the case when trip points only become relevant when
> their temperatures are set via ops->set_trip_temp() and they are
> THERMAL_TEMP_INVALID initially, which needs to be handled by the core
> either.

Ok, then I guess the simplest change is to assign THERMAL_TEMP_INVALID 
in this driver, if get_trip_temp fails at the initialization time.

Later we can add a thermal_zone_device_update_trips() with the needed 
locking and actions related to the update.

> When the driver has no way to update trip point temperatures, either
> through a firmware notification or via ops->set_trip_temp(), then I
> agree that registering them is not very useful if their temperatures
> cannot be determined.

+1

Thanks!

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

Re: [PATCH 1/3] thermal/drivers/intel: Use generic trip points for quark_dts

[Rafael J. Wysocki]

On Wed, Feb 1, 2023 at 8:27 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On 01/02/2023 19:47, Rafael J. Wysocki wrote:
> > On Wed, Feb 1, 2023 at 11:42 AM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> On 31/01/2023 20:11, Rafael J. Wysocki wrote:
> >>> On Tue, Jan 31, 2023 at 5:41 PM Daniel Lezcano
> >>> <daniel.lezcano@linaro.org> wrote:
> >>>>
> >>>> On 26/01/2023 15:15, Rafael J. Wysocki wrote:
> >>>>> 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>
> >>>>>> ---
> >>>>
> >>>> [ ... ]
> >>>>
> >>>>>> -       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.
> >>>>
> >>>> Thanks for catching this.
> >>>>
> >>>> Two solutions:
> >>>>
> >>>> 1. Set the temperature to THERMAL_TEMP_INVALID and change
> >>>> get_thermal_trip() to return -EINVAL or -ERANGE if the temperature is
> >>>> THERMAL_TEMP_INVALID
> >>>>
> >>>> 2. Register only the valid trip points.
> >>>>
> >>>> What would be the preferable way ?
> >>>
> >>> I think that the trip points that are registered currently need to
> >>> still be registered after the change.
> >>>
> >>> Does registering a trip point with the temperature set to
> >>> THERMAL_TEMP_INVALID cause it to be effectively disabled?
> >>
> >> The initial behavior before the changes is:
> >>
> >> The function thermal_zone_device_register() will go through all the trip
> >> points and call thermal_zone_get_trip(), resulting in a call to
> >> ops->get_trip_temp(). If the call fails, the trip point is tagged as
> >> disabled and will stay in this state forever, so discarded in the trip
> >> point crossed detection.
> >>
> >> That does not report an error and the trip point is showed in sysfs but
> >> in a inconsistent state as it is actually disabled. Reading the trip
> >> point will return an error or not, but it is in any case disabled in the
> >> thermal framework. The userspace does not have the information about the
> >> trip point being disabled, so showing it up regardless its state is
> >> pointless and prone to confusion for the userspace.
> >>
> >> IMO, it would be more sane to register the trip points which are
> >> actually valid, so invalid trip points are not showed up and does
> >> prevent extra complexity in the thermal core to handle them.
> >
> > Except when the trip point can be updated to become a valid one later,
> > for example in response to a system configuration change.  That can
> > happen to ACPI-provided trip points, for example.
> >
> > I don't think that this is an issue for this particular driver, but
> > the core needs to handle that case anyway.
>
> Yes, but the point is the core code never handled that case.

True.

What I wanted to say, though, is that the core needs to allow
registering trip points with THERMAL_TEMP_INVALID without disabling
them automatically, so they can be updated and used later.

> If the trip point fails when registering the thermal zone (and this is
> not related to our changes), the trip point is added to the disabled
> trips bitmap and then whatever the action to validate the trip point, it
> remains disabled for the thermal framework. There is no action to enable
> it (except I missed something).
>
> > Moreover, there is the case when trip points only become relevant when
> > their temperatures are set via ops->set_trip_temp() and they are
> > THERMAL_TEMP_INVALID initially, which needs to be handled by the core
> > either.
>
> Ok, then I guess the simplest change is to assign THERMAL_TEMP_INVALID
> in this driver, if get_trip_temp fails at the initialization time.
>
> Later we can add a thermal_zone_device_update_trips() with the needed
> locking and actions related to the update.

Well, there is thermal_zone_device_update() and one of the events it
is supposed to handle is THERMAL_TRIP_CHANGED, so I'm not sure how the
new interface would differ from it?

Re: [PATCH 1/3] thermal/drivers/intel: Use generic trip points for quark_dts

[Daniel Lezcano]

On 02/02/2023 11:32, Rafael J. Wysocki wrote:
> On Wed, Feb 1, 2023 at 8:27 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On 01/02/2023 19:47, Rafael J. Wysocki wrote:
>>> On Wed, Feb 1, 2023 at 11:42 AM Daniel Lezcano
>>> <daniel.lezcano@linaro.org> wrote:
>>>>
>>>> On 31/01/2023 20:11, Rafael J. Wysocki wrote:
>>>>> On Tue, Jan 31, 2023 at 5:41 PM Daniel Lezcano
>>>>> <daniel.lezcano@linaro.org> wrote:
>>>>>>
>>>>>> On 26/01/2023 15:15, Rafael J. Wysocki wrote:
>>>>>>> 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>
>>>>>>>> ---
>>>>>>
>>>>>> [ ... ]
>>>>>>
>>>>>>>> -       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.
>>>>>>
>>>>>> Thanks for catching this.
>>>>>>
>>>>>> Two solutions:
>>>>>>
>>>>>> 1. Set the temperature to THERMAL_TEMP_INVALID and change
>>>>>> get_thermal_trip() to return -EINVAL or -ERANGE if the temperature is
>>>>>> THERMAL_TEMP_INVALID
>>>>>>
>>>>>> 2. Register only the valid trip points.
>>>>>>
>>>>>> What would be the preferable way ?
>>>>>
>>>>> I think that the trip points that are registered currently need to
>>>>> still be registered after the change.
>>>>>
>>>>> Does registering a trip point with the temperature set to
>>>>> THERMAL_TEMP_INVALID cause it to be effectively disabled?
>>>>
>>>> The initial behavior before the changes is:
>>>>
>>>> The function thermal_zone_device_register() will go through all the trip
>>>> points and call thermal_zone_get_trip(), resulting in a call to
>>>> ops->get_trip_temp(). If the call fails, the trip point is tagged as
>>>> disabled and will stay in this state forever, so discarded in the trip
>>>> point crossed detection.
>>>>
>>>> That does not report an error and the trip point is showed in sysfs but
>>>> in a inconsistent state as it is actually disabled. Reading the trip
>>>> point will return an error or not, but it is in any case disabled in the
>>>> thermal framework. The userspace does not have the information about the
>>>> trip point being disabled, so showing it up regardless its state is
>>>> pointless and prone to confusion for the userspace.
>>>>
>>>> IMO, it would be more sane to register the trip points which are
>>>> actually valid, so invalid trip points are not showed up and does
>>>> prevent extra complexity in the thermal core to handle them.
>>>
>>> Except when the trip point can be updated to become a valid one later,
>>> for example in response to a system configuration change.  That can
>>> happen to ACPI-provided trip points, for example.
>>>
>>> I don't think that this is an issue for this particular driver, but
>>> the core needs to handle that case anyway.
>>
>> Yes, but the point is the core code never handled that case.
> 
> True.
> 
> What I wanted to say, though, is that the core needs to allow
> registering trip points with THERMAL_TEMP_INVALID without disabling
> them automatically, so they can be updated and used later.

Ok, so it is fine with the current code AFAICT.

The handle_thermal_trip() functions are discarding trips with 
temperature below zero for hot and critical. The trip crossing detection 
won't happen with these values.

However PASSIVE and ACTIVE trip points are going through the throttling 
governor callback with a -273000 trip temperature. I suppose those very 
specific trip points initialized to THERMAL_TEMP_INVALID are not 
associated with a cooling device, right ?


>> If the trip point fails when registering the thermal zone (and this is
>> not related to our changes), the trip point is added to the disabled
>> trips bitmap and then whatever the action to validate the trip point, it
>> remains disabled for the thermal framework. There is no action to enable
>> it (except I missed something).
>>
>>> Moreover, there is the case when trip points only become relevant when
>>> their temperatures are set via ops->set_trip_temp() and they are
>>> THERMAL_TEMP_INVALID initially, which needs to be handled by the core
>>> either.
>>
>> Ok, then I guess the simplest change is to assign THERMAL_TEMP_INVALID
>> in this driver, if get_trip_temp fails at the initialization time.
>>
>> Later we can add a thermal_zone_device_update_trips() with the needed
>> locking and actions related to the update.
> 
> Well, there is thermal_zone_device_update() and one of the events it
> is supposed to handle is THERMAL_TRIP_CHANGED, so I'm not sure how the
> new interface would differ from it?

Yes, we may have to investigate if the event should trigger the update 
or the update should trigger the event.



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