[PATCH] thermal: intel_pch_thermal: Support skylake PCH 100 series thermal

[PATCH] thermal: intel_pch_thermal: Support skylake PCH 100 series thermal

[OGAWA Hirofumi]

Using registers are compatible with current driver.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 drivers/thermal/intel_pch_thermal.c |   48 +++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff -puN drivers/thermal/intel_pch_thermal.c~intel_pch_thermal-cleanup drivers/thermal/intel_pch_thermal.c
--- linux/drivers/thermal/intel_pch_thermal.c~intel_pch_thermal-cleanup	2016-10-18 05:41:58.576845111 +0900
+++ linux-hirofumi/drivers/thermal/intel_pch_thermal.c	2016-10-18 05:57:52.391883794 +0900
@@ -220,32 +220,39 @@ static struct thermal_zone_device_ops tz
 	.get_trip_temp = pch_get_trip_temp,
 };
 
+enum board_ids {
+	board_wpt,
+	board_skl,
+};
+
+static const struct board_info {
+	const char *name;
+	const struct pch_dev_ops *ops;
+} board_info[] = {
+	[board_wpt] = {
+		.name = "pch_wildcat_point",
+		.ops = &pch_dev_ops_wpt,
+	},
+	[board_skl] = {
+		.name = "pch_skylake",
+		.ops = &pch_dev_ops_wpt,
+	},
+};
 
 static int intel_pch_thermal_probe(struct pci_dev *pdev,
 				   const struct pci_device_id *id)
 {
+	enum board_ids board_id = id->driver_data;
+	const struct board_info *bi = &board_info[board_id];
 	struct pch_thermal_device *ptd;
 	int err;
 	int nr_trips;
-	char *dev_name;
 
 	ptd = devm_kzalloc(&pdev->dev, sizeof(*ptd), GFP_KERNEL);
 	if (!ptd)
 		return -ENOMEM;
 
-	switch (pdev->device) {
-	case PCH_THERMAL_DID_WPT:
-		ptd->ops = &pch_dev_ops_wpt;
-		dev_name = "pch_wildcat_point";
-		break;
-	case PCH_THERMAL_DID_SKL:
-		ptd->ops = &pch_dev_ops_wpt;
-		dev_name = "pch_skylake";
-		break;
-	default:
-		dev_err(&pdev->dev, "unknown pch thermal device\n");
-		return -ENODEV;
-	}
+	ptd->ops = bi->ops;
 
 	pci_set_drvdata(pdev, ptd);
 	ptd->pdev = pdev;
@@ -273,11 +280,11 @@ static int intel_pch_thermal_probe(struc
 	if (err)
 		goto error_cleanup;
 
-	ptd->tzd = thermal_zone_device_register(dev_name, nr_trips, 0, ptd,
+	ptd->tzd = thermal_zone_device_register(bi->name, 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",
-			dev_name);
+			bi->name);
 		err = PTR_ERR(ptd->tzd);
 		goto error_cleanup;
 	}
@@ -322,8 +329,13 @@ static int intel_pch_thermal_resume(stru
 }
 
 static struct pci_device_id intel_pch_thermal_id[] = {
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT) },
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT),
+	  .driver_data = board_wpt, },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL),
+	  .driver_data = board_skl, },
+	/* skylake PCH 100 series */
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa131),
+	  .driver_data = board_skl, },
 	{ 0, },
 };
 MODULE_DEVICE_TABLE(pci, intel_pch_thermal_id);
_

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] thermal: intel_pch_thermal: Support skylake PCH 100 series thermal

[Pandruvada, Srinivas]

On Tue, 2016-10-18 at 06:44 +0900, OGAWA Hirofumi wrote:
> Using registers are compatible with current driver.
This description is not enough. Without going through the whole code, I
was not able to find out what this patch is doing.

Basically you want to tie the themal_zone name and device operations to
a board, rather than to PCI devices ids. In this way multiple PCI ids
for the same board can be accounted for.

Also need to consider another patch sent to mailing list for supporting
Haswell PCH.

One comment below in the code.

[...]
 
>  static struct pci_device_id intel_pch_thermal_id[] = {
> -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT) },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT),
> +	  .driver_data = board_wpt, },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL),
> +	  .driver_data = board_skl, },
> +	/* skylake PCH 100 series */
> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa131),
Change a131 to a define (PCH_THERMAL_DID_SKL_H)

Thanks,
Srinivas

Re: [PATCH] thermal: intel_pch_thermal: Support skylake PCH 100 series thermal

[OGAWA Hirofumi]

"Pandruvada, Srinivas" <srinivas.pandruvada@intel.com> writes:

> On Tue, 2016-10-18 at 06:44 +0900, OGAWA Hirofumi wrote:
>> Using registers are compatible with current driver.
> This description is not enough. Without going through the whole code, I
> was not able to find out what this patch is doing.
>
> Basically you want to tie the themal_zone name and device operations to
> a board, rather than to PCI devices ids. In this way multiple PCI ids
> for the same board can be accounted for.

Exactly. I thought the patch is enough easy to read, and changelog would
be better to not explain what code itself, and wrote note of code
(registers are same with current).

Well, ok. I will explain what patch does. Your explain looks like good,
so I will borrow it :)

> Also need to consider another patch sent to mailing list for supporting
> Haswell PCH.

Sounds good. I think the patch makes it easier.

> One comment below in the code.
>
> [...]
>  
>>  static struct pci_device_id intel_pch_thermal_id[] = {
>> -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT) },
>> -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL) },
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT),
>> +	  .driver_data = board_wpt, },
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL),
>> +	  .driver_data = board_skl, },
>> +	/* skylake PCH 100 series */
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa131),
> Change a131 to a define (PCH_THERMAL_DID_SKL_H)

I don't think it helps anything actually, because no users of define
anymore. Well, anyway, ok, I don't care either case.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] thermal: intel_pch_thermal: Support skylake PCH 100 series thermal

[Pandruvada, Srinivas]

On Wed, 2016-10-19 at 05:10 +0900, OGAWA Hirofumi wrote:
> "Pandruvada, Srinivas" <srinivas.pandruvada@intel.com> writes:
> 
> > 
> > On Tue, 2016-10-18 at 06:44 +0900, OGAWA Hirofumi wrote:
> > > 
> > > Using registers are compatible with current driver.
> > This description is not enough. Without going through the whole
> > code, I
> > was not able to find out what this patch is doing.
> > 
> > Basically you want to tie the themal_zone name and device
> > operations to
> > a board, rather than to PCI devices ids. In this way multiple PCI
> > ids
> > for the same board can be accounted for.
> 
> Exactly. I thought the patch is enough easy to read, and changelog
> would
> be better to not explain what code itself, and wrote note of code
> (registers are same with current).

Reviewers will look at the description before to look at the code.

[...]

> > >  static struct pci_device_id intel_pch_thermal_id[] = {
> > > -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT)
> > > },
> > > -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL)
> > > },
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT),
> > > +	  .driver_data = board_wpt, },
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL),
> > > +	  .driver_data = board_skl, },
> > > +	/* skylake PCH 100 series */
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa131),
> > Change a131 to a define (PCH_THERMAL_DID_SKL_H)
> 
> I don't think it helps anything actually, because no users of define
> anymore. Well, anyway, ok, I don't care either case.

Only for the sake consistency with rest of the ids defined here.

Thanks,
Srinivas

Re: [PATCH] thermal: intel_pch_thermal: Support skylake PCH 100 series thermal

[OGAWA Hirofumi]

"Pandruvada, Srinivas" <srinivas.pandruvada@intel.com> writes:

>> Exactly. I thought the patch is enough easy to read, and changelog
>> would
>> be better to not explain what code itself, and wrote note of code
>> (registers are same with current).
>
> Reviewers will look at the description before to look at the code.

Oops, I was thinking I sent updated patch before this.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[PATCH v2] thermal: intel_pch_thermal: Support skylake PCH 100 series thermal

[OGAWA Hirofumi]

This patch uses .driver_data and board_info[] to make per pci device
behavior table (name and ops), instead of adding the code for each pci
device in switch-case. This will make easier to add new pci device
ids.

Then this adds new device id actually for skylake PCH 100 series
(using registers are compatible with currently driver, so no need to
change except adding device id to table).

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 drivers/thermal/intel_pch_thermal.c |   48 +++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff -puN drivers/thermal/intel_pch_thermal.c~intel_pch_thermal-cleanup drivers/thermal/intel_pch_thermal.c
--- linux/drivers/thermal/intel_pch_thermal.c~intel_pch_thermal-cleanup	2016-10-18 06:02:07.469831874 +0900
+++ linux-hirofumi/drivers/thermal/intel_pch_thermal.c	2016-10-19 05:13:39.028672579 +0900
@@ -26,6 +26,7 @@
 /* Intel PCH thermal Device IDs */
 #define PCH_THERMAL_DID_WPT	0x9CA4 /* Wildcat Point */
 #define PCH_THERMAL_DID_SKL	0x9D31 /* Skylake PCH */
+#define PCH_THERMAL_DID_SKL_H	0xA131 /* Skylake PCH 100 series */
 
 /* Wildcat Point-LP  PCH Thermal registers */
 #define WPT_TEMP	0x0000	/* Temperature */
