[PATCH v1 0/8] thermal: intel: intel_pch: Code simplification and cleanups

[PATCH v1 0/8] thermal: intel: intel_pch: Code simplification and cleanups

[Rafael J. Wysocki]

Hi All,

This patch series removes some uneeded code and data structures from the
intel_pch_thermal driver, rearranges it and does some assorted minor cleanups
(no change in behavior should result from it).

Please refer to the individual patch changelogs for details.

Thanks!

[PATCH v1 1/8] thermal: intel: intel_pch: Make pch_wpt_add_acpi_psv_trip() return int

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Modify pch_wpt_add_acpi_psv_trip() to return an int value instead of
using a return pointer for that.

While at it, drop an excessive empty code line.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/intel_pch_thermal.c |   21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
+++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
@@ -90,34 +90,31 @@ struct pch_thermal_device {
 };
 
 #ifdef CONFIG_ACPI
-
 /*
  * On some platforms, there is a companion ACPI device, which adds
  * passive trip temperature using _PSV method. There is no specific
  * passive temperature setting in MMIO interface of this PCI device.
  */
-static void pch_wpt_add_acpi_psv_trip(struct pch_thermal_device *ptd,
-				      int *nr_trips)
+static int pch_wpt_add_acpi_psv_trip(struct pch_thermal_device *ptd, int trip)
 {
 	struct acpi_device *adev;
 	int temp;
 
 	adev = ACPI_COMPANION(&ptd->pdev->dev);
 	if (!adev)
-		return;
+		return 0;
 
 	if (thermal_acpi_passive_trip_temp(adev, &temp) || temp <= 0)
-		return;
+		return 0;
 
-	ptd->trips[*nr_trips].type = THERMAL_TRIP_PASSIVE;
-	ptd->trips[*nr_trips].temperature = temp;
-	++(*nr_trips);
+	ptd->trips[trip].type = THERMAL_TRIP_PASSIVE;
+	ptd->trips[trip].temperature = temp;
+	return 1;
 }
 #else
-static void pch_wpt_add_acpi_psv_trip(struct pch_thermal_device *ptd,
-				      int *nr_trips)
+static int pch_wpt_add_acpi_psv_trip(struct pch_thermal_device *ptd, int trip)
 {
-
+	return 0;
 }
 #endif
 
@@ -167,7 +164,7 @@ read_trips:
 		++(*nr_trips);
 	}
 
-	pch_wpt_add_acpi_psv_trip(ptd, nr_trips);
+	*nr_trips += pch_wpt_add_acpi_psv_trip(ptd, *nr_trips);
 
 	return 0;
 }

Re: [PATCH v1 1/8] thermal: intel: intel_pch: Make pch_wpt_add_acpi_psv_trip() return int

[Daniel Lezcano]

On 30/01/2023 19:58, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Modify pch_wpt_add_acpi_psv_trip() to return an int value instead of
> using a return pointer for that.
> 
> While at it, drop an excessive empty code line.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


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

[PATCH v1 2/8] thermal: intel: intel_pch: Eliminate redundant return pointers

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Both pch_wpt_init() and pch_wpt_get_temp() can return the proper
result via their return values, so they do not need to use return
pointers.

Modify them accordingly.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/intel_pch_thermal.c |   40 +++++++++++++-----------------
 1 file changed, 18 insertions(+), 22 deletions(-)

Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
+++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
@@ -118,12 +118,11 @@ static int pch_wpt_add_acpi_psv_trip(str
 }
 #endif
 
-static int pch_wpt_init(struct pch_thermal_device *ptd, int *nr_trips)
+static int pch_wpt_init(struct pch_thermal_device *ptd)
 {
-	u8 tsel;
+	int nr_trips = 0;
 	u16 trip_temp;
-
-	*nr_trips = 0;
+	u8 tsel;
 
 	/* Check if BIOS has already enabled thermal sensor */
 	if (WPT_TSEL_ETS & readb(ptd->hw_base + WPT_TSEL)) {
@@ -151,29 +150,23 @@ read_trips:
 	trip_temp = readw(ptd->hw_base + WPT_CTT);
 	trip_temp &= 0x1FF;
 	if (trip_temp) {
-		ptd->trips[*nr_trips].temperature = GET_WPT_TEMP(trip_temp);
-		ptd->trips[*nr_trips].type = THERMAL_TRIP_CRITICAL;
-		++(*nr_trips);
+		ptd->trips[nr_trips].temperature = GET_WPT_TEMP(trip_temp);
+		ptd->trips[nr_trips++].type = THERMAL_TRIP_CRITICAL;
 	}
 
 	trip_temp = readw(ptd->hw_base + WPT_PHL);
 	trip_temp &= 0x1FF;
 	if (trip_temp) {
-		ptd->trips[*nr_trips].temperature = GET_WPT_TEMP(trip_temp);
-		ptd->trips[*nr_trips].type = THERMAL_TRIP_HOT;
-		++(*nr_trips);
+		ptd->trips[nr_trips].temperature = GET_WPT_TEMP(trip_temp);
+		ptd->trips[nr_trips++].type = THERMAL_TRIP_HOT;
 	}
 
-	*nr_trips += pch_wpt_add_acpi_psv_trip(ptd, *nr_trips);
-
-	return 0;
+	return nr_trips + pch_wpt_add_acpi_psv_trip(ptd, nr_trips);
 }
 
-static int pch_wpt_get_temp(struct pch_thermal_device *ptd, int *temp)
+static int pch_wpt_get_temp(struct pch_thermal_device *ptd)
 {
-	*temp = GET_WPT_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TEMP));
-
-	return 0;
+	return GET_WPT_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TEMP));
 }
 
 /* Cool the PCH when it's overheat in .suspend_noirq phase */
@@ -259,8 +252,8 @@ static int pch_wpt_resume(struct pch_the
 }
 
 struct pch_dev_ops {
-	int (*hw_init)(struct pch_thermal_device *ptd, int *nr_trips);
-	int (*get_temp)(struct pch_thermal_device *ptd, int *temp);
+	int (*hw_init)(struct pch_thermal_device *ptd);
+	int (*get_temp)(struct pch_thermal_device *ptd);
 	int (*suspend)(struct pch_thermal_device *ptd);
 	int (*resume)(struct pch_thermal_device *ptd);
 };
