Re: [PATCH 2/6] thermal: exynos: Provide separate TMU data for Exynos4412
Hi Eduardo,
> Lukasz,
>
> Just minor comments.
>
> On 24-09-2013 04:08, Lukasz Majewski wrote:
> > Up till now Exynos5250 and Exynos4412 had the same definitions for
> > TMU data. Following commit changes that, by introducing separate
> > exynos4412_default_tmu_data structure.
> >
> > Since Exynos4412 was chronologically first, the corresponding name
> > for TMU registers and default data was renamed.
> >
> > Additionally, new SOC_ARCH_EXYNOS4412 type has been defined.
>
> It is not clear the objective of the patch itself. Was it just to make
> the code more readable or are you fixing something?
I'm fixing thermal on EXYNOS4412 device (trats2). The problem here is
to do it properly, since on the current code EXYNOS4412 and EXYNOS5250
uses the same configuration data.
To fix this problem, I had to split them. Also chronologically the
THERMAL IP first appeared on EXYNOS4412, not EXYNOS5250.
>
> >
> > Signed-off-by: Lukasz Majewski
> > Reviewed-by: Bartlomiej Zolnierkiewicz
> > Reviewed-by: Tomasz Figa
> > ---
> > drivers/thermal/samsung/exynos_tmu.c |7 ---
> > drivers/thermal/samsung/exynos_tmu.h |1 +
> > drivers/thermal/samsung/exynos_tmu_data.c | 26
> > --
> > drivers/thermal/samsung/exynos_tmu_data.h |9 - 4 files
> > changed, 33 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/thermal/samsung/exynos_tmu.c
> > b/drivers/thermal/samsung/exynos_tmu.c index b43afda..a858cc4 100644
> > --- a/drivers/thermal/samsung/exynos_tmu.c
> > +++ b/drivers/thermal/samsung/exynos_tmu.c
> > @@ -488,7 +488,7 @@ static const struct of_device_id
> > exynos_tmu_match[] = { },
> > {
> > .compatible = "samsung,exynos4412-tmu",
> > - .data = (void *)EXYNOS5250_TMU_DRV_DATA,
> > + .data = (void *)EXYNOS4412_TMU_DRV_DATA,
> > },
> > {
> > .compatible = "samsung,exynos5250-tmu",
> > @@ -630,8 +630,9 @@ static int exynos_tmu_probe(struct
> > platform_device *pdev) return ret;
> >
> > if (pdata->type == SOC_ARCH_EXYNOS ||
> > - pdata->type == SOC_ARCH_EXYNOS4210 ||
> > - pdata->type == SOC_ARCH_EXYNOS5440)
> > + pdata->type == SOC_ARCH_EXYNOS4210 ||
> > + pdata->type == SOC_ARCH_EXYNOS4412 ||
> > + pdata->type == SOC_ARCH_EXYNOS5440)
> > data->soc = pdata->type;
> > else {
> > ret = -EINVAL;
> > diff --git a/drivers/thermal/samsung/exynos_tmu.h
> > b/drivers/thermal/samsung/exynos_tmu.h index b364c9e..db37df0 100644
> > --- a/drivers/thermal/samsung/exynos_tmu.h
> > +++ b/drivers/thermal/samsung/exynos_tmu.h
> > @@ -41,6 +41,7 @@ enum calibration_mode {
> >
> > enum soc_type {
> > SOC_ARCH_EXYNOS4210 = 1,
> > + SOC_ARCH_EXYNOS4412,
> > SOC_ARCH_EXYNOS,
> > SOC_ARCH_EXYNOS5440,
> > };
>
> I believe the above enum needs some documentation. SOC_ARCH_EXYNOS
> seams to be a bit confusing.
This is fixed at patch 4/6 of the series.
>
> > diff --git a/drivers/thermal/samsung/exynos_tmu_data.c
> > b/drivers/thermal/samsung/exynos_tmu_data.c index 9002499..bd08093
> > 100644 --- a/drivers/thermal/samsung/exynos_tmu_data.c
> > +++ b/drivers/thermal/samsung/exynos_tmu_data.c
> > @@ -90,8 +90,8 @@ struct exynos_tmu_init_data const
> > exynos4210_default_tmu_data = { };
> > #endif
> >
> > -#if defined(CONFIG_SOC_EXYNOS5250) ||
> > defined(CONFIG_SOC_EXYNOS4412) -static const struct
> > exynos_tmu_registers exynos5250_tmu_registers = { +#if
> > defined(CONFIG_SOC_EXYNOS4412) || defined(CONFIG_SOC_EXYNOS5250)
> > +static const struct exynos_tmu_registers exynos4412_tmu_registers
> > = { .triminfo_data = EXYNOS_TMU_REG_TRIMINFO, .triminfo_25_shift =
> > EXYNOS_TRIMINFO_25_SHIFT, .triminfo_85_shift =
> > EXYNOS_TRIMINFO_85_SHIFT, @@ -128,7 +128,7 @@ static const struct
> > exynos_tmu_registers exynos5250_tmu_registers = { .emul_time_mask =
> > EXYNOS_EMUL_TIME_MASK, };
> >
> > -#define EXYNOS5250_TMU_DATA \
> > +#define EXYNOS4412_TMU_DATA \
> > .threshold_falling = 10, \
> > .trigger_levels[0] = 85, \
> > .trigger_levels[1] = 103, \
> > @@ -162,15 +162,29 @@ static const struct exynos_tmu_registers
> > exynos5250_tmu_registers = { .temp_level = 103, \
> > }, \
> > .freq_tab_count = 2, \
> > - .type = SOC_ARCH_EXYNOS, \
> > - .registers = &exynos5250_tmu_registers, \
> > + .registers = &exynos4412_tmu_registers, \
> > .features = (TMU_SUPPORT_EMULATION |
> > TMU_SUPPORT_TRIM_RELOAD | \ TMU_SUPPORT_FALLING_TRIP |
> > TMU_SUPPORT_READY_STATUS | \ TMU_SUPPORT_EMUL_TIME)
> > +#endif
> >
> > +#if defined(CONFIG_SOC_EXYNOS4412)
> > +struct exynos_tmu_init_data const exynos4412_default_tmu_data = {
> > + .tmu_data = {
> > + { EXYNOS4412_TMU_DATA,
> > + .type = SOC_ARCH_EXYNOS4412,
> > + },
> > + },
> > + .tmu_count = 1,
> > +};
> > +#endif
> > +
> > +#if defined(CONFIG_SOC_EXYNOS5250)
> > struct exynos_tmu_init_data const exynos5250_def
Re: [PATCH 2/6] thermal: exynos: Provide separate TMU data for Exynos4412
Lukasz,
Just minor comments.
On 24-09-2013 04:08, Lukasz Majewski wrote:
> Up till now Exynos5250 and Exynos4412 had the same definitions for TMU
> data. Following commit changes that, by introducing separate
> exynos4412_default_tmu_data structure.
>
> Since Exynos4412 was chronologically first, the corresponding name for
> TMU registers and default data was renamed.
>
> Additionally, new SOC_ARCH_EXYNOS4412 type has been defined.
It is not clear the objective of the patch itself. Was it just to make
the code more readable or are you fixing something?
>
> Signed-off-by: Lukasz Majewski
> Reviewed-by: Bartlomiej Zolnierkiewicz
> Reviewed-by: Tomasz Figa
> ---
> drivers/thermal/samsung/exynos_tmu.c |7 ---
> drivers/thermal/samsung/exynos_tmu.h |1 +
> drivers/thermal/samsung/exynos_tmu_data.c | 26 --
> drivers/thermal/samsung/exynos_tmu_data.h |9 -
> 4 files changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c
> b/drivers/thermal/samsung/exynos_tmu.c
> index b43afda..a858cc4 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -488,7 +488,7 @@ static const struct of_device_id exynos_tmu_match[] = {
> },
> {
> .compatible = "samsung,exynos4412-tmu",
> - .data = (void *)EXYNOS5250_TMU_DRV_DATA,
> + .data = (void *)EXYNOS4412_TMU_DRV_DATA,
> },
> {
> .compatible = "samsung,exynos5250-tmu",
> @@ -630,8 +630,9 @@ static int exynos_tmu_probe(struct platform_device *pdev)
> return ret;
>
> if (pdata->type == SOC_ARCH_EXYNOS ||
> - pdata->type == SOC_ARCH_EXYNOS4210 ||
> - pdata->type == SOC_ARCH_EXYNOS5440)
> + pdata->type == SOC_ARCH_EXYNOS4210 ||
> + pdata->type == SOC_ARCH_EXYNOS4412 ||
> + pdata->type == SOC_ARCH_EXYNOS5440)
> data->soc = pdata->type;
> else {
> ret = -EINVAL;
> diff --git a/drivers/thermal/samsung/exynos_tmu.h
> b/drivers/thermal/samsung/exynos_tmu.h
> index b364c9e..db37df0 100644
> --- a/drivers/thermal/samsung/exynos_tmu.h
> +++ b/drivers/thermal/samsung/exynos_tmu.h
> @@ -41,6 +41,7 @@ enum calibration_mode {
>
> enum soc_type {
> SOC_ARCH_EXYNOS4210 = 1,
> + SOC_ARCH_EXYNOS4412,
> SOC_ARCH_EXYNOS,
> SOC_ARCH_EXYNOS5440,
> };
I believe the above enum needs some documentation. SOC_ARCH_EXYNOS seams
to be a bit confusing.
> diff --git a/drivers/thermal/samsung/exynos_tmu_data.c
> b/drivers/thermal/samsung/exynos_tmu_data.c
> index 9002499..bd08093 100644
> --- a/drivers/thermal/samsung/exynos_tmu_data.c
> +++ b/drivers/thermal/samsung/exynos_tmu_data.c
> @@ -90,8 +90,8 @@ struct exynos_tmu_init_data const
> exynos4210_default_tmu_data = {
> };
> #endif
>
> -#if defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412)
> -static const struct exynos_tmu_registers exynos5250_tmu_registers = {
> +#if defined(CONFIG_SOC_EXYNOS4412) || defined(CONFIG_SOC_EXYNOS5250)
> +static const struct exynos_tmu_registers exynos4412_tmu_registers = {
> .triminfo_data = EXYNOS_TMU_REG_TRIMINFO,
> .triminfo_25_shift = EXYNOS_TRIMINFO_25_SHIFT,
> .triminfo_85_shift = EXYNOS_TRIMINFO_85_SHIFT,
> @@ -128,7 +128,7 @@ static const struct exynos_tmu_registers
> exynos5250_tmu_registers = {
> .emul_time_mask = EXYNOS_EMUL_TIME_MASK,
> };
>
> -#define EXYNOS5250_TMU_DATA \
> +#define EXYNOS4412_TMU_DATA \
> .threshold_falling = 10, \
> .trigger_levels[0] = 85, \
> .trigger_levels[1] = 103, \
> @@ -162,15 +162,29 @@ static const struct exynos_tmu_registers
> exynos5250_tmu_registers = {
> .temp_level = 103, \
> }, \
> .freq_tab_count = 2, \
> - .type = SOC_ARCH_EXYNOS, \
> - .registers = &exynos5250_tmu_registers, \
> + .registers = &exynos4412_tmu_registers, \
> .features = (TMU_SUPPORT_EMULATION | TMU_SUPPORT_TRIM_RELOAD | \
> TMU_SUPPORT_FALLING_TRIP | TMU_SUPPORT_READY_STATUS | \
> TMU_SUPPORT_EMUL_TIME)
> +#endif
>
> +#if defined(CONFIG_SOC_EXYNOS4412)
> +struct exynos_tmu_init_data const exynos4412_default_tmu_data = {
> + .tmu_data = {
> + { EXYNOS4412_TMU_DATA,
> + .type = SOC_ARCH_EXYNOS4412,
> + },
> + },
> + .tmu_count = 1,
> +};
> +#endif
> +
> +#if defined(CONFIG_SOC_EXYNOS5250)
> struct exynos_tmu_init_data const exynos5250_default_tmu_data = {
> .tmu_data = {
> - { EXYNOS5250_TMU_DATA },
> + { EXYNOS4412_TMU_DATA,
> + .type = SOC_ARCH_EXYNOS,
> + },
Please add a comment on this initialization, specially because we start
to mix macros with inline initialization. Starts to be hard to follow.
Besides, I believe
+ {
+ EXYNOS4412_TMU_DATA
Re: [PATCH 2/6] thermal: exynos: Provide separate TMU data for Exynos4412
On Tue, Sep 24, 2013 at 1:38 PM, Lukasz Majewski wrote:
> Up till now Exynos5250 and Exynos4412 had the same definitions for TMU
> data. Following commit changes that, by introducing separate
> exynos4412_default_tmu_data structure.
>
> Since Exynos4412 was chronologically first, the corresponding name for
> TMU registers and default data was renamed.
>
> Additionally, new SOC_ARCH_EXYNOS4412 type has been defined.
>
> Signed-off-by: Lukasz Majewski
> Reviewed-by: Bartlomiej Zolnierkiewicz
> Reviewed-by: Tomasz Figa
Reviewed-by: Amit Daniel Kachhap
Thanks,
Amit Daniel
> ---
> drivers/thermal/samsung/exynos_tmu.c |7 ---
> drivers/thermal/samsung/exynos_tmu.h |1 +
> drivers/thermal/samsung/exynos_tmu_data.c | 26 --
> drivers/thermal/samsung/exynos_tmu_data.h |9 -
> 4 files changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/thermal/samsung/exynos_tmu.c
> b/drivers/thermal/samsung/exynos_tmu.c
> index b43afda..a858cc4 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -488,7 +488,7 @@ static const struct of_device_id exynos_tmu_match[] = {
> },
> {
> .compatible = "samsung,exynos4412-tmu",
> - .data = (void *)EXYNOS5250_TMU_DRV_DATA,
> + .data = (void *)EXYNOS4412_TMU_DRV_DATA,
> },
> {
> .compatible = "samsung,exynos5250-tmu",
> @@ -630,8 +630,9 @@ static int exynos_tmu_probe(struct platform_device *pdev)
> return ret;
>
> if (pdata->type == SOC_ARCH_EXYNOS ||
> - pdata->type == SOC_ARCH_EXYNOS4210 ||
> - pdata->type == SOC_ARCH_EXYNOS5440)
> + pdata->type == SOC_ARCH_EXYNOS4210 ||
> + pdata->type == SOC_ARCH_EXYNOS4412 ||
> + pdata->type == SOC_ARCH_EXYNOS5440)
> data->soc = pdata->type;
> else {
> ret = -EINVAL;
> diff --git a/drivers/thermal/samsung/exynos_tmu.h
> b/drivers/thermal/samsung/exynos_tmu.h
> index b364c9e..db37df0 100644
> --- a/drivers/thermal/samsung/exynos_tmu.h
> +++ b/drivers/thermal/samsung/exynos_tmu.h
> @@ -41,6 +41,7 @@ enum calibration_mode {
>
> enum soc_type {
> SOC_ARCH_EXYNOS4210 = 1,
> + SOC_ARCH_EXYNOS4412,
> SOC_ARCH_EXYNOS,
> SOC_ARCH_EXYNOS5440,
> };
> diff --git a/drivers/thermal/samsung/exynos_tmu_data.c
> b/drivers/thermal/samsung/exynos_tmu_data.c
> index 9002499..bd08093 100644
> --- a/drivers/thermal/samsung/exynos_tmu_data.c
> +++ b/drivers/thermal/samsung/exynos_tmu_data.c
> @@ -90,8 +90,8 @@ struct exynos_tmu_init_data const
> exynos4210_default_tmu_data = {
> };
> #endif
>
> -#if defined(CONFIG_SOC_EXYNOS5250) || defined(CONFIG_SOC_EXYNOS4412)
> -static const struct exynos_tmu_registers exynos5250_tmu_registers = {
> +#if defined(CONFIG_SOC_EXYNOS4412) || defined(CONFIG_SOC_EXYNOS5250)
> +static const struct exynos_tmu_registers exynos4412_tmu_registers = {
> .triminfo_data = EXYNOS_TMU_REG_TRIMINFO,
> .triminfo_25_shift = EXYNOS_TRIMINFO_25_SHIFT,
> .triminfo_85_shift = EXYNOS_TRIMINFO_85_SHIFT,
> @@ -128,7 +128,7 @@ static const struct exynos_tmu_registers
> exynos5250_tmu_registers = {
> .emul_time_mask = EXYNOS_EMUL_TIME_MASK,
> };
>
> -#define EXYNOS5250_TMU_DATA \
> +#define EXYNOS4412_TMU_DATA \
> .threshold_falling = 10, \
> .trigger_levels[0] = 85, \
> .trigger_levels[1] = 103, \
> @@ -162,15 +162,29 @@ static const struct exynos_tmu_registers
> exynos5250_tmu_registers = {
> .temp_level = 103, \
> }, \
> .freq_tab_count = 2, \
> - .type = SOC_ARCH_EXYNOS, \
> - .registers = &exynos5250_tmu_registers, \
> + .registers = &exynos4412_tmu_registers, \
> .features = (TMU_SUPPORT_EMULATION | TMU_SUPPORT_TRIM_RELOAD | \
> TMU_SUPPORT_FALLING_TRIP | TMU_SUPPORT_READY_STATUS |
> \
> TMU_SUPPORT_EMUL_TIME)
> +#endif
>
> +#if defined(CONFIG_SOC_EXYNOS4412)
> +struct exynos_tmu_init_data const exynos4412_default_tmu_data = {
> + .tmu_data = {
> + { EXYNOS4412_TMU_DATA,
> + .type = SOC_ARCH_EXYNOS4412,
> + },
> + },
> + .tmu_count = 1,
> +};
> +#endif
> +
> +#if defined(CONFIG_SOC_EXYNOS5250)
> struct exynos_tmu_init_data const exynos5250_default_tmu_data = {
> .tmu_data = {
> - { EXYNOS5250_TMU_DATA },
> + { EXYNOS4412_TMU_DATA,
> + .type = SOC_ARCH_EXYNOS,
> + },
> },
> .tmu_count = 1,
> };
> diff --git a/drivers/thermal/samsung/exynos_tmu_data.h
> b/drivers/thermal/samsung/exynos_tmu_data.h
> index dc7feb5..b130b1e 100644
> --- a/drivers/thermal/samsung/exynos_tmu_data.h
> +++ b/drivers/thermal/samsung/exynos_tmu_data.h
> @@ -138,7 +138,14 @@ extern struct exynos_
