Auf 07.03.2011 02:42, Michael Karcher schrieb:
Hello Carl-Daniel,
+ifeq ($(CONFIG_MEDIATEK_PATA), yes) +FEATURE_CFLAGS += -D'CONFIG_MEDIATEK_PATA=1' +PROGRAMMER_OBJS += mediatek.o +# actually, I/O access, not PCI +NEED_PCI := yes +ifeq ($(OS_ARCH), Linux) +LIBS += $(shell pkg-config --libs libsysfs)
"pkg-config --libs libsysfs" returns an error on my opensuse 11.3 machine, but libsysfs is installed. This is caused by a missing libsysfs.pc file. Could you fall back to -lsysfs in that case?
libsysfs-devel or the like is also installed? If yes, we need the fallback.
On opensuse 11.3, all needed files are part of the sysfsutils package (headers + lib).
+FEATURE_CFLAGS += $(shell pkg-config --cflags libsysfs)
Same problem here. Fallback: -I/usr/include/sysfs
Besides that, do we want to test for libsysfs like we test for libpci?
Probably we need to. The sysfs code (for kicking the linux driver and reenabling it) might even be made optional.
Related side note: Should we finally kill the overly complicated libftdi detection code, enable it always, and do a libpci-style detection instead (with a slightly modified error message which suggests to set CONFIG_FT2232_SPI=no to avoid the compile error)?
Didn't look at it yet, but auto-setting to "no" is more user friendly than requiring some magic option.
Hm. You're right, autodetection is more user friendly.
+#include <libsysfs.h>
sysfs/libsysfs.h instead? Not sure where it lives on Debian.
No, not <sysfs/libsysfs.h>. The file resides in /usr/include/sysfs, but that is passed to the "-I" option, so just <libsysfs.h> needs to be specified here.
OK.
+struct ataintf {
A comment what "intf" means would be appreciated.
"interface". OK, I might make a comment.
Thanks.
outb(command[i], ata->portbase+i);OUTB please, otherwise non-Linux platforms will fail.
Good point.
- outb(command[0], ata->portbase+7);
- programmer_delay(10000);
A comment explaining the delay would be nice.
[..]
INB Magic numbers. Please use #defines instead where possible.
Yep, will change that.
+{
- ata->portbase = portbase;
- if (is_slave)
ata->slavebit = 0x10;#define for 0x10 please.
OK, that looks so obvious to IDE-knowing people that you don't think about naming it, but you are right.
+#define MTK_A19toA17 1 +#define MTK_CONTROL 2 +#define MTK_DATA 3 +#define MTK_A7toA0 4 +#define MTK_A15toA8 5
+#define MTK_CTL_RAISE_nWE 0x01 +#define MTK_CTL_LOWER_nWE 0x02 +#define MTK_CTL_RAISE_nOE 0x04 +#define MTK_CTL_LOWER_nOE 0x08 +#define MTK_CTL_RAISE_nCS 0x10 +#define MTK_CTL_LOWER_nCS 0x20 /* latches low 8 address bits */ +#define MTK_CTL_DRIVE_DATA 0x40 +#define MTK_CTL_ADDRESS_A16 0x80
Some #defines have lowercase chars, but I don't really think that readability would benefit from making them uppercase. Hm.
I even suspect that readability is improved by using this small amount of lowercase letters, just like a small amount of uppercase letters can improve readability of variable names. If there is a strict "all defines pure uppercase" policy, I will reconsider the names, otherwise, I prefer to keep them as is.
Keep them. There is no hard policy AFAIK, and even if there was one, code readability wins.
+static int mtk_open_flash(struct ataintf * ata) +{
- unsigned char task[7];
- task[0] = 0x80; /* Vendor specific */
- task[3] = 0x2A; /* mediatek: start flash access */
- ide_submit(ata, task);
I think Linux once supported ATA command filters for security reasons. Do we have to catch something like that, and does that security feature even impact us?
If I were using the Linux ATA command submission function, you might raise a valid point, but "ide_submit" is the direct I/O function some lines above. In fact, this is the only command which could be sent through the linux interface, as it generates an IRQ. But most likely only if the drive currently has a valid firmware. If the drive has no valid firmware, the linux command layer can't be used anyway.
Ah OK.
+int mediatek_pata_init(void) +{
- unsigned iobase;
- int isslave;
- char *portpos = NULL, *devpos = NULL;
- /* IDE port specified */
- if ((portpos = extract_programmer_param("iobase"))) {
char *endptr = NULL;char *slaveptr;unsigned long tmp;tmp = strtoul(portpos, &endptr, 0);free(portpos);We free portpos here.
/* Port 0, port >0x10000, unaligned ports and garbage strings* are rejected.*/if (!tmp || (tmp >= 0x10000) || (tmp & 0x7) ||Is the alignment really 0x8, or something different?
For IDE, alignment is 8.
(*endptr != '\0')) {And dereference portpos here. Boom. (If this is copy-pasted, this means we access freed memory in another driver as well.)
The freeing error is my fault. I felt clever moving the free to the top to not have it in the error case as well as in the success case. Obviously I outsmarted myself. Excellent catch.
Thanks!
slaveptr = extract_programmer_param("slave");Mh, so anything starting with slave= will match?
The intention was that the pure mentioning of "slave" as option will match (a boolean option toggled by presence), but it turned out that "slave=" will be needed, so I will change that to "drive=master"/"drive=slave".
Looks good.
- if (mtk_open(iobase, isslave, &mtk_intf) < 0)
- {
msg_perr("can't access that device, aborting\n");+// return -1;
Why is this commented out?
Because that's a leftover from a test. mtk_open currently never fails, and I am considering merging it with mtk_open_flash. Previously, it checked for a valid ATAPI device on that port. But that check fails if there is no valid firmware, so I disabled it.
It might be interesting to print a diagnostic message in that case, e.g. "It seems this drive does not have a valid flash image matching the drive. Continuing anyway." Your choice, though.
I did not review any Linux-specific stuff, but the rest looks OK apart from the coding style issues you mentioned. Someone with more sysfs/libsysfs knowledge should take a look at the sysfs related stuff.
Yeah, that's a good idea. Maybe that code profits from some comments telling the expected position in the device tree.
Thank you for the review. I will send an improved patch soon.
Thank you for reverse engineering and coding this patch!
Regards, Carl-Daniel