mbox series

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

Message ID 1751684.VLH7GnMWUR@kreacher
Headers show
Series thermal: intel: intel_pch: Code simplification and cleanups | expand

Message

Rafael J. Wysocki Jan. 30, 2023, 6:56 p.m. UTC 
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!

Comments

Zhang Rui Jan. 31, 2023, 11:17 a.m. UTC | #1 
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);
> 
> 
>
Rafael J. Wysocki Jan. 31, 2023, 1:08 p.m. UTC | #2 
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;
 	}
Zhang Rui Feb. 1, 2023, 9:06 a.m. UTC | #3 
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
Zhang Rui Feb. 1, 2023, 9:08 a.m. UTC | #4 
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
Zhang Rui Feb. 1, 2023, 9:30 a.m. UTC | #5 
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