Skip to contentSkip to conversation

leds: blinkm: Don't init a workqueue per brightness_set call

PATCH In review Axel Lin 2 participants 2 messages Oct 29, 2012 – Oct 31, 2012 1 message not in the archive

Subsystems (2)
LED Blinkm RGB LED driver
lore
Share & tools
lore

Axel Lin

author
#0 lore raw

PATCH 1/1 leds: blinkm: Don't init a workqueue per brightness_set call

Calling INIT_WORK in blinkm_led_common_set() means we init a workqueue every time when brightness_set callback is called.
Move INIT_WORK to blinkm_probe() so we only need to init the workqueue once.

So we only need to init a workqueue per blinkm led rather than init a workqueue per brightness_set call.

Signed-off-by Axel Lin <[email protected]>

---
This patch is v2 of "[PATCH RFT] leds: blinkm: Avoid calling INIT_WORK in blinkm_led_common_set()" I think the new subject line is better for the reason I sent the patch.

v2:
Now we have a workqueue per blinkm led.
I think the code can be simpler and has better readability if we embeded struct work_struct in struct blinkm_led.

Note I don't have this hardware, so I added Request-For-Test (RFT) in the subject line. Axel

 drivers/leds/leds-blinkm.c |   30 +++++++-----------------------
 1 file changed, 7 insertions(+), 23 deletions(-)
drivers/leds/leds-blinkm.c +7 -23
index f7c3d7f..b62d214 100644
@@ -37,19 +37,15 @@ static int blinkm_transfer_hw(struct i2c_client *client, int cmd);
 static int blinkm_test_run(struct i2c_client *client);
 
 struct blinkm_led {
+	struct work_struct work;
 	struct i2c_client *i2c_client;
 	struct led_classdev led_cdev;
 	int id;
 	atomic_t active;
 };
 
-struct blinkm_work {
-	struct blinkm_led *blinkm_led;
-	struct work_struct work;
-};
-
 #define cdev_to_blmled(c)          container_of(c, struct blinkm_led, led_cdev)
-#define work_to_blmwork(c)         container_of(c, struct blinkm_work, work)
+#define work_to_blmled(c)         container_of(c, struct blinkm_led, work)
 
 struct blinkm_data {
 	struct i2c_client *i2c_client;

@@ -446,12 +442,9 @@ static int blinkm_transfer_hw(struct i2c_client *client, int cmd)
 static void led_work(struct work_struct *work)
 {
 	int ret;
-	struct blinkm_led *led;
-	struct blinkm_data *data ;
-	struct blinkm_work *blm_work = work_to_blmwork(work);
+	struct blinkm_led *led = work_to_blmled(work);
+	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);
 
-	led = blm_work->blinkm_led;
-	data = i2c_get_clientdata(led->i2c_client);
 	ret = blinkm_transfer_hw(led->i2c_client, BLM_GO_RGB);
 	atomic_dec(&led->active);
 	dev_dbg(&led->i2c_client->dev,

@@ -459,7 +452,6 @@ static void led_work(struct work_struct *work)
 			" next_blue = %d, active = %d\n",
 			data->next_red, data->next_green,
 			data->next_blue, atomic_read(&led->active));
-	kfree(blm_work);
 }
 
 static int blinkm_led_common_set(struct led_classdev *led_cdev,

@@ -468,7 +460,6 @@ static int blinkm_led_common_set(struct led_classdev *led_cdev,
 	/* led_brightness is 0, 127 or 255 - we just use it here as-is */
 	struct blinkm_led *led = cdev_to_blmled(led_cdev);
 	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);
-	struct blinkm_work *bl_work;
 
 	switch (color) {
 	case RED:

@@ -510,10 +501,6 @@ static int blinkm_led_common_set(struct led_classdev *led_cdev,
 		return -EINVAL;
 	}
 
-	bl_work = kzalloc(sizeof(*bl_work), GFP_ATOMIC);
-	if (!bl_work)
-		return -ENOMEM;
-
 	atomic_inc(&led->active);
 	dev_dbg(&led->i2c_client->dev,
 			"#TO_SCHED# next_red = %d, next_green = %d,"

@@ -521,11 +508,8 @@ static int blinkm_led_common_set(struct led_classdev *led_cdev,
 			data->next_red, data->next_green,
 			data->next_blue, atomic_read(&led->active));
 
-	/* a fresh work _item_ for each change */
-	bl_work->blinkm_led = led;
-	INIT_WORK(&bl_work->work, led_work);
 	/* queue work in own queue for easy sync on exit*/
-	schedule_work(&bl_work->work);
+	schedule_work(&led->work);
 
 	return 0;
 }

@@ -667,6 +651,7 @@ static int __devinit blinkm_probe(struct i2c_client *client,
 	}
 
 	for (i = 0; i < 3; i++) {
+		INIT_WORK(&data->blinkm_leds[i].work, led_work);
 		/* RED = 0, GREEN = 1, BLUE = 2 */
 		led[i] = &data->blinkm_leds[i];
 		led[i]->i2c_client = client;

@@ -746,8 +731,7 @@ exit:
 static int __devexit blinkm_remove(struct i2c_client *client)
 {
 	struct blinkm_data *data = i2c_get_clientdata(client);
-	int ret = 0;
-	int i;
+	int i, ret;
 
 	/* make sure no workqueue entries are pending */
 	for (i = 0; i < 3; i++) {
Signature · 1 line
1.7.9.5

A message that is not in the archive (sent off-list or not yet received): CAFRkauDWf8y24DgoS1wE2ZvG4QtP6sYKbMatHmKjNvR_aFWtuA@mail.gmail.com

#2 lore raw
Quoted 22 lines · Axel Lin

2012/10/29 Axel Lin <[email protected]>

Quoted 10 lines

Calling INIT_WORK in blinkm_led_common_set() means we init a workqueue every time
when brightness_set callback is called.
Move INIT_WORK to blinkm_probe() so we only need to init the workqueue once.

So we only need to init a workqueue per blinkm led rather than init a workqueue
per brightness_set call.

Ah. the commit log and subject line is wrong.
We are using the global workqueue.
What the patch does is to avoid init work rather than workqueue per brightness_set call.
We just need to call INIT_WORK once for the first time that structure is set up.

Quoted 4 lines

There should be no functional change with this patch.

Regards,
Axel

Right, I think I did same review when this driver posted on the list firstly. Probably it is a hardware issue. So let Jan-Simon give us some updates.

If the hardware requires this specific handling, I suggest we add some comments or documents to clarify that.

Thanks,
-Bryan