mbox series

[v6,0/6] Add StarFive Camera Subsystem driver

Message ID 20230525083202.67933-1-jack.zhu@starfivetech.com
Headers show
Series Add StarFive Camera Subsystem driver | expand

Message

Jack Zhu May 25, 2023, 8:31 a.m. UTC
Hi,

This series is the v6 series that attempts to support the Camera Subsystem
found on StarFive JH7110 SoC.

Changes in v6:
- Added 'bus-type' in bindings example.
- Corrected spelling errors.
- As reviewed by Bryan, used 'nclks' and 'nrsts' variables.
- Added lccf config for ISP.

v5 link: https://lore.kernel.org/all/20230512102844.51084-1-jack.zhu@starfivetech.com/

Changes in v5:
- Rebased on v6.4-rc1.
- Added new patch.
- Modified ISP driver.

v4 link: https://lore.kernel.org/all/20230413035541.62129-1-jack.zhu@starfivetech.com/

Previous cover letter from v4:

This patch series adds support for the StarFive Camera Subsystem
found on StarFive JH7110 SoC.

The driver implements V4L2, Media controller and V4L2 subdev interfaces.
Camera sensor using V4L2 subdev interface in the kernel is supported.

The driver is tested on VisionFive V2 board with IMX219 camera sensor.
GStreamer 1.18.5 with v4l2src plugin is supported.

Changes since v3:
Patch 1:
- Modified port@0 and port@1 properties.
- Extended the port@0 example with appropriate properties.
- Added 'port@0' for 'required'
Patch 2:
- Modified spelling errors.
Patch 3:
- Merged patch 5 into the patch with an explanation for compatible in
  commit msg.
Patch 6:
- Asserted pixel_rst[i] reset in the loop after the err_disable_pixclk
  label.
- Modified Code Style for getting sys_rst and p_rst.
- Renamed clk_name to name and modified the relevant code.
Patch 9:
- Added static for stfcamss_get_mem_res function.
- Added static for isp_close function.
- Fixed implicit conversion warning for stf_vin_map_isp_pad function.
- Dropped unused variables.

  v3: https://lore.kernel.org/all/20230331121826.96973-1-jack.zhu@starfivetech.com/

Changes since v2:
- Rebased on v6.3-rc1.
Patch 1:
- Modified spelling errors.
- Added port@0.
- Modified '$ref' of port.
- Added 'ports' to 'required'.
- Dropped 'stfcamss' label in example.
- Added port@0 in example.
- Added MAINTAINERS file.
Patch 2:
- Split this patch into three new patches.
- Modified compatible property.
- Replaced clock names with the existing names.
- Modified 'bus-type' and 'clock-lanes'
- Added port@2 - port@4
- Dropped 'csi2rx' label in example.
Patch 3:
- Updated rst and dot file as three pipelines were deleted.
Patch 4:
- Split this patch into three new patches.
- Dropped .s_power() and .get_fmt().
- Dropped CSI-2 DT support.
- Dropped v4l2_device_register_subdev_nodes().
- Used assigned-clock-rates in DT to set clk value.
- Modified 'compatible' field.
Patch 5:
- Deleted three pipelines. 
- Modified 'stfcamss_clocks'/'stfcamss_resets' struct.
- Dropped stfcamss_find_sensor() function.
- Removed redundant code from stfcamss_of_parse_endpoint_node().
- Modified spelling errors.
- Rewrote stfcamss_reg_media_subdev_node() function.
- Modified stfcamss_subdev_notifier_bound().
- Modified stfcamss_probe() function.
- Dropped stfcamss_suspend() and stfcamss_resume().
- Dropped dev_info() in stfcamss_remove() function.
- Added 'stf_' prefix for enum subdev_type.
- Moved all includes to the top in stf_camss.h file.
- Dropped unused fields in stfcamss struct.
- Replaced Custom logging macros with regular macros.
- Rewrote register read and write functions.
- Used lowercase for all hex constants.
- Used macro to name registers.
- Dropped unused ioctl and stf_isp_ioctl.h file.

  v2: https://lore.kernel.org/all/20230310120553.60586-1-jack.zhu@starfivetech.com/

Changes since v1:
- Deleted starfive,jh7110-mipi-csi2.yaml.
- Converted cdns,csi2rx.txt to cdns,csi2rx.yaml and added ‘resets’
  properties.