@@ -278,7 +271,8 @@ static int pch_thermal_get_temp(struct t
 {
 	struct pch_thermal_device *ptd = tzd->devdata;
 
-	return	ptd->ops->get_temp(ptd, temp);
+	*temp = ptd->ops->get_temp(ptd);
+	return 0;
 }
 
 static void pch_critical(struct thermal_zone_device *tzd)
@@ -372,9 +366,11 @@ static int intel_pch_thermal_probe(struc
 		goto error_release;
 	}
 
-	err = ptd->ops->hw_init(ptd, &nr_trips);
-	if (err)
+	nr_trips = ptd->ops->hw_init(ptd);
+	if (nr_trips < 0) {
+		err = nr_trips;
 		goto error_cleanup;
+	}
 
 	ptd->tzd = thermal_zone_device_register_with_trips(bi->name, ptd->trips,
 							   nr_trips, 0, ptd,

Re: [PATCH v1 2/8] thermal: intel: intel_pch: Eliminate redundant return pointers

[Daniel Lezcano]

On 30/01/2023 19:59, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Both pch_wpt_init() and pch_wpt_get_temp() can return the proper
> result via their return values, so they do not need to use return
> pointers.
> 
> Modify them accordingly.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


-- 
<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 v1 2/8] thermal: intel: intel_pch: Eliminate redundant return pointers

[Daniel Lezcano]

On 30/01/2023 19:59, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Both pch_wpt_init() and pch_wpt_get_temp() can return the proper
> result via their return values, so they do not need to use return
> pointers.
> 
> Modify them accordingly.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


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

[PATCH v1 3/8] thermal: intel: intel_pch: Rename device operations callbacks

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because the same device operations callbacks are used for all supported
boards, they are in fact generic, so rename them to reflect that.

Also rename the operations object itself for consistency.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/intel_pch_thermal.c |   34 ++++++++++++++----------------
 1 file changed, 16 insertions(+), 18 deletions(-)

Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
+++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
@@ -118,7 +118,7 @@ static int pch_wpt_add_acpi_psv_trip(str
 }
 #endif
 
-static int pch_wpt_init(struct pch_thermal_device *ptd)
+static int pch_hw_init(struct pch_thermal_device *ptd)
 {
 	int nr_trips = 0;
 	u16 trip_temp;
@@ -164,13 +164,13 @@ read_trips:
 	return nr_trips + pch_wpt_add_acpi_psv_trip(ptd, nr_trips);
 }
 
-static int pch_wpt_get_temp(struct pch_thermal_device *ptd)
+static int pch_get_temp(struct pch_thermal_device *ptd)
 {
 	return GET_WPT_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TEMP));
 }
 
 /* Cool the PCH when it's overheat in .suspend_noirq phase */
