[PATCH 0/7] ARM: omap2: GPMC cleanup

[PATCH 0/7] ARM: omap2: GPMC cleanup

[Ezequiel Garcia]

This patchset is a small cleanup consisting in:
 * mark some functions as 'static' when appropriate
 * remove an unused function from gpmc.c
 * improve error messages when a CS request fails
 * migrate to dev_err and dev_warn

It has been tested on a IGEP v2 board with OneNAND,
which means the gpmc-nand patch is tested by compilation only.

Altough these patchset is almost trivial,
any feedback or testing is more than welcome.

Ezequiel Garcia (7):
  ARM: omap2: gpmc: Mark local scoped functions static
  ARM: omap2: gpmc: Remove unused gpmc_round_ns_to_ticks() function
  ARM: omap2: gpmc: Fix gpmc_cs_reserved() return value
  ARM: omap2: gpmc-nand: Print something useful on CS request failure
  ARM: omap2: gpmc-onenand: Print something useful on CS request failure
  ARM: omap2: gpmc-onenand: Replace pr_err() with dev_err()
  ARM: omap2: gpmc-onenand: Replace printk KERN_ERR with dev_warn()

 arch/arm/mach-omap2/gpmc-nand.c    |    3 ++-
 arch/arm/mach-omap2/gpmc-onenand.c |    8 +++++---
 arch/arm/mach-omap2/gpmc.c         |   31 ++++++++++++++-----------------
 arch/arm/mach-omap2/gpmc.h         |    7 -------
 4 files changed, 21 insertions(+), 28 deletions(-)

-- 
1.7.8.6

[PATCH 1/7] ARM: omap2: gpmc: Mark local scoped functions static

[Ezequiel Garcia]

This patch marks a bunch of functions that are local
to gpmc.c file only as static.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-omap2/gpmc.c |   14 +++++++-------
 arch/arm/mach-omap2/gpmc.h |    7 -------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 1e8bcb4..ffe3e1e 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -181,7 +181,7 @@ void gpmc_cs_write_reg(int cs, int idx, u32 val)
 	__raw_writel(val, reg_addr);
 }
 
-u32 gpmc_cs_read_reg(int cs, int idx)
+static u32 gpmc_cs_read_reg(int cs, int idx)
 {
 	void __iomem *reg_addr;
 
@@ -190,7 +190,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
 }
 
 /* TODO: Add support for gpmc_fck to clock framework and use it */
-unsigned long gpmc_get_fclk_period(void)
+static unsigned long gpmc_get_fclk_period(void)
 {
 	unsigned long rate = clk_get_rate(gpmc_l3_clk);
 
@@ -205,7 +205,7 @@ unsigned long gpmc_get_fclk_period(void)
 	return rate;
 }
 
-unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
+static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
 {
 	unsigned long tick_ps;
 
@@ -215,7 +215,7 @@ unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
 	return (time_ns * 1000 + tick_ps - 1) / tick_ps;
 }
 
-unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
+static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
 {
 	unsigned long tick_ps;
 
@@ -230,7 +230,7 @@ unsigned int gpmc_ticks_to_ns(unsigned int ticks)
 	return ticks * gpmc_get_fclk_period() / 1000;
 }
 
-unsigned int gpmc_round_ns_to_ticks(unsigned int time_ns)
+static unsigned int gpmc_round_ns_to_ticks(unsigned int time_ns)
 {
 	unsigned long ticks = gpmc_ns_to_ticks(time_ns);
 
@@ -448,7 +448,7 @@ static int gpmc_cs_mem_enabled(int cs)
 	return l & GPMC_CONFIG7_CSVALID;
 }
 
-int gpmc_cs_set_reserved(int cs, int reserved)
+static int gpmc_cs_set_reserved(int cs, int reserved)
 {
 	if (cs > GPMC_CS_NUM)
 		return -ENODEV;
@@ -459,7 +459,7 @@ int gpmc_cs_set_reserved(int cs, int reserved)
 	return 0;
 }
 
