Hi Umang, Thank you for the patch. On Thu, Mar 21, 2024 at 04:07:38PM +0530, Umang Jain wrote: > Drop global members for tracking vchiq driver connections in > vchiq_connected.[ch] and use the struct vchiq_drv_mgmt to > track the connections. I'd write "Move global connection tracking to vchiq_drv_mgmt" in the subject line, as you're not just dropping them completely. Same in the commit message. > Also, store a vchiq_drv_mgmt pointer to struct vchiq_device to > have easy access to struct vchiq_drv_mgmt across vchiq devices. > > Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx> > --- > .../interface/vchiq_arm/vchiq_arm.c | 2 +- > .../interface/vchiq_arm/vchiq_arm.h | 6 +++++ > .../interface/vchiq_arm/vchiq_bus.c | 4 +++ > .../interface/vchiq_arm/vchiq_bus.h | 3 +++ > .../interface/vchiq_arm/vchiq_connected.c | 25 +++++++++---------- > .../interface/vchiq_arm/vchiq_connected.h | 3 ++- > 6 files changed, 28 insertions(+), 15 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 01b4e4b010c6..ba096bcb32c8 100644 > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > @@ -559,7 +559,7 @@ static int vchiq_platform_init(struct platform_device *pdev, struct vchiq_state > dev_dbg(&pdev->dev, "arm: vchiq_init - done (slots %pK, phys %pad)\n", > vchiq_slot_zero, &slot_phys); > > - vchiq_call_connected_callbacks(); > + vchiq_call_connected_callbacks(drv_mgmt); > > return 0; > } > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h > index fc4122c27e94..1190fab2efc4 100644 > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.h > @@ -23,6 +23,8 @@ > #define MAX_ELEMENTS 8 > #define MSG_QUEUE_SIZE 128 > > +#define VCHIQ_DRV_MAX_CALLBACKS 10 It would be nice to remove this, but I think we can live with it for now. > + > enum USE_TYPE_E { > USE_TYPE_SERVICE, > USE_TYPE_VCHIQ > @@ -35,6 +37,10 @@ struct vchiq_platform_info { > struct vchiq_drv_mgmt { > struct rpi_firmware *fw; > const struct vchiq_platform_info *pinfo; > + > + bool connected; > + int num_deferred_callbacks; > + void (*deferred_callback[VCHIQ_DRV_MAX_CALLBACKS])(void); > }; > > struct user_service { > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c > index 68f830d75531..fb837e64838b 100644 > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.c > @@ -54,6 +54,7 @@ static void vchiq_device_release(struct device *dev) > struct vchiq_device * > vchiq_device_register(struct device *parent, const char *name) > { > + struct platform_device *pdev; > struct vchiq_device *device; > int ret; > > @@ -67,6 +68,9 @@ vchiq_device_register(struct device *parent, const char *name) > device->dev.dma_mask = &device->dev.coherent_dma_mask; > device->dev.release = vchiq_device_release; > > + pdev = to_platform_device(parent); > + device->drv_mgmt = platform_get_drvdata(pdev); device->drv_mgmt = dev_get_drvdata(parent); > + > of_dma_configure(&device->dev, parent->of_node, true); > > ret = device_register(&device->dev); > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h > index 4db86e76edbd..b0e85ca2365e 100644 > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_bus.h > @@ -9,8 +9,11 @@ > #include <linux/device.h> > #include <linux/mod_devicetable.h> > > +#include "vchiq_arm.h" > + Use a declaration instead. struct vchiq_drv_mgmt; > struct vchiq_device { > struct device dev; > + struct vchiq_drv_mgmt *drv_mgmt; > }; > > struct vchiq_driver { > 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 4604a2f4d2de..ec5e9107868e 100644 > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.c > @@ -8,9 +8,6 @@ > > #define MAX_CALLBACKS 10 > > -static int g_connected; > -static int g_num_deferred_callbacks; > -static void (*g_deferred_callback[MAX_CALLBACKS])(void); > static DEFINE_MUTEX(g_connected_mutex); Any reason not to move this to vchiq_drv_mgmt ? > > /* > @@ -21,21 +18,23 @@ static DEFINE_MUTEX(g_connected_mutex); > */ > void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void)) > { > + struct vchiq_drv_mgmt *drv_mgmt = device->drv_mgmt; > + > if (mutex_lock_killable(&g_connected_mutex)) > return; > > - if (g_connected) { > + if (drv_mgmt->connected) { > /* We're already connected. Call the callback immediately. */ > callback(); > } else { > - if (g_num_deferred_callbacks >= MAX_CALLBACKS) { > + if (drv_mgmt->num_deferred_callbacks >= MAX_CALLBACKS) { > dev_err(&device->dev, > "core: There already %d callback registered - please increase MAX_CALLBACKS\n", > - g_num_deferred_callbacks); > + drv_mgmt->num_deferred_callbacks); > } else { > - g_deferred_callback[g_num_deferred_callbacks] = > + drv_mgmt->deferred_callback[drv_mgmt->num_deferred_callbacks] = > callback; > - g_num_deferred_callbacks++; > + drv_mgmt->num_deferred_callbacks++; > } > } > mutex_unlock(&g_connected_mutex); > @@ -46,17 +45,17 @@ EXPORT_SYMBOL(vchiq_add_connected_callback); > * This function is called by the vchiq stack once it has been connected to > * the videocore and clients can start to use the stack. > */ > -void vchiq_call_connected_callbacks(void) > +void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *drv_mgmt) > { > int i; > > if (mutex_lock_killable(&g_connected_mutex)) > return; > > - for (i = 0; i < g_num_deferred_callbacks; i++) > - g_deferred_callback[i](); > + for (i = 0; i < drv_mgmt->num_deferred_callbacks; i++) > + drv_mgmt->deferred_callback[i](); > > - g_num_deferred_callbacks = 0; > - g_connected = 1; > + drv_mgmt->num_deferred_callbacks = 0; > + drv_mgmt->connected = true; > mutex_unlock(&g_connected_mutex); > } > 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 e4ed56446f8a..0a3adefc69e0 100644 > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_connected.h > @@ -1,12 +1,13 @@ > /* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */ > /* Copyright (c) 2010-2012 Broadcom. All rights reserved. */ > > +#include "vchiq_arm.h" Structure declaration too. > #include "vchiq_bus.h" > > #ifndef VCHIQ_CONNECTED_H > #define VCHIQ_CONNECTED_H > > void vchiq_add_connected_callback(struct vchiq_device *device, void (*callback)(void)); > -void vchiq_call_connected_callbacks(void); > +void vchiq_call_connected_callbacks(struct vchiq_drv_mgmt *mgmt); > > #endif /* VCHIQ_CONNECTED_H */ -- Regards, Laurent Pinchart