-static int pch_wpt_suspend(struct pch_thermal_device *ptd)
+static int pch_suspend(struct pch_thermal_device *ptd)
 {
 	u8 tsel;
 	int pch_delay_cnt = 0;
@@ -237,7 +237,7 @@ static int pch_wpt_suspend(struct pch_th
 	return 0;
 }
 
-static int pch_wpt_resume(struct pch_thermal_device *ptd)
+static int pch_resume(struct pch_thermal_device *ptd)
 {
 	u8 tsel;
 
@@ -258,13 +258,11 @@ struct pch_dev_ops {
 	int (*resume)(struct pch_thermal_device *ptd);
 };
 
-
-/* dev ops for Wildcat Point */
-static const struct pch_dev_ops pch_dev_ops_wpt = {
-	.hw_init = pch_wpt_init,
-	.get_temp = pch_wpt_get_temp,
-	.suspend = pch_wpt_suspend,
-	.resume = pch_wpt_resume,
+static const struct pch_dev_ops pch_dev_ops = {
+	.hw_init = pch_hw_init,
+	.get_temp = pch_get_temp,
+	.suspend = pch_suspend,
+	.resume = pch_resume,
 };
 
 static int pch_thermal_get_temp(struct thermal_zone_device *tzd, int *temp)
@@ -301,31 +299,31 @@ static const struct board_info {
 } board_info[] = {
 	[board_hsw] = {
 		.name = "pch_haswell",
-		.ops = &pch_dev_ops_wpt,
+		.ops = &pch_dev_ops,
 	},
 	[board_wpt] = {
 		.name = "pch_wildcat_point",
-		.ops = &pch_dev_ops_wpt,
+		.ops = &pch_dev_ops,
 	},
 	[board_skl] = {
 		.name = "pch_skylake",
-		.ops = &pch_dev_ops_wpt,
+		.ops = &pch_dev_ops,
 	},
 	[board_cnl] = {
 		.name = "pch_cannonlake",
-		.ops = &pch_dev_ops_wpt,
+		.ops = &pch_dev_ops,
 	},
 	[board_cml] = {
 		.name = "pch_cometlake",
-		.ops = &pch_dev_ops_wpt,
+		.ops = &pch_dev_ops,
 	},
 	[board_lwb] = {
 		.name = "pch_lewisburg",
-		.ops = &pch_dev_ops_wpt,
+		.ops = &pch_dev_ops,
 	},
 	[board_wbg] = {
 		.name = "pch_wellsburg",
-		.ops = &pch_dev_ops_wpt,
+		.ops = &pch_dev_ops,
 	},
 };
 

Re: [PATCH v1 3/8] thermal: intel: intel_pch: Rename device operations callbacks

[Daniel Lezcano]

On 30/01/2023 20:00, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Because the same device operations callbacks are used for all supported
> boards, they are in fact generic, so rename them to reflect that.
> 
> Also rename the operations object itself for consistency.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


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

[PATCH v1 4/8] thermal: intel: intel_pch: Eliminate device operations object

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The same device operations object is pointed to by all of the board
configurations in the driver, so effectively the same operations
callbacks are used by all of them which only adds overhead (that can
be significant due to retpolines) for no real purpose.

For this reason, drop the device operations object and replace the
respective callback invocations by direct calls to the specific
functions that were previously pointed to by callback pointers.

No intentional change in behavior.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/intel_pch_thermal.c |   33 +++---------------------------
 1 file changed, 4 insertions(+), 29 deletions(-)

Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
+++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
@@ -82,7 +82,6 @@ static char driver_name[] = "Intel PCH t
 
 struct pch_thermal_device {
 	void __iomem *hw_base;
-	const struct pch_dev_ops *ops;
 	struct pci_dev *pdev;
 	struct thermal_zone_device *tzd;
 	struct thermal_trip trips[PCH_MAX_TRIPS];
@@ -251,25 +250,11 @@ static int pch_resume(struct pch_thermal
 	return 0;
 }
 
-struct pch_dev_ops {
-	int (*hw_init)(struct pch_thermal_device *ptd);
-	int (*get_temp)(struct pch_thermal_device *ptd);
-	int (*suspend)(struct pch_thermal_device *ptd);
-	int (*resume)(struct pch_thermal_device *ptd);
-};
-
-static const struct pch_dev_ops pch_dev_ops = {
-	.hw_init = pch_hw_init,
-	.get_temp = pch_get_temp,
-	.suspend = pch_suspend,
-	.resume = pch_resume,
-};
-
 static int pch_thermal_get_temp(struct thermal_zone_device *tzd, int *temp)
 {
 	struct pch_thermal_device *ptd = tzd->devdata;
 
-	*temp = ptd->ops->get_temp(ptd);
+	*temp = pch_get_temp(ptd);
 	return 0;
 }
 
@@ -295,35 +280,27 @@ enum board_ids {
 
 static const struct board_info {
 	const char *name;
-	const struct pch_dev_ops *ops;
 } board_info[] = {
 	[board_hsw] = {
 		.name = "pch_haswell",
-		.ops = &pch_dev_ops,
 	},
 	[board_wpt] = {
 		.name = "pch_wildcat_point",
-		.ops = &pch_dev_ops,
 	},
 	[board_skl] = {
 		.name = "pch_skylake",
-		.ops = &pch_dev_ops,
 	},
 	[board_cnl] = {
 		.name = "pch_cannonlake",
-		.ops = &pch_dev_ops,
 	},
 	[board_cml] = {
 		.name = "pch_cometlake",
-		.ops = &pch_dev_ops,
 	},
 	[board_lwb] = {
 		.name = "pch_lewisburg",
-		.ops = &pch_dev_ops,
 	},
 	[board_wbg] = {
 		.name = "pch_wellsburg",
-		.ops = &pch_dev_ops,
 	},
 };
 
@@ -340,8 +317,6 @@ static int intel_pch_thermal_probe(struc
 	if (!ptd)
 		return -ENOMEM;
 
-	ptd->ops = bi->ops;
-
 	pci_set_drvdata(pdev, ptd);
 	ptd->pdev = pdev;
 
@@ -364,7 +339,7 @@ static int intel_pch_thermal_probe(struc
 		goto error_release;
 	}
 
-	nr_trips = ptd->ops->hw_init(ptd);
+	nr_trips = pch_hw_init(ptd);
 	if (nr_trips < 0) {
 		err = nr_trips;
 		goto error_cleanup;
@@ -412,14 +387,14 @@ static int intel_pch_thermal_suspend_noi
 {
 	struct pch_thermal_device *ptd = dev_get_drvdata(device);
 
-	return ptd->ops->suspend(ptd);
+	return pch_suspend(ptd);
 }
 
 static int intel_pch_thermal_resume(struct device *device)
 {
 	struct pch_thermal_device *ptd = dev_get_drvdata(device);
 
-	return ptd->ops->resume(ptd);
+	return pch_resume(ptd);
 }
 
 static const struct pci_device_id intel_pch_thermal_id[] = {

Re: [PATCH v1 4/8] thermal: intel: intel_pch: Eliminate device operations object

[Daniel Lezcano]

On 30/01/2023 20:02, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The same device operations object is pointed to by all of the board
> configurations in the driver, so effectively the same operations
> callbacks are used by all of them which only adds overhead (that can
> be significant due to retpolines) for no real purpose.
> 
> For this reason, drop the device operations object and replace the
> respective callback invocations by direct calls to the specific
> functions that were previously pointed to by callback pointers.
> 
> No intentional change in behavior.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>


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

[PATCH v1 5/8] thermal: intel: intel_pch: Fold two functions into their callers

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Fold two functions, pch_hw_init() and pch_get_temp(), that each have
only one caller, into their respective callers to make the code somewhat
easier to follow.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/intel_pch_thermal.c |   98 ++++++++++++------------------
 1 file changed, 42 insertions(+), 56 deletions(-)

Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
+++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
@@ -117,57 +117,6 @@ static int pch_wpt_add_acpi_psv_trip(str
 }
 #endif
 
-static int pch_hw_init(struct pch_thermal_device *ptd)
-{
-	int nr_trips = 0;
-	u16 trip_temp;
-	u8 tsel;
-
-	/* Check if BIOS has already enabled thermal sensor */
-	if (WPT_TSEL_ETS & readb(ptd->hw_base + WPT_TSEL)) {
-		ptd->bios_enabled = true;
-		goto read_trips;
-	}
-
-	tsel = readb(ptd->hw_base + WPT_TSEL);
-	/*
-	 * When TSEL's Policy Lock-Down bit is 1, TSEL become RO.
-	 * If so, thermal sensor cannot enable. Bail out.
-	 */
-	if (tsel & WPT_TSEL_PLDB) {
-		dev_err(&ptd->pdev->dev, "Sensor can't be enabled\n");
-		return -ENODEV;
-	}
-
-	writeb(tsel|WPT_TSEL_ETS, ptd->hw_base + WPT_TSEL);
-	if (!(WPT_TSEL_ETS & readb(ptd->hw_base + WPT_TSEL))) {
-		dev_err(&ptd->pdev->dev, "Sensor can't be enabled\n");
-		return -ENODEV;
-	}
-
-read_trips:
-	trip_temp = readw(ptd->hw_base + WPT_CTT);
-	trip_temp &= 0x1FF;
-	if (trip_temp) {
-		ptd->trips[nr_trips].temperature = GET_WPT_TEMP(trip_temp);
-		ptd->trips[nr_trips++].type = THERMAL_TRIP_CRITICAL;
-	}
-
-	trip_temp = readw(ptd->hw_base + WPT_PHL);
-	trip_temp &= 0x1FF;
-	if (trip_temp) {
-		ptd->trips[nr_trips].temperature = GET_WPT_TEMP(trip_temp);
-		ptd->trips[nr_trips++].type = THERMAL_TRIP_HOT;
-	}
-
-	return nr_trips + pch_wpt_add_acpi_psv_trip(ptd, nr_trips);
-}
-
-static int pch_get_temp(struct pch_thermal_device *ptd)
-{
-	return GET_WPT_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TEMP));
-}
-
 /* Cool the PCH when it's overheat in .suspend_noirq phase */
 static int pch_suspend(struct pch_thermal_device *ptd)
 {
@@ -254,7 +203,7 @@ static int pch_thermal_get_temp(struct t
 {
 	struct pch_thermal_device *ptd = tzd->devdata;
 
-	*temp = pch_get_temp(ptd);
+	*temp = GET_WPT_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TEMP));
 	return 0;
 }
 
@@ -310,8 +259,10 @@ static int intel_pch_thermal_probe(struc
 	enum board_ids board_id = id->driver_data;
 	const struct board_info *bi = &board_info[board_id];
 	struct pch_thermal_device *ptd;
-	int err;
+	u16 trip_temp;
 	int nr_trips;
+	u8 tsel;
+	int err;
 
 	ptd = devm_kzalloc(&pdev->dev, sizeof(*ptd), GFP_KERNEL);
 	if (!ptd)
@@ -339,12 +290,47 @@ static int intel_pch_thermal_probe(struc
 		goto error_release;
 	}
 
-	nr_trips = pch_hw_init(ptd);
-	if (nr_trips < 0) {
-		err = nr_trips;
+	/* Check if BIOS has already enabled thermal sensor */
+	if (WPT_TSEL_ETS & readb(ptd->hw_base + WPT_TSEL)) {
+		ptd->bios_enabled = true;
+		goto read_trips;
+	}
+
+	tsel = readb(ptd->hw_base + WPT_TSEL);
+	/*
+	 * When TSEL's Policy Lock-Down bit is 1, TSEL become RO.
+	 * If so, thermal sensor cannot enable. Bail out.
+	 */
+	if (tsel & WPT_TSEL_PLDB) {
+		dev_err(&ptd->pdev->dev, "Sensor can't be enabled\n");
+		err = -ENODEV;
 		goto error_cleanup;
 	}
 
+	writeb(tsel|WPT_TSEL_ETS, ptd->hw_base + WPT_TSEL);
+	if (!(WPT_TSEL_ETS & readb(ptd->hw_base + WPT_TSEL))) {
+		dev_err(&ptd->pdev->dev, "Sensor can't be enabled\n");
+		err = -ENODEV;
+		goto error_cleanup;
+	}
+
+read_trips:
+	trip_temp = readw(ptd->hw_base + WPT_CTT);
+	trip_temp &= 0x1FF;
+	if (trip_temp) {
+		ptd->trips[nr_trips].temperature = GET_WPT_TEMP(trip_temp);
+		ptd->trips[nr_trips++].type = THERMAL_TRIP_CRITICAL;
+	}
+
+	trip_temp = readw(ptd->hw_base + WPT_PHL);
+	trip_temp &= 0x1FF;
+	if (trip_temp) {
+		ptd->trips[nr_trips].temperature = GET_WPT_TEMP(trip_temp);
+		ptd->trips[nr_trips++].type = THERMAL_TRIP_HOT;
+	}
+
+	nr_trips += pch_wpt_add_acpi_psv_trip(ptd, nr_trips);
+
 	ptd->tzd = thermal_zone_device_register_with_trips(bi->name, ptd->trips,
 							   nr_trips, 0, ptd,
 							   &tzd_ops, NULL, 0, 0);

Re: [PATCH v1 5/8] thermal: intel: intel_pch: Fold two functions into their callers

[Daniel Lezcano]

On 30/01/2023 20:03, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Fold two functions, pch_hw_init() and pch_get_temp(), that each have
> only one caller, into their respective callers to make the code somewhat
> easier to follow.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

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

[PATCH v1 6/8] thermal: intel: intel_pch: Fold suspend and resume routines into their callers

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Fold pch_suspend() and pch_resume(), that each have only one caller,
into their respective callers to make the code somewhat easier to
follow.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/intel_pch_thermal.c |  155 +++++++++++++-----------------
 1 file changed, 71 insertions(+), 84 deletions(-)

Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
+++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
@@ -117,88 +117,6 @@ static int pch_wpt_add_acpi_psv_trip(str
 }
 #endif
 
-/* Cool the PCH when it's overheat in .suspend_noirq phase */
-static int pch_suspend(struct pch_thermal_device *ptd)
-{
-	u8 tsel;
-	int pch_delay_cnt = 0;
-	u16 pch_thr_temp, pch_cur_temp;
-
-	/* Shutdown the thermal sensor if it is not enabled by BIOS */
-	if (!ptd->bios_enabled) {
-		tsel = readb(ptd->hw_base + WPT_TSEL);
-		writeb(tsel & 0xFE, ptd->hw_base + WPT_TSEL);
-		return 0;
-	}
-
-	/* Do not check temperature if it is not s2idle */
-	if (pm_suspend_via_firmware())
-		return 0;
-
-	/* Get the PCH temperature threshold value */
-	pch_thr_temp = GET_PCH_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TSPM));
-
-	/* Get the PCH current temperature value */
-	pch_cur_temp = GET_PCH_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TEMP));
-
-	/*
-	 * If current PCH temperature is higher than configured PCH threshold
-	 * value, run some delay loop with sleep to let the current temperature
-	 * go down below the threshold value which helps to allow system enter
-	 * lower power S0ix suspend state. Even after delay loop if PCH current
-	 * temperature stays above threshold, notify the warning message
-	 * which helps to indentify the reason why S0ix entry was rejected.
-	 */
-	while (pch_delay_cnt < delay_cnt) {
-		if (pch_cur_temp < pch_thr_temp)
-			break;
-
-		if (pm_wakeup_pending()) {
-			dev_warn(&ptd->pdev->dev, "Wakeup event detected, abort cooling\n");
-			return 0;
-		}
-
-		pch_delay_cnt++;
-		dev_dbg(&ptd->pdev->dev,
-			"CPU-PCH current temp [%dC] higher than the threshold temp [%dC], sleep %d times for %d ms duration\n",
-			pch_cur_temp, pch_thr_temp, pch_delay_cnt, delay_timeout);
-		msleep(delay_timeout);
-		/* Read the PCH current temperature for next cycle. */
-		pch_cur_temp = GET_PCH_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TEMP));
-	}
-
-	if (pch_cur_temp >= pch_thr_temp)
-		dev_warn(&ptd->pdev->dev,
-			"CPU-PCH is hot [%dC] after %d ms delay. S0ix might fail\n",
-			pch_cur_temp, pch_delay_cnt * delay_timeout);
-	else {
-		if (pch_delay_cnt)
-			dev_info(&ptd->pdev->dev,
-				"CPU-PCH is cool [%dC] after %d ms delay\n",
-				pch_cur_temp, pch_delay_cnt * delay_timeout);
-		else
-			dev_info(&ptd->pdev->dev,
-				"CPU-PCH is cool [%dC]\n",
-				pch_cur_temp);
-	}
-
-	return 0;
-}
-
-static int pch_resume(struct pch_thermal_device *ptd)
-{
-	u8 tsel;
-
-	if (ptd->bios_enabled)
-		return 0;
-
-	tsel = readb(ptd->hw_base + WPT_TSEL);
-
-	writeb(tsel | WPT_TSEL_ETS, ptd->hw_base + WPT_TSEL);
-
-	return 0;
-}
-
 static int pch_thermal_get_temp(struct thermal_zone_device *tzd, int *temp)
 {
 	struct pch_thermal_device *ptd = tzd->devdata;
@@ -372,15 +290,84 @@ static void intel_pch_thermal_remove(str
 static int intel_pch_thermal_suspend_noirq(struct device *device)
 {
 	struct pch_thermal_device *ptd = dev_get_drvdata(device);
+	u16 pch_thr_temp, pch_cur_temp;
+	int pch_delay_cnt = 0;
+	u8 tsel;
+
+	/* Shutdown the thermal sensor if it is not enabled by BIOS */
+	if (!ptd->bios_enabled) {
+		tsel = readb(ptd->hw_base + WPT_TSEL);
+		writeb(tsel & 0xFE, ptd->hw_base + WPT_TSEL);
+		return 0;
+	}
+
+	/* Do not check temperature if it is not s2idle */
+	if (pm_suspend_via_firmware())
+		return 0;
+
+	/* Get the PCH temperature threshold value */
+	pch_thr_temp = GET_PCH_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TSPM));
+
+	/* Get the PCH current temperature value */
+	pch_cur_temp = GET_PCH_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TEMP));
 
-	return pch_suspend(ptd);
+	/*
+	 * If current PCH temperature is higher than configured PCH threshold
+	 * value, run some delay loop with sleep to let the current temperature
+	 * go down below the threshold value which helps to allow system enter
+	 * lower power S0ix suspend state. Even after delay loop if PCH current
+	 * temperature stays above threshold, notify the warning message
+	 * which helps to indentify the reason why S0ix entry was rejected.
+	 */
+	while (pch_delay_cnt < delay_cnt) {
+		if (pch_cur_temp < pch_thr_temp)
+			break;
+
+		if (pm_wakeup_pending()) {
+			dev_warn(&ptd->pdev->dev, "Wakeup event detected, abort cooling\n");
+			return 0;
+		}
+
+		pch_delay_cnt++;
+		dev_dbg(&ptd->pdev->dev,
+			"CPU-PCH current temp [%dC] higher than the threshold temp [%dC], sleep %d times for %d ms duration\n",
+			pch_cur_temp, pch_thr_temp, pch_delay_cnt, delay_timeout);
+		msleep(delay_timeout);
+		/* Read the PCH current temperature for next cycle. */
+		pch_cur_temp = GET_PCH_TEMP(WPT_TEMP_TSR & readw(ptd->hw_base + WPT_TEMP));
+	}
+
+	if (pch_cur_temp >= pch_thr_temp)
+		dev_warn(&ptd->pdev->dev,
+			"CPU-PCH is hot [%dC] after %d ms delay. S0ix might fail\n",
+			pch_cur_temp, pch_delay_cnt * delay_timeout);
+	else {
+		if (pch_delay_cnt)
+			dev_info(&ptd->pdev->dev,
+				"CPU-PCH is cool [%dC] after %d ms delay\n",
+				pch_cur_temp, pch_delay_cnt * delay_timeout);
+		else
+			dev_info(&ptd->pdev->dev,
+				"CPU-PCH is cool [%dC]\n",
+				pch_cur_temp);
+	}
+
+	return 0;
 }
 
 static int intel_pch_thermal_resume(struct device *device)
 {
 	struct pch_thermal_device *ptd = dev_get_drvdata(device);
+	u8 tsel;
 
-	return pch_resume(ptd);
+	if (ptd->bios_enabled)
+		return 0;
+
+	tsel = readb(ptd->hw_base + WPT_TSEL);
+
+	writeb(tsel | WPT_TSEL_ETS, ptd->hw_base + WPT_TSEL);
+
+	return 0;
 }
 
 static const struct pci_device_id intel_pch_thermal_id[] = {

Re: [PATCH v1 6/8] thermal: intel: intel_pch: Fold suspend and resume routines into their callers

[Daniel Lezcano]

On 30/01/2023 20:04, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Fold pch_suspend() and pch_resume(), that each have only one caller,
> into their respective callers to make the code somewhat easier to
> follow.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

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

[PATCH v1 7/8] thermal: intel: intel_pch: Rename board ID symbols

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Use capitals in the names of the board ID symbols and add the PCH_
prefix to each of them for consistency.

Also rename the board_ids enum accordingly.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/intel_pch_thermal.c |   54 +++++++++++++++---------------
 1 file changed, 27 insertions(+), 27 deletions(-)

Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
+++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
@@ -135,38 +135,38 @@ static struct thermal_zone_device_ops tz
 	.critical = pch_critical,
 };
 
