On Tue, Apr 19, 2011 at 12:20:42PM +0200, Wolfram Sang wrote:
>
> On Fri, Mar 25, 2011 at 04:48:47PM +0800, Shawn Guo wrote:
> > The patch turns the common stuff in sdhci-pltfm.c into functions, and
> > add device drivers their own .probe and .remove which in turn call
> > into the common functions, so that those sdhci-pltfm device drivers
> > register itself and keep all device specific things away from common
> > sdhci-pltfm file.
> >
> > Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
> > ---
>
> I'll second the comments from Grant (with one slight exception which is
> noted below)
>
> > +static int __devexit sdhci_dove_remove(struct platform_device *pdev)
> > +{
> > + struct sdhci_host *host = platform_get_drvdata(pdev);
> > + int dead = 0;
> > + u32 scratch;
> > +
> > + scratch = readl(host->ioaddr + SDHCI_INT_STATUS);
> > + if (scratch == (u32)-1)
> > + dead = 1;
>
> I'd prefer
>
> dead = (readl() == 0xffffffff);
>
> (or (u32)-1 if you prefer). But keeping a variable 'dead' is more
> descriptive than keeping 'scratch'.
>
Ok.
> > +MODULE_LICENSE("GPL v2");
>
> Just to be sure: Did you double-check if the original licenses were v2
> or v2+?
>
It seems to me that sdhci-cns3xxx.c, sdhci-dove.c, sdhci-esdhc-imx.c
and sdhci-tegra.c all use v2.
Actually I do not even know how v2+ is stated. Do you have an example
for me to refer?
> > --- a/drivers/mmc/host/sdhci-pltfm.c
> > +++ b/drivers/mmc/host/sdhci-pltfm.c
>
> [...]
>
> > -err_add_host:
> > - if (pdata && pdata->exit)
> > - pdata->exit(host);
> > -err_plat_init:
> > - iounmap(host->ioaddr);
> > err_remap:
> > release_mem_region(iomem->start, resource_size(iomem));
> > err_request:
> > sdhci_free_host(host);
> > err:
> > - printk(KERN_ERR"Probing of sdhci-pltfm failed: %d\n", ret);
> > - return ret;
> > + pr_err("%s failed %d\n", __func__, ret);
>
> dev_err?
>
Good catch.
> > + return NULL;
> > }
> >
>
> I didn't pay much attention to the OF version of the tegra driver, since
> it still is not upstream yet, right?
>
Do not worry about that. You will not see that in the next version,
which will be based on mmc-next. And tegra OF support should be in
another patch.
--
Regards,
Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
