Message-ID: <abe77aed-c777-4892-9fce-aaad508700f6@kernel.org>
Date: Tue, 22 Oct 2024 07:44:46 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Shu-hsiang Yang <Shu-hsiang.Yang@...iatek.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 Sumit Semwal <sumit.semwal@...aro.org>,
 Christian Konig <christian.koenig@....com>
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org, dri-devel@...ts.freedesktop.org,
 linaro-mm-sig@...ts.linaro.org,
 Project_Global_Chrome_Upstream_Group@...iatek.com, yaya.chang@...iatek.com,
 teddy.chen@...iatek.com, hidenorik@...omium.org, yunkec@...omium.org,
 shun-yi.wang@...iatek.com
Subject: Re: [PATCH v1 05/10] media: platform: mediatek: add isp_7x camsys
 unit

On 09/10/2024 13:15, Shu-hsiang Yang wrote:
> Introduces the top media device driver for the MediaTek ISP7X CAMSYS.
> The driver maintains the camera system, including sub-device management,
> DMA operations, and integration with the V4L2 framework. It handles
> request stream data, buffer management, and MediaTek-specific features,
> and pipeline management, streaming control, error handling mechanism.
> Additionally, it aggregates sub-drivers for the camera interface, raw
> and yuv pipelines.
> 
> Signed-off-by: Shu-hsiang Yang <Shu-hsiang.Yang@...iatek.com>

...

> +
> +static int mtk_cam_probe(struct platform_device *pdev)
> +{
> +	struct mtk_cam_device *cam_dev;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	int ret;
> +	unsigned int i;
> +
> +	dev_dbg(dev, "camsys | start %s\n", __func__);

NAK. Same issues.

> +
> +	/* initialize structure */
> +	cam_dev = devm_kzalloc(dev, sizeof(*cam_dev), GFP_KERNEL);
> +	if (!cam_dev)
> +		return -ENOMEM;
> +
> +	if (dma_set_mask_and_coherent(dev, DMA_BIT_MASK(34))) {
> +		dev_err(dev, "%s: No suitable DMA available\n", __func__);
> +		return -EIO;
> +	}
> +
> +	if (!dev->dma_parms) {
> +		dev->dma_parms =
> +			devm_kzalloc(dev, sizeof(*dev->dma_parms), GFP_KERNEL);
> +		if (!dev->dma_parms)
> +			return -ENOMEM;
> +	}
> +
> +	dma_set_max_seg_size(dev, UINT_MAX);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(dev, "failed to get mem\n");
> +		return -ENODEV;
> +	}
> +
> +	cam_dev->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(cam_dev->base)) {
> +		dev_err(dev, "failed to map register base\n");
> +		return PTR_ERR(cam_dev->base);
> +	}
> +
> +	cam_dev->dev = dev;
> +	dev_set_drvdata(dev, cam_dev);
> +
> +	cam_dev->composer_cnt = 0;
> +	cam_dev->num_seninf_devices = 0;
> +
> +	cam_dev->max_stream_num = MTKCAM_SUBDEV_MAX;
> +	cam_dev->ctxs = devm_kcalloc(dev, cam_dev->max_stream_num,
> +				     sizeof(*cam_dev->ctxs), GFP_KERNEL);
> +	if (!cam_dev->ctxs)
> +		return -ENOMEM;
> +
> +	cam_dev->streaming_ctx = 0;
> +	for (i = 0; i < cam_dev->max_stream_num; i++)
> +		mtk_cam_ctx_init(cam_dev->ctxs + i, cam_dev, i);
> +
> +	cam_dev->running_job_count = 0;
> +	spin_lock_init(&cam_dev->pending_job_lock);
> +	spin_lock_init(&cam_dev->running_job_lock);
> +	INIT_LIST_HEAD(&cam_dev->pending_job_list);
> +	INIT_LIST_HEAD(&cam_dev->running_job_list);
> +
> +	cam_dev->dma_processing_count = 0;
> +	spin_lock_init(&cam_dev->dma_pending_lock);
> +	spin_lock_init(&cam_dev->dma_processing_lock);
> +	INIT_LIST_HEAD(&cam_dev->dma_pending);
> +	INIT_LIST_HEAD(&cam_dev->dma_processing);
> +
> +	mutex_init(&cam_dev->queue_lock);
> +
> +	pm_runtime_enable(dev);
> +
> +	ret = mtk_cam_of_rproc(cam_dev, pdev);
> +	if (ret)
> +		goto fail_destroy_mutex;
> +
> +	ret = register_sub_drivers(dev);
> +	if (ret) {
> +		dev_err(dev, "fail to register_sub_drivers\n");
> +		goto fail_destroy_mutex;
> +	}
> +
> +	/* register mtk_cam as all isp subdev async parent */
> +	cam_dev->notifier.ops = &mtk_cam_async_nf_ops;
> +	v4l2_async_nf_init(&cam_dev->notifier, &cam_dev->v4l2_dev);
> +	ret = mtk_cam_async_subdev_add(dev); /* wait all isp sub drivers */
> +	if (ret) {
> +		dev_err(dev, "%s failed mtk_cam_async_subdev_add\n", __func__);
> +		goto fail_unregister_sub_drivers;
> +	}
> +
> +	ret = v4l2_async_nf_register(&cam_dev->notifier);
> +	if (ret) {
> +		dev_err(dev, "%s async_nf_register ret:%d\n", __func__, ret);
> +		v4l2_async_nf_cleanup(&cam_dev->notifier);
> +		goto fail_unregister_sub_drivers;
> +	}
> +
> +	ret = mtk_cam_debug_fs_init(cam_dev);
> +	if (ret < 0)
> +		goto fail_unregister_async_nf;
> +
> +	dev_info(dev, "camsys | [%s] success\n", __func__);

NAK. Same issues.

> +
> +	return 0;
> +
> +fail_unregister_async_nf:
> +	v4l2_async_nf_unregister(&cam_dev->notifier);
> +
> +fail_unregister_sub_drivers:
> +	unregister_sub_drivers(dev);
> +
> +fail_destroy_mutex:
> +	mutex_destroy(&cam_dev->queue_lock);
> +
> +	return ret;
> +}
> +
> +static void mtk_cam_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mtk_cam_device *cam_dev = dev_get_drvdata(dev);
> +
> +	pm_runtime_disable(dev);
> +
> +	mtk_cam_debug_fs_deinit(cam_dev);
> +
> +	v4l2_async_nf_unregister(&cam_dev->notifier);
> +
> +	unregister_sub_drivers(dev);
> +
> +	mutex_destroy(&cam_dev->queue_lock);
> +}
> +
> +static const struct dev_pm_ops mtk_cam_pm_ops = {
> +	SET_RUNTIME_PM_OPS(mtk_cam_runtime_suspend, mtk_cam_runtime_resume,
> +			   NULL)
> +};
> +
> +static struct platform_driver mtk_cam_driver = {
> +	.probe   = mtk_cam_probe,
> +	.remove  = mtk_cam_remove,
> +	.driver  = {
> +		.name  = "mtk-cam",
> +		.of_match_table = of_match_ptr(mtk_cam_of_ids),

Same issues as in previous patch.

All my comments apply to all your patches in this thread.

> +		.pm     = &mtk_cam_pm_ops,
> +	}
> +};



Best regards,
Krzysztof

Powered by blists - more mailing lists