[PATCH 0/9] staging: vc04_services: Smatch fixes and remove custom logging

[PATCH 0/9] staging: vc04_services: Smatch fixes and remove custom logging

[Umang Jain]

Patch 1/9 to 3/9 targets the warnings exposed by Smatch analysis.
Smatch fixes was sent earlier under "staging: vc04: Smatch analysis
fixes", but now I have patches on top of them, I have tied
these fixes in this series (so could be merged all at once).

This series also removes the vchiq_log_* macro and makes use of dev_dbg()
directly.

4/9 is just a drive-by patch to shorten function helper.
5/9 is bug fixup where NULL was passed instead of a struct device
pointer

6/9 to 7/9 removes each of the vchiq_log_* respectively.

This completes the following TODO item:

```
* Cleanup logging mechanism

The driver should probably be using the standard kernel logging mechanisms
such as dev_info, dev_dbg, and friends.
```

Umang Jain (9):
  staging: vc04_services: vchiq_core: Log through struct vchiq_instance
  staging: vc04_services: Log using pr_err() when vchiq_state is unset
  staging: vc04_services: bcm2835-camera: Remove redundant null check
  staging: vc04_services: Shorten helper function name
  staging: vc04_services: Do not pass NULL to vchiq_log_error()
  staging: vc04_services: Drop vchiq_log_error() in favour of dev_dbg
  staging: vc04_services: Drop vchiq_log_warning() in favour of dev_dbg
  staging: vc04_services: Drop vchiq_log_trace() in favour of dev_dbg
  staging: vc04_services: Drop vchiq_log_debug() in favour of dev_dbg

 .../bcm2835-camera/bcm2835-camera.c           |   7 +-
 drivers/staging/vc04_services/interface/TODO  |   5 -
 .../interface/vchiq_arm/vchiq_arm.c           | 223 +++++----
 .../interface/vchiq_arm/vchiq_connected.c     |   8 +-
 .../interface/vchiq_arm/vchiq_connected.h     |   4 +-
 .../interface/vchiq_arm/vchiq_core.c          | 463 ++++++++++--------
 .../interface/vchiq_arm/vchiq_core.h          |  36 +-
 .../interface/vchiq_arm/vchiq_dev.c           | 112 +++--
 8 files changed, 461 insertions(+), 397 deletions(-)


base-commit: d2f51b3516dade79269ff45eae2a7668ae711b25
-- 
2.41.0

[PATCH 1/9] staging: vc04_services: vchiq_core: Log through struct vchiq_instance

[Umang Jain]

The handle_to_service() helper can return NULL, so `service` pointer
can indeed be set to NULL. So, do not log through service pointer
(which will cause NULL-deference), instead, use the vchiq_instance
function argument to get access to the struct device.

Fixes: f67af5940d6d("staging: vc04: Convert(and rename) vchiq_log_info() to use dynamic debug")
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Ricardo B. Marliere <ricardo@marliere.net>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c  | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 39b857da2d42..8a9eb0101c2e 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -245,7 +245,7 @@ find_service_by_handle(struct vchiq_instance *instance, unsigned int handle)
 		return service;
 	}
 	rcu_read_unlock();
-	vchiq_log_debug(service->state->dev, VCHIQ_CORE,
+	vchiq_log_debug(instance->state->dev, VCHIQ_CORE,
 			"Invalid service handle 0x%x", handle);
 	return NULL;
 }
@@ -287,7 +287,7 @@ find_service_for_instance(struct vchiq_instance *instance, unsigned int handle)
 		return service;
 	}
 	rcu_read_unlock();
-	vchiq_log_debug(service->state->dev, VCHIQ_CORE,
+	vchiq_log_debug(instance->state->dev, VCHIQ_CORE,
 			"Invalid service handle 0x%x", handle);
 	return NULL;
 }
@@ -310,7 +310,7 @@ find_closed_service_for_instance(struct vchiq_instance *instance, unsigned int h
 		return service;
 	}
 	rcu_read_unlock();
-	vchiq_log_debug(service->state->dev, VCHIQ_CORE,
+	vchiq_log_debug(instance->state->dev, VCHIQ_CORE,
 			"Invalid service handle 0x%x", handle);
 	return service;
 }
-- 
2.41.0

[PATCH 2/9] staging: vc04_services: Log using pr_err() when vchiq_state is unset

[Umang Jain]

In cases, where the global vchiq state is still unset, we cannot log
to dynamic debug (access to struct device is needed, hence potential
NULL de-reference). Log using pr_err() instead.

In vchiq_initialise(), remove the 'goto' because it is just again
trying to log to dynamic debug. Simply return with -ENNOTCONN after
logging to pr_err().

In vchiq_open(), move the vchiq_log_debug() after the state pointer
null check.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Ricardo B. Marliere <ricardo@marliere.net>
---
 .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c  | 6 ++----
 .../staging/vc04_services/interface/vchiq_arm/vchiq_dev.c  | 7 +++----
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 9fb8f657cc78..9fb3e240d9de 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -687,10 +687,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
 		usleep_range(500, 600);
 	}
 	if (i == VCHIQ_INIT_RETRIES) {
-		vchiq_log_error(state->dev, VCHIQ_CORE, "%s: videocore not initialized\n",
-				__func__);
-		ret = -ENOTCONN;
-		goto failed;
+		pr_err("%s: videocore not initialized\n", __func__);
+		return -ENOTCONN;
 	} else if (i > 0) {
 		vchiq_log_warning(state->dev, VCHIQ_CORE,
 				  "%s: videocore initialized after %d retries\n", __func__, i);
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index 0bc93f48c14c..3425d2b199c2 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -1170,14 +1170,13 @@ static int vchiq_open(struct inode *inode, struct file *file)
 	struct vchiq_state *state = vchiq_get_state();
 	struct vchiq_instance *instance;
 
-	vchiq_log_debug(state->dev, VCHIQ_ARM, "vchiq_open");
-
 	if (!state) {
-		vchiq_log_error(state->dev, VCHIQ_ARM,
-				"vchiq has no connection to VideoCore");
+		pr_err("%s: vchiq has no connection to VideoCore\n", __func__);
 		return -ENOTCONN;
 	}
 
+	vchiq_log_debug(state->dev, VCHIQ_ARM, "vchiq_open");
+
 	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
 	if (!instance)
 		return -ENOMEM;
-- 
2.41.0

[PATCH 3/9] staging: vc04_services: bcm2835-camera: Remove redundant null check

[Umang Jain]

Remove the buf pointer null check in buffer_cb() as it can never be
NULL. buffer_cb() is always called with a valid mmal_buf pointer
(which is NULL checked) from which, struct vb2_mmal_buf buf pointer
is derived, using container_of macro.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Ricardo B. Marliere <ricardo@marliere.net>
---
 .../staging/vc04_services/bcm2835-camera/bcm2835-camera.c  | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
index e6e89784d84b..4b2b8f3bf903 100644
--- a/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
+++ b/drivers/staging/vc04_services/bcm2835-camera/bcm2835-camera.c
@@ -333,11 +333,8 @@ static void buffer_cb(struct vchiq_mmal_instance *instance,
 		 mmal_buf->pts);
 
 	if (status) {
-		/* error in transfer */
-		if (buf) {
-			/* there was a buffer with the error so return it */
-			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
-		}
+		/* There was a buffer with the error so return it. */
+		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
 		return;
 	}
 
-- 
2.41.0

[PATCH 4/9] staging: vc04_services: Shorten helper function name

[Umang Jain]

Shorten the helper log_category_str() to log_cat().
Going forwards, this will be used at multiple places with
dev_dbg().

No functiional changes intended in this patch.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_core.h     | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 161358db457c..cc7bb6ca832a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -39,7 +39,7 @@ enum vchiq_log_category {
 	VCHIQ_SUSPEND,
 };
 
-static inline const char *log_category_str(enum vchiq_log_category c)
+static inline const char *log_cat(enum vchiq_log_category c)
 {
 	static const char * const strings[] = {
 		"vchiq_arm",
@@ -54,19 +54,19 @@ static inline const char *log_category_str(enum vchiq_log_category c)
 
 #ifndef vchiq_log_error
 #define vchiq_log_error(dev, cat, fmt, ...) \
-	do { dev_dbg(dev, "%s error: " fmt, log_category_str(cat), ##__VA_ARGS__); } while (0)
+	do { dev_dbg(dev, "%s error: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
 #endif
 #ifndef vchiq_log_warning
 #define vchiq_log_warning(dev, cat, fmt, ...) \
-	do { dev_dbg(dev, "%s warning: " fmt, log_category_str(cat), ##__VA_ARGS__); } while (0)
+	do { dev_dbg(dev, "%s warning: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
 #endif
 #ifndef vchiq_log_debug
 #define vchiq_log_debug(dev, cat, fmt, ...) \
-	do { dev_dbg(dev, "%s debug: " fmt, log_category_str(cat), ##__VA_ARGS__); } while (0)
+	do { dev_dbg(dev, "%s debug: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
 #endif
 #ifndef vchiq_log_trace
 #define vchiq_log_trace(dev, cat, fmt, ...) \
-	do { dev_dbg(dev, "%s trace: " fmt, log_category_str(cat), ##__VA_ARGS__); } while (0)
+	do { dev_dbg(dev, "%s trace: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
 #endif
 
 #define VCHIQ_SLOT_MASK        (VCHIQ_SLOT_SIZE - 1)
-- 
2.41.0

[PATCH 5/9] staging: vc04_services: Do not pass NULL to vchiq_log_error()

[Umang Jain]

vchiq_add_connected_callback() logs using vchiq_log_error() macro,
but passes NULL instead of a struct device pointer. Fix it.

vchiq_add_connected_callback() is not used anywhere in the vc04_services
as of now. It will be used when we add new drivers(VC shared memory and
bcm2835-isp), hence it kept as it is for now.

Fixes: 1d8915cf8899 ("staging: vc04: Convert vchiq_log_error() to use dynamic debug")
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../vc04_services/interface/vchiq_arm/vchiq_connected.c       | 4 ++--
 .../vc04_services/interface/vchiq_arm/vchiq_connected.h       | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
index b3928bd8c9c6..21f9fa1a1713 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
@@ -27,7 +27,7 @@ static void connected_init(void)
  * be made immediately, otherwise it will be deferred until
  * vchiq_call_connected_callbacks is called.
  */
-void vchiq_add_connected_callback(void (*callback)(void))
+void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
 {
 	connected_init();
 
@@ -39,7 +39,7 @@ void vchiq_add_connected_callback(void (*callback)(void))
 		callback();
 	} else {
 		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
-			vchiq_log_error(NULL, VCHIQ_CORE,
+			vchiq_log_error(&device->dev, VCHIQ_CORE,
 					"There already %d callback registered - please increase MAX_CALLBACKS",
 					g_num_deferred_callbacks);
 		} else {
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
index 4caf5e30099d..e4ed56446f8a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
@@ -1,10 +1,12 @@
 /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
 /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
 
+#include "vchiq_bus.h"
+
 #ifndef VCHIQ_CONNECTED_H
 #define VCHIQ_CONNECTED_H
 
-void vchiq_add_connected_callback(void (*callback)(void));
+void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
 void vchiq_call_connected_callbacks(void);
 
 #endif /* VCHIQ_CONNECTED_H */
-- 
2.41.0

[PATCH 6/9] staging: vc04_services: Drop vchiq_log_error() in favour of dev_dbg

[Umang Jain]

Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
of dev_dbg() directly.

Add a new enum vchiq_log_type and log_type() helper to faciliate the
type of logging in dev_dbg(). This will help to determine the type of
logging("error", "warning", "debug", "trace") to dynamic debug.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           |  54 ++++----
 .../interface/vchiq_arm/vchiq_connected.c     |   6 +-
 .../interface/vchiq_arm/vchiq_core.c          | 126 ++++++++++--------
 .../interface/vchiq_arm/vchiq_core.h          |  23 +++-
 .../interface/vchiq_arm/vchiq_dev.c           |  47 ++++---
 5 files changed, 143 insertions(+), 113 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 9fb3e240d9de..2cb2a6503058 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -696,8 +696,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
 
 	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
 	if (!instance) {
-		vchiq_log_error(state->dev, VCHIQ_CORE,
-				"%s: error allocating vchiq instance\n", __func__);
+		dev_dbg(state->dev, "%s: %s: %s: error allocating vchiq instance\n",
+			log_cat(VCHIQ_CORE), log_type(ERROR), __func__);
 		ret = -ENOMEM;
 		goto failed;
 	}
@@ -969,8 +969,8 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
 	} else {
 		waiter = kzalloc(sizeof(*waiter), GFP_KERNEL);
 		if (!waiter) {
-			vchiq_log_error(service->state->dev, VCHIQ_CORE,
-					"%s - out of memory", __func__);
+			dev_dbg(service->state->dev, "%s: %s: %s - out of memory\n",
+				log_cat(VCHIQ_CORE), log_type(ERROR), __func__);
 			return -ENOMEM;
 		}
 	}
@@ -1344,8 +1344,8 @@ vchiq_keepalive_vchiq_callback(struct vchiq_instance *instance,
 			       struct vchiq_header *header,
 			       unsigned int service_user, void *bulk_user)
 {
-	vchiq_log_error(instance->state->dev, VCHIQ_SUSPEND,
-			"%s callback reason %d", __func__, reason);
+	dev_dbg(instance->state->dev, "%s: %s: %s callback reason %d\n",
+		log_cat(VCHIQ_SUSPEND), log_type(ERROR), __func__, reason);
 	return 0;
 }
 
@@ -1369,22 +1369,22 @@ vchiq_keepalive_thread_func(void *v)
 
 	ret = vchiq_initialise(&instance);
 	if (ret) {
-		vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-				"%s vchiq_initialise failed %d", __func__, ret);
+		dev_dbg(state->dev, "%s: %s: %s vchiq_initialise failed %d\n",
+			log_cat(VCHIQ_SUSPEND), log_type(ERROR), __func__, ret);
 		goto exit;
 	}
 
 	status = vchiq_connect(instance);
 	if (status) {
-		vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-				"%s vchiq_connect failed %d", __func__, status);
+		dev_dbg(state->dev, "%s: %s: %s vchiq_connect failed %d\n",
+			log_cat(VCHIQ_SUSPEND), log_type(ERROR), __func__, status);
 		goto shutdown;
 	}
 
 	status = vchiq_add_service(instance, &params, &ka_handle);
 	if (status) {
-		vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-				"%s vchiq_open_service failed %d", __func__, status);
+		dev_dbg(state->dev, "%s: %s: %s vchiq_open_service failed %d\n",
+			log_cat(VCHIQ_SUSPEND), log_type(ERROR), __func__, status);
 		goto shutdown;
 	}
 
@@ -1392,8 +1392,8 @@ vchiq_keepalive_thread_func(void *v)
 		long rc = 0, uc = 0;
 
 		if (wait_for_completion_interruptible(&arm_state->ka_evt)) {
-			vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-					"%s interrupted", __func__);
+			dev_dbg(state->dev, "%s: %s: %s interrupted\n",
+				log_cat(VCHIQ_SUSPEND), log_type(ERROR), __func__);
 			flush_signals(current);
 			continue;
 		}
@@ -1413,16 +1413,15 @@ vchiq_keepalive_thread_func(void *v)
 			atomic_inc(&arm_state->ka_use_ack_count);
 			status = vchiq_use_service(instance, ka_handle);
 			if (status) {
-				vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-						"%s vchiq_use_service error %d", __func__, status);
+				dev_dbg(state->dev, "%s: %s: %s vchiq_use_service error %d\n",
+					log_cat(VCHIQ_SUSPEND), log_type(ERROR), __func__, status);
 			}
 		}
 		while (rc--) {
 			status = vchiq_release_service(instance, ka_handle);
 			if (status) {
-				vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-						"%s vchiq_release_service error %d", __func__,
-						status);
+				dev_dbg(state->dev, "%s: %s: %s vchiq_release_service error %d\n",
+					log_cat(VCHIQ_SUSPEND), log_type(ERROR), __func__, status);
 			}
 		}
 	}
@@ -1457,7 +1456,8 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 			 service->client_id);
 		entity_uc = &service->service_use_count;
 	} else {
-		vchiq_log_error(state->dev, VCHIQ_SUSPEND, "%s null service ptr", __func__);
+		dev_dbg(state->dev, "%s: %s: %s null service ptr\n",
+			log_cat(VCHIQ_SUSPEND), log_type(ERROR), __func__);
 		ret = -EINVAL;
 		goto out;
 	}
@@ -1740,10 +1740,11 @@ vchiq_check_service(struct vchiq_service *service)
 	read_unlock_bh(&arm_state->susp_res_lock);
 
 	if (ret) {
-		vchiq_log_error(service->state->dev, VCHIQ_SUSPEND,
-				"%s ERROR - %p4cc:%d service count %d, state count %d", __func__,
-				&service->base.fourcc, service->client_id,
-				service->service_use_count, arm_state->videocore_use_count);
+		dev_dbg(service->state->dev,
+			"%s: %s: %s ERROR - %p4cc:%d service count %d, state count %d\n",
+			log_cat(VCHIQ_SUSPEND), log_type(ERROR), __func__,
+			&service->base.fourcc, service->client_id, service->service_use_count,
+			arm_state->videocore_use_count);
 		vchiq_dump_service_use_state(service->state);
 	}
 out:
@@ -1776,9 +1777,8 @@ void vchiq_platform_conn_state_changed(struct vchiq_state *state,
 					      (void *)state,
 					      threadname);
 	if (IS_ERR(arm_state->ka_thread)) {
-		vchiq_log_error(state->dev, VCHIQ_SUSPEND,
-				"vchiq: FATAL: couldn't create thread %s",
-				threadname);
+		dev_dbg(state->dev, "%s: %s: vchiq: FATAL: couldn't create thread %s\n",
+			log_cat(VCHIQ_SUSPEND), log_type(ERROR), threadname);
 	} else {
 		wake_up_process(arm_state->ka_thread);
 	}
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
index 21f9fa1a1713..2b4cbea5f259 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
@@ -39,9 +39,9 @@ void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(
 		callback();
 	} else {
 		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
-			vchiq_log_error(&device->dev, VCHIQ_CORE,
-					"There already %d callback registered - please increase MAX_CALLBACKS",
-					g_num_deferred_callbacks);
+			dev_dbg(&device->dev,
+				"%s: %s: There already %d callback registered - please increase MAX_CALLBACKS\n",
+				log_cat(VCHIQ_CORE), log_type(ERROR), g_num_deferred_callbacks);
 		} else {
 			g_deferred_callback[g_num_deferred_callbacks] =
 				callback;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 8a9eb0101c2e..5bc7c66e71e1 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -741,10 +741,10 @@ process_free_data_message(struct vchiq_state *state, u32 *service_found,
 		 */
 		complete(&quota->quota_event);
 	} else if (count == 0) {
-		vchiq_log_error(state->dev, VCHIQ_CORE,
-				"service %d message_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)",
-				port, quota->message_use_count, header, msgid, header->msgid,
-				header->size);
+		dev_dbg(state->dev,
+			"%s: %s: service %d message_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)\n",
+			log_cat(VCHIQ_CORE), log_type(ERROR), port, quota->message_use_count,
+			header, msgid, header->msgid, header->size);
 		WARN(1, "invalid message use count\n");
 	}
 	if (!BITSET_IS_SET(service_found, port)) {
@@ -766,9 +766,10 @@ process_free_data_message(struct vchiq_state *state, u32 *service_found,
 			vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: pfq:%d %x@%pK - slot_use->%d",
 					state->id, port, header->size, header, count - 1);
 		} else {
-			vchiq_log_error(state->dev, VCHIQ_CORE,
-					"service %d slot_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)",
-					port, count, header, msgid, header->msgid, header->size);
+			dev_dbg(state->dev,
+				"%s: %s: service %d slot_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)\n",
+				log_cat(VCHIQ_CORE), log_type(ERROR),
+				port, count, header, msgid, header->msgid, header->size);
 			WARN(1, "bad slot use count\n");
 		}
 	}
@@ -831,9 +832,10 @@ process_free_queue(struct vchiq_state *state, u32 *service_found,
 
 			pos += calc_stride(header->size);
 			if (pos > VCHIQ_SLOT_SIZE) {
-				vchiq_log_error(state->dev, VCHIQ_CORE,
-						"pfq - pos %x: header %pK, msgid %x, header->msgid %x, header->size %x",
-						pos, header, msgid, header->msgid, header->size);
+				dev_dbg(state->dev,
+					"%s: %s: pfq - pos %x: header %pK, msgid %x, header->msgid %x, header->size %x\n",
+					log_cat(VCHIQ_CORE), log_type(ERROR), pos, header, msgid,
+					header->msgid, header->size);
 				WARN(1, "invalid slot position\n");
 			}
 		}
@@ -1167,8 +1169,8 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
 		int oldmsgid = header->msgid;
 
 		if (oldmsgid != VCHIQ_MSGID_PADDING)
-			vchiq_log_error(state->dev, VCHIQ_CORE, "%d: qms - msgid %x, not PADDING",
-					state->id, oldmsgid);
+			dev_dbg(state->dev, "%s: %s: %d: qms - msgid %x, not PADDING\n",
+				log_cat(VCHIQ_CORE), log_type(ERROR), state->id, oldmsgid);
 	}
 
 	vchiq_log_debug(state->dev, VCHIQ_SYNC,
@@ -1616,10 +1618,10 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 		}
 
 		if (!service) {
-			vchiq_log_error(state->dev, VCHIQ_CORE,
-					"%d: prs %s@%pK (%d->%d) - invalid/closed service %d",
-					state->id, msg_type_str(type), header, remoteport,
-					localport, localport);
+			dev_dbg(state->dev,
+				"%s: %s: %d: prs %s@%pK (%d->%d) - invalid/closed service %d\n",
+				log_cat(VCHIQ_CORE), log_type(ERROR), state->id, msg_type_str(type),
+				header, remoteport, localport, localport);
 			goto skip_message;
 		}
 		break;
@@ -1640,9 +1642,9 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 
 	if (((unsigned long)header & VCHIQ_SLOT_MASK) +
 	    calc_stride(size) > VCHIQ_SLOT_SIZE) {
-		vchiq_log_error(state->dev, VCHIQ_CORE,
-				"header %pK (msgid %x) - size %x too big for slot",
-				header, (unsigned int)msgid, (unsigned int)size);
+		dev_dbg(state->dev, "%s: %s: header %pK (msgid %x) - size %x too big for slot\n",
+			log_cat(VCHIQ_CORE), log_type(ERROR),
+			header, (unsigned int)msgid, (unsigned int)size);
 		WARN(1, "oversized for slot\n");
 	}
 
@@ -1668,8 +1670,9 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 			set_service_state(service, VCHIQ_SRVSTATE_OPEN);
 			complete(&service->remove_event);
 		} else {
-			vchiq_log_error(state->dev, VCHIQ_CORE, "OPENACK received in state %s",
-					srvstate_names[service->srvstate]);
+			dev_dbg(state->dev, "%s: %s: OPENACK received in state %s\n",
+				log_cat(VCHIQ_CORE), log_type(ERROR),
+				srvstate_names[service->srvstate]);
 		}
 		break;
 	case VCHIQ_MSG_CLOSE:
@@ -1740,11 +1743,11 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 			}
 			if ((int)(queue->remote_insert -
 				queue->local_insert) >= 0) {
-				vchiq_log_error(state->dev, VCHIQ_CORE,
-						"%d: prs %s@%pK (%d->%d) unexpected (ri=%d,li=%d)",
-						state->id, msg_type_str(type), header, remoteport,
-						localport, queue->remote_insert,
-						queue->local_insert);
+				dev_dbg(state->dev,
+					"%s: %s: %d: prs %s@%pK (%d->%d) unexpected (ri=%d,li=%d)\n",
+					log_cat(VCHIQ_CORE), log_type(ERROR),
+					state->id, msg_type_str(type), header, remoteport,
+					localport, queue->remote_insert, queue->local_insert);
 				mutex_unlock(&service->bulk_mutex);
 				break;
 			}
@@ -1790,8 +1793,8 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 		vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: prs PAUSE@%pK,%x",
 				state->id, header, size);
 		if (state->conn_state == VCHIQ_CONNSTATE_PAUSED) {
-			vchiq_log_error(state->dev, VCHIQ_CORE,
-					"%d: PAUSE received in state PAUSED", state->id);
+			dev_dbg(state->dev, "%s: %s: %d: PAUSE received in state PAUSED\n",
+				log_cat(VCHIQ_CORE), log_type(ERROR), state->id);
 			break;
 		}
 		if (state->conn_state != VCHIQ_CONNSTATE_PAUSE_SENT) {
@@ -1821,8 +1824,8 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 		break;
 
 	default:
-		vchiq_log_error(state->dev, VCHIQ_CORE, "%d: prs invalid msgid %x@%pK,%x",
-				state->id, msgid, header, size);
+		dev_dbg(state->dev, "%s: %s: %d: prs invalid msgid %x@%pK,%x\n",
+			log_cat(VCHIQ_CORE), log_type(ERROR), state->id, msgid, header, size);
 		WARN(1, "invalid message\n");
 		break;
 	}
@@ -1932,7 +1935,8 @@ handle_poll(struct vchiq_state *state)
 			 * since the PAUSE should have flushed
 			 * through outstanding messages.
 			 */
-			vchiq_log_error(state->dev, VCHIQ_CORE, "Failed to send RESUME message");
+			dev_dbg(state->dev, "%s: %s: Failed to send RESUME message\n",
+				log_cat(VCHIQ_CORE), log_type(ERROR));
 		}
 		break;
 	default:
@@ -2032,10 +2036,10 @@ sync_func(void *v)
 		service = find_service_by_port(state, localport);
 
 		if (!service) {
-			vchiq_log_error(state->dev, VCHIQ_SYNC,
-					"%d: sf %s@%pK (%d->%d) - invalid/closed service %d",
-					state->id, msg_type_str(type), header,
-					remoteport, localport, localport);
+			dev_dbg(state->dev,
+				"%s: %s: %d: sf %s@%pK (%d->%d) - invalid/closed service %d\n",
+				log_cat(VCHIQ_SYNC), log_type(ERROR), state->id, msg_type_str(type),
+				header, remoteport, localport, localport);
 			release_message_sync(state, header);
 			continue;
 		}
@@ -2078,15 +2082,16 @@ sync_func(void *v)
 			    (service->srvstate == VCHIQ_SRVSTATE_OPENSYNC)) {
 				if (make_service_callback(service, VCHIQ_MESSAGE_AVAILABLE, header,
 							  NULL) == -EAGAIN)
-					vchiq_log_error(state->dev, VCHIQ_SYNC,
-							"synchronous callback to service %d returns -EAGAIN",
-							localport);
+					dev_dbg(state->dev,
+						"%s: %s: synchronous callback to service %d returns -EAGAIN\n",
+						log_cat(VCHIQ_SYNC), log_type(ERROR), localport);
 			}
 			break;
 
 		default:
-			vchiq_log_error(state->dev, VCHIQ_SYNC, "%d: sf unexpected msgid %x@%pK,%x",
-					state->id, msgid, header, size);
+			dev_dbg(state->dev, "%s: %s: %d: sf unexpected msgid %x@%pK,%x\n",
+				log_cat(VCHIQ_SYNC), log_type(ERROR),
+				state->id, msgid, header, size);
 			release_message_sync(state, header);
 			break;
 		}
@@ -2119,8 +2124,8 @@ vchiq_init_slots(struct device *dev, void *mem_base, int mem_size)
 	num_slots -= first_data_slot;
 
 	if (num_slots < 4) {
-		vchiq_log_error(dev, VCHIQ_CORE, "%s - insufficient memory %x bytes",
-				__func__, mem_size);
+		dev_dbg(dev, "%s: %s: %s - insufficient memory %x bytes\n",
+			log_cat(VCHIQ_CORE), log_type(ERROR), __func__, mem_size);
 		return NULL;
 	}
 
@@ -2501,11 +2506,11 @@ vchiq_open_service_internal(struct vchiq_service *service, int client_id)
 	} else if ((service->srvstate != VCHIQ_SRVSTATE_OPEN) &&
 		   (service->srvstate != VCHIQ_SRVSTATE_OPENSYNC)) {
 		if (service->srvstate != VCHIQ_SRVSTATE_CLOSEWAIT)
-			vchiq_log_error(service->state->dev, VCHIQ_CORE,
-					"%d: osi - srvstate = %s (ref %u)",
-					service->state->id,
-					srvstate_names[service->srvstate],
-					kref_read(&service->ref_count));
+			dev_dbg(service->state->dev,
+				"%s: %s: %d: osi - srvstate = %s (ref %u)\n",
+				log_cat(VCHIQ_CORE), log_type(ERROR),
+				service->state->id, srvstate_names[service->srvstate],
+				kref_read(&service->ref_count));
 		status = -EINVAL;
 		VCHIQ_SERVICE_STATS_INC(service, error_count);
 		vchiq_release_service_internal(service);
@@ -2566,9 +2571,10 @@ release_service_messages(struct vchiq_service *service)
 			}
 			pos += calc_stride(header->size);
 			if (pos > VCHIQ_SLOT_SIZE) {
-				vchiq_log_error(state->dev, VCHIQ_CORE,
-						"fsi - pos %x: header %pK, msgid %x, header->msgid %x, header->size %x",
-						pos, header, msgid, header->msgid, header->size);
+				dev_dbg(state->dev,
+					"%s: %s: fsi - pos %x: header %pK, msgid %x, header->msgid %x, header->size %x\n",
+					log_cat(VCHIQ_CORE), log_type(ERROR), pos, header, msgid,
+					header->msgid, header->size);
 				WARN(1, "invalid slot position\n");
 			}
 		}
@@ -2622,8 +2628,9 @@ close_service_complete(struct vchiq_service *service, int failstate)
 	case VCHIQ_SRVSTATE_LISTENING:
 		break;
 	default:
-		vchiq_log_error(service->state->dev, VCHIQ_CORE, "%s(%x) called in state %s",
-				__func__, service->handle, srvstate_names[service->srvstate]);
+		dev_dbg(service->state->dev, "%s: %s: %s(%x) called in state %s\n",
+			log_cat(VCHIQ_CORE), log_type(ERROR), __func__, service->handle,
+			srvstate_names[service->srvstate]);
 		WARN(1, "%s in unexpected state\n", __func__);
 		return -EINVAL;
 	}
