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 child
> 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 explains what the patch does, but not why.
> This is part of an effort to address TODO item "Get rid of all non
> essential global structures and create a proper per device structure"
And this explains part of the reason only. Could you please expand the
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>
> ---
> .../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);
The fact that this isn't const and is used by two different
platform_device instances is worrying. Either it can be made const, or
it's wrong.
> +
> +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,
> + }
> +
Extra blank line.
> +};
> +
> +static struct platform_device *vchiq_devices[] __initdata = {
Make it const.
> + &bcm2835_camera,
> + &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;
> + }
If you unbind and rebind this driver, the platform_device instances
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.
>
> return 0;
>
> @@ -1845,8 +1847,9 @@ static int vchiq_probe(struct platform_device *pdev)
>
> static int vchiq_remove(struct platform_device *pdev)
> {
> - platform_device_unregister(bcm2835_audio);
> - platform_device_unregister(bcm2835_camera);
> + for (unsigned int i = 0; i < ARRAY_SIZE(vchiq_devices); i++)
> + platform_device_unregister(vchiq_devices[i]);
> +
> vchiq_debugfs_deinit();
> vchiq_deregister_chrdev();
>
--
Regards,
Laurent Pinchart
