Re: [PATCH v7 3/6] media: starfive: camss: Add basic driver

Hi Jack,

On Tue, Aug 01, 2023 at 11:24:22AM +0800, Jack Zhu wrote:

On 2023/7/27 19:33, Laurent Pinchart wrote:

quoted

On Mon, Jun 19, 2023 at 07:28:35PM +0800, Jack Zhu wrote:

quoted

Add basic platform driver for StarFive Camera Subsystem.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Jack Zhu <jack.zhu@starfivetech.com>
---
 MAINTAINERS                                   |   1 +
 drivers/media/platform/Kconfig                |   1 +
 drivers/media/platform/Makefile               |   1 +
 drivers/media/platform/starfive/Kconfig       |   5 +
 drivers/media/platform/starfive/Makefile      |   2 +
 drivers/media/platform/starfive/camss/Kconfig |  16 +
 .../media/platform/starfive/camss/Makefile    |   8 +
 .../media/platform/starfive/camss/stf_camss.c | 338 ++++++++++++++++++
 .../media/platform/starfive/camss/stf_camss.h | 146 ++++++++
 9 files changed, 518 insertions(+)
 create mode 100644 drivers/media/platform/starfive/Kconfig
 create mode 100644 drivers/media/platform/starfive/Makefile
 create mode 100644 drivers/media/platform/starfive/camss/Kconfig
 create mode 100644 drivers/media/platform/starfive/camss/Makefile
 create mode 100644 drivers/media/platform/starfive/camss/stf_camss.c
 create mode 100644 drivers/media/platform/starfive/camss/stf_camss.h

[snip]

quoted

quoted

diff --git a/drivers/media/platform/starfive/camss/Kconfig b/drivers/media/platform/starfive/camss/Kconfig
new file mode 100644
index 000000000000..dafe1d24324b
--- /dev/null
+++ b/drivers/media/platform/starfive/camss/Kconfig

@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config VIDEO_STARFIVE_CAMSS
+	tristate "Starfive Camera Subsystem driver"
+	depends on V4L_PLATFORM_DRIVERS
+	depends on VIDEO_DEV && OF
+	depends on HAS_DMA

You need to depend on PM, otherwise the runtime PM operations will be
no-ops and the driver won't work as clocks won't be enabled.

OK, I will add dependency.

By the way, if it makes it easier for you, you don't need to acknowledge
every single review comment. You can reply to comments you disagree
with, or comments that you find unclear. Anything that you agree with
and will address in the next version can be left unanswered in your
e-mail replies. It's entirely up to you.

quoted

quoted

+	select MEDIA_CONTROLLER
+	select VIDEO_V4L2_SUBDEV_API
+	select VIDEOBUF2_DMA_CONTIG
+	select V4L2_FWNODE
+	help
+	   Enable this to support for the Starfive Camera subsystem
+	   found on Starfive JH7110 SoC.
+
+	   To compile this driver as a module, choose M here: the
+	   module will be called stf-camss.

[snip]

quoted

quoted

diff --git a/drivers/media/platform/starfive/camss/stf_camss.c b/drivers/media/platform/starfive/camss/stf_camss.c
new file mode 100644
index 000000000000..dc2b5dba7bd4
--- /dev/null
+++ b/drivers/media/platform/starfive/camss/stf_camss.c

@@ -0,0 +1,338 @@

[snip]

quoted

quoted

+/*
+ * stfcamss_probe - Probe STFCAMSS platform device
+ * @pdev: Pointer to STFCAMSS platform device
+ *
+ * Return 0 on success or a negative error code on failure
+ */
+static int stfcamss_probe(struct platform_device *pdev)
+{
+	struct stfcamss *stfcamss;
+	struct device *dev = &pdev->dev;
+	int ret, num_subdevs;
+	unsigned int i;
+
+	stfcamss = devm_kzalloc(dev, sizeof(*stfcamss), GFP_KERNEL);
+	if (!stfcamss)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(stfcamss->irq); ++i) {
+		stfcamss->irq[i] = platform_get_irq(pdev, i);
+		if (stfcamss->irq[i] < 0)
+			return dev_err_probe(&pdev->dev, stfcamss->irq[i],
+					     "Failed to get irq%d", i);
+	}
+
+	stfcamss->nclks = ARRAY_SIZE(stfcamss->sys_clk);
+	for (i = 0; i < stfcamss->nclks; ++i)
+		stfcamss->sys_clk[i].id = stfcamss_clocks[i];
+	ret = devm_clk_bulk_get(dev, stfcamss->nclks, stfcamss->sys_clk);
+	if (ret) {
+		dev_err(dev, "Failed to get clk controls\n");
+		return ret;
+	}
+
+	stfcamss->nrsts = ARRAY_SIZE(stfcamss->sys_rst);
+	for (i = 0; i < stfcamss->nrsts; ++i)
+		stfcamss->sys_rst[i].id = stfcamss_resets[i];
+	ret = devm_reset_control_bulk_get_shared(dev, stfcamss->nrsts,
+						 stfcamss->sys_rst);
+	if (ret) {
+		dev_err(dev, "Failed to get reset controls\n");
+		return ret;
+	}
+
+	ret = stfcamss_get_mem_res(pdev, stfcamss);
+	if (ret) {
+		dev_err(dev, "Could not map registers\n");
+		return ret;
+	}
+
+	stfcamss->dev = dev;

Move this right after allocating stfcamss, and drop the pdev argument to
stfcamss_get_mem_res(). The platform device can be retrieved in the
function using to_platform_device().

OK, I will modify.

quoted

quoted

+	platform_set_drvdata(pdev, stfcamss);
+
+	v4l2_async_nf_init(&stfcamss->notifier);
+
+	num_subdevs = stfcamss_of_parse_ports(stfcamss);
+	if (num_subdevs < 0) {
+		ret = -ENODEV;

An error message would be useful, silent errors are hard to debug.

OK, will add error printing information.

quoted

quoted

+		goto err_cleanup_notifier;
+	}
+
+	stfcamss_mc_init(pdev, stfcamss);
+
+	ret = v4l2_device_register(stfcamss->dev, &stfcamss->v4l2_dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
+		goto err_cleanup_notifier;
+	}
+
+	ret = media_device_register(&stfcamss->media_dev);
+	if (ret) {
+		dev_err(dev, "Failed to register media device: %d\n", ret);
+		goto err_unregister_device;
+	}
+
+	pm_runtime_enable(dev);

Would it be useful to enable autosuspend too, to avoid expensive
suspend/resume cycles when userspace wants to briefly stop capture and
restart it immediately ?

It seems rare to use autosuspend in the Linux camera system.

It's a relatively recent practice, and is more common in sensor drivers
than ISP drivers, but I think it's a good practice nonetheless. It makes
stop-reconfigure-start cycles much faster.

quoted

quoted

+
+	stfcamss->notifier.ops = &stfcamss_subdev_notifier_ops;
+	ret = v4l2_async_nf_register(&stfcamss->v4l2_dev, &stfcamss->notifier);
+	if (ret) {
+		dev_err(dev, "Failed to register async subdev nodes: %d\n",
+			ret);
+		goto err_unregister_media_dev;

You need to disable runtime PM in this error path.

OK, will fix it.

quoted

quoted

+	}
+
+	return 0;
+
+err_unregister_media_dev:
+	media_device_unregister(&stfcamss->media_dev);
+err_unregister_device:
+	v4l2_device_unregister(&stfcamss->v4l2_dev);
+err_cleanup_notifier:
+	v4l2_async_nf_cleanup(&stfcamss->notifier);
+	return ret;
+}

[snip]

-- 
Regards,

Laurent Pinchart