Hi,
On Sat, Feb 09, 2013 at 03:14:33PM -0300, Ezequiel Garcia wrote:
> > > static int gpmc_cs_reserved(int cs)
> > > {
> > > if (cs > GPMC_CS_NUM)
> > > return -ENODEV;
> > >
> > > - return gpmc_cs_map & (1 << cs);
> > > + if (gpmc_cs_map & (1 << cs))
> > > + return -EBUSY;
> > > +
> > > + return 0;
> >
> > you could use a ternary operator here:
> >
> > bit = gpmc_cs_map & (1 << cs);
> >
> > return bit ? -EBUSY : 0;
> >
> > But to be frank, I'm not sure this makes that much sense, I'd expect
> > gpmc_cs_reserved() to return 0 or 1 depending on the state (just as it
> > was before). The name of the method asks for a boolean return value.
> >
>
> Well, we can change the return value to a boolean.
>
> However, note that the function 'as it was before' was somewhat inconsistent:
> gpmc_cs_reserved returned -ENODEV if the given 'cs' was out of range and
> a non-negative integer otherwise, 0 if the cs is available and positive
> integer otherwise.
>
> So, what I'm doing here is fixing this by returning an appropriate error
> condition or 0 if the CS is available.
>
> If we change it to return a boolean, we won't be able to distinguish
> between EBUSY and ENODEV.
that's the thing. IMO this should return 1 if it is available and 0 if
it's not. The method looks like a yes/no question:
Q: Is this GPMC CS reserved ?
A: Yes (1)
or
A: No (0)
then it's als far easier to use, just as code was before:
if (gpmc_cs_reserved(cs)) {
dev_dbg(dev, "Oops, CS #%d is busy\n", cs);
return -EBUSY;
} else {
dev_dbg(dev, "Hurray!\n");
return 0;
}
--
balbi
Attachment:
signature.asc
Description: Digital signature
