| Message ID | 1360427896-12004-4-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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/ > 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.
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,
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; }
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 */ 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; } static unsigned long gpmc_mem_align(unsigned long size) @@ -516,10 +520,10 @@ int gpmc_cs_request(int cs, unsigned long size, unsigned long *base) return -ENOMEM; spin_lock(&gpmc_mem_lock); - if (gpmc_cs_reserved(cs)) { - r = -EBUSY; + r = gpmc_cs_reserved(cs); + if (r < 0) goto out; - } + if (gpmc_cs_mem_enabled(cs)) r = adjust_resource(res, res->start & ~(size - 1), size); if (r < 0)
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(-)