Hi Tero, On 04/25/2012 02:33 AM, Tero Kristo wrote: > 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. Thanks. Good question. If we are not seeing failures often, and I would hope not, probably ok to remove. However, I will let Tony comment here ... Cheers Jon -- 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