-int gpmc_cs_reserved(int cs)
+static int gpmc_cs_reserved(int cs)
 {
 	if (cs > GPMC_CS_NUM)
 		return -ENODEV;
diff --git a/arch/arm/mach-omap2/gpmc.h b/arch/arm/mach-omap2/gpmc.h
index fe0a844..b79e35c 100644
--- a/arch/arm/mach-omap2/gpmc.h
+++ b/arch/arm/mach-omap2/gpmc.h
@@ -195,20 +195,13 @@ extern int gpmc_calc_timings(struct gpmc_timings *gpmc_t,
 extern void gpmc_update_nand_reg(struct gpmc_nand_regs *reg, int cs);
 extern int gpmc_get_client_irq(unsigned irq_config);
 
-extern unsigned int gpmc_ns_to_ticks(unsigned int time_ns);
-extern unsigned int gpmc_ps_to_ticks(unsigned int time_ps);
 extern unsigned int gpmc_ticks_to_ns(unsigned int ticks);
-extern unsigned int gpmc_round_ns_to_ticks(unsigned int time_ns);
-extern unsigned long gpmc_get_fclk_period(void);
 
 extern void gpmc_cs_write_reg(int cs, int idx, u32 val);
-extern u32 gpmc_cs_read_reg(int cs, int idx);
 extern int gpmc_calc_divider(unsigned int sync_clk);
 extern int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t);
 extern int gpmc_cs_request(int cs, unsigned long size, unsigned long *base);
 extern void gpmc_cs_free(int cs);
-extern int gpmc_cs_set_reserved(int cs, int reserved);
-extern int gpmc_cs_reserved(int cs);
 extern void omap3_gpmc_save_context(void);
 extern void omap3_gpmc_restore_context(void);
 extern int gpmc_cs_configure(int cs, int cmd, int wval);
-- 
1.7.8.6

[PATCH 2/7] ARM: omap2: gpmc: Remove unused gpmc_round_ns_to_ticks() function

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-omap2/gpmc.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index ffe3e1e..bd3bc93 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -230,13 +230,6 @@ unsigned int gpmc_ticks_to_ns(unsigned int ticks)
 	return ticks * gpmc_get_fclk_period() / 1000;
 }
 
-static unsigned int gpmc_round_ns_to_ticks(unsigned int time_ns)
-{
-	unsigned long ticks = gpmc_ns_to_ticks(time_ns);
-
-	return ticks * gpmc_get_fclk_period() / 1000;
-}
-
 static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
 {
 	return ticks * gpmc_get_fclk_period();
-- 
1.7.8.6

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

[Ezequiel Garcia]

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 */
 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)
-- 
1.7.8.6

[PATCH 4/7] ARM: omap2: gpmc-nand: Print something useful on CS request failure

[Ezequiel Garcia]

If CS request fails the current error message is rather unhelpful.
Fix it by printing the failing chip select and the error code.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-omap2/gpmc-nand.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index afc1e8c..e50e438 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -122,7 +122,8 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data,
 	err = gpmc_cs_request(gpmc_nand_data->cs, NAND_IO_SIZE,
 				(unsigned long *)&gpmc_nand_resource[0].start);
 	if (err < 0) {
-		dev_err(dev, "Cannot request GPMC CS\n");
+		dev_err(dev, "Cannot request GPMC CS %d, error %d\n",
+			gpmc_nand_data->cs, err);
 		return err;
 	}
 
-- 
1.7.8.6

[PATCH 5/7] ARM: omap2: gpmc-onenand: Print something useful on CS request failure

[Ezequiel Garcia]

If CS request fails the current error message is rather unhelpful.
Fix it by printing the failing chip select and the error code.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-omap2/gpmc-onenand.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index fadd8743..0ee5317 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -379,7 +379,8 @@ void gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
 	err = gpmc_cs_request(gpmc_onenand_data->cs, ONENAND_IO_SIZE,
 				(unsigned long *)&gpmc_onenand_resource.start);
 	if (err < 0) {
-		pr_err("%s: Cannot request GPMC CS\n", __func__);
+		pr_err("%s: Cannot request GPMC CS %d, error %d\n",
+		       __func__, gpmc_onenand_data->cs, err);
 		return;
 	}
 
-- 
1.7.8.6

[PATCH 6/7] ARM: omap2: gpmc-onenand: Replace pr_err() with dev_err()

