Re: [PATCH 3/7] ARM: omap2: gpmc: Fix gpmc_cs_reserved() return value

[Ezequiel Garcia <ezequiel.garcia@free-electrons.com>]

Hi Felipe,

On Sat, Feb 09, 2013 at 06:53:35PM +0200, Felipe Balbi wrote:
> On Sat, Feb 09, 2013 at 01:38:12PM -0300, Ezequiel Garcia wrote:
> > Fix gpmc_cs_reserved() so it returns 0 if the chip select
> > is available (not reserved) or an error otherwise.
> > This allows to use the return value directly and return a proper error code.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  arch/arm/mach-omap2/gpmc.c |   12 ++++++++----
> >  1 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> > index bd3bc93..604c1eb 100644
> > --- a/arch/arm/mach-omap2/gpmc.c
> > +++ b/arch/arm/mach-omap2/gpmc.c
> > @@ -452,12 +452,16 @@ static int gpmc_cs_set_reserved(int cs, int reserved)
> >  	return 0;
> >  }
> >  
> > +/* Returns 0 if CS is available (not reseverd) or an error otherwise */
> 
> s/reseverd/reserved/
> 

Indeed.

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

Let me know what you prefer and I'll fix it on v2.

Thanks for the review,

-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html