Re: [PATCH v2 3/4] mmc: renesas_sdhi: Fix sampling clock position selecting

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Wolfram,

Thanks for your feedback.

On 2018-07-15 20:45:06 +0200, Wolfram Sang wrote:
> On Thu, Jul 05, 2018 at 04:18:40PM +0200, Niklas Söderlund wrote:
> > From: Masaharu Hayakawa <masaharu.hayakawa.ry@xxxxxxxxxxx>
> > 
> > When tuning each tap is issued CMD19 twice and the result of both runs
> > recorded in host->taps. If the result is different between the two runs
> > the wrong sampling clock position was selected. Fix this by merging the
> > two runs and only keep the result for each tap if it was good in both
> > sets.
> > 
> > Signed-off-by: Masaharu Hayakawa <masaharu.hayakawa.ry@xxxxxxxxxxx>
> > [Niklas: update commit message]
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> 
> Much better commit message.
> 
> 
> > +	for (i = 0; i < host->tap_num * 2; i++) {
> > +		if (!test_bit(i, host->taps)) {
> > +			clear_bit(i % host->tap_num, host->taps);
> > +			clear_bit((i % host->tap_num) + host->tap_num,
> > +				  host->taps);
> > +		}
> > +	}
> 
> I just think the code is a bit clumsy maybe?
> 
> a) it clears the bit which is already cleared
> b) if a bit in the first half clears a bit in the second half,
>    they will both be cleared again when the loop processes the
>    second half
> 
> One idea I have is to let the loop iterate only over tap_num and then
> use a mask 'BIT(i) | BIT(i+tap_num)' and work with binary operators
> then. But maybe there are also macros to test and clear bit patterns?

I agree that the loop is clumsy. Unfortunately I can't find any bitmap 
operations that work with bit patterns. If this where just integers the 
following would have been a better solution:

    mask = (1 << host->tap_num) - 1;
    taps = (host->taps & (host->taps >> host->tap_num)) & mask;
    host->taps = (taps << host->tap_num) | taps;

Doing the same using the bitmap operations turns out to be more complex 
then the original loop. There is the option of using the 
bitmap_{from,to}_arr32() and use the bit operations above but that would 
limit the code to 16 taps. And that would deprive the value of using the 
bitmap in the first place.

However, I have reworked the loop to be easier to read and only use one 
clear_bit(). Hopefully this will make it less clumsy, if anyone know of 
a better way to handle this please let me know.

    for (i = 0; i < host->tap_num * 2; i++) {
	    int offset = host->tap_num * (i < host->tap_num ? 1 : -1);

	    if (!test_bit(i, host->taps))
		    clear_bit(i + offset, host->taps);
    }

> 
> And Geert's comment.
> 

-- 
Regards,
Niklas Söderlund
--
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



[Index of Archives]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux