staging: bcm2835-audio: fix coding style issues

PATCH Superseded Aishwarya Pant 2 participants 8 messages Mar 6, 2017 – Mar 7, 2017

Subsystems (3)

Staging Broadcom BCM2711/BCM2835 Arm architecture Broadcom BCM281XX/BCM11XXX/BCM216XX Arm architecture

lore

Share & tools

lore

Aishwarya Pant

author

2017-03-06 19:46 UTC #0 lore raw

v2 cover staging: bcm2835-audio: fix coding style issues

This patchset makes the following changes:
- Replace kmalloc and memset with kzalloc
- Replace null return value with PTR_ERR values
- Propagate the PTR_ERR values forward instead of a hardcoded value for easier debugging
- Replace if (success) else { } after kmalloc with if (error) to fail fast.

Changes in v2:
- Return error value -EBUSY instead of -1 when queue_work() fails

Aishwarya Pant (4):
staging: bcm2835-audio: Replace kmalloc with kzalloc
staging: bcm2835-audio: replace null with error pointer value
staging: bcm2835-audio: propagate PTR_ERR value instead of -EPERM
staging: bcm2835-audio: use conditional only for error case

 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 83 +++++++++++-----------
 1 file changed, 41 insertions(+), 42 deletions(-)

Signature · 1 line

2.7.4

Aishwarya Pant

author

2017-03-06 19:46 UTC #1 lore raw

PATCH v2 1/4 staging: bcm2835-audio: Replace kmalloc with kzalloc

Replace kmalloc and memset with kzalloc.

Signed-off-by Aishwarya Pant <[email protected]>

---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -2

index c54bef3..005e287 100644
@@ -290,11 +290,10 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
 		return NULL;
 	}
 	/* Allocate memory for this instance */
-	instance = kmalloc(sizeof(*instance), GFP_KERNEL);
+	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
 	if (!instance)
 		return NULL;
 
-	memset(instance, 0, sizeof(*instance));
 	instance->num_connections = num_connections;
 
 	/* Create a lock for exclusive, serialized VCHI connection access */

Signature · 1 line

2.7.4

Aishwarya Pant

author

2017-03-06 19:46 UTC #2 lore raw

PATCH v2 2/4 staging: bcm2835-audio: replace null with error pointer value

In reply to Aishwarya Pant

This patch replaces NULL values returned by vc_vchi_audio_init(...) with error pointer values:
- Return ERR_PTR(-EINVAL) when too many instances of audio service are initialised
- Return ERR_PTR(-ENOMEM) when kzalloc fails
- RETURN ERR_PTR(-EPERM) when vchi connections fail to open

Similarly, a NULL check where vc_vchi_audio_init(...) is called is replaced by IS_ERR(..)

Signed-off-by Aishwarya Pant <[email protected]>

---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +4 -4

index 005e287..d2686b4 100644
@@ -287,12 +287,12 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
 		LOG_ERR("%s: unsupported number of connections %u (max=%u)\n",
 			__func__, num_connections, VCHI_MAX_NUM_CONNECTIONS);
 
-		return NULL;
+		return ERR_PTR(-EINVAL);
 	}
 	/* Allocate memory for this instance */
 	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
 	if (!instance)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	instance->num_connections = num_connections;

@@ -341,7 +341,7 @@ vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance,
 	kfree(instance);
 	LOG_ERR("%s: error\n", __func__);
 
-	return NULL;
+	return ERR_PTR(-EPERM);
 }
 
 static int vc_vchi_audio_deinit(struct bcm2835_audio_instance *instance)

@@ -432,7 +432,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 	/* Initialize an instance of the audio service */
 	instance = vc_vchi_audio_init(vchi_instance, &vchi_connection, 1);
 
-	if (!instance) {
+	if (IS_ERR(instance)) {
 		LOG_ERR("%s: failed to initialize audio service\n", __func__);
 
 		ret = -EPERM;

Signature · 1 line

2.7.4

Aishwarya Pant

author

2017-03-06 19:46 UTC #3 lore raw

PATCH v2 3/4 staging: bcm2835-audio: propagate PTR_ERR value instead of -EPERM

In reply to Aishwarya Pant

It is better to propagate PTR_ERR value instead of a hardcoded value (-EPERM here)

Signed-off-by Aishwarya Pant <[email protected]>

---
 drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +1 -1

index d2686b4..c5f3be8 100644
@@ -435,7 +435,7 @@ static int bcm2835_audio_open_connection(struct bcm2835_alsa_stream *alsa_stream
 	if (IS_ERR(instance)) {
 		LOG_ERR("%s: failed to initialize audio service\n", __func__);
 
-		ret = -EPERM;
+		ret = PTR_ERR(instance);
 		goto err_free_mem;
 	}

Signature · 1 line

2.7.4

Aishwarya Pant

author

2017-03-06 19:47 UTC #4 lore raw

PATCH v2 4/4 staging: bcm2835-audio: use conditional only for error case

In reply to Aishwarya Pant

This patch refactors conditional to check if memory allocation has failed and immediately returns (-ENOMEM); the redundant if block for success case is removed. The work functions now return the error value -EBUSY instead of -1 when queue_work() fails.

Signed-off-by Aishwarya Pant <[email protected]>

---
Changes in v2:
- Return error value -EBUSY when queue_work fails

 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 70 +++++++++++-----------
 1 file changed, 35 insertions(+), 35 deletions(-)

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +35 ~~-35~~

index c5f3be8..08f200f 100644
@@ -131,77 +131,77 @@ static void my_wq_function(struct work_struct *work)
 
 int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_START;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_START;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_STOP;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_STOP;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 			unsigned int count, void *src)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_WRITE;
-			work->src = src;
-			work->count = count;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_WRITE;
+		work->src = src;
+		work->count = count;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 static void my_workqueue_init(struct bcm2835_alsa_stream *alsa_stream)

Signature · 1 line

2.7.4

Julia Lawall

2017-03-06 21:41 UTC #5 lore raw

Re: [Outreachy kernel] [PATCH v2 4/4] staging: bcm2835-audio: use conditional only for error case

Quoted 10 lines · Aishwarya Pant

This patch refactors conditional to check if memory allocation has failed and immediately returns (-ENOMEM); the redundant if block for success case is removed. The work functions now return the error value -EBUSY instead of -1 when queue_work() fails.
Signed-off-by Aishwarya Pant <[email protected]>
---
Changes in v2:
- Return error value -EBUSY when queue_work fails

This is definitely an improvement. Some opportunities for further improvement remain:

queue_work seems to fail in a very particular situation, which is mentioned in the definition of the function. It could be good to make the error message more spcific.

The code causes a memory leak if queue_work returns false.

The IN and OUT debugging messages are probably not so useful. The don't even say what function one is in or out of, and if that information is needed, it could be just added to the error messages in the failure cases (__func__).

julia

Quoted 130 lines

 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 70 +++++++++++-----------
 1 file changed, 35 insertions(+), 35 deletions(-)

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +35 ~~-36~~

index c5f3be8..08f200f 100644
@@ -131,77 +131,77 @@ static void my_wq_function(struct work_struct *work)
 
 int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_START;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_START;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_STOP;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_STOP;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 			unsigned int count, void *src)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_WRITE;
-			work->src = src;
-			work->count = count;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_WRITE;
+		work->src = src;
+		work->count = count;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 static void my_workqueue_init(struct bcm2835_alsa_stream *alsa_stream)
--

2.7.4

--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/b013f557b3bff6e4264bc9e6d1f7e502a8d1ac8c.1488829304.git.aishpant%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Aishwarya Pant

author

2017-03-07 05:50 UTC #6 lore raw

Re: [Outreachy kernel] [PATCH v2 4/4] staging: bcm2835-audio: use conditional only for error case

Quoted 15 lines · Julia Lawall

Quoted 10 lines · Aishwarya Pant
This patch refactors conditional to check if memory allocation has failed and immediately returns (-ENOMEM); the redundant if block for success case is removed. The work functions now return the error value -EBUSY instead of -1 when queue_work() fails.
Signed-off-by Aishwarya Pant <[email protected]>
---
Changes in v2:
- Return error value -EBUSY when queue_work fails

Quoted 6 lines

This is definitely an improvement. Some opportunities for further improvement remain:
queue_work seems to fail in a very particular situation, which is mentioned in the definition of the function. It could be good to make the error message more spcific.

Sure, will add error messages like failed to start/stop audio, write audio bytes etc.

Quoted 2 lines

The code causes a memory leak if queue_work returns false.

Yes. kfree(work) needs to be done when queue_work is busy. One thing I noticed is that in the same file kfree((void *)work) has been used. Both kfree(work) and kfree((void *)work) are valid as long as work is not defined as const. kfree looks up the address and the memory actually keeps track of the size of allocation done by kmalloc, and that is how kfree knows how much to deallocate.

Quoted 5 lines

The IN and OUT debugging messages are probably not so useful. The don't even say what function one is in or out of, and if that information is needed, it could be just added to the error messages in the failure cases (__func__).

The IN and OUT debug messages are present in every function of the file. I'll keep them and add the current function name __func__

Quoted 133 lines

julia

Quoted 130 lines

 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 70 +++++++++++-----------
 1 file changed, 35 insertions(+), 35 deletions(-)

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +35 ~~-36~~

index c5f3be8..08f200f 100644
@@ -131,77 +131,77 @@ static void my_wq_function(struct work_struct *work)
 
 int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_START;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_START;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_STOP;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_STOP;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 			unsigned int count, void *src)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_WRITE;
