On Mon, Sep 22, 2014 at 03:55:14PM +0800, Xiubo Li-B47053 wrote:
> > > +static void __init ls1021a_clocks_init(struct device_node *np)
> > > +{
> > > + void __iomem *dcfg_base;
> > > +
> > > +#define DCFG_CCSR_DEVDISR1 (dcfg_base + 0x70)
> > > +#define DCFG_CCSR_DEVDISR2 (dcfg_base + 0x74)
> > > +#define DCFG_CCSR_DEVDISR3 (dcfg_base + 0x78)
> > > +#define DCFG_CCSR_DEVDISR4 (dcfg_base + 0x7c)
> > > +#define DCFG_CCSR_DEVDISR5 (dcfg_base + 0x80)
> > > +
> > > + dcfg_base = of_iomap(np, 0);
> > > +
> > > + BUG_ON(!dcfg_base);
> > Is this worth a BUG?
> Yes, I do think so.
>
> There is one story about my platform:
> We are using the imx2_wdt watchdog device, which cannot stop or suspend
> once it has started.
> And our platform will also support the Power Management, if the gating
> Clock initialized failed, so when the system enters sleep mode, we cannot
> Stop the imx2_wdt IP block, so it will reset the board after 180 seconds at
> most.
>
> So using this gating clock, the watchdog IP block's clock could be disabled
> When the system enter sleep mode.
>
> So I think BUG_ON here is make sense.
>
> Doesn't it ?
>
> > Or is it enough to do
> > if (!dcfg_base) {
> > pr_warn("failed to map fsl,ls1021a-gate device\n");
> > return
> > }
> >
> > > +
> > > + clk[LS1021A_CLK_PBL_EN] = ls1021a_clk_gate("pbl_en", "dummy",
> > > + DCFG_CCSR_DEVDISR1, 0, true);
> > If the machine's device tree contains two (or more) nodes that are
> > compatible to "fsl,ls1021a-gate", you overwrite your static clk array. Is
> > it worth to add another check here?:
> >
> On LS1021A SoC, I can make sure that there will only be one gate node. But for
> More compatibly using one dynamic clk array will be better.
I do not think it's really necessary to use dynamic allocation. Making
dcfg_base a static variable and check if it's null at the beginning of
the function is probably enough.
Shawn
--
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
