On Monday 21 July 2008 22:41:35 David Brownell wrote: > So what this driver adds is mostly a userspace interface, yes?
Mostly. It also provides a simple platform device. > If so, please refocus the descriptions a bit more on that; > it's not actually a "GPIO based MMC/SD driver"; it's really > a "Configfs creation interface for GPIO based MMC/SD". > > > > > + res = sscanf(buf, "%" GPIOMMC_MAX_NAMELEN_STR "s %u %u %u %u %u %u %u > > > %u", > > Kind of ugly, yes? ;) This is already gone. > Please switch to the more conventional "MOSI" (Master Out, Slave In) > and "MISO" (Master In, Slave Out). That's unambiguous. If you want > to be a bit more clear you could say that MOSI goes to the MMC/SD > card pin 2 (card data in), and MISO goes to MMC/SD card pin 7 (card > data out). Maybe. > > > + if (res < 8) > > > + pdata->p.no_spi_delay = 0; /* Default: Delay turned on */ > > > + if (res < 7) > > > + pdata->p.max_bus_speed = 5000000; /* Default: 5Mhz */ > > So I'd default to leaving the delay off How to? > > > + if (res < 6) > > > + pdata->p.mode = 0; /* Default: SPI mode 0 */ > > Don't need to do this. The mmc_spi driver forces mode 0 in > any case... Where's that documented? > > > +config GPIOMMC > > > + tristate "MMC/SD over GPIO-based SPI" > > "Sysfs interface for ..." > > Because we can already do MMC/SD over GPIO-SPI, > without using this code. This also adds a platform device interface for convenience (as it also uses that internally). > > > +config MMC_S3C > > > + tristate "Samsung S3C SD/MMC Card Interface support" > > > + depends on ARCH_S3C2410 && MMC > > MMC_S3C doesn't belong in this patch... Yeah that was a mismerge. You are commenting on an obsolete patch. > > > + * @no_spi_delay: Do not use delays in the lowlevel SPI bitbanging code. > > > + * This is not standards compliant, but may be required > > > for some > > > + * embedded machines to gain reasonable speed. > > Rather than emphasize "not standards compliant", I'd instead emphasize that > bitbanged SPI is so slow that you probably don't want to slow it down any > more by additional delays between per-bit operations! I'd say you cannot tell how fast the GPIOs are. They can be pretty fast on a huge machine. > Again, please don't promote a *SECOND* platform-device based mechanism It is not a second mechanism, it is a mechanism on top of the first two mechanisms. > > > +Registering devices via platform-device > > > +======================================= > > > + > > > +The platform-device interface is used for registering MMC/SD devices > > > that are > > > +part of the hardware platform. This is most useful only for embedded > > > machines > > > +with MMC/SD devices statically connected to the platform GPIO bus. > > There is no "platform GPIO bus" ... hm?? > This is the interface I'd rather just not see exposed ... People asked me to expose it. It was hidden in the first patch that I submitted. I'm not going to change this just to see the next one yelling "I want to see this interface in public" again. > > > +Registering devices via sysfs > > > +============================= > > This is the interface I don't have any particular issues with. Yeah, but it's gone. See updated patches. -- Greetings Michael. _______________________________________________ openwrt-devel mailing list [email protected] http://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
