Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0d0cec7f-030f-ebc1-11f0-06214197a351@linaro.org>
Date:   Fri, 14 Jul 2023 10:00:23 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v7 2/5] media: platform: visconti: Add Toshiba Visconti
 Video Input Interface driver
Content-Language: en-US
To:     Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Sakari Ailus <sakari.ailus@iki.fi>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>,
        Mark Brown <broonie@kernel.org>
Cc:     linux-media@vger.kernel.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20230714015059.18775-1-yuji2.ishikawa@toshiba.co.jp>
 <20230714015059.18775-3-yuji2.ishikawa@toshiba.co.jp>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230714015059.18775-3-yuji2.ishikawa@toshiba.co.jp>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 14/07/2023 03:50, Yuji Ishikawa wrote:
> Add support to Video Input Interface on Toshiba Visconti ARM SoCs.
> The interface device includes CSI2 Receiver,
> frame grabber, video DMAC and image signal processor.
> 
> A driver instance provides three /dev/videoX device files;
> one for RGB image capture, another one for optional RGB capture
> with different parameters and the last one for RAW capture.
> 

...

> +static int visconti_viif_parse_dt(struct viif_device *viif_dev)
> +{
> +	struct device_node *of = viif_dev->dev->of_node;
> +	struct v4l2_fwnode_endpoint fw_ep;
> +	struct viif_subdev *viif_sd;
> +	struct device_node *ep;
> +	unsigned int i;
> +	int num_ep;
> +	int ret;
> +
> +	memset(&fw_ep, 0, sizeof(struct v4l2_fwnode_endpoint));

Why you cannot initialize it in declaration? = { 0 }?

> +
> +	num_ep = of_graph_get_endpoint_count(of);
> +	if (!num_ep)
> +		return -ENODEV;
> +
> +	ret = visconti_viif_init_async_subdevs(viif_dev, num_ep);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < num_ep; i++) {
> +		ep = of_graph_get_endpoint_by_regs(of, 0, i);
> +		if (!ep) {
> +			dev_err(viif_dev->dev, "No subdevice connected on endpoint %u.\n", i);
> +			ret = -ENODEV;
> +			goto error_put_node;
> +		}
> +
> +		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &fw_ep);
> +		if (ret) {
> +			dev_err(viif_dev->dev, "Unable to parse endpoint #%u.\n", i);
> +			goto error_put_node;
> +		}
> +
> +		if (fw_ep.bus_type != V4L2_MBUS_CSI2_DPHY ||
> +		    fw_ep.bus.mipi_csi2.num_data_lanes == 0) {
> +			dev_err(viif_dev->dev, "missing CSI-2 properties in endpoint\n");
> +			ret = -EINVAL;
> +			goto error_put_node;
> +		}
> +
> +		/* Setup the ceu subdevice and the async subdevice. */
> +		viif_sd = &viif_dev->subdevs[i];
> +		INIT_LIST_HEAD(&viif_sd->asd.list);
> +
> +		viif_sd->num_lane = fw_ep.bus.mipi_csi2.num_data_lanes;
> +		viif_sd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +		viif_sd->asd.match.fwnode =
> +			fwnode_graph_get_remote_port_parent(of_fwnode_handle(ep));
> +
> +		viif_dev->asds[i] = &viif_sd->asd;
> +		of_node_put(ep);
> +	}
> +
> +	return num_ep;
> +
> +error_put_node:
> +	of_node_put(ep);
> +	return ret;
> +}
> +
> +static const struct of_device_id visconti_viif_of_table[] = {
> +	{
> +		.compatible = "toshiba,visconti5-viif",
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, visconti_viif_of_table);
> +
> +#define NUM_IRQS   3
> +#define IRQ_ID_STR "viif"
> +
> +static int visconti_viif_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct viif_device *viif_dev;
> +	dma_addr_t tables_dma;
> +	int ret, i, num_sd;
> +
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
> +	if (ret)
> +		return ret;
> +
> +	viif_dev = devm_kzalloc(dev, sizeof(*viif_dev), GFP_KERNEL);
> +	if (!viif_dev)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, viif_dev);
> +	viif_dev->dev = dev;
> +
> +	spin_lock_init(&viif_dev->regbuf_lock);
> +	mutex_init(&viif_dev->pow_lock);
> +	mutex_init(&viif_dev->stream_lock);
> +
> +	viif_dev->capture_reg = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(viif_dev->capture_reg))
> +		return PTR_ERR(viif_dev->capture_reg);
> +
> +	viif_dev->csi2host_reg = devm_platform_ioremap_resource(pdev, 1);
> +	if (IS_ERR(viif_dev->csi2host_reg))
> +		return PTR_ERR(viif_dev->csi2host_reg);
> +
> +	viif_dev->hwaif_reg = devm_platform_ioremap_resource(pdev, 2);
> +	if (IS_ERR(viif_dev->hwaif_reg))
> +		return PTR_ERR(viif_dev->hwaif_reg);
> +
> +	viif_dev->mpu_reg = devm_platform_ioremap_resource(pdev, 3);
> +	if (IS_ERR(viif_dev->mpu_reg))
> +		return PTR_ERR(viif_dev->mpu_reg);
> +
> +	viif_dev->run_flag_main = false;
> +
> +	for (i = 0; i < NUM_IRQS; i++) {
> +		ret = platform_get_irq(pdev, i);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to acquire irq resource\n");
> +			return ret;

return dev_err_probe()

> +		}
> +		viif_dev->irq[i] = ret;
> +		ret = devm_request_irq(dev, viif_dev->irq[i], visconti_viif_irq, 0, IRQ_ID_STR,
> +				       viif_dev);
> +		if (ret) {
> +			dev_err(dev, "irq request failed\n");

return dev_err_probe()

> +			return ret;
> +		}
> +	}
> +
> +	viif_dev->tables =
> +		dma_alloc_wc(dev, sizeof(struct viif_table_area), &tables_dma, GFP_KERNEL);
> +	if (!viif_dev->tables) {
> +		dev_err(dev, "dma_alloc_wc failed\n");

Are you sure DMA memory allocation errors shall be printed?

> +		return -ENOMEM;
> +	}
> +	viif_dev->tables_dma = (struct viif_table_area *)tables_dma;
> +
> +	/* power control */