@@ -2678,8 +2685,9 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
 	case VCHIQ_SRVSTATE_LISTENING:
 	case VCHIQ_SRVSTATE_CLOSEWAIT:
 		if (close_recvd) {
-			vchiq_log_error(state->dev, VCHIQ_CORE, "%s(1) called in state %s",
-					__func__, srvstate_names[service->srvstate]);
+			dev_dbg(state->dev, "%s: %s: %s(1) called in state %s\n",
+				log_cat(VCHIQ_CORE), log_type(ERROR), __func__,
+				srvstate_names[service->srvstate]);
 		} else if (is_server) {
 			if (service->srvstate == VCHIQ_SRVSTATE_LISTENING) {
 				status = -EINVAL;
@@ -2766,8 +2774,9 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
 		break;
 
 	default:
-		vchiq_log_error(state->dev, VCHIQ_CORE, "%s(%d) called in state %s", __func__,
-				close_recvd, srvstate_names[service->srvstate]);
+		dev_dbg(state->dev, "%s: %s: %s(%d) called in state %s\n",
+			log_cat(VCHIQ_CORE), log_type(ERROR), __func__,
+			close_recvd, srvstate_names[service->srvstate]);
 		break;
 	}
 
@@ -2806,8 +2815,9 @@ vchiq_free_service_internal(struct vchiq_service *service)
 	case VCHIQ_SRVSTATE_CLOSEWAIT:
 		break;
 	default:
-		vchiq_log_error(state->dev, VCHIQ_CORE, "%d: fsi - (%d) in state %s", state->id,
-				service->localport, srvstate_names[service->srvstate]);
+		dev_dbg(state->dev, "%s: %s: %d: fsi - (%d) in state %s\n",
+			log_cat(VCHIQ_CORE), log_type(ERROR), state->id,
+			service->localport, srvstate_names[service->srvstate]);
 		return;
 	}
 
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index cc7bb6ca832a..3583604430b3 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -39,6 +39,13 @@ enum vchiq_log_category {
 	VCHIQ_SUSPEND,
 };
 
+enum vchiq_log_type {
+	ERROR,
+	WARN,
+	DEBUG,
+	TRACE,
+};
+
 static inline const char *log_cat(enum vchiq_log_category c)
 {
 	static const char * const strings[] = {
@@ -52,10 +59,18 @@ static inline const char *log_cat(enum vchiq_log_category c)
 	return strings[c];
 };
 
-#ifndef vchiq_log_error
-#define vchiq_log_error(dev, cat, fmt, ...) \
-	do { dev_dbg(dev, "%s error: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
-#endif
+static inline const char *log_type(enum vchiq_log_type t)
+{
+	static const char * const types_str[] = {
+		"error",
+		"warning",
+		"debug",
+		"trace",
+	};
+
+	return types_str[t];
+};
+
 #ifndef vchiq_log_warning
 #define vchiq_log_warning(dev, cat, fmt, ...) \
 	do { dev_dbg(dev, "%s warning: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index 3425d2b199c2..76e16128d4b1 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -271,9 +271,10 @@ static int vchiq_ioc_dequeue_message(struct vchiq_instance *instance,
 			ret = -EFAULT;
 		}
 	} else {
-		vchiq_log_error(service->state->dev, VCHIQ_ARM,
-				"header %pK: bufsize %x < size %x",
-				header, args->bufsize, header->size);
+		dev_dbg(service->state->dev,
+			"%s: %s: header %pK: bufsize %x < size %x\n",
+			log_cat(VCHIQ_ARM), log_type(ERROR), header,
+			args->bufsize, header->size);
 		WARN(1, "invalid size\n");
 		ret = -EMSGSIZE;
 	}
@@ -318,8 +319,9 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 		}
 		mutex_unlock(&instance->bulk_waiter_list_mutex);
 		if (!waiter) {
-			vchiq_log_error(service->state->dev, VCHIQ_ARM,
-					"no bulk_waiter found for pid %d", current->pid);
+			dev_dbg(service->state->dev,
+				"%s: %s: no bulk_waiter found for pid %d\n",
+				log_cat(VCHIQ_ARM), log_type(ERROR), current->pid);
 			ret = -ESRCH;
 			goto out;
 		}
@@ -501,10 +503,11 @@ static int vchiq_ioc_await_completion(struct vchiq_instance *instance,
 			msglen = header->size + sizeof(struct vchiq_header);
 			/* This must be a VCHIQ-style service */
 			if (args->msgbufsize < msglen) {
-				vchiq_log_error(service->state->dev, VCHIQ_ARM,
-						"header %pK: msgbufsize %x < msglen %x",
-						header, args->msgbufsize, msglen);
-						WARN(1, "invalid message size\n");
+				dev_dbg(service->state->dev,
+					"%s: %s: header %pK: msgbufsize %x < msglen %x\n",
+					log_cat(VCHIQ_ARM), log_type(ERROR), header,
+					args->msgbufsize, msglen);
+				WARN(1, "invalid message size\n");
 				if (ret == 0)
 					ret = -EMSGSIZE;
 				break;
@@ -618,9 +621,9 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		}
 		rc = mutex_lock_killable(&instance->state->mutex);
 		if (rc) {
-			vchiq_log_error(instance->state->dev, VCHIQ_ARM,
-					"vchiq: connect: could not lock mutex for state %d: %d",
-					instance->state->id, rc);
+			dev_dbg(instance->state->dev,
+				"%s: %s: vchiq: connect: could not lock mutex for state %d: %d\n",
+				log_cat(VCHIQ_ARM), log_type(ERROR), instance->state->id, rc);
 			ret = -EINTR;
 			break;
 		}
@@ -630,8 +633,9 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		if (!status)
 			instance->connected = 1;
 		else
-			vchiq_log_error(instance->state->dev, VCHIQ_ARM,
-					"vchiq: could not connect: %d", status);
+			dev_dbg(instance->state->dev,
+				"%s: %s: vchiq: could not connect: %d\n",
+				log_cat(VCHIQ_ARM), log_type(ERROR), status);
 		break;
 
 	case VCHIQ_IOC_CREATE_SERVICE: {
@@ -700,13 +704,14 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 				vchiq_use_service_internal(service) :
 				vchiq_release_service_internal(service);
 			if (ret) {
-				vchiq_log_error(instance->state->dev, VCHIQ_SUSPEND,
-						"%s: cmd %s returned error %ld for service %p4cc:%03d",
-						__func__, (cmd == VCHIQ_IOC_USE_SERVICE) ?
-						"VCHIQ_IOC_USE_SERVICE" :
-						"VCHIQ_IOC_RELEASE_SERVICE",
-						ret, &service->base.fourcc,
-						service->client_id);
+				dev_dbg(instance->state->dev,
+					"%s: %s: %s: cmd %s returned error %ld for service %p4cc:%03d\n",
+					log_cat(VCHIQ_SUSPEND), log_type(ERROR),
+					__func__, (cmd == VCHIQ_IOC_USE_SERVICE) ?
+					"VCHIQ_IOC_USE_SERVICE" :
+					"VCHIQ_IOC_RELEASE_SERVICE",
+					ret, &service->base.fourcc,
+					service->client_id);
 			}
 		} else {
 			ret = -EINVAL;
-- 
2.41.0

[PATCH 7/9] staging: vc04_services: Drop vchiq_log_warning() in favour of dev_dbg

[Umang Jain]

Drop vchiq_log_warning() macro which wraps dev_dbg(). Introduce the usage
of dev_dbg() directly.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           | 35 ++++++++++---------
 .../interface/vchiq_arm/vchiq_core.c          | 33 +++++++++--------
 .../interface/vchiq_arm/vchiq_core.h          |  4 ---
 3 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 2cb2a6503058..bc0ee8b9d1c3 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -690,8 +690,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
 		pr_err("%s: videocore not initialized\n", __func__);
 		return -ENOTCONN;
 	} else if (i > 0) {
-		vchiq_log_warning(state->dev, VCHIQ_CORE,
-				  "%s: videocore initialized after %d retries\n", __func__, i);
+		dev_dbg(state->dev, "%s: %s: %s: videocore initialized after %d retries\n",
+			log_cat(VCHIQ_CORE), log_type(WARN), __func__, i);
 	}
 
 	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
@@ -1705,20 +1705,22 @@ vchiq_dump_service_use_state(struct vchiq_state *state)
 	read_unlock_bh(&arm_state->susp_res_lock);
 
 	if (only_nonzero)
-		vchiq_log_warning(state->dev, VCHIQ_SUSPEND,
-				  "Too many active services (%d). Only dumping up to first %d services with non-zero use-count",
-				  active_services, found);
+		dev_dbg(state->dev,
+			"%s: %s: Too many active services (%d). Only dumping up to first %d services with non-zero use-count\n",
+			log_cat(VCHIQ_SUSPEND), log_type(WARN), active_services, found);
 
 	for (i = 0; i < found; i++) {
-		vchiq_log_warning(state->dev, VCHIQ_SUSPEND,
-				  "%p4cc:%d service count %d %s",
-				  &service_data[i].fourcc,
-				  service_data[i].clientid, service_data[i].use_count,
-				  service_data[i].use_count ? nz : "");
+		dev_dbg(state->dev,
+			"%s: %s: %p4cc:%d service count %d %s\n",
+			log_cat(VCHIQ_SUSPEND), log_type(WARN),
+			&service_data[i].fourcc,
+			service_data[i].clientid, service_data[i].use_count,
+			service_data[i].use_count ? nz : "");
 	}
-	vchiq_log_warning(state->dev, VCHIQ_SUSPEND, "VCHIQ use count %d", peer_count);
-	vchiq_log_warning(state->dev, VCHIQ_SUSPEND, "Overall vchiq instance use count %d",
-			  vc_use_count);
+	dev_dbg(state->dev, "%s: %s: VCHIQ use count %d\n",
+		log_cat(VCHIQ_SUSPEND), log_type(WARN), peer_count);
+	dev_dbg(state->dev, "%s: %s: Overall vchiq instance use count %d\n",
+		log_cat(VCHIQ_SUSPEND), log_type(WARN), vc_use_count);
 
 	kfree(service_data);
 }
@@ -1833,8 +1835,8 @@ static int vchiq_probe(struct platform_device *pdev)
 	 */
 	err = vchiq_register_chrdev(&pdev->dev);
 	if (err) {
-		vchiq_log_warning(&pdev->dev, VCHIQ_ARM,
-				  "Failed to initialize vchiq cdev");
+		dev_dbg(&pdev->dev, "%s: %s: Failed to initialize vchiq cdev\n",
+			log_cat(VCHIQ_ARM), log_type(WARN));
 		goto error_exit;
 	}
 
@@ -1844,7 +1846,8 @@ static int vchiq_probe(struct platform_device *pdev)
 	return 0;
 
 failed_platform_init:
-	vchiq_log_warning(&pdev->dev, VCHIQ_ARM, "could not initialize vchiq platform");
+	dev_dbg(&pdev->dev, "%s: %s: Could not initialize vchiq platform\n",
+		log_cat(VCHIQ_ARM), log_type(WARN));
 error_exit:
 	return err;
 }
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 5bc7c66e71e1..20055aea29f5 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -465,9 +465,9 @@ make_service_callback(struct vchiq_service *service, enum vchiq_reason reason,
 	status = service->base.callback(service->instance, reason, header, service->handle,
 					bulk_userdata);
 	if (status && (status != -EAGAIN)) {
-		vchiq_log_warning(service->state->dev, VCHIQ_CORE,
-				  "%d: ignoring ERROR from callback to service %x",
-				  service->state->id, service->handle);
+		dev_dbg(service->state->dev,
+			"%s: %s: %d: ignoring ERROR from callback to service %x\n",
+			log_cat(VCHIQ_CORE), log_type(WARN), service->state->id, service->handle);
 		status = 0;
 	}
 
@@ -1611,10 +1611,11 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 				vchiq_service_put(service);
 			service = get_connected_service(state, remoteport);
 			if (service)
-				vchiq_log_warning(state->dev, VCHIQ_CORE,
-						  "%d: prs %s@%pK (%d->%d) - found connected service %d",
-						  state->id, msg_type_str(type), header,
-						  remoteport, localport, service->localport);
+				dev_dbg(state->dev,
+					"%s: %s: %d: prs %s@%pK (%d->%d) - found connected service %d\n",
+					log_cat(VCHIQ_CORE), log_type(WARN),
+					state->id, msg_type_str(type), header,
+					remoteport, localport, service->localport);
 		}
 
 		if (!service) {
@@ -2917,10 +2918,11 @@ vchiq_close_service(struct vchiq_instance *instance, unsigned int handle)
 		    (service->srvstate == VCHIQ_SRVSTATE_OPEN))
 			break;
 
-		vchiq_log_warning(service->state->dev, VCHIQ_CORE,
-				  "%d: close_service:%d - waiting in state %s",
-				  service->state->id, service->localport,
-				  srvstate_names[service->srvstate]);
+		dev_dbg(service->state->dev,
+			"%s: %s: %d: close_service:%d - waiting in state %s\n",
+			log_cat(VCHIQ_CORE), log_type(WARN),
+			service->state->id, service->localport,
+			srvstate_names[service->srvstate]);
 	}
 
 	if (!status &&
@@ -2978,10 +2980,11 @@ vchiq_remove_service(struct vchiq_instance *instance, unsigned int handle)
 		    (service->srvstate == VCHIQ_SRVSTATE_OPEN))
 			break;
 
