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

[Anton Vorontsov <avorontsov@ru.mvista.com>]

On Wed, May 21, 2008 at 09:28:31PM +0200, Pierre Ossman wrote:
> On Wed, 21 May 2008 22:47:13 +0400
> Anton Vorontsov <avorontsov@ru.mvista.com> 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.

I think this is applicable to the .set_ios() too.

[...]
> > +	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.

Yeah, this makes sense. Indeed pretty easy to trigger [if poll interval
increased to 3 seconds, for example].

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

cd_status was a bitfield, so I thought that bit operations would be
appropriate. :-)

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

-ENOSYS (not implemented) sounds better for this purpose...

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

Ok. How about this version?

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2