On Thu, Jan 13, 2011 at 5:21 PM, Premi, Sanjeev <premi@xxxxxx> wrote: > >> -----Original Message----- >> From: G, Manjunath Kondaiah [mailto:manjugk@xxxxxx] >> Sent: Thursday, January 13, 2011 5:16 PM >> To: Premi, Sanjeev >> Cc: linux-omap@xxxxxxxxxxxxxxx >> Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part >> >> On Thu, Jan 13, 2011 at 4:48 PM, Premi, Sanjeev <premi@xxxxxx> wrote: >> >> -----Original Message----- >> >> From: G, Manjunath Kondaiah >> >> Sent: Thursday, January 13, 2011 2:58 PM >> >> To: Premi, Sanjeev >> >> Cc: linux-omap@xxxxxxxxxxxxxxx >> >> Subject: Re: [PATCHv2] omap3: Add basic support for 720MHz part >> >> >> > >> > [....] >> > >> >> >> 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. >> >> >> > >> > [sp] Don't want to get into details of next patch here, but >> when there >> > is no IVA, the "if" condition for for iva won't/shouldn't >> be entered. >> exactly >> > >> > Values returned should just be the status opp_enable. OR am >> I missing >> > something in your comment. >> > e.g. >> > 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; >> > } >> > >> > if omap3_has_iva()) { >> > oh_iva = omap_hwmod_lookup("iva"); //move >> from current impl. >> >> This looks fine and it can translate into: >> if (!oh_mpu || !oh_mpu->od) >> goto err_nodevice; >> >> pdev = oh_mpu->od->pdev; >> r = opp_enable(&pdev->dev, 720000000); >> if (r < 0) >> goto err_oppenable; >> >> if (!omap3_has_iva()) >> goto err_nodevice; > > Not having an IVA is not error! and goto's really aren't saving anything. it is returning value from mpu opp enable and goto naming is misleading. You can change the name if it is misleading. -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
