linux-kernel - Re: [PATCH 1/1] MMC: SDIO driver for Intel Moorestown platform

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] [day] [month] [year] [list]

Message-ID: <20090615132622.45fce509@lxorguk.ukuu.org.uk>
Date:	Mon, 15 Jun 2009 13:26:22 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	"Li, Jiebing" <jiebing.li@...el.com>
Cc:	Pierre Ossman <pierre@...man.eu>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Johnson, Charles F" <charles.f.johnson@...el.com>,
	"Zhu, Daniel" <daniel.zhu@...el.com>,
	"Yuan, Hang" <hang.yuan@...el.com>,
	"Li, Jiebing" <jiebing.li@...el.com>
Subject: Re: [PATCH 1/1] MMC: SDIO driver for Intel Moorestown  platform

> +       /*
> +        * Read the common CIS tuples.
> +        */
> +       err = sdio_read_cccr(card);
> +       if (err)
> +               goto remove;
> +
> +#ifdef CONFIG_MRST_MMC_WR
> +       /* restricting to 24MHz for Langwell A0 */
> +       if (card->cis.max_dtr > 24000000)
> +               card->cis.max_dtr = 24000000;
> +#endif

What if you have an SDIO device on a PCI card plugged into the system ?
You need some kind of card specific "fix up" call as a lot of other
drivers have I suspect ?


>
>         for (i = 0;i < host->card->sdio_funcs;i++) {
>                 if (host->card->sdio_func[i]) {
> +#ifdef CONFIG_SDIO_SUSPEND
> +                       sdio_remove_sysfs_file(host->card->sdio_func[i]);
> +#endif

Would be cleaner to always define sdio_remove_sysfs_file() and arrange in
the header files that this is

#define sdio_remove_sysfs_file(x)  do {} while (0)

>  static const struct mmc_bus_ops mmc_sdio_ops = {
>         .remove = mmc_sdio_remove,
>         .detect = mmc_sdio_detect,
> +       .suspend = mmc_sdio_suspend,
> +       .resume = mmc_sdio_resume,
>  };

You always expose suspend/resume yet you ifdef other stuff ?

> 
> @@ -323,6 +722,10 @@ int mmc_attach_sdio(struct mmc_host *host, u32 ocr)
>                 goto err;
>         }
> 
> +#ifdef CONFIG_SDIO_SUSPEND
> +       mutex_init(&card->pm_mutex);
> +#endif

Again this and the create_sysfs_file one could be hidden more nicely in
the header

>
--
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/