On 08/25/2014 08:19 PM, Bartlomiej Zolnierkiewicz wrote:
> On Monday, August 25, 2014 07:37:25 PM Chanwoo Choi wrote:
>> Hi Bartlomiej,
>>
>> On 08/25/2014 07:15 PM, Bartlomiej Zolnierkiewicz wrote:
>>>
>>> Hi,
>>>
>>> On Monday, August 25, 2014 04:30:23 PM Chanwoo Choi wrote:
>>>> This patch support many TRIMINFO_CTRL registers if specific Exynos SoC
>>>> has one more TRIMINFO_CTRL registers. Also this patch uses proper 'RELOAD'
>>>> shift/mask bit operation to set RELOAD feature instead of static value.
>>>>
>>>> Signed-off-by: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>>>> Acked-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>>>> Cc: Zhang Rui <rui.zhang@xxxxxxxxx>
>>>> Cc: Eduardo Valentin <edubezval@xxxxxxxxx>
>>>> Cc: Amit Daniel Kachhap <amit.daniel@xxxxxxxxxxx>
>>>> Reviewed-by: Amit Daniel Kachhap <amit.daniel@xxxxxxxxxxx>
>>>> ---
>>>> drivers/thermal/samsung/exynos_thermal_common.h | 1 +
>>>> drivers/thermal/samsung/exynos_tmu.c | 23 ++++++++++++++++++++---
>>>> drivers/thermal/samsung/exynos_tmu.h | 9 +++++++--
>>>> drivers/thermal/samsung/exynos_tmu_data.c | 5 ++++-
>>>> drivers/thermal/samsung/exynos_tmu_data.h | 3 +++
>>>> 5 files changed, 35 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/samsung/exynos_thermal_common.h b/drivers/thermal/samsung/exynos_thermal_common.h
>>>> index 3eb2ed9..b211976 100644
>>>> --- a/drivers/thermal/samsung/exynos_thermal_common.h
>>>> +++ b/drivers/thermal/samsung/exynos_thermal_common.h
>>>> @@ -28,6 +28,7 @@
>>>> #define MAX_TRIP_COUNT 8
>>>> #define MAX_COOLING_DEVICE 4
>>>> #define MAX_THRESHOLD_LEVS 5
>>>> +#define MAX_TRIMINFO_CTRL_REG 2
>>>>
>>>> #define ACTIVE_INTERVAL 500
>>>> #define IDLE_INTERVAL 10000
>>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
>>>> index acbff14..7234f38 100644
>>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>>> @@ -147,7 +147,7 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>> struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>>>> struct exynos_tmu_platform_data *pdata = data->pdata;
>>>> const struct exynos_tmu_registers *reg = pdata->registers;
>>>> - unsigned int status, trim_info = 0, con;
>>>> + unsigned int status, trim_info = 0, con, ctrl;
>>>> unsigned int rising_threshold = 0, falling_threshold = 0;
>>>> int ret = 0, threshold_code, i, trigger_levs = 0;
>>>>
>>>> @@ -164,8 +164,25 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>>>> }
>>>> }
>>>>
>>>> - if (TMU_SUPPORTS(pdata, TRIM_RELOAD))
>>>> - __raw_writel(1, data->base + reg->triminfo_ctrl);
>>>> + if (TMU_SUPPORTS(pdata, TRIM_RELOAD)) {
>>>> + if (reg->triminfo_ctrl_count > MAX_TRIMINFO_CTRL_REG) {
>>>
>>> Please remove this check and MAX_TRIMINFO_CTRL_REG define.
>>>
>>> We do not want such runtime checks for development time errors.
>>
>> OK, I'll remove it.
>>
>>>
>>>> + ret = -EINVAL;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + for (i = 0; i < reg->triminfo_ctrl_count; i++) {
>>>> + if (pdata->triminfo_reload[i]) {
>>>> + ctrl = readl(data->base +
>>>> + reg->triminfo_ctrl[i]);
>>>> + ctrl &= ~(reg->triminfo_reload_mask <<
>>>> + reg->triminfo_reload_shift);
>>>> + ctrl |= pdata->triminfo_reload[i] <<
>>>> + reg->triminfo_reload_shift;
>>>
>>> triminfo_reload_shift and triminfo_reload_mask variables have always
>>> the same values when this code is run so there is no need for them.
>>
>> I don't understand. Do you mean that timinfo_reload_{shift/mask} variable is un-needed?
>
> Yes.
>
>> If you possible, I need more detailed comment.
>
> Currently triminfo_reload_shift is always "0" and triminfo_reload_mask
> is "1" so there is no need to add an abstraction for different SoCs
> (it should be added only when there is a real need for it).
>
> Please just rewrite this code as:
>
> if (pdata->triminfo_reload[i]) {
> ctrl = readl(data->base +
> reg->triminfo_ctrl[i]);
> ctrl |= pdata->triminfo_reload[i];
> __raw_writel(ctrl, data->base +
> reg->triminfo_ctrl[i]);
> }
>
> Then you can remove unused triminfo_reload_shift and
> EXYNOS_TRIMINFO_RELOAD_SHIFT.
>
> Please also include my patch (https://lkml.org/lkml/2014/8/20/481) in
> your patch series (or at least mark it in the cover letter that my
> patch should be merged before your patch #2/4).
OK. thanks for your comment.
Best Regards,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
