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

[Mike Frysinger]

On Thu, Apr 16, 2009 at 13:40,  <cliffcai.sh@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/