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