-			work->src = src;
-			work->count = count;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_WRITE;
+		work->src = src;
+		work->count = count;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 static void my_workqueue_init(struct bcm2835_alsa_stream *alsa_stream)
--

2.7.4

Julia Lawall

2017-03-07 06:31 UTC #7 lore raw

Re: [Outreachy kernel] [PATCH v2 4/4] staging: bcm2835-audio: use conditional only for error case

Quoted 36 lines · Aishwarya Pant

Quoted 15 lines · Julia Lawall
Quoted 10 lines · Aishwarya Pant
This patch refactors conditional to check if memory allocation has failed and immediately returns (-ENOMEM); the redundant if block for success case is removed. The work functions now return the error value -EBUSY instead of -1 when queue_work() fails.
Signed-off-by Aishwarya Pant <[email protected]>
---
Changes in v2:
- Return error value -EBUSY when queue_work fails
Quoted 6 lines
This is definitely an improvement. Some opportunities for further improvement remain:
queue_work seems to fail in a very particular situation, which is mentioned in the definition of the function. It could be good to make the error message more spcific.
Sure, will add error messages like failed to start/stop audio, write audio bytes etc.
Quoted 2 lines
The code causes a memory leak if queue_work returns false.
Yes. kfree(work) needs to be done when queue_work is busy. One thing I noticed is that in the same file kfree((void *)work) has been used. Both kfree(work) and kfree((void *)work) are valid as long as work is not defined as const. kfree looks up the address and the memory actually keeps track of the size of allocation done by kmalloc, and that is how kfree knows how much to deallocate.
Quoted 1 line

Quoted 7 lines

Quoted 4 lines
The IN and OUT debugging messages are probably not so useful. The don't even say what function one is in or out of, and if that information is needed, it could be just added to the error messages in the failure cases (__func__).
The IN and OUT debug messages are present in every function of the file. I'll keep them and add the current function name __func__

Probably not worth it. They should probably all go, but you may want to leave that for someone else to do.

julia

Quoted 142 lines

Quoted 127 lines

julia

Quoted 123 lines

 .../vc04_services/bcm2835-audio/bcm2835-vchiq.c    | 70 +++++++++++-----------
 1 file changed, 35 insertions(+), 35 deletions(-)

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c +35 ~~-36~~

index c5f3be8..08f200f 100644
@@ -131,77 +131,77 @@ static void my_wq_function(struct work_struct *work)
 
 int bcm2835_audio_start(struct bcm2835_alsa_stream *alsa_stream)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_START;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_START;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 int bcm2835_audio_stop(struct bcm2835_alsa_stream *alsa_stream)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_STOP;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_STOP;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 int bcm2835_audio_write(struct bcm2835_alsa_stream *alsa_stream,
 			unsigned int count, void *src)
 {
-	int ret = -1;
-
 	LOG_DBG(" .. IN\n");
 	if (alsa_stream->my_wq) {
 		struct bcm2835_audio_work *work;
 
 		work = kmalloc(sizeof(*work), GFP_ATOMIC);
 		/*--- Queue some work (item 1) ---*/
-		if (work) {
-			INIT_WORK(&work->my_work, my_wq_function);
-			work->alsa_stream = alsa_stream;
-			work->cmd = BCM2835_AUDIO_WRITE;
-			work->src = src;
-			work->count = count;
-			if (queue_work(alsa_stream->my_wq, &work->my_work))
-				ret = 0;
-		} else {
+		if (!work) {
 			LOG_ERR(" .. Error: NULL work kmalloc\n");
+			return -ENOMEM;
+		}
+		INIT_WORK(&work->my_work, my_wq_function);
+		work->alsa_stream = alsa_stream;
+		work->cmd = BCM2835_AUDIO_WRITE;
+		work->src = src;
+		work->count = count;
+		if (!queue_work(alsa_stream->my_wq, &work->my_work)) {
+			LOG_ERR(" .. Error: Unable to queue work\n");
+			return -EBUSY;
 		}
 	}
-	LOG_DBG(" .. OUT %d\n", ret);
-	return ret;
+	LOG_DBG(" .. OUT\n");
+	return 0;
 }
 
 static void my_workqueue_init(struct bcm2835_alsa_stream *alsa_stream)
--

2.7.4

Quoted 7 lines

Quoted 7 lines
--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/b013f557b3bff6e4264bc9e6d1f7e502a8d1ac8c.1488829304.git.aishpant%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170307055034.GA30995%40aishwarya.
For more options, visit https://groups.google.com/d/optout.