-		vchiq_log_warning(service->state->dev, VCHIQ_CORE,
-				  "%d: remove_service:%d - waiting in state %s",
-				  service->state->id, service->localport,
-				  srvstate_names[service->srvstate]);
+		dev_dbg(service->state->dev,
+			"%s: %s: %d: remove_service:%d - waiting in state %s\n",
+			log_cat(VCHIQ_CORE), log_type(WARN),
+			service->state->id, service->localport,
+			srvstate_names[service->srvstate]);
 	}
 
 	if (!status && (service->srvstate != VCHIQ_SRVSTATE_FREE))
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 3583604430b3..80d0100094e6 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -71,10 +71,6 @@ static inline const char *log_type(enum vchiq_log_type t)
 	return types_str[t];
 };
 
-#ifndef vchiq_log_warning
-#define vchiq_log_warning(dev, cat, fmt, ...) \
-	do { dev_dbg(dev, "%s warning: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
-#endif
 #ifndef vchiq_log_debug
 #define vchiq_log_debug(dev, cat, fmt, ...) \
 	do { dev_dbg(dev, "%s debug: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
-- 
2.41.0

[PATCH 8/9] staging: vc04_services: Drop vchiq_log_trace() in favour of dev_dbg

[Umang Jain]

Drop vchiq_log_trace() macro which wraps dev_dbg(). Introduce the usage
of dev_dbg() directly.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 .../interface/vchiq_arm/vchiq_arm.c           |  75 +++++++-----
 .../interface/vchiq_arm/vchiq_core.c          | 115 ++++++++++--------
 .../interface/vchiq_arm/vchiq_core.h          |   4 -
 .../interface/vchiq_arm/vchiq_dev.c           |  17 +--
 4 files changed, 116 insertions(+), 95 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index bc0ee8b9d1c3..5bf22ff8ba83 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -255,8 +255,8 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 	pagelist = dma_alloc_coherent(instance->state->dev, pagelist_size, &dma_addr,
 				      GFP_KERNEL);
 
-	vchiq_log_trace(instance->state->dev, VCHIQ_ARM,
-			"%s - %pK", __func__, pagelist);
+	dev_dbg(instance->state->dev,
+		"%s: %s: %s - %pK\n", log_cat(VCHIQ_ARM), log_type(TRACE), __func__, pagelist);
 
 	if (!pagelist)
 		return NULL;
@@ -407,8 +407,8 @@ free_pagelist(struct vchiq_instance *instance, struct vchiq_pagelist_info *pagel
 	struct page **pages = pagelistinfo->pages;
 	unsigned int num_pages = pagelistinfo->num_pages;
 
-	vchiq_log_trace(instance->state->dev, VCHIQ_ARM,
-			"%s - %pK, %d", __func__, pagelistinfo->pagelist, actual);
+	dev_dbg(instance->state->dev, "%s: %s: %s - %pK, %d\n",
+		log_cat(VCHIQ_ARM), log_type(TRACE), __func__, pagelistinfo->pagelist, actual);
 
 	/*
 	 * NOTE: dma_unmap_sg must be called before the
@@ -712,8 +712,9 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
 	ret = 0;
 
 failed:
-	vchiq_log_trace(state->dev, VCHIQ_CORE,
-			"%s(%p): returning %d", __func__, instance, ret);
+	dev_dbg(state->dev,
+		"%s: %s: %s(%p): returning %d\n", __func__,
+		log_cat(VCHIQ_CORE), log_type(TRACE), instance, ret);
 
 	return ret;
 }
@@ -746,8 +747,9 @@ int vchiq_shutdown(struct vchiq_instance *instance)
 
 	mutex_unlock(&state->mutex);
 
-	vchiq_log_trace(state->dev, VCHIQ_CORE,
-			"%s(%p): returning %d", __func__, instance, status);
+	dev_dbg(state->dev,
+		"%s: %s: %s(%p): returning %d\n", __func__,
+		log_cat(VCHIQ_CORE), log_type(TRACE), instance, status);
 
 	free_bulk_waiter(instance);
 	kfree(instance);
@@ -767,8 +769,9 @@ int vchiq_connect(struct vchiq_instance *instance)
 	struct vchiq_state *state = instance->state;
 
 	if (mutex_lock_killable(&state->mutex)) {
-		vchiq_log_trace(state->dev, VCHIQ_CORE,
-				"%s: call to mutex_lock failed", __func__);
+		dev_dbg(state->dev,
+			"%s: %s: %s: call to mutex_lock failed\n",
+			log_cat(VCHIQ_CORE), log_type(TRACE), __func__);
 		status = -EAGAIN;
 		goto failed;
 	}
@@ -780,8 +783,9 @@ int vchiq_connect(struct vchiq_instance *instance)
 	mutex_unlock(&state->mutex);
 
 failed:
-	vchiq_log_trace(state->dev, VCHIQ_CORE,
-			"%s(%p): returning %d", __func__, instance, status);
+	dev_dbg(state->dev,
+		"%s: %s: %s(%p): returning %d\n",
+		log_cat(VCHIQ_CORE), log_type(TRACE), __func__, instance, status);
 
 	return status;
 }
@@ -812,8 +816,9 @@ vchiq_add_service(struct vchiq_instance *instance,
 		status = -EINVAL;
 	}
 
-	vchiq_log_trace(state->dev, VCHIQ_CORE,
-			"%s(%p): returning %d", __func__, instance, status);
+	dev_dbg(state->dev,
+		"%s: %s: %s(%p): returning %d\n",
+		log_cat(VCHIQ_CORE), log_type(TRACE), __func__, instance, status);
 
 	return status;
 }
@@ -844,8 +849,9 @@ vchiq_open_service(struct vchiq_instance *instance,
 	}
 
 failed:
-	vchiq_log_trace(state->dev, VCHIQ_CORE,
-			"%s(%p): returning %d", __func__, instance, status);
+	dev_dbg(state->dev,
+		"%s: %s: %s(%p): returning %d\n",
+		log_cat(VCHIQ_CORE), log_type(TRACE), __func__, instance, status);
 
 	return status;
 }
@@ -1015,8 +1021,9 @@ add_completion(struct vchiq_instance *instance, enum vchiq_reason reason,
 	while ((insert - instance->completion_remove) >= MAX_COMPLETIONS) {
 		/* Out of space - wait for the client */
 		DEBUG_TRACE(SERVICE_CALLBACK_LINE);
-		vchiq_log_trace(instance->state->dev, VCHIQ_CORE,
-				"%s - completion queue full", __func__);
+		dev_dbg(instance->state->dev,
+			"%s: %s: %s - completion queue full\n",
+			log_cat(VCHIQ_CORE), log_type(TRACE), __func__);
 		DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
 		if (wait_for_completion_interruptible(&instance->remove_event)) {
 			vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
@@ -1104,11 +1111,12 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 	vchiq_service_get(service);
 	rcu_read_unlock();
 
-	vchiq_log_trace(service->state->dev, VCHIQ_ARM,
-			"%s - service %lx(%d,%p), reason %d, header %lx, instance %lx, bulk_userdata %lx",
-			__func__, (unsigned long)user_service, service->localport,
-			user_service->userdata, reason, (unsigned long)header,
-			(unsigned long)instance, (unsigned long)bulk_userdata);
+	dev_dbg(service->state->dev,
+		"%s: %s: %s - service %lx(%d,%p), reason %d, header %lx, instance %lx, bulk_userdata %lx\n",
+		log_cat(VCHIQ_ARM), log_type(TRACE),
+		__func__, (unsigned long)user_service, service->localport,
+		user_service->userdata, reason, (unsigned long)header,
+		(unsigned long)instance, (unsigned long)bulk_userdata);
 
 	if (header && user_service->is_vchi) {
 		spin_lock(&msg_queue_spinlock);
@@ -1117,8 +1125,9 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 			spin_unlock(&msg_queue_spinlock);
 			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 			DEBUG_COUNT(MSG_QUEUE_FULL_COUNT);
-			vchiq_log_trace(service->state->dev, VCHIQ_ARM,
-					"%s - msg queue full", __func__);
+			dev_dbg(service->state->dev,
+				"%s: %s: %s - msg queue full\n",
+				log_cat(VCHIQ_ARM), log_type(TRACE), __func__);
 			/*
 			 * If there is no MESSAGE_AVAILABLE in the completion
 			 * queue, add one
@@ -1466,8 +1475,9 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 	local_uc = ++arm_state->videocore_use_count;
 	++(*entity_uc);
 
-	vchiq_log_trace(state->dev, VCHIQ_SUSPEND, "%s %s count %d, state count %d",
-			__func__, entity, *entity_uc, local_uc);
+	dev_dbg(state->dev, "%s: %s: %s %s count %d, state count %d\n",
+		log_cat(VCHIQ_SUSPEND), log_type(TRACE), __func__,
+		entity, *entity_uc, local_uc);
 
 	write_unlock_bh(&arm_state->susp_res_lock);
 
@@ -1486,7 +1496,8 @@ vchiq_use_internal(struct vchiq_state *state, struct vchiq_service *service,
 	}
 
 out:
-	vchiq_log_trace(state->dev, VCHIQ_SUSPEND, "%s exit %d", __func__, ret);
+	dev_dbg(state->dev, "%s: %s: %s exit %d\n",
+		log_cat(VCHIQ_SUSPEND), log_type(TRACE), __func__, ret);
 	return ret;
 }
 
@@ -1524,14 +1535,16 @@ vchiq_release_internal(struct vchiq_state *state, struct vchiq_service *service)
 	--arm_state->videocore_use_count;
 	--(*entity_uc);
 
-	vchiq_log_trace(state->dev, VCHIQ_SUSPEND, "%s %s count %d, state count %d",
-			__func__, entity, *entity_uc, arm_state->videocore_use_count);
+	dev_dbg(state->dev, "%s: %s: %s %s count %d, state count %d\n",
+		log_cat(VCHIQ_SUSPEND), log_type(TRACE), __func__,
+		entity, *entity_uc, arm_state->videocore_use_count);
 
 unlock:
 	write_unlock_bh(&arm_state->susp_res_lock);
 
 out:
-	vchiq_log_trace(state->dev, VCHIQ_SUSPEND, "%s exit %d", __func__, ret);
+	dev_dbg(state->dev, "%s: %s: %s exit %d\n",
+		log_cat(VCHIQ_SUSPEND), log_type(TRACE), __func__, ret);
 	return ret;
 }
 
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index 20055aea29f5..f8578d4c81bc 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -459,9 +459,10 @@ make_service_callback(struct vchiq_service *service, enum vchiq_reason reason,
 {
 	int status;
 
-	vchiq_log_trace(service->state->dev, VCHIQ_CORE, "%d: callback:%d (%s, %pK, %pK)",
-			service->state->id, service->localport, reason_names[reason],
-			header, bulk_userdata);
+	dev_dbg(service->state->dev, "%s: %s: %d: callback:%d (%s, %pK, %pK)\n",
+		log_cat(VCHIQ_CORE), log_type(TRACE),
+		service->state->id, service->localport, reason_names[reason],
+		header, bulk_userdata);
 	status = service->base.callback(service->instance, reason, header, service->handle,
 					bulk_userdata);
 	if (status && (status != -EAGAIN)) {
@@ -763,8 +764,9 @@ process_free_data_message(struct vchiq_state *state, u32 *service_found,
 			 * it has dropped below its quota
 			 */
 			complete(&quota->quota_event);
-			vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: pfq:%d %x@%pK - slot_use->%d",
-					state->id, port, header->size, header, count - 1);
+			dev_dbg(state->dev, "%s: %s: %d: pfq:%d %x@%pK - slot_use->%d\n",
+				log_cat(VCHIQ_CORE), log_type(TRACE),
+				state->id, port, header->size, header, count - 1);
 		} else {
 			dev_dbg(state->dev,
 				"%s: %s: service %d slot_use_count=%d (header %pK, msgid %x, header->msgid %x, header->size %x)\n",
@@ -810,9 +812,10 @@ process_free_queue(struct vchiq_state *state, u32 *service_found,
 		 */
 		rmb();
 
-		vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: pfq %d=%pK %x %x",
-				state->id, slot_index, data, local->slot_queue_recycle,
-				slot_queue_available);
+		dev_dbg(state->dev, "%s: %s: %d: pfq %d=%pK %x %x\n",
+			log_cat(VCHIQ_CORE), log_type(TRACE),
+			state->id, slot_index, data, local->slot_queue_recycle,
+			slot_queue_available);
 
 		/* Initialise the bitmask for services which have used this slot */
 		memset(service_found, 0, length);
@@ -982,10 +985,11 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 		       ((tx_end_index != quota->previous_tx_index) &&
 			(quota->slot_use_count == quota->slot_quota))) {
 			spin_unlock(&quota_spinlock);
-			vchiq_log_trace(state->dev, VCHIQ_CORE,
-					"%d: qm:%d %s,%zx - quota stall (msg %d, slot %d)",
-					state->id, service->localport, msg_type_str(type), size,
-					quota->message_use_count, quota->slot_use_count);
+			dev_dbg(state->dev,
+				"%s: %s: %d: qm:%d %s,%zx - quota stall (msg %d, slot %d)\n",
+				log_cat(VCHIQ_CORE), log_type(TRACE),
+				state->id, service->localport, msg_type_str(type), size,
+				quota->message_use_count, quota->slot_use_count);
 			VCHIQ_SERVICE_STATS_INC(service, quota_stalls);
 			mutex_unlock(&state->slot_mutex);
 			if (wait_for_completion_interruptible(&quota->quota_event))
@@ -1075,10 +1079,10 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 		spin_unlock(&quota_spinlock);
 
 		if (slot_use_count)
-			vchiq_log_trace(state->dev, VCHIQ_CORE,
-					"%d: qm:%d %s,%zx - slot_use->%d (hdr %p)", state->id,
-					service->localport, msg_type_str(VCHIQ_MSG_TYPE(msgid)),
-					size, slot_use_count, header);
+			dev_dbg(state->dev, "%s: %s: %d: qm:%d %s,%zx - slot_use->%d (hdr %p)\n",
+				log_cat(VCHIQ_CORE), log_type(TRACE), state->id,
+				service->localport, msg_type_str(VCHIQ_MSG_TYPE(msgid)),
+				size, slot_use_count, header);
 
 		VCHIQ_SERVICE_STATS_INC(service, ctrl_tx_count);
 		VCHIQ_SERVICE_STATS_ADD(service, ctrl_tx_bytes, size);
@@ -1207,11 +1211,12 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
 	svc_fourcc = service ? service->base.fourcc
 			     : VCHIQ_MAKE_FOURCC('?', '?', '?', '?');
 
-	vchiq_log_trace(state->dev, VCHIQ_SYNC,
-			"Sent Sync Msg %s(%u) to %p4cc s:%u d:%d len:%d",
-			msg_type_str(VCHIQ_MSG_TYPE(msgid)), VCHIQ_MSG_TYPE(msgid),
-			&svc_fourcc, VCHIQ_MSG_SRCPORT(msgid),
-			VCHIQ_MSG_DSTPORT(msgid), size);
+	dev_dbg(state->dev,
+		"%s: %s: Sent Sync Msg %s(%u) to %p4cc s:%u d:%d len:%d\n",
+		log_cat(VCHIQ_SYNC), log_type(TRACE),
+		msg_type_str(VCHIQ_MSG_TYPE(msgid)), VCHIQ_MSG_TYPE(msgid),
+		&svc_fourcc, VCHIQ_MSG_SRCPORT(msgid),
+		VCHIQ_MSG_DSTPORT(msgid), size);
 
 	remote_event_signal(&state->remote->sync_trigger);
 
@@ -1300,11 +1305,12 @@ notify_bulks(struct vchiq_service *service, struct vchiq_bulk_queue *queue,
 {
 	int status = 0;
 
-	vchiq_log_trace(service->state->dev, VCHIQ_CORE,
-			"%d: nb:%d %cx - p=%x rn=%x r=%x",
-			service->state->id, service->localport,
-			(queue == &service->bulk_tx) ? 't' : 'r',
-			queue->process, queue->remote_notify, queue->remove);
+	dev_dbg(service->state->dev,
+		"%s: %s: %d: nb:%d %cx - p=%x rn=%x r=%x\n",
+		log_cat(VCHIQ_CORE), log_type(TRACE),
+		service->state->id, service->localport,
+		(queue == &service->bulk_tx) ? 't' : 'r',
+		queue->process, queue->remote_notify, queue->remove);
 
 	queue->remote_notify = queue->process;
 
@@ -1427,11 +1433,12 @@ abort_outstanding_bulks(struct vchiq_service *service,
 {
 	int is_tx = (queue == &service->bulk_tx);
 
-	vchiq_log_trace(service->state->dev, VCHIQ_CORE,
-			"%d: aob:%d %cx - li=%x ri=%x p=%x",
-			service->state->id, service->localport,
-			is_tx ? 't' : 'r', queue->local_insert,
-			queue->remote_insert, queue->process);
+	dev_dbg(service->state->dev,
+		"%s: %s: %d: aob:%d %cx - li=%x ri=%x p=%x\n",
+		log_cat(VCHIQ_CORE), log_type(TRACE),
+		service->state->id, service->localport,
+		is_tx ? 't' : 'r', queue->local_insert,
+		queue->remote_insert, queue->process);
 
 	WARN_ON((int)(queue->local_insert - queue->process) < 0);
 	WARN_ON((int)(queue->remote_insert - queue->process) < 0);
@@ -1770,10 +1777,11 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 					state->id, msg_type_str(type), header, remoteport,
 					localport, bulk->actual, &bulk->data);
 
-			vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: prs:%d %cx li=%x ri=%x p=%x",
-					state->id, localport,
-					(type == VCHIQ_MSG_BULK_RX_DONE) ? 'r' : 't',
-					queue->local_insert, queue->remote_insert, queue->process);
+			dev_dbg(state->dev, "%s: %s: %d: prs:%d %cx li=%x ri=%x p=%x\n",
+				log_cat(VCHIQ_CORE), log_type(TRACE),
+				state->id, localport,
+				(type == VCHIQ_MSG_BULK_RX_DONE) ? 'r' : 't',
+				queue->local_insert, queue->remote_insert, queue->process);
 
 			DEBUG_TRACE(PARSE_LINE);
 			WARN_ON(queue->process == queue->local_insert);
@@ -1786,13 +1794,13 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 		}
 		break;
 	case VCHIQ_MSG_PADDING:
-		vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: prs PADDING@%pK,%x",
-				state->id, header, size);
+		dev_dbg(state->dev, "%s: %s: %d: prs PADDING@%pK,%x\n",
+			log_cat(VCHIQ_CORE), log_type(TRACE), state->id, header, size);
 		break;
 	case VCHIQ_MSG_PAUSE:
 		/* If initiated, signal the application thread */
-		vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: prs PAUSE@%pK,%x",
-				state->id, header, size);
+		dev_dbg(state->dev, "%s: %s: %d: prs PAUSE@%pK,%x\n",
+			log_cat(VCHIQ_CORE), log_type(TRACE), state->id, header, size);
 		if (state->conn_state == VCHIQ_CONNSTATE_PAUSED) {
 			dev_dbg(state->dev, "%s: %s: %d: PAUSE received in state PAUSED\n",
 				log_cat(VCHIQ_CORE), log_type(ERROR), state->id);
@@ -1808,8 +1816,8 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 		vchiq_set_conn_state(state, VCHIQ_CONNSTATE_PAUSED);
 		break;
 	case VCHIQ_MSG_RESUME:
-		vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: prs RESUME@%pK,%x",
-				state->id, header, size);
+		dev_dbg(state->dev, "%s: %s: %d: prs RESUME@%pK,%x\n",
+			log_cat(VCHIQ_CORE), log_type(TRACE), state->id, header, size);
 		/* Release the slot mutex */
 		mutex_unlock(&state->slot_mutex);
 		vchiq_set_conn_state(state, VCHIQ_CONNSTATE_CONNECTED);
@@ -2048,10 +2056,9 @@ sync_func(void *v)
 		svc_fourcc = service ? service->base.fourcc
 				     : VCHIQ_MAKE_FOURCC('?', '?', '?', '?');
 
-		vchiq_log_trace(state->dev, VCHIQ_SYNC,
-				"Rcvd Msg %s from %p4cc s:%d d:%d len:%d",
-				msg_type_str(type), &svc_fourcc,
-				remoteport, localport, size);
+		dev_dbg(state->dev, "%s: %s: Rcvd Msg %s from %p4cc s:%d d:%d len:%d\n",
+			log_cat(VCHIQ_SYNC), log_type(TRACE),
+			msg_type_str(type), &svc_fourcc, remoteport, localport, size);
 		if (size > 0)
 			vchiq_log_dump_mem(state->dev, "Rcvd", 0, header->data, min(16, size));
 
@@ -2076,8 +2083,9 @@ sync_func(void *v)
 			break;
 
 		case VCHIQ_MSG_DATA:
-			vchiq_log_trace(state->dev, VCHIQ_SYNC, "%d: sf DATA@%pK,%x (%d->%d)",
-					state->id, header, size, remoteport, localport);
+			dev_dbg(state->dev, "%s: %s: %d: sf DATA@%pK,%x (%d->%d)\n",
+				log_cat(VCHIQ_SYNC), log_type(TRACE),
+				state->id, header, size, remoteport, localport);
 
 			if ((service->remoteport == remoteport) &&
 			    (service->srvstate == VCHIQ_SRVSTATE_OPENSYNC)) {
@@ -3128,9 +3136,10 @@ int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
 	mutex_unlock(&state->slot_mutex);
 	mutex_unlock(&service->bulk_mutex);
 
-	vchiq_log_trace(state->dev, VCHIQ_CORE, "%d: bt:%d %cx li=%x ri=%x p=%x",
-			state->id, service->localport, dir_char, queue->local_insert,
-			queue->remote_insert, queue->process);
+	dev_dbg(state->dev, "%s: %s: %d: bt:%d %cx li=%x ri=%x p=%x\n",
+		log_cat(VCHIQ_CORE), log_type(TRACE),
+		state->id, service->localport, dir_char, queue->local_insert,
+		queue->remote_insert, queue->process);
 
 waiting:
 	vchiq_service_put(service);
@@ -3666,9 +3675,11 @@ void vchiq_log_dump_mem(struct device *dev, const char *label, u32 addr,
 		*s++ = '\0';
 
 		if (label && (*label != '\0'))
-			vchiq_log_trace(dev, VCHIQ_CORE, "%s: %08x: %s", label, addr, line_buf);
+			dev_dbg(dev, "%s: %s: %s: %08x: %s\n",
+				log_cat(VCHIQ_CORE), log_type(TRACE), label, addr, line_buf);
 		else
-			vchiq_log_trace(dev, VCHIQ_CORE, "%s: %08x: %s", label, addr, line_buf);
+			dev_dbg(dev, "%s: %s: %s: %08x: %s\n",
+				log_cat(VCHIQ_CORE), log_type(TRACE), label, addr, line_buf);
 
 		addr += 16;
 		mem += 16;
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index 80d0100094e6..de0d49e58e39 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -75,10 +75,6 @@ static inline const char *log_type(enum vchiq_log_type t)
 #define vchiq_log_debug(dev, cat, fmt, ...) \
 	do { dev_dbg(dev, "%s debug: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
 #endif
-#ifndef vchiq_log_trace
-#define vchiq_log_trace(dev, cat, fmt, ...) \
-	do { dev_dbg(dev, "%s trace: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
-#endif
 
 #define VCHIQ_SLOT_MASK        (VCHIQ_SLOT_SIZE - 1)
 #define VCHIQ_SLOT_QUEUE_MASK  (VCHIQ_MAX_SLOTS_PER_SIDE - 1)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index 76e16128d4b1..7f5bba005653 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -585,10 +585,10 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	long ret = 0;
 	int i, rc;
 
-	vchiq_log_trace(instance->state->dev, VCHIQ_ARM,
-			"%s - instance %pK, cmd %s, arg %lx", __func__, instance,
-			((_IOC_TYPE(cmd) == VCHIQ_IOC_MAGIC) && (_IOC_NR(cmd) <= VCHIQ_IOC_MAX)) ?
-			ioctl_names[_IOC_NR(cmd)] : "<invalid>", arg);
+	dev_dbg(instance->state->dev, "%s: %s: %s - instance %pK, cmd %s, arg %lx\n",
+		log_cat(VCHIQ_ARM), log_type(TRACE), __func__, instance,
+		((_IOC_TYPE(cmd) == VCHIQ_IOC_MAGIC) && (_IOC_NR(cmd) <= VCHIQ_IOC_MAX)) ?
+		ioctl_names[_IOC_NR(cmd)] : "<invalid>", arg);
 
 	switch (cmd) {
 	case VCHIQ_IOC_SHUTDOWN:
@@ -878,10 +878,11 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 				instance, (_IOC_NR(cmd) <= VCHIQ_IOC_MAX) ?
 				ioctl_names[_IOC_NR(cmd)] : "<invalid>", status, ret);
 	} else {
-		vchiq_log_trace(instance->state->dev, VCHIQ_ARM,
-				"  ioctl instance %pK, cmd %s -> status %d, %ld",
-				instance, (_IOC_NR(cmd) <= VCHIQ_IOC_MAX) ?
-				ioctl_names[_IOC_NR(cmd)] : "<invalid>", status, ret);
+		dev_dbg(instance->state->dev,
+			"%s: %s: ioctl instance %pK, cmd %s -> status %d\n, %ld\n",
+			log_cat(VCHIQ_ARM), log_type(TRACE),
+			instance, (_IOC_NR(cmd) <= VCHIQ_IOC_MAX) ?
+			ioctl_names[_IOC_NR(cmd)] : "<invalid>", status, ret);
 	}
 
 	return ret;
-- 
2.41.0

[PATCH 9/9] staging: vc04_services: Drop vchiq_log_debug() in favour of dev_dbg

[Umang Jain]

Drop vchiq_log_debug() macro which wraps dev_dbg(). Introduce the usage
of dev_dbg() directly.

Remove the entry from TODO regarding custom logging. VC04 is now
aligned according to the standard kernel logging mechanisms.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/staging/vc04_services/interface/TODO  |   5 -
 .../interface/vchiq_arm/vchiq_arm.c           |  53 ++---
 .../interface/vchiq_arm/vchiq_core.c          | 189 ++++++++++--------
 .../interface/vchiq_arm/vchiq_core.h          |   5 -
 .../interface/vchiq_arm/vchiq_dev.c           |  43 ++--
 5 files changed, 155 insertions(+), 140 deletions(-)

diff --git a/drivers/staging/vc04_services/interface/TODO b/drivers/staging/vc04_services/interface/TODO
index 6d9d4a800aa7..05eb5140d096 100644
--- a/drivers/staging/vc04_services/interface/TODO
+++ b/drivers/staging/vc04_services/interface/TODO
@@ -23,11 +23,6 @@ should properly handle a module unload. This also includes that all
 resources must be freed (kthreads, debugfs entries, ...) and global
 variables avoided.
 
-* Cleanup logging mechanism
-
-The driver should probably be using the standard kernel logging mechanisms
-such as dev_info, dev_dbg, and friends.
-
 * Documentation
 
 A short top-down description of this driver's architecture (function of
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 5bf22ff8ba83..4a2023919f5f 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -311,9 +311,9 @@ create_pagelist(struct vchiq_instance *instance, char *buf, char __user *ubuf,
 						   type == PAGELIST_READ, pages);
 
 		if (actual_pages != num_pages) {
-			vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
-					"%s - only %d/%d pages locked",
-					__func__, actual_pages, num_pages);
+			dev_dbg(instance->state->dev, "%s: %s: %s - only %d/%d pages locked\n",
+				log_cat(VCHIQ_ARM), log_type(DEBUG),
+				__func__, actual_pages, num_pages);
 
 			/* This is probably due to the process being killed */
 			if (actual_pages > 0)
@@ -556,8 +556,8 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state
 		return -ENXIO;
 	}
 
-	vchiq_log_debug(&pdev->dev, VCHIQ_ARM, "vchiq_init - done (slots %pK, phys %pad)",
-			vchiq_slot_zero, &slot_phys);
+	dev_dbg(&pdev->dev, "%s: %s: vchiq_init - done (slots %pK, phys %pad)\n",
+		log_cat(VCHIQ_ARM), log_type(DEBUG), vchiq_slot_zero, &slot_phys);
 
 	vchiq_call_connected_callbacks();
 
@@ -727,9 +727,9 @@ void free_bulk_waiter(struct vchiq_instance *instance)
 	list_for_each_entry_safe(waiter, next,
 				 &instance->bulk_waiter_list, list) {
 		list_del(&waiter->list);
-		vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
-				"bulk_waiter - cleaned up %pK for pid %d",
-				waiter, waiter->pid);
+		dev_dbg(instance->state->dev,
+			"%s: %s: bulk_waiter - cleaned up %pK for pid %d\n",
+			log_cat(VCHIQ_ARM), log_type(DEBUG), waiter, waiter->pid);
 		kfree(waiter);
 	}
 }
@@ -999,9 +999,8 @@ vchiq_blocking_bulk_transfer(struct vchiq_instance *instance, unsigned int handl
 		mutex_lock(&instance->bulk_waiter_list_mutex);
 		list_add(&waiter->list, &instance->bulk_waiter_list);
 		mutex_unlock(&instance->bulk_waiter_list_mutex);
-		vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
-				"saved bulk_waiter %pK for pid %d", waiter,
-				current->pid);
+		dev_dbg(instance->state->dev, "%s: %s: saved bulk_waiter %pK for pid %d\n",
+			log_cat(VCHIQ_ARM), log_type(DEBUG), waiter, current->pid);
 	}
 
 	return status;
@@ -1026,12 +1025,12 @@ add_completion(struct vchiq_instance *instance, enum vchiq_reason reason,
 			log_cat(VCHIQ_CORE), log_type(TRACE), __func__);
 		DEBUG_COUNT(COMPLETION_QUEUE_FULL_COUNT);
 		if (wait_for_completion_interruptible(&instance->remove_event)) {
-			vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
-					"service_callback interrupted");
+			dev_dbg(instance->state->dev, "%s: %s: service_callback interrupted\n",
+				log_cat(VCHIQ_ARM), log_type(DEBUG));
 			return -EAGAIN;
 		} else if (instance->closing) {
-			vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
-					"service_callback closing");
+			dev_dbg(instance->state->dev, "%s: %s: service_callback closing\n",
+				log_cat(VCHIQ_ARM), log_type(DEBUG));
 			return 0;
 		}
 		DEBUG_TRACE(SERVICE_CALLBACK_LINE);
@@ -1136,8 +1135,9 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 				instance->completion_remove) < 0) {
 				int status;
 
-				vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
-						"Inserting extra MESSAGE_AVAILABLE");
+				dev_dbg(instance->state->dev,
+					"%s: %s: Inserting extra MESSAGE_AVAILABLE\n",
+					log_cat(VCHIQ_ARM), log_type(DEBUG));
 				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 				status = add_completion(instance, reason, NULL, user_service,
 							bulk_userdata);
@@ -1150,14 +1150,15 @@ service_callback(struct vchiq_instance *instance, enum vchiq_reason reason,
 
 			DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 			if (wait_for_completion_interruptible(&user_service->remove_event)) {
-				vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
-						"%s interrupted", __func__);
+				dev_dbg(instance->state->dev,
+					"%s: %s: %s interrupted\n",
+					log_cat(VCHIQ_ARM), log_type(DEBUG), __func__);
 				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 				vchiq_service_put(service);
 				return -EAGAIN;
 			} else if (instance->closing) {
-				vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
-						"%s closing", __func__);
+				dev_dbg(instance->state->dev, "%s: %s: %s closing",
+					log_cat(VCHIQ_ARM), log_type(DEBUG), __func__);
 				DEBUG_TRACE(SERVICE_CALLBACK_LINE);
 				vchiq_service_put(service);
 				return -EINVAL;
@@ -1773,8 +1774,9 @@ void vchiq_platform_conn_state_changed(struct vchiq_state *state,
 	struct vchiq_arm_state *arm_state = vchiq_platform_get_arm_state(state);
 	char threadname[16];
 
-	vchiq_log_debug(state->dev, VCHIQ_SUSPEND, "%d: %s->%s", state->id,
-			get_conn_state_name(oldstate), get_conn_state_name(newstate));
+	dev_dbg(state->dev, "%s: %s: %d: %s->%s\n",
+		log_cat(VCHIQ_SUSPEND), log_type(DEBUG), state->id,
+		get_conn_state_name(oldstate), get_conn_state_name(newstate));
 	if (state->conn_state != VCHIQ_CONNSTATE_CONNECTED)
 		return;
 
@@ -1838,9 +1840,8 @@ static int vchiq_probe(struct platform_device *pdev)
 
 	vchiq_debugfs_init();
 
-	vchiq_log_debug(&pdev->dev, VCHIQ_ARM,
-			"vchiq: platform initialised - version %d (min %d)",
-			VCHIQ_VERSION, VCHIQ_VERSION_MIN);
+	dev_dbg(&pdev->dev, "%s: %s: vchiq: platform initialised - version %d (min %d)\n",
+		log_cat(VCHIQ_ARM), log_type(DEBUG), VCHIQ_VERSION, VCHIQ_VERSION_MIN);
 
 	/*
 	 * Simply exit on error since the function handles cleanup in
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
index f8578d4c81bc..ec02bc9b74f7 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.c
@@ -217,10 +217,11 @@ static const char *msg_type_str(unsigned int msg_type)
 static inline void
 set_service_state(struct vchiq_service *service, int newstate)
 {
-	vchiq_log_debug(service->state->dev, VCHIQ_CORE, "%d: srv:%d %s->%s",
-			service->state->id, service->localport,
-			srvstate_names[service->srvstate],
-			srvstate_names[newstate]);
+	dev_dbg(service->state->dev, "%s: %s: %d: srv:%d %s->%s\n",
+		log_cat(VCHIQ_CORE), log_type(DEBUG),
+		service->state->id, service->localport,
+		srvstate_names[service->srvstate],
+		srvstate_names[newstate]);
 	service->srvstate = newstate;
 }
 
@@ -245,8 +246,9 @@ find_service_by_handle(struct vchiq_instance *instance, unsigned int handle)
 		return service;
 	}
 	rcu_read_unlock();
-	vchiq_log_debug(instance->state->dev, VCHIQ_CORE,
-			"Invalid service handle 0x%x", handle);
+	dev_dbg(instance->state->dev,
+		"%s: %s: Invalid service handle 0x%x\n",
+		log_cat(VCHIQ_CORE), log_type(DEBUG), handle);
 	return NULL;
 }
 
@@ -266,8 +268,8 @@ find_service_by_port(struct vchiq_state *state, unsigned int localport)
 		}
 		rcu_read_unlock();
 	}
-	vchiq_log_debug(state->dev, VCHIQ_CORE,
-			"Invalid port %u", localport);
+	dev_dbg(state->dev, "%s: %s: Invalid port %u\n",
+		log_cat(VCHIQ_CORE), log_type(DEBUG), localport);
 	return NULL;
 }
 
@@ -287,8 +289,8 @@ find_service_for_instance(struct vchiq_instance *instance, unsigned int handle)
 		return service;
 	}
 	rcu_read_unlock();
-	vchiq_log_debug(instance->state->dev, VCHIQ_CORE,
-			"Invalid service handle 0x%x", handle);
+	dev_dbg(instance->state->dev, "%s: %s: Invalid service handle 0x%x\n",
+		log_cat(VCHIQ_CORE), log_type(DEBUG), handle);
 	return NULL;
 }
 
@@ -310,8 +312,8 @@ find_closed_service_for_instance(struct vchiq_instance *instance, unsigned int h
 		return service;
 	}
 	rcu_read_unlock();
-	vchiq_log_debug(instance->state->dev, VCHIQ_CORE,
-			"Invalid service handle 0x%x", handle);
+	dev_dbg(instance->state->dev, "%s: %s: Invalid service handle 0x%x\n",
+		log_cat(VCHIQ_CORE), log_type(DEBUG), handle);
 	return service;
 }
 
@@ -483,8 +485,8 @@ vchiq_set_conn_state(struct vchiq_state *state, enum vchiq_connstate newstate)
 {
 	enum vchiq_connstate oldstate = state->conn_state;
 
-	vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: %s->%s", state->id, conn_state_names[oldstate],
-			conn_state_names[newstate]);
+	dev_dbg(state->dev, "%s: %s: %d: %s->%s\n", log_cat(VCHIQ_CORE), log_type(DEBUG),
+		state->id, conn_state_names[oldstate], conn_state_names[newstate]);
 	state->conn_state = newstate;
 	vchiq_platform_conn_state_changed(state, oldstate, newstate);
 }
@@ -1029,9 +1031,10 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 		int tx_end_index;
 		int slot_use_count;
 
-		vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: qm %s@%pK,%zx (%d->%d)", state->id,
-				msg_type_str(VCHIQ_MSG_TYPE(msgid)), header, size,
-				VCHIQ_MSG_SRCPORT(msgid), VCHIQ_MSG_DSTPORT(msgid));
+		dev_dbg(state->dev, "%s: %s: %d: qm %s@%pK,%zx (%d->%d)\n",
+			log_cat(VCHIQ_SUSPEND), log_type(DEBUG),
+			state->id, msg_type_str(VCHIQ_MSG_TYPE(msgid)), header, size,
+			VCHIQ_MSG_SRCPORT(msgid), VCHIQ_MSG_DSTPORT(msgid));
 
 		WARN_ON(flags & (QMFLAGS_NO_MUTEX_LOCK |
 				 QMFLAGS_NO_MUTEX_UNLOCK));
@@ -1087,9 +1090,10 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 		VCHIQ_SERVICE_STATS_INC(service, ctrl_tx_count);
 		VCHIQ_SERVICE_STATS_ADD(service, ctrl_tx_bytes, size);
 	} else {
-		vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: qm %s@%pK,%zx (%d->%d)", state->id,
-				msg_type_str(VCHIQ_MSG_TYPE(msgid)), header, size,
-				VCHIQ_MSG_SRCPORT(msgid), VCHIQ_MSG_DSTPORT(msgid));
+		dev_dbg(state->dev, "%s: %s: %d: qm %s@%pK,%zx (%d->%d)\n",
+			log_cat(VCHIQ_CORE), log_type(DEBUG), state->id,
+			msg_type_str(VCHIQ_MSG_TYPE(msgid)), header, size,
+			VCHIQ_MSG_SRCPORT(msgid), VCHIQ_MSG_DSTPORT(msgid));
 		if (size != 0) {
 			/*
 			 * It is assumed for now that this code path
@@ -1117,11 +1121,10 @@ queue_message(struct vchiq_state *state, struct vchiq_service *service,
 			? service->base.fourcc
 			: VCHIQ_MAKE_FOURCC('?', '?', '?', '?');
 
-		vchiq_log_debug(state->dev, VCHIQ_CORE_MSG,
-				"Sent Msg %s(%u) to %p4cc s:%u d:%d len:%zu",
-				msg_type_str(VCHIQ_MSG_TYPE(msgid)), VCHIQ_MSG_TYPE(msgid),
-				&svc_fourcc, VCHIQ_MSG_SRCPORT(msgid),
-				VCHIQ_MSG_DSTPORT(msgid), size);
+		dev_dbg(state->dev, "%s: %s: Sent Msg %s(%u) to %p4cc s:%u d:%d len:%zu\n",
+			log_cat(VCHIQ_CORE_MSG), log_type(DEBUG),
+			msg_type_str(VCHIQ_MSG_TYPE(msgid)), VCHIQ_MSG_TYPE(msgid),
+			&svc_fourcc, VCHIQ_MSG_SRCPORT(msgid), VCHIQ_MSG_DSTPORT(msgid), size);
 	}
 
 	/* Make sure the new header is visible to the peer. */
@@ -1177,11 +1180,11 @@ queue_message_sync(struct vchiq_state *state, struct vchiq_service *service,
 				log_cat(VCHIQ_CORE), log_type(ERROR), state->id, oldmsgid);
 	}
 
-	vchiq_log_debug(state->dev, VCHIQ_SYNC,
-			"%d: qms %s@%pK,%x (%d->%d)", state->id,
-			msg_type_str(VCHIQ_MSG_TYPE(msgid)),
-			header, size, VCHIQ_MSG_SRCPORT(msgid),
-			VCHIQ_MSG_DSTPORT(msgid));
+	dev_dbg(state->dev, "%s: %s: %d: qms %s@%pK,%x (%d->%d)\n",
+		log_cat(VCHIQ_SYNC), log_type(DEBUG), state->id,
+		msg_type_str(VCHIQ_MSG_TYPE(msgid)),
+		header, size, VCHIQ_MSG_SRCPORT(msgid),
+		VCHIQ_MSG_DSTPORT(msgid));
 
 	callback_result =
 		copy_message_data(copy_callback, context,
@@ -1268,9 +1271,10 @@ release_slot(struct vchiq_state *state, struct vchiq_slot_info *slot_info,
 			VCHIQ_SLOT_QUEUE_MASK] =
 			SLOT_INDEX_FROM_INFO(state, slot_info);
 		state->remote->slot_queue_recycle = slot_queue_recycle + 1;
-		vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: %s %d - recycle->%x",
-				state->id, __func__, SLOT_INDEX_FROM_INFO(state, slot_info),
-				state->remote->slot_queue_recycle);
+		dev_dbg(state->dev, "%s: %s: %d: %s %d - recycle->%x\n",
+			log_cat(VCHIQ_CORE), log_type(DEBUG),
+			state->id, __func__, SLOT_INDEX_FROM_INFO(state, slot_info),
+			state->remote->slot_queue_recycle);
 
 		/*
 		 * A write barrier is necessary, but remote_event_signal
@@ -1390,8 +1394,9 @@ poll_services_of_group(struct vchiq_state *state, int group)
 
 		service_flags = atomic_xchg(&service->poll_flags, 0);
 		if (service_flags & BIT(VCHIQ_POLL_REMOVE)) {
-			vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: ps - remove %d<->%d",
-					state->id, service->localport, service->remoteport);
+			dev_dbg(state->dev, "%s: %s: %d: ps - remove %d<->%d\n",
+				log_cat(VCHIQ_CORE), log_type(DEBUG),
+				state->id, service->localport, service->remoteport);
 
 			/*
 			 * Make it look like a client, because
@@ -1403,8 +1408,9 @@ poll_services_of_group(struct vchiq_state *state, int group)
 			if (vchiq_close_service_internal(service, NO_CLOSE_RECVD))
 				request_poll(state, service, VCHIQ_POLL_REMOVE);
 		} else if (service_flags & BIT(VCHIQ_POLL_TERMINATE)) {
-			vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: ps - terminate %d<->%d",
-					state->id, service->localport, service->remoteport);
+			dev_dbg(state->dev, "%s: %s: %d: ps - terminate %d<->%d\n",
+				log_cat(VCHIQ_CORE), log_type(DEBUG),
+				state->id, service->localport, service->remoteport);
 			if (vchiq_close_service_internal(service, NO_CLOSE_RECVD))
 				request_poll(state, service, VCHIQ_POLL_TERMINATE);
 		}
@@ -1457,11 +1463,12 @@ abort_outstanding_bulks(struct vchiq_service *service,
 		if (queue->process != queue->local_insert) {
 			vchiq_complete_bulk(service->instance, bulk);
 
-			vchiq_log_debug(service->state->dev, VCHIQ_CORE_MSG,
-					"%s %p4cc d:%d ABORTED - tx len:%d, rx len:%d",
-					is_tx ? "Send Bulk to" : "Recv Bulk from",
-					&service->base.fourcc,
-					service->remoteport, bulk->size, bulk->remote_size);
+			dev_dbg(service->state->dev,
+				"%s: %s: %s %p4cc d:%d ABORTED - tx len:%d, rx len:%d\n",
+				log_cat(VCHIQ_CORE_MSG), log_type(DEBUG),
+				is_tx ? "Send Bulk to" : "Recv Bulk from",
+				&service->base.fourcc,
+				service->remoteport, bulk->size, bulk->remote_size);
 		} else {
 			/* fabricate a matching dummy bulk */
 			bulk->data = 0;
@@ -1494,8 +1501,9 @@ parse_open(struct vchiq_state *state, struct vchiq_header *header)
 
 	payload = (struct vchiq_open_payload *)header->data;
 	fourcc = payload->fourcc;
-	vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: prs OPEN@%pK (%d->'%p4cc')",
-			state->id, header, localport, &fourcc);
+	dev_dbg(state->dev, "%s: %s: %d: prs OPEN@%pK (%d->'%p4cc')\n",
+		log_cat(VCHIQ_CORE), log_type(DEBUG),
+		state->id, header, localport, &fourcc);
 
 	service = get_listening_service(state, fourcc);
 	if (!service)
@@ -1641,10 +1649,9 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 	svc_fourcc = service ? service->base.fourcc
 			     : VCHIQ_MAKE_FOURCC('?', '?', '?', '?');
 
-	vchiq_log_debug(state->dev, VCHIQ_CORE_MSG,
-			"Rcvd Msg %s(%u) from %p4cc s:%d d:%d len:%d",
-			msg_type_str(type), type, &svc_fourcc,
-			remoteport, localport, size);
+	dev_dbg(state->dev, "%s: %s: Rcvd Msg %s(%u) from %p4cc s:%d d:%d len:%d\n",
+		log_cat(VCHIQ_CORE_MSG), log_type(DEBUG),
+		msg_type_str(type), type, &svc_fourcc, remoteport, localport, size);
 	if (size > 0)
 		vchiq_log_dump_mem(state->dev, "Rcvd", 0, header->data, min(16, size));
 
@@ -1669,10 +1676,11 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 				header->data;
 			service->peer_version = payload->version;
 		}
-		vchiq_log_debug(state->dev, VCHIQ_CORE,
-				"%d: prs OPENACK@%pK,%x (%d->%d) v:%d",
-				state->id, header, size, remoteport, localport,
-				service->peer_version);
+		dev_dbg(state->dev,
+			"%s: %s: %d: prs OPENACK@%pK,%x (%d->%d) v:%d\n",
+			log_cat(VCHIQ_CORE), log_type(DEBUG),
+			state->id, header, size, remoteport, localport,
+			service->peer_version);
 		if (service->srvstate == VCHIQ_SRVSTATE_OPENING) {
 			service->remoteport = remoteport;
 			set_service_state(service, VCHIQ_SRVSTATE_OPEN);
@@ -1686,21 +1694,23 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 	case VCHIQ_MSG_CLOSE:
 		WARN_ON(size); /* There should be no data */
 
-		vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: prs CLOSE@%pK (%d->%d)",
-				state->id, header, remoteport, localport);
+		dev_dbg(state->dev, "%s: %s: %d: prs CLOSE@%pK (%d->%d)\n",
+			log_cat(VCHIQ_CORE), log_type(DEBUG),
+			state->id, header, remoteport, localport);
 
 		mark_service_closing_internal(service, 1);
 
 		if (vchiq_close_service_internal(service, CLOSE_RECVD) == -EAGAIN)
 			goto bail_not_ready;
 
-		vchiq_log_debug(state->dev, VCHIQ_CORE, "Close Service %p4cc s:%u d:%d",
-				&service->base.fourcc,
-				service->localport, service->remoteport);
+		dev_dbg(state->dev, "%s: %s: Close Service %p4cc s:%u d:%d\n",
+			log_cat(VCHIQ_CORE), log_type(DEBUG),
+			&service->base.fourcc, service->localport, service->remoteport);
 		break;
 	case VCHIQ_MSG_DATA:
-		vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: prs DATA@%pK,%x (%d->%d)",
-				state->id, header, size, remoteport, localport);
+		dev_dbg(state->dev, "%s: %s: %d: prs DATA@%pK,%x (%d->%d)\n",
+			log_cat(VCHIQ_CORE), log_type(DEBUG),
+			state->id, header, size, remoteport, localport);
 
 		if ((service->remoteport == remoteport) &&
 		    (service->srvstate == VCHIQ_SRVSTATE_OPEN)) {
@@ -1719,8 +1729,8 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 		}
 		break;
 	case VCHIQ_MSG_CONNECT:
