Re: [PATCH 3/4] [MMC] mmc_spi: add polling support for the carddetect line

[Pierre Ossman]

On Wed, 21 May 2008 22:47:13 +0400
Anton Vorontsov <avorontsov@xxxxxxxxxxxxx> wrote:

> 
> Calling get_cd() for every request smells like overhead, especially given
> that that get_cd() could ask for GPIO status via relatively slow bus (like
> I2C GPIO expanders). So, polling seems most reasonable solution here, no
> need to call it very often.
> 

Fair enough. You should probably add a comment about this somewhere so
that people do not call get_cd() in the core request function and
similar places. Place it so that both get_cd() and get_ro() are covered
though, as it should be relevant for both.

> How about these patches? Tested with and without get_cd() optimization.
> 

Very nice. But I have some concerns:

> @@ -53,6 +53,7 @@ struct mmc_host_ops {
>  	void	(*request)(struct mmc_host *host, struct mmc_request *req);
>  	void	(*set_ios)(struct mmc_host *host, struct mmc_ios *ios);
>  	int	(*get_ro)(struct mmc_host *host);
> +	int	(*get_cd)(struct mmc_host *host);
>  	void	(*enable_sdio_irq)(struct mmc_host *host, int enable);
>  };

I'm always nervous when it comes to adding more callbacks, but I
suppose I've resisted this one for far too long already.

> +	if (host->ops->get_cd) {
> +		int old_cd_status = host->cd_status;
> +
> +		host->cd_status = !!host->ops->get_cd(host);
> +		if (!(old_cd_status ^ host->cd_status)) {
> +			mmc_bus_put(host);
> +			goto out;
> +		}
> +	}
> +

This should only be done when there is no bus handler. Since we are
polling, we might actually miss the user removing and reinserting the
card. The only way to check for that is to poke the card and see if it
is still alive. This also means you won't need that state variable.

Also, that second if clause seems fit for an obfuscation contest. ;)

> p.s. Since mmc_host_ops no longer the same for every instance of
> mmc_spi, struct mmc_host_ops can't be const and should be allocated
> dynamically.

This can be solved by allowing get_cd() to return an error that will be
treated as if get_cd() wasn't defined. -ENODEV seems suitable.

(get_ro() needs the same treatment, but I haven't gotten around to
that)

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/