Re: Results of my work on memorystick subsystem

[Maxim Levitsky]

On Thu, 2010-10-14 at 23:45 -0700, Alex Dubov wrote:
> > 
> > 
> > And what I had to go through to understand the
> > mspro_block_read_attributes....
> > 
> 
> Ellipsis to the rescue! :-)
> This is just one straightforward function which parses the header block
> in an originally intended fashion. You can simplify it by making it "less
> correct", windows driver style, but otherwise I fail to see what could be
> so difficult about it.

Alex, it is now straight forward.

Lets see what gems I found in old version:

* It does obscure math with sizes, offsets.
multiplies, divides offsets, sizes, uses data stored in 'param' register
to 'recover' the data.

* It overloads the 'rc' variable to store attribute size.
That is outright nasty as you would say.

* It has not a single comment, especially about its caching.

Alex, don't get me wrong.

Let me explain why did I refactor the mspro_blk.c

I have created the ms_block.c, and I just had to create a set of common
helpers to reduce code duplication because I hate it.
It works well, very well now.

Now I had put quite a lot of changes to common code, including few state
variables in the 'card' structure.

I really had to do that. I added common INT polling feature to reduce
code duplication. I also added a timeout for INT polling. It saved me
from restart for a lot of times (while I debugged the code).
And I really can't put a piece of code that can enter an endless loop
if conditions are right.

If I were to inline the INT read function, it would create a huge code
duplication.

I added a register read/write functions.
These made it possible to be free of constant fear of sending TPC of
wrong size because register window isn't updated.
It allowed me to read/write just the right registers everywhere without
additional effor, and most importantly reduce the >8 bytes TPC which
improve performance significantly on Jmicron.

It also helped me a lot to debug and workaround that damn Jmicon reader.

The way I handle card states not only allowed me to have several states
that handle a same TPC, but allowed me to have states that don't handle
any.

Now on the other hand, the mspro_blk.c did't use these changes, and
therefore was in constant danger of beeing broken.
I had to think carefully how to add changes, while keeping the old way
working.

So I bit the bullet, and made the mspro_blk.c use new support code.
Among the way I tried to make it simpler.
I understand that you might not like that I 'made your code better',
That is very subjective, and besides,
I'll say maybe I didn't. Yet at least it is consistent with ms_block.c

Alex, maybe I shouldnât say that new code is nice and clean.
Don't hate me because of that.




--
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/