[PATCH 1/2] thermal: db8500: Finalize device tree conversion

[PATCH 1/2] thermal: db8500: Finalize device tree conversion

[Linus Walleij]

At some point there was an attempt to convert the DB8500
thermal sensor to device tree: a probe path was added
and the device tree was augmented for the Snowball board.
The switchover was never completed: instead the thermal
devices came from from the PRCMU MFD device and the probe
on the Snowball was confused as another set of configuration
appeared from the device tree.

Move over to a device-tree only approach, as we fixed up
the device trees.

Cc: Lee Jones <lee.jones@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Lee: it'd be great if you could ACK this, it is a file
with low change rate so we should likely not see any
collisions.
---
 drivers/mfd/db8500-prcmu.c                   | 53 +-------------------
 drivers/thermal/Kconfig                      |  2 +-
 drivers/thermal/db8500_thermal.c             | 30 +++++------
 include/linux/platform_data/db8500_thermal.h | 29 -----------
 4 files changed, 17 insertions(+), 97 deletions(-)
 delete mode 100644 include/linux/platform_data/db8500_thermal.h

diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
index 3f21e26b8d36..a1e09bf06977 100644
--- a/drivers/mfd/db8500-prcmu.c
+++ b/drivers/mfd/db8500-prcmu.c
@@ -36,7 +36,6 @@
 #include <linux/regulator/db8500-prcmu.h>
 #include <linux/regulator/machine.h>
 #include <linux/platform_data/ux500_wdt.h>
-#include <linux/platform_data/db8500_thermal.h>
 #include "dbx500-prcmu-regs.h"
 
 /* Index of different voltages to be used when accessing AVSData */
@@ -2982,53 +2981,6 @@ static struct ux500_wdt_data db8500_wdt_pdata = {
 	.timeout = 600, /* 10 minutes */
 	.has_28_bits_resolution = true,
 };
