Re: [PATCH][RESEND][mmc/host]:Blackfin SD Host Controller Driver

[Mike Frysinger <vapier.adi@gmail.com>]

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