shweta gulati <shweta.gulati@xxxxxx> writes:
> From: Shweta Gulati <shweta.gulati@xxxxxx>
>
> This version of patch incorporates review comments which includes
> shifting the code change in specific function 'omap3_iva_idle' and
> removing iva_pwrdm from pwrst_list rather than checking all the pwrdms
> in list to exclude iva_pwrdm.
Comments about the version history of a patch don't belong here. They
belong after the '---' so they are not picked up in the git history.
> The PM code should not set latency on IVA power state based on
> the flag 'enable_off_mode'.
Why? A changelog is about answering *both* 'what' and 'why'
Also the SRF change should be a separate patch since SRF is PM branch
only. The primary change here should be targetted for mainline.
> This is taken care of in this version.
>
> Signed-off-by: Sripathy Vishwanath <vishwanath.bs@xxxxxx>
> Signed-off-by: Shweta Gulati <shweta.gulati@xxxxxx>
> ---
I *strongly* recommend using git-format-patch, then manually editing
this part to add patch version history.
> Index: linux-omap-pm/arch/arm/mach-omap2/pm34xx.c
> ===================================================================
> --- linux-omap-pm.orig/arch/arm/mach-omap2/pm34xx.c
> +++ linux-omap-pm/arch/arm/mach-omap2/pm34xx.c
> @@ -786,6 +786,12 @@ static void __init omap3_iva_idle(void)
> OMAP3430_RST2_IVA2 |
> OMAP3430_RST3_IVA2,
> OMAP3430_IVA2_MOD, OMAP2_RM_RSTCTRL);
> + /* Put the IVA2 In Idle */
> + prm_rmw_mod_reg_bits(OMAP3430_LASTPOWERSTATEENTERED_MASK, 0,
> + OMAP3430_IVA2_MOD, OMAP2_PM_PWSTCTRL);
huh?
this is confusing for multiple reasons
1. The comment is wrong. You're setting the nex powerstate to OFF.
2. LASTPOWERSTATEENTERED is a field in PM_PREPWSTST, not in PM_PWSTCTRL,
so the field your changing is the POWERSTATE field of PM_PWRSTCTRL.
3. setting this state is already done in pwrdms_setup()
> + /* Make Clock transition Automatic*/
nit: need a space before the '*/'
> + cm_rmw_mod_reg_bits(OMAP3430_CLKTRCTRL_IVA2_MASK, 0x3,
> + OMAP3430_IVA2_MOD, OMAP2_CM_CLKSTCTRL);
> }
>
> static void __init omap3_d2d_idle(void)
> @@ -1074,8 +1080,11 @@ static int __init pwrdms_setup(struct po
> if (!pwrst)
> return -ENOMEM;
> pwrst->pwrdm = pwrdm;
> - pwrst->next_state = PWRDM_POWER_RET;
> - list_add(&pwrst->node, &pwrst_list);
> + if (strcmp("iva2_pwrdm", pwrdm->name)) {
> + pwrst->next_state = PWRDM_POWER_RET;
> + list_add(&pwrst->node, &pwrst_list);
> + } else
> + pwrst->next_state = PWRDM_POWER_OFF;
See Documentation/CodingStyle about use of {}. If one part
of an 'if' has {}s, the other(s) should as well.
> if (pwrdm_has_hdwr_sar(pwrdm))
> pwrdm_enable_hdwr_sar(pwrdm);
> Index: linux-omap-pm/arch/arm/mach-omap2/resource34xx.c
> ===================================================================
> --- linux-omap-pm.orig/arch/arm/mach-omap2/resource34xx.c
> +++ linux-omap-pm/arch/arm/mach-omap2/resource34xx.c
> @@ -140,7 +140,8 @@ int set_pd_latency(struct shared_resourc
> }
>
> if (!enable_off_mode && pd_lat_level == PD_LATENCY_OFF)
> - pd_lat_level = PD_LATENCY_RET;
> + if (strcmp("iva2_pwrdm", pwrdm->name))
> + pd_lat_level = PD_LATENCY_RET;
>
> resp->curr_level = pd_lat_level;
> set_pwrdm_state(pwrdm, pd_lat_level);
This change should be a separate patch including a changelog that
answers "why?"
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
