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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 22 Apr 2009 23:07:46 -0400
From:	Mike Frysinger <vapier.adi@...il.com>
To:	cliffcai.sh@...il.com
Cc:	pierre@...man.eu, linux-kernel@...r.kernel.org,
	cliff.cai@...log.com
Subject: Re: [PATCH][RESEND][mmc/host]:Blackfin SD Host Controller Driver

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