-enum board_ids {
-	board_hsw,
-	board_wpt,
-	board_skl,
-	board_cnl,
-	board_cml,
-	board_lwb,
-	board_wbg,
+enum pch_board_ids {
+	PCH_BOARD_HSW = 0,
+	PCH_BOARD_WPT,
+	PCH_BOARD_SKL,
+	PCH_BOARD_CNL,
+	PCH_BOARD_CML,
+	PCH_BOARD_LWB,
+	PCH_BOARD_WBG,
 };
 
 static const struct board_info {
 	const char *name;
 } board_info[] = {
-	[board_hsw] = {
+	[PCH_BOARD_HSW] = {
 		.name = "pch_haswell",
 	},
-	[board_wpt] = {
+	[PCH_BOARD_WPT] = {
 		.name = "pch_wildcat_point",
 	},
-	[board_skl] = {
+	[PCH_BOARD_SKL] = {
 		.name = "pch_skylake",
 	},
-	[board_cnl] = {
+	[PCH_BOARD_CNL] = {
 		.name = "pch_cannonlake",
 	},
-	[board_cml] = {
+	[PCH_BOARD_CML] = {
 		.name = "pch_cometlake",
 	},
-	[board_lwb] = {
+	[PCH_BOARD_LWB] = {
 		.name = "pch_lewisburg",
 	},
-	[board_wbg] = {
+	[PCH_BOARD_WBG] = {
 		.name = "pch_wellsburg",
 	},
 };
@@ -174,7 +174,7 @@ static const struct board_info {
 static int intel_pch_thermal_probe(struct pci_dev *pdev,
 				   const struct pci_device_id *id)
 {
-	enum board_ids board_id = id->driver_data;
+	enum pch_board_ids board_id = id->driver_data;
 	const struct board_info *bi = &board_info[board_id];
 	struct pch_thermal_device *ptd;
 	u16 trip_temp;
@@ -372,27 +372,27 @@ static int intel_pch_thermal_resume(stru
 
 static const struct pci_device_id intel_pch_thermal_id[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_HSW_1),
-		.driver_data = board_hsw, },
+		.driver_data = PCH_BOARD_HSW, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_HSW_2),
-		.driver_data = board_hsw, },
+		.driver_data = PCH_BOARD_HSW, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT),
-		.driver_data = board_wpt, },
+		.driver_data = PCH_BOARD_WPT, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL),
-		.driver_data = board_skl, },
+		.driver_data = PCH_BOARD_SKL, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL_H),
-		.driver_data = board_skl, },
+		.driver_data = PCH_BOARD_SKL, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_CNL),
-		.driver_data = board_cnl, },
+		.driver_data = PCH_BOARD_CNL, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_CNL_H),
-		.driver_data = board_cnl, },
+		.driver_data = PCH_BOARD_CNL, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_CNL_LP),
-		.driver_data = board_cnl, },
+		.driver_data = PCH_BOARD_CNL, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_CML_H),
-		.driver_data = board_cml, },
+		.driver_data = PCH_BOARD_CML, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_LWB),
-		.driver_data = board_lwb, },
+		.driver_data = PCH_BOARD_LWB, },
 	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WBG),