- Added ‘cdns,csi2rx.yaml’ in ‘CADENCE MIPI-CSI2 BRIDGES’ entry.
- The following contents were modified in starfive,jh7110-camss.yaml:
  dropped quotes from ’id’ and ‘schema’; dropped ‘|’ for ‘description’;
  corrected the wrong or redundant words: ‘a ISP’, ‘PD ISP’;
  dropped ‘minItems’ for ‘reg’, ‘clocks’, ‘resets’ and ‘interrupts’;
  dropped the '_clk' and 'rst_' prefix about the 'clock-names' and
  'reset-names';
  changed ‘endpoint@1’ to ‘endpoint’; updated examples;
- Updated Subject for some patches.
- Merged patch 6, 7, 8, 9, 10, 11 into one patch.

Jack Zhu (6):
  media: dt-bindings: Add JH7110 Camera Subsystem
  media: admin-guide: Add starfive_camss.rst for Starfive Camera
    Subsystem
  media: starfive: Add basic driver
  media: starfive: Add video driver
  media: starfive: Add ISP driver
  media: starfive: Add VIN driver

 .../admin-guide/media/starfive_camss.rst      |   57 +
 .../media/starfive_camss_graph.dot            |   16 +
 .../admin-guide/media/v4l-drivers.rst         |    1 +
 .../bindings/media/starfive,jh7110-camss.yaml |  180 +++
 MAINTAINERS                                   |    9 +
 drivers/media/platform/Kconfig                |    1 +
 drivers/media/platform/Makefile               |    1 +
 drivers/media/platform/starfive/Kconfig       |   19 +
 drivers/media/platform/starfive/Makefile      |   14 +
 drivers/media/platform/starfive/stf_camss.c   |  476 +++++++
 drivers/media/platform/starfive/stf_camss.h   |  157 +++
 drivers/media/platform/starfive/stf_common.h  |   18 +
 drivers/media/platform/starfive/stf_isp.c     |  550 ++++++++
 drivers/media/platform/starfive/stf_isp.h     |  483 +++++++
 .../media/platform/starfive/stf_isp_hw_ops.c  |  468 +++++++
 drivers/media/platform/starfive/stf_video.c   |  864 +++++++++++++
 drivers/media/platform/starfive/stf_video.h   |   95 ++
 drivers/media/platform/starfive/stf_vin.c     | 1134 +++++++++++++++++
 drivers/media/platform/starfive/stf_vin.h     |  180 +++
 .../media/platform/starfive/stf_vin_hw_ops.c  |  241 ++++
 20 files changed, 4964 insertions(+)
 create mode 100644 Documentation/admin-guide/media/starfive_camss.rst
 create mode 100644 Documentation/admin-guide/media/starfive_camss_graph.dot
 create mode 100644 Documentation/devicetree/bindings/media/starfive,jh7110-camss.yaml
 create mode 100644 drivers/media/platform/starfive/Kconfig
 create mode 100644 drivers/media/platform/starfive/Makefile
 create mode 100644 drivers/media/platform/starfive/stf_camss.c
 create mode 100644 drivers/media/platform/starfive/stf_camss.h
 create mode 100644 drivers/media/platform/starfive/stf_common.h
 create mode 100644 drivers/media/platform/starfive/stf_isp.c
 create mode 100644 drivers/media/platform/starfive/stf_isp.h
 create mode 100644 drivers/media/platform/starfive/stf_isp_hw_ops.c
 create mode 100644 drivers/media/platform/starfive/stf_video.c
 create mode 100644 drivers/media/platform/starfive/stf_video.h
 create mode 100644 drivers/media/platform/starfive/stf_vin.c
 create mode 100644 drivers/media/platform/starfive/stf_vin.h
 create mode 100644 drivers/media/platform/starfive/stf_vin_hw_ops.c

Comments

Laurent Pinchart June 1, 2023, 10:44 a.m. UTC | #1
Hi Jack,

Thank you for the patch.

On Thu, May 25, 2023 at 04:31:59PM +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      |  19 +
>  drivers/media/platform/starfive/Makefile     |   9 +
>  drivers/media/platform/starfive/stf_camss.c  | 372 +++++++++++++++++++
>  drivers/media/platform/starfive/stf_camss.h  | 153 ++++++++
>  drivers/media/platform/starfive/stf_common.h |  18 +
>  8 files changed, 574 insertions(+)
>  create mode 100644 drivers/media/platform/starfive/Kconfig
>  create mode 100644 drivers/media/platform/starfive/Makefile
>  create mode 100644 drivers/media/platform/starfive/stf_camss.c
>  create mode 100644 drivers/media/platform/starfive/stf_camss.h
>  create mode 100644 drivers/media/platform/starfive/stf_common.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 71291dc58671..33a752569942 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/
>  
>  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..1a2293475bd2
> --- /dev/null
> +++ b/drivers/media/platform/starfive/Kconfig
> @@ -0,0 +1,19 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +comment "Starfive media platform drivers"
> +
> +config VIDEO_STARFIVE_CAMSS
> +	tristate "Starfive Camera Subsystem driver"
> +	depends on V4L_PLATFORM_DRIVERS
> +	depends on VIDEO_DEV && OF
> +	depends on DMA_CMA

