Hi
some comments:
On Tue, 4 Oct 2011, Vishwanath BS wrote:
> From: Rajendra Nayak <rnayak@xxxxxx>
>
> patch adds IO Daisychain support for OMAP4 as per section 3.9.4 in OMAP4430
> Public TRM.
>
> Signed-off-by: Rajendra Nayak <rnayak@xxxxxx>
> Signed-off-by: Vishwanath BS <vishwanath.bs@xxxxxx>
> ---
> arch/arm/mach-omap2/pm.h | 1 +
> arch/arm/mach-omap2/pm44xx.c | 36 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 37 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index 9a36a7c..2e09d72 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -22,6 +22,7 @@ extern int omap3_can_sleep(void);
> extern int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state);
> extern int omap3_idle_init(void);
> extern void omap3_enable_io_chain(void);
> +extern void omap4_trigger_wuclk_ctrl(void);
>
> #if defined(CONFIG_PM_OPP)
> extern int omap3_opp_init(void);
> diff --git a/arch/arm/mach-omap2/pm44xx.c b/arch/arm/mach-omap2/pm44xx.c
> index 59a870b..aa7cff4 100644
> --- a/arch/arm/mach-omap2/pm44xx.c
> +++ b/arch/arm/mach-omap2/pm44xx.c
> @@ -16,8 +16,17 @@
> #include <linux/err.h>
> #include <linux/slab.h>
>
> +#include <plat/common.h>
> +
> #include "powerdomain.h"
> #include <mach/omap4-common.h>
> +#include "pm.h"
> +#include "cm-regbits-44xx.h"
> +#include "cminst44xx.h"
> +#include "prm-regbits-44xx.h"
> +#include "prcm44xx.h"
> +#include "prm44xx.h"
> +#include "prminst44xx.h"
>
> struct power_state {
> struct powerdomain *pwrdm;
> @@ -30,6 +39,33 @@ struct power_state {
>
> static LIST_HEAD(pwrst_list);
>
> +#define MAX_IOPAD_LATCH_TIME 1000
This macro is missing a comment, which should precede it, describing what
this value is.
> +
> +void omap4_trigger_wuclk_ctrl(void)
> +{
> + int i = 0;
> +
> + /* Enable GLOBAL_WUEN */
> + if (!omap4_cminst_read_inst_reg_bits(OMAP4430_PRM_PARTITION,
> + OMAP4430_PRM_DEVICE_INST, OMAP4_PRM_IO_PMCTRL_OFFSET,
> + OMAP4430_GLOBAL_WUEN_MASK))
The above line doesn't look right. It's accessing a PRM instance register
with omap4_cminst_*()? Shouldn't that be omap4_prminst_*()?
> + omap4_prminst_rmw_inst_reg_bits(OMAP4430_GLOBAL_WUEN_MASK,
> + OMAP4430_GLOBAL_WUEN_MASK, OMAP4430_PRM_PARTITION,
> + OMAP4430_PRM_DEVICE_INST, OMAP4_PRM_IO_PMCTRL_OFFSET);
> +
> + /* Trigger WUCLKIN enable */
> + omap4_prminst_rmw_inst_reg_bits(OMAP4430_WUCLK_CTRL_MASK, OMAP4430_WUCLK_CTRL_MASK,
> + OMAP4430_PRM_PARTITION, OMAP4430_PRM_DEVICE_INST, OMAP4_PRM_IO_PMCTRL_OFFSET);
> + omap_test_timeout(
> + ((omap4_prminst_read_inst_reg(OMAP4430_PRM_PARTITION, OMAP4430_PRM_DEVICE_INST, OMAP4_PRM_IO_PMCTRL_OFFSET)
> + >> OMAP4430_WUCLK_STATUS_SHIFT) == 1),
> + MAX_IOPAD_LATCH_TIME, i);
> + /* Trigger WUCLKIN disable */
> + omap4_prminst_rmw_inst_reg_bits(OMAP4430_WUCLK_CTRL_MASK, 0x0,
> + OMAP4430_PRM_PARTITION, OMAP4430_PRM_DEVICE_INST, OMAP4_PRM_IO_PMCTRL_OFFSET);
> + return;
> +}
This function belongs in mach-omap2/prminst44xx.c. I still think it would
be good if we moved all PRM/CM direct register accesses into prm*.c or
cm*.c files in mach-omap2/. Then none of those prm*.h includes would be
needed in pm44xx.c either.
> +
> #ifdef CONFIG_SUSPEND
> static int omap4_pm_suspend(void)
> {
> --
> 1.7.0.4
>
> --
> 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
>
- Paul
--
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