-		.driver_data = board_wbg, },
+		.driver_data = PCH_BOARD_WBG, },
 	{ 0, },
 };
 MODULE_DEVICE_TABLE(pci, intel_pch_thermal_id);

Re: [PATCH v1 7/8] thermal: intel: intel_pch: Rename board ID symbols

[Zhang, Rui]

On Mon, 2023-01-30 at 20:04 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Use capitals in the names of the board ID symbols and add the PCH_
> prefix to each of them for consistency.
> 
> Also rename the board_ids enum accordingly.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/thermal/intel/intel_pch_thermal.c |   54 +++++++++++++++--
> -------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 
> Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
> +++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> @@ -135,38 +135,38 @@ static struct thermal_zone_device_ops tz
>  	.critical = pch_critical,
>  };
>  
> -enum board_ids {
> -	board_hsw,
> -	board_wpt,
> -	board_skl,
> -	board_cnl,
> -	board_cml,
> -	board_lwb,
> -	board_wbg,
> +enum pch_board_ids {
> +	PCH_BOARD_HSW = 0,
> +	PCH_BOARD_WPT,
> +	PCH_BOARD_SKL,
> +	PCH_BOARD_CNL,
> +	PCH_BOARD_CML,
> +	PCH_BOARD_LWB,
> +	PCH_BOARD_WBG,
>  };
>  
>  static const struct board_info {
>  	const char *name;
>  } board_info[] = {

Now struct board_info has "name" field only, so maybe we can remove
struct board_info, and use a "static const char *" array instead?

BTW, I'm building a kernel with this patch series as well as 
https://patchwork.kernel.org/project/linux-pm/list/?series=717084,
will update the test result later.

thanks,
rui


> -	[board_hsw] = {
> +	[PCH_BOARD_HSW] = {
>  		.name = "pch_haswell",
>  	},
> -	[board_wpt] = {
> +	[PCH_BOARD_WPT] = {
>  		.name = "pch_wildcat_point",
>  	},
> -	[board_skl] = {
> +	[PCH_BOARD_SKL] = {
>  		.name = "pch_skylake",
>  	},
> -	[board_cnl] = {
> +	[PCH_BOARD_CNL] = {
>  		.name = "pch_cannonlake",
>  	},
> -	[board_cml] = {
> +	[PCH_BOARD_CML] = {
>  		.name = "pch_cometlake",
>  	},
> -	[board_lwb] = {
> +	[PCH_BOARD_LWB] = {
>  		.name = "pch_lewisburg",
>  	},
> -	[board_wbg] = {
> +	[PCH_BOARD_WBG] = {
>  		.name = "pch_wellsburg",
>  	},
>  };
> @@ -174,7 +174,7 @@ static const struct board_info {
>  static int intel_pch_thermal_probe(struct pci_dev *pdev,
>  				   const struct pci_device_id *id)
>  {
> -	enum board_ids board_id = id->driver_data;
> +	enum pch_board_ids board_id = id->driver_data;
>  	const struct board_info *bi = &board_info[board_id];
>  	struct pch_thermal_device *ptd;
>  	u16 trip_temp;
> @@ -372,27 +372,27 @@ static int intel_pch_thermal_resume(stru
>  
>  static const struct pci_device_id intel_pch_thermal_id[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_HSW_1),
> -		.driver_data = board_hsw, },
> +		.driver_data = PCH_BOARD_HSW, },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_HSW_2),
> -		.driver_data = board_hsw, },
> +		.driver_data = PCH_BOARD_HSW, },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT),
> -		.driver_data = board_wpt, },
> +		.driver_data = PCH_BOARD_WPT, },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL),
> -		.driver_data = board_skl, },
> +		.driver_data = PCH_BOARD_SKL, },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL_H),
> -		.driver_data = board_skl, },
> +		.driver_data = PCH_BOARD_SKL, },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_CNL),
> -		.driver_data = board_cnl, },
> +		.driver_data = PCH_BOARD_CNL, },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_CNL_H),
> -		.driver_data = board_cnl, },
> +		.driver_data = PCH_BOARD_CNL, },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_CNL_LP),
> -		.driver_data = board_cnl, },
> +		.driver_data = PCH_BOARD_CNL, },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_CML_H),
> -		.driver_data = board_cml, },
> +		.driver_data = PCH_BOARD_CML, },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_LWB),
> -		.driver_data = board_lwb, },
> +		.driver_data = PCH_BOARD_LWB, },
>  	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WBG),
> -		.driver_data = board_wbg, },
> +		.driver_data = PCH_BOARD_WBG, },
>  	{ 0, },
>  };
>  MODULE_DEVICE_TABLE(pci, intel_pch_thermal_id);
> 
> 
> 

