Re: [PATCH] MMC: add support for the Marvell platform SDHCI controller

[saeed bishara <saeed.bishara@gmail.com>]

On Wed, Jun 23, 2010 at 2:52 PM, Matt Fleming <matt@console-pimps.org> wrote:
> On Tue, 22 Jun 2010 17:09:02 +0300, Saeed Bishara <saeed@marvell.com> wrote:
>> This patch implements the driver for the platfrom SDHCI controllers that found on some of Marvell's SoC's.
>>
>> Signed-off-by: Saeed Bishara <saeed@marvell.com>
>
> That's a nice, small driver ;-)
>
> [...]
>
>> +
>> +     if (!devm_request_mem_region(&pdev->dev, iomem->start,
>> +                                  resource_size(iomem),
>> +                                  mmc_hostname(host->mmc))) {
>> +             dev_err(&pdev->dev, "cannot request region\n");
>> +             ret = -EBUSY;
>> +             goto err_request;
>> +     }
>> +
>> +     host->ioaddr = devm_ioremap(&pdev->dev, iomem->start,
>> +                                 resource_size(iomem));
>> +     if (!host->ioaddr) {
>> +             dev_err(&pdev->dev, "failed to remap registers\n");
>> +             ret = -ENOMEM;
>> +             goto err_request;
>> +     }
>
> If you fail to remap the registers shouldn't you release the mem_region
> you've allocated above?
the point behind the devm_ (device managed) resouce allocation is not
to do the free explicitly, the kernel does that on behalf of the
driver.
>
>> +
>> +#if defined(CONFIG_HAVE_CLK)
>> +     mv_host->clk = clk_get(&pdev->dev, NULL);
>> +     if (IS_ERR(mv_host->clk))
>> +             dev_notice(&pdev->dev, "cannot get clkdev\n");
>> +     else
>> +             clk_enable(mv_host->clk);
>> +#endif
>> +
>> +     ret = sdhci_add_host(host);
>> +     if (ret)
>> +             goto err_request;
>
> Likewise here, I think you should be iounmap()'ing the registers if this
> fails. Otherwise this looks good.
ditto
>