@@ -220,32 +221,39 @@ static struct thermal_zone_device_ops tz
 	.get_trip_temp = pch_get_trip_temp,
 };
 
+enum board_ids {
+	board_wpt,
+	board_skl,
+};
+
+static const struct board_info {
+	const char *name;
+	const struct pch_dev_ops *ops;
+} board_info[] = {
+	[board_wpt] = {
+		.name = "pch_wildcat_point",
+		.ops = &pch_dev_ops_wpt,
+	},
+	[board_skl] = {
+		.name = "pch_skylake",
+		.ops = &pch_dev_ops_wpt,
+	},
+};
 
 static int intel_pch_thermal_probe(struct pci_dev *pdev,
 				   const struct pci_device_id *id)
 {
+	enum board_ids board_id = id->driver_data;
+	const struct board_info *bi = &board_info[board_id];
 	struct pch_thermal_device *ptd;
 	int err;
 	int nr_trips;
-	char *dev_name;
 
 	ptd = devm_kzalloc(&pdev->dev, sizeof(*ptd), GFP_KERNEL);
 	if (!ptd)
 		return -ENOMEM;
 
-	switch (pdev->device) {
-	case PCH_THERMAL_DID_WPT:
-		ptd->ops = &pch_dev_ops_wpt;
-		dev_name = "pch_wildcat_point";
-		break;
-	case PCH_THERMAL_DID_SKL:
-		ptd->ops = &pch_dev_ops_wpt;
-		dev_name = "pch_skylake";
-		break;
-	default:
-		dev_err(&pdev->dev, "unknown pch thermal device\n");
-		return -ENODEV;
-	}
+	ptd->ops = bi->ops;
 
 	pci_set_drvdata(pdev, ptd);
 	ptd->pdev = pdev;
@@ -273,11 +281,11 @@ static int intel_pch_thermal_probe(struc
 	if (err)
 		goto error_cleanup;
 
-	ptd->tzd = thermal_zone_device_register(dev_name, nr_trips, 0, ptd,
+	ptd->tzd = thermal_zone_device_register(bi->name, 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",
-			dev_name);
+			bi->name);
 		err = PTR_ERR(ptd->tzd);
 		goto error_cleanup;
 	}
@@ -322,8 +330,12 @@ static int intel_pch_thermal_resume(stru
 }
 
 static struct pci_device_id intel_pch_thermal_id[] = {
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT) },
-	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL) },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT),
+	  .driver_data = board_wpt, },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL),
+	  .driver_data = board_skl, },
+	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL_H),
+	  .driver_data = board_skl, },
 	{ 0, },
 };
 MODULE_DEVICE_TABLE(pci, intel_pch_thermal_id);
_

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v2] thermal: intel_pch_thermal: Support skylake PCH 100 series thermal

[Pandruvada, Srinivas]

On Wed, 2016-10-19 at 05:59 +0900, OGAWA Hirofumi wrote:
> This patch uses .driver_data and board_info[] to make per pci device
> behavior table (name and ops), instead of adding the code for each
> pci
> device in switch-case. This will make easier to add new pci device
> ids.
> 
> Then this adds new device id actually for skylake PCH 100 series
> (using registers are compatible with currently driver, so no need to
> change except adding device id to table).
> 
> Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
> 
>  drivers/thermal/intel_pch_thermal.c |   48 +++++++++++++++++++++--
> ------------
>  1 file changed, 30 insertions(+), 18 deletions(-)
> 
> diff -puN drivers/thermal/intel_pch_thermal.c~intel_pch_thermal-
> cleanup drivers/thermal/intel_pch_thermal.c
> --- linux/drivers/thermal/intel_pch_thermal.c~intel_pch_thermal-
> cleanup	2016-10-18 06:02:07.469831874 +0900
> +++ linux-hirofumi/drivers/thermal/intel_pch_thermal.c	2016-
> 10-19 05:13:39.028672579 +0900
> @@ -26,6 +26,7 @@
>  /* Intel PCH thermal Device IDs */
>  #define PCH_THERMAL_DID_WPT	0x9CA4 /* Wildcat Point */
>  #define PCH_THERMAL_DID_SKL	0x9D31 /* Skylake PCH */
> +#define PCH_THERMAL_DID_SKL_H	0xA131 /* Skylake PCH 100
> series */
>  
>  /* Wildcat Point-LP  PCH Thermal registers */
>  #define WPT_TEMP	0x0000	/* Temperature */
> @@ -220,32 +221,39 @@ static struct thermal_zone_device_ops tz
>  	.get_trip_temp = pch_get_trip_temp,
>  };
>  
> +enum board_ids {
> +	board_wpt,
> +	board_skl,
> +};
> +
> +static const struct board_info {
> +	const char *name;
> +	const struct pch_dev_ops *ops;
> +} board_info[] = {
> +	[board_wpt] = {
> +		.name = "pch_wildcat_point",
> +		.ops = &pch_dev_ops_wpt,
> +	},
> +	[board_skl] = {
> +		.name = "pch_skylake",
> +		.ops = &pch_dev_ops_wpt,
> +	},
> +};
>  
>  static int intel_pch_thermal_probe(struct pci_dev *pdev,
>  				   const struct pci_device_id *id)
>  {
> +	enum board_ids board_id = id->driver_data;
> +	const struct board_info *bi = &board_info[board_id];
>  	struct pch_thermal_device *ptd;
>  	int err;
>  	int nr_trips;
> -	char *dev_name;
>  
>  	ptd = devm_kzalloc(&pdev->dev, sizeof(*ptd), GFP_KERNEL);
>  	if (!ptd)
>  		return -ENOMEM;
>  
> -	switch (pdev->device) {
> -	case PCH_THERMAL_DID_WPT:
> -		ptd->ops = &pch_dev_ops_wpt;
> -		dev_name = "pch_wildcat_point";
> -		break;
> -	case PCH_THERMAL_DID_SKL:
> -		ptd->ops = &pch_dev_ops_wpt;
> -		dev_name = "pch_skylake";
> -		break;
> -	default:
> -		dev_err(&pdev->dev, "unknown pch thermal device\n");
> -		return -ENODEV;
> -	}
> +	ptd->ops = bi->ops;
>  
>  	pci_set_drvdata(pdev, ptd);
>  	ptd->pdev = pdev;
> @@ -273,11 +281,11 @@ static int intel_pch_thermal_probe(struc
>  	if (err)
>  		goto error_cleanup;
>  
> -	ptd->tzd = thermal_zone_device_register(dev_name, nr_trips,
> 0, ptd,
> +	ptd->tzd = thermal_zone_device_register(bi->name, 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",
> -			dev_name);
> +			bi->name);
>  		err = PTR_ERR(ptd->tzd);
>  		goto error_cleanup;
>  	}
> @@ -322,8 +330,12 @@ static int intel_pch_thermal_resume(stru
>  }
>  
>  static struct pci_device_id intel_pch_thermal_id[] = {
> -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT) },
> -	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL) },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_WPT),
> +	  .driver_data = board_wpt, },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL),
> +	  .driver_data = board_skl, },
> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCH_THERMAL_DID_SKL_H),
> +	  .driver_data = board_skl, },
>  	{ 0, },
>  };
>  MODULE_DEVICE_TABLE(pci, intel_pch_thermal_id);
> _
> 

Re: [PATCH v2] thermal: intel_pch_thermal: Support skylake PCH 100 series thermal

[Zhang Rui]

On Tue, 2016-10-18 at 15:04 -0600, Pandruvada, Srinivas wrote:
> On Wed, 2016-10-19 at 05:59 +0900, OGAWA Hirofumi wrote:
> > 
> > This patch uses .driver_data and board_info[] to make per pci device
> > behavior table (name and ops), instead of adding the code for each
> > pci
> > device in switch-case. This will make easier to add new pci device
> > ids.
> > 
> > Then this adds new device id actually for skylake PCH 100 series
> > (using registers are compatible with currently driver, so no need to
> > change except adding device id to table).
> > 
> > Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
hmmm, this patch is queued in my tree for some time.
However, it conflicts with
commit 33086a9a3071812a829d4b63bed826736a5d8b17
Author: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Date:   Mon Oct 3 12:36:02 2016 -0700

    thermal: intel_pch_thermal: Enable Haswell PCH
    
    Added missing support for Haswell PCH thermal sensor.
which has been shipped in v4.9-rc3.

Rebased patch sent, please double check.

thanks,
rui

Re: [PATCH v2] thermal: intel_pch_thermal: Support skylake PCH 100 series thermal

[OGAWA Hirofumi]

Zhang Rui <rui.zhang@intel.com> writes:

> hmmm, this patch is queued in my tree for some time.
> However, it conflicts with
> commit 33086a9a3071812a829d4b63bed826736a5d8b17
> Author: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Date:   Mon Oct 3 12:36:02 2016 -0700
>
>     thermal: intel_pch_thermal: Enable Haswell PCH
>     
>     Added missing support for Haswell PCH thermal sensor.
> which has been shipped in v4.9-rc3.
>
> Rebased patch sent, please double check.

"[PATCH v3] thermal: intel_pch_thermal: ..." looks good to me.

[I was thinking I checked latest kernel though, it looks like I was
missing something.]

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>