I think you can drop this, the driver doesn't depend on DMA. It uses the
DMA mapping API though, so you should depend on HAS_DMA instead.

> +	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/Makefile b/drivers/media/platform/starfive/Makefile
> new file mode 100644
> index 000000000000..796775fa52f4
> --- /dev/null
> +++ b/drivers/media/platform/starfive/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 \
> diff --git a/drivers/media/platform/starfive/stf_camss.c b/drivers/media/platform/starfive/stf_camss.c
> new file mode 100644
> index 000000000000..cb2b62d49416
> --- /dev/null
> +++ b/drivers/media/platform/starfive/stf_camss.c
> @@ -0,0 +1,372 @@
> +// 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/media-device.h>
> +#include <media/v4l2-async.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");
> +	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,
> +					   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 = 0x%x, id = 0x%x\n",
> +		vep.base.port, vep.base.id);

You can print the port and id as decimal numbers, they're indices.

> +
> +	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;
> +	struct device_node *remote = 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;
> +
> +		remote = of_graph_get_remote_port_parent(node);
> +		if (!remote) {
> +			dev_err(dev, "Cannot get remote parent\n");
> +			ret = -EINVAL;
> +			goto err_cleanup;
> +		}
> +
> +		csd = v4l2_async_nf_add_fwnode(&stfcamss->notifier,
> +					       of_fwnode_handle(remote),
> +					       struct stfcamss_async_subdev);
> +		of_node_put(remote);

You can use v4l2_async_nf_add_fwnode_remote() to avoid the need to call
of_graph_get_remote_port_parent() yourself.

> +		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 stfcamss *stfcamss =
> +		container_of(async, struct stfcamss, notifier);
> +	struct host_data *host_data = &stfcamss->host_data;
> +	struct media_entity *source;
> +	int i, j;

i and j are never negative, you can make them unsigned int.

> +
> +	source = &subdev->entity;

You can assign the variable when declaring it:

	struct media_entity *source = &subdev->entity;

> +
> +	for (i = 0; i < source->num_pads; i++) {
> +		if (source->pads[i].flags & MEDIA_PAD_FL_SOURCE)
> +			break;
> +	}
> +
> +	if (i == source->num_pads) {
> +		dev_err(stfcamss->dev, "No source pad in external entity\n");
> +		return -EINVAL;
> +	}
> +
> +	for (j = 0; host_data->host_entity[j] && (j < HOST_ENTITY_MAX); j++) {
> +		struct media_entity *input;
> +		int ret;
> +
> +		input = host_data->host_entity[j];
> +
> +		ret = media_create_pad_link(
> +			source,
> +			i,
> +			input,
> +			STF_PAD_SINK,
> +			source->function == MEDIA_ENT_F_CAM_SENSOR ?
> +			MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED :
> +			0);
> +		if (ret < 0) {
> +			dev_err(stfcamss->dev,
> +				"Failed to link %s->%s entities: %d\n",
> +				source->name, input->name, ret);
> +			return ret;
> +		}
> +	}

I think you can simplify this. First of all, the port to which the
newly bound subdev is attached is available in the stfcamss_async_subdev
structure. You can cast the asd to a stfcamss_async_subdev with
container_of(), access the port number from there, and get the
corresponding sink subdev and pad (for the DVP or CSI-2 port). Then, I
think you can use v4l2_create_fwnode_links_to_pad(), which will search
for the source pad in the source subdev, instead of doing it yourself.

I believe the stfcamss.host_data field can be dropped, as it's only used
in this function.

> +
> +	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,
> +};
> +
> +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));
> +	snprintf(stfcamss->media_dev.bus_info,
> +		 sizeof(stfcamss->media_dev.bus_info),
> +		 "%s:%s", dev_bus_name(&pdev->dev), pdev->name);

No need to set bus_info manually, the framework does if for you in
media_device_init().

> +	stfcamss->media_dev.hw_revision = 0x01;

Would the hardware have a version or revision register that you could
use to fill this, by any chance ?

> +	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 = 0, i, num_subdevs;

No need to initialize ret to 0.