Drop the comment, it is useless.

> +	pm_runtime_enable(dev);
> +
> +	/* build media_dev */
> +	viif_dev->media_dev.hw_revision = 0;
> +	strscpy(viif_dev->media_dev.model, VIIF_DRIVER_NAME, sizeof(viif_dev->media_dev.model));
> +	viif_dev->media_dev.dev = dev;
> +	/* TODO: platform:visconti-viif-0,1,2,3 for each VIIF driver instance */
> +	snprintf(viif_dev->media_dev.bus_info, sizeof(viif_dev->media_dev.bus_info), "%s-0",
> +		 VIIF_BUS_INFO_BASE);
> +	media_device_init(&viif_dev->media_dev);
> +
> +	/* build v4l2_dev */
> +	viif_dev->v4l2_dev.mdev = &viif_dev->media_dev;
> +	ret = v4l2_device_register(dev, &viif_dev->v4l2_dev);
> +	if (ret)
> +		goto error_dma_free;
> +
> +	ret = media_device_register(&viif_dev->media_dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register media device: %d\n", ret);
> +		goto error_v4l2_unregister;

dev_err_probe

> +	}
> +
> +	ret = visconti_viif_csi2rx_register(viif_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to register csi2rx sub node: %d\n", ret);

dev_err_probe


> +		goto error_media_unregister;
> +	}
> +
> +	ret = visconti_viif_isp_register(viif_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to register isp sub node: %d\n", ret);

dev_err_probe


> +		goto error_media_unregister;
> +	}
> +	ret = visconti_viif_capture_register(viif_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to register capture node: %d\n", ret);

dev_err_probe

> +		goto error_media_unregister;
> +	}
> +
> +	/* handle subdevices in device tree */
> +	num_sd = visconti_viif_parse_dt(viif_dev);
> +	if (ret < 0) {
> +		ret = num_sd;

ret = dev_err_probe

> +		goto error_media_unregister;
> +	}
> +
> +	viif_dev->notifier.v4l2_dev = &viif_dev->v4l2_dev;
> +	v4l2_async_nf_init(&viif_dev->notifier);
> +	for (i = 0; i < num_sd; i++)
> +		__v4l2_async_nf_add_subdev(&viif_dev->notifier, viif_dev->asds[i]);
> +	viif_dev->notifier.ops = &viif_notify_ops;
> +	ret = v4l2_async_nf_register(&viif_dev->v4l2_dev, &viif_dev->notifier);
> +	if (ret)
> +		goto error_media_unregister;
> +
> +	viif_dev->wq = create_workqueue("visconti-viif");
> +	if (!viif_dev->wq)
> +		return -ENOMEM;

