On Thu, Jan 13, 2011 at 12:33 PM, Premi, Sanjeev <premi@xxxxxx> wrote: >> -----Original Message----- >> From: G, Manjunath Kondaiah [mailto:manjugk@xxxxxx] >> Sent: Thursday, January 13, 2011 9:24 AM >> To: Premi, Sanjeev >> Cc: linux-omap@xxxxxxxxxxxxxxx >> Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part >> >> Hi Sanjeev, >> > [snip]...[snip] > >> > */ >> > + >> > + /* >> > + * Does it support 720MHz? >> > + */ >> multiline style comment > [sp] This is used many places. It will be good if we fix that also. >Don't see any issue with it. > unless you really want to see this on single line. It is mentioned in http://www.kernel.org/doc/Documentation/CodingStyle > >> > + status = (OMAP3_SKUID_MASK & read_tap_reg(OMAP3_PRODID)); >> > + >> > + if (status & OMAP3_SKUID_720MHZ) { >> > + omap3_features |= OMAP3_HAS_720MHZ; >> > + } >> braces not required > > [sp] Will change. forgot to remove after moving code to a > different function. > >> > } > > [snip]...[snip] > >> > + r = opp_enable(&(oh_mpu->od->pdev.dev), 720000000); >> > + if (r < 0) { >> > + pr_err("%s: Unable to enable OPP for >> mpu.", __func__); >> since you have dev pointer, pr_err can be replaced with dev_err > > [sp] Idea was to get to this function - in case of error; but can > be changed as well. you can also use __func__ with dev_err but it may not be required since dev_err provides enough information about the error. > >> > + goto err; >> > + } >> > + } >> > + >> > + if (!oh_iva || !oh_iva->od) { >> > + r = -ENODEV; >> > + goto err; >> > + } else { >> > + r = opp_enable(&(oh_iva->od->pdev.dev), 520000000); >> > + if (r < 0) { >> > + pr_err("%s: Unable to enable OPP for >> iva.", __func__); >> > + goto err; >> > + } >> > + } >> Can be optimized as(not tested and compiled): >> >> struct platform_device *pdev; > > [sp] Like this! ... to get to "dev". > >> struct omap_hwmod *oh_mpu = omap_hwmod_lookup("mpu"); >> struct omap_hwmod *oh_iva = omap_hwmod_lookup("iva"); >> int r = -ENODEV; >> >> if (!oh_mpu || !oh_mpu->od || !oh_iva || !oh_iva->od) >> goto err; >> >> pdev = oh_mpu->od->pdev; >> r = opp_enable(&pdev->dev, 720000000); >> if (r < 0) { >> dev_err(&pdev->dev, "Unable to enable OPP for mpu.\n"); >> goto err; >> } >> pdev = oh_iva->od->pdev; >> r = opp_enable(&pdev->dev, 520000000); >> if (r < 0) { >> dev_err(&pdev->dev, "Unable to enable OPP for iva.\n"); >> goto err; >> } >> >> Do you see any issues? > > Though it is not considered in this patch, but in the devices that > don't contain iva, the code related to enabling the iva freq can > be easily excluded in current flow. In this case, the function returns error if we have device with no iva. The API should enable only mpu opp and return sane value for devices with no iva. -Manjunath [...] -- 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
