staging: bcm2835-audio: clean-up coding style issues

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 -3

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

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/43aa5452c7e19cdf5bb0c1046c4c11a82569083a.1489055580.git.aishpant%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

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 +2 -2

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;
 

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

 }
 
 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;
--

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/cc9cc8c7a3576a9d556815fd9f57ca58338abb01.1489055580.git.aishpant%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Quoted 35 lines · 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 +2 -2

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;
 

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance, · line 344

Quoted 5 lines

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

@@ -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);

I find it a bit odd to have -EPERM hard coded at the end of the function. If the function becomes more complicated, this code could end up being shared. In that case, different exits would have different codes. Also, ultimately the code should come from the result of vchi_service_open, although that is not suitable now.

How about adding status = -EPERM just before the goto, and the putting return ERR_PTR(status) at the end of the function?

By the way, I found that having all the LOG_DBG lines made the code really hard to follow. Maybe it could be possible to just remove them.

Another suggestion on this particular function is to do something about this declaration:

SERVICE_CREATION_T params = {
VCHI_VERSION_EX(VC_AUDIOSERV_VER, VC_AUDIOSERV_MIN_VER), VC_AUDIO_SERVER_NAME, // 4cc service code vchi_connections[i], // passed in fn pointers 0, // rx fifo size (unused)
0, // tx fifo size (unused) audio_vchi_callback, // service callback instance, // service callback parameter
1, //TODO: remove VCOS_FALSE, // unaligned bulk receives
1, //TODO: remove VCOS_FALSE, // unaligned bulk transmits
0 // want crc check on bulk transfers
};

The typedef needs to go, the fields should be initialied using their names (.x = e) and the comments should be dropped.

julia

Quoted 22 lines

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

 }
 
 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;
--

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/cc9cc8c7a3576a9d556815fd9f57ca58338abb01.1489055580.git.aishpant%40gmail.com.
For more options, visit https://groups.google.com/d/optout.

Quoted 51 lines · Julia Lawall

Quoted 35 lines · 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 +2 -2

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;
 

drivers/staging/vc04_services/bcm2835-audio/bcm2835-vchiq.c vc_vchi_audio_init(VCHI_INSTANCE_T vchi_instance, · line 344

Quoted 5 lines

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

@@ -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);

I find it a bit odd to have -EPERM hard coded at the end of the function. If the function becomes more complicated, this code could end up being shared. In that case, different exits would have different codes. Also, ultimately the code should come from the result of vchi_service_open, although that is not suitable now.

Quoted 2 lines

How about adding status = -EPERM just before the goto, and the putting return ERR_PTR(status) at the end of the function?

Yes that sounds fair.

Quoted 4 lines

By the way, I found that having all the LOG_DBG lines made the code really hard to follow. Maybe it could be possible to just remove them.

The LOG_DBG / LOG_ERR messages here are a wrapper of pr_err / pr_dbg appended with the current function names and line numbers. So I am taking a guess they are useful as long as the driver is in staging.

Quoted 16 lines

Another suggestion on this particular function is to do something about this declaration:

SERVICE_CREATION_T params = {
VCHI_VERSION_EX(VC_AUDIOSERV_VER, VC_AUDIOSERV_MIN_VER), VC_AUDIO_SERVER_NAME, // 4cc service code vchi_connections[i], // passed in fn pointers 0, // rx fifo size (unused)
0, // tx fifo size (unused) audio_vchi_callback, // service callback instance, // service callback parameter
1, //TODO: remove VCOS_FALSE, // unaligned bulk receives
1, //TODO: remove VCOS_FALSE, // unaligned bulk transmits
0 // want crc check on bulk transfers
};

Quoted 3 lines

The typedef needs to go, the fields should be initialied using their names (.x = e) and the comments should be dropped.

All-right I'll give this a go. The vchi_services driver is sprinkled with typedefs.

Quoted 18 lines

julia

Quoted 15 lines

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

 }
 
 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;
--

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/cc9cc8c7a3576a9d556815fd9f57ca58338abb01.1489055580.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/20170309183432.GA15683%40aishwarya.
For more options, visit https://groups.google.com/d/optout.