Hi,
Thanks for the review. All good points (and I forgot to do a last run
through checkpatch, my bad).
All have been addressed in the next version (not yet posted).
-Olof
On Wed, Dec 22, 2010 at 8:22 AM, Wolfram Sang <w.sang@xxxxxxxxxxxxxx> wrote:
> Hi Olof,
>
> looks good to me. Just some minor comments, but in general
>
> Reviewed-by: Wolfram Sang <w.sang@xxxxxxxxxxxxxx>
>
> On Wed, Dec 22, 2010 at 02:01:11AM -0600, Olof Johansson wrote:
>> SDHCI driver for Tegra. Pretty straight forward, a few pieces of
>> functionality left to fill in but nothing that stops it from going
>> upstream. Board enablement submitted separately.
>
> I'd think this can go below the "---" line?
>
> ...
>> This driver plugs in as a new variant of sdhci-pltfm, using the platform
>> data structure passed in to specify the GPIOs to use for card detect,
>> write protect and card power enablement.
>>
>> Original driver (of which only the header file is left):
>> Signed-off-by: Yvonne Yip <y@xxxxxxxx>
>>
>> The rest, which has been rewritten by now:
>> Signed-off-by: Olof Johansson <olof@xxxxxxxxx>
>> Cc: Wolfram Sang <w.sang@xxxxxxxxxxxxxx>
>> Cc: Mike Rapoport <mike@xxxxxxxxxxxxxx>
>> +static unsigned int tegra_sdhci_get_ro(struct sdhci_host *sdhci)
>> +{
>> + struct platform_device *pdev = to_platform_device(mmc_dev(sdhci->mmc));
>> + struct tegra_sdhci_platform_data *plat;
>> +
>> + plat = pdev->dev.platform_data;
>> +
>> + if (gpio_is_valid(plat->wp_gpio))
>> + return gpio_get_value(plat->wp_gpio);
>> +
>> + return -1;
>
> So, read_only is the default case? I would have gone for returning 0.
> Well, safety vs. convenience...
>
> ...
>> + if (rc) {
>> + dev_err(mmc_dev(host->mmc),
>
> Trailing white space
>
>> + "failed to allocate power gpio\n");
>> + goto out;
>> + }
>> + tegra_gpio_enable(plat->power_gpio);
>> + gpio_direction_output(plat->power_gpio, 1);
>> + }
>> +
>> + if (gpio_is_valid(plat->cd_gpio)) {
>> + rc = gpio_request(plat->cd_gpio, "sdhci_cd");
>> + if (rc) {
>> + dev_err(mmc_dev(host->mmc),
>
> ditto
>
>> + "failed to allocate cd gpio\n");
>> + goto out_power;
>> + }
>> + tegra_gpio_enable(plat->cd_gpio);
>> +
>> + rc = request_irq(gpio_to_irq(plat->cd_gpio), carddetect_irq,
>> + IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
>> + mmc_hostname(host->mmc), host);
>> +
>> + if (rc) {
>> + dev_err(mmc_dev(host->mmc), "request irq error\n");
>> + goto out_cd;
>> + }
>> +
>> + }
>> +
>> + if (gpio_is_valid(plat->wp_gpio)) {
>> + rc = gpio_request(plat->wp_gpio, "sdhci_wp");
>> + if (rc) {
>> + dev_err(mmc_dev(host->mmc),
>
> ditto
>
> Kind regards,
>
> Wolfram
>
> --
> Pengutronix e.K. | Wolfram Sang |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk0SJccACgkQD27XaX1/VRtnbgCdH8+R7hjsq4YjJIjvQgtkwOrF
> GHwAoJBRrdEv6+5d2sxZ5J/AqWsTXLb4
> =RLqI
> -----END PGP SIGNATURE-----
>
>
--
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
