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: 6957 bytes --]

On 20-06-2013 22:01, amit daniel kachhap wrote:
> Hi Eduardo,
> 
> On Thu, Jun 20, 2013 at 1:25 AM, Eduardo Valentin
> <eduardo.valentin@ti.com> wrote:
>> 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 ?
> Yes you are right. Bit 0 is the actual enable bit. Bit 1 is the
> reserve bit and its default value is 1. Earlier both bits are used to
> turn on/off the controller which was wrong so this patch rectifies it.

Care to mention this in your patch description? It is good for code history.

> 
> Thanks,
> Amit
>>
>>>               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
>>
> 
> 


-- 
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 --]