>>-----Original Message----- >>From: Kevin Hilman [mailto:khil...@deeprootsystems.com] >>Sent: Thursday, December 10, 2009 4:39 AM >>To: Gopinath, Thara >>Cc: linux-omap@vger.kernel.org; Paul Walmsley >>Subject: Re: [PATCH] OMAP3: hwmod: check for clkdomain pointer before >>accesing it to change the sleep >>dependencies. >> >>Thara Gopinath <th...@ti.com> writes: >> >>> Some clock nodes like wdt1_fck, sr1_fck, sr2_fck etc do not have a >>> clock domain associated . For such nodes accessing the clock domain pointers >>> in _add_initiator_dep and _del_initiator_dep, will lead to null pointer >>> defreferencing crash. Adding support in these functions to check for >>> existence of clkdm pointer before trying to acess it. Even if tomorrow >>> we correct all the clock nodes to have an associated clock domain, checking >>> for the existence of the pointer is a good programming practice. >>> >>> Signed-off-by: Thara Gopinath <th...@ti.com> >>> Cc: Paul Walmsley <p...@pwsan.com> >>> --- >>> This patch depends on http://patchwork.kernel.org/patch/63383/ >> >>I think clocks without associated clockdomains should be handled with >>a flag instead of a check for a NULL pointer. >> >>Your current approach will silently fail if someone forgets to add >>a clockdomain to a clock that should have one.
Are you suggesting adding a flag to the clock node in case it does not have an associated clockdomain? I am using your tree origin/pm-wip/omap_device branch. On this I see a commit (e89a95db7095d998991c564a756a33ee0ec722c5) in which a warning is printed in omap2_init_clk_clkdm if there is no clockdomain associated with a clock node. My idea was this warning plus the check so as to avoid the system crash would suffice. >> >>Kevin >> >> >>> arch/arm/mach-omap2/omap_hwmod.c | 8 ++++++-- >>> 1 files changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c >>> b/arch/arm/mach-omap2/omap_hwmod.c >>> index 18e6478..3edc387 100644 >>> --- a/arch/arm/mach-omap2/omap_hwmod.c >>> +++ b/arch/arm/mach-omap2/omap_hwmod.c >>> @@ -314,8 +314,10 @@ static int _add_initiator_dep(struct omap_hwmod *oh, >>> struct omap_hwmod >>*init_oh) >>> if (!oh->_clk) >>> return -EINVAL; >>> >>> - return pwrdm_add_sleepdep(oh->_clk->clkdm->pwrdm.ptr, >>> + if (oh->_clk->clkdm) >>> + return pwrdm_add_sleepdep(oh->_clk->clkdm->pwrdm.ptr, >>> init_oh->_clk->clkdm->pwrdm.ptr); >>> + return 0; >>> } >>> >>> /** >>> @@ -335,8 +337,10 @@ static int _del_initiator_dep(struct omap_hwmod *oh, >>> struct omap_hwmod >>*init_oh) >>> if (!oh->_clk) >>> return -EINVAL; >>> >>> - return pwrdm_del_sleepdep(oh->_clk->clkdm->pwrdm.ptr, >>> + if (oh->_clk->clkdm) >>> + return pwrdm_del_sleepdep(oh->_clk->clkdm->pwrdm.ptr, >>> init_oh->_clk->clkdm->pwrdm.ptr); >>> + return 0; >>> } >>> >>> /** >>> -- >>> 1.5.4.7 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in >>> the body of a message to majord...@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
