On Wed, Mar 27, 2013 at 8:07 AM, Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > Grant, > > Thanks for posting! See below... thanks for reviewing/feedback! :) > On Tue, Mar 26, 2013 at 3:50 PM, Grant Grundler <grundler@xxxxxxxxxxxx> wrote: >> Last year Seungwon Jeon (Samsung) fixed a bug in CLKDIV computation. >> But when debugging a related issue (http://crbug.com/221828) I found >> the code unreadable. This rewrite simplifies the computation and >> explains each step. > > The fact that you mention a bug here is confusing. I think this patch > has nothing to do with fixing that bug and is just a readability > patch. Is that correct? Correct. "related" implies "not the same". But you are right - the reference is causing confusion. I will move the reference to the bug out of the commit log to below the '---' area of the patch. > Please add to the description if so and > maybe remove unrelated comment about the bug. Thanks! Will do and repost later today. ... >> + /* See 6.2.3 CLKDIV in "Mobile Storage Host Databook" >> + * Look for dwc_mobile_storage_db.pdf from Synopsys. >> + * CLKDIV value 0 means divisor 1, val 1 -> 2, ... > > You are quoting exynos5250 docs here. This driver is used for more > than just exynos and so this could be confusing to users on other > platforms. I'm quoting Synopsys docs - that's the origin of this HW's ip. You and I looked at exynos5250 docs originally and they say exactly the same thing. But the section numbers are different. > >> */ >> - div += 1; >> - >> - div = (host->bus_hz != slot->clock) ? DIV_ROUND_UP(div, 2) : 0; >> + div /= 2; > > It does look like you're re-implementing DIV_ROUND_UP. Yes, it does look like that but by breaking it out into simple steps AND explaining why we do each step, the code becomes maintainable by normal developers. The comments are key to *quickly* understanding the code in this case. > Maybe replace your "if" test and division with just a DIV_ROUND_UP? No. I'd rather just drop the patch. This code can and should be stupid simple. DIV_ROUND_UP just makes it harder to understand and impossible to document as clearly. cheers, grant -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
