Patchwork mtd: dc21285.c: handle nw_gpio_lock correctly

login
register
mail settings
Submitter Christian Dietrich
Date May 25, 2012, 8:28 a.m.
Message ID <20120525082812.GA12238@faui49q.informatik.uni-erlangen.de>
Download mbox | patch
Permalink /patch/161254/
State New
Headers show

Comments

Christian Dietrich - May 25, 2012, 8:28 a.m.
nw_gpio_lock is a raw_spinlock_t, therefore raw_spin_lock_irqsave should be
used. it doesn't make a difference, while rlock is the first element of
spinlock_t.

Also the check for machine_is_netwinder() is a double check of
CONFIG_ARCH_NETWINDER.

Signed-off-by: Christian Dietrich <christian.dietrich@informatik.uni-erlangen.de>
---
 drivers/mtd/maps/dc21285.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)
Artem Bityutskiy - May 26, 2012, 1:54 p.m.
On Fri, 2012-05-25 at 10:28 +0200, Christian Dietrich wrote:
> nw_gpio_lock is a raw_spinlock_t, therefore raw_spin_lock_irqsave should be
> used. it doesn't make a difference, while rlock is the first element of
> spinlock_t.
> 
> Also the check for machine_is_netwinder() is a double check of
> CONFIG_ARCH_NETWINDER.
> 
> Signed-off-by: Christian Dietrich <christian.dietrich@informatik.uni-erlangen.de>

I do not understand this commit message. Please, split your patch on 2
patches and put a better to each one. Thanks!

> ---
>  drivers/mtd/maps/dc21285.c |   13 +++++--------
>  1 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mtd/maps/dc21285.c b/drivers/mtd/maps/dc21285.c
> index 080f060..38fbf23 100644
> --- a/drivers/mtd/maps/dc21285.c
> +++ b/drivers/mtd/maps/dc21285.c
> @@ -38,9 +38,9 @@ static void nw_en_write(void)
>  	 * we want to write a bit pattern XXX1 to Xilinx to enable
>  	 * the write gate, which will be open for about the next 2ms.
>  	 */
> -	spin_lock_irqsave(&nw_gpio_lock, flags);
> +	raw_spin_lock_irqsave(&nw_gpio_lock, flags);

Please, make this to be a separate patch.

And surely there are many other places in the kernel which need this
conversion?

>  static void dc21285_write8(struct map_info *map, const map_word d, unsigned long adr)
>  {
> -	if (machine_is_netwinder())
> -		nw_en_write();
> +	nw_en_write();
>  	*CSR_ROMWRITEREG = adr & 3;

I do not understand why this "if" statement is not needed? Why it was
there in the first place and why you remove it? Please, describe this in
the commit message.

Patch

diff --git a/drivers/mtd/maps/dc21285.c b/drivers/mtd/maps/dc21285.c
index 080f060..38fbf23 100644
--- a/drivers/mtd/maps/dc21285.c
+++ b/drivers/mtd/maps/dc21285.c
@@ -38,9 +38,9 @@  static void nw_en_write(void)
 	 * we want to write a bit pattern XXX1 to Xilinx to enable
 	 * the write gate, which will be open for about the next 2ms.
 	 */
-	spin_lock_irqsave(&nw_gpio_lock, flags);
+	raw_spin_lock_irqsave(&nw_gpio_lock, flags);
 	nw_cpld_modify(CPLD_FLASH_WR_ENABLE, CPLD_FLASH_WR_ENABLE);
-	spin_unlock_irqrestore(&nw_gpio_lock, flags);
+	raw_spin_unlock_irqrestore(&nw_gpio_lock, flags);
 
 	/*
 	 * let the ISA bus to catch on...
@@ -79,8 +79,7 @@  static void dc21285_copy_from(struct map_info *map, void *to, unsigned long from
 
 static void dc21285_write8(struct map_info *map, const map_word d, unsigned long adr)
 {
-	if (machine_is_netwinder())
-		nw_en_write();
+	nw_en_write();
 	*CSR_ROMWRITEREG = adr & 3;
 	adr &= ~3;
 	*(uint8_t*)(map->virt + adr) = d.x[0];
@@ -88,8 +87,7 @@  static void dc21285_write8(struct map_info *map, const map_word d, unsigned long
 
 static void dc21285_write16(struct map_info *map, const map_word d, unsigned long adr)
 {
-	if (machine_is_netwinder())
-		nw_en_write();
+	nw_en_write();
 	*CSR_ROMWRITEREG = adr & 3;
 	adr &= ~3;
 	*(uint16_t*)(map->virt + adr) = d.x[0];
@@ -97,8 +95,7 @@  static void dc21285_write16(struct map_info *map, const map_word d, unsigned lon
 
 static void dc21285_write32(struct map_info *map, const map_word d, unsigned long adr)
 {
-	if (machine_is_netwinder())
-		nw_en_write();
+	nw_en_write();
 	*(uint32_t*)(map->virt + adr) = d.x[0];
 }