Re: [PATCH] mmc: dw_mmc: rewrite CLKDIV computation

[Grant Grundler]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/