i is never negative, make it an unsigned int.

> +
> +	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 clock%d", i);

I think you meant "irq %u", not clock.

> +	}
> +
> +	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;
> +	platform_set_drvdata(pdev, stfcamss);
> +
> +	v4l2_async_nf_init(&stfcamss->notifier);
> +
> +	num_subdevs = stfcamss_of_parse_ports(stfcamss);
> +	if (num_subdevs < 0) {
> +		dev_err(dev, "Failed to find subdevices\n");
> +		return -ENODEV;

I would

		ret = -ENODEV;
		goto err_cleanup_notifier;

here.

> +	}
> +
> +	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;
> +	}
> +
> +	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;
> +	}
> +
> +	pm_runtime_enable(dev);

You should enable runtime PM before registering the notifier, otherwise
there's a race condition where userspace could start using the device
before runtime PM gets enabled.

> +
> +	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_ISP_TOP_AXI].rstc);
> +	reset_control_assert(stfcamss->sys_rst[STF_RST_ISP_TOP_N].rstc);
> +	clk_disable_unprepare(stfcamss->sys_clk[STF_CLK_ISP_AXI].clk);
> +	clk_disable_unprepare(stfcamss->sys_clk[STF_CLK_ISPCORE_2X].clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused stfcamss_runtime_resume(struct device *dev)
> +{
> +	struct stfcamss *stfcamss = dev_get_drvdata(dev);
> +
> +	clk_prepare_enable(stfcamss->sys_clk[STF_CLK_ISPCORE_2X].clk);
> +	clk_prepare_enable(stfcamss->sys_clk[STF_CLK_ISP_AXI].clk);

Error handling is missing for those two calls.

> +	reset_control_deassert(stfcamss->sys_rst[STF_RST_ISP_TOP_N].rstc);
> +	reset_control_deassert(stfcamss->sys_rst[STF_RST_ISP_TOP_AXI].rstc);
> +
> +	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 = DRV_NAME,
> +		.pm = &stfcamss_pm_ops,
> +		.of_match_table = of_match_ptr(stfcamss_of_match),
> +	},
> +};
> +
> +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/stf_camss.h b/drivers/media/platform/starfive/stf_camss.h
> new file mode 100644
> index 000000000000..8f39cd65950c
> --- /dev/null
> +++ b/drivers/media/platform/starfive/stf_camss.h
> @@ -0,0 +1,153 @@
> +/* 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/v4l2-device.h>

You should include media/media-device.h, media/media-entity.h and
media/v4l2-async.h.

> +	struct device *dev;

> +};

> +
> +#include "stf_common.h"
> +
> +#define DRV_NAME     "starfive-camss"

DRV_NAME is used in stfcamss_driver.driver.name only. I would drop the
macro and write

		.name = "starfive-camss",

above, it will be more readable.

> +#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 port_num {
> +	PORT_NUMBER_DVP_SENSOR = 0,
> +	PORT_NUMBER_CSI2RX

Please name this stf_port_num and STF_PORT_*, to avoid namespace
clashes. I would drop "NUMBER_", and name the two ports STF_PORT_DVP and
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
> +};
> +
> +#define HOST_ENTITY_MAX		2
> +
> +struct host_data {

Same here, please name this stf_host_data.

> +	struct media_entity *host_entity[HOST_ENTITY_MAX];
> +};
> +
> +struct stfcamss {
> +	struct v4l2_device v4l2_dev;
> +	struct media_device media_dev;
> +	struct media_pipeline pipe;
> +	struct device *dev;
> +	struct v4l2_async_notifier notifier;
> +	struct host_data host_data;
> +	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 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 */
> diff --git a/drivers/media/platform/starfive/stf_common.h b/drivers/media/platform/starfive/stf_common.h
> new file mode 100644
> index 000000000000..e04fca2d9cd4
> --- /dev/null
> +++ b/drivers/media/platform/starfive/stf_common.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * stf_common.h
> + *
> + * StarFive Camera Subsystem - Common definitions
> + *
> + * Copyright (C) 2021-2023 StarFive Technology Co., Ltd.
> + */
> +
> +#ifndef STF_COMMON_H
> +#define STF_COMMON_H
> +
> +enum stf_subdev_type {
> +	STF_SUBDEV_TYPE_VIN,
> +	STF_SUBDEV_TYPE_ISP,
> +};

Looking at the whole series, these two values are set in the struct vin_line
and stf_isp_dev sdev_type fields, but they are never read. You can drop
those two fields, drop this enum, and drop this header.

> +
> +#endif /* STF_COMMON_H */
>