lkml.org 
[lkml]   [2008]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2] Add GPIO-based MMC/SD driver
    Date
    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.


    \
     
     \ /
      Last update: 2008-07-21 23:25    [W:0.027 / U:29.012 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site