Le 25/08/2023 à 12:44, Jack Zhu a écrit :
Hi Christophe,
Thank you for your comment!
On 2023/8/25 2:31, Christophe JAILLET wrote:
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 msstf-cas.
stf_camss? (s/-/_)
Refer to the writing method of other media drivers, most of them use hyphen.
Forget about my comment. I have been puzzled by "msstf-cas" here, vs
"stf_camss" below.
it may be better to use ‘starfive-camss'？
Yes, I think so.
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.
Is it better to replace 'stf_camss.o' with 'stf-camss.o', which is consistent
with the driving style of other media drivers?
No strong opinion on it.
Consistency is always good.
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?
We do not call a 'get' interface, is it necessary to use the 'put' interface?
for_each_endpoint_of_node() does.
See [1] for doc, and [2] for an example.
[1]:
https://elixir.bootlin.com/linux/v6.5-rc7/source/include/linux/of_graph.h#L30
[2]:
https://elixir.bootlin.com/linux/v6.5-rc7/source/drivers/gpu/drm/bridge/tc358767.c#L2196
Also, at least because of the recent b8ec754ae4c5 in -next, your patch
does not compile as-is on -next.
CJ
+ 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;
+}
+
...
