Message-ID: <f5778b751ee5044d5e3448a77032ff020d63994b.camel@ndufresne.ca>
Date: Thu, 24 Apr 2025 13:36:51 -0400
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: Nas Chung <nas.chung@...psnmedia.com>, mchehab@...nel.org, 
	hverkuil@...all.nl, sebastian.fricke@...labora.com, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-imx@....com, marex@...x.de, 
	jackson.lee@...psnmedia.com, lafley.kim@...psnmedia.com, Ming Qian
	 <ming.qian@....nxp.com>
Subject: Re: [PATCH v2 3/8] media: chips-media: wave6: Add Wave6 driver

Le mardi 22 avril 2025 à 18:31 +0900, Nas Chung a écrit :
> This adds the main driver for the Chips&Media Wave6 video codec IP.
> 
> On NXP i.MX platforms, the Wave6 consists of two functional regions:
> a control region responsible for firmware and shared resources,
> and a core region for encoding and decoding.
> 
> This driver binds the `wave6-ctrl` and `wave6-core` sub-devices,
> and coordinates their initialization and teardown.
> 
> Signed-off-by: Nas Chung <nas.chung@...psnmedia.com>
> Tested-by: Ming Qian <ming.qian@....nxp.com>
> ---
>  MAINTAINERS                                   |   1 +
>  drivers/media/platform/chips-media/Kconfig    |   1 +
>  drivers/media/platform/chips-media/Makefile   |   1 +
>  .../media/platform/chips-media/wave6/Kconfig  |  24 +
>  .../media/platform/chips-media/wave6/Makefile |   4 +
>  .../platform/chips-media/wave6/wave6-vpu.c    | 469 ++++++++++++++++++
>  .../platform/chips-media/wave6/wave6-vpu.h    |  85 ++++
>  7 files changed, 585 insertions(+)
>  create mode 100644 drivers/media/platform/chips-media/wave6/Kconfig
>  create mode 100644 drivers/media/platform/chips-media/wave6/Makefile
>  create mode 100644 drivers/media/platform/chips-media/wave6/wave6-vpu.c
>  create mode 100644 drivers/media/platform/chips-media/wave6/wave6-vpu.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6ca159e532e7..4fc54c824f65 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25529,6 +25529,7 @@ M:	Jackson Lee <jackson.lee@...psnmedia.com>
>  L:	linux-media@...r.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/media/cnm,wave633c.yaml
> +F:	drivers/media/platform/chips-media/wave6/
>  
>  WHISKEYCOVE PMIC GPIO DRIVER
>  M:	Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
> diff --git a/drivers/media/platform/chips-media/Kconfig b/drivers/media/platform/chips-media/Kconfig
> index ad350eb6b1fc..8ef7fc8029a4 100644
> --- a/drivers/media/platform/chips-media/Kconfig
> +++ b/drivers/media/platform/chips-media/Kconfig
> @@ -4,3 +4,4 @@ comment "Chips&Media media platform drivers"
>  
>  source "drivers/media/platform/chips-media/coda/Kconfig"
>  source "drivers/media/platform/chips-media/wave5/Kconfig"
> +source "drivers/media/platform/chips-media/wave6/Kconfig"
> diff --git a/drivers/media/platform/chips-media/Makefile b/drivers/media/platform/chips-media/Makefile
> index 6b5d99de8b54..b9a07a91c9d6 100644
> --- a/drivers/media/platform/chips-media/Makefile
> +++ b/drivers/media/platform/chips-media/Makefile
> @@ -2,3 +2,4 @@
>  
>  obj-y += coda/
>  obj-y += wave5/
> +obj-y += wave6/
> diff --git a/drivers/media/platform/chips-media/wave6/Kconfig b/drivers/media/platform/chips-media/wave6/Kconfig
> new file mode 100644
> index 000000000000..3d7369ca690c
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/Kconfig
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +config VIDEO_WAVE6_VPU
> +	tristate "Chips&Media Wave6 Codec Driver"
> +	depends on V4L_MEM2MEM_DRIVERS
> +	depends on VIDEO_DEV && OF
> +	depends on ARCH_MXC || COMPILE_TEST
> +	select VIDEOBUF2_DMA_CONTIG
> +	select V4L2_MEM2MEM_DEV
> +	select GENERIC_ALLOCATOR
> +	help
> +	  Chips&Media Wave6 stateful codec driver.
> +	  The codec driver provides encoding and decoding capabilities
> +	  for H.264, HEVC, and other video formats.
> +	  To compile this driver as modules, choose M here: the
> +	  modules will be called wave6.
> +
> +config VIDEO_WAVE6_VPU_SUPPORT_FOLLOWER
> +	bool "Support Wave6 VPU follower"
> +	depends on VIDEO_WAVE6_VPU
> +	depends on ARCH_MXC || COMPILE_TEST
> +	default n
> +	help
> +	  Indicates whether the VPU domain power always on.
                                               >is< ?

This configuration is pretty vague to me. Do we really need that ?
Isn't there other ways to disable PM runtime ? If unsure, just remove
that, and we can discuss separately.

> diff --git a/drivers/media/platform/chips-media/wave6/Makefile b/drivers/media/platform/chips-media/wave6/Makefile
> new file mode 100644
> index 000000000000..255fc90bc642
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +wave6-objs += wave6-vpu.o
> +obj-$(CONFIG_VIDEO_WAVE6_VPU) += wave6.o
> diff --git a/drivers/media/platform/chips-media/wave6/wave6-vpu.c b/drivers/media/platform/chips-media/wave6/wave6-vpu.c
> new file mode 100644
> index 000000000000..5d0c093a9cc5
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-vpu.c
> @@ -0,0 +1,469 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)

nit: Its first time I notice, wave5 is like this too, but what the
purpose of BDS-3-Clause here ? This driver can't possibly be used
outside of Linux, and when loaded inside Linux, GPL is the only valid
choice as far as I know.

> +/*
> + * Wave6 series multi-standard codec IP - wave6 driver
> + *
> + * Copyright (C) 2025 CHIPS&MEDIA INC
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/pm_runtime.h>
> +#include "wave6-vpu.h"
> +
> +#define VPU_PLATFORM_DEVICE_NAME "wave6-vpu"
> +#define VPU_CLK_NAME "vpu"
> +
> +#define WAVE6_VPU_FLAG_SLEEP	BIT(0)
> +#define WAVE6_VPU_FLAG_WAKEUP	BIT(1)

Mind aligning these ?

> +
> +/**
> + * wave6_alloc_dma - Allocate DMA memory
> + * @dev: device pointer
> + * @vb: VPU buffer structure
> + *
> + * Allocates a contiguous DMA memory region for VPU usage.
> + * The allocated memory information is stored in the given
> + * @vb structure.
> + *
> + * Return: 0 on success, -EINVAL for invalid arguments, -ENOMEM on failure
> + */
> +int wave6_alloc_dma(struct device *dev, struct vpu_buf *vb)
> +{
> +	void *vaddr;
> +	dma_addr_t daddr;
> +
> +	if (!vb || !vb->size)
> +		return -EINVAL;
> +
> +	vaddr = dma_alloc_coherent(dev, vb->size, &daddr, GFP_KERNEL);
> +	if (!vaddr)
> +		return -ENOMEM;
> +
> +	vb->vaddr = vaddr;
> +	vb->daddr = daddr;
> +	vb->dev = dev;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(wave6_alloc_dma);

Also to emphasis the clash with dual license.

> +
> +/**
> + * wave6_free_dma - Free DMA memory
> + * @vb: VPU buffer structure
> + *
> + * Frees the DMA memory previously allocated by wave6_alloc_dma().
> + * @vb structure is also cleared to zero.
> + */
> +void wave6_free_dma(struct vpu_buf *vb)
> +{
> +	if (!vb || !vb->size || !vb->vaddr)
> +		return;
> +
> +	dma_free_coherent(vb->dev, vb->size, vb->vaddr, vb->daddr);
> +	memset(vb, 0, sizeof(*vb));
> +}
> +EXPORT_SYMBOL_GPL(wave6_free_dma);
> +
> +static int wave6_check_entity(struct wave6_vpu_device *vpu,
> +			      struct wave6_vpu_entity *entity)

When I read code below, I don't see what wave6_check_entity() does. You
should rename this, perhaps it means "wave6_valid_entity()" ?

Its also not obvious to me in which normal condition you will hold a
ref to an entity that is no longer valid. I'd ask here, can this fail
without a programming error ? And in which case, if its a programming
error, a WARN_ON would likely be a good idea.

> +{
> +	if (!entity || !entity->vpu || !vpu || entity->vpu != vpu->dev)
> +		return -EINVAL;
> +	if (entity->index < 0 || entity->index >= WAVE6_VPU_MAXIMUM_ENTITY_CNT)
> +		return -EINVAL;
> +	if (entity != vpu->entities[entity->index])
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static unsigned long wave6_vpu_get_clk_rate(struct wave6_vpu_device *vpu)
> +{
> +	unsigned long rate = 0;
> +	int i;
> +
> +	mutex_lock(&vpu->lock);
> +
> +	for (i = 0; i < vpu->num_clks; i++) {
> +		if (vpu->clks[i].id && !strcmp(vpu->clks[i].id, VPU_CLK_NAME))
> +			rate = clk_get_rate(vpu->clks[i].clk);
> +	}
> +
> +	mutex_unlock(&vpu->lock);
> +	return rate;
> +}
> +
> +static int __wave6_vpu_get(struct wave6_vpu_device *vpu,
> +			   struct wave6_vpu_entity *entity)
> +{
> +	int ret;

Would be nice to add:

	lockdep_assert_held(&vpu->lock);

> +
> +	if (atomic_inc_return(&vpu->ref_count) > 1)
> +		return 0;
> +
> +	ret = pm_runtime_resume_and_get(vpu->dev);
> +	if (ret) {
> +		dev_err(vpu->dev, "pm runtime resume fail, ret = %d\n", ret);
> +		atomic_dec(&vpu->ref_count);
> +		return -EINVAL;
> +	}
> +
> +	if (vpu->ctrl && vpu->ctrl_ops) {
> +		ret = vpu->ctrl_ops->get_ctrl(vpu->ctrl, entity);
> +		if (ret) {
> +			dev_err(vpu->dev, "get ctrl fail, ret = %d\n", ret);
> +			pm_runtime_put_sync(vpu->dev);
> +			atomic_dec(&vpu->ref_count);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int wave6_vpu_get(struct wave6_vpu_device *vpu,
> +			 struct wave6_vpu_entity *entity)
> +{
> +	int ret = 0;

Drop.

> +
> +	mutex_lock(&vpu->lock);

Replace with:

	guard(mutex)(&vpu->lock);

> +
> +	if (wave6_check_entity(vpu, entity)) {
> +		ret = -EINVAL;
> +		goto unlock;

Then these two lines becomes:

		return -EINVAL;

You won't even need a scope.

> +	}
> +
> +	if (!entity->active)
> +		goto unlock;
		return 0;

> +
> +	ret = __wave6_vpu_get(vpu, entity);
> +
> +unlock:
> +	mutex_unlock(&vpu->lock);

Drop the two above lines;

> +	return ret;
	return 0;

> +}
> +
> +static void __wave6_vpu_put(struct wave6_vpu_device *vpu,
> +			    struct wave6_vpu_entity *entity)
> +{
> +	if (atomic_dec_return(&vpu->ref_count) > 0)
> +		return;
> +
> +	if (vpu->ctrl && vpu->ctrl_ops)
> +		vpu->ctrl_ops->put_ctrl(vpu->ctrl, entity);
> +
> +	pm_runtime_put_sync(vpu->dev);
> +}
> +
> +static void wave6_vpu_put(struct wave6_vpu_device *vpu,
> +			  struct wave6_vpu_entity *entity)
> +{
> +	mutex_lock(&vpu->lock);

Same, you should use guard()().

> +
> +	if (wave6_check_entity(vpu, entity))
> +		goto unlock;
> +
> +	if (!entity->active)
> +		goto unlock;
> +
> +	__wave6_vpu_put(vpu, entity);
> +
> +unlock:
> +	mutex_unlock(&vpu->lock);
> +}
> +
> +static void wave6_support_follower(struct wave6_vpu_device *vpu,
> +				   struct wave6_vpu_entity *entity, u32 flag)
> +{

I haven't figure-out what this is about, bare in mind.

> +	struct wave6_vpu_entity *target = NULL;
> +	int ret;
> +	int i;

Seems like this needs to be called with lock held:

	lockdep_assert_held(&vpu->lock);

> +
> +	if (!vpu->support_follower)
> +		return;
> +	if (!vpu->ctrl)
> +		return;
> +
> +	if (entity)
> +		target = entity;
> +
> +	ret = pm_runtime_resume_and_get(vpu->dev);
> +	if (ret) {
> +		dev_warn(vpu->dev, "pm runtime resume fail, ret = %d\n", ret);
> +		return;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(vpu->entities); i++) {
> +		if (!vpu->entities[i])
> +			continue;
> +		if (target && vpu->entities[i] != target)
> +			continue;
> +		if (flag & WAVE6_VPU_FLAG_WAKEUP)
> +			__wave6_vpu_get(vpu, vpu->entities[i]);
> +		if (flag & WAVE6_VPU_FLAG_SLEEP)
> +			__wave6_vpu_put(vpu, vpu->entities[i]);
> +	}
> +
> +	pm_runtime_put_sync(vpu->dev);
> +}
> +
> +static int wave6_find_unused_index(struct wave6_vpu_device *vpu)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(vpu->entities); i++) {
> +		if (!vpu->entities[i])
> +			return i;
> +	}
> +
> +	return -1;
> +}
> +
> +static int wave6_register_vpu_core(struct wave6_vpu_device *vpu,
> +				   struct wave6_vpu_entity *entity)
> +{
> +	int ret = 0;
> +	int index;
> +
> +	mutex_lock(&vpu->lock);

Also:
	guard(mutex)(&vpu->lock);

> +
> +	if (!entity || !entity->dev) {
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	index = wave6_find_unused_index(vpu);
> +	if (index < 0 || index >= ARRAY_SIZE(vpu->entities)) {

Drop the second condition, its defensive coding, you can trust your
wave6_find_unused_index() helper to return a valid index or -1.

> +		ret = -1;
> +		goto unlock;
> +	}
> +
> +	entity->vpu = vpu->dev;
> +	entity->index = index;
> +	vpu->entities[index] = entity;
> +	wave6_support_follower(vpu, entity, WAVE6_VPU_FLAG_WAKEUP);

So this support_follower() actually does wave6_vpu_get()/put(), except
when the build config forces always on. I think if you drop that
config, you can drop that strange function and just use get/put.

I don't have the full portait of when vpu core are registered and when
not. It does give me the strange impression that once a stream is
active, it cannot sleep anymore. I'd like to see some text about the PM
runtime strategies.

> +
> +unlock:
> +	mutex_unlock(&vpu->lock);
> +	return ret;
> +}
> +
> +static void wave6_unregister_vpu_core(struct wave6_vpu_device *vpu,
> +				      struct wave6_vpu_entity *entity)
> +{
> +	mutex_lock(&vpu->lock);
Also:
	guard(mutex)(&vpu->lock);

> +
> +	if (wave6_check_entity(vpu, entity))
> +		goto unlock;
> +
> +	wave6_support_follower(vpu, entity, WAVE6_VPU_FLAG_SLEEP);
> +	vpu->entities[entity->index] = NULL;
> +	entity->vpu = NULL;
> +	entity->index = -1;
> +
> +unlock:
> +	mutex_unlock(&vpu->lock);
> +}
> +
> +static int wave6_register_vpu_ctrl(struct wave6_vpu_device *vpu,
> +				   struct device *ctrl,
> +				   const struct wave6_vpu_ctrl_ops *ops)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&vpu->lock);

Also:
	guard(mutex)(&vpu->lock);

> +
> +	if (!ctrl || !ops) {

Seems like some WARN_ON would be preferred, you don't expect this to
happen outside of programmer error right ?

> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	if (vpu->ctrl) {
> +		if (vpu->ctrl != ctrl)
> +			ret = -EINVAL;
> +
> +		goto unlock;
> +	}
> +
> +	vpu->ctrl = ctrl;
> +	vpu->ctrl_ops = ops;
> +	wave6_support_follower(vpu, NULL, WAVE6_VPU_FLAG_WAKEUP);
> +
> +unlock:
> +	mutex_unlock(&vpu->lock);
> +	return ret;
> +}
> +
> +static void wave6_unregister_vpu_ctrl(struct wave6_vpu_device *vpu,
> +				      struct device *ctrl)
> +{
> +	mutex_lock(&vpu->lock);
> +
> +	if (vpu->ctrl != ctrl)
> +		goto unlock;
> +
> +	wave6_support_follower(vpu, NULL, WAVE6_VPU_FLAG_SLEEP);
> +	vpu->ctrl = NULL;
> +
> +unlock:
> +	mutex_unlock(&vpu->lock);
> +}
> +
> +static void wave6_require_work_buffer(struct wave6_vpu_device *vpu,
> +				      struct wave6_vpu_entity *entity)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&vpu->lock);

Also:
	guard(mutex)(&vpu->lock);

> +
> +	if (wave6_check_entity(vpu, entity))
> +		goto unlock;
> +
> +	if (!vpu->ctrl || !vpu->ctrl_ops)
> +		goto unlock;
> +
> +	ret = vpu->ctrl_ops->require_work_buffer(vpu->ctrl, entity);
> +	if (ret)
> +		dev_warn(vpu->dev, "require_work_buffer fail %d\n", ret);
> +
> +unlock:
> +	mutex_unlock(&vpu->lock);
> +}
> +
> +static const struct wave6_vpu_ops wave6_vpu_ops = {
> +	.get_vpu = wave6_vpu_get,
> +	.put_vpu = wave6_vpu_put,
> +	.reg_core = wave6_register_vpu_core,
> +	.unreg_core = wave6_unregister_vpu_core,
> +	.reg_ctrl = wave6_register_vpu_ctrl,
> +	.unreg_ctrl = wave6_unregister_vpu_ctrl,
> +	.req_work_buffer = wave6_require_work_buffer,
> +	.get_clk_rate = wave6_vpu_get_clk_rate,
> +};
> +
> +static int wave6_vpu_probe(struct platform_device *pdev)
> +{
> +	struct wave6_vpu_device *vpu;
> +	int ret;
> +
> +	vpu = devm_kzalloc(&pdev->dev, sizeof(*vpu), GFP_KERNEL);
> +	if (!vpu)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&pdev->dev, vpu);
> +	vpu->dev = &pdev->dev;
> +	vpu->ops = &wave6_vpu_ops;
> +
> +	mutex_init(&vpu->lock);
> +	atomic_set(&vpu->ref_count, 0);
> +
> +	ret = devm_clk_bulk_get_all(&pdev->dev, &vpu->clks);
> +	if (ret < 0) {
> +		dev_warn(&pdev->dev, "unable to get clocks: %d\n", ret);
> +		ret = 0;
> +	}
> +	vpu->num_clks = ret;
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +#if IS_ENABLED(CONFIG_VIDEO_WAVE6_VPU_SUPPORT_FOLLOWER)
> +	vpu->support_follower = true;
> +#endif
> +	if (vpu->support_follower) {

This scope seems unreachable if CONFIG_VIDEO_WAVE6_VPU_SUPPORT_FOLLOWER
is not set, move it inside the ifdef.

> +		ret = pm_runtime_resume_and_get(&pdev->dev);
> +		if (ret) {
> +			dev_warn(&pdev->dev, "pm resume fail %d\n", ret);
> +			vpu->support_follower = false;

If you couldn't wake the HW now, its unlikely to wake later. Better
cleanup and fail the probe ?

> +		}
> +	}
> +
> +	of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> +
> +	return 0;
> +}
> +
> +static void wave6_vpu_remove(struct platform_device *pdev)
> +{
> +	struct wave6_vpu_device *vpu = dev_get_drvdata(&pdev->dev);
> +
> +	if (vpu->support_follower) {
> +		if (!pm_runtime_suspended(&pdev->dev))
> +			pm_runtime_put_sync(&pdev->dev);
> +
> +		wave6_support_follower(vpu, NULL, WAVE6_VPU_FLAG_SLEEP);
> +	}
> +
> +	pm_runtime_disable(&pdev->dev);
> +	mutex_destroy(&vpu->lock);
> +}
> +
> +static int __maybe_unused wave6_vpu_runtime_suspend(struct device *dev)
> +{
> +	struct wave6_vpu_device *vpu = dev_get_drvdata(dev);
> +
> +	if (!vpu->num_clks)
> +		return 0;
> +
> +	clk_bulk_disable_unprepare(vpu->num_clks, vpu->clks);
> +	return 0;
> +}
> +
> +static int __maybe_unused wave6_vpu_runtime_resume(struct device *dev)
> +{
> +	struct wave6_vpu_device *vpu = dev_get_drvdata(dev);
> +
> +	if (!vpu->num_clks)
> +		return 0;
> +
> +	return clk_bulk_prepare_enable(vpu->num_clks, vpu->clks);
> +}
> +
> +static int __maybe_unused wave6_vpu_suspend(struct device *dev)
> +{
> +	struct wave6_vpu_device *vpu = dev_get_drvdata(dev);
> +
> +	wave6_support_follower(vpu, NULL, WAVE6_VPU_FLAG_SLEEP);

Not sure I like it, its kind of move the ref-count in the air. I don't
have a suggestion atm, but perhaps we can do better.

> +
> +	return 0;
> +}
> +
> +static int __maybe_unused wave6_vpu_resume(struct device *dev)
> +{
> +	struct wave6_vpu_device *vpu = dev_get_drvdata(dev);
> +
> +	wave6_support_follower(vpu, NULL, WAVE6_VPU_FLAG_WAKEUP);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops wave6_vpu_pm_ops = {
> +	SET_RUNTIME_PM_OPS(wave6_vpu_runtime_suspend,
> +			   wave6_vpu_runtime_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(wave6_vpu_suspend,
> +				wave6_vpu_resume)
> +};
> +
> +static const struct of_device_id wave6_vpu_ids[] = {
> +	{ .compatible = "nxp,imx95-vpu" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, wave6_vpu_ids);
> +
> +static struct platform_driver wave6_vpu_driver = {
> +	.driver = {
> +		.name = VPU_PLATFORM_DEVICE_NAME,
> +		.of_match_table = wave6_vpu_ids,
> +		.pm = &wave6_vpu_pm_ops,
> +	},
> +	.probe = wave6_vpu_probe,
> +	.remove = wave6_vpu_remove,
> +};
> +
> +module_platform_driver(wave6_vpu_driver);
> +MODULE_DESCRIPTION("chips&media Wave6 VPU driver");
> +MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/drivers/media/platform/chips-media/wave6/wave6-vpu.h b/drivers/media/platform/chips-media/wave6/wave6-vpu.h
> new file mode 100644
> index 000000000000..faa5f8af3191
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-vpu.h
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
> +/*
> + * Wave6 series multi-standard codec IP - wave6 driver
> + *
> + * Copyright (C) 2025 CHIPS&MEDIA INC
> + */
> +
> +#ifndef __WAVE6_VPU_H__
> +#define __WAVE6_VPU_H__
> +
> +#include <linux/device.h>
> +
> +#define WAVE6_VPU_MAXIMUM_ENTITY_CNT	4
> +
> +#define call_vop(vpu, op, args...)					\
> +	((vpu)->ops->op ? (vpu)->ops->op(vpu, ##args) : 0)		\
> +
> +#define call_void_vop(vpu, op, args...)					\
> +	do {								\
> +		if ((vpu)->ops->op)					\
> +			(vpu)->ops->op(vpu, ##args);			\
> +	} while (0)
> +
> +struct vpu_buf {
> +	size_t size;
> +	dma_addr_t daddr;
> +	void *vaddr;
> +	struct device *dev;
> +};
> +
> +struct wave6_vpu_entity {
> +	struct list_head list;
> +	struct device *dev;
> +	struct device *vpu;
> +	u32 (*read_reg)(struct device *dev, u32 addr);
> +	void (*write_reg)(struct device *dev, u32 addr, u32 data);
> +	void (*on_boot)(struct device *dev);
> +	void (*pause)(struct device *dev, int resume);
> +	bool active;
> +	int index;
> +};
> +
> +struct wave6_vpu_ctrl_ops {
> +	int (*get_ctrl)(struct device *ctrl, struct wave6_vpu_entity *entity);
> +	void (*put_ctrl)(struct device *ctrl, struct wave6_vpu_entity *entity);
> +	int (*require_work_buffer)(struct device *ctrl,
> +				   struct wave6_vpu_entity *entity);
> +};
> +
> +struct wave6_vpu_device;
> +
> +struct wave6_vpu_ops {
> +	int (*get_vpu)(struct wave6_vpu_device *vpu,
> +		       struct wave6_vpu_entity *entity);
> +	void (*put_vpu)(struct wave6_vpu_device *vpu,
> +			struct wave6_vpu_entity *entity);
> +	int (*reg_core)(struct wave6_vpu_device *vpu,
> +			struct wave6_vpu_entity *entity);
> +	void (*unreg_core)(struct wave6_vpu_device *vpu,
> +			   struct wave6_vpu_entity *entity);
> +	int (*reg_ctrl)(struct wave6_vpu_device *vpu, struct device *ctrl,
> +			const struct wave6_vpu_ctrl_ops *ops);
> +	void (*unreg_ctrl)(struct wave6_vpu_device *vpu, struct device *ctrl);
> +	void (*req_work_buffer)(struct wave6_vpu_device *vpu,
> +				struct wave6_vpu_entity *entity);
> +	unsigned long (*get_clk_rate)(struct wave6_vpu_device *vpu);
> +};
> +
> +struct wave6_vpu_device {
> +	struct device *dev;
> +	const struct wave6_vpu_ops *ops;
> +	struct clk_bulk_data *clks;
> +	int num_clks;
> +	struct device *ctrl;
> +	const struct wave6_vpu_ctrl_ops *ctrl_ops;
> +	struct wave6_vpu_entity *entities[WAVE6_VPU_MAXIMUM_ENTITY_CNT];
> +	struct mutex lock; /* the lock for vpu device */
> +	atomic_t ref_count;
> +	bool support_follower;
> +};

All structs could gain having documentation.

Nicolas

> +
> +int wave6_alloc_dma(struct device *dev, struct vpu_buf *vb);
> +void wave6_free_dma(struct vpu_buf *vb);
> +
> +#endif /* __WAVE6_VPU_H__ */

Powered by blists - more mailing lists