[Ezequiel Garcia]

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-omap2/gpmc-onenand.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index 0ee5317..4771945 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -359,6 +359,7 @@ static int gpmc_onenand_setup(void __iomem *onenand_base, int *freq_ptr)
 void gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
 {
 	int err;
+	struct device *dev = &gpmc_onenand_device.dev;
 
 	gpmc_onenand_data = _onenand_data;
 	gpmc_onenand_data->onenand_setup = gpmc_onenand_setup;
@@ -379,8 +380,8 @@ void gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
 	err = gpmc_cs_request(gpmc_onenand_data->cs, ONENAND_IO_SIZE,
 				(unsigned long *)&gpmc_onenand_resource.start);
 	if (err < 0) {
-		pr_err("%s: Cannot request GPMC CS %d, error %d\n",
-		       __func__, gpmc_onenand_data->cs, err);
+		dev_err(dev, "Cannot request GPMC CS %d, error %d\n",
+			gpmc_onenand_data->cs, err);
 		return;
 	}
 
@@ -388,7 +389,7 @@ void gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
 							ONENAND_IO_SIZE - 1;
 
 	if (platform_device_register(&gpmc_onenand_device) < 0) {
-		pr_err("%s: Unable to register OneNAND device\n", __func__);
+		dev_err(dev, "Unable to register OneNAND device\n");
 		gpmc_cs_free(gpmc_onenand_data->cs);
 		return;
 	}
-- 
1.7.8.6

[PATCH 7/7] ARM: omap2: gpmc-onenand: Replace printk KERN_ERR with dev_warn()

[Ezequiel Garcia]

Since the condition is not an error but a warning, replace
printk KERN_ERR with dev_warn.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-omap2/gpmc-onenand.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index 4771945..fd6e35b 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -367,7 +367,7 @@ void gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
 
 	if (cpu_is_omap24xx() &&
 			(gpmc_onenand_data->flags & ONENAND_SYNC_READWRITE)) {
-		printk(KERN_ERR "Onenand using only SYNC_READ on 24xx\n");
+		dev_warn(dev, "OneNAND using only SYNC_READ on 24xx\n");
 		gpmc_onenand_data->flags &= ~ONENAND_SYNC_READWRITE;
 		gpmc_onenand_data->flags |= ONENAND_SYNC_READ;
 	}
-- 
1.7.8.6

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

[Felipe Balbi]

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.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130209/5a458fb9/attachment.sig>

[PATCH 7/7] ARM: omap2: gpmc-onenand: Replace printk KERN_ERR with dev_warn()

[Felipe Balbi]

Hi,

On Sat, Feb 09, 2013 at 01:38:16PM -0300, Ezequiel Garcia wrote:
> Since the condition is not an error but a warning, replace
> printk KERN_ERR with dev_warn.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  arch/arm/mach-omap2/gpmc-onenand.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> index 4771945..fd6e35b 100644
> --- a/arch/arm/mach-omap2/gpmc-onenand.c
> +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> @@ -367,7 +367,7 @@ void gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
>  
>  	if (cpu_is_omap24xx() &&
>  			(gpmc_onenand_data->flags & ONENAND_SYNC_READWRITE)) {
> -		printk(KERN_ERR "Onenand using only SYNC_READ on 24xx\n");
> +		dev_warn(dev, "OneNAND using only SYNC_READ on 24xx\n");

it would seem more natural to use dev_err() instead.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130209/3367ddbe/attachment.sig>

[PATCH 7/7] ARM: omap2: gpmc-onenand: Replace printk KERN_ERR with dev_warn()

[Ezequiel Garcia]

On Sat, Feb 09, 2013 at 06:55:32PM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Sat, Feb 09, 2013 at 01:38:16PM -0300, Ezequiel Garcia wrote:
> > Since the condition is not an error but a warning, replace
> > printk KERN_ERR with dev_warn.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > ---
> >  arch/arm/mach-omap2/gpmc-onenand.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> > index 4771945..fd6e35b 100644
> > --- a/arch/arm/mach-omap2/gpmc-onenand.c
> > +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> > @@ -367,7 +367,7 @@ void gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
> >  
> >  	if (cpu_is_omap24xx() &&
> >  			(gpmc_onenand_data->flags & ONENAND_SYNC_READWRITE)) {
> > -		printk(KERN_ERR "Onenand using only SYNC_READ on 24xx\n");
> > +		dev_warn(dev, "OneNAND using only SYNC_READ on 24xx\n");
> 
> it would seem more natural to use dev_err() instead.
> 

Are you sure? The error seems more a warning to me,
although I guess it's arguable.

Let me know and I'll fix it in v2.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

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

[Ezequiel Garcia]

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

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

[Felipe Balbi]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130209/77b5416f/attachment.sig>

[PATCH 7/7] ARM: omap2: gpmc-onenand: Replace printk KERN_ERR with dev_warn()

[Felipe Balbi]

On Sat, Feb 09, 2013 at 03:05:31PM -0300, Ezequiel Garcia wrote:
> On Sat, Feb 09, 2013 at 06:55:32PM +0200, Felipe Balbi wrote:
> > Hi,
> > 
> > On Sat, Feb 09, 2013 at 01:38:16PM -0300, Ezequiel Garcia wrote:
> > > Since the condition is not an error but a warning, replace
> > > printk KERN_ERR with dev_warn.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > > ---
> > >  arch/arm/mach-omap2/gpmc-onenand.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
> > > index 4771945..fd6e35b 100644
> > > --- a/arch/arm/mach-omap2/gpmc-onenand.c
> > > +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> > > @@ -367,7 +367,7 @@ void gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
> > >  
> > >  	if (cpu_is_omap24xx() &&
> > >  			(gpmc_onenand_data->flags & ONENAND_SYNC_READWRITE)) {
> > > -		printk(KERN_ERR "Onenand using only SYNC_READ on 24xx\n");
> > > +		dev_warn(dev, "OneNAND using only SYNC_READ on 24xx\n");
> > 
> > it would seem more natural to use dev_err() instead.
> > 
> 
> Are you sure? The error seems more a warning to me,
> although I guess it's arguable.
> 
> Let me know and I'll fix it in v2.

my bad, should've read the code more carefuly. Indeed it looks more like
a warning as we continue to try to use the IP. My bad.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130209/3e00d098/attachment.sig>