Re: [PATCH v1 7/8] thermal: intel: intel_pch: Rename board ID symbols

[Rafael J. Wysocki]

On Tuesday, January 31, 2023 12:17:55 PM CET Zhang, Rui wrote:
> On Mon, 2023-01-30 at 20:04 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Use capitals in the names of the board ID symbols and add the PCH_
> > prefix to each of them for consistency.
> > 
> > Also rename the board_ids enum accordingly.
> > 
> > No intentional functional impact.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/thermal/intel/intel_pch_thermal.c |   54 +++++++++++++++--
> > -------------
> >  1 file changed, 27 insertions(+), 27 deletions(-)
> > 
> > Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
> > +++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> > @@ -135,38 +135,38 @@ static struct thermal_zone_device_ops tz
> >  	.critical = pch_critical,
> >  };
> >  
> > -enum board_ids {
> > -	board_hsw,
> > -	board_wpt,
> > -	board_skl,
> > -	board_cnl,
> > -	board_cml,
> > -	board_lwb,
> > -	board_wbg,
> > +enum pch_board_ids {
> > +	PCH_BOARD_HSW = 0,
> > +	PCH_BOARD_WPT,
> > +	PCH_BOARD_SKL,
> > +	PCH_BOARD_CNL,
> > +	PCH_BOARD_CML,
> > +	PCH_BOARD_LWB,
> > +	PCH_BOARD_WBG,
> >  };
> >  
> >  static const struct board_info {
> >  	const char *name;
> >  } board_info[] = {
> 
> Now struct board_info has "name" field only, so maybe we can remove
> struct board_info, and use a "static const char *" array instead?

Good point.

I think that the last patch in the series can be replaced with the
appended one.

> BTW, I'm building a kernel with this patch series as well as 
> https://patchwork.kernel.org/project/linux-pm/list/?series=717084,
> will update the test result later.

Thank you!

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] thermal: intel: intel_pch: Drop struct board_info

Because the only member of struct board_info is the name, the
board_info[] array of struct board_info elements can be replaced with
an array of strings.

Modify the code accordingly and drop struct board_info.

No intentional functional impact.

Suggested-by: Zhang Rui <rui.zhang@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/thermal/intel/intel_pch_thermal.c |   42 +++++++++---------------------
 1 file changed, 13 insertions(+), 29 deletions(-)

Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
+++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
@@ -145,37 +145,20 @@ enum pch_board_ids {
 	PCH_BOARD_WBG,
 };
 
-static const struct board_info {
-	const char *name;
-} board_info[] = {
-	[PCH_BOARD_HSW] = {
-		.name = "pch_haswell",
-	},
-	[PCH_BOARD_WPT] = {
-		.name = "pch_wildcat_point",
-	},
-	[PCH_BOARD_SKL] = {
-		.name = "pch_skylake",
-	},
-	[PCH_BOARD_CNL] = {
-		.name = "pch_cannonlake",
-	},
-	[PCH_BOARD_CML] = {
-		.name = "pch_cometlake",
-	},
-	[PCH_BOARD_LWB] = {
-		.name = "pch_lewisburg",
-	},
-	[PCH_BOARD_WBG] = {
-		.name = "pch_wellsburg",
-	},
+static const char *board_names[] = {
+	[PCH_BOARD_HSW] = "pch_haswell",
+	[PCH_BOARD_WPT] = "pch_wildcat_point",
+	[PCH_BOARD_SKL] = "pch_skylake",
+	[PCH_BOARD_CNL] = "pch_cannonlake",
+	[PCH_BOARD_CML] = "pch_cometlake",
+	[PCH_BOARD_LWB] = "pch_lewisburg",
+	[PCH_BOARD_WBG] = "pch_wellsburg",
 };
 
 static int intel_pch_thermal_probe(struct pci_dev *pdev,
 				   const struct pci_device_id *id)
 {
 	enum pch_board_ids board_id = id->driver_data;
-	const struct board_info *bi = &board_info[board_id];
 	struct pch_thermal_device *ptd;
 	u16 trip_temp;
 	int nr_trips;
@@ -249,12 +232,13 @@ read_trips:
 
 	nr_trips += pch_wpt_add_acpi_psv_trip(ptd, nr_trips);
 
-	ptd->tzd = thermal_zone_device_register_with_trips(bi->name, ptd->trips,
-							   nr_trips, 0, ptd,
-							   &tzd_ops, NULL, 0, 0);
+	ptd->tzd = thermal_zone_device_register_with_trips(board_names[board_id],
+							   ptd->trips, nr_trips,
+							   0, ptd, &tzd_ops,
+							   NULL, 0, 0);
 	if (IS_ERR(ptd->tzd)) {
 		dev_err(&pdev->dev, "Failed to register thermal zone %s\n",
-			bi->name);
+			board_names[board_id]);
 		err = PTR_ERR(ptd->tzd);
 		goto error_cleanup;
 	}

Re: [PATCH v1 7/8] thermal: intel: intel_pch: Rename board ID symbols

[Daniel Lezcano]

On 31/01/2023 14:08, Rafael J. Wysocki wrote:
> On Tuesday, January 31, 2023 12:17:55 PM CET Zhang, Rui wrote:
>> On Mon, 2023-01-30 at 20:04 +0100, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Use capitals in the names of the board ID symbols and add the PCH_
>>> prefix to each of them for consistency.
>>>
>>> Also rename the board_ids enum accordingly.
>>>
>>> No intentional functional impact.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

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

[PATCH v1 8/8] thermal: intel: intel_pch: Refer to thermal zone name directly

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Make intel_pch_thermal_probe() use a const char pointer instead of
a struct board_info one for accessing the thermal zone name.

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

BTW, Srinivas, I'm wondering if user space would be terribly confused by
changing this driver to use "Intel PCH" as the thermal zone name of all
of the supported platforms?

---
 drivers/thermal/intel/intel_pch_thermal.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
===================================================================
--- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
+++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
@@ -175,7 +175,7 @@ static int intel_pch_thermal_probe(struc
 				   const struct pci_device_id *id)
 {
 	enum pch_board_ids board_id = id->driver_data;
-	const struct board_info *bi = &board_info[board_id];
+	const char *zone_name = board_info[board_id].name;
 	struct pch_thermal_device *ptd;
 	u16 trip_temp;
 	int nr_trips;
@@ -249,12 +249,12 @@ read_trips:
 
 	nr_trips += pch_wpt_add_acpi_psv_trip(ptd, nr_trips);
 
-	ptd->tzd = thermal_zone_device_register_with_trips(bi->name, ptd->trips,
+	ptd->tzd = thermal_zone_device_register_with_trips(zone_name, ptd->trips,
 							   nr_trips, 0, ptd,
 							   &tzd_ops, NULL, 0, 0);
 	if (IS_ERR(ptd->tzd)) {
 		dev_err(&pdev->dev, "Failed to register thermal zone %s\n",
-			bi->name);
+			zone_name);
 		err = PTR_ERR(ptd->tzd);
 		goto error_cleanup;
 	}

Re: [PATCH v1 8/8] thermal: intel: intel_pch: Refer to thermal zone name directly

[Daniel Lezcano]

On 30/01/2023 20:07, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Make intel_pch_thermal_probe() use a const char pointer instead of
> a struct board_info one for accessing the thermal zone name.
> 
> No intentional functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> BTW, Srinivas, I'm wondering if user space would be terribly confused by
> changing this driver to use "Intel PCH" as the thermal zone name of all
> of the supported platforms?
> 
> ---
>   drivers/thermal/intel/intel_pch_thermal.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> ===================================================================
> --- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
> +++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> @@ -175,7 +175,7 @@ static int intel_pch_thermal_probe(struc
>   				   const struct pci_device_id *id)
>   {
>   	enum pch_board_ids board_id = id->driver_data;
> -	const struct board_info *bi = &board_info[board_id];
> +	const char *zone_name = board_info[board_id].name;

Assuming you will change 'board_info[board_id].name' by
'board_info[board_id]'

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

>   	struct pch_thermal_device *ptd;
>   	u16 trip_temp;
>   	int nr_trips;
> @@ -249,12 +249,12 @@ read_trips:
>   
>   	nr_trips += pch_wpt_add_acpi_psv_trip(ptd, nr_trips);
>   
> -	ptd->tzd = thermal_zone_device_register_with_trips(bi->name, ptd->trips,
> +	ptd->tzd = thermal_zone_device_register_with_trips(zone_name, ptd->trips,
>   							   nr_trips, 0, ptd,
>   							   &tzd_ops, NULL, 0, 0);
>   	if (IS_ERR(ptd->tzd)) {
>   		dev_err(&pdev->dev, "Failed to register thermal zone %s\n",
> -			bi->name);
> +			zone_name);
>   		err = PTR_ERR(ptd->tzd);
>   		goto error_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 v1 8/8] thermal: intel: intel_pch: Refer to thermal zone name directly

[Rafael J. Wysocki]

On Tue, Jan 31, 2023 at 5:02 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 30/01/2023 20:07, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Make intel_pch_thermal_probe() use a const char pointer instead of
> > a struct board_info one for accessing the thermal zone name.
> >
> > No intentional functional impact.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >
> > BTW, Srinivas, I'm wondering if user space would be terribly confused by
> > changing this driver to use "Intel PCH" as the thermal zone name of all
> > of the supported platforms?
> >
> > ---
> >   drivers/thermal/intel/intel_pch_thermal.c |    6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> > ===================================================================
> > --- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
> > +++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
> > @@ -175,7 +175,7 @@ static int intel_pch_thermal_probe(struc
> >                                  const struct pci_device_id *id)
> >   {
> >       enum pch_board_ids board_id = id->driver_data;
> > -     const struct board_info *bi = &board_info[board_id];
> > +     const char *zone_name = board_info[board_id].name;
>
> Assuming you will change 'board_info[board_id].name' by
> 'board_info[board_id]'

Hmm, why would that be required?

> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Anyway, though, I'm planning to use this replacement patch instead of
the $subject one:

https://lore.kernel.org/linux-pm/12166249.O9o76ZdvQC@kreacher/

Re: [PATCH v1 8/8] thermal: intel: intel_pch: Refer to thermal zone name directly

[Daniel Lezcano]

On 31/01/2023 20:20, Rafael J. Wysocki wrote:
> On Tue, Jan 31, 2023 at 5:02 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> On 30/01/2023 20:07, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Make intel_pch_thermal_probe() use a const char pointer instead of
>>> a struct board_info one for accessing the thermal zone name.
>>>
>>> No intentional functional impact.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>
>>> BTW, Srinivas, I'm wondering if user space would be terribly confused by
>>> changing this driver to use "Intel PCH" as the thermal zone name of all
>>> of the supported platforms?
>>>
>>> ---
>>>    drivers/thermal/intel/intel_pch_thermal.c |    6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> Index: linux-pm/drivers/thermal/intel/intel_pch_thermal.c
>>> ===================================================================
>>> --- linux-pm.orig/drivers/thermal/intel/intel_pch_thermal.c
>>> +++ linux-pm/drivers/thermal/intel/intel_pch_thermal.c
>>> @@ -175,7 +175,7 @@ static int intel_pch_thermal_probe(struc
>>>                                   const struct pci_device_id *id)
>>>    {
>>>        enum pch_board_ids board_id = id->driver_data;
>>> -     const struct board_info *bi = &board_info[board_id];
>>> +     const char *zone_name = board_info[board_id].name;
>>
>> Assuming you will change 'board_info[board_id].name' by
>> 'board_info[board_id]'
> 
> Hmm, why would that be required?

I meant board_names[board_id] (related to the change proposed in the 
patch replacement below)

>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> 
> Anyway, though, I'm planning to use this replacement patch instead of
> the $subject one:
> 
> https://lore.kernel.org/linux-pm/12166249.O9o76ZdvQC@kreacher/

-- 
<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 v1 0/8] thermal: intel: intel_pch: Code simplification and cleanups

[Rafael J. Wysocki]

On Mon, Jan 30, 2023 at 8:08 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi All,
>
> This patch series removes some uneeded code and data structures from the
> intel_pch_thermal driver, rearranges it and does some assorted minor cleanups
> (no change in behavior should result from it).
>
> Please refer to the individual patch changelogs for details.

I forgot to mention that this series is applicable on top of

https://patchwork.kernel.org/project/linux-pm/patch/5641279.DvuYhMxLoT@kreacher/

which in turn applies on top of the current thermal branch in linux-pm.git,
that is also present in the linux-next branch in linux-pm.git.

Thanks!

Re: [PATCH v1 0/8] thermal: intel: intel_pch: Code simplification and cleanups

[Zhang, Rui]

On Mon, 2023-01-30 at 20:13 +0100, Rafael J. Wysocki wrote:
> On Mon, Jan 30, 2023 at 8:08 PM Rafael J. Wysocki <rjw@rjwysocki.net>
> wrote:
> > Hi All,
> > 
> > This patch series removes some uneeded code and data structures
> > from the
> > intel_pch_thermal driver, rearranges it and does some assorted
> > minor cleanups
> > (no change in behavior should result from it).
> > 
> > Please refer to the individual patch changelogs for details.
> 
> I forgot to mention that this series is applicable on top of
> 
> https://patchwork.kernel.org/project/linux-pm/patch/5641279.DvuYhMxLoT@kreacher/
> 
> which in turn applies on top of the current thermal branch in linux-
> pm.git,
> that is also present in the linux-next branch in linux-pm.git.
> 
I also tested your linux-next branch but without the "thermal" merge.

-/sys/class/thermal/thermal_zone7/trip_point_4_temp:-32768000
+/sys/class/thermal/thermal_zone7/trip_point_4_hyst:0
+/sys/class/thermal/thermal_zone7/trip_point_4_temp:-2147483648

The new "hyst" attribute is not a problem as it is mandatory for
generic trips.

The temp value changes is introduced by commit
3d2f20ad46f8 ("wifi: iwlwifi: Use generic thermal_zone_get_trip()
function")

-       for (i = 0 ; i < IWL_MAX_DTS_TRIPS; i++)
-               mvm->tz_device.temp_trips[i] = S16_MIN;
+       for (i = 0 ; i < IWL_MAX_DTS_TRIPS; i++) {
+               mvm->tz_device.trips[i].temperature = INT_MIN;
+               mvm->tz_device.trips[i].type = THERMAL_TRIP_PASSIVE;
+       }

It is kind of strange to use different values, but both represents a
bogus temperature. What about using THERMAL_TEMP_INVALID for future
consistency?

thanks,
rui

Re: [PATCH v1 0/8] thermal: intel: intel_pch: Code simplification and cleanups

[Zhang, Rui]

On Mon, 2023-01-30 at 19:56 +0100, Rafael J. Wysocki wrote:
> Hi All,
> 
> This patch series removes some uneeded code and data structures from
> the
> intel_pch_thermal driver, rearranges it and does some assorted minor
> cleanups
> (no change in behavior should result from it).
> 
> Please refer to the individual patch changelogs for details.
> 
> Thanks!
> 
Tested on one KBL-R platform, everything works fine.

Tested-by: Zhang Rui <rui.zhang@intel.com>
Reviewed-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui

Re: [PATCH v1 0/8] thermal: intel: intel_pch: Code simplification and cleanups

[Zhang, Rui]

On Wed, 2023-02-01 at 17:06 +0800, Zhang Rui wrote:
> On Mon, 2023-01-30 at 19:56 +0100, Rafael J. Wysocki wrote:
> > Hi All,
> > 
> > This patch series removes some uneeded code and data structures
> > from
> > the
> > intel_pch_thermal driver, rearranges it and does some assorted
> > minor
> > cleanups
> > (no change in behavior should result from it).
> > 
> > Please refer to the individual patch changelogs for details.
> > 
> > Thanks!
> > 
> Tested on one KBL-R platform, everything works fine.
> 
> Tested-by: Zhang Rui <rui.zhang@intel.com>
> Reviewed-by: Zhang Rui <rui.zhang@intel.com>

Forgot to mention, I tested patch 1~7 in this series, plus the appended
patch in patch 7/8 thread.

> 
> thanks,
> rui