Re: [PATCH 2/3] media: rockchip: Introduce driver for Rockhip's camera interface

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Maxime,

Just a handful of drive-by cleanup suggestions to help this smell less like a BSP driver...

(one day I still hope to get round to playing with what appears to be the DVP interface populated but unused inside my RK3288 box, and the camera module I picked up to supposedly fit it, but alas not today)

On 2020-04-03 3:21 pm, Maxime Chevallier wrote:
[...]
+static int rkcif_plat_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct device_node *node = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct v4l2_device *v4l2_dev;
+	struct rkcif_device *cif_dev;
+	const struct cif_match_data *data;
+	struct resource *res;
+	int i, ret, irq;
+
+	match = of_match_node(rkcif_plat_of_match, node);
+	if (IS_ERR(match))
+		return PTR_ERR(match);

of_device_get_match_data()

+
+	cif_dev = devm_kzalloc(dev, sizeof(*cif_dev), GFP_KERNEL);
+	if (!cif_dev)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, cif_dev);
+	cif_dev->dev = dev;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(dev, irq, rkcif_irq_handler, IRQF_SHARED,
+			       dev_driver_string(dev), dev);
+	if (ret < 0) {
+		dev_err(dev, "request irq failed: %d\n", ret);
+		return ret;
+	}
+
+	cif_dev->irq = irq;
+	data = match->data;
+	cif_dev->chip_id = data->chip_id;

It pretty much breaks even at the moment, but consider just holding a pointer to data itself rather than copying multiple fields into cif_dev, particularly if there's any likelihood of adding more in future. The couple of places clk_size and chip_id are used here don't really look like critical fast-paths where cache/TLB locality is super-important.

+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	cif_dev->base_addr = devm_ioremap_resource(dev, res);

devm_platform_ioremap_resource()

+
+	if (IS_ERR(cif_dev->base_addr))
+		return PTR_ERR(cif_dev->base_addr);
+
+	if (data->clks_num > RKCIF_MAX_BUS_CLK ||
+		data->rsts_num > RKCIF_MAX_RESET) {

It's silly to have runtime checks of constants - BUILD_BUG_ON() somewhere if you really want to validate that the driver data is self-consistent.

+		dev_err(dev, "out of range: clks(%d %d) rsts(%d %d)\n",
+			data->clks_num, RKCIF_MAX_BUS_CLK,
+			data->rsts_num, RKCIF_MAX_RESET);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < data->clks_num; i++) {
+		struct clk *clk = devm_clk_get(dev, data->clks[i]);
+
+		if (IS_ERR(clk)) {
+			dev_err(dev, "failed to get %s\n", data->clks[i]);
+			return PTR_ERR(clk);
+		}
+
+		cif_dev->clks[i] = clk;
+	}

All of the clock handling looks like it could use the clk_bulk_* APIs.

+
+	cif_dev->clk_size = data->clks_num;
+
+	for (i = 0; i < data->rsts_num; i++) {
+		struct reset_control *rst =
+			devm_reset_control_get(dev, data->rsts[i]);
+		if (IS_ERR(rst)) {
+			dev_err(dev, "failed to get %s\n", data->rsts[i]);
+			return PTR_ERR(rst);
+		}
+		cif_dev->cif_rst[i] = rst;
+	}

And possibly the reset_control_array_* APIs for resets? (assuming there isn't a subtle ordering requirement implicit in data->rsts)

+
+	/* Initialize the stream */
+	rkcif_stream_init(cif_dev);
+
+	strlcpy(cif_dev->media_dev.model, "rkcif",
+		sizeof(cif_dev->media_dev.model));
+	cif_dev->media_dev.dev = &pdev->dev;
+	v4l2_dev = &cif_dev->v4l2_dev;
+	v4l2_dev->mdev = &cif_dev->media_dev;
+	strlcpy(v4l2_dev->name, "rkcif", sizeof(v4l2_dev->name));
+	v4l2_ctrl_handler_init(&cif_dev->ctrl_handler, 8);
+	v4l2_dev->ctrl_handler = &cif_dev->ctrl_handler;
+
+	ret = v4l2_device_register(cif_dev->dev, &cif_dev->v4l2_dev);
+	if (ret < 0)
+		return ret;
+
+	media_device_init(&cif_dev->media_dev);
+
+	ret = media_device_register(&cif_dev->media_dev);
+	if (ret < 0) {
+		v4l2_err(v4l2_dev, "Failed to register media device: %d\n",
+			 ret);
+		goto err_unreg_v4l2_dev;
+	}
+
+	/* create & register platefom subdev (from of_node) */
+	ret = rkcif_register_platform_subdevs(cif_dev);
+	if (ret < 0)
+		goto err_unreg_media_dev;
+
+	ret = of_reserved_mem_device_init(dev);
+	if (ret)
+		v4l2_warn(v4l2_dev, "No reserved memory region assign to CIF\n");

It feels like that should probably happen earlier in the "resource acquisition" part of probe, before any external init/register calls that in principle could want to preallocate DMA buffers.

Also, is the lack of reserved memory really a warn-level condition? The DT binding doesn't even appear to treat a "memory-region" property as legal, and with CMA or (as appears to be the case for at least RK3288) an IOMMU, it should be largely moot anyway.

Robin.

+
+	pm_runtime_enable(&pdev->dev);
+
+	return 0;
+
+err_unreg_media_dev:
+	media_device_unregister(&cif_dev->media_dev);
+err_unreg_v4l2_dev:
+	v4l2_device_unregister(&cif_dev->v4l2_dev);
+	return ret;
+}



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux