Re: [PATCH V6 08/30] thermal: exynos: Add missing definations and code cleanup

[Eduardo Valentin <eduardo.valentin@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 5365 bytes --]

On 17-06-2013 02:46, Amit Daniel Kachhap wrote:
> This patch adds some extra register bitfield definations and cleans
> up the code to prepare for moving register macros and definations inside
> the TMU data section.
> 
> Acked-by: Kukjin Kim <kgene.kim@samsung.com>
> Acked-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> ---
>  drivers/thermal/samsung/exynos_tmu.c |   62 +++++++++++++++++++++++++---------
>  1 files changed, 46 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 5df04a1..fa33a48 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -43,9 +43,12 @@
>  
>  #define EXYNOS_TMU_TRIM_TEMP_MASK	0xff
>  #define EXYNOS_TMU_GAIN_SHIFT		8
> +#define EXYNOS_TMU_GAIN_MASK		0xf
>  #define EXYNOS_TMU_REF_VOLTAGE_SHIFT	24
> -#define EXYNOS_TMU_CORE_ON		3
> -#define EXYNOS_TMU_CORE_OFF		2
> +#define EXYNOS_TMU_REF_VOLTAGE_MASK	0x1f
> +#define EXYNOS_TMU_BUF_SLOPE_SEL_MASK	0xf
> +#define EXYNOS_TMU_BUF_SLOPE_SEL_SHIFT	8
> +#define EXYNOS_TMU_CORE_EN_SHIFT	0
>  #define EXYNOS_TMU_DEF_CODE_TO_TEMP_OFFSET	50
>  
>  /* Exynos4210 specific registers */
> @@ -63,6 +66,7 @@
>  #define EXYNOS4210_TMU_TRIG_LEVEL1_MASK	0x10
>  #define EXYNOS4210_TMU_TRIG_LEVEL2_MASK	0x100
>  #define EXYNOS4210_TMU_TRIG_LEVEL3_MASK	0x1000
> +#define EXYNOS4210_TMU_TRIG_LEVEL_MASK	0x1111
>  #define EXYNOS4210_TMU_INTCLEAR_VAL	0x1111
>  
>  /* Exynos5250 and Exynos4412 specific registers */
> @@ -72,17 +76,30 @@
>  #define EXYNOS_EMUL_CON		0x80
>  
>  #define EXYNOS_TRIMINFO_RELOAD		0x1
> +#define EXYNOS_TRIMINFO_SHIFT		0x0
> +#define EXYNOS_TMU_RISE_INT_MASK	0x111
> +#define EXYNOS_TMU_RISE_INT_SHIFT	0
> +#define EXYNOS_TMU_FALL_INT_MASK	0x111
> +#define EXYNOS_TMU_FALL_INT_SHIFT	12
>  #define EXYNOS_TMU_CLEAR_RISE_INT	0x111
>  #define EXYNOS_TMU_CLEAR_FALL_INT	(0x111 << 12)
> -#define EXYNOS_MUX_ADDR_VALUE		6
> -#define EXYNOS_MUX_ADDR_SHIFT		20
>  #define EXYNOS_TMU_TRIP_MODE_SHIFT	13
> +#define EXYNOS_TMU_TRIP_MODE_MASK	0x7
> +
> +#define EXYNOS_TMU_INTEN_RISE0_SHIFT	0
> +#define EXYNOS_TMU_INTEN_RISE1_SHIFT	4
> +#define EXYNOS_TMU_INTEN_RISE2_SHIFT	8
> +#define EXYNOS_TMU_INTEN_RISE3_SHIFT	12
> +#define EXYNOS_TMU_INTEN_FALL0_SHIFT	16
> +#define EXYNOS_TMU_INTEN_FALL1_SHIFT	20
> +#define EXYNOS_TMU_INTEN_FALL2_SHIFT	24
>  
>  #define EFUSE_MIN_VALUE 40
>  #define EFUSE_MAX_VALUE 100
>  
>  #ifdef CONFIG_THERMAL_EMULATION
>  #define EXYNOS_EMUL_TIME	0x57F0
> +#define EXYNOS_EMUL_TIME_MASK	0xffff
>  #define EXYNOS_EMUL_TIME_SHIFT	16
>  #define EXYNOS_EMUL_DATA_SHIFT	8
>  #define EXYNOS_EMUL_DATA_MASK	0xFF
> @@ -261,24 +278,37 @@ static void exynos_tmu_control(struct platform_device *pdev, bool on)
>  	mutex_lock(&data->lock);
>  	clk_enable(data->clk);
>  
> -	con = pdata->reference_voltage << EXYNOS_TMU_REF_VOLTAGE_SHIFT |
> -		pdata->gain << EXYNOS_TMU_GAIN_SHIFT;
> +	con = readl(data->base + EXYNOS_TMU_REG_CONTROL);
>  
> -	if (data->soc == SOC_ARCH_EXYNOS) {
> -		con |= pdata->noise_cancel_mode << EXYNOS_TMU_TRIP_MODE_SHIFT;
> -		con |= (EXYNOS_MUX_ADDR_VALUE << EXYNOS_MUX_ADDR_SHIFT);
> +	if (pdata->reference_voltage) {
> +		con &= ~(EXYNOS_TMU_REF_VOLTAGE_MASK <<
> +				EXYNOS_TMU_REF_VOLTAGE_SHIFT);
> +		con |= pdata->reference_voltage << EXYNOS_TMU_REF_VOLTAGE_SHIFT;
> +	}
> +
> +	if (pdata->gain) {
> +		con &= ~(EXYNOS_TMU_GAIN_MASK << EXYNOS_TMU_GAIN_SHIFT);
> +		con |= (pdata->gain << EXYNOS_TMU_GAIN_SHIFT);
> +	}
> +
> +	if (pdata->noise_cancel_mode) {
> +		con &= ~(EXYNOS_TMU_TRIP_MODE_MASK <<
> +					EXYNOS_TMU_TRIP_MODE_SHIFT);
> +		con |= (pdata->noise_cancel_mode << EXYNOS_TMU_TRIP_MODE_SHIFT);
>  	}
>  
>  	if (on) {
> -		con |= EXYNOS_TMU_CORE_ON;



Before, in order to turn core on you had:
	con = con | 3;

now you do:
	con = con | (1 << 0);

To me, before you would set bit 1 and 0, now you set bit 0.


> -		interrupt_en = pdata->trigger_level3_en << 12 |
> -			pdata->trigger_level2_en << 8 |
> -			pdata->trigger_level1_en << 4 |
> -			pdata->trigger_level0_en;
> +		con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT);
> +		interrupt_en =
> +		pdata->trigger_level3_en << EXYNOS_TMU_INTEN_RISE3_SHIFT |
> +		pdata->trigger_level2_en << EXYNOS_TMU_INTEN_RISE2_SHIFT |
> +		pdata->trigger_level1_en << EXYNOS_TMU_INTEN_RISE1_SHIFT |
> +		pdata->trigger_level0_en << EXYNOS_TMU_INTEN_RISE0_SHIFT;
>  		if (pdata->threshold_falling)
> -			interrupt_en |= interrupt_en << 16;
> +			interrupt_en |=
> +				interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT;
>  	} else {
> -		con |= EXYNOS_TMU_CORE_OFF;
> +		con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT);


Before, in order to turno core off you had:
	con = con | 2;

now you do:
	con = con & ~(1 << 0);

To me, before you would set bit 2, now you clear bit 0.

Using the approach on this patch looks correct to me if you have 1 bit
core_en for instance.

so, Is this a fix?

Just to be clear, is this what you want ?

>  		interrupt_en = 0; /* Disable all interrupts */
>  	}
>  	writel(interrupt_en, data->base + EXYNOS_TMU_REG_INTEN);
> 


-- 
You have got to be excited about what you are doing. (L. Lamport)

Eduardo Valentin


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 295 bytes --]