Re: [PATCH v2 2/2] MEM2MEM: Add support for eMMa-PrP mem2mem operations.

[Sascha Hauer <s.hauer@pengutronix.de>]

Hi Javier,

On Tue, Nov 22, 2011 at 01:01:56PM +0100, Javier Martin wrote:
> Changes since v1:
> - Embed queue data in ctx structure to allow multi instance.
> - Remove redundant job_ready callback.
> - Adjust format against device capabilities.
> - Register/unregister video device at the right time.
> - Other minor coding fixes.
> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
>  drivers/media/video/Kconfig       |   10 +
>  drivers/media/video/Makefile      |    2 +
>  drivers/media/video/mx2_emmaprp.c | 1035 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 1047 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/mx2_emmaprp.c
> 
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index b303a3f..77d7921 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -1107,4 +1107,14 @@ config VIDEO_SAMSUNG_S5P_MFC
>  	help
>  	    MFC 5.1 driver for V4L2.
>  
> +config VIDEO_MX2_EMMAPRP
> +	tristate "MX2 eMMa-PrP support"
> +	depends on VIDEO_DEV && VIDEO_V4L2 && MACH_MX27

Please do not add new references to MACH_MX27. Use SOC_IMX27 instead.

> +	select VIDEOBUF2_DMA_CONTIG
> +	select V4L2_MEM2MEM_DEV
> +	help
> +	    MX2X chips have a PrP that can be used to process buffers from
> +	    memory to memory. Operations include resizing and format
> +	    conversion.
> +

[...]

> +
> +static int emmaprp_probe(struct platform_device *pdev)
> +{
> +	struct emmaprp_dev *pcdev;
> +	struct video_device *vfd;
> +	struct resource *res_emma;
> +	int irq_emma;
> +	int ret;
> +
> +	pcdev = kzalloc(sizeof *pcdev, GFP_KERNEL);
> +	if (!pcdev)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&pcdev->irqlock);
> +
> +	pcdev->clk_emma = clk_get(NULL, "emma");

You should change the entry for the emma in
arch/arm/mach-imx/clock-imx27.c to the following:

_REGISTER_CLOCK("m2m-emmaprp", NULL, emma_clk)

and use clk_get(&pdev->dev, NULL) here.

> +	if (IS_ERR(pcdev->clk_emma)) {
> +		ret = PTR_ERR(pcdev->clk_emma);
> +		goto free_dev;
> +	}
> +
> +	irq_emma = platform_get_irq(pdev, 0);
> +	res_emma = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (irq_emma < 0 || res_emma == NULL) {
> +		dev_err(&pdev->dev, "Missing platform resources data\n");
> +		ret = -ENODEV;
> +		goto free_clk;
> +	}
> +
> +	ret = v4l2_device_register(&pdev->dev, &pcdev->v4l2_dev);
> +	if (ret)
> +		goto free_clk;
> +
> +	mutex_init(&pcdev->dev_mutex);
> +
> +	vfd = video_device_alloc();
> +	if (!vfd) {
> +		v4l2_err(&pcdev->v4l2_dev, "Failed to allocate video device\n");
> +		ret = -ENOMEM;
> +		goto unreg_dev;
> +	}
> +
> +	*vfd = emmaprp_videodev;
> +	vfd->lock = &pcdev->dev_mutex;
> +
> +	video_set_drvdata(vfd, pcdev);
> +	snprintf(vfd->name, sizeof(vfd->name), "%s", emmaprp_videodev.name);
> +	pcdev->vfd = vfd;
> +	v4l2_info(&pcdev->v4l2_dev, EMMAPRP_MODULE_NAME
> +			" Device registered as /dev/video%d\n", vfd->num);
> +
> +	platform_set_drvdata(pdev, pcdev);
> +
> +	if (!request_mem_region(res_emma->start, resource_size(res_emma),
> +				MEM2MEM_NAME)) {
> +		ret = -EBUSY;
> +		goto rel_vdev;
> +	}
> +
> +	pcdev->base_emma = ioremap(res_emma->start, resource_size(res_emma));
> +	if (!pcdev->base_emma) {
> +		ret = -ENOMEM;
> +		goto rel_mem;
> +	}
> +	pcdev->irq_emma = irq_emma;
> +	pcdev->res_emma = res_emma;
> +
> +	ret = request_irq(pcdev->irq_emma, emmaprp_irq, 0,
> +			  MEM2MEM_NAME, pcdev);
> +	if (ret)
> +		goto rel_map;
> +

consider using devm_request_mem_region, devm_ioremap and
devm_request_irq here. It simplifies your error handling considerably.

> +
> +	pcdev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
> +	if (IS_ERR(pcdev->alloc_ctx)) {
> +		v4l2_err(&pcdev->v4l2_dev, "Failed to alloc vb2 context\n");
> +		ret = PTR_ERR(pcdev->alloc_ctx);
> +		goto rel_irq;
> +	}
> +
> +	pcdev->m2m_dev = v4l2_m2m_init(&m2m_ops);
> +	if (IS_ERR(pcdev->m2m_dev)) {
> +		v4l2_err(&pcdev->v4l2_dev, "Failed to init mem2mem device\n");
> +		ret = PTR_ERR(pcdev->m2m_dev);
> +		goto rel_ctx;
> +	}
> +

[...]

> +
> +static struct platform_driver emmaprp_pdrv = {
> +	.probe		= emmaprp_probe,
> +	.remove		= emmaprp_remove,
> +	.driver		= {
> +		.name	= MEM2MEM_NAME,
> +		.owner	= THIS_MODULE,
> +	},
> +};
> +
> +static void __exit emmaprp_exit(void)
> +{
> +	platform_driver_unregister(&emmaprp_pdrv);
> +}
> +
> +static int __init emmaprp_init(void)
> +{
> +	return platform_driver_register(&emmaprp_pdrv);
> +}
> +
> +module_init(emmaprp_init);
> +module_exit(emmaprp_exit);
> +

No blank line at end of file please.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |