lkml.org 
[lkml]   [2008]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/4] [MMC] mmc_spi: add polling support for the card detect line
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


\
 
 \ /
  Last update: 2008-05-22 20:19    [W:0.048 / U:28.196 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site