lkml.org 
[lkml]   [2010]   [Jul]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3] MMC: Add JZ4740 mmc driver
    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA1

    Hi

    Andrew Morton wrote:
    > On Mon, 28 Jun 2010 03:20:41 +0200
    > Lars-Peter Clausen <lars@metafoo.de> wrote:
    >
    >> This patch adds support for the mmc controller on JZ4740 SoCs.
    >>
    >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
    >> Cc: Andrew Morton <akpm@linux-foundation.org>
    >> Cc: Matt Fleming <matt@console-pimps.org>
    >> Cc: linux-mmc@vger.kernel.org
    >>
    >> ...
    >>
    >> +#define JZ4740_MMC_MAX_TIMEOUT 10000000
    >
    > That was a really big timeout. How long do 1e7 readw's take? Oh well.
    >

    Hm, right. It doesn't hurt though. I'll do some tests and to try to come up with a
    more realistic value.

    >> ...
    >>
    >> +static void jz4740_mmc_clock_disable(struct jz4740_mmc_host *host)
    >> +{
    >> + uint32_t status;
    >> +
    >> + writew(JZ_MMC_STRPCL_CLOCK_STOP, host->base + JZ_REG_MMC_STRPCL);
    >> + do {
    >> + status = readl(host->base + JZ_REG_MMC_STATUS);
    >> + } while (status & JZ_MMC_STATUS_CLK_EN);
    >> +}
    >> +
    >> +static void jz4740_mmc_reset(struct jz4740_mmc_host *host)
    >> +{
    >> + uint32_t status;
    >> +
    >> + writew(JZ_MMC_STRPCL_RESET, host->base + JZ_REG_MMC_STRPCL);
    >> + udelay(10);
    >> + do {
    >> + status = readl(host->base + JZ_REG_MMC_STATUS);
    >> + } while (status & JZ_MMC_STATUS_IS_RESETTING);
    >> +}
    >
    > Maybe these should have a timeout too?

    Its very unlikely that these will ever timeout. But right, to be on the safe, there
    should be a timeout as well.

    >
    >> ...
    >>
    >> +static inline unsigned int jz4740_mmc_wait_irq(struct jz4740_mmc_host *host,
    >> + unsigned int irq)
    >> +{
    >> + unsigned int timeout = JZ4740_MMC_MAX_TIMEOUT;
    >> + uint16_t status;
    >> +
    >> + do {
    >> + status = readw(host->base + JZ_REG_MMC_IREG);
    >> + } while (!(status & irq) && --timeout);
    >> +
    >> + return timeout;
    >> +}
    >
    > This guy's too big to inline. Recent gcc's know that and they tend to
    > uninline such things behind your back anwyay.

    Actually, even without the inline attribute and compiling with -Os gcc will inline
    this function by itself.

    >
    >> ...
    >>
    >> +struct jz4740_mmc_platform_data {
    >> + int gpio_power;
    >> + int gpio_card_detect;
    >> + int gpio_read_only;
    >> + unsigned card_detect_active_low:1;
    >> + unsigned read_only_active_low:1;
    >> + unsigned power_active_low:1;
    >> +
    >> + unsigned data_1bit:1;
    >> +};
    >
    > The bitfields will all share the same word, so modification of one
    > field can race against modification of another field. Hence some form
    > of locking which covers *all* the bitfields is needed.
    >
    > Is that a problem in this driver?
    >
    They are all read-only in the driver.


    Thanks for reviewing the patch
    - - Lars

    -----BEGIN PGP SIGNATURE-----
    Version: GnuPG v1.4.9 (GNU/Linux)
    Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

    iEYEARECAAYFAkwsuDQACgkQBX4mSR26RiOiMACeMMNj4koCYFAUnxyM0LBr+wOZ
    x6oAnizk5CaAvZjQ2doXrD6ZYgDeNjSr
    =92D2
    -----END PGP SIGNATURE-----


    \
     
     \ /
      Last update: 2010-07-01 17:49    [W:4.160 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site