Message-ID: <2c3b9e21-06fc-4108-80d7-c4e2c4ceb2fa@kernel.org>
Date: Mon, 10 Feb 2025 18:37:44 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
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,
 linux-arm-kernel@...ts.infradead.org, jackson.lee@...psnmedia.com,
 lafley.kim@...psnmedia.com
Subject: Re: [PATCH 7/8] media: chips-media: wave6: Add Wave6 control driver

On 10/02/2025 10:07, Nas Chung wrote:
> +
> +int wave6_vpu_ctrl_resume_and_get(struct device *dev, struct wave6_vpu_entity *entity)
> +{
> +	struct vpu_ctrl *ctrl = dev_get_drvdata(dev);
> +	bool boot;
> +	int ret;
> +
> +	if (!ctrl)
> +		return -EINVAL;
> +
> +	if (!entity || !entity->dev || !entity->read_reg || !entity->write_reg)
> +		return -EINVAL;
> +
> +	mutex_lock(&ctrl->ctrl_lock);
> +
> +	ret = pm_runtime_resume_and_get(ctrl->dev);
> +	if (ret) {
> +		dev_err(dev, "pm runtime resume fail, ret = %d\n", ret);
> +		mutex_unlock(&ctrl->ctrl_lock);
> +		return ret;
> +	}
> +
> +	entity->booted = false;
> +
> +	if (ctrl->current_entity)
> +		boot = false;
> +	else
> +		boot = list_empty(&ctrl->entities) ? true : false;
> +
> +	list_add_tail(&entity->list, &ctrl->entities);
> +	if (boot)
> +		ret = wave6_vpu_ctrl_try_boot(ctrl, entity);
> +
> +	if (ctrl->state == WAVE6_VPU_STATE_ON)
> +		wave6_vpu_ctrl_on_boot(entity);
> +
> +	if (ret)
> +		pm_runtime_put_sync(ctrl->dev);
> +
> +	mutex_unlock(&ctrl->ctrl_lock);
> +
> +	return ret;
> +}

Drop entire function.

> +EXPORT_SYMBOL_GPL(wave6_vpu_ctrl_resume_and_get);

Drop.

> +
> +void wave6_vpu_ctrl_put_sync(struct device *dev, struct wave6_vpu_entity *entity)
> +{
> +	struct vpu_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	if (!ctrl)
> +		return;
> +
> +	if (entity == ctrl->current_entity)
> +		wave6_vpu_ctrl_wait_done(dev);
> +
> +	mutex_lock(&ctrl->ctrl_lock);
> +
> +	if (!wave6_vpu_ctrl_find_entity(ctrl, entity))
> +		goto exit;
> +
> +	list_del_init(&entity->list);
> +	if (list_empty(&ctrl->entities)) {
> +		if (!entity->read_reg(entity->dev, W6_VPU_VCPU_CUR_PC))
> +			wave6_vpu_ctrl_set_state(ctrl, WAVE6_VPU_STATE_OFF);
> +		else
> +			wave6_vpu_ctrl_sleep(ctrl, entity);
> +	}
> +
> +	if (!pm_runtime_suspended(ctrl->dev))
> +		pm_runtime_put_sync(ctrl->dev);
> +exit:
> +	mutex_unlock(&ctrl->ctrl_lock);
> +}
> +EXPORT_SYMBOL_GPL(wave6_vpu_ctrl_put_sync);

Drop entire function. Dead code.

... or you arranged your patches really incorrectly, but this I can't
really judge.


