Re: [PATCH v2 39/44] media: ipu6: Move isys fw mapping to pci_probe

Moi,

Thanks for the update!

On Fri, Aug 21, 2026 at 02:42:57PM +0300, Antti Laakso wrote:
> We are bout to add ipu7 firmware mapping. It is slightly different from

s/bout/about/

> ipu6. Handling this and possible errors is easier when both isys and
> psys mapping is in one place.
> 
> Signed-off-by: Antti Laakso <[email protected]>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-bus.h  |  1 -
>  drivers/media/pci/intel/ipu6/ipu6-isys.c | 29 -----------
>  drivers/media/pci/intel/ipu6/ipu6.c      | 63 +++++++++++++++++++-----
>  3 files changed, 50 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-bus.h b/drivers/media/pci/intel/ipu6/ipu6-bus.h
> index aef8e4a66c4a..d2f93eb03dea 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-bus.h
> +++ b/drivers/media/pci/intel/ipu6/ipu6-bus.h
> @@ -26,7 +26,6 @@ struct ipu6_bus_device {
>  	struct ipu6_mmu *mmu;
>  	struct ipu6_device *isp;
>  	const struct ipu6_buttress_ctrl *ctrl;
> -	const struct firmware *fw;
>  	struct sg_table fw_sgt;
>  	u64 *pkg_dir;
>  	dma_addr_t pkg_dir_dma_addr;
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> index 459121e6c1cb..3fb34d2d189c 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
> @@ -995,7 +995,6 @@ static int isys_probe(struct auxiliary_device *auxdev,
>  	const struct ipu6_isys_internal_csi2_pdata *csi2_pdata;
>  	struct ipu6_bus_device *adev = auxdev_to_adev(auxdev);
>  	struct ipu6_device *isp = adev->isp;
> -	const struct firmware *fw;
>  	struct ipu6_isys *isys;
>  	unsigned int i;
>  	int ret;
> @@ -1040,18 +1039,6 @@ static int isys_probe(struct auxiliary_device *auxdev,
>  
>  	isys_stream_init(isys);
>  
> -	if (!isp->secure_mode) {
> -		fw = isp->cpd_fw;
> -		ret = ipu6_map_fw_region(adev, fw->data, fw->size,
> -					 DMA_TO_DEVICE, 0);
> -		if (ret)
> -			goto release_firmware;
> -
> -		ret = ipu6_cpd_create_pkg_dir(adev, isp->cpd_fw->data);
> -		if (ret)
> -			goto remove_shared_buffer;
> -	}
> -
>  	cpu_latency_qos_add_request(&isys->pm_qos, PM_QOS_DEFAULT_VALUE);
>  
>  	ret = alloc_fw_msg_bufs(isys, 20);
> @@ -1079,14 +1066,6 @@ static int isys_probe(struct auxiliary_device *auxdev,
>  	free_fw_msg_bufs(isys);
>  out_remove_pkg_dir_shared_buffer:
>  	cpu_latency_qos_remove_request(&isys->pm_qos);
> -	if (!isp->secure_mode)
> -		ipu6_cpd_free_pkg_dir(adev);
> -remove_shared_buffer:
> -	if (!isp->secure_mode)
> -		ipu6_unmap_fw_region(adev, DMA_TO_DEVICE);
> -release_firmware:
> -	if (!isp->secure_mode)
> -		release_firmware(adev->fw);
>  
>  	for (i = 0; i < IPU6_ISYS_MAX_STREAMS; i++)
>  		mutex_destroy(&isys->streams[i].mutex);
> @@ -1099,9 +1078,7 @@ static int isys_probe(struct auxiliary_device *auxdev,
>  
>  static void isys_remove(struct auxiliary_device *auxdev)
>  {
> -	struct ipu6_bus_device *adev = auxdev_to_adev(auxdev);
>  	struct ipu6_isys *isys = dev_get_drvdata(&auxdev->dev);
> -	struct ipu6_device *isp = adev->isp;
>  	unsigned int i;
>  
>  	free_fw_msg_bufs(isys);
> @@ -1111,12 +1088,6 @@ static void isys_remove(struct auxiliary_device *auxdev)
>  
>  	cpu_latency_qos_remove_request(&isys->pm_qos);
>  
> -	if (!isp->secure_mode) {
> -		ipu6_cpd_free_pkg_dir(adev);
> -		ipu6_unmap_fw_region(adev, DMA_TO_DEVICE);
> -		release_firmware(adev->fw);
> -	}
> -
>  	for (i = 0; i < IPU6_ISYS_MAX_STREAMS; i++)
>  		mutex_destroy(&isys->streams[i].mutex);
>  
> diff --git a/drivers/media/pci/intel/ipu6/ipu6.c b/drivers/media/pci/intel/ipu6/ipu6.c
> index af5581da7149..d27ffd7ac6f0 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6.c
> @@ -480,6 +480,40 @@ static void ipu6_configure_vc_mechanism(struct ipu6_device *isp)
>  	writel(val, isp->base + BUTTRESS_REG_BTRS_CTRL);
>  }
>  
> +static int __ipu6_map_fw_by_sys(struct ipu6_device *isp, struct ipu6_bus_device *adev)
> +{
> +	int ret = ipu6_map_fw_region(adev, isp->cpd_fw->data, isp->cpd_fw->size,

Please avoid assigning variables in declaration block when there are errors
to check.

> +				 DMA_TO_DEVICE, 0);
> +
> +	if (ret) {
> +		dev_err_probe(&isp->pdev->dev, ret,
> +			      "Firmware mapping failed\n");
> +		return ret;
> +	}
> +
> +	ret = ipu6_cpd_create_pkg_dir(adev, isp->cpd_fw->data);
> +	if (ret) {
> +		dev_err_probe(&isp->pdev->dev, ret,
> +			      "failed to create pkg dir\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ipu6_map_fw(struct ipu6_device *isp)
> +{
> +	int ret = __ipu6_map_fw_by_sys(isp, isp->psys);

Ditto.

> +
> +	if (ret)
> +		return ret;
> +
> +	if (!isp->secure_mode)
> +		return __ipu6_map_fw_by_sys(isp, isp->isys);
> +
> +	return 0;
> +}
> +
>  static int ipu6_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	const struct ipu6_buttress_ctrl *isys_ctrl, *psys_ctrl;
> @@ -624,19 +658,9 @@ static int ipu6_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  		goto out_ipu6_rpm_put;
>  	}
>  
> -	ret = ipu6_map_fw_region(isp->psys, isp->cpd_fw->data,
> -				 isp->cpd_fw->size, DMA_TO_DEVICE, 0);
> -	if (ret) {
> -		dev_err_probe(&isp->pdev->dev, ret, "failed to map fw image\n");
> -		goto out_ipu6_rpm_put;
> -	}
> -
> -	ret = ipu6_cpd_create_pkg_dir(isp->psys, isp->cpd_fw->data);
> -	if (ret) {
> -		dev_err_probe(&isp->pdev->dev, ret,
> -			      "failed to create pkg dir\n");
> +	ret = ipu6_map_fw(isp);
> +	if (ret)
>  		goto out_ipu6_rpm_put;
> -	}
>  
>  	ret = devm_request_threaded_irq(dev, pdev->irq, ipu6_buttress_isr,
>  					ipu6_buttress_isr_threaded,
> @@ -679,7 +703,13 @@ static int ipu6_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  out_ipu6_bus_del_devices:
>  	if (!IS_ERR_OR_NULL(isp->psys)) {
>  		ipu6_cpd_free_pkg_dir(isp->psys);
> -		ipu6_unmap_fw_region(isp->psys, DMA_TO_DEVICE);
> +		if (isp->psys->fw_sgt.nents)
> +			ipu6_unmap_fw_region(isp->psys, DMA_TO_DEVICE);
> +	}
> +	if (!IS_ERR_OR_NULL(isp->isys)) {
> +		ipu6_cpd_free_pkg_dir(isp->isys);
> +		if (isp->isys->fw_sgt.nents)
> +			ipu6_unmap_fw_region(isp->isys, DMA_TO_DEVICE);
>  	}
>  	if (!IS_ERR_OR_NULL(isp->psys) && !IS_ERR_OR_NULL(isp->psys->mmu))
>  		ipu6_mmu_cleanup(isp->psys->mmu);
> @@ -703,6 +733,13 @@ static void ipu6_pci_remove(struct pci_dev *pdev)
>  	ipu6_cpd_free_pkg_dir(isp->psys);
>  
>  	ipu6_unmap_fw_region(isp->psys, DMA_TO_DEVICE);
> +
> +	if (isp->isys) {
> +		ipu6_cpd_free_pkg_dir(isp->isys);
> +		if (isp->isys->fw_sgt.nents)
> +			ipu6_unmap_fw_region(isp->isys, DMA_TO_DEVICE);
> +	}
> +
>  	ipu6_buttress_exit(isp);
>  
>  	ipu6_bus_del_devices(pdev);

-- 
Terveisin,

Sakari Ailus