Hi Archit, > From: Archit Taneja [mailto:archit@xxxxxx] > Sent: Tuesday, March 11, 2014 9:34 AM > > vpe fops(vpe_open in particular) should be called only when VPDMA > firmware is loaded. File operations on the video device are possible > the moment it is registered. > > Currently, we register the video device for VPE at driver probe, after > calling a vpdma helper to initialize VPDMA and load firmware. This > function is non-blocking(it calls request_firmware_nowait()), and > doesn't ensure that the firmware is actually loaded when it returns. > > We remove the device registration from vpe probe, and move it to a > callback provided by the vpe driver to the vpdma library, through > vpdma_create(). > > The ready field in vpdma_data is no longer needed since we always have > firmware loaded before the device is registered. > > A minor problem with this approach is that if the video_register_device > fails(which doesn't really happen), the vpe platform device would be > registered. > however, there won't be any v4l2 device corresponding to it. Could you explain to me one thing. request_firmware cannot be used in probe, thus you are using request_firmware_nowait. Why cannot the firmware be loaded on open with a regular request_firmware that is waiting? This patch seems to swap one problem for another. The possibility that open fails (because firmware is not yet loaded) is swapped for a vague possibility that video_register_device. Best wishes, -- Kamil Debski Samsung R&D Institute Poland > > Signed-off-by: Archit Taneja <archit@xxxxxx> > --- > drivers/media/platform/ti-vpe/vpdma.c | 8 +++-- > drivers/media/platform/ti-vpe/vpdma.h | 7 +++-- > drivers/media/platform/ti-vpe/vpe.c | 55 ++++++++++++++++++++------- > -------- > 3 files changed, 41 insertions(+), 29 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/vpdma.c > b/drivers/media/platform/ti-vpe/vpdma.c > index e8175e7..73dd38e 100644 > --- a/drivers/media/platform/ti-vpe/vpdma.c > +++ b/drivers/media/platform/ti-vpe/vpdma.c > @@ -781,7 +781,7 @@ static void vpdma_firmware_cb(const struct firmware > *f, void *context) > /* already initialized */ > if (read_field_reg(vpdma, VPDMA_LIST_ATTR, VPDMA_LIST_RDY_MASK, > VPDMA_LIST_RDY_SHFT)) { > - vpdma->ready = true; > + vpdma->cb(vpdma->pdev); > return; > } > > @@ -811,7 +811,7 @@ static void vpdma_firmware_cb(const struct firmware > *f, void *context) > goto free_buf; > } > > - vpdma->ready = true; > + vpdma->cb(vpdma->pdev); > > free_buf: > vpdma_unmap_desc_buf(vpdma, &fw_dma_buf); @@ -839,7 +839,8 @@ > static int vpdma_load_firmware(struct vpdma_data *vpdma) > return 0; > } > > -struct vpdma_data *vpdma_create(struct platform_device *pdev) > +struct vpdma_data *vpdma_create(struct platform_device *pdev, > + void (*cb)(struct platform_device *pdev)) > { > struct resource *res; > struct vpdma_data *vpdma; > @@ -854,6 +855,7 @@ struct vpdma_data *vpdma_create(struct > platform_device *pdev) > } > > vpdma->pdev = pdev; > + vpdma->cb = cb; > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "vpdma"); > if (res == NULL) { > diff --git a/drivers/media/platform/ti-vpe/vpdma.h > b/drivers/media/platform/ti-vpe/vpdma.h > index cf40f11..bf5f8bb 100644 > --- a/drivers/media/platform/ti-vpe/vpdma.h > +++ b/drivers/media/platform/ti-vpe/vpdma.h > @@ -35,8 +35,8 @@ struct vpdma_data { > > struct platform_device *pdev; > > - /* tells whether vpdma firmware is loaded or not */ > - bool ready; > + /* callback to VPE driver when the firmware is loaded */ > + void (*cb)(struct platform_device *pdev); > }; > > enum vpdma_data_format_type { > @@ -208,6 +208,7 @@ void vpdma_set_frame_start_event(struct vpdma_data > *vpdma, void vpdma_dump_regs(struct vpdma_data *vpdma); > > /* initialize vpdma, passed with VPE's platform device pointer */ - > struct vpdma_data *vpdma_create(struct platform_device *pdev); > +struct vpdma_data *vpdma_create(struct platform_device *pdev, > + void (*cb)(struct platform_device *pdev)); > > #endif > diff --git a/drivers/media/platform/ti-vpe/vpe.c > b/drivers/media/platform/ti-vpe/vpe.c > index f3143ac..f1eae67 100644 > --- a/drivers/media/platform/ti-vpe/vpe.c > +++ b/drivers/media/platform/ti-vpe/vpe.c > @@ -1817,11 +1817,6 @@ static int vpe_open(struct file *file) > > vpe_dbg(dev, "vpe_open\n"); > > - if (!dev->vpdma->ready) { > - vpe_err(dev, "vpdma firmware not loaded\n"); > - return -ENODEV; > - } > - > ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); > if (!ctx) > return -ENOMEM; > @@ -2039,10 +2034,40 @@ static void vpe_runtime_put(struct > platform_device *pdev) > WARN_ON(r < 0 && r != -ENOSYS); > } > > +static void vpe_fw_cb(struct platform_device *pdev) { > + struct vpe_dev *dev = platform_get_drvdata(pdev); > + struct video_device *vfd; > + int ret; > + > + vfd = &dev->vfd; > + *vfd = vpe_videodev; > + vfd->lock = &dev->dev_mutex; > + vfd->v4l2_dev = &dev->v4l2_dev; > + > + ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0); > + if (ret) { > + vpe_err(dev, "Failed to register video device\n"); > + > + vpe_set_clock_enable(dev, 0); > + vpe_runtime_put(pdev); > + pm_runtime_disable(&pdev->dev); > + v4l2_m2m_release(dev->m2m_dev); > + vb2_dma_contig_cleanup_ctx(dev->alloc_ctx); > + v4l2_device_unregister(&dev->v4l2_dev); > + > + return; > + } > + > + video_set_drvdata(vfd, dev); > + snprintf(vfd->name, sizeof(vfd->name), "%s", vpe_videodev.name); > + dev_info(dev->v4l2_dev.dev, "Device registered as /dev/video%d\n", > + vfd->num); > +} > + > static int vpe_probe(struct platform_device *pdev) { > struct vpe_dev *dev; > - struct video_device *vfd; > int ret, irq, func; > > dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL); @@ - > 2123,28 +2148,12 @@ static int vpe_probe(struct platform_device *pdev) > goto runtime_put; > } > > - dev->vpdma = vpdma_create(pdev); > + dev->vpdma = vpdma_create(pdev, vpe_fw_cb); > if (IS_ERR(dev->vpdma)) { > ret = PTR_ERR(dev->vpdma); > goto runtime_put; > } > > - vfd = &dev->vfd; > - *vfd = vpe_videodev; > - vfd->lock = &dev->dev_mutex; > - vfd->v4l2_dev = &dev->v4l2_dev; > - > - ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0); > - if (ret) { > - vpe_err(dev, "Failed to register video device\n"); > - goto runtime_put; > - } > - > - video_set_drvdata(vfd, dev); > - snprintf(vfd->name, sizeof(vfd->name), "%s", vpe_videodev.name); > - dev_info(dev->v4l2_dev.dev, "Device registered as /dev/video%d\n", > - vfd->num); > - > return 0; > > runtime_put: > -- > 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
