| Message ID | 1560242912-17138-3-git-send-email-hugues.fruchet@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DCMI bridge support | expand |
On 6/11/19 10:48 AM, Hugues Fruchet wrote: > Add media controller support to dcmi. > > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > --- > drivers/media/platform/Kconfig | 2 +- > drivers/media/platform/stm32/stm32-dcmi.c | 83 +++++++++++++++++++++++-------- > 2 files changed, 63 insertions(+), 22 deletions(-) > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > index 8a19654..de7e21f 100644 > --- a/drivers/media/platform/Kconfig > +++ b/drivers/media/platform/Kconfig > @@ -121,7 +121,7 @@ config VIDEO_S3C_CAMIF > > config VIDEO_STM32_DCMI > tristate "STM32 Digital Camera Memory Interface (DCMI) support" > - depends on VIDEO_V4L2 && OF > + depends on VIDEO_V4L2 && OF && MEDIA_CONTROLLER > depends on ARCH_STM32 || COMPILE_TEST > select VIDEOBUF2_DMA_CONTIG > select V4L2_FWNODE > diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c > index 7a4d559..3a69783 100644 > --- a/drivers/media/platform/stm32/stm32-dcmi.c > +++ b/drivers/media/platform/stm32/stm32-dcmi.c > @@ -170,6 +170,9 @@ struct stm32_dcmi { > > /* Ensure DMA operations atomicity */ > struct mutex dma_lock; > + > + struct media_device mdev; > + struct media_pad vid_cap_pad; > }; > > static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n) > @@ -1545,21 +1548,12 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier) > dev_err(dcmi->dev, "Could not get sensor bounds\n"); > return ret; > } > - > ret = dcmi_set_default_fmt(dcmi); > if (ret) { > dev_err(dcmi->dev, "Could not set default format\n"); > return ret; > } > > - ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1); > - if (ret) { > - dev_err(dcmi->dev, "Failed to register video device\n"); > - return ret; > - } Why was this moved to probe()? Off-hand I see no reason for that. Regards, Hans > - > - dev_dbg(dcmi->dev, "Device registered as %s\n", > - video_device_node_name(dcmi->vdev)); > return 0; > } > > @@ -1648,6 +1642,12 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi) > return 0; > } > > +static void dcmi_graph_deinit(struct stm32_dcmi *dcmi) > +{ > + v4l2_async_notifier_unregister(&dcmi->notifier); > + v4l2_async_notifier_cleanup(&dcmi->notifier); > +} > + > static int dcmi_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > @@ -1752,10 +1752,27 @@ static int dcmi_probe(struct platform_device *pdev) > > q = &dcmi->queue; > > + dcmi->v4l2_dev.mdev = &dcmi->mdev; > + > + /* Initialize media device */ > + strscpy(dcmi->mdev.model, DRV_NAME, sizeof(dcmi->mdev.model)); > + snprintf(dcmi->mdev.bus_info, sizeof(dcmi->mdev.bus_info), > + "platform:%s", DRV_NAME); > + dcmi->mdev.dev = &pdev->dev; > + media_device_init(&dcmi->mdev); > + > + /* Register the media device */ > + ret = media_device_register(&dcmi->mdev); > + if (ret) { > + dev_err(dcmi->dev, "Failed to register media device (%d)\n", > + ret); > + goto err_media_device_cleanup; > + } > + > /* Initialize the top-level structure */ > ret = v4l2_device_register(&pdev->dev, &dcmi->v4l2_dev); > if (ret) > - goto err_dma_release; > + goto err_media_device_unregister; > > dcmi->vdev = video_device_alloc(); > if (!dcmi->vdev) { > @@ -1775,6 +1792,25 @@ static int dcmi_probe(struct platform_device *pdev) > V4L2_CAP_READWRITE; > video_set_drvdata(dcmi->vdev, dcmi); > > + /* Media entity pads */ > + dcmi->vid_cap_pad.flags = MEDIA_PAD_FL_SINK; > + ret = media_entity_pads_init(&dcmi->vdev->entity, > + 1, &dcmi->vid_cap_pad); > + if (ret) { > + dev_err(dcmi->dev, "Failed to init media entity pad\n"); > + goto err_device_unregister; > + } > + dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT; > + > + ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1); > + if (ret) { > + dev_err(dcmi->dev, "Failed to register video device\n"); > + goto err_media_entity_cleanup; > + } > + > + dev_dbg(dcmi->dev, "Device registered as %s\n", > + video_device_node_name(dcmi->vdev)); > + > /* Buffer queue */ > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF; > @@ -1790,18 +1826,18 @@ static int dcmi_probe(struct platform_device *pdev) > ret = vb2_queue_init(q); > if (ret < 0) { > dev_err(&pdev->dev, "Failed to initialize vb2 queue\n"); > - goto err_device_release; > + goto err_media_entity_cleanup; > } > > ret = dcmi_graph_init(dcmi); > if (ret < 0) > - goto err_device_release; > + goto err_media_entity_cleanup; > > /* Reset device */ > ret = reset_control_assert(dcmi->rstc); > if (ret) { > dev_err(&pdev->dev, "Failed to assert the reset line\n"); > - goto err_cleanup; > + goto err_graph_deinit; > } > > usleep_range(3000, 5000); > @@ -1809,7 +1845,7 @@ static int dcmi_probe(struct platform_device *pdev) > ret = reset_control_deassert(dcmi->rstc); > if (ret) { > dev_err(&pdev->dev, "Failed to deassert the reset line\n"); > - goto err_cleanup; > + goto err_graph_deinit; > } > > dev_info(&pdev->dev, "Probe done\n"); > @@ -1820,13 +1856,16 @@ static int dcmi_probe(struct platform_device *pdev) > > return 0; > > -err_cleanup: > - v4l2_async_notifier_cleanup(&dcmi->notifier); > -err_device_release: > - video_device_release(dcmi->vdev); > +err_graph_deinit: > + dcmi_graph_deinit(dcmi); > +err_media_entity_cleanup: > + media_entity_cleanup(&dcmi->vdev->entity); > err_device_unregister: > v4l2_device_unregister(&dcmi->v4l2_dev); > -err_dma_release: > +err_media_device_unregister: > + media_device_unregister(&dcmi->mdev); > +err_media_device_cleanup: > + media_device_cleanup(&dcmi->mdev); > dma_release_channel(dcmi->dma_chan); > > return ret; > @@ -1838,9 +1877,11 @@ static int dcmi_remove(struct platform_device *pdev) > > pm_runtime_disable(&pdev->dev); > > - v4l2_async_notifier_unregister(&dcmi->notifier); > - v4l2_async_notifier_cleanup(&dcmi->notifier); > + dcmi_graph_deinit(dcmi); > + media_entity_cleanup(&dcmi->vdev->entity); > v4l2_device_unregister(&dcmi->v4l2_dev); > + media_device_unregister(&dcmi->mdev); > + media_device_cleanup(&dcmi->mdev); > > dma_release_channel(dcmi->dma_chan); > >
Hi Hugues, On Tue, Jun 11, 2019 at 10:48:31AM +0200, Hugues Fruchet wrote: > Add media controller support to dcmi. > > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > --- > drivers/media/platform/Kconfig | 2 +- > drivers/media/platform/stm32/stm32-dcmi.c | 83 +++++++++++++++++++++++-------- > 2 files changed, 63 insertions(+), 22 deletions(-) > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > index 8a19654..de7e21f 100644 > --- a/drivers/media/platform/Kconfig > +++ b/drivers/media/platform/Kconfig > @@ -121,7 +121,7 @@ config VIDEO_S3C_CAMIF > > config VIDEO_STM32_DCMI > tristate "STM32 Digital Camera Memory Interface (DCMI) support" > - depends on VIDEO_V4L2 && OF > + depends on VIDEO_V4L2 && OF && MEDIA_CONTROLLER Ok, if the intent is to require MC from now on, then I think you could simply rely on media_entity_remote_pad() in finding the image source. > depends on ARCH_STM32 || COMPILE_TEST > select VIDEOBUF2_DMA_CONTIG > select V4L2_FWNODE > diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c > index 7a4d559..3a69783 100644 > --- a/drivers/media/platform/stm32/stm32-dcmi.c > +++ b/drivers/media/platform/stm32/stm32-dcmi.c > @@ -170,6 +170,9 @@ struct stm32_dcmi { > > /* Ensure DMA operations atomicity */ > struct mutex dma_lock; > + > + struct media_device mdev; > + struct media_pad vid_cap_pad; > }; > > static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n) > @@ -1545,21 +1548,12 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier) > dev_err(dcmi->dev, "Could not get sensor bounds\n"); > return ret; > } > - > ret = dcmi_set_default_fmt(dcmi); > if (ret) { > dev_err(dcmi->dev, "Could not set default format\n"); > return ret; > } > > - ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1); > - if (ret) { > - dev_err(dcmi->dev, "Failed to register video device\n"); > - return ret; > - } > - > - dev_dbg(dcmi->dev, "Device registered as %s\n", > - video_device_node_name(dcmi->vdev)); > return 0; > } > > @@ -1648,6 +1642,12 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi) > return 0; > } > > +static void dcmi_graph_deinit(struct stm32_dcmi *dcmi) > +{ > + v4l2_async_notifier_unregister(&dcmi->notifier); > + v4l2_async_notifier_cleanup(&dcmi->notifier); I'd just leave the calls where they are now. This doesn't improve readability of the code, rather the opposite. > +} > + > static int dcmi_probe(struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > @@ -1752,10 +1752,27 @@ static int dcmi_probe(struct platform_device *pdev) > > q = &dcmi->queue; > > + dcmi->v4l2_dev.mdev = &dcmi->mdev; > + > + /* Initialize media device */ > + strscpy(dcmi->mdev.model, DRV_NAME, sizeof(dcmi->mdev.model)); > + snprintf(dcmi->mdev.bus_info, sizeof(dcmi->mdev.bus_info), > + "platform:%s", DRV_NAME); > + dcmi->mdev.dev = &pdev->dev; > + media_device_init(&dcmi->mdev); > + > + /* Register the media device */ > + ret = media_device_register(&dcmi->mdev); > + if (ret) { > + dev_err(dcmi->dev, "Failed to register media device (%d)\n", > + ret); > + goto err_media_device_cleanup; > + } > + > /* Initialize the top-level structure */ > ret = v4l2_device_register(&pdev->dev, &dcmi->v4l2_dev); > if (ret) > - goto err_dma_release; > + goto err_media_device_unregister; > > dcmi->vdev = video_device_alloc(); > if (!dcmi->vdev) { > @@ -1775,6 +1792,25 @@ static int dcmi_probe(struct platform_device *pdev) > V4L2_CAP_READWRITE; > video_set_drvdata(dcmi->vdev, dcmi); > > + /* Media entity pads */ > + dcmi->vid_cap_pad.flags = MEDIA_PAD_FL_SINK; > + ret = media_entity_pads_init(&dcmi->vdev->entity, > + 1, &dcmi->vid_cap_pad); > + if (ret) { > + dev_err(dcmi->dev, "Failed to init media entity pad\n"); > + goto err_device_unregister; > + } > + dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT; > + > + ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1); > + if (ret) { > + dev_err(dcmi->dev, "Failed to register video device\n"); > + goto err_media_entity_cleanup; > + } > + > + dev_dbg(dcmi->dev, "Device registered as %s\n", > + video_device_node_name(dcmi->vdev)); > + > /* Buffer queue */ > q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; > q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF; > @@ -1790,18 +1826,18 @@ static int dcmi_probe(struct platform_device *pdev) > ret = vb2_queue_init(q); > if (ret < 0) { > dev_err(&pdev->dev, "Failed to initialize vb2 queue\n"); > - goto err_device_release; > + goto err_media_entity_cleanup; > } > > ret = dcmi_graph_init(dcmi); > if (ret < 0) > - goto err_device_release; > + goto err_media_entity_cleanup; > > /* Reset device */ > ret = reset_control_assert(dcmi->rstc); > if (ret) { > dev_err(&pdev->dev, "Failed to assert the reset line\n"); > - goto err_cleanup; > + goto err_graph_deinit; > } > > usleep_range(3000, 5000); > @@ -1809,7 +1845,7 @@ static int dcmi_probe(struct platform_device *pdev) > ret = reset_control_deassert(dcmi->rstc); > if (ret) { > dev_err(&pdev->dev, "Failed to deassert the reset line\n"); > - goto err_cleanup; > + goto err_graph_deinit; > } > > dev_info(&pdev->dev, "Probe done\n"); > @@ -1820,13 +1856,16 @@ static int dcmi_probe(struct platform_device *pdev) > > return 0; > > -err_cleanup: > - v4l2_async_notifier_cleanup(&dcmi->notifier); > -err_device_release: > - video_device_release(dcmi->vdev); > +err_graph_deinit: > + dcmi_graph_deinit(dcmi); > +err_media_entity_cleanup: > + media_entity_cleanup(&dcmi->vdev->entity); > err_device_unregister: > v4l2_device_unregister(&dcmi->v4l2_dev); > -err_dma_release: > +err_media_device_unregister: > + media_device_unregister(&dcmi->mdev); > +err_media_device_cleanup: > + media_device_cleanup(&dcmi->mdev); > dma_release_channel(dcmi->dma_chan); > > return ret; > @@ -1838,9 +1877,11 @@ static int dcmi_remove(struct platform_device *pdev) > > pm_runtime_disable(&pdev->dev); > > - v4l2_async_notifier_unregister(&dcmi->notifier); > - v4l2_async_notifier_cleanup(&dcmi->notifier); > + dcmi_graph_deinit(dcmi); > + media_entity_cleanup(&dcmi->vdev->entity); > v4l2_device_unregister(&dcmi->v4l2_dev); > + media_device_unregister(&dcmi->mdev); Please unregister the media device first before unregistering anything else it depends on (i.e. async notifier or the entity). > + media_device_cleanup(&dcmi->mdev); > > dma_release_channel(dcmi->dma_chan); >
Hi Hans, On 6/20/19 2:01 PM, Hans Verkuil wrote: > On 6/11/19 10:48 AM, Hugues Fruchet wrote: >> Add media controller support to dcmi. >> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> >> --- >> drivers/media/platform/Kconfig | 2 +- >> drivers/media/platform/stm32/stm32-dcmi.c | 83 +++++++++++++++++++++++-------- >> 2 files changed, 63 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig >> index 8a19654..de7e21f 100644 >> --- a/drivers/media/platform/Kconfig >> +++ b/drivers/media/platform/Kconfig >> @@ -121,7 +121,7 @@ config VIDEO_S3C_CAMIF >> >> config VIDEO_STM32_DCMI >> tristate "STM32 Digital Camera Memory Interface (DCMI) support" >> - depends on VIDEO_V4L2 && OF >> + depends on VIDEO_V4L2 && OF && MEDIA_CONTROLLER >> depends on ARCH_STM32 || COMPILE_TEST >> select VIDEOBUF2_DMA_CONTIG >> select V4L2_FWNODE >> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c >> index 7a4d559..3a69783 100644 >> --- a/drivers/media/platform/stm32/stm32-dcmi.c >> +++ b/drivers/media/platform/stm32/stm32-dcmi.c >> @@ -170,6 +170,9 @@ struct stm32_dcmi { >> >> /* Ensure DMA operations atomicity */ >> struct mutex dma_lock; >> + >> + struct media_device mdev; >> + struct media_pad vid_cap_pad; >> }; >> >> static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n) >> @@ -1545,21 +1548,12 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier) >> dev_err(dcmi->dev, "Could not get sensor bounds\n"); >> return ret; >> } >> - >> ret = dcmi_set_default_fmt(dcmi); >> if (ret) { >> dev_err(dcmi->dev, "Could not set default format\n"); >> return ret; >> } >> >> - ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1); >> - if (ret) { >> - dev_err(dcmi->dev, "Failed to register video device\n"); >> - return ret; >> - } > > Why was this moved to probe()? Off-hand I see no reason for that. > > Regards, > > Hans > I need to do that otherwise the dcmi_graph_notify_bound() subdevice pad link code crashes: + /* + * Link this sub-device to DCMI, it could be + * a parallel camera sensor or a bridge + */ + src_pad = media_entity_get_fwnode_pad(&subdev->entity, + subdev->fwnode, + MEDIA_PAD_FL_SOURCE); + + ret = media_create_pad_link(&subdev->entity, src_pad, + &dcmi->vdev->entity, 0, + MEDIA_LNK_FL_IMMUTABLE | + MEDIA_LNK_FL_ENABLED); see https://www.spinics.net/lists/linux-media/msg153120.html. >> - >> - dev_dbg(dcmi->dev, "Device registered as %s\n", >> - video_device_node_name(dcmi->vdev)); >> return 0; >> } >> >> @@ -1648,6 +1642,12 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi) >> return 0; >> } >> >> +static void dcmi_graph_deinit(struct stm32_dcmi *dcmi) >> +{ >> + v4l2_async_notifier_unregister(&dcmi->notifier); >> + v4l2_async_notifier_cleanup(&dcmi->notifier); >> +} >> + >> static int dcmi_probe(struct platform_device *pdev) >> { >> struct device_node *np = pdev->dev.of_node; >> @@ -1752,10 +1752,27 @@ static int dcmi_probe(struct platform_device *pdev) >> >> q = &dcmi->queue; >> >> + dcmi->v4l2_dev.mdev = &dcmi->mdev; >> + >> + /* Initialize media device */ >> + strscpy(dcmi->mdev.model, DRV_NAME, sizeof(dcmi->mdev.model)); >> + snprintf(dcmi->mdev.bus_info, sizeof(dcmi->mdev.bus_info), >> + "platform:%s", DRV_NAME); >> + dcmi->mdev.dev = &pdev->dev; >> + media_device_init(&dcmi->mdev); >> + >> + /* Register the media device */ >> + ret = media_device_register(&dcmi->mdev); >> + if (ret) { >> + dev_err(dcmi->dev, "Failed to register media device (%d)\n", >> + ret); >> + goto err_media_device_cleanup; >> + } >> + >> /* Initialize the top-level structure */ >> ret = v4l2_device_register(&pdev->dev, &dcmi->v4l2_dev); >> if (ret) >> - goto err_dma_release; >> + goto err_media_device_unregister; >> >> dcmi->vdev = video_device_alloc(); >> if (!dcmi->vdev) { >> @@ -1775,6 +1792,25 @@ static int dcmi_probe(struct platform_device *pdev) >> V4L2_CAP_READWRITE; >> video_set_drvdata(dcmi->vdev, dcmi); >> >> + /* Media entity pads */ >> + dcmi->vid_cap_pad.flags = MEDIA_PAD_FL_SINK; >> + ret = media_entity_pads_init(&dcmi->vdev->entity, >> + 1, &dcmi->vid_cap_pad); >> + if (ret) { >> + dev_err(dcmi->dev, "Failed to init media entity pad\n"); >> + goto err_device_unregister; >> + } >> + dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT; >> + >> + ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1); >> + if (ret) { >> + dev_err(dcmi->dev, "Failed to register video device\n"); >> + goto err_media_entity_cleanup; >> + } >> + >> + dev_dbg(dcmi->dev, "Device registered as %s\n", >> + video_device_node_name(dcmi->vdev)); >> + >> /* Buffer queue */ >> q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF; >> @@ -1790,18 +1826,18 @@ static int dcmi_probe(struct platform_device *pdev) >> ret = vb2_queue_init(q); >> if (ret < 0) { >> dev_err(&pdev->dev, "Failed to initialize vb2 queue\n"); >> - goto err_device_release; >> + goto err_media_entity_cleanup; >> } >> >> ret = dcmi_graph_init(dcmi); >> if (ret < 0) >> - goto err_device_release; >> + goto err_media_entity_cleanup; >> >> /* Reset device */ >> ret = reset_control_assert(dcmi->rstc); >> if (ret) { >> dev_err(&pdev->dev, "Failed to assert the reset line\n"); >> - goto err_cleanup; >> + goto err_graph_deinit; >> } >> >> usleep_range(3000, 5000); >> @@ -1809,7 +1845,7 @@ static int dcmi_probe(struct platform_device *pdev) >> ret = reset_control_deassert(dcmi->rstc); >> if (ret) { >> dev_err(&pdev->dev, "Failed to deassert the reset line\n"); >> - goto err_cleanup; >> + goto err_graph_deinit; >> } >> >> dev_info(&pdev->dev, "Probe done\n"); >> @@ -1820,13 +1856,16 @@ static int dcmi_probe(struct platform_device *pdev) >> >> return 0; >> >> -err_cleanup: >> - v4l2_async_notifier_cleanup(&dcmi->notifier); >> -err_device_release: >> - video_device_release(dcmi->vdev); >> +err_graph_deinit: >> + dcmi_graph_deinit(dcmi); >> +err_media_entity_cleanup: >> + media_entity_cleanup(&dcmi->vdev->entity); >> err_device_unregister: >> v4l2_device_unregister(&dcmi->v4l2_dev); >> -err_dma_release: >> +err_media_device_unregister: >> + media_device_unregister(&dcmi->mdev); >> +err_media_device_cleanup: >> + media_device_cleanup(&dcmi->mdev); >> dma_release_channel(dcmi->dma_chan); >> >> return ret; >> @@ -1838,9 +1877,11 @@ static int dcmi_remove(struct platform_device *pdev) >> >> pm_runtime_disable(&pdev->dev); >> >> - v4l2_async_notifier_unregister(&dcmi->notifier); >> - v4l2_async_notifier_cleanup(&dcmi->notifier); >> + dcmi_graph_deinit(dcmi); >> + media_entity_cleanup(&dcmi->vdev->entity); >> v4l2_device_unregister(&dcmi->v4l2_dev); >> + media_device_unregister(&dcmi->mdev); >> + media_device_cleanup(&dcmi->mdev); >> >> dma_release_channel(dcmi->dma_chan); >> >> > BR, Hugues.
Hi Sakari, On 6/20/19 6:13 PM, Sakari Ailus wrote: > Hi Hugues, > > On Tue, Jun 11, 2019 at 10:48:31AM +0200, Hugues Fruchet wrote: >> Add media controller support to dcmi. >> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> >> --- >> drivers/media/platform/Kconfig | 2 +- >> drivers/media/platform/stm32/stm32-dcmi.c | 83 +++++++++++++++++++++++-------- >> 2 files changed, 63 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig >> index 8a19654..de7e21f 100644 >> --- a/drivers/media/platform/Kconfig >> +++ b/drivers/media/platform/Kconfig >> @@ -121,7 +121,7 @@ config VIDEO_S3C_CAMIF >> >> config VIDEO_STM32_DCMI >> tristate "STM32 Digital Camera Memory Interface (DCMI) support" >> - depends on VIDEO_V4L2 && OF >> + depends on VIDEO_V4L2 && OF && MEDIA_CONTROLLER > > Ok, if the intent is to require MC from now on, then I think you could > simply rely on media_entity_remote_pad() in finding the image source. > >> depends on ARCH_STM32 || COMPILE_TEST >> select VIDEOBUF2_DMA_CONTIG >> select V4L2_FWNODE >> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c >> index 7a4d559..3a69783 100644 >> --- a/drivers/media/platform/stm32/stm32-dcmi.c >> +++ b/drivers/media/platform/stm32/stm32-dcmi.c >> @@ -170,6 +170,9 @@ struct stm32_dcmi { >> >> /* Ensure DMA operations atomicity */ >> struct mutex dma_lock; >> + >> + struct media_device mdev; >> + struct media_pad vid_cap_pad; >> }; >> >> static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n) >> @@ -1545,21 +1548,12 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier) >> dev_err(dcmi->dev, "Could not get sensor bounds\n"); >> return ret; >> } >> - >> ret = dcmi_set_default_fmt(dcmi); >> if (ret) { >> dev_err(dcmi->dev, "Could not set default format\n"); >> return ret; >> } >> >> - ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1); >> - if (ret) { >> - dev_err(dcmi->dev, "Failed to register video device\n"); >> - return ret; >> - } >> - >> - dev_dbg(dcmi->dev, "Device registered as %s\n", >> - video_device_node_name(dcmi->vdev)); >> return 0; >> } >> >> @@ -1648,6 +1642,12 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi) >> return 0; >> } >> >> +static void dcmi_graph_deinit(struct stm32_dcmi *dcmi) >> +{ >> + v4l2_async_notifier_unregister(&dcmi->notifier); >> + v4l2_async_notifier_cleanup(&dcmi->notifier); > > I'd just leave the calls where they are now. This doesn't improve > readability of the code, rather the opposite. > fixed in v3. >> +} >> + >> static int dcmi_probe(struct platform_device *pdev) >> { >> struct device_node *np = pdev->dev.of_node; >> @@ -1752,10 +1752,27 @@ static int dcmi_probe(struct platform_device *pdev) >> >> q = &dcmi->queue; >> >> + dcmi->v4l2_dev.mdev = &dcmi->mdev; >> + >> + /* Initialize media device */ >> + strscpy(dcmi->mdev.model, DRV_NAME, sizeof(dcmi->mdev.model)); >> + snprintf(dcmi->mdev.bus_info, sizeof(dcmi->mdev.bus_info), >> + "platform:%s", DRV_NAME); >> + dcmi->mdev.dev = &pdev->dev; >> + media_device_init(&dcmi->mdev); >> + >> + /* Register the media device */ >> + ret = media_device_register(&dcmi->mdev); >> + if (ret) { >> + dev_err(dcmi->dev, "Failed to register media device (%d)\n", >> + ret); >> + goto err_media_device_cleanup; >> + } >> + >> /* Initialize the top-level structure */ >> ret = v4l2_device_register(&pdev->dev, &dcmi->v4l2_dev); >> if (ret) >> - goto err_dma_release; >> + goto err_media_device_unregister; >> >> dcmi->vdev = video_device_alloc(); >> if (!dcmi->vdev) { >> @@ -1775,6 +1792,25 @@ static int dcmi_probe(struct platform_device *pdev) >> V4L2_CAP_READWRITE; >> video_set_drvdata(dcmi->vdev, dcmi); >> >> + /* Media entity pads */ >> + dcmi->vid_cap_pad.flags = MEDIA_PAD_FL_SINK; >> + ret = media_entity_pads_init(&dcmi->vdev->entity, >> + 1, &dcmi->vid_cap_pad); >> + if (ret) { >> + dev_err(dcmi->dev, "Failed to init media entity pad\n"); >> + goto err_device_unregister; >> + } >> + dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT; >> + >> + ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1); >> + if (ret) { >> + dev_err(dcmi->dev, "Failed to register video device\n"); >> + goto err_media_entity_cleanup; >> + } >> + >> + dev_dbg(dcmi->dev, "Device registered as %s\n", >> + video_device_node_name(dcmi->vdev)); >> + >> /* Buffer queue */ >> q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; >> q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF; >> @@ -1790,18 +1826,18 @@ static int dcmi_probe(struct platform_device *pdev) >> ret = vb2_queue_init(q); >> if (ret < 0) { >> dev_err(&pdev->dev, "Failed to initialize vb2 queue\n"); >> - goto err_device_release; >> + goto err_media_entity_cleanup; >> } >> >> ret = dcmi_graph_init(dcmi); >> if (ret < 0) >> - goto err_device_release; >> + goto err_media_entity_cleanup; >> >> /* Reset device */ >> ret = reset_control_assert(dcmi->rstc); >> if (ret) { >> dev_err(&pdev->dev, "Failed to assert the reset line\n"); >> - goto err_cleanup; >> + goto err_graph_deinit; >> } >> >> usleep_range(3000, 5000); >> @@ -1809,7 +1845,7 @@ static int dcmi_probe(struct platform_device *pdev) >> ret = reset_control_deassert(dcmi->rstc); >> if (ret) { >> dev_err(&pdev->dev, "Failed to deassert the reset line\n"); >> - goto err_cleanup; >> + goto err_graph_deinit; >> } >> >> dev_info(&pdev->dev, "Probe done\n"); >> @@ -1820,13 +1856,16 @@ static int dcmi_probe(struct platform_device *pdev) >> >> return 0; >> >> -err_cleanup: >> - v4l2_async_notifier_cleanup(&dcmi->notifier); >> -err_device_release: >> - video_device_release(dcmi->vdev); >> +err_graph_deinit: >> + dcmi_graph_deinit(dcmi); >> +err_media_entity_cleanup: >> + media_entity_cleanup(&dcmi->vdev->entity); >> err_device_unregister: >> v4l2_device_unregister(&dcmi->v4l2_dev); >> -err_dma_release: >> +err_media_device_unregister: >> + media_device_unregister(&dcmi->mdev); >> +err_media_device_cleanup: >> + media_device_cleanup(&dcmi->mdev); >> dma_release_channel(dcmi->dma_chan); >> >> return ret; >> @@ -1838,9 +1877,11 @@ static int dcmi_remove(struct platform_device *pdev) >> >> pm_runtime_disable(&pdev->dev); >> >> - v4l2_async_notifier_unregister(&dcmi->notifier); >> - v4l2_async_notifier_cleanup(&dcmi->notifier); >> + dcmi_graph_deinit(dcmi); >> + media_entity_cleanup(&dcmi->vdev->entity); >> v4l2_device_unregister(&dcmi->v4l2_dev); >> + media_device_unregister(&dcmi->mdev); > > Please unregister the media device first before unregistering anything else > it depends on (i.e. async notifier or the entity). > Media device registry is dropped in v3 to not expose media controller interfaces to userspace as discussed here: https://www.spinics.net/lists/linux-media/msg153417.html >> + media_device_cleanup(&dcmi->mdev); >> >> dma_release_channel(dcmi->dma_chan); >> > BR, Hugues.
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig index 8a19654..de7e21f 100644 --- a/drivers/media/platform/Kconfig +++ b/drivers/media/platform/Kconfig @@ -121,7 +121,7 @@ config VIDEO_S3C_CAMIF config VIDEO_STM32_DCMI tristate "STM32 Digital Camera Memory Interface (DCMI) support" - depends on VIDEO_V4L2 && OF + depends on VIDEO_V4L2 && OF && MEDIA_CONTROLLER depends on ARCH_STM32 || COMPILE_TEST select VIDEOBUF2_DMA_CONTIG select V4L2_FWNODE diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c index 7a4d559..3a69783 100644 --- a/drivers/media/platform/stm32/stm32-dcmi.c +++ b/drivers/media/platform/stm32/stm32-dcmi.c @@ -170,6 +170,9 @@ struct stm32_dcmi { /* Ensure DMA operations atomicity */ struct mutex dma_lock; + + struct media_device mdev; + struct media_pad vid_cap_pad; }; static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n) @@ -1545,21 +1548,12 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier) dev_err(dcmi->dev, "Could not get sensor bounds\n"); return ret; } - ret = dcmi_set_default_fmt(dcmi); if (ret) { dev_err(dcmi->dev, "Could not set default format\n"); return ret; } - ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1); - if (ret) { - dev_err(dcmi->dev, "Failed to register video device\n"); - return ret; - } - - dev_dbg(dcmi->dev, "Device registered as %s\n", - video_device_node_name(dcmi->vdev)); return 0; } @@ -1648,6 +1642,12 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi) return 0; } +static void dcmi_graph_deinit(struct stm32_dcmi *dcmi) +{ + v4l2_async_notifier_unregister(&dcmi->notifier); + v4l2_async_notifier_cleanup(&dcmi->notifier); +} + static int dcmi_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -1752,10 +1752,27 @@ static int dcmi_probe(struct platform_device *pdev) q = &dcmi->queue; + dcmi->v4l2_dev.mdev = &dcmi->mdev; + + /* Initialize media device */ + strscpy(dcmi->mdev.model, DRV_NAME, sizeof(dcmi->mdev.model)); + snprintf(dcmi->mdev.bus_info, sizeof(dcmi->mdev.bus_info), + "platform:%s", DRV_NAME); + dcmi->mdev.dev = &pdev->dev; + media_device_init(&dcmi->mdev); + + /* Register the media device */ + ret = media_device_register(&dcmi->mdev); + if (ret) { + dev_err(dcmi->dev, "Failed to register media device (%d)\n", + ret); + goto err_media_device_cleanup; + } + /* Initialize the top-level structure */ ret = v4l2_device_register(&pdev->dev, &dcmi->v4l2_dev); if (ret) - goto err_dma_release; + goto err_media_device_unregister; dcmi->vdev = video_device_alloc(); if (!dcmi->vdev) { @@ -1775,6 +1792,25 @@ static int dcmi_probe(struct platform_device *pdev) V4L2_CAP_READWRITE; video_set_drvdata(dcmi->vdev, dcmi); + /* Media entity pads */ + dcmi->vid_cap_pad.flags = MEDIA_PAD_FL_SINK; + ret = media_entity_pads_init(&dcmi->vdev->entity, + 1, &dcmi->vid_cap_pad); + if (ret) { + dev_err(dcmi->dev, "Failed to init media entity pad\n"); + goto err_device_unregister; + } + dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT; + + ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1); + if (ret) { + dev_err(dcmi->dev, "Failed to register video device\n"); + goto err_media_entity_cleanup; + } + + dev_dbg(dcmi->dev, "Device registered as %s\n", + video_device_node_name(dcmi->vdev)); + /* Buffer queue */ q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE; q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF; @@ -1790,18 +1826,18 @@ static int dcmi_probe(struct platform_device *pdev) ret = vb2_queue_init(q); if (ret < 0) { dev_err(&pdev->dev, "Failed to initialize vb2 queue\n"); - goto err_device_release; + goto err_media_entity_cleanup; } ret = dcmi_graph_init(dcmi); if (ret < 0) - goto err_device_release; + goto err_media_entity_cleanup; /* Reset device */ ret = reset_control_assert(dcmi->rstc); if (ret) { dev_err(&pdev->dev, "Failed to assert the reset line\n"); - goto err_cleanup; + goto err_graph_deinit; } usleep_range(3000, 5000); @@ -1809,7 +1845,7 @@ static int dcmi_probe(struct platform_device *pdev) ret = reset_control_deassert(dcmi->rstc); if (ret) { dev_err(&pdev->dev, "Failed to deassert the reset line\n"); - goto err_cleanup; + goto err_graph_deinit; } dev_info(&pdev->dev, "Probe done\n"); @@ -1820,13 +1856,16 @@ static int dcmi_probe(struct platform_device *pdev) return 0; -err_cleanup: - v4l2_async_notifier_cleanup(&dcmi->notifier); -err_device_release: - video_device_release(dcmi->vdev); +err_graph_deinit: + dcmi_graph_deinit(dcmi); +err_media_entity_cleanup: + media_entity_cleanup(&dcmi->vdev->entity); err_device_unregister: v4l2_device_unregister(&dcmi->v4l2_dev); -err_dma_release: +err_media_device_unregister: + media_device_unregister(&dcmi->mdev); +err_media_device_cleanup: + media_device_cleanup(&dcmi->mdev); dma_release_channel(dcmi->dma_chan); return ret; @@ -1838,9 +1877,11 @@ static int dcmi_remove(struct platform_device *pdev) pm_runtime_disable(&pdev->dev); - v4l2_async_notifier_unregister(&dcmi->notifier); - v4l2_async_notifier_cleanup(&dcmi->notifier); + dcmi_graph_deinit(dcmi); + media_entity_cleanup(&dcmi->vdev->entity); v4l2_device_unregister(&dcmi->v4l2_dev); + media_device_unregister(&dcmi->mdev); + media_device_cleanup(&dcmi->mdev); dma_release_channel(dcmi->dma_chan);
Add media controller support to dcmi. Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> --- drivers/media/platform/Kconfig | 2 +- drivers/media/platform/stm32/stm32-dcmi.c | 83 +++++++++++++++++++++++-------- 2 files changed, 63 insertions(+), 22 deletions(-)