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

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

Hi Bryan,

Thank you for your comments.

On 2023/7/26 18:55, Bryan O'Donoghue wrote:
> On 19/06/2023 12:28, Jack Zhu wrote:
> 
>> +static int stfcamss_of_parse_ports(struct stfcamss *stfcamss)
>> +{
>> +    struct device *dev = stfcamss->dev;
>> +    struct device_node *node = NULL;
>> +    int ret, num_subdevs = 0;
>> +
>> +    for_each_endpoint_of_node(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);
>> +            goto err_cleanup;
>> +        }
>> +
>> +        ret = stfcamss_of_parse_endpoint_node(dev, node, csd);
>> +        if (ret < 0)
>> +            goto err_cleanup;
>> +
>> +        num_subdevs++;
>> +    }
>> +
>> +    return num_subdevs;
>> +
>> +err_cleanup:
>> +    of_node_put(node);
> 
> Where is the _get() for this and if you are releasing it on the error path when is the _get() released on the non-error path ?
> 

OK, I will fix it.

>> +    return ret;
>> +}
>> +
>> +static int stfcamss_subdev_notifier_bound(struct v4l2_async_notifier *async,
>> +                      struct v4l2_subdev *subdev,
>> +                      struct v4l2_async_subdev *asd)
>> +{
>> +    struct media_pad *pad[STF_PADS_NUM];
>> +    unsigned int i, pad_num = 0;
>> +
>> +    for (i = 0; i < pad_num; ++i) {
> 
> Does this loop ever run ?
> I don't see how it can..
> 

OK, I will fix it, although there are modifications to this code in patch 5 and patch 6.

> ---
> bod

Hi Jack,

Thank you for the patch.

On Mon, Jun 19, 2023 at 07:28:35PM +0800, Jack Zhu wrote:
> 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
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 71291dc58671..62f9e3593eab 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20077,6 +20077,7 @@ L:	linux-media@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/admin-guide/media/starfive_camss.rst
>  F:	Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
> +F:	drivers/media/platform/starfive/camss
>  
>  STARFIVE DEVICETREES
>  M:	Emil Renner Berthing <kernel@esmil.dk>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index ee579916f874..627eaa0ab3ee 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -80,6 +80,7 @@ source "drivers/media/platform/renesas/Kconfig"
>  source "drivers/media/platform/rockchip/Kconfig"
>  source "drivers/media/platform/samsung/Kconfig"
>  source "drivers/media/platform/st/Kconfig"
> +source "drivers/media/platform/starfive/Kconfig"
>  source "drivers/media/platform/sunxi/Kconfig"
>  source "drivers/media/platform/ti/Kconfig"
>  source "drivers/media/platform/verisilicon/Kconfig"
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 5453bb868e67..5a038498a370 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -23,6 +23,7 @@ obj-y += renesas/
>  obj-y += rockchip/
>  obj-y += samsung/
>  obj-y += st/
> +obj-y += starfive/
>  obj-y += sunxi/
>  obj-y += ti/
>  obj-y += verisilicon/
> diff --git a/drivers/media/platform/starfive/Kconfig b/drivers/media/platform/starfive/Kconfig
> new file mode 100644
> index 000000000000..7955c2a0a4a3
> --- /dev/null
> +++ b/drivers/media/platform/starfive/Kconfig
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +comment "StarFive media platform drivers"
> +
> +source "drivers/media/platform/starfive/camss/Kconfig"
> diff --git a/drivers/media/platform/starfive/Makefile b/drivers/media/platform/starfive/Makefile
> new file mode 100644
> index 000000000000..4639fa1bca32
> --- /dev/null
> +++ b/drivers/media/platform/starfive/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-y += camss/
> 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.

> +	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.
> diff --git a/drivers/media/platform/starfive/camss/Makefile b/drivers/media/platform/starfive/camss/Makefile
> new file mode 100644
> index 000000000000..d56ddd078a71
> --- /dev/null
> +++ b/drivers/media/platform/starfive/camss/Makefile
> @@ -0,0 +1,8 @@
> +# 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
> 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 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * stf_camss.c
> + *
> + * Starfive Camera Subsystem driver
> + *
> + * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
> + */
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/videodev2.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-mc.h>
> +
> +#include "stf_camss.h"
> +
> +static const char * const stfcamss_clocks[] = {
> +	"clk_apb_func",
> +	"clk_wrapper_clk_c",
> +	"clk_dvp_inv",
> +	"clk_axiwr",
> +	"clk_mipi_rx0_pxl",
> +	"clk_ispcore_2x",
> +	"clk_isp_axi",
> +};
> +
> +static const char * const stfcamss_resets[] = {
> +	"rst_wrapper_p",
> +	"rst_wrapper_c",
> +	"rst_axird",
> +	"rst_axiwr",
> +	"rst_isp_top_n",
> +	"rst_isp_top_axi",
> +};
> +
> +static int stfcamss_get_mem_res(struct platform_device *pdev,
> +				struct stfcamss *stfcamss)
> +{
> +	stfcamss->syscon_base =
> +		devm_platform_ioremap_resource_byname(pdev, "syscon");
> +	if (IS_ERR(stfcamss->syscon_base))
> +		return PTR_ERR(stfcamss->syscon_base);
> +
> +	stfcamss->isp_base =
> +		devm_platform_ioremap_resource_byname(pdev, "isp");

This hols on a single line:

	stfcamss->isp_base = devm_platform_ioremap_resource_byname(pdev, "isp");

I would do the same above too.

> +	if (IS_ERR(stfcamss->isp_base))
> +		return PTR_ERR(stfcamss->isp_base);
> +
> +	return 0;
> +}
> +
> +/*
> + * stfcamss_of_parse_endpoint_node - Parse port endpoint node
> + * @dev: Device
> + * @node: Device node to be parsed
> + * @csd: Parsed data from port endpoint node
> + *
> + * Return 0 on success or a negative error code on failure
> + */
> +static int stfcamss_of_parse_endpoint_node(struct device *dev,

For consistency, pass the stfcamss pointer to this function, not the
struct device.

> +					   struct device_node *node,
> +					   struct stfcamss_async_subdev *csd)
> +{
> +	struct v4l2_fwnode_endpoint vep = { { 0 } };
> +
> +	v4l2_fwnode_endpoint_parse(of_fwnode_handle(node), &vep);
> +	dev_dbg(dev, "vep.base.port = %d, id = %d\n",
> +		vep.base.port, vep.base.id);

There are debug messages in v4l2_fwnode_endpoint_parse() already, you
can drop this one.

> +
> +	csd->port = vep.base.port;
> +
> +	return 0;
> +}
> +
> +/*
> + * stfcamss_of_parse_ports - Parse ports node
> + * @stfcamss: STFCAMSS device
> + *
> + * Return number of "port" nodes found in "ports" node
> + */
> +static int stfcamss_of_parse_ports(struct stfcamss *stfcamss)
> +{
> +	struct device *dev = stfcamss->dev;
> +	struct device_node *node = NULL;
> +	int ret, num_subdevs = 0;
> +
> +	for_each_endpoint_of_node(dev->of_node, node) {

	for_each_endpoint_of_node(stfcamss->dev->of_node, node) {

and drop the local dev variable.

> +		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);
> +			goto err_cleanup;
> +		}
> +
> +		ret = stfcamss_of_parse_endpoint_node(dev, node, csd);
> +		if (ret < 0)
> +			goto err_cleanup;
> +
> +		num_subdevs++;
> +	}
> +
> +	return num_subdevs;
> +
> +err_cleanup:
> +	of_node_put(node);
> +	return ret;
> +}
> +
> +static int stfcamss_subdev_notifier_bound(struct v4l2_async_notifier *async,
> +					  struct v4l2_subdev *subdev,
> +					  struct v4l2_async_subdev *asd)
> +{
> +	struct media_pad *pad[STF_PADS_NUM];
> +	unsigned int i, pad_num = 0;
> +
> +	for (i = 0; i < pad_num; ++i) {

The loop never runs as pad_num is 0. This is modified in subsequent
patches in the series, but it makes it hard to review the patches
individually. Is there any way you could restructure the series to avoid
this ?

> +		int ret;
> +
> +		ret = v4l2_create_fwnode_links_to_pad(subdev, pad[i], 0);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int stfcamss_subdev_notifier_complete(struct v4l2_async_notifier *ntf)
> +{
> +	struct stfcamss *stfcamss =
> +		container_of(ntf, struct stfcamss, notifier);
> +
> +	return v4l2_device_register_subdev_nodes(&stfcamss->v4l2_dev);
> +}
> +
> +static const struct v4l2_async_notifier_operations
> +stfcamss_subdev_notifier_ops = {
> +	.bound = stfcamss_subdev_notifier_bound,
> +	.complete = stfcamss_subdev_notifier_complete,
> +};
> +
> +static const struct media_device_ops stfcamss_media_ops = {
> +	.link_notify = v4l2_pipeline_link_notify,

You can drop this, link management notification only serves to call the
subdev .s_power() operations, which are a deprecated API.

> +};
> +
> +static void stfcamss_mc_init(struct platform_device *pdev,
> +			     struct stfcamss *stfcamss)
> +{
> +	stfcamss->media_dev.dev = stfcamss->dev;
> +	strscpy(stfcamss->media_dev.model, "Starfive Camera Subsystem",
> +		sizeof(stfcamss->media_dev.model));
> +	stfcamss->media_dev.ops = &stfcamss_media_ops;
> +	media_device_init(&stfcamss->media_dev);
> +
> +	stfcamss->v4l2_dev.mdev = &stfcamss->media_dev;
> +}
> +
> +/*
> + * 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().

> +	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.

> +		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 ?

> +
> +	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.

> +	}
> +
> +	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;
> +}
> +
> +/*
> + * stfcamss_remove - Remove STFCAMSS platform device
> + * @pdev: Pointer to STFCAMSS platform device
> + *
> + * Always returns 0.
> + */
> +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);
> +	pm_runtime_disable(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id stfcamss_of_match[] = {
> +	{ .compatible = "starfive,jh7110-camss" },
> +	{ /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, stfcamss_of_match);
> +
> +static int __maybe_unused stfcamss_runtime_suspend(struct device *dev)
> +{
> +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> +
> +	reset_control_assert(stfcamss->sys_rst[STF_RST_AXIWR].rstc);
> +	reset_control_assert(stfcamss->sys_rst[STF_RST_ISP_TOP_AXI].rstc);
> +	reset_control_assert(stfcamss->sys_rst[STF_RST_ISP_TOP_N].rstc);

The reset bulk API (reset_control_bulk_assert() and
reset_control_bulk_deassert() for below) would help.

> +	clk_disable_unprepare(stfcamss->sys_clk[STF_CLK_ISP_AXI].clk);
> +	clk_disable_unprepare(stfcamss->sys_clk[STF_CLK_ISPCORE_2X].clk);

Same for clocks, possibly.

> +
> +	return 0;
> +}
> +
> +static int __maybe_unused stfcamss_runtime_resume(struct device *dev)
> +{
> +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(stfcamss->sys_clk[STF_CLK_ISPCORE_2X].clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(stfcamss->sys_clk[STF_CLK_ISP_AXI].clk);
> +	if (ret)

You need to disable the STF_CLK_ISPCORE_2X clock here.

> +		return ret;
> +
> +	reset_control_deassert(stfcamss->sys_rst[STF_RST_ISP_TOP_N].rstc);
> +	reset_control_deassert(stfcamss->sys_rst[STF_RST_ISP_TOP_AXI].rstc);
> +	reset_control_deassert(stfcamss->sys_rst[STF_RST_AXIWR].rstc);

These calls can fail too.

Error handling would be easier with the reset and clk bulk APIs.

> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops stfcamss_pm_ops = {
> +	SET_RUNTIME_PM_OPS(stfcamss_runtime_suspend,
> +			   stfcamss_runtime_resume,
> +			   NULL)
> +};
> +
> +static struct platform_driver stfcamss_driver = {
> +	.probe = stfcamss_probe,
> +	.remove = stfcamss_remove,
> +	.driver = {
> +		.name = "starfive-camss",
> +		.pm = &stfcamss_pm_ops,
> +		.of_match_table = of_match_ptr(stfcamss_of_match),

The driver depends on OF, so let's drop the of_match_ptr().

> +	},
> +};
> +
> +module_platform_driver(stfcamss_driver);
> +
> +MODULE_AUTHOR("StarFive Corporation");
> +MODULE_DESCRIPTION("StarFive Camera Subsystem driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/media/platform/starfive/camss/stf_camss.h b/drivers/media/platform/starfive/camss/stf_camss.h
> new file mode 100644
> index 000000000000..15c4f34b9054
> --- /dev/null
> +++ b/drivers/media/platform/starfive/camss/stf_camss.h
> @@ -0,0 +1,146 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * stf_camss.h
> + *
> + * Starfive Camera Subsystem driver
> + *
> + * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
> + */
> +
> +#ifndef STF_CAMSS_H
> +#define STF_CAMSS_H
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/reset.h>
> +#include <media/media-device.h>
> +#include <media/media-entity.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-device.h>
> +
> +#define STF_DVP_NAME "stf_dvp"
> +#define STF_CSI_NAME "cdns_csi2rx"
> +#define STF_ISP_NAME "stf_isp"
> +#define STF_VIN_NAME "stf_vin"
> +
> +#define STF_PAD_SINK   0
> +#define STF_PAD_SRC    1
> +#define STF_PADS_NUM   2
> +
> +enum stf_port_num {
> +	STF_PORT_DVP = 0,
> +	STF_PORT_CSI2RX
> +};
> +
> +enum stf_clk {
> +	STF_CLK_APB_FUNC = 0,
> +	STF_CLK_WRAPPER_CLK_C,
> +	STF_CLK_DVP_INV,
> +	STF_CLK_AXIWR,
> +	STF_CLK_MIPI_RX0_PXL,
> +	STF_CLK_ISPCORE_2X,
> +	STF_CLK_ISP_AXI,
> +	STF_CLK_NUM
> +};
> +
> +enum stf_rst {
> +	STF_RST_WRAPPER_P = 0,
> +	STF_RST_WRAPPER_C,
> +	STF_RST_AXIRD,
> +	STF_RST_AXIWR,
> +	STF_RST_ISP_TOP_N,
> +	STF_RST_ISP_TOP_AXI,
> +	STF_RST_NUM
> +};
> +
> +enum stf_irq {
> +	STF_IRQ_VINWR = 0,
> +	STF_IRQ_ISP,
> +	STF_IRQ_ISPCSIL,
> +	STF_IRQ_NUM
> +};
> +
> +struct stfcamss {
> +	struct v4l2_device v4l2_dev;
> +	struct media_device media_dev;
> +	struct media_pipeline pipe;
> +	struct device *dev;
> +	struct v4l2_async_notifier notifier;
> +	void __iomem *syscon_base;
> +	void __iomem *isp_base;
> +	int irq[STF_IRQ_NUM];
> +	struct clk_bulk_data sys_clk[STF_CLK_NUM];
> +	int nclks;
> +	struct reset_control_bulk_data sys_rst[STF_RST_NUM];
> +	int nrsts;
> +};
> +
> +struct stfcamss_async_subdev {
> +	struct v4l2_async_subdev asd;  /* must be first */
> +	enum stf_port_num port;
> +};
> +
> +static inline u32 stf_isp_reg_read(struct stfcamss *stfcamss, u32 reg)
> +{
> +	return ioread32(stfcamss->isp_base + reg);
> +}
> +
> +static inline void stf_isp_reg_write(struct stfcamss *stfcamss,
> +				     u32 reg, u32 val)
> +{
> +	iowrite32(val, stfcamss->isp_base + reg);
> +}
> +
> +static inline void stf_isp_reg_write_delay(struct stfcamss *stfcamss,
> +					   u32 reg, u32 val, u32 delay)
> +{
> +	iowrite32(val, stfcamss->isp_base + reg);
> +	usleep_range(1000 * delay, 1000 * delay + 100);
> +}
> +
> +static inline void stf_isp_reg_set_bit(struct stfcamss *stfcamss,
> +				       u32 reg, u32 mask, u32 val)
> +{
> +	u32 value;
> +
> +	value = ioread32(stfcamss->isp_base + reg) & ~mask;
> +	val &= mask;
> +	val |= value;
> +	iowrite32(val, stfcamss->isp_base + reg);
> +}
> +
> +static inline void stf_isp_reg_set(struct stfcamss *stfcamss, u32 reg, u32 mask)
> +{
> +	iowrite32(ioread32(stfcamss->isp_base + reg) | mask,
> +		  stfcamss->isp_base + reg);
> +}
> +
> +static inline u32 stf_syscon_reg_read(struct stfcamss *stfcamss, u32 reg)
> +{
> +	return ioread32(stfcamss->syscon_base + reg);
> +}
> +
> +static inline void stf_syscon_reg_write(struct stfcamss *stfcamss,
> +					u32 reg, u32 val)
> +{
> +	iowrite32(val, stfcamss->syscon_base + reg);
> +}
> +
> +static inline void stf_syscon_reg_set_bit(struct stfcamss *stfcamss,
> +					  u32 reg, u32 bit_mask)
> +{
> +	u32 value;
> +
> +	value = ioread32(stfcamss->syscon_base + reg);
> +	iowrite32(value | bit_mask, stfcamss->syscon_base + reg);
> +}
> +
> +static inline void stf_syscon_reg_clear_bit(struct stfcamss *stfcamss,
> +					    u32 reg, u32 bit_mask)
> +{
> +	u32 value;
> +
> +	value = ioread32(stfcamss->syscon_base + reg);
> +	iowrite32(value & ~bit_mask, stfcamss->syscon_base + reg);
> +}
> +#endif /* STF_CAMSS_H */

Hi Jack,

On Tue, Aug 01, 2023 at 11:24:22AM +0800, Jack Zhu wrote:
> On 2023/7/27 19:33, Laurent Pinchart wrote:
> > On Mon, Jun 19, 2023 at 07:28:35PM +0800, Jack Zhu wrote:
> >> 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]

> >> 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.

> >> +	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]

> >> 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]

> >> +/*
> >> + * 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.
> 
> >> +	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.
> 
> >> +		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.

> >> +
> >> +	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.
> 
> >> +	}
> >> +
> >> +	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]