Re: [PATCH v8 3/8] media: staging: media: starfive: camss: Add core driver

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

 



Le 24/08/2023 à 10:01, Jack Zhu a écrit :
Add core driver for StarFive Camera Subsystem. The code parses
the device platform resources and registers related devices.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
Signed-off-by: Jack Zhu <jack.zhu@xxxxxxxxxxxxxxxx>
---

...

diff --git a/drivers/staging/media/starfive/camss/Kconfig b/drivers/staging/media/starfive/camss/Kconfig
new file mode 100644
index 000000000000..8d20e2bd2559
--- /dev/null
+++ b/drivers/staging/media/starfive/camss/Kconfig
@@ -0,0 +1,17 @@
+# 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
+	depends on PM
+	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.

stf_camss? (s/-/_)

diff --git a/drivers/staging/media/starfive/camss/Makefile b/drivers/staging/media/starfive/camss/Makefile
new file mode 100644
index 000000000000..f53c5cbe958f
--- /dev/null
+++ b/drivers/staging/media/starfive/camss/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for StarFive Camera Subsystem driver
+#
+
+starfive-camss-objs += \
+		stf_camss.o
+
+obj-$(CONFIG_VIDEO_STARFIVE_CAMSS) += starfive-camss.o

I'm not an expert in Makefile files, but this stf_camss.o and starfive-camss.o look strange to me.

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

...

+static int stfcamss_of_parse_ports(struct stfcamss *stfcamss)
+{
+	struct device_node *node = NULL;
+	int ret, num_subdevs = 0;
+
+	for_each_endpoint_of_node(stfcamss->dev->of_node, node) {
+		struct stfcamss_async_subdev *csd;
+
+		if (!of_device_is_available(node))
+			continue;
+
+		csd = v4l2_async_nf_add_fwnode_remote(&stfcamss->notifier,
+						      of_fwnode_handle(node),
+						      struct stfcamss_async_subdev);
+		if (IS_ERR(csd)) {
+			ret = PTR_ERR(csd);
+			dev_err(stfcamss->dev, "failed to add async notifier\n");
+			v4l2_async_nf_cleanup(&stfcamss->notifier);

having it here, looks strange to me.
It is already called in the error handling path of the probe.

Should there be a "of_node_put(node);" if we return here?

+			return ret;
+		}
+
+		ret = stfcamss_of_parse_endpoint_node(stfcamss, node, csd);
+		if (ret)
+			return ret;
+
+		num_subdevs++;
+	}
+
+	return num_subdevs;
+}

...

+static int stfcamss_remove(struct platform_device *pdev)
+{
+	struct stfcamss *stfcamss = platform_get_drvdata(pdev);
+
+	v4l2_device_unregister(&stfcamss->v4l2_dev);
+	media_device_cleanup(&stfcamss->media_dev);

Is a "v4l2_async_nf_cleanup(&stfcamss->notifier);" missing to match the error handling path of the probe?

+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+

...




[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux