On 07.06.2010 22:53, Andrew Morgan wrote:
On 04/06/10 15:13, Carl-Daniel Hailfinger wrote:
Oh, it looks really nice. In fact, we could commit this instantly except for two reasons:
- It's untested, and completely untested drivers should default to no
in the Makefile. Once we have the first successful test, we can change it to yes. 2. The INB/OUTB for data contradicts the datasheet (I know, the datasheet might very well be wrong), so a comment would be appreciated in those places, e.g. "The datasheet says this register can only be accessed with full 32 bit width, but that would make 8 bit writes impossible. Due to that, we assume the meaning was garbled in translation."
Hm. It is entirely possible that the datasheet means the register access mode is 32 bit. The contents of the data register might only care about the lowest 8 bits. That would also mean your driver could work...
Change those two things, and you get an ack from me and I'll commit.
Regards, Carl-Daniel
Thanks for looking at the previous patch! New patch attached... This is now for DP83815/DP83816 (same PCI ID) and DP83820 (same flash access). I disabled compilation by default, and added the comment in both places. I was hoping that now I have had a chance to solder a boot ROM socket to the card I could test the code and make it work. I have done some tests and it looks like this is very close, in fact it may well be that this code works but my soldering is no good! I've been getting some good data read & written, and some random data appearing. Probing seems to work fine. Erasing seems to work too, but read and/or write sometimes get random data. I believe a dry solder joint would explain this behavior.
Signed-off-by: Andrew Morganziltro@ziltro.com
- /*
- The datasheet says this register can only be
- accessed with full 32 bit width, but that would make 8 bit writes
- impossible. Due to that, we assume the meaning was garbled in translation.
- */
Can you reformat this comment (and the other identical comment) a bit? Please make sure to take care of the 80 column limit, start each line with an asterisk and align the asterisks, similar to this:
/* * comment * more comment * even more comment */
Ah yes, and while I know I suggested this wording, I'd like to rephrase it a bit. Sorry.
"The datasheet requires 32 bit accesses to this register, but it seems that requirement might only apply if the register is memory mapped. Bit 8-31 of this register are apparently don't care, and if this register is I/O port mapped 8 bit accesses to the lowest byte of the register seem to work fine. Due to that, we ignore the advice in the data sheet."
Note to you: It would be an interesting experiment to replace the OUTB with an OUTL, and compare write/read reliability if the high 24 bits are 1 or 0. This is not needed before I commit, but hey... maybe it helps you and the soldering is OK after all.
Sorry for the nitpicks in the second review round. I think the patch looks really good, and plan to apply it later tonight.
Regards, Carl-Daniel