[v1,2/6] media: imx-pxp: Add media controller support

Register a media device for the PXP, using the v4l2-mem2mem MC
infrastructure to populate the media graph. No media device operation is
implemented, the main use of the MC API is to allow consistent discovery
of media devices for userspace.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx-pxp.c | 37 ++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

Hi Michael,

On Thu, Jan 12, 2023 at 02:58:20PM +0100, Michael Tretter wrote:
> On Fri, 06 Jan 2023 15:32:23 +0200, Laurent Pinchart wrote:
> > Register a media device for the PXP, using the v4l2-mem2mem MC
> > infrastructure to populate the media graph. No media device operation is
> > implemented, the main use of the MC API is to allow consistent discovery
> > of media devices for userspace.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/platform/nxp/imx-pxp.c | 37 ++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> > 
> > diff --git a/drivers/media/platform/nxp/imx-pxp.c b/drivers/media/platform/nxp/imx-pxp.c
> > index dcb04217288b..132065c8b8b4 100644
> > --- a/drivers/media/platform/nxp/imx-pxp.c
> > +++ b/drivers/media/platform/nxp/imx-pxp.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/slab.h>
> >  
> > +#include <media/media-device.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-event.h>
> > @@ -200,6 +201,9 @@ struct pxp_pdata {
> >  struct pxp_dev {
> >  	struct v4l2_device	v4l2_dev;
> >  	struct video_device	vfd;
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +	struct media_device	mdev;
> > +#endif
> >  
> >  	struct clk		*clk;
> >  	void __iomem		*mmio;
> > @@ -1832,8 +1836,36 @@ static int pxp_probe(struct platform_device *pdev)
> >  		goto err_m2m;
> >  	}
> >  
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +	dev->mdev.dev = &pdev->dev;
> > +	strscpy(dev->mdev.model, MEM2MEM_NAME, sizeof(dev->mdev.model));
> > +	snprintf(dev->mdev.bus_info, sizeof(dev->mdev.bus_info), "platform:%s",
> > +		 dev_name(&pdev->dev));
> 
> v4l2-compliance gives the following warning:
> 
> 	warn: v4l2-compliance.cpp(642): media bus_info 'platform:30700000.pxp' differs from V4L2 bus_info 'platform:pxp'
> 
> I would change this patch to use the same name as in the V4L2 bus_info. Do you
> have a reason for including the address in the bus_info?

Actually, I should drop bus_info here. The documentation of
media_device_init() states

 * The bus_info field is set by media_device_init() for PCI and platform devices
 * if the field begins with '\0'.

media_device_init() calls media_set_bus_info() which is defined as

/**
 * media_set_bus_info() - Set bus_info field
 *
 * @bus_info:		Variable where to write the bus info (char array)
 * @bus_info_size:	Length of the bus_info
 * @dev:		Related struct device
 *
 * Sets bus information based on &dev. This is currently done for PCI and
 * platform devices. dev is required to be non-NULL for this to happen.
 *
 * This function is not meant to be called from drivers.
 */
static inline void
media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev)
{
	if (!dev)
		strscpy(bus_info, "no bus info", bus_info_size);
	else if (dev_is_platform(dev))
		snprintf(bus_info, bus_info_size, "platform:%s", dev_name(dev));
	else if (dev_is_pci(dev))
		snprintf(bus_info, bus_info_size, "PCI:%s", dev_name(dev));
}

v4l_querycap() also calls media_set_bus_info(). Including the device
name is thus the recommended option.

I'll include in v2 another patch that drops the bus_info from the
video_device to make v4l2-compliance happy.

> > +	media_device_init(&dev->mdev);
> > +	dev->v4l2_dev.mdev = &dev->mdev;
> > +
> > +	ret = v4l2_m2m_register_media_controller(dev->m2m_dev, vfd,
> > +						 MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to initialize media device\n");
> > +		goto err_vfd;
> > +	}
> > +
> > +	ret = media_device_register(&dev->mdev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to register media device\n");
> > +		goto err_m2m_mc;
> > +	}
> > +#endif
> > +
> >  	return 0;
> >  
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +err_m2m_mc:
> > +	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
> > +err_vfd:
> > +	video_unregister_device(vfd);
> > +#endif
> >  err_m2m:
> >  	v4l2_m2m_release(dev->m2m_dev);
> >  err_v4l2:
> > @@ -1854,6 +1886,11 @@ static int pxp_remove(struct platform_device *pdev)
> >  	clk_disable_unprepare(dev->clk);
> >  
> >  	v4l2_info(&dev->v4l2_dev, "Removing " MEM2MEM_NAME);
> > +
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +	media_device_unregister(&dev->mdev);
> > +	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
> > +#endif
> >  	video_unregister_device(&dev->vfd);
> >  	v4l2_m2m_release(dev->m2m_dev);
> >  	v4l2_device_unregister(&dev->v4l2_dev);