sdhci: Best Practice Question

[Philip Rakity <prakity@xxxxxxxxxxx>]

and a 
host->ops-> get_max_clock()
function that is used to set the max clock.  The code snippet is below


	host->max_clk =
		(caps & SDHCI_CLOCK_BASE_MASK) >> SDHCI_CLOCK_BASE_SHIFT;
	host->max_clk *= 1000000;
	if (host->max_clk == 0 || host->quirks &
			SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN) {
		if (!host->ops->get_max_clock) {
			printk(KERN_ERR
			       "%s: Hardware doesn't specify base clock "
			       "frequency.\n", mmc_hostname(mmc));
			return -ENODEV;
		}
		host->max_clk = host->ops->get_max_clock(host);
	}


The quirk seems redundant since this could be coded as follows to remove the need for the quirk

> +	if (host->ops->get_max_clock)
> 		host->max_clk = host->ops->get_max_clock(host);
> +	else {
> +		host->max_clk =
> +			(caps & SDHCI_CLOCK_BASE_MASK) 
> +				>> SDHCI_CLOCK_BASE_SHIFT;
> +		host->max_clk *= 1000000;
> +	}
> +
> +	if (host->max_clk == 0) {
> +		printk(KERN_ERR
> +		       "%s: Hardware doesn't specify base clock "
> +		       "frequency.\n", mmc_hostname(mmc));
> +		return -ENODEV;
> 	}
> 


This approach was not used since it was felt a quirk was needed.

http://kerneltrap.org/mailarchive/linux-kernel/2010/2/19/4540115

but the quirk redundant and it requires two items be set; the quirk and the callback.
Defining the quirk and not the callback is meaningless.  

The change proposed by Richard Zhu for handling write protect uses only a callback.

<snip>
static int sdhci_get_ro(struct mmc_host *mmc)
{
	struct sdhci_host *host;
	unsigned long flags;
	int present;

	host = mmc_priv(mmc);

	if (host->ops->get_ro)
		return host->ops->get_ro(host);

	spin_lock_irqsave(&host->lock, flags);

<end snip>

What is the correct practice?  

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