On Thursday 05 June 2008, Anton Vorontsov wrote:
> On Tue, Jun 03, 2008 at 12:07:49PM +0200, Marc Pignat wrote:
> > Hi all!
> >
> > On Friday 23 May 2008, Anton Vorontsov wrote:
> > > get_ro() callback must return values >= 0 for its logical state, and
> > ...
> > > static void pxamci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
> > > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> > > index f2e9885..ef3b773 100644
> > > --- a/include/linux/mmc/host.h
> > > +++ b/include/linux/mmc/host.h
> > > @@ -55,6 +55,9 @@ struct mmc_host_ops {
> > > * Avoid calling these three functions too often or in a "fast path",
> > > * since underlaying controller might implement them in an expensive
> > > * and/or slow way.
> > > + *
> > > + * .get_ro and .get_cd should return >= 0 for their logical values,
> > > + * or negative errno value in case of error.
> > > */
> >
> > I would suggest to use something more strict (bulletproof), something like:
> >
> > /*
> > * get_ro will return:
> > * 0 for a read/write card
> > * 1 for a read-only card
>
> This isn't always practical. For example, host is using u8 register for
> the status, so it might safely return u8 & mask, that will always fit
> into int. Or very smart/adventurous authors might be aware that, for the
> particular host, mask's bit isn't last, and safely do uXX & mask. :-)
>
> The above is weak argument of course, since it is about optimization.
Ack, we will gain at most 1-4 assembly instructions, in a function that
is unlikely to be called more than once a second.
>
> As an counter-evidence, the strict scheme that you described probably
> less error prone. But is it? To implement it we'll need something like
> WARN_ON(ret > 0 && ret != 1) to catch erroneous users. Take a closer
> look though, will it catch uXX & lastbit case? Nope. :-)
WARN_ON(ret > 0 && ret != 1 || ret == INT_MIN) will do ;)
I agree with you once more, I never thinked about a runtime check.
I don't really want to see a WARN_ON(foo) after each call to get_ro or get_cd.
But I'm sure if we specify "give me a positive value when a card is detected",
someone will write gpio & bit, and three years later, someone will fall in
the (gpio & lastbit < 0 case).
So we should specify: "give me 1 whan a card is present, 0 when not, -ENOSYS if
you don't know and a negative errno when something else goes wrong".
Best regards
Marc
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
