Skip to contentSkip to conversation

ARM: OMAP: ISP1301 workqueue fixes

PATCH In review Dirk Behme 4 participants 7 messages Dec 28, 2006 – Jan 2, 2007

Subsystems (3)
Arm (32-bit) Arm sub-architectures OMAP1 support
lore
Share & tools
lore

Dirk Behme

author
#0 lore raw

PATCH 1/1 ARM: OMAP: ISP1301 workqueue fixes

ARM: OMAP: ISP1301 workqueue fixes.

Signed-off-by Dirk Behme <dirk.behme_at_gmail.com>

ARM: OMAP: ISP1301 workqueue fixes.

Signed-off-by Dirk Behme <dirk.behme_at_gmail.com>
drivers/i2c/chips/isp1301_omap.c +3 -3
Index: linux-osk/drivers/i2c/chips/isp1301_omap.c
===================================================================
@@ -1119,9 +1119,9 @@ static u8 isp1301_clear_latch(struct isp
 }
 
 static void
-isp1301_work(void *data)
+isp1301_work(struct work_struct *data)
 {
-	struct isp1301	*isp = data;
+	struct isp1301	*isp = (struct isp1301 *)data;
 	int		stop;
 
 	/* implicit lock:  we're the only task using this device */

@@ -1525,7 +1525,7 @@ static int isp1301_probe(struct i2c_adap
 	if (!isp)
 		return 0;
 
-	INIT_WORK(&isp->work, isp1301_work, isp);
+	INIT_WORK(&isp->work, isp1301_work);
 	init_timer(&isp->timer);
 	isp->timer.function = isp1301_timer;
 	isp->timer.data = (unsigned long) isp;
#1 lore raw
Quoted 5 lines · Dirk Behme

ARM: OMAP: ISP1301 workqueue fixes.

Signed-off-by Dirk Behme <dirk.behme_at_gmail.com>

Why not @. ? No obfuscation please.

Quoted 11 lines

ARM: OMAP: ISP1301 workqueue fixes.

Signed-off-by Dirk Behme <dirk.behme_at_gmail.com>

Index: linux-osk/drivers/i2c/chips/isp1301_omap.c
===================================================================

drivers/i2c/chips/isp1301_omap.c static u8 isp1301_clear_latch(struct isp · line 1124

Quoted 8 lines
drivers/i2c/chips/isp1301_omap.c +2 -2
@@ -1119,9 +1119,9 @@ static u8 isp1301_clear_latch(struct isp
 }
 
 static void
-isp1301_work(void *data)
+isp1301_work(struct work_struct *data)
 {
-       struct isp1301  *isp = data;
+       struct isp1301  *isp = (struct isp1301 *)data;

Why not use container_of ? Did you tested it?

Quoted 6 lines
drivers/i2c/chips/isp1301_omap.c +0 -0
        int             stop;
 
        /* implicit lock:  we're the only task using this device */

drivers/i2c/chips/isp1301_omap.c static int isp1301_probe(struct i2c_adap · line 1531

Quoted 8 lines
drivers/i2c/chips/isp1301_omap.c +1 -1
@@ -1525,7 +1525,7 @@ static int isp1301_probe(struct i2c_adap
        if (!isp)
                return 0;
 
-       INIT_WORK(&isp->work, isp1301_work, isp);
+       INIT_WORK(&isp->work, isp1301_work);
        init_timer(&isp->timer);
        isp->timer.function = isp1301_timer;
        isp->timer.data = (unsigned long) isp;

And I think above should be treated as INIT_DELAYED_WORK instead.

Signature · 2 lines
---Komal Shah
http://komalshah.blogspot.com
author
#2 lore raw
Quoted 1 line · Komal Shah

On 12/28/06, Dirk Behme <[email protected]> wrote:

...

Quoted 7 lines
Quoted 7 lines

Index: linux-osk/drivers/i2c/chips/isp1301_omap.c
===================================================================

drivers/i2c/chips/isp1301_omap.c +0 -0
@@ -1119,9 +1119,9 @@ static u8 isp1301_clear_latch(struct isp
 }
Quoted 9 lines

drivers/i2c/chips/isp1301_omap.c static u8 isp1301_clear_latch(struct isp · line 1124

Quoted 6 lines
drivers/i2c/chips/isp1301_omap.c +2 -2
 static void
-isp1301_work(void *data)
+isp1301_work(struct work_struct *data)
 {
-       struct isp1301  *isp = data;
+       struct isp1301  *isp = (struct isp1301 *)data;

Why not use container_of ? Did you tested it?

Can anybody with H2/H3 board test this? Same with board-h3 workqueue fix from Ragner.

Many thanks

Dirk

#3 lore raw
Quoted 20 lines · Dirk Behme
Quoted 1 line · Komal Shah

On 12/28/06, Dirk Behme <[email protected]> wrote:

...

Quoted 7 lines
Quoted 7 lines

Index: linux-osk/drivers/i2c/chips/isp1301_omap.c
===================================================================

drivers/i2c/chips/isp1301_omap.c +0 -0
@@ -1119,9 +1119,9 @@ static u8 isp1301_clear_latch(struct isp
 }
Quoted 9 lines

drivers/i2c/chips/isp1301_omap.c static u8 isp1301_clear_latch(struct isp · line 1124

Quoted 6 lines
drivers/i2c/chips/isp1301_omap.c +2 -2
 static void
-isp1301_work(void *data)
+isp1301_work(struct work_struct *data)
 {
-       struct isp1301  *isp = data;
+       struct isp1301  *isp = (struct isp1301 *)data;

Why not use container_of ? Did you tested it?

Quoted 1 line

Can anybody with H2/H3 board test this?

Not worth testing, it's clearly incorrect.

#4 lore raw
In reply to Dirk Behme
Quoted 7 lines · Dirk Behme

 static void

drivers/i2c/chips/isp1301_omap.c +1 -1
-isp1301_work(void *data)
+isp1301_work(struct work_struct *data)

 {
-       struct isp1301  *isp = data;
+       struct isp1301  *isp = (struct isp1301 *)data; int             stop;

Clearly incorrect. It should use container_of(). The work_struct is NOT the first member of that struct... this will be oopsing.

author
#5 lore raw
Quoted 13 lines · David Brownell

Review

Quoted 7 lines · Dirk Behme

static void

(quoted diff) +1 -1
-isp1301_work(void *data)
+isp1301_work(struct work_struct *data)

{

(quoted diff) +1 -1
-       struct isp1301  *isp = data;
+       struct isp1301  *isp = (struct isp1301 *)data;
       int             stop;

Clearly incorrect. It should use container_of(). The work_struct is NOT the first member of that struct... this will be oopsing.

Yes, I see. Thanks!

So, next attempt ;) Below both, ISP1301 and board-h3. Again, only checked for compilation.

Dirk

drivers/i2c/chips/isp1301_omap.c +3 -3
Index: linux-osk/drivers/i2c/chips/isp1301_omap.c
===================================================================
@@ -1119,9 +1119,9 @@ static u8 isp1301_clear_latch(struct isp
 }
 
 static void
-isp1301_work(void *data)
+isp1301_work(struct work_struct *work)
 {
-	struct isp1301	*isp = data;
+	struct isp1301	*isp = container_of(work, struct isp1301, work);
 	int		stop;
 
 	/* implicit lock:  we're the only task using this device */

@@ -1525,7 +1525,7 @@ static int isp1301_probe(struct i2c_adap
 	if (!isp)
 		return 0;
 
-	INIT_WORK(&isp->work, isp1301_work, isp);
+	INIT_WORK(&isp->work, isp1301_work);
 	init_timer(&isp->timer);
 	isp->timer.function = isp1301_timer;
 	isp->timer.data = (unsigned long) isp;
arch/arm/mach-omap1/board-h3.c +7 -5
Index: linux-osk/arch/arm/mach-omap1/board-h3.c
===================================================================
@@ -296,9 +296,11 @@ static int h3_select_irda(struct device 
 	return err;
 }
 
-static void set_trans_mode(void *data)
+static void set_trans_mode(struct work_struct *work)
 {
-	int *mode = data;
+	struct omap_irda_config *irda_config =
+		container_of(work, struct omap_irda_config, gpio_expa.work);
+	int mode = irda_config->mode;
 	unsigned char expa;
 	int err = 0;
 

@@ -308,7 +310,7 @@ static void set_trans_mode(void *data)
 
 	expa &= ~0x03;
 
-	if (*mode & IR_SIRMODE) {
+	if (mode & IR_SIRMODE) {
 		expa |= 0x01;
 	} else { /* MIR/FIR */
 		expa |= 0x03;

@@ -323,9 +325,9 @@ static int h3_transceiver_mode(struct de
 {
 	struct omap_irda_config *irda_config = dev->platform_data;
 
+	irda_config->mode = mode;
 	cancel_delayed_work(&irda_config->gpio_expa);
-	PREPARE_WORK(&irda_config->gpio_expa, set_trans_mode, &mode);
-#error this is not permitted - mode is an argument variable
+	PREPARE_DELAYED_WORK(&irda_config->gpio_expa, set_trans_mode);
 	schedule_delayed_work(&irda_config->gpio_expa, 0);
 
 	return 0;
include/asm-arm/arch-omap/irda.h +1 -0
Index: linux-osk/include/asm-arm/arch-omap/irda.h
===================================================================
@@ -31,6 +31,7 @@ struct omap_irda_config {
 	unsigned long src_start;
 	int tx_trigger;
 	int rx_trigger;
+	int mode;
 };
 
 #endif
reviewer
#6 lore raw
Quoted 15 lines · Dirk Behme
Quoted 13 lines · David Brownell

drivers/i2c/chips/isp1301_omap.c static u8 isp1301_clear_latch(struct isp · line 1125

Quoted 7 lines · Dirk Behme

static void

(quoted diff) +1 -1
-isp1301_work(void *data)
+isp1301_work(struct work_struct *data)

{

drivers/i2c/chips/isp1301_omap.c +1 -1
-       struct isp1301  *isp = data;
+       struct isp1301  *isp = (struct isp1301 *)data;
      int             stop;

Clearly incorrect. It should use container_of(). The work_struct is NOT the first member of that struct... this will be oopsing.

Quoted 4 lines

Yes, I see. Thanks!

So, next attempt ;) Below both, ISP1301 and board-h3. Again, only checked for compilation.

Pushing as two patches.

Tony