Re: [PATCH v2 08/13] media: platform: mediatek: add isp_7x camsys unit

[Krzysztof Kozlowski <krzk@kernel.org>]

On 07/07/2025 03:31, shangyao lin wrote:
> +static int mtk_cam_create_links(struct mtk_cam_device *cam)
> +{
> +	struct v4l2_subdev *sd;
> +	int ret = 0;
> +
> +	v4l2_device_for_each_subdev(sd, &cam->v4l2_dev) {
> +		if (sd->entity.function == MEDIA_ENT_F_VID_IF_BRIDGE)
> +			ret = config_bridge_pad_links(cam, sd);
> +	}
> +
> +	return ret;
> +}
> +
> +static int mtk_cam_master_register(struct device *dev)
> +{
> +	struct mtk_cam_device *cam_dev = dev_get_drvdata(dev);
> +	struct media_device *media_dev = &cam_dev->media_dev;
> +	int ret;
> +
> +	dev_info(dev, "camsys | start %s\n", __func__);

NAK, nothing improved, you already got comments on that. Provide
detailed changelog next time. And respond to EACH comment acknowledging
that you understood it.

This code still has so many trivial issues and terrible coding style,
including wrong indentation, wrong alignment/wrapping.


> +
> +	media_dev->dev = cam_dev->dev;
> +	strscpy(media_dev->model, dev_driver_string(dev),
> +		sizeof(media_dev->model));
> +	snprintf(media_dev->bus_info, sizeof(media_dev->bus_info),
> +		 "platform:%s", dev_name(dev));
> +	media_dev->hw_revision = 0;
> +	media_dev->ops = &mtk_cam_dev_ops;
> +	media_device_init(media_dev);
> +
> +	cam_dev->v4l2_dev.mdev = media_dev;
> +	ret = v4l2_device_register(cam_dev->dev, &cam_dev->v4l2_dev);
> +	if (ret) {
> +		dev_dbg(dev, "Failed to register V4L2 device: %d\n", ret);
> +		goto fail_media_device_cleanup;
> +	}
> +
> +	ret = media_device_register(media_dev);
> +	if (ret) {
> +		dev_dbg(dev, "Failed to register media device: %d\n",
> +			ret);
> +		goto fail_v4l2_device_unreg;
> +	}
> +
> +	dev_info(dev, "%s success\n", __func__);
> +	return 0;
> +
> +fail_v4l2_device_unreg:
> +	v4l2_device_unregister(&cam_dev->v4l2_dev);
> +
> +fail_media_device_cleanup:
> +	media_device_cleanup(&cam_dev->media_dev);
> +
> +	return ret;
> +}
> +
> +static void mtk_cam_master_unregister(struct device *dev)
> +{
> +	struct mtk_cam_device *cam_dev = dev_get_drvdata(dev);
> +
> +	dev_info(dev, "camsys | start %s\n", __func__);

NAK

> +
> +	media_device_unregister(&cam_dev->media_dev);
> +	v4l2_device_unregister(&cam_dev->v4l2_dev);
> +	media_device_cleanup(&cam_dev->media_dev);
> +}
> +
> +static int mtk_cam_async_add_by_driver(struct device *dev,
> +				       struct platform_driver *drv,
> +				       struct v4l2_async_notifier *notifier)
> +{
> +	struct fwnode_handle *fwnode;
> +	struct device *p;
> +	struct v4l2_async_connection *asc;
> +	int dev_num = 0;
> +
> +	p = platform_find_device_by_driver(NULL, &drv->driver);
> +	while (p) {
> +		dev_info(dev, "camsys | %s add %s\n", __func__, p->kobj.name);
> +
> +		fwnode = dev_fwnode(p);
> +		asc = v4l2_async_nf_add_fwnode(notifier, fwnode,
> +					       struct v4l2_async_connection);
> +		put_device(p);
> +

...

> +
> +	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;
> +	}
> +
> +    cam_dev->clks_pdev = platform_device_register_data(
> +        dev, "clk-mt8188-cam", PLATFORM_DEVID_AUTO, NULL, 0);
> +    if (IS_ERR(cam_dev->clks_pdev)) {
> +        ret = PTR_ERR(cam_dev->clks_pdev);
> +        dev_err(dev, "Failed to register cam clk device: %d\n", ret);
> +        goto fail_unregister_sub_drivers;
> +    }
> +	
> +	/* 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_clks;
> +	}
> +
> +	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_clks;
> +	}
> +
> +	dev_info(dev, "camsys | [%s] success\n", __func__);

You already got comments. Nothing improved here.

> +
> +	return 0;
> +
> +fail_unregister_clks:
> +    if (cam_dev->clks_pdev)
> +        platform_device_unregister(cam_dev->clks_pdev);
> +

Best regards,
Krzysztof