-/*
- * Thermal Sensor
- */
-
-static struct resource db8500_thsens_resources[] = {
-	{
-		.name = "IRQ_HOTMON_LOW",
-		.start  = IRQ_PRCMU_HOTMON_LOW,
-		.end    = IRQ_PRCMU_HOTMON_LOW,
-		.flags  = IORESOURCE_IRQ,
-	},
-	{
-		.name = "IRQ_HOTMON_HIGH",
-		.start  = IRQ_PRCMU_HOTMON_HIGH,
-		.end    = IRQ_PRCMU_HOTMON_HIGH,
-		.flags  = IORESOURCE_IRQ,
-	},
-};
-
-static struct db8500_thsens_platform_data db8500_thsens_data = {
-	.trip_points[0] = {
-		.temp = 70000,
-		.type = THERMAL_TRIP_ACTIVE,
-		.cdev_name = {
-			[0] = "thermal-cpufreq-0",
-		},
-	},
-	.trip_points[1] = {
-		.temp = 75000,
-		.type = THERMAL_TRIP_ACTIVE,
-		.cdev_name = {
-			[0] = "thermal-cpufreq-0",
-		},
-	},
-	.trip_points[2] = {
-		.temp = 80000,
-		.type = THERMAL_TRIP_ACTIVE,
-		.cdev_name = {
-			[0] = "thermal-cpufreq-0",
-		},
-	},
-	.trip_points[3] = {
-		.temp = 85000,
-		.type = THERMAL_TRIP_CRITICAL,
-	},
-	.num_trips = 4,
-};
 
 static const struct mfd_cell common_prcmu_devs[] = {
 	{
@@ -3052,10 +3004,7 @@ static const struct mfd_cell db8500_prcmu_devs[] = {
 	},
 	{
 		.name = "db8500-thermal",
-		.num_resources = ARRAY_SIZE(db8500_thsens_resources),
-		.resources = db8500_thsens_resources,
-		.platform_data = &db8500_thsens_data,
-		.pdata_size = sizeof(db8500_thsens_data),
+		.of_compatible = "stericsson,db8500-thermal",
 	},
 };
 
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 9966364a6deb..001a21abcc28 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -310,7 +310,7 @@ config DOVE_THERMAL
 
 config DB8500_THERMAL
 	tristate "DB8500 thermal management"
-	depends on MFD_DB8500_PRCMU
+	depends on MFD_DB8500_PRCMU && OF
 	default y
 	help
 	  Adds DB8500 thermal management implementation according to the thermal
diff --git a/drivers/thermal/db8500_thermal.c b/drivers/thermal/db8500_thermal.c
index b71a999d17d6..d650ae5fdf2a 100644
--- a/drivers/thermal/db8500_thermal.c
+++ b/drivers/thermal/db8500_thermal.c
@@ -13,13 +13,24 @@
 #include <linux/mfd/dbx500-prcmu.h>
 #include <linux/module.h>
 #include <linux/of.h>
-#include <linux/platform_data/db8500_thermal.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/thermal.h>
 
 #define PRCMU_DEFAULT_MEASURE_TIME	0xFFF
 #define PRCMU_DEFAULT_LOW_TEMP		0
+#define COOLING_DEV_MAX 8
+
+struct db8500_trip_point {
+	unsigned long temp;
+	enum thermal_trip_type type;
+	char cdev_name[COOLING_DEV_MAX][THERMAL_NAME_LENGTH];
+};
+
+struct db8500_thsens_platform_data {
+	struct db8500_trip_point trip_points[THERMAL_MAX_TRIPS];
+	int num_trips;
+};
 
 struct db8500_thermal_zone {
 	struct thermal_zone_device *therm_dev;
@@ -301,7 +312,6 @@ static void db8500_thermal_work(struct work_struct *work)
 	dev_dbg(&pzone->therm_dev->device, "thermal work finished.\n");
 }
 
-#ifdef CONFIG_OF
 static struct db8500_thsens_platform_data*
 		db8500_thermal_parse_dt(struct platform_device *pdev)
 {
@@ -370,13 +380,6 @@ static struct db8500_thsens_platform_data*
 	dev_err(&pdev->dev, "Parsing device tree data error.\n");
 	return NULL;
 }
-#else
-static inline struct db8500_thsens_platform_data*
-		db8500_thermal_parse_dt(struct platform_device *pdev)
-{
-	return NULL;
-}
-#endif
 
 static int db8500_thermal_probe(struct platform_device *pdev)
 {
@@ -386,11 +389,10 @@ static int db8500_thermal_probe(struct platform_device *pdev)
 	int low_irq, high_irq, ret = 0;
 	unsigned long dft_low, dft_high;
 
-	if (np)
-		ptrips = db8500_thermal_parse_dt(pdev);
-	else
-		ptrips = dev_get_platdata(&pdev->dev);
+	if (!np)
+		return -EINVAL;
 
+	ptrips = db8500_thermal_parse_dt(pdev);
 	if (!ptrips)
 		return -EINVAL;
 
@@ -498,13 +500,11 @@ static int db8500_thermal_resume(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_OF
 static const struct of_device_id db8500_thermal_match[] = {
 	{ .compatible = "stericsson,db8500-thermal" },
 	{},
 };
 MODULE_DEVICE_TABLE(of, db8500_thermal_match);
-#endif
 
 static struct platform_driver db8500_thermal_driver = {
 	.driver = {
diff --git a/include/linux/platform_data/db8500_thermal.h b/include/linux/platform_data/db8500_thermal.h
deleted file mode 100644
index 55e55750a165..000000000000
--- a/include/linux/platform_data/db8500_thermal.h
+++ /dev/null
@@ -1,29 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * db8500_thermal.h - DB8500 Thermal Management Implementation
- *
- * Copyright (C) 2012 ST-Ericsson
- * Copyright (C) 2012 Linaro Ltd.
- *
- * Author: Hongbo Zhang <hongbo.zhang@linaro.com>
- */
-
-#ifndef _DB8500_THERMAL_H_
-#define _DB8500_THERMAL_H_
-
-#include <linux/thermal.h>
-
-#define COOLING_DEV_MAX 8
-
-struct db8500_trip_point {
-	unsigned long temp;
-	enum thermal_trip_type type;
-	char cdev_name[COOLING_DEV_MAX][THERMAL_NAME_LENGTH];
-};
-
-struct db8500_thsens_platform_data {
-	struct db8500_trip_point trip_points[THERMAL_MAX_TRIPS];
-	int num_trips;
-};
-
-#endif /* _DB8500_THERMAL_H_ */
-- 
2.21.0

[PATCH 2/2] thermal: db8500: Use dev helper variable

[Linus Walleij]

The code gets easier to read like this.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/thermal/db8500_thermal.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/thermal/db8500_thermal.c b/drivers/thermal/db8500_thermal.c
index d650ae5fdf2a..d250bcf3c10c 100644
--- a/drivers/thermal/db8500_thermal.c
+++ b/drivers/thermal/db8500_thermal.c
@@ -313,16 +313,16 @@ static void db8500_thermal_work(struct work_struct *work)
 }
 
 static struct db8500_thsens_platform_data*
-		db8500_thermal_parse_dt(struct platform_device *pdev)
+		db8500_thermal_parse_dt(struct device *dev)
 {
 	struct db8500_thsens_platform_data *ptrips;
-	struct device_node *np = pdev->dev.of_node;
+	struct device_node *np = dev->of_node;
 	char prop_name[32];
 	const char *tmp_str;
 	u32 tmp_data;
 	int i, j;
 
-	ptrips = devm_kzalloc(&pdev->dev, sizeof(*ptrips), GFP_KERNEL);
+	ptrips = devm_kzalloc(dev, sizeof(*ptrips), GFP_KERNEL);
 	if (!ptrips)
 		return NULL;
 
@@ -377,7 +377,7 @@ static struct db8500_thsens_platform_data*
 	return ptrips;
 
 err_parse_dt:
-	dev_err(&pdev->dev, "Parsing device tree data error.\n");
+	dev_err(dev, "Parsing device tree data error.\n");
 	return NULL;
 }
 
@@ -385,18 +385,19 @@ static int db8500_thermal_probe(struct platform_device *pdev)
 {
 	struct db8500_thermal_zone *pzone = NULL;
 	struct db8500_thsens_platform_data *ptrips = NULL;
-	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
 	int low_irq, high_irq, ret = 0;
 	unsigned long dft_low, dft_high;
 
 	if (!np)
 		return -EINVAL;
 
-	ptrips = db8500_thermal_parse_dt(pdev);
+	ptrips = db8500_thermal_parse_dt(dev);
 	if (!ptrips)
 		return -EINVAL;
 
-	pzone = devm_kzalloc(&pdev->dev, sizeof(*pzone), GFP_KERNEL);
+	pzone = devm_kzalloc(dev, sizeof(*pzone), GFP_KERNEL);
 	if (!pzone)
 		return -ENOMEM;
 
@@ -410,31 +411,31 @@ static int db8500_thermal_probe(struct platform_device *pdev)
 
 	low_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_LOW");
 	if (low_irq < 0) {
-		dev_err(&pdev->dev, "Get IRQ_HOTMON_LOW failed.\n");
+		dev_err(dev, "Get IRQ_HOTMON_LOW failed.\n");
 		ret = low_irq;
 		goto out_unlock;
 	}
 
-	ret = devm_request_threaded_irq(&pdev->dev, low_irq, NULL,
+	ret = devm_request_threaded_irq(dev, low_irq, NULL,
 		prcmu_low_irq_handler, IRQF_NO_SUSPEND | IRQF_ONESHOT,
 		"dbx500_temp_low", pzone);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "Failed to allocate temp low irq.\n");
+		dev_err(dev, "Failed to allocate temp low irq.\n");
 		goto out_unlock;
 	}
 
 	high_irq = platform_get_irq_byname(pdev, "IRQ_HOTMON_HIGH");
 	if (high_irq < 0) {
-		dev_err(&pdev->dev, "Get IRQ_HOTMON_HIGH failed.\n");
+		dev_err(dev, "Get IRQ_HOTMON_HIGH failed.\n");
 		ret = high_irq;
 		goto out_unlock;
 	}
 
-	ret = devm_request_threaded_irq(&pdev->dev, high_irq, NULL,
+	ret = devm_request_threaded_irq(dev, high_irq, NULL,
 		prcmu_high_irq_handler, IRQF_NO_SUSPEND | IRQF_ONESHOT,
 		"dbx500_temp_high", pzone);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "Failed to allocate temp high irq.\n");
+		dev_err(dev, "Failed to allocate temp high irq.\n");
 		goto out_unlock;
 	}
 
@@ -442,11 +443,11 @@ static int db8500_thermal_probe(struct platform_device *pdev)
 		ptrips->num_trips, 0, pzone, &thdev_ops, NULL, 0, 0);
 
 	if (IS_ERR(pzone->therm_dev)) {
-		dev_err(&pdev->dev, "Register thermal zone device failed.\n");
+		dev_err(dev, "Register thermal zone device failed.\n");
 		ret = PTR_ERR(pzone->therm_dev);
 		goto out_unlock;
 	}
-	dev_info(&pdev->dev, "Thermal zone device registered.\n");
+	dev_info(dev, "Thermal zone device registered.\n");
 
 	dft_low = PRCMU_DEFAULT_LOW_TEMP;
 	dft_high = ptrips->trip_points[0].temp;
-- 
2.21.0

Re: [PATCH 1/2] thermal: db8500: Finalize device tree conversion

[Lee Jones]

On Wed, 17 Jul 2019, Linus Walleij wrote:

> At some point there was an attempt to convert the DB8500
> thermal sensor to device tree: a probe path was added
> and the device tree was augmented for the Snowball board.
> The switchover was never completed: instead the thermal
> devices came from from the PRCMU MFD device and the probe
> on the Snowball was confused as another set of configuration
> appeared from the device tree.
> 
> Move over to a device-tree only approach, as we fixed up
> the device trees.
> 
> Cc: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Lee: it'd be great if you could ACK this, it is a file
> with low change rate so we should likely not see any
> collisions.
> ---
>  drivers/mfd/db8500-prcmu.c                   | 53 +-------------------
>  drivers/thermal/Kconfig                      |  2 +-
>  drivers/thermal/db8500_thermal.c             | 30 +++++------
>  include/linux/platform_data/db8500_thermal.h | 29 -----------
>  4 files changed, 17 insertions(+), 97 deletions(-)
>  delete mode 100644 include/linux/platform_data/db8500_thermal.h

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] thermal: db8500: Finalize device tree conversion

[Linus Walleij]

On Wed, Jul 17, 2019 at 8:34 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> At some point there was an attempt to convert the DB8500
> thermal sensor to device tree: a probe path was added
> and the device tree was augmented for the Snowball board.
> The switchover was never completed: instead the thermal
> devices came from from the PRCMU MFD device and the probe
> on the Snowball was confused as another set of configuration
> appeared from the device tree.
>
> Move over to a device-tree only approach, as we fixed up
> the device trees.
>
> Cc: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Thermal folks: can you apply/look into this?

If you're short on time, please just ACK it if it looks OK and
I can send it through the ARM SoC tree.

Yours,
Linus Walleij

Re: [PATCH 1/2] thermal: db8500: Finalize device tree conversion

[Linus Walleij]

On Sat, Aug 3, 2019 at 3:58 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Jul 17, 2019 at 8:34 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> > At some point there was an attempt to convert the DB8500
> > thermal sensor to device tree: a probe path was added
> > and the device tree was augmented for the Snowball board.
> > The switchover was never completed: instead the thermal
> > devices came from from the PRCMU MFD device and the probe
> > on the Snowball was confused as another set of configuration
> > appeared from the device tree.
> >
> > Move over to a device-tree only approach, as we fixed up
> > the device trees.
> >
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> Thermal folks: can you apply/look into this?
>
> If you're short on time, please just ACK it if it looks OK and
> I can send it through the ARM SoC tree.

Ping on this patch, I need to merge dependent code, so it'd
be great of it could be applied. Sorry for hammering, just a
bit desperate.

Yours,
Linus Walleij

Re: [PATCH 1/2] thermal: db8500: Finalize device tree conversion

[Daniel Lezcano]

On 17/07/2019 08:32, Linus Walleij wrote:
> At some point there was an attempt to convert the DB8500
> thermal sensor to device tree: a probe path was added
> and the device tree was augmented for the Snowball board.
> The switchover was never completed: instead the thermal
> devices came from from the PRCMU MFD device and the probe
> on the Snowball was confused as another set of configuration
> appeared from the device tree.
> 
> Move over to a device-tree only approach, as we fixed up
> the device trees.
> 
> Cc: Lee Jones <lee.jones@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Lee: it'd be great if you could ACK this, it is a file
> with low change rate so we should likely not see any
> collisions.
> ---
>  drivers/mfd/db8500-prcmu.c                   | 53 +-------------------
>  drivers/thermal/Kconfig                      |  2 +-
>  drivers/thermal/db8500_thermal.c             | 30 +++++------
>  include/linux/platform_data/db8500_thermal.h | 29 -----------
>  4 files changed, 17 insertions(+), 97 deletions(-)
>  delete mode 100644 include/linux/platform_data/db8500_thermal.h
> 
> diff --git a/drivers/mfd/db8500-prcmu.c b/drivers/mfd/db8500-prcmu.c
> index 3f21e26b8d36..a1e09bf06977 100644
> --- a/drivers/mfd/db8500-prcmu.c
> +++ b/drivers/mfd/db8500-prcmu.c
> @@ -36,7 +36,6 @@
>  #include <linux/regulator/db8500-prcmu.h>
>  #include <linux/regulator/machine.h>
>  #include <linux/platform_data/ux500_wdt.h>
> -#include <linux/platform_data/db8500_thermal.h>
>  #include "dbx500-prcmu-regs.h"
>  
>  /* Index of different voltages to be used when accessing AVSData */
> @@ -2982,53 +2981,6 @@ static struct ux500_wdt_data db8500_wdt_pdata = {
>  	.timeout = 600, /* 10 minutes */
>  	.has_28_bits_resolution = true,
>  };
> -/*
> - * Thermal Sensor
> - */
> -
> -static struct resource db8500_thsens_resources[] = {
> -	{
> -		.name = "IRQ_HOTMON_LOW",
> -		.start  = IRQ_PRCMU_HOTMON_LOW,
> -		.end    = IRQ_PRCMU_HOTMON_LOW,
> -		.flags  = IORESOURCE_IRQ,
> -	},
> -	{
> -		.name = "IRQ_HOTMON_HIGH",
> -		.start  = IRQ_PRCMU_HOTMON_HIGH,
> -		.end    = IRQ_PRCMU_HOTMON_HIGH,
> -		.flags  = IORESOURCE_IRQ,
> -	},
> -};
> -
> -static struct db8500_thsens_platform_data db8500_thsens_data = {
> -	.trip_points[0] = {
> -		.temp = 70000,
> -		.type = THERMAL_TRIP_ACTIVE,
> -		.cdev_name = {
> -			[0] = "thermal-cpufreq-0",
> -		},
> -	},
> -	.trip_points[1] = {
> -		.temp = 75000,
> -		.type = THERMAL_TRIP_ACTIVE,
> -		.cdev_name = {
> -			[0] = "thermal-cpufreq-0",
> -		},
> -	},
> -	.trip_points[2] = {
> -		.temp = 80000,
> -		.type = THERMAL_TRIP_ACTIVE,
> -		.cdev_name = {
> -			[0] = "thermal-cpufreq-0",
> -		},
> -	},
> -	.trip_points[3] = {
> -		.temp = 85000,
> -		.type = THERMAL_TRIP_CRITICAL,
> -	},
> -	.num_trips = 4,
> -};

I've been through the DT and I don't understand why there is:

	[ ... ]
        trip0-temp = <70000>;
        trip0-type = "active";
        trip0-cdev-num = <1>;
        trip0-cdev-name0 = "thermal-cpufreq-0";
	[ ... ]

Those bindings already exists for the thermal no?

Why not create a thermal-zone node and then add the values above?

Another point is there are too many trip points, two should be enough,
one for throttling and one for critical, the governor will handle that
properly by stepping the opps.

And one last point is the trip point should be passive, not active.


>  static const struct mfd_cell common_prcmu_devs[] = {
>  	{
> @@ -3052,10 +3004,7 @@ static const struct mfd_cell db8500_prcmu_devs[] = {
>  	},
>  	{
>  		.name = "db8500-thermal",
> -		.num_resources = ARRAY_SIZE(db8500_thsens_resources),
> -		.resources = db8500_thsens_resources,
> -		.platform_data = &db8500_thsens_data,
> -		.pdata_size = sizeof(db8500_thsens_data),
> +		.of_compatible = "stericsson,db8500-thermal",
>  	},
>  };
>  
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 9966364a6deb..001a21abcc28 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -310,7 +310,7 @@ config DOVE_THERMAL
>  
>  config DB8500_THERMAL
>  	tristate "DB8500 thermal management"
> -	depends on MFD_DB8500_PRCMU
> +	depends on MFD_DB8500_PRCMU && OF
>  	default y
>  	help
>  	  Adds DB8500 thermal management implementation according to the thermal
> diff --git a/drivers/thermal/db8500_thermal.c b/drivers/thermal/db8500_thermal.c
> index b71a999d17d6..d650ae5fdf2a 100644
> --- a/drivers/thermal/db8500_thermal.c
> +++ b/drivers/thermal/db8500_thermal.c
> @@ -13,13 +13,24 @@
>  #include <linux/mfd/dbx500-prcmu.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> -#include <linux/platform_data/db8500_thermal.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/thermal.h>
>  
>  #define PRCMU_DEFAULT_MEASURE_TIME	0xFFF
>  #define PRCMU_DEFAULT_LOW_TEMP		0
> +#define COOLING_DEV_MAX 8
> +
> +struct db8500_trip_point {
> +	unsigned long temp;
> +	enum thermal_trip_type type;
> +	char cdev_name[COOLING_DEV_MAX][THERMAL_NAME_LENGTH];
> +};
> +
> +struct db8500_thsens_platform_data {
> +	struct db8500_trip_point trip_points[THERMAL_MAX_TRIPS];
> +	int num_trips;
> +};
>  
>  struct db8500_thermal_zone {
>  	struct thermal_zone_device *therm_dev;
> @@ -301,7 +312,6 @@ static void db8500_thermal_work(struct work_struct *work)
>  	dev_dbg(&pzone->therm_dev->device, "thermal work finished.\n");
>  }
>  
> -#ifdef CONFIG_OF
>  static struct db8500_thsens_platform_data*
>  		db8500_thermal_parse_dt(struct platform_device *pdev)
>  {
> @@ -370,13 +380,6 @@ static struct db8500_thsens_platform_data*
>  	dev_err(&pdev->dev, "Parsing device tree data error.\n");
>  	return NULL;
>  }
> -#else
> -static inline struct db8500_thsens_platform_data*
> -		db8500_thermal_parse_dt(struct platform_device *pdev)
> -{
> -	return NULL;
> -}
> -#endif
>  
>  static int db8500_thermal_probe(struct platform_device *pdev)
>  {
> @@ -386,11 +389,10 @@ static int db8500_thermal_probe(struct platform_device *pdev)
>  	int low_irq, high_irq, ret = 0;
>  	unsigned long dft_low, dft_high;
>  
> -	if (np)
> -		ptrips = db8500_thermal_parse_dt(pdev);
> -	else
> -		ptrips = dev_get_platdata(&pdev->dev);
> +	if (!np)
> +		return -EINVAL;
>  
> +	ptrips = db8500_thermal_parse_dt(pdev);
>  	if (!ptrips)
>  		return -EINVAL;
>  
> @@ -498,13 +500,11 @@ static int db8500_thermal_resume(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -#ifdef CONFIG_OF
>  static const struct of_device_id db8500_thermal_match[] = {
>  	{ .compatible = "stericsson,db8500-thermal" },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, db8500_thermal_match);
> -#endif
>  
>  static struct platform_driver db8500_thermal_driver = {
>  	.driver = {
> diff --git a/include/linux/platform_data/db8500_thermal.h b/include/linux/platform_data/db8500_thermal.h
> deleted file mode 100644
> index 55e55750a165..000000000000
> --- a/include/linux/platform_data/db8500_thermal.h
> +++ /dev/null
> @@ -1,29 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * db8500_thermal.h - DB8500 Thermal Management Implementation
> - *
> - * Copyright (C) 2012 ST-Ericsson
> - * Copyright (C) 2012 Linaro Ltd.
> - *
> - * Author: Hongbo Zhang <hongbo.zhang@linaro.com>
> - */
> -
> -#ifndef _DB8500_THERMAL_H_
> -#define _DB8500_THERMAL_H_
> -
> -#include <linux/thermal.h>
> -
> -#define COOLING_DEV_MAX 8
> -
> -struct db8500_trip_point {
> -	unsigned long temp;
> -	enum thermal_trip_type type;
> -	char cdev_name[COOLING_DEV_MAX][THERMAL_NAME_LENGTH];
> -};
> -
> -struct db8500_thsens_platform_data {
> -	struct db8500_trip_point trip_points[THERMAL_MAX_TRIPS];
> -	int num_trips;
> -};
> -
> -#endif /* _DB8500_THERMAL_H_ */
> 


-- 
 <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/2] thermal: db8500: Finalize device tree conversion

[Linus Walleij]

On Thu, Aug 22, 2019 at 8:16 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:

> I've been through the DT and I don't understand why there is:
>
>         [ ... ]
>         trip0-temp = <70000>;
>         trip0-type = "active";
>         trip0-cdev-num = <1>;
>         trip0-cdev-name0 = "thermal-cpufreq-0";
>         [ ... ]
>
> Those bindings already exists for the thermal no?
>
> Why not create a thermal-zone node and then add the values above?

I guess you refer to arch/arm/boot/dts/ste-snowball.dts
where this is in the thermal@801573c0 node.
(I moved it to the main SoC file for the next kernel.)

I think the reason is that this was merged in:
commit dc1956b5f84de7d453ec4d9fe68385fffd689686
"Thermal: Add ST-Ericsson DB8500 thermal properties and platform data."
Date:   Thu Nov 15 18:56:43 2012 +0800

Thermal zones were not added until:
commit 4e5e4705bf69ea450f58fc709ac5888f321a9299
"thermal: introduce device tree parser"
Date:   Wed Jul 3 15:35:39 2013 -0400

Which is half a year after this device tree was written.

Apparently Hongbo stopped working on this driver before
that and didn't convert the driver or consumers over to the
new thermal zone APIs with trip points and cooling maps
defined in the device tree (which by the way is an awesome
feature).

> Another point is there are too many trip points, two should be enough,
> one for throttling and one for critical, the governor will handle that
> properly by stepping the opps.
>
> And one last point is the trip point should be passive, not active.

OK I get it, can we merge these two patches that just move
the code to the thermal driver and then I can make
a new patch or some new patches on top to migrate to
Eduardo's new framework for device tree defined thermal
zones?

Yours,
Linus Walleij

Re: [PATCH 1/2] thermal: db8500: Finalize device tree conversion

[Daniel Lezcano]

Hi Linus,

On 27/08/2019 13:21, Linus Walleij wrote:
> On Thu, Aug 22, 2019 at 8:16 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> 
>> I've been through the DT and I don't understand why there is:
>>
>>         [ ... ]
>>         trip0-temp = <70000>;
>>         trip0-type = "active";
>>         trip0-cdev-num = <1>;
>>         trip0-cdev-name0 = "thermal-cpufreq-0";
>>         [ ... ]
>>
>> Those bindings already exists for the thermal no?
>>
>> Why not create a thermal-zone node and then add the values above?
> 
> I guess you refer to arch/arm/boot/dts/ste-snowball.dts
> where this is in the thermal@801573c0 node.
> (I moved it to the main SoC file for the next kernel.)
> 
> I think the reason is that this was merged in:
> commit dc1956b5f84de7d453ec4d9fe68385fffd689686
> "Thermal: Add ST-Ericsson DB8500 thermal properties and platform data."
> Date:   Thu Nov 15 18:56:43 2012 +0800
> 
> Thermal zones were not added until:
> commit 4e5e4705bf69ea450f58fc709ac5888f321a9299
> "thermal: introduce device tree parser"
> Date:   Wed Jul 3 15:35:39 2013 -0400

Oh, right. Time is running away ... :)

> Which is half a year after this device tree was written.
> 
> Apparently Hongbo stopped working on this driver before
> that and didn't convert the driver or consumers over to the
> new thermal zone APIs with trip points and cooling maps
> defined in the device tree (which by the way is an awesome
> feature).
> 
>> Another point is there are too many trip points, two should be enough,
>> one for throttling and one for critical, the governor will handle that
>> properly by stepping the opps.
>>
>> And one last point is the trip point should be passive, not active.
> 
> OK I get it, can we merge these two patches that just move
> the code to the thermal driver and then I can make
> a new patch or some new patches on top to migrate to
> Eduardo's new framework for device tree defined thermal
> zones?

Sounds good to me



-- 
 <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/2] thermal: db8500: Finalize device tree conversion

[Linus Walleij]

On Thu, Aug 22, 2019 at 8:16 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:

> Another point is there are too many trip points, two should be enough,
> one for throttling and one for critical, the governor will handle that
> properly by stepping the opps.

I looked a bit into this, and it seems that there are a number of
trip points simply for inspecting the system (Vincent may know better
because he worked directly with this): the thermal sensor can only
set an IRQ for a certain temperature, there is no interface to just
read the temperature. By setting a number of trip points and
first let the IRQ fire at the lowest you see how the system moves
up in the temperature points and that makes it easier to inspect,
so this sensor will set 70 degress, wait until the IRQ fires,
then set 75 degrees, wait until IRQ fires, set 80 degrees...
etc. It does the same with the IRQ for falling temperature so it
moves up and down this scale, and interpolates the temperature
between the trip points.

It is a bit crude but it gives some kind of granularity and control
over the reported temperature from the sensor.

I think this is something that should be done inside the
driver, not by abusing the trip points, so I try to rewrite the logic
so that the driver fires these IRQs internally without using any
trip points from the thermal subsystem.

Yours,
Linus Walleij

Re: [PATCH 1/2] thermal: db8500: Finalize device tree conversion

[Daniel Lezcano]

On 27/08/2019 15:07, Linus Walleij wrote:
> On Thu, Aug 22, 2019 at 8:16 AM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> 
>> Another point is there are too many trip points, two should be enough,
>> one for throttling and one for critical, the governor will handle that
>> properly by stepping the opps.
> 
> I looked a bit into this, and it seems that there are a number of
> trip points simply for inspecting the system (Vincent may know better
> because he worked directly with this): the thermal sensor can only
> set an IRQ for a certain temperature, there is no interface to just
> read the temperature. 

I see, it is a limitation of the sensor.

> By setting a number of trip points and
> first let the IRQ fire at the lowest you see how the system moves
> up in the temperature points and that makes it easier to inspect,
> so this sensor will set 70 degress, wait until the IRQ fires,
> then set 75 degrees, wait until IRQ fires, set 80 degrees...
> etc. It does the same with the IRQ for falling temperature so it
> moves up and down this scale, and interpolates the temperature
> between the trip points.
> 
> It is a bit crude but it gives some kind of granularity and control
> over the reported temperature from the sensor.
> 
> I think this is something that should be done inside the
> driver, not by abusing the trip points, so I try to rewrite the logic
> so that the driver fires these IRQs internally without using any
> trip points from the thermal subsystem.

In the comments, it is mentioned something is missing with the PRCMU but
when the interface will be available, the get_temp won't have to
interpolate. Wouldn't make sense to add the PRCMU interface instead ?


-- 
 <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/2] thermal: db8500: Finalize device tree conversion

[Linus Walleij]

On Tue, Aug 27, 2019 at 3:54 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 27/08/2019 15:07, Linus Walleij wrote:

> > I think this is something that should be done inside the
> > driver, not by abusing the trip points, so I try to rewrite the logic
> > so that the driver fires these IRQs internally without using any
> > trip points from the thermal subsystem.
>
> In the comments, it is mentioned something is missing with the PRCMU but
> when the interface will be available, the get_temp won't have to
> interpolate. Wouldn't make sense to add the PRCMU interface instead ?

Yes. But this is a firmware that I cannot rewrite or control,
and which is preinstalled on a lot of shipped devices, so we
cannot fix it that way. It was more a suggestion for internal
development in later generations at the time :(

I have made a new 3-part patch set that fixes this up as
good as I can, will send it out!

Yours,
Linus Walleij

Re: [PATCH 1/2] thermal: db8500: Finalize device tree conversion

[Daniel Lezcano]

On 28/08/2019 10:57, Linus Walleij wrote:
> On Tue, Aug 27, 2019 at 3:54 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 27/08/2019 15:07, Linus Walleij wrote:
> 
>>> I think this is something that should be done inside the
>>> driver, not by abusing the trip points, so I try to rewrite the logic
>>> so that the driver fires these IRQs internally without using any
>>> trip points from the thermal subsystem.
>>
>> In the comments, it is mentioned something is missing with the PRCMU but
>> when the interface will be available, the get_temp won't have to
>> interpolate. Wouldn't make sense to add the PRCMU interface instead ?
> 
> Yes. But this is a firmware that I cannot rewrite or control,
> and which is preinstalled on a lot of shipped devices, so we
> cannot fix it that way. It was more a suggestion for internal
> development in later generations at the time :(

The THSENS_TEMP register gives invalid values because of the firmware?



-- 
 <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/2] thermal: db8500: Finalize device tree conversion

[Linus Walleij]

On Wed, Aug 28, 2019 at 11:04 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:

> The THSENS_TEMP register gives invalid values because of the firmware?

There is a range of registers we can try to experiment with, but
I don't know if any of them really work as intended.

My long-term plan is to break out the mailbox mechanism so
that sub drivers can talk directly to the PRCMU and then we
can start to be more elaborate in how we use it, including
checking if these registers make sense, I just want to drive
home this cleanup first.

Yours,
Linus Walleij