On Mon, 2012-04-23 at 11:09 -0500, Jon Hunter wrote:
> Hi Tero,
>
> On 04/20/2012 04:33 AM, Tero Kristo wrote:
>
> [...]
>
> > +/**
> > + * omap4_dpll_print_reg - dump out a single DPLL register value
> > + * @dpll_reg: register to dump
> > + * @name: name of the register
> > + * @tuple: content of the register
> > + *
> > + * Helper dump function to print out a DPLL register value in case
> > + * of restore failures.
> > + */
> > +static void omap4_dpll_print_reg(struct omap4_dpll_regs *dpll_reg, char *name,
> > + struct dpll_reg *tuple)
> > +{
> > + if (tuple->offset)
> > + pr_warn("%s - Address offset = 0x%08x, value=0x%08x\n", name,
> > + tuple->offset, tuple->val);
>
> Minor nit-pick here ...
>
> 1. Offset is 16-bits and so we can just have "offset = 0x%04x"
> 2. Consider dropping "Address" from "Address offset"
> 3. Can we be consistent in our spaces for "offset = " and "value="?
>
> > +}
> > +
> > +/*
> > + * omap4_dpll_dump_regs - dump out DPLL registers
> > + * @dpll_reg: DPLL to dump
> > + *
> > + * Dump out the contents of the registers for a DPLL. Called if a
> > + * restore for DPLL fails to lock.
> > + */
> > +static void omap4_dpll_dump_regs(struct omap4_dpll_regs *dpll_reg)
> > +{
> > + pr_warn("%s: Unable to lock dpll %s[part=%x inst=%x]:\n",
> > + __func__, dpll_reg->name, dpll_reg->mod_partition,
> > + dpll_reg->mod_inst);
> > + omap4_dpll_print_reg(dpll_reg, "clksel", &dpll_reg->clksel);
> > + omap4_dpll_print_reg(dpll_reg, "div_m2", &dpll_reg->div_m2);
> > + omap4_dpll_print_reg(dpll_reg, "div_m3", &dpll_reg->div_m3);
> > + omap4_dpll_print_reg(dpll_reg, "div_m4", &dpll_reg->div_m4);
> > + omap4_dpll_print_reg(dpll_reg, "div_m5", &dpll_reg->div_m5);
> > + omap4_dpll_print_reg(dpll_reg, "div_m6", &dpll_reg->div_m6);
> > + omap4_dpll_print_reg(dpll_reg, "div_m7", &dpll_reg->div_m7);
> > + omap4_dpll_print_reg(dpll_reg, "clkdcoldo", &dpll_reg->clkdcoldo);
> > + omap4_dpll_print_reg(dpll_reg, "clkmode", &dpll_reg->clkmode);
> > + omap4_dpll_print_reg(dpll_reg, "autoidle", &dpll_reg->autoidle);
> > + if (dpll_reg->idlest.offset)
> > + pr_warn("idlest - Address offset = 0x%08x, before val=0x%08x"
> > + " after = 0x%08x\n", dpll_reg->idlest.offset,
> > + dpll_reg->idlest.val,
> > + omap4_dpll_read_reg(dpll_reg, &dpll_reg->idlest));
>
> Nit-pick ...
>
> 1. Same comments as above
> 2. Consider dropping "Address offset" altogether here as we print
> "idlest" the offset should be implied.
> 3. Also can we be consistent in our naming of "before val and "after"?
> For example, "val before =" and "val now = ".
Okay, I'll modify both prints slightly. Question though, do we want
these prints in the kernel in the first place? At least Tony has been
frowning upon register dumps in the kernel and this falls into that
category. What we could do is just to print the warning but drop the
register dumps out.
-Tero
--
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
