lkml.org 
[lkml]   [2009]   [Apr]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH][RESEND][mmc/host]:Blackfin SD Host Controller Driver
    From
    On Thu, Apr 16, 2009 at 13:40,  <cliffcai.sh@gmail.com> wrote:
    > +#define NR_SG  32

    seems silly to have a define when it's used only once

    > +struct sdh_host {
    > +       struct mmc_host         *mmc;
    > +       spinlock_t              lock; /* Why I have to give a comment here? */

    why ?

    > +static void sdh_setup_data(struct sdh_host *host, struct mmc_data *data)
    > +{
    > +       pr_debug("%s enter flags:0x%x\n", __func__, data->flags);

    i wonder why these fixes exist in this patch but not the Blackfin tree
    (__FUNCTION__ -> __func__) ...

    also, many of these pr_debug()'s should probably be dev_dbg() ...

    > +/* RSI DMA doesn't work in array mode */

    comment should be indented

    > +static int sdh_get_ro(struct mmc_host *mmc)
    > +{
    > +       /* Host doesn't support read only detection so assume writeable */
    > +       return -ENOSYS;
    > +}

    the common code already checks to see if get_ro is NULL before calling
    it, so we dont want this stub

    > +       if (ios->bus_width == MMC_BUS_WIDTH_4) {
    > +               cfg = bfin_read_SDH_CFG();
    > +               cfg &= ~0x80;
    > +               cfg |= 0x40;
    > +               /* Enable 4 bit SDIO */
    > +               cfg |= 0x0c;

    shouldnt these magic numbers be defines ?

    > +static int __devinit sdh_probe(struct platform_device *pdev)
    > +{
    > +       struct mmc_host *mmc;
    > +       struct sdh_host *host = NULL;

    dont think this is actually needed

    > +       mmc = mmc_alloc_host(sizeof(struct sdh_host), &pdev->dev);

    sizeof(*mmc)

    > +       mmc->f_min = get_sclk() >> 9;
    > +       mmc->f_max = get_sclk();

    store get_sclk() once to avoid calling this function twice
    mmc->f_max = get_sclk();
    mmc->f_min = mmc->f_max >> 9;

    > +       sd_entry = create_proc_entry("driver/sdh", 0600, NULL);
    > +       sd_entry->read_proc = NULL;
    > +       sd_entry->write_proc = proc_write;
    > +       sd_entry->data = host;

    and if create_proc_entry() returns NULL ? like when procfs support is
    turned off ?

    > +static struct platform_driver sdh_driver = {
    > +       .probe          = sdh_probe,
    > +       .remove         = sdh_remove,

    shouldnt that be __devexit_p(sdh_remove) ?
    -mike
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2009-04-23 05:11    [W:0.023 / U:0.904 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site