diff mbox

[v2,29/38] thermal: exynos: Support both Exynos4x12 SoCs

Message ID 1371486863-12398-30-git-send-email-t.figa@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Figa June 17, 2013, 4:34 p.m. UTC 
Exynos4212 and Exynos4412 have the same thermal block, so there is no
reason to include support only for Exynos4412 in this driver.

Cc: linux-pm@vger.kernel.org
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <eduardo.valentin@ti.com>
Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/thermal/exynos_thermal.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Arnd Bergmann June 17, 2013, 7:59 p.m. UTC | #1 
On Monday 17 June 2013, Tomasz Figa wrote:
> diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c
> index 788b1dd..4cbe3ee 100644
> --- a/drivers/thermal/exynos_thermal.c
> +++ b/drivers/thermal/exynos_thermal.c
> @@ -817,7 +817,8 @@ static struct exynos_tmu_platform_data const exynos4210_default_tmu_data = {
>  #define EXYNOS4210_TMU_DRV_DATA (NULL)
>  #endif
>  
> -#if defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412)
> +#if defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412) || \
> +       defined(CONFIG_SOC_EXYNOS4212)
>  static struct exynos_tmu_platform_data const exynos_default_tmu_data = {
>         .threshold_falling = 10,
>         .trigger_levels[0] = 85,

The patch is correct, but generally speaking I think we should get away from
having the drivers get configured per SoC on such a fine-grained level.
Better make this driver (and others) always work on all exynos variants.

	Arnd
Eduardo Valentin June 17, 2013, 11:34 p.m. UTC | #2 
Arnd,

On 17-06-2013 15:59, Arnd Bergmann wrote:
> On Monday 17 June 2013, Tomasz Figa wrote:
>> diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c
>> index 788b1dd..4cbe3ee 100644
>> --- a/drivers/thermal/exynos_thermal.c
>> +++ b/drivers/thermal/exynos_thermal.c
>> @@ -817,7 +817,8 @@ static struct exynos_tmu_platform_data const exynos4210_default_tmu_data = {
>>  #define EXYNOS4210_TMU_DRV_DATA (NULL)
>>  #endif
>>  
>> -#if defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412)
>> +#if defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412) || \
>> +       defined(CONFIG_SOC_EXYNOS4212)
>>  static struct exynos_tmu_platform_data const exynos_default_tmu_data = {
>>         .threshold_falling = 10,
>>         .trigger_levels[0] = 85,
> 
> The patch is correct, but generally speaking I think we should get away from
> having the drivers get configured per SoC on such a fine-grained level.
> Better make this driver (and others) always work on all exynos variants.

Amit, correct if I am wrong, but:

The driver will work on supported exynos variants. Those that have the
need for thermal sensing. And each of them have specific thermal needs
(trigger points, thresholds, etc). That is what this file tries to
isolate. And there is specific data structures for each soc version.


> 
> 	Arnd
> 
>
Amit Kachhap June 18, 2013, 8:54 a.m. UTC | #3 
Hi Eduardo,

On Tue, Jun 18, 2013 at 5:04 AM, Eduardo Valentin
<eduardo.valentin@ti.com> wrote:
> Arnd,
>
> On 17-06-2013 15:59, Arnd Bergmann wrote:
>> On Monday 17 June 2013, Tomasz Figa wrote:
>>> diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c
>>> index 788b1dd..4cbe3ee 100644
>>> --- a/drivers/thermal/exynos_thermal.c
>>> +++ b/drivers/thermal/exynos_thermal.c
>>> @@ -817,7 +817,8 @@ static struct exynos_tmu_platform_data const exynos4210_default_tmu_data = {
>>>  #define EXYNOS4210_TMU_DRV_DATA (NULL)
>>>  #endif
>>>
>>> -#if defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412)
>>> +#if defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412) || \
>>> +       defined(CONFIG_SOC_EXYNOS4212)
>>>  static struct exynos_tmu_platform_data const exynos_default_tmu_data = {
>>>         .threshold_falling = 10,
>>>         .trigger_levels[0] = 85,
>>
>> The patch is correct, but generally speaking I think we should get away from
>> having the drivers get configured per SoC on such a fine-grained level.
>> Better make this driver (and others) always work on all exynos variants.
>
> Amit, correct if I am wrong, but:
>
> The driver will work on supported exynos variants. Those that have the
> need for thermal sensing. And each of them have specific thermal needs
> (trigger points, thresholds, etc). That is what this file tries to
> isolate. And there is specific data structures for each soc version.
Yes its correct that there may be some changes in data points for
different soc's. in the new re-structured code data is separated from
driver so more data can be added cleanly.

>
>
>>
>>       Arnd
>>
>>
>
>
> --
> You have got to be excited about what you are doing. (L. Lamport)
>
> Eduardo Valentin
>
Arnd Bergmann June 18, 2013, 1:02 p.m. UTC | #4 
On Tuesday 18 June 2013, Eduardo Valentin wrote:
> The driver will work on supported exynos variants. Those that have the
> need for thermal sensing. And each of them have specific thermal needs
> (trigger points, thresholds, etc). That is what this file tries to
> isolate. And there is specific data structures for each soc version.

Correct. My point is that the driver itself is much larger than the
SoC-specific data sets in it. There is no reason to conditionally
build a 108 byte data structure, making it more maintainably by removing
all the #ifdef far outweighs the cost.

You can also change the driver to work only for DT based booting,
that will simplify it further and save you more in terms of object
code size than the exynos_tmu_platform_data instances.

	Arnd
diff mbox

Patch

diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c
index 788b1dd..4cbe3ee 100644
--- a/drivers/thermal/exynos_thermal.c
+++ b/drivers/thermal/exynos_thermal.c
@@ -817,7 +817,8 @@  static struct exynos_tmu_platform_data const exynos4210_default_tmu_data = {
 #define EXYNOS4210_TMU_DRV_DATA (NULL)
 #endif
 
-#if defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412)
+#if defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412) || \
+	defined(CONFIG_SOC_EXYNOS4212)
 static struct exynos_tmu_platform_data const exynos_default_tmu_data = {
 	.threshold_falling = 10,
 	.trigger_levels[0] = 85,