-		vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: prs CONNECT@%pK",
-				state->id, header);
+		dev_dbg(state->dev, "%s: %s: %d: prs CONNECT@%pK\n",
+			log_cat(VCHIQ_CORE), log_type(DEBUG), state->id, header);
 		state->version_common =	((struct vchiq_slot_zero *)
 					 state->slot_data)->version;
 		complete(&state->connect);
@@ -1772,10 +1782,10 @@ parse_message(struct vchiq_state *state, struct vchiq_header *header)
 			bulk->actual = *(int *)header->data;
 			queue->remote_insert++;
 
-			vchiq_log_debug(state->dev, VCHIQ_CORE,
-					"%d: prs %s@%pK (%d->%d) %x@%pad",
-					state->id, msg_type_str(type), header, remoteport,
-					localport, bulk->actual, &bulk->data);
+			dev_dbg(state->dev, "%s: %s: %d: prs %s@%pK (%d->%d) %x@%pad\n",
+				log_cat(VCHIQ_CORE), log_type(DEBUG),
+				state->id, msg_type_str(type), header, remoteport,
+				localport, bulk->actual, &bulk->data);
 
 			dev_dbg(state->dev, "%s: %s: %d: prs:%d %cx li=%x ri=%x p=%x\n",
 				log_cat(VCHIQ_CORE), log_type(TRACE),
@@ -2070,9 +2080,10 @@ sync_func(void *v)
 					header->data;
 				service->peer_version = payload->version;
 			}
