On Wed, Dec 21, 2022 at 01:14:59PM +0200, Laurent Pinchart wrote:
Hi Umang,
Thank you for the patch.
On Tue, Dec 20, 2022 at 02:14:04PM +0530, Umang Jain wrote:
Create a proper per device platorm_device structure for all the childThis explains what the patch does, but not why.
devices that needs to be registered by vchiq platform driver. Replace
the vchiq_register_child() with platform_add_devices() to register the
child devices.
This is part of an effort to address TODO item "Get rid of all nonAnd this explains part of the reason only. Could you please expand the
essential global structures and create a proper per device structure"
commit message with the reasoning behind this change ? It's not clear
from the change below why this is needed and good.
Signed-off-by: Umang Jain <umang.jain@xxxxxxxxxxxxxxxx>The fact that this isn't const and is used by two different
---
.../interface/vchiq_arm/vchiq_arm.c | 59 ++++++++++---------
1 file changed, 31 insertions(+), 28 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 22de23f3af02..fa42ea3791a7 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -65,8 +65,29 @@ int vchiq_susp_log_level = VCHIQ_LOG_ERROR;
DEFINE_SPINLOCK(msg_queue_spinlock);
struct vchiq_state g_state;
-static struct platform_device *bcm2835_camera;
-static struct platform_device *bcm2835_audio;
+static u64 vchiq_device_dmamask = DMA_BIT_MASK(32);
platform_device instances is worrying. Either it can be made const, or
it's wrong.
+Extra blank line.
+static struct platform_device bcm2835_camera = {
+ .name = "bcm2835-camera",
+ .id = PLATFORM_DEVID_NONE,
+ .dev = {
+ .dma_mask = &vchiq_device_dmamask,
+ }
+};
+
+static struct platform_device bcm2835_audio = {
+ .name = "bcm2835_audio",
+ .id = PLATFORM_DEVID_NONE,
+ .dev = {
+ .dma_mask = &vchiq_device_dmamask,
+ }
+
static instances of any device are a horrible idea, but it seems that
+};Make it const.
+
+static struct platform_device *vchiq_devices[] __initdata = {
+ &bcm2835_camera,If you unbind and rebind this driver, the platform_device instances
+ &bcm2835_audio,
+};
struct vchiq_drvdata {
const unsigned int cache_line_size;
@@ -1763,28 +1784,6 @@ static const struct of_device_id vchiq_of_match[] = {
};
MODULE_DEVICE_TABLE(of, vchiq_of_match);
-static struct platform_device *
-vchiq_register_child(struct platform_device *pdev, const char *name)
-{
- struct platform_device_info pdevinfo;
- struct platform_device *child;
-
- memset(&pdevinfo, 0, sizeof(pdevinfo));
-
- pdevinfo.parent = &pdev->dev;
- pdevinfo.name = name;
- pdevinfo.id = PLATFORM_DEVID_NONE;
- pdevinfo.dma_mask = DMA_BIT_MASK(32);
-
- child = platform_device_register_full(&pdevinfo);
- if (IS_ERR(child)) {
- dev_warn(&pdev->dev, "%s not registered\n", name);
- child = NULL;
- }
-
- return child;
-}
-
static int vchiq_probe(struct platform_device *pdev)
{
struct device_node *fw_node;
@@ -1832,8 +1831,11 @@ static int vchiq_probe(struct platform_device *pdev)
goto error_exit;
}
- bcm2835_camera = vchiq_register_child(pdev, "bcm2835-camera");
- bcm2835_audio = vchiq_register_child(pdev, "bcm2835_audio");
+ err = platform_add_devices(vchiq_devices, ARRAY_SIZE(vchiq_devices));
+ if (err) {
+ dev_warn(&pdev->dev, "Failed to add vchiq child devices");
+ goto error_exit;
+ }
defined as global variables will be reused, and I'm pretty sure that
will cause issues, for instance with the kobj->state_initialized check
in kobject_init() (called from device_initialize(), itself called from
platform_device_register(), from platform_add_devices()). I'm not sure
static instances of platform_device are a very good idea in general.
many drivers do this and abuse platform devices this way :(
Ideally this should be done properly, with the correct devices created
automatically based on the device tree structure, NOT hard-coded into a
.c file like this.
So I too really do not like this change, why are these not being created
by the firware layer automatically?
thanks,
greg k-h