> +
> +int wave6_vpu_ctrl_wait_done(struct device *dev)
> +{
> +	struct vpu_ctrl *ctrl = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!ctrl)
> +		return -EINVAL;
> +
> +	if (ctrl->state == WAVE6_VPU_STATE_OFF)
> +		return -EINVAL;
> +
> +	if (ctrl->state == WAVE6_VPU_STATE_ON)
> +		return 0;
> +
> +	ret = wave6_wait_event_freezable_timeout(ctrl->load_fw_wq,
> +						 wave6_vpu_ctrl_get_state(dev) ==
> +						 WAVE6_VPU_STATE_ON,
> +						 msecs_to_jiffies(W6_BOOT_WAIT_TIMEOUT));
> +	if (ret == -ERESTARTSYS || !ret) {
> +		dev_err(ctrl->dev, "fail to wait vcpu boot done,ret %d\n", ret);
> +		mutex_lock(&ctrl->ctrl_lock);
> +		wave6_vpu_ctrl_set_state(ctrl, WAVE6_VPU_STATE_OFF);
> +		mutex_unlock(&ctrl->ctrl_lock);
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&ctrl->ctrl_lock);
> +	wave6_vpu_ctrl_boot_done(ctrl, 0);
> +	mutex_unlock(&ctrl->ctrl_lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(wave6_vpu_ctrl_wait_done);

There is no user of this outside. Drop.


> +
> +int wave6_vpu_ctrl_get_state(struct device *dev)
> +{
> +	struct vpu_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	if (!ctrl)
> +		return -EINVAL;
> +
> +	return ctrl->state;
> +}
> +EXPORT_SYMBOL_GPL(wave6_vpu_ctrl_get_state);

There is no user of this outside. Drop.

Whatever export stays, must be used by a following patch. You *must* add
the kerneldoc for it.

> +
> +static void wave6_vpu_ctrl_init_reserved_boot_region(struct vpu_ctrl *ctrl)
> +{
> +	if (ctrl->boot_mem.size < WAVE6_CODE_BUF_SIZE) {
> +		dev_warn(ctrl->dev, "boot memory size (%zu) is too small\n", ctrl->boot_mem.size);
> +		ctrl->boot_mem.phys_addr = 0;
> +		ctrl->boot_mem.size = 0;
> +		memset(&ctrl->boot_mem, 0, sizeof(ctrl->boot_mem));
> +		return;
> +	}
> +
> +	ctrl->boot_mem.vaddr = devm_memremap(ctrl->dev,
> +					     ctrl->boot_mem.phys_addr,
> +					     ctrl->boot_mem.size,
> +					     MEMREMAP_WC);
> +	if (!ctrl->boot_mem.vaddr) {
> +		memset(&ctrl->boot_mem, 0, sizeof(ctrl->boot_mem));
> +		return;
> +	}
> +
> +	ctrl->boot_mem.dma_addr = dma_map_resource(ctrl->dev,
> +						   ctrl->boot_mem.phys_addr,
> +						   ctrl->boot_mem.size,
> +						   DMA_BIDIRECTIONAL,
> +						   0);
> +	if (!ctrl->boot_mem.dma_addr) {
> +		memset(&ctrl->boot_mem, 0, sizeof(ctrl->boot_mem));
> +		return;
> +	}
> +
> +	dev_info(ctrl->dev, "boot phys_addr: %pad, dma_addr: %pad, size: 0x%zx\n",
> +		 &ctrl->boot_mem.phys_addr,
> +		 &ctrl->boot_mem.dma_addr,
> +		 ctrl->boot_mem.size);

No, drop. Reasoning further.

> +}
...

> +
> +	ctrl->num_clks = ret;
> +
> +	np = of_parse_phandle(pdev->dev.of_node, "boot", 0);
> +	if (np) {
> +		struct resource mem;
> +
> +		ret = of_address_to_resource(np, 0, &mem);
> +		of_node_put(np);
> +		if (!ret) {
> +			ctrl->boot_mem.phys_addr = mem.start;
> +			ctrl->boot_mem.size = resource_size(&mem);
> +			wave6_vpu_ctrl_init_reserved_boot_region(ctrl);
> +		} else {
> +			dev_warn(&pdev->dev, "boot resource is not available.\n");
> +		}
> +	}
> +
> +	ctrl->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
> +	if (ctrl->sram_pool) {
> +		ctrl->sram_buf.size = ctrl->res->sram_size;
> +		ctrl->sram_buf.vaddr = gen_pool_dma_alloc(ctrl->sram_pool,
> +							  ctrl->sram_buf.size,
> +							  &ctrl->sram_buf.phys_addr);
> +		if (!ctrl->sram_buf.vaddr)
> +			ctrl->sram_buf.size = 0;
> +		else
> +			ctrl->sram_buf.dma_addr = dma_map_resource(&pdev->dev,
> +								   ctrl->sram_buf.phys_addr,
> +								   ctrl->sram_buf.size,
> +								   DMA_BIDIRECTIONAL,
> +								   0);
> +
> +		dev_info(&pdev->dev, "sram 0x%pad, 0x%pad, size 0x%lx\n",
> +			 &ctrl->sram_buf.phys_addr, &ctrl->sram_buf.dma_addr, ctrl->sram_buf.size);

You are not supposed to print addresses. This look like fixed hardware
mappings, so probably harmless but dma_addr not. Drop entire dev_info,
this is really discouraged practice.


> +	}
> +
> +	if (of_find_property(pdev->dev.of_node, "support-follower", NULL))
> +		ctrl->support_follower = true;
> +
> +	wave6_cooling_init(ctrl);
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static void wave6_vpu_ctrl_remove(struct platform_device *pdev)
> +{
> +	struct vpu_ctrl *ctrl = dev_get_drvdata(&pdev->dev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	wave6_vpu_ctrl_clear_buffers(ctrl);
> +	wave6_cooling_remove(ctrl);
> +	if (ctrl->sram_pool && ctrl->sram_buf.vaddr) {
> +		dma_unmap_resource(&pdev->dev,
> +				   ctrl->sram_buf.dma_addr,
> +				   ctrl->sram_buf.size,
> +				   DMA_BIDIRECTIONAL,
> +				   0);
> +		gen_pool_free(ctrl->sram_pool,
> +			      (unsigned long)ctrl->sram_buf.vaddr,
> +			      ctrl->sram_buf.size);
> +	}
> +	if (ctrl->boot_mem.dma_addr)
> +		dma_unmap_resource(&pdev->dev,
> +				   ctrl->boot_mem.dma_addr,
> +				   ctrl->boot_mem.size,
> +				   DMA_BIDIRECTIONAL,
> +				   0);
> +	mutex_destroy(&ctrl->ctrl_lock);
> +}
> +
> +#ifdef CONFIG_PM
> +static int wave6_vpu_ctrl_runtime_suspend(struct device *dev)
> +{
> +	struct vpu_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	clk_bulk_disable_unprepare(ctrl->num_clks, ctrl->clks);
> +	return 0;
> +}
> +
> +static int wave6_vpu_ctrl_runtime_resume(struct device *dev)
> +{
> +	struct vpu_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	return clk_bulk_prepare_enable(ctrl->num_clks, ctrl->clks);
> +}
> +#endif
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int wave6_vpu_ctrl_suspend(struct device *dev)
> +{
> +	return 0;

Why do you need empty callbacks?

Best regards,
Krzysztof

Powered by blists - more mailing lists