-			vchiq_log_debug(state->dev, VCHIQ_SYNC, "%d: sf OPENACK@%pK,%x (%d->%d) v:%d",
-					state->id, header, size, remoteport, localport,
-					service->peer_version);
+			dev_err(state->dev, "%s: %s: %d: sf OPENACK@%pK,%x (%d->%d) v:%d\n",
+				log_cat(VCHIQ_SYNC), log_type(DEBUG),
+				state->id, header, size, remoteport, localport,
+				service->peer_version);
 			if (service->srvstate == VCHIQ_SRVSTATE_OPENING) {
 				service->remoteport = remoteport;
 				set_service_state(service, VCHIQ_SRVSTATE_OPENSYNC);
@@ -2476,9 +2487,10 @@ vchiq_add_service_internal(struct vchiq_state *state,
 	/* Bring this service online */
 	set_service_state(service, srvstate);
 
-	vchiq_log_debug(state->dev, VCHIQ_CORE_MSG, "%s Service %p4cc SrcPort:%d",
-			(srvstate == VCHIQ_SRVSTATE_OPENING) ? "Open" : "Add",
-			&params->fourcc, service->localport);
+	dev_dbg(state->dev, "%s: %s: %s Service %p4cc SrcPort:%d\n",
+		log_cat(VCHIQ_CORE_MSG), log_type(DEBUG),
+		(srvstate == VCHIQ_SRVSTATE_OPENING) ? "Open" : "Add",
+		&params->fourcc, service->localport);
 
 	/* Don't unlock the service - leave it with a ref_count of 1. */
 
@@ -2574,8 +2586,8 @@ release_service_messages(struct vchiq_service *service)
 			int port = VCHIQ_MSG_DSTPORT(msgid);
 
 			if ((port == service->localport) && (msgid & VCHIQ_MSGID_CLAIMED)) {
-				vchiq_log_debug(state->dev, VCHIQ_CORE,
-						"  fsi - hdr %pK", header);
+				dev_dbg(state->dev, "%s: %s:  fsi - hdr %pK\n",
+					log_cat(VCHIQ_CORE), log_type(DEBUG), header);
 				release_slot(state, slot_info, header, NULL);
 			}
 			pos += calc_stride(header->size);
@@ -2685,8 +2697,9 @@ vchiq_close_service_internal(struct vchiq_service *service, int close_recvd)
 	int close_id = MAKE_CLOSE(service->localport,
 				  VCHIQ_MSG_DSTPORT(service->remoteport));
 
-	vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: csi:%d,%d (%s)", service->state->id,
-			service->localport, close_recvd, srvstate_names[service->srvstate]);
+	dev_dbg(state->dev, "%s: %s: %d: csi:%d,%d (%s)\n",
+		log_cat(VCHIQ_CORE), log_type(DEBUG), service->state->id,
+		service->localport, close_recvd, srvstate_names[service->srvstate]);
 
 	switch (service->srvstate) {
 	case VCHIQ_SRVSTATE_CLOSED:
@@ -2798,8 +2811,9 @@ vchiq_terminate_service_internal(struct vchiq_service *service)
 {
 	struct vchiq_state *state = service->state;
 
-	vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: tsi - (%d<->%d)", state->id,
-			service->localport, service->remoteport);
+	dev_dbg(state->dev, "%s: %s: %d: tsi - (%d<->%d)\n",
+		log_cat(VCHIQ_CORE), log_type(DEBUG),
+		state->id, service->localport, service->remoteport);
 
 	mark_service_closing(service);
 
@@ -2813,8 +2827,8 @@ vchiq_free_service_internal(struct vchiq_service *service)
 {
 	struct vchiq_state *state = service->state;
 
-	vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: fsi - (%d)",
-			state->id, service->localport);
+	dev_dbg(state->dev, "%s: %s: %d: fsi - (%d)\n",
+		log_cat(VCHIQ_CORE), log_type(DEBUG), state->id, service->localport);
 
 	switch (service->srvstate) {
 	case VCHIQ_SRVSTATE_OPENING:
@@ -2895,8 +2909,9 @@ vchiq_close_service(struct vchiq_instance *instance, unsigned int handle)
 	if (!service)
 		return -EINVAL;
 
-	vchiq_log_debug(service->state->dev, VCHIQ_CORE, "%d: close_service:%d",
-			service->state->id, service->localport);
+	dev_dbg(service->state->dev, "%s: %s: %d: close_service:%d\n",
+		log_cat(VCHIQ_CORE), log_type(DEBUG),
+		service->state->id, service->localport);
 
 	if ((service->srvstate == VCHIQ_SRVSTATE_FREE) ||
 	    (service->srvstate == VCHIQ_SRVSTATE_LISTENING) ||
@@ -2954,8 +2969,9 @@ vchiq_remove_service(struct vchiq_instance *instance, unsigned int handle)
 	if (!service)
 		return -EINVAL;
 
-	vchiq_log_debug(service->state->dev, VCHIQ_CORE, "%d: remove_service:%d",
-			service->state->id, service->localport);
+	dev_dbg(service->state->dev, "%s: %s: %d: remove_service:%d\n",
+		log_cat(VCHIQ_CORE), log_type(DEBUG),
+		service->state->id, service->localport);
 
 	if (service->srvstate == VCHIQ_SRVSTATE_FREE) {
 		vchiq_service_put(service);
@@ -3099,9 +3115,10 @@ int vchiq_bulk_transfer(struct vchiq_instance *instance, unsigned int handle,
 	 */
 	wmb();
 
-	vchiq_log_debug(state->dev, VCHIQ_CORE, "%d: bt (%d->%d) %cx %x@%pad %pK",
-			state->id, service->localport, service->remoteport,
-			dir_char, size, &bulk->data, userdata);
+	dev_dbg(state->dev, "%s: %s: %d: bt (%d->%d) %cx %x@%pad %pK\n",
+		log_cat(VCHIQ_CORE), log_type(DEBUG),
+		state->id, service->localport, service->remoteport,
+		dir_char, size, &bulk->data, userdata);
 
 	/*
 	 * The slot mutex must be held when the service is being closed, so
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
index de0d49e58e39..d66b60a9f6da 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
@@ -71,11 +71,6 @@ static inline const char *log_type(enum vchiq_log_type t)
 	return types_str[t];
 };
 
-#ifndef vchiq_log_debug
-#define vchiq_log_debug(dev, cat, fmt, ...) \
-	do { dev_dbg(dev, "%s debug: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
-#endif
-
 #define VCHIQ_SLOT_MASK        (VCHIQ_SLOT_SIZE - 1)
 #define VCHIQ_SLOT_QUEUE_MASK  (VCHIQ_MAX_SLOTS_PER_SIDE - 1)
 #define VCHIQ_SLOT_ZERO_SLOTS  DIV_ROUND_UP(sizeof(struct vchiq_slot_zero), \
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
index 7f5bba005653..a64d6333cf0a 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_dev.c
@@ -47,9 +47,10 @@ user_service_free(void *userdata)
 
 static void close_delivered(struct user_service *user_service)
 {
-	vchiq_log_debug(user_service->service->state->dev, VCHIQ_ARM,
-			"%s(handle=%x)",
-			__func__, user_service->service->handle);
+	dev_dbg(user_service->service->state->dev,
+		"%s: %s: %s(handle=%x)\n",
+		log_cat(VCHIQ_ARM), log_type(DEBUG),
+		__func__, user_service->service->handle);
 
 	if (user_service->close_pending) {
 		/* Allow the underlying service to be culled */
@@ -235,8 +236,9 @@ static int vchiq_ioc_dequeue_message(struct vchiq_instance *instance,
 			spin_unlock(&msg_queue_spinlock);
 			DEBUG_TRACE(DEQUEUE_MESSAGE_LINE);
 			if (wait_for_completion_interruptible(&user_service->insert_event)) {
-				vchiq_log_debug(service->state->dev, VCHIQ_ARM,
-						"DEQUEUE_MESSAGE interrupted");
+				dev_dbg(service->state->dev,
+					"%s: %s: DEQUEUE_MESSAGE interrupted\n",
+					log_cat(VCHIQ_ARM), log_type(DEBUG));
 				ret = -EINTR;
 				break;
 			}
@@ -325,8 +327,9 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 			ret = -ESRCH;
 			goto out;
 		}
-		vchiq_log_debug(service->state->dev, VCHIQ_ARM,
-				"found bulk_waiter %pK for pid %d", waiter, current->pid);
+		dev_dbg(service->state->dev, "%s: %s: found bulk_waiter %pK for pid %d\n",
+			log_cat(VCHIQ_ARM), log_type(DEBUG),
+			waiter, current->pid);
 		userdata = &waiter->bulk_waiter;
 	} else {
 		userdata = args->userdata;
@@ -357,8 +360,8 @@ static int vchiq_irq_queue_bulk_tx_rx(struct vchiq_instance *instance,
 		mutex_lock(&instance->bulk_waiter_list_mutex);
 		list_add(&waiter->list, &instance->bulk_waiter_list);
 		mutex_unlock(&instance->bulk_waiter_list_mutex);
-		vchiq_log_debug(service->state->dev, VCHIQ_ARM,
-				"saved bulk_waiter %pK for pid %d", waiter, current->pid);
+		dev_dbg(service->state->dev, "%s: %s: saved bulk_waiter %pK for pid %d\n",
+			log_cat(VCHIQ_ARM), log_type(DEBUG), waiter, current->pid);
 
 		ret = put_user(mode_waiting, mode);
 	}
@@ -457,8 +460,9 @@ static int vchiq_ioc_await_completion(struct vchiq_instance *instance,
 		mutex_lock(&instance->completion_mutex);
 		if (rc) {
 			DEBUG_TRACE(AWAIT_COMPLETION_LINE);
-			vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
-					"AWAIT_COMPLETION interrupted");
+			dev_dbg(instance->state->dev,
+				"%s: %s: AWAIT_COMPLETION interrupted\n",
+				log_cat(VCHIQ_ARM), log_type(DEBUG));
 			ret = -EINTR;
 			goto out;
 		}
@@ -873,10 +877,11 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	}
 
 	if (!status && (ret < 0) && (ret != -EINTR) && (ret != -EWOULDBLOCK)) {
-		vchiq_log_debug(instance->state->dev, VCHIQ_ARM,
-				"  ioctl instance %pK, cmd %s -> status %d, %ld",
-				instance, (_IOC_NR(cmd) <= VCHIQ_IOC_MAX) ?
-				ioctl_names[_IOC_NR(cmd)] : "<invalid>", status, ret);
+		dev_dbg(instance->state->dev,
+			"%s: %s: ioctl instance %pK, cmd %s -> status %d, %ld\n",
+			log_cat(VCHIQ_ARM), log_type(DEBUG),
+			instance, (_IOC_NR(cmd) <= VCHIQ_IOC_MAX) ?
+			ioctl_names[_IOC_NR(cmd)] : "<invalid>", status, ret);
 	} else {
 		dev_dbg(instance->state->dev,
 			"%s: %s: ioctl instance %pK, cmd %s -> status %d\n, %ld\n",
@@ -1181,7 +1186,8 @@ static int vchiq_open(struct inode *inode, struct file *file)
 		return -ENOTCONN;
 	}
 
-	vchiq_log_debug(state->dev, VCHIQ_ARM, "vchiq_open");
+	dev_dbg(state->dev, "%s: %s: vchiq_open\n",
+		log_cat(VCHIQ_ARM), log_type(DEBUG));
 
 	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
 	if (!instance)
@@ -1211,8 +1217,9 @@ static int vchiq_release(struct inode *inode, struct file *file)
 	int ret = 0;
 	int i;
 
-	vchiq_log_debug(state->dev, VCHIQ_ARM, "%s: instance=%lx", __func__,
-			(unsigned long)instance);
+	dev_dbg(state->dev, "%s: %s: %s: instance=%lx\n",
+		log_cat(VCHIQ_ARM), log_type(DEBUG),
+		__func__, (unsigned long)instance);
 
 	if (!state) {
 		ret = -EPERM;
-- 
2.41.0

Re: [PATCH 5/9] staging: vc04_services: Do not pass NULL to vchiq_log_error()

[Laurent Pinchart]

Hi Umang,

Thank you for the patch.

On Tue, Nov 07, 2023 at 04:51:52AM -0500, Umang Jain wrote:
> vchiq_add_connected_callback() logs using vchiq_log_error() macro,
> but passes NULL instead of a struct device pointer. Fix it.
> 
> vchiq_add_connected_callback() is not used anywhere in the vc04_services
> as of now. It will be used when we add new drivers(VC shared memory and
> bcm2835-isp), hence it kept as it is for now.
> 
> Fixes: 1d8915cf8899 ("staging: vc04: Convert vchiq_log_error() to use dynamic debug")
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../vc04_services/interface/vchiq_arm/vchiq_connected.c       | 4 ++--
>  .../vc04_services/interface/vchiq_arm/vchiq_connected.h       | 4 +++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> index b3928bd8c9c6..21f9fa1a1713 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> @@ -27,7 +27,7 @@ static void connected_init(void)
>   * be made immediately, otherwise it will be deferred until
>   * vchiq_call_connected_callbacks is called.
>   */
> -void vchiq_add_connected_callback(void (*callback)(void))
> +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))

You're changing the prototype of the function, but the patch doesn't
update any user. If the function is unused, it looks like you can drop
it instead. Looking at the rest of the vchiq_connected.c file, I think
you can actually drop the whole file.

>  {
>  	connected_init();
>  
> @@ -39,7 +39,7 @@ void vchiq_add_connected_callback(void (*callback)(void))
>  		callback();
>  	} else {
>  		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
> -			vchiq_log_error(NULL, VCHIQ_CORE,
> +			vchiq_log_error(&device->dev, VCHIQ_CORE,
>  					"There already %d callback registered - please increase MAX_CALLBACKS",
>  					g_num_deferred_callbacks);
>  		} else {
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> index 4caf5e30099d..e4ed56446f8a 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> @@ -1,10 +1,12 @@
>  /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
>  /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
>  
> +#include "vchiq_bus.h"
> +
>  #ifndef VCHIQ_CONNECTED_H
>  #define VCHIQ_CONNECTED_H
>  
> -void vchiq_add_connected_callback(void (*callback)(void));
> +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
>  void vchiq_call_connected_callbacks(void);
>  
>  #endif /* VCHIQ_CONNECTED_H */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/9] staging: vc04_services: Do not pass NULL to vchiq_log_error()

[Umang Jain]

Hi Laurent,

On 11/7/23 5:55 PM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Tue, Nov 07, 2023 at 04:51:52AM -0500, Umang Jain wrote:
>> vchiq_add_connected_callback() logs using vchiq_log_error() macro,
>> but passes NULL instead of a struct device pointer. Fix it.
>>
>> vchiq_add_connected_callback() is not used anywhere in the vc04_services
>> as of now. It will be used when we add new drivers(VC shared memory and
>> bcm2835-isp), hence it kept as it is for now.
>>
>> Fixes: 1d8915cf8899 ("staging: vc04: Convert vchiq_log_error() to use dynamic debug")
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   .../vc04_services/interface/vchiq_arm/vchiq_connected.c       | 4 ++--
>>   .../vc04_services/interface/vchiq_arm/vchiq_connected.h       | 4 +++-
>>   2 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>> index b3928bd8c9c6..21f9fa1a1713 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>> @@ -27,7 +27,7 @@ static void connected_init(void)
>>    * be made immediately, otherwise it will be deferred until
>>    * vchiq_call_connected_callbacks is called.
>>    */
>> -void vchiq_add_connected_callback(void (*callback)(void))
>> +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
> You're changing the prototype of the function, but the patch doesn't
> update any user. If the function is unused, it looks like you can drop
> it instead. Looking at the rest of the vchiq_connected.c file, I think
> you can actually drop the whole file.

I mentioned in the commit message of this patch. There will be users in 
the near future.
>
>>   {
>>   	connected_init();
>>   
>> @@ -39,7 +39,7 @@ void vchiq_add_connected_callback(void (*callback)(void))
>>   		callback();
>>   	} else {
>>   		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
>> -			vchiq_log_error(NULL, VCHIQ_CORE,
>> +			vchiq_log_error(&device->dev, VCHIQ_CORE,
>>   					"There already %d callback registered - please increase MAX_CALLBACKS",
>>   					g_num_deferred_callbacks);
>>   		} else {
>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>> index 4caf5e30099d..e4ed56446f8a 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>> @@ -1,10 +1,12 @@
>>   /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
>>   /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
>>   
>> +#include "vchiq_bus.h"
>> +
>>   #ifndef VCHIQ_CONNECTED_H
>>   #define VCHIQ_CONNECTED_H
>>   
>> -void vchiq_add_connected_callback(void (*callback)(void));
>> +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
>>   void vchiq_call_connected_callbacks(void);
>>   
>>   #endif /* VCHIQ_CONNECTED_H */

Re: [PATCH 4/9] staging: vc04_services: Shorten helper function name

[Kieran Bingham]

Quoting Umang Jain (2023-11-07 09:51:51)
> Shorten the helper log_category_str() to log_cat().
> Going forwards, this will be used at multiple places with
> dev_dbg().
> 
> No functiional changes intended in this patch.

s/functiional/functional/

But if we're replacing this like this - maybe we don't need a log_cat -
would it make more sense to just make the VCHIQ_CORE VCHIQ_SUSPEND etc
just a #define "vchiq_core" ... and remove the indirection? Are they
used as enum's anywhere now still?

--
Kieran


> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../vc04_services/interface/vchiq_arm/vchiq_core.h     | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> index 161358db457c..cc7bb6ca832a 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -39,7 +39,7 @@ enum vchiq_log_category {
>         VCHIQ_SUSPEND,
>  };
>  
> -static inline const char *log_category_str(enum vchiq_log_category c)
> +static inline const char *log_cat(enum vchiq_log_category c)
>  {
>         static const char * const strings[] = {
>                 "vchiq_arm",
> @@ -54,19 +54,19 @@ static inline const char *log_category_str(enum vchiq_log_category c)
>  
>  #ifndef vchiq_log_error
>  #define vchiq_log_error(dev, cat, fmt, ...) \
> -       do { dev_dbg(dev, "%s error: " fmt, log_category_str(cat), ##__VA_ARGS__); } while (0)
> +       do { dev_dbg(dev, "%s error: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
>  #endif
>  #ifndef vchiq_log_warning
>  #define vchiq_log_warning(dev, cat, fmt, ...) \
> -       do { dev_dbg(dev, "%s warning: " fmt, log_category_str(cat), ##__VA_ARGS__); } while (0)
> +       do { dev_dbg(dev, "%s warning: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
>  #endif
>  #ifndef vchiq_log_debug
>  #define vchiq_log_debug(dev, cat, fmt, ...) \
> -       do { dev_dbg(dev, "%s debug: " fmt, log_category_str(cat), ##__VA_ARGS__); } while (0)
> +       do { dev_dbg(dev, "%s debug: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
>  #endif
>  #ifndef vchiq_log_trace
>  #define vchiq_log_trace(dev, cat, fmt, ...) \
> -       do { dev_dbg(dev, "%s trace: " fmt, log_category_str(cat), ##__VA_ARGS__); } while (0)
> +       do { dev_dbg(dev, "%s trace: " fmt, log_cat(cat), ##__VA_ARGS__); } while (0)
>  #endif
>  
>  #define VCHIQ_SLOT_MASK        (VCHIQ_SLOT_SIZE - 1)
> -- 
> 2.41.0
>

Re: [PATCH 5/9] staging: vc04_services: Do not pass NULL to vchiq_log_error()

[Laurent Pinchart]

On Tue, Nov 07, 2023 at 02:25:52PM +0200, Laurent Pinchart wrote:
> On Tue, Nov 07, 2023 at 04:51:52AM -0500, Umang Jain wrote:
> > vchiq_add_connected_callback() logs using vchiq_log_error() macro,
> > but passes NULL instead of a struct device pointer. Fix it.
> > 
> > vchiq_add_connected_callback() is not used anywhere in the vc04_services
> > as of now. It will be used when we add new drivers(VC shared memory and
> > bcm2835-isp), hence it kept as it is for now.
> > 
> > Fixes: 1d8915cf8899 ("staging: vc04: Convert vchiq_log_error() to use dynamic debug")
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  .../vc04_services/interface/vchiq_arm/vchiq_connected.c       | 4 ++--
> >  .../vc04_services/interface/vchiq_arm/vchiq_connected.h       | 4 +++-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > index b3928bd8c9c6..21f9fa1a1713 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > @@ -27,7 +27,7 @@ static void connected_init(void)
> >   * be made immediately, otherwise it will be deferred until
> >   * vchiq_call_connected_callbacks is called.
> >   */
> > -void vchiq_add_connected_callback(void (*callback)(void))
> > +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
> 
> You're changing the prototype of the function, but the patch doesn't
> update any user. If the function is unused, it looks like you can drop
> it instead. Looking at the rest of the vchiq_connected.c file, I think
> you can actually drop the whole file.

Except that the vc-sm-cma driver will use it. I'm curious though, that
driver will have no way to acquire a pointer to a vchiq_device, so I
don't see how this will be usable. pr_err() may be a better pick here.

> >  {
> >  	connected_init();
> >  
> > @@ -39,7 +39,7 @@ void vchiq_add_connected_callback(void (*callback)(void))
> >  		callback();
> >  	} else {
> >  		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
> > -			vchiq_log_error(NULL, VCHIQ_CORE,
> > +			vchiq_log_error(&device->dev, VCHIQ_CORE,
> >  					"There already %d callback registered - please increase MAX_CALLBACKS",
> >  					g_num_deferred_callbacks);
> >  		} else {
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> > index 4caf5e30099d..e4ed56446f8a 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> > @@ -1,10 +1,12 @@
> >  /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> >  /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
> >  
> > +#include "vchiq_bus.h"
> > +
> >  #ifndef VCHIQ_CONNECTED_H
> >  #define VCHIQ_CONNECTED_H
> >  
> > -void vchiq_add_connected_callback(void (*callback)(void));
> > +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
> >  void vchiq_call_connected_callbacks(void);
> >  
> >  #endif /* VCHIQ_CONNECTED_H */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/9] staging: vc04_services: Do not pass NULL to vchiq_log_error()

[Laurent Pinchart]

On Tue, Nov 07, 2023 at 06:01:58PM +0530, Umang Jain wrote:
> On 11/7/23 5:55 PM, Laurent Pinchart wrote:
> > On Tue, Nov 07, 2023 at 04:51:52AM -0500, Umang Jain wrote:
> >> vchiq_add_connected_callback() logs using vchiq_log_error() macro,
> >> but passes NULL instead of a struct device pointer. Fix it.
> >>
> >> vchiq_add_connected_callback() is not used anywhere in the vc04_services
> >> as of now. It will be used when we add new drivers(VC shared memory and
> >> bcm2835-isp), hence it kept as it is for now.
> >>
> >> Fixes: 1d8915cf8899 ("staging: vc04: Convert vchiq_log_error() to use dynamic debug")
> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> >> ---
> >>   .../vc04_services/interface/vchiq_arm/vchiq_connected.c       | 4 ++--
> >>   .../vc04_services/interface/vchiq_arm/vchiq_connected.h       | 4 +++-
> >>   2 files changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> >> index b3928bd8c9c6..21f9fa1a1713 100644
> >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> >> @@ -27,7 +27,7 @@ static void connected_init(void)
> >>    * be made immediately, otherwise it will be deferred until
> >>    * vchiq_call_connected_callbacks is called.
> >>    */
> >> -void vchiq_add_connected_callback(void (*callback)(void))
> >> +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
> >
> > You're changing the prototype of the function, but the patch doesn't
> > update any user. If the function is unused, it looks like you can drop
> > it instead. Looking at the rest of the vchiq_connected.c file, I think
> > you can actually drop the whole file.
> 
> I mentioned in the commit message of this patch. There will be users in 
> the near future.

Yes sorry I missed it, I should have read it more carefully. My fault.

> >>   {
> >>   	connected_init();
> >>   
> >> @@ -39,7 +39,7 @@ void vchiq_add_connected_callback(void (*callback)(void))
> >>   		callback();
> >>   	} else {
> >>   		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
> >> -			vchiq_log_error(NULL, VCHIQ_CORE,
> >> +			vchiq_log_error(&device->dev, VCHIQ_CORE,
> >>   					"There already %d callback registered - please increase MAX_CALLBACKS",
> >>   					g_num_deferred_callbacks);
> >>   		} else {
> >> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> >> index 4caf5e30099d..e4ed56446f8a 100644
> >> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
> >> @@ -1,10 +1,12 @@
> >>   /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> >>   /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
> >>   
> >> +#include "vchiq_bus.h"
> >> +
> >>   #ifndef VCHIQ_CONNECTED_H
> >>   #define VCHIQ_CONNECTED_H
> >>   
> >> -void vchiq_add_connected_callback(void (*callback)(void));
> >> +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
> >>   void vchiq_call_connected_callbacks(void);
> >>   
> >>   #endif /* VCHIQ_CONNECTED_H */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/9] staging: vc04_services: Do not pass NULL to vchiq_log_error()

[Stefan Wahren]

Hi Umang,

Am 07.11.23 um 13:36 schrieb Laurent Pinchart:
> On Tue, Nov 07, 2023 at 02:25:52PM +0200, Laurent Pinchart wrote:
>> On Tue, Nov 07, 2023 at 04:51:52AM -0500, Umang Jain wrote:
>>> vchiq_add_connected_callback() logs using vchiq_log_error() macro,
>>> but passes NULL instead of a struct device pointer. Fix it.
>>>
>>> vchiq_add_connected_callback() is not used anywhere in the vc04_services
>>> as of now. It will be used when we add new drivers(VC shared memory and
>>> bcm2835-isp), hence it kept as it is for now.
>>>
>>> Fixes: 1d8915cf8899 ("staging: vc04: Convert vchiq_log_error() to use dynamic debug")
>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>> ---
>>>   .../vc04_services/interface/vchiq_arm/vchiq_connected.c       | 4 ++--
>>>   .../vc04_services/interface/vchiq_arm/vchiq_connected.h       | 4 +++-
>>>   2 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>>> index b3928bd8c9c6..21f9fa1a1713 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>>> @@ -27,7 +27,7 @@ static void connected_init(void)
>>>    * be made immediately, otherwise it will be deferred until
>>>    * vchiq_call_connected_callbacks is called.
>>>    */
>>> -void vchiq_add_connected_callback(void (*callback)(void))
>>> +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
>>
>> You're changing the prototype of the function, but the patch doesn't
>> update any user. If the function is unused, it looks like you can drop
>> it instead. Looking at the rest of the vchiq_connected.c file, I think
>> you can actually drop the whole file.
>
> Except that the vc-sm-cma driver will use it. I'm curious though, that
> driver will have no way to acquire a pointer to a vchiq_device, so I
> don't see how this will be usable. pr_err() may be a better pick here.

in case there is currently no user, but vc-sm-cma will be then i'm
convinced that moving this driver into staging before vchiq is out is a
good idea.

AFAIK your main goal is to get bcm2835-isp into mainline, so it would be
faster to have it in staging, so everyone could fix the style issues.

My initial concern about importing new drivers was that after moving it
into staging nobody wants to finish the job. But currently i've a good
feeling that vchiq is on a good way.

Best regards

>
>>>   {
>>>   	connected_init();
>>>
>>> @@ -39,7 +39,7 @@ void vchiq_add_connected_callback(void (*callback)(void))
>>>   		callback();
>>>   	} else {
>>>   		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
>>> -			vchiq_log_error(NULL, VCHIQ_CORE,
>>> +			vchiq_log_error(&device->dev, VCHIQ_CORE,
>>>   					"There already %d callback registered - please increase MAX_CALLBACKS",
>>>   					g_num_deferred_callbacks);
>>>   		} else {
>>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>>> index 4caf5e30099d..e4ed56446f8a 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>>> @@ -1,10 +1,12 @@
>>>   /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
>>>   /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
>>>
>>> +#include "vchiq_bus.h"
>>> +
>>>   #ifndef VCHIQ_CONNECTED_H
>>>   #define VCHIQ_CONNECTED_H
>>>
>>> -void vchiq_add_connected_callback(void (*callback)(void));
>>> +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
>>>   void vchiq_call_connected_callbacks(void);
>>>
>>>   #endif /* VCHIQ_CONNECTED_H */
>

Re: [PATCH 5/9] staging: vc04_services: Do not pass NULL to vchiq_log_error()

[Umang Jain]

Hi Stefan

On 11/10/23 3:51 PM, Stefan Wahren wrote:
> Hi Umang,
>
> Am 07.11.23 um 13:36 schrieb Laurent Pinchart:
>> On Tue, Nov 07, 2023 at 02:25:52PM +0200, Laurent Pinchart wrote:
>>> On Tue, Nov 07, 2023 at 04:51:52AM -0500, Umang Jain wrote:
>>>> vchiq_add_connected_callback() logs using vchiq_log_error() macro,
>>>> but passes NULL instead of a struct device pointer. Fix it.
>>>>
>>>> vchiq_add_connected_callback() is not used anywhere in the 
>>>> vc04_services
>>>> as of now. It will be used when we add new drivers(VC shared memory 
>>>> and
>>>> bcm2835-isp), hence it kept as it is for now.
>>>>
>>>> Fixes: 1d8915cf8899 ("staging: vc04: Convert vchiq_log_error() to 
>>>> use dynamic debug")
>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>>> ---
>>>> .../vc04_services/interface/vchiq_arm/vchiq_connected.c | 4 ++--
>>>> .../vc04_services/interface/vchiq_arm/vchiq_connected.h | 4 +++-
>>>>   2 files changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git 
>>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c 
>>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>>>> index b3928bd8c9c6..21f9fa1a1713 100644
>>>> --- 
>>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>>>> +++ 
>>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>>>> @@ -27,7 +27,7 @@ static void connected_init(void)
>>>>    * be made immediately, otherwise it will be deferred until
>>>>    * vchiq_call_connected_callbacks is called.
>>>>    */
>>>> -void vchiq_add_connected_callback(void (*callback)(void))
>>>> +void vchiq_add_connected_callback(struct vchiq_device *device, 
>>>> void (*callback)(void))
>>>
>>> You're changing the prototype of the function, but the patch doesn't
>>> update any user. If the function is unused, it looks like you can drop
>>> it instead. Looking at the rest of the vchiq_connected.c file, I think
>>> you can actually drop the whole file.
>>
>> Except that the vc-sm-cma driver will use it. I'm curious though, that
>> driver will have no way to acquire a pointer to a vchiq_device, so I
>> don't see how this will be usable. pr_err() may be a better pick here.
>
> in case there is currently no user, but vc-sm-cma will be then i'm
> convinced that moving this driver into staging before vchiq is out is a
> good idea.
>
> AFAIK your main goal is to get bcm2835-isp into mainline, so it would be
> faster to have it in staging, so everyone could fix the style issues.

This would really *really* help me. In the sense that, once the 
bcm2835-isp is in staging, I wouldn't need to keep the bcm2835-isp 
patches rebased on my local computer and keep re-sending the series 
again on the list.

If the bcm2835-isp series looks in good shape, I prefer getting them in 
staging and my focus will solely be on continuing working the de-staging 
part, so that the churn is minimized on my end.
>
> My initial concern about importing new drivers was that after moving it
> into staging nobody wants to finish the job. But currently i've a good
> feeling that vchiq is on a good way.
>
> Best regards
>
>>
>>>>   {
>>>>       connected_init();
>>>>
>>>> @@ -39,7 +39,7 @@ void vchiq_add_connected_callback(void 
>>>> (*callback)(void))
>>>>           callback();
>>>>       } else {
>>>>           if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
>>>> -            vchiq_log_error(NULL, VCHIQ_CORE,
>>>> +            vchiq_log_error(&device->dev, VCHIQ_CORE,
>>>>                       "There already %d callback registered - 
>>>> please increase MAX_CALLBACKS",
>>>>                       g_num_deferred_callbacks);
>>>>           } else {
>>>> diff --git 
>>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h 
>>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>>>> index 4caf5e30099d..e4ed56446f8a 100644
>>>> --- 
>>>> a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>>>> +++ 
>>>> b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>>>> @@ -1,10 +1,12 @@
>>>>   /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
>>>>   /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
>>>>
>>>> +#include "vchiq_bus.h"
>>>> +
>>>>   #ifndef VCHIQ_CONNECTED_H
>>>>   #define VCHIQ_CONNECTED_H
>>>>
>>>> -void vchiq_add_connected_callback(void (*callback)(void));
>>>> +void vchiq_add_connected_callback(struct vchiq_device *device, 
>>>> void (*callback)(void));
>>>>   void vchiq_call_connected_callbacks(void);
>>>>
>>>>   #endif /* VCHIQ_CONNECTED_H */
>>

Re: [PATCH 5/9] staging: vc04_services: Do not pass NULL to vchiq_log_error()

[Greg Kroah-Hartman]

On Tue, Nov 07, 2023 at 06:01:58PM +0530, Umang Jain wrote:
> Hi Laurent,
> 
> On 11/7/23 5:55 PM, Laurent Pinchart wrote:
> > Hi Umang,
> > 
> > Thank you for the patch.
> > 
> > On Tue, Nov 07, 2023 at 04:51:52AM -0500, Umang Jain wrote:
> > > vchiq_add_connected_callback() logs using vchiq_log_error() macro,
> > > but passes NULL instead of a struct device pointer. Fix it.
> > > 
> > > vchiq_add_connected_callback() is not used anywhere in the vc04_services
> > > as of now. It will be used when we add new drivers(VC shared memory and
> > > bcm2835-isp), hence it kept as it is for now.
> > > 
> > > Fixes: 1d8915cf8899 ("staging: vc04: Convert vchiq_log_error() to use dynamic debug")
> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > ---
> > >   .../vc04_services/interface/vchiq_arm/vchiq_connected.c       | 4 ++--
> > >   .../vc04_services/interface/vchiq_arm/vchiq_connected.h       | 4 +++-
> > >   2 files changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > > index b3928bd8c9c6..21f9fa1a1713 100644
> > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > > @@ -27,7 +27,7 @@ static void connected_init(void)
> > >    * be made immediately, otherwise it will be deferred until
> > >    * vchiq_call_connected_callbacks is called.
> > >    */
> > > -void vchiq_add_connected_callback(void (*callback)(void))
> > > +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
> > You're changing the prototype of the function, but the patch doesn't
> > update any user. If the function is unused, it looks like you can drop
> > it instead. Looking at the rest of the vchiq_connected.c file, I think
> > you can actually drop the whole file.
> 
> I mentioned in the commit message of this patch. There will be users in the
> near future.

We write code for today, not any potential users in the future.  If it's
not used now, let's delete it and then when we need it in the future,
you can add it back then.

thanks,

greg k-h

Re: [PATCH 1/9] staging: vc04_services: vchiq_core: Log through struct vchiq_instance

[Greg Kroah-Hartman]

On Tue, Nov 07, 2023 at 04:51:48AM -0500, Umang Jain wrote:
> The handle_to_service() helper can return NULL, so `service` pointer
> can indeed be set to NULL. So, do not log through service pointer
> (which will cause NULL-deference), instead, use the vchiq_instance
> function argument to get access to the struct device.
> 
> Fixes: f67af5940d6d("staging: vc04: Convert(and rename) vchiq_log_info() to use dynamic debug")

You need a space there :(

Re: [PATCH 1/9] staging: vc04_services: vchiq_core: Log through struct vchiq_instance

[Greg Kroah-Hartman]

On Tue, Nov 07, 2023 at 04:51:48AM -0500, Umang Jain wrote:
> The handle_to_service() helper can return NULL, so `service` pointer
> can indeed be set to NULL. So, do not log through service pointer
> (which will cause NULL-deference), instead, use the vchiq_instance
> function argument to get access to the struct device.
> 
> Fixes: f67af5940d6d("staging: vc04: Convert(and rename) vchiq_log_info() to use dynamic debug")
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Ricardo B. Marliere <ricardo@marliere.net>
> ---
>  .../staging/vc04_services/interface/vchiq_arm/vchiq_core.c  | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Also, please send this as a stand-alone patch so we can apply it for
6.7-final.

thanks,

greg k-h

Re: [PATCH 2/9] staging: vc04_services: Log using pr_err() when vchiq_state is unset

[Greg Kroah-Hartman]

On Tue, Nov 07, 2023 at 04:51:49AM -0500, Umang Jain wrote:
> In cases, where the global vchiq state is still unset, we cannot log
> to dynamic debug (access to struct device is needed, hence potential
> NULL de-reference). Log using pr_err() instead.

No, something is wrong here, don't do that.

> In vchiq_initialise(), remove the 'goto' because it is just again
> trying to log to dynamic debug. Simply return with -ENNOTCONN after
> logging to pr_err().
> 
> In vchiq_open(), move the vchiq_log_debug() after the state pointer
> null check.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Ricardo B. Marliere <ricardo@marliere.net>
> ---
>  .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c  | 6 ++----
>  .../staging/vc04_services/interface/vchiq_arm/vchiq_dev.c  | 7 +++----
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 9fb8f657cc78..9fb3e240d9de 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -687,10 +687,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
>  		usleep_range(500, 600);
>  	}
>  	if (i == VCHIQ_INIT_RETRIES) {
> -		vchiq_log_error(state->dev, VCHIQ_CORE, "%s: videocore not initialized\n",
> -				__func__);
> -		ret = -ENOTCONN;
> -		goto failed;
> +		pr_err("%s: videocore not initialized\n", __func__);
> +		return -ENOTCONN;

Here's a good reason to get rid of the crazy "this subsystem is special
so let us use a custom logging macro" logic.

Convert everything to just use real dev_*() calls so it makes it obvious
what is happening and how it all is working.  It will save you from
doing stuff like this in the future as you will "know" that there isn't
a valid device pointer here.

thanks,

greg k-h

Re: [PATCH 6/9] staging: vc04_services: Drop vchiq_log_error() in favour of dev_dbg

[Greg Kroah-Hartman]

On Tue, Nov 07, 2023 at 04:51:53AM -0500, Umang Jain wrote:
> Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
> of dev_dbg() directly.
> 
> Add a new enum vchiq_log_type and log_type() helper to faciliate the
> type of logging in dev_dbg(). This will help to determine the type of
> logging("error", "warning", "debug", "trace") to dynamic debug.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../interface/vchiq_arm/vchiq_arm.c           |  54 ++++----
>  .../interface/vchiq_arm/vchiq_connected.c     |   6 +-
>  .../interface/vchiq_arm/vchiq_core.c          | 126 ++++++++++--------
>  .../interface/vchiq_arm/vchiq_core.h          |  23 +++-
>  .../interface/vchiq_arm/vchiq_dev.c           |  47 ++++---
>  5 files changed, 143 insertions(+), 113 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 9fb3e240d9de..2cb2a6503058 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -696,8 +696,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
>  
>  	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
>  	if (!instance) {
> -		vchiq_log_error(state->dev, VCHIQ_CORE,
> -				"%s: error allocating vchiq instance\n", __func__);
> +		dev_dbg(state->dev, "%s: %s: %s: error allocating vchiq instance\n",
> +			log_cat(VCHIQ_CORE), log_type(ERROR), __func__);

All dev_dbg() calls have __func__ in them automatically, you never need
to duplicate it again as that's redundant :(

thanks,

greg k-h

Re: [PATCH 7/9] staging: vc04_services: Drop vchiq_log_warning() in favour of dev_dbg

[Greg Kroah-Hartman]

On Tue, Nov 07, 2023 at 04:51:54AM -0500, Umang Jain wrote:
> Drop vchiq_log_warning() macro which wraps dev_dbg(). Introduce the usage
> of dev_dbg() directly.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../interface/vchiq_arm/vchiq_arm.c           | 35 ++++++++++---------
>  .../interface/vchiq_arm/vchiq_core.c          | 33 +++++++++--------
>  .../interface/vchiq_arm/vchiq_core.h          |  4 ---
>  3 files changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 2cb2a6503058..bc0ee8b9d1c3 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -690,8 +690,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
>  		pr_err("%s: videocore not initialized\n", __func__);
>  		return -ENOTCONN;
>  	} else if (i > 0) {
> -		vchiq_log_warning(state->dev, VCHIQ_CORE,
> -				  "%s: videocore initialized after %d retries\n", __func__, i);
> +		dev_dbg(state->dev, "%s: %s: %s: videocore initialized after %d retries\n",
> +			log_cat(VCHIQ_CORE), log_type(WARN), __func__, i);

what is the log_type(WARN) stuff for?  How does that work?  You should
just use the normal dev_dbg() call and don't try to add new prefixes to
the message as that will just confuse the common parsing tools we have
for these types of things.

Also, as before, __func__ is redundant, and now you see it twice in the
output :(

thanks,

greg k-h

Re: [PATCH 4/9] staging: vc04_services: Shorten helper function name

[Greg Kroah-Hartman]

On Tue, Nov 07, 2023 at 04:51:51AM -0500, Umang Jain wrote:
> Shorten the helper log_category_str() to log_cat().
> Going forwards, this will be used at multiple places with
> dev_dbg().
> 
> No functiional changes intended in this patch.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  .../vc04_services/interface/vchiq_arm/vchiq_core.h     | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> index 161358db457c..cc7bb6ca832a 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_core.h
> @@ -39,7 +39,7 @@ enum vchiq_log_category {
>  	VCHIQ_SUSPEND,
>  };
>  
> -static inline const char *log_category_str(enum vchiq_log_category c)
> +static inline const char *log_cat(enum vchiq_log_category c)

This is a bad name for a subsystem-wide function name, why is this
needed at all?

thanks,

greg k-h

Re: [PATCH 5/9] staging: vc04_services: Do not pass NULL to vchiq_log_error()

[Laurent Pinchart]

On Thu, Nov 23, 2023 at 12:57:14PM +0000, Greg Kroah-Hartman wrote:
> On Tue, Nov 07, 2023 at 06:01:58PM +0530, Umang Jain wrote:
> > Hi Laurent,
> > 
> > On 11/7/23 5:55 PM, Laurent Pinchart wrote:
> > > Hi Umang,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Tue, Nov 07, 2023 at 04:51:52AM -0500, Umang Jain wrote:
> > > > vchiq_add_connected_callback() logs using vchiq_log_error() macro,
> > > > but passes NULL instead of a struct device pointer. Fix it.
> > > > 
> > > > vchiq_add_connected_callback() is not used anywhere in the vc04_services
> > > > as of now. It will be used when we add new drivers(VC shared memory and
> > > > bcm2835-isp), hence it kept as it is for now.
> > > > 
> > > > Fixes: 1d8915cf8899 ("staging: vc04: Convert vchiq_log_error() to use dynamic debug")
> > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > ---
> > > >   .../vc04_services/interface/vchiq_arm/vchiq_connected.c       | 4 ++--
> > > >   .../vc04_services/interface/vchiq_arm/vchiq_connected.h       | 4 +++-
> > > >   2 files changed, 5 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > > > index b3928bd8c9c6..21f9fa1a1713 100644
> > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > > > @@ -27,7 +27,7 @@ static void connected_init(void)
> > > >    * be made immediately, otherwise it will be deferred until
> > > >    * vchiq_call_connected_callbacks is called.
> > > >    */
> > > > -void vchiq_add_connected_callback(void (*callback)(void))
> > > > +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
> > > You're changing the prototype of the function, but the patch doesn't
> > > update any user. If the function is unused, it looks like you can drop
> > > it instead. Looking at the rest of the vchiq_connected.c file, I think
> > > you can actually drop the whole file.
> > 
> > I mentioned in the commit message of this patch. There will be users in the
> > near future.
> 
> We write code for today, not any potential users in the future.  If it's
> not used now, let's delete it and then when we need it in the future,
> you can add it back then.

What was the near future on November the 7th is now the present :-)
Umang has sent a new version of the ISP driver that uses this API. I
think this kind of near future is fine, and we routinely do the same
during review when large feature are split across multiple series
(whether those series are merged together or independently is another
question of course).

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/9] staging: vc04_services: vchiq_core: Log through struct vchiq_instance

[Laurent Pinchart]

On Thu, Nov 23, 2023 at 12:57:33PM +0000, Greg Kroah-Hartman wrote:
> On Tue, Nov 07, 2023 at 04:51:48AM -0500, Umang Jain wrote:
> > The handle_to_service() helper can return NULL, so `service` pointer
> > can indeed be set to NULL. So, do not log through service pointer
> > (which will cause NULL-deference), instead, use the vchiq_instance
> > function argument to get access to the struct device.
> > 
> > Fixes: f67af5940d6d("staging: vc04: Convert(and rename) vchiq_log_info() to use dynamic debug")
> 
> You need a space there :(

I have this in my .bashrc:

gpf() {
	git show --pretty=fixes -s ${1:-HEAD} | wl-copy
}

It's quite handy.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/9] staging: vc04_services: Drop vchiq_log_error() in favour of dev_dbg

[Laurent Pinchart]

On Thu, Nov 23, 2023 at 01:02:45PM +0000, Greg Kroah-Hartman wrote:
> On Tue, Nov 07, 2023 at 04:51:53AM -0500, Umang Jain wrote:
> > Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
> > of dev_dbg() directly.
> > 
> > Add a new enum vchiq_log_type and log_type() helper to faciliate the
> > type of logging in dev_dbg(). This will help to determine the type of
> > logging("error", "warning", "debug", "trace") to dynamic debug.
> > 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  .../interface/vchiq_arm/vchiq_arm.c           |  54 ++++----
> >  .../interface/vchiq_arm/vchiq_connected.c     |   6 +-
> >  .../interface/vchiq_arm/vchiq_core.c          | 126 ++++++++++--------
> >  .../interface/vchiq_arm/vchiq_core.h          |  23 +++-
> >  .../interface/vchiq_arm/vchiq_dev.c           |  47 ++++---
> >  5 files changed, 143 insertions(+), 113 deletions(-)
> > 
> > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > index 9fb3e240d9de..2cb2a6503058 100644
> > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > @@ -696,8 +696,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
> >  
> >  	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> >  	if (!instance) {
> > -		vchiq_log_error(state->dev, VCHIQ_CORE,
> > -				"%s: error allocating vchiq instance\n", __func__);
> > +		dev_dbg(state->dev, "%s: %s: %s: error allocating vchiq instance\n",
> > +			log_cat(VCHIQ_CORE), log_type(ERROR), __func__);
> 
> All dev_dbg() calls have __func__ in them automatically, you never need
> to duplicate it again as that's redundant :(

Oh ? I didn't know that, and can't find it in the code. I may be missing
something though. Are you referring to the +f flag for dynamic debug
entries ? It won't work if dynamic debug isn't enabled though, but maybe
we don't care about that ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/9] staging: vc04_services: Drop vchiq_log_error() in favour of dev_dbg

[Greg Kroah-Hartman]

On Thu, Nov 23, 2023 at 03:49:41PM +0200, Laurent Pinchart wrote:
> On Thu, Nov 23, 2023 at 01:02:45PM +0000, Greg Kroah-Hartman wrote:
> > On Tue, Nov 07, 2023 at 04:51:53AM -0500, Umang Jain wrote:
> > > Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
> > > of dev_dbg() directly.
> > > 
> > > Add a new enum vchiq_log_type and log_type() helper to faciliate the
> > > type of logging in dev_dbg(). This will help to determine the type of
> > > logging("error", "warning", "debug", "trace") to dynamic debug.
> > > 
> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > ---
> > >  .../interface/vchiq_arm/vchiq_arm.c           |  54 ++++----
> > >  .../interface/vchiq_arm/vchiq_connected.c     |   6 +-
> > >  .../interface/vchiq_arm/vchiq_core.c          | 126 ++++++++++--------
> > >  .../interface/vchiq_arm/vchiq_core.h          |  23 +++-
> > >  .../interface/vchiq_arm/vchiq_dev.c           |  47 ++++---
> > >  5 files changed, 143 insertions(+), 113 deletions(-)
> > > 
> > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > index 9fb3e240d9de..2cb2a6503058 100644
> > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > @@ -696,8 +696,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
> > >  
> > >  	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> > >  	if (!instance) {
> > > -		vchiq_log_error(state->dev, VCHIQ_CORE,
> > > -				"%s: error allocating vchiq instance\n", __func__);
> > > +		dev_dbg(state->dev, "%s: %s: %s: error allocating vchiq instance\n",
> > > +			log_cat(VCHIQ_CORE), log_type(ERROR), __func__);
> > 
> > All dev_dbg() calls have __func__ in them automatically, you never need
> > to duplicate it again as that's redundant :(
> 
> Oh ? I didn't know that, and can't find it in the code. I may be missing
> something though. Are you referring to the +f flag for dynamic debug
> entries ? It won't work if dynamic debug isn't enabled though, but maybe
> we don't care about that ?

Yes, the "f" flag is what controls this, and if dynamic debug isn't
enabled, you don't get any message here and we don't care about it :)

thanks,

greg k-h

Re: [PATCH 5/9] staging: vc04_services: Do not pass NULL to vchiq_log_error()

[Greg Kroah-Hartman]

On Thu, Nov 23, 2023 at 03:41:27PM +0200, Laurent Pinchart wrote:
> On Thu, Nov 23, 2023 at 12:57:14PM +0000, Greg Kroah-Hartman wrote:
> > On Tue, Nov 07, 2023 at 06:01:58PM +0530, Umang Jain wrote:
> > > Hi Laurent,
> > > 
> > > On 11/7/23 5:55 PM, Laurent Pinchart wrote:
> > > > Hi Umang,
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > On Tue, Nov 07, 2023 at 04:51:52AM -0500, Umang Jain wrote:
> > > > > vchiq_add_connected_callback() logs using vchiq_log_error() macro,
> > > > > but passes NULL instead of a struct device pointer. Fix it.
> > > > > 
> > > > > vchiq_add_connected_callback() is not used anywhere in the vc04_services
> > > > > as of now. It will be used when we add new drivers(VC shared memory and
> > > > > bcm2835-isp), hence it kept as it is for now.
> > > > > 
> > > > > Fixes: 1d8915cf8899 ("staging: vc04: Convert vchiq_log_error() to use dynamic debug")
> > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > > ---
> > > > >   .../vc04_services/interface/vchiq_arm/vchiq_connected.c       | 4 ++--
> > > > >   .../vc04_services/interface/vchiq_arm/vchiq_connected.h       | 4 +++-
> > > > >   2 files changed, 5 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > > > > index b3928bd8c9c6..21f9fa1a1713 100644
> > > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
> > > > > @@ -27,7 +27,7 @@ static void connected_init(void)
> > > > >    * be made immediately, otherwise it will be deferred until
> > > > >    * vchiq_call_connected_callbacks is called.
> > > > >    */
> > > > > -void vchiq_add_connected_callback(void (*callback)(void))
> > > > > +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
> > > > You're changing the prototype of the function, but the patch doesn't
> > > > update any user. If the function is unused, it looks like you can drop
> > > > it instead. Looking at the rest of the vchiq_connected.c file, I think
> > > > you can actually drop the whole file.
> > > 
> > > I mentioned in the commit message of this patch. There will be users in the
> > > near future.
> > 
> > We write code for today, not any potential users in the future.  If it's
> > not used now, let's delete it and then when we need it in the future,
> > you can add it back then.
> 
> What was the near future on November the 7th is now the present :-)
> Umang has sent a new version of the ISP driver that uses this API. I
> think this kind of near future is fine, and we routinely do the same
> during review when large feature are split across multiple series
> (whether those series are merged together or independently is another
> question of course).

That other series is long gone from my review queue after the first
round of issues found with it, I had forgotten all about it :)

greg k-h

Re: [PATCH 6/9] staging: vc04_services: Drop vchiq_log_error() in favour of dev_dbg

[Laurent Pinchart]

On Thu, Nov 23, 2023 at 01:53:42PM +0000, Greg Kroah-Hartman wrote:
> On Thu, Nov 23, 2023 at 03:49:41PM +0200, Laurent Pinchart wrote:
> > On Thu, Nov 23, 2023 at 01:02:45PM +0000, Greg Kroah-Hartman wrote:
> > > On Tue, Nov 07, 2023 at 04:51:53AM -0500, Umang Jain wrote:
> > > > Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
> > > > of dev_dbg() directly.
> > > > 
> > > > Add a new enum vchiq_log_type and log_type() helper to faciliate the
> > > > type of logging in dev_dbg(). This will help to determine the type of
> > > > logging("error", "warning", "debug", "trace") to dynamic debug.
> > > > 
> > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > ---
> > > >  .../interface/vchiq_arm/vchiq_arm.c           |  54 ++++----
> > > >  .../interface/vchiq_arm/vchiq_connected.c     |   6 +-
> > > >  .../interface/vchiq_arm/vchiq_core.c          | 126 ++++++++++--------
> > > >  .../interface/vchiq_arm/vchiq_core.h          |  23 +++-
> > > >  .../interface/vchiq_arm/vchiq_dev.c           |  47 ++++---
> > > >  5 files changed, 143 insertions(+), 113 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > index 9fb3e240d9de..2cb2a6503058 100644
> > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > @@ -696,8 +696,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
> > > >  
> > > >  	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> > > >  	if (!instance) {
> > > > -		vchiq_log_error(state->dev, VCHIQ_CORE,
> > > > -				"%s: error allocating vchiq instance\n", __func__);
> > > > +		dev_dbg(state->dev, "%s: %s: %s: error allocating vchiq instance\n",
> > > > +			log_cat(VCHIQ_CORE), log_type(ERROR), __func__);
> > > 
> > > All dev_dbg() calls have __func__ in them automatically, you never need
> > > to duplicate it again as that's redundant :(
> > 
> > Oh ? I didn't know that, and can't find it in the code. I may be missing
> > something though. Are you referring to the +f flag for dynamic debug
> > entries ? It won't work if dynamic debug isn't enabled though, but maybe
> > we don't care about that ?
> 
> Yes, the "f" flag is what controls this, and if dynamic debug isn't
> enabled, you don't get any message here and we don't care about it :)

You do if you #define DEBUG, that's one of the three options for
dev_dbg() (dynamic debug and no_printk() being the other two). Maybe
__func__ should be added to the dev_printk() version of dev_dbg() to
have a consistent behaviour.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/9] staging: vc04_services: Drop vchiq_log_error() in favour of dev_dbg

[Greg Kroah-Hartman]

On Thu, Nov 23, 2023 at 07:28:25PM +0200, Laurent Pinchart wrote:
> On Thu, Nov 23, 2023 at 01:53:42PM +0000, Greg Kroah-Hartman wrote:
> > On Thu, Nov 23, 2023 at 03:49:41PM +0200, Laurent Pinchart wrote:
> > > On Thu, Nov 23, 2023 at 01:02:45PM +0000, Greg Kroah-Hartman wrote:
> > > > On Tue, Nov 07, 2023 at 04:51:53AM -0500, Umang Jain wrote:
> > > > > Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
> > > > > of dev_dbg() directly.
> > > > > 
> > > > > Add a new enum vchiq_log_type and log_type() helper to faciliate the
> > > > > type of logging in dev_dbg(). This will help to determine the type of
> > > > > logging("error", "warning", "debug", "trace") to dynamic debug.
> > > > > 
> > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > > ---
> > > > >  .../interface/vchiq_arm/vchiq_arm.c           |  54 ++++----
> > > > >  .../interface/vchiq_arm/vchiq_connected.c     |   6 +-
> > > > >  .../interface/vchiq_arm/vchiq_core.c          | 126 ++++++++++--------
> > > > >  .../interface/vchiq_arm/vchiq_core.h          |  23 +++-
> > > > >  .../interface/vchiq_arm/vchiq_dev.c           |  47 ++++---
> > > > >  5 files changed, 143 insertions(+), 113 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > index 9fb3e240d9de..2cb2a6503058 100644
> > > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > @@ -696,8 +696,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
> > > > >  
> > > > >  	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> > > > >  	if (!instance) {
> > > > > -		vchiq_log_error(state->dev, VCHIQ_CORE,
> > > > > -				"%s: error allocating vchiq instance\n", __func__);
> > > > > +		dev_dbg(state->dev, "%s: %s: %s: error allocating vchiq instance\n",
> > > > > +			log_cat(VCHIQ_CORE), log_type(ERROR), __func__);
> > > > 
> > > > All dev_dbg() calls have __func__ in them automatically, you never need
> > > > to duplicate it again as that's redundant :(
> > > 
> > > Oh ? I didn't know that, and can't find it in the code. I may be missing
> > > something though. Are you referring to the +f flag for dynamic debug
> > > entries ? It won't work if dynamic debug isn't enabled though, but maybe
> > > we don't care about that ?
> > 
> > Yes, the "f" flag is what controls this, and if dynamic debug isn't
> > enabled, you don't get any message here and we don't care about it :)
> 
> You do if you #define DEBUG, that's one of the three options for
> dev_dbg() (dynamic debug and no_printk() being the other two). Maybe
> __func__ should be added to the dev_printk() version of dev_dbg() to
> have a consistent behaviour.

Drivers should NOT be defining DEBUG for anything in the tree, just use
the normal interfaces, as no one will be selecting debug options from
Kconfig.  DEBUG is really only good for out-of-tree work.

thanks,

greg k-h

Re: [PATCH 6/9] staging: vc04_services: Drop vchiq_log_error() in favour of dev_dbg

[Laurent Pinchart]

On Thu, Nov 23, 2023 at 05:31:32PM +0000, Greg Kroah-Hartman wrote:
> On Thu, Nov 23, 2023 at 07:28:25PM +0200, Laurent Pinchart wrote:
> > On Thu, Nov 23, 2023 at 01:53:42PM +0000, Greg Kroah-Hartman wrote:
> > > On Thu, Nov 23, 2023 at 03:49:41PM +0200, Laurent Pinchart wrote:
> > > > On Thu, Nov 23, 2023 at 01:02:45PM +0000, Greg Kroah-Hartman wrote:
> > > > > On Tue, Nov 07, 2023 at 04:51:53AM -0500, Umang Jain wrote:
> > > > > > Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
> > > > > > of dev_dbg() directly.
> > > > > > 
> > > > > > Add a new enum vchiq_log_type and log_type() helper to faciliate the
> > > > > > type of logging in dev_dbg(). This will help to determine the type of
> > > > > > logging("error", "warning", "debug", "trace") to dynamic debug.
> > > > > > 
> > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > > > ---
> > > > > >  .../interface/vchiq_arm/vchiq_arm.c           |  54 ++++----
> > > > > >  .../interface/vchiq_arm/vchiq_connected.c     |   6 +-
> > > > > >  .../interface/vchiq_arm/vchiq_core.c          | 126 ++++++++++--------
> > > > > >  .../interface/vchiq_arm/vchiq_core.h          |  23 +++-
> > > > > >  .../interface/vchiq_arm/vchiq_dev.c           |  47 ++++---
> > > > > >  5 files changed, 143 insertions(+), 113 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > > index 9fb3e240d9de..2cb2a6503058 100644
> > > > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > > @@ -696,8 +696,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
> > > > > >  
> > > > > >  	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> > > > > >  	if (!instance) {
> > > > > > -		vchiq_log_error(state->dev, VCHIQ_CORE,
> > > > > > -				"%s: error allocating vchiq instance\n", __func__);
> > > > > > +		dev_dbg(state->dev, "%s: %s: %s: error allocating vchiq instance\n",
> > > > > > +			log_cat(VCHIQ_CORE), log_type(ERROR), __func__);
> > > > > 
> > > > > All dev_dbg() calls have __func__ in them automatically, you never need
> > > > > to duplicate it again as that's redundant :(
> > > > 
> > > > Oh ? I didn't know that, and can't find it in the code. I may be missing
> > > > something though. Are you referring to the +f flag for dynamic debug
> > > > entries ? It won't work if dynamic debug isn't enabled though, but maybe
> > > > we don't care about that ?
> > > 
> > > Yes, the "f" flag is what controls this, and if dynamic debug isn't
> > > enabled, you don't get any message here and we don't care about it :)
> > 
> > You do if you #define DEBUG, that's one of the three options for
> > dev_dbg() (dynamic debug and no_printk() being the other two). Maybe
> > __func__ should be added to the dev_printk() version of dev_dbg() to
> > have a consistent behaviour.
> 
> Drivers should NOT be defining DEBUG for anything in the tree, just use
> the normal interfaces, as no one will be selecting debug options from
> Kconfig.  DEBUG is really only good for out-of-tree work.

I didn't know that either. Of course '#define DEBUG' shouldn't be merged
upstream, but I thought it was supported by the kernel to make that
possible during development, as an alternative to dynamic debug. Does it
mean we should drop '#define DEBUG' support from dev_dbg() eventually ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/9] staging: vc04_services: Drop vchiq_log_error() in favour of dev_dbg

[Greg Kroah-Hartman]

On Thu, Nov 23, 2023 at 08:00:22PM +0200, Laurent Pinchart wrote:
> On Thu, Nov 23, 2023 at 05:31:32PM +0000, Greg Kroah-Hartman wrote:
> > On Thu, Nov 23, 2023 at 07:28:25PM +0200, Laurent Pinchart wrote:
> > > On Thu, Nov 23, 2023 at 01:53:42PM +0000, Greg Kroah-Hartman wrote:
> > > > On Thu, Nov 23, 2023 at 03:49:41PM +0200, Laurent Pinchart wrote:
> > > > > On Thu, Nov 23, 2023 at 01:02:45PM +0000, Greg Kroah-Hartman wrote:
> > > > > > On Tue, Nov 07, 2023 at 04:51:53AM -0500, Umang Jain wrote:
> > > > > > > Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
> > > > > > > of dev_dbg() directly.
> > > > > > > 
> > > > > > > Add a new enum vchiq_log_type and log_type() helper to faciliate the
> > > > > > > type of logging in dev_dbg(). This will help to determine the type of
> > > > > > > logging("error", "warning", "debug", "trace") to dynamic debug.
> > > > > > > 
> > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > > > > ---
> > > > > > >  .../interface/vchiq_arm/vchiq_arm.c           |  54 ++++----
> > > > > > >  .../interface/vchiq_arm/vchiq_connected.c     |   6 +-
> > > > > > >  .../interface/vchiq_arm/vchiq_core.c          | 126 ++++++++++--------
> > > > > > >  .../interface/vchiq_arm/vchiq_core.h          |  23 +++-
> > > > > > >  .../interface/vchiq_arm/vchiq_dev.c           |  47 ++++---
> > > > > > >  5 files changed, 143 insertions(+), 113 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > > > index 9fb3e240d9de..2cb2a6503058 100644
> > > > > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > > > @@ -696,8 +696,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
> > > > > > >  
> > > > > > >  	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> > > > > > >  	if (!instance) {
> > > > > > > -		vchiq_log_error(state->dev, VCHIQ_CORE,
> > > > > > > -				"%s: error allocating vchiq instance\n", __func__);
> > > > > > > +		dev_dbg(state->dev, "%s: %s: %s: error allocating vchiq instance\n",
> > > > > > > +			log_cat(VCHIQ_CORE), log_type(ERROR), __func__);
> > > > > > 
> > > > > > All dev_dbg() calls have __func__ in them automatically, you never need
> > > > > > to duplicate it again as that's redundant :(
> > > > > 
> > > > > Oh ? I didn't know that, and can't find it in the code. I may be missing
> > > > > something though. Are you referring to the +f flag for dynamic debug
> > > > > entries ? It won't work if dynamic debug isn't enabled though, but maybe
> > > > > we don't care about that ?
> > > > 
> > > > Yes, the "f" flag is what controls this, and if dynamic debug isn't
> > > > enabled, you don't get any message here and we don't care about it :)
> > > 
> > > You do if you #define DEBUG, that's one of the three options for
> > > dev_dbg() (dynamic debug and no_printk() being the other two). Maybe
> > > __func__ should be added to the dev_printk() version of dev_dbg() to
> > > have a consistent behaviour.
> > 
> > Drivers should NOT be defining DEBUG for anything in the tree, just use
> > the normal interfaces, as no one will be selecting debug options from
> > Kconfig.  DEBUG is really only good for out-of-tree work.
> 
> I didn't know that either. Of course '#define DEBUG' shouldn't be merged
> upstream, but I thought it was supported by the kernel to make that
> possible during development, as an alternative to dynamic debug. Does it
> mean we should drop '#define DEBUG' support from dev_dbg() eventually ?

Probably, once all of the "define DEBUG" lines are dropped from the
kernel itself, which might take a while :)

greg k-h

Re: [PATCH 6/9] staging: vc04_services: Drop vchiq_log_error() in favour of dev_dbg

[Laurent Pinchart]

On Sun, Nov 26, 2023 at 10:26:41AM +0000, Greg Kroah-Hartman wrote:
> On Thu, Nov 23, 2023 at 08:00:22PM +0200, Laurent Pinchart wrote:
> > On Thu, Nov 23, 2023 at 05:31:32PM +0000, Greg Kroah-Hartman wrote:
> > > On Thu, Nov 23, 2023 at 07:28:25PM +0200, Laurent Pinchart wrote:
> > > > On Thu, Nov 23, 2023 at 01:53:42PM +0000, Greg Kroah-Hartman wrote:
> > > > > On Thu, Nov 23, 2023 at 03:49:41PM +0200, Laurent Pinchart wrote:
> > > > > > On Thu, Nov 23, 2023 at 01:02:45PM +0000, Greg Kroah-Hartman wrote:
> > > > > > > On Tue, Nov 07, 2023 at 04:51:53AM -0500, Umang Jain wrote:
> > > > > > > > Drop vchiq_log_error() macro which wraps dev_dbg(). Introduce the usage
> > > > > > > > of dev_dbg() directly.
> > > > > > > > 
> > > > > > > > Add a new enum vchiq_log_type and log_type() helper to faciliate the
> > > > > > > > type of logging in dev_dbg(). This will help to determine the type of
> > > > > > > > logging("error", "warning", "debug", "trace") to dynamic debug.
> > > > > > > > 
> > > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > > > > > > ---
> > > > > > > >  .../interface/vchiq_arm/vchiq_arm.c           |  54 ++++----
> > > > > > > >  .../interface/vchiq_arm/vchiq_connected.c     |   6 +-
> > > > > > > >  .../interface/vchiq_arm/vchiq_core.c          | 126 ++++++++++--------
> > > > > > > >  .../interface/vchiq_arm/vchiq_core.h          |  23 +++-
> > > > > > > >  .../interface/vchiq_arm/vchiq_dev.c           |  47 ++++---
> > > > > > > >  5 files changed, 143 insertions(+), 113 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > > > > index 9fb3e240d9de..2cb2a6503058 100644
> > > > > > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > > > > > @@ -696,8 +696,8 @@ int vchiq_initialise(struct vchiq_instance **instance_out)
> > > > > > > >  
> > > > > > > >  	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
> > > > > > > >  	if (!instance) {
> > > > > > > > -		vchiq_log_error(state->dev, VCHIQ_CORE,
> > > > > > > > -				"%s: error allocating vchiq instance\n", __func__);
> > > > > > > > +		dev_dbg(state->dev, "%s: %s: %s: error allocating vchiq instance\n",
> > > > > > > > +			log_cat(VCHIQ_CORE), log_type(ERROR), __func__);
> > > > > > > 
> > > > > > > All dev_dbg() calls have __func__ in them automatically, you never need
> > > > > > > to duplicate it again as that's redundant :(
> > > > > > 
> > > > > > Oh ? I didn't know that, and can't find it in the code. I may be missing
> > > > > > something though. Are you referring to the +f flag for dynamic debug
> > > > > > entries ? It won't work if dynamic debug isn't enabled though, but maybe
> > > > > > we don't care about that ?
> > > > > 
> > > > > Yes, the "f" flag is what controls this, and if dynamic debug isn't
> > > > > enabled, you don't get any message here and we don't care about it :)
> > > > 
> > > > You do if you #define DEBUG, that's one of the three options for
> > > > dev_dbg() (dynamic debug and no_printk() being the other two). Maybe
> > > > __func__ should be added to the dev_printk() version of dev_dbg() to
> > > > have a consistent behaviour.
> > > 
> > > Drivers should NOT be defining DEBUG for anything in the tree, just use
> > > the normal interfaces, as no one will be selecting debug options from
> > > Kconfig.  DEBUG is really only good for out-of-tree work.
> > 
> > I didn't know that either. Of course '#define DEBUG' shouldn't be merged
> > upstream, but I thought it was supported by the kernel to make that
> > possible during development, as an alternative to dynamic debug. Does it
> > mean we should drop '#define DEBUG' support from dev_dbg() eventually ?
> 
> Probably, once all of the "define DEBUG" lines are dropped from the
> kernel itself, which might take a while :)

Would a patch that simply drops them be acceptable ? If not, what's
missing to remove them ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/9] staging: vc04_services: Do not pass NULL to vchiq_log_error()

[Umang Jain]

Hi Laurent,

On 11/7/23 6:06 PM, Laurent Pinchart wrote:
> On Tue, Nov 07, 2023 at 02:25:52PM +0200, Laurent Pinchart wrote:
>> On Tue, Nov 07, 2023 at 04:51:52AM -0500, Umang Jain wrote:
>>> vchiq_add_connected_callback() logs using vchiq_log_error() macro,
>>> but passes NULL instead of a struct device pointer. Fix it.
>>>
>>> vchiq_add_connected_callback() is not used anywhere in the vc04_services
>>> as of now. It will be used when we add new drivers(VC shared memory and
>>> bcm2835-isp), hence it kept as it is for now.
>>>
>>> Fixes: 1d8915cf8899 ("staging: vc04: Convert vchiq_log_error() to use dynamic debug")
>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>> ---
>>>   .../vc04_services/interface/vchiq_arm/vchiq_connected.c       | 4 ++--
>>>   .../vc04_services/interface/vchiq_arm/vchiq_connected.h       | 4 +++-
>>>   2 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>>> index b3928bd8c9c6..21f9fa1a1713 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c
>>> @@ -27,7 +27,7 @@ static void connected_init(void)
>>>    * be made immediately, otherwise it will be deferred until
>>>    * vchiq_call_connected_callbacks is called.
>>>    */
>>> -void vchiq_add_connected_callback(void (*callback)(void))
>>> +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void))
>> You're changing the prototype of the function, but the patch doesn't
>> update any user. If the function is unused, it looks like you can drop
>> it instead. Looking at the rest of the vchiq_connected.c file, I think
>> you can actually drop the whole file.
> Except that the vc-sm-cma driver will use it. I'm curious though, that
> driver will have no way to acquire a pointer to a vchiq_device, so I
> don't see how this will be usable. pr_err() may be a better pick here.

vc-sm-cma probe function is called through vchiq_device, hence access to 
dev pointer is established.

https://git.uk.ideasonboard.com/uajain/linux/src/branch/uajain/rpi3/staging-next/isp/drivers/staging/vc04_services/vc-sm-cma/vc_sm.c#L676

>>>   {
>>>   	connected_init();
>>>   
>>> @@ -39,7 +39,7 @@ void vchiq_add_connected_callback(void (*callback)(void))
>>>   		callback();
>>>   	} else {
>>>   		if (g_num_deferred_callbacks >= MAX_CALLBACKS) {
>>> -			vchiq_log_error(NULL, VCHIQ_CORE,
>>> +			vchiq_log_error(&device->dev, VCHIQ_CORE,
>>>   					"There already %d callback registered - please increase MAX_CALLBACKS",
>>>   					g_num_deferred_callbacks);
>>>   		} else {
>>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>>> index 4caf5e30099d..e4ed56446f8a 100644
>>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h
>>> @@ -1,10 +1,12 @@
>>>   /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
>>>   /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */
>>>   
>>> +#include "vchiq_bus.h"
>>> +
>>>   #ifndef VCHIQ_CONNECTED_H
>>>   #define VCHIQ_CONNECTED_H
>>>   
>>> -void vchiq_add_connected_callback(void (*callback)(void));
>>> +void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void));
>>>   void vchiq_call_connected_callbacks(void);
>>>   
>>>   #endif /* VCHIQ_CONNECTED_H */