Message-ID:
 <SL2P216MB1246B403C2904F92EB5E9572FB91A@SL2P216MB1246.KORP216.PROD.OUTLOOK.COM>
Date: Wed, 14 May 2025 08:09:36 +0000
From: Nas Chung <nas.chung@...psnmedia.com>
To: Nicolas Dufresne <nicolas@...fresne.ca>
CC: "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-imx@....com" <linux-imx@....com>, "marex@...x.de" <marex@...x.de>,
	jackson.lee <jackson.lee@...psnmedia.com>, lafley.kim
	<lafley.kim@...psnmedia.com>, Ming Qian <ming.qian@....nxp.com>,
	"mchehab@...nel.org" <mchehab@...nel.org>, "hverkuil@...all.nl"
	<hverkuil@...all.nl>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>
Subject: RE: [PATCH v2 3/8] media: chips-media: wave6: Add Wave6 driver

Hi, Nicolas.

Thank you for your feedback, I apologize for the delayed response.

>-----Original Message-----
>From: Nicolas Dufresne <nicolas@...fresne.ca>
>Sent: Friday, April 25, 2025 2:37 AM
>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
><jackson.lee@...psnmedia.com>; lafley.kim <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.

I agree with you.
I will remove this configuration and discuss it separately.
And, I think the support_follower related code should be removed together.
I will address this in v3.

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

We include the BSD-3-Clause license primarily to allow reuse of this code.
Although the V4L2 driver itself cannot be used outside of Linux,
certain parts of the code, such as register read/write operations,
can be referenced or reused.

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

Sure !

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

Thank you. I'll revise it to avoid using the EXPORT_SYMBOL_GPL in v3.

>
>> +
>> +/**
>> + * 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()" ?

Sorry for confusion. I will rename the function.

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

Yes, I wanted to prevent programming errors in this case.
I will modify it to use WARN_ON in v3.

>
>> +{
>> +	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);

Okay, I will address this in v3.

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

Okay.

>
>> +
>> +	mutex_lock(&vpu->lock);
>
>Replace with:
>
>	guard(mutex)(&vpu->lock);

Okay, I will replace all below mutex_lock() to guard().

>
>> +
>> +	if (wave6_check_entity(vpu, entity)) {
>> +		ret = -EINVAL;
>> +		goto unlock;
>
>Then these two lines becomes:
>
>		return -EINVAL;
>
>You won't even need a scope.

I see. I will address this in v3.

>
>> +	}
>> +
>> +	if (!entity->active)
>> +		goto unlock;
>		return 0;
>
>> +
>> +	ret = __wave6_vpu_get(vpu, entity);
>> +
>> +unlock:
>> +	mutex_unlock(&vpu->lock);
>
>Drop the two above lines;

Okay.

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

Understood. I will remove the support_follower related code first.

>
>> +	struct wave6_vpu_entity *target = NULL;
>> +	int ret;
>> +	int i;
>
>Seems like this needs to be called with lock held:

Okay.

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

I see, I will address this in v3.

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

Understood, I will remove the support_follower related code to improve clarity.

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

Yes ! I will modify it to use WARN_ON in v3.

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

Sorry again for your confusion, I will remove this ambiguous codes.

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

Understood. I will remove the support_follower related code.

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

Okay, I will add the documentation for all structs.

Thanks.
Nas.

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