Re: [PATCH v7] ESDHC: Workaround for data crc error on p1010rdb

[Chris Ball <cjb@laptop.org>]

Hi,

On Wed, Feb 08 2012, Huang Changming-R66093 wrote:
>> > +static void esdhc_of_set_clock(struct sdhci_host *host, unsigned int
>> clock)
>> > +{
>> > +	/* Workaround to reduce the clock frequency for p1010 esdhc */
>> > +	if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
>> > +		if (clock == 0) {
>> > +			host->clock = clock;
>> > +			return;
>> > +		}
>> 
>> Is this if (clock == 0) necessary?  You'll be calling esdhc_set_clock()
>> immediately afterwards, and it performs the same test.
>
> Yes, I perform this test two times.
> The first time, I test the original value (as the parameter from set_clock)
> The second time, I test it again, that is because the clock value has
> been changed or reduced,
> I think it is necessary to test it again. 

I don't see how it's necessary to test *if it is zero* again, because
you're not going to change or reduce it to zero.  You're going to
subtract 5M/10M from it if it was over 20M/40M, so there is no chance
of it reaching zero.

This isn't a huge deal, but:  could you walk me through an example of
where it's necessary to have the first test for 0, because something
bad would happen if you didn't?  I'm talking about this piece of code:

+       if (of_find_compatible_node(NULL, NULL, "fsl,p1010-esdhc")) {
+               if (clock == 0) {
+                       host->clock = clock;
+                       return;
+               }

.. which I think will get caught by the same test in esdhc_set_clock()
when you call that immediately after this test.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child