No error cleanup?

> +	INIT_WORK(&viif_dev->work, visconti_viif_wthread_l1info);
> +
> +	return 0;
> +
> +error_media_unregister:
> +	media_device_unregister(&viif_dev->media_dev);
> +error_v4l2_unregister:
> +	v4l2_device_unregister(&viif_dev->v4l2_dev);
> +error_dma_free:
> +	pm_runtime_disable(dev);
> +	dma_free_wc(&pdev->dev, sizeof(struct viif_table_area), viif_dev->tables,
> +		    (dma_addr_t)viif_dev->tables_dma);
> +	return ret;
> +}
> +
> +static int visconti_viif_remove(struct platform_device *pdev)
> +{
> +	struct viif_device *viif_dev = platform_get_drvdata(pdev);
> +
> +	destroy_workqueue(viif_dev->wq);
> +	visconti_viif_isp_unregister(viif_dev);
> +	visconti_viif_capture_unregister(viif_dev);
> +	v4l2_async_nf_unregister(&viif_dev->notifier);

Why these three are not called in probe error paths?

> +	media_device_unregister(&viif_dev->media_dev);
> +	v4l2_device_unregister(&viif_dev->v4l2_dev);
> +	pm_runtime_disable(&pdev->dev);
> +	dma_free_wc(&pdev->dev, sizeof(struct viif_table_area), viif_dev->tables,
> +		    (dma_addr_t)viif_dev->tables_dma);
> +
> +	return 0;
> +}
> +
> +static int visconti_viif_runtime_suspend(struct device *dev)
> +{
> +	/* This callback is kicked when the last device-file is closed */
> +	struct viif_device *viif_dev = dev_get_drvdata(dev);
> +
> +	mutex_lock(&viif_dev->pow_lock);

Why?

> +	visconti_viif_hw_off(viif_dev);
> +	mutex_unlock(&viif_dev->pow_lock);
> +
> +	return 0;
> +}
> +
> +static int visconti_viif_runtime_resume(struct device *dev)
> +{
> +	/* This callback is kicked when the first device-file is opened */
> +	struct viif_device *viif_dev = dev_get_drvdata(dev);
> +
> +	viif_dev->rawpack_mode = (u32)VIIF_RAWPACK_DISABLE;
> +
> +	mutex_lock(&viif_dev->pow_lock);

Why?

> +
> +	/* Initialize HWD driver */
> +	visconti_viif_hw_on(viif_dev);
> +
> +	/* VSYNC mask setting of MAIN unit */
> +	viif_main_vsync_set_irq_mask(viif_dev, MASK_INT_M_SYNC_MASK_SET);
> +
> +	/* STATUS error mask setting of MAIN unit */
> +	viif_main_status_err_set_irq_mask(viif_dev, MASK_INT_M_DELAY_INT_ERROR);
> +
> +	/* VSYNC mask settings of SUB unit */
> +	viif_sub_vsync_set_irq_mask(viif_dev, MASK_INT_S_SYNC_MASK_SET);
> +
> +	/* STATUS error mask setting(unmask) of SUB unit */
> +	viif_sub_status_err_set_irq_mask(viif_dev,
> +					 MASK_INT_S_RESERVED_SET | MASK_INT_S_DELAY_INT_ERROR);
> +
> +	mutex_unlock(&viif_dev->pow_lock);
> +
> +	return 0;
> +}
> +

Best regards,
Krzysztof