Re: [PATCH 13/57] media: atomisp: Remove deferred firmware loading support

[Andy Shevchenko <andy@kernel.org>]

On Mon, Jan 23, 2023 at 01:51:21PM +0100, Hans de Goede wrote:
> Make atomisp behave like any othet drivers and have it load the firmware

other

> at probe time (as it was already doing by default).
> 
> The deferred firmware loading support needlessly complicates the
> v4l2_file_operations.open callback (atomisp_open()), getting in
> the way of allowing multiple opens like a normal v4l2 device would.

So it actually removes that variable and accompanying code w.o.
changing behaviour (as default).

Reviewed-by: Andy Shevchenko <andy@kernel.org>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../staging/media/atomisp/pci/atomisp_fops.c  | 25 -----------
>  .../staging/media/atomisp/pci/atomisp_fops.h  |  2 -
>  .../staging/media/atomisp/pci/atomisp_v4l2.c  | 42 +++++++------------
>  3 files changed, 14 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
> index 682239ea042f..036ad339b344 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
> @@ -757,25 +757,6 @@ static int atomisp_open(struct file *file)
>  	mutex_lock(&isp->mutex);
>  
>  	asd->subdev.devnode = vdev;
> -	/* Deferred firmware loading case. */
> -	if (isp->css_env.isp_css_fw.bytes == 0) {
> -		dev_err(isp->dev, "Deferred firmware load.\n");
> -		isp->firmware = atomisp_load_firmware(isp);
> -		if (!isp->firmware) {
> -			dev_err(isp->dev, "Failed to load ISP firmware.\n");
> -			ret = -ENOENT;
> -			goto error;
> -		}
> -		ret = atomisp_css_load_firmware(isp);
> -		if (ret) {
> -			dev_err(isp->dev, "Failed to init css.\n");
> -			goto error;
> -		}
> -		/* No need to keep FW in memory anymore. */
> -		release_firmware(isp->firmware);
> -		isp->firmware = NULL;
> -		isp->css_env.isp_css_fw.data = NULL;
> -	}
>  
>  	if (!isp->input_cnt) {
>  		dev_err(isp->dev, "no camera attached\n");
> @@ -901,12 +882,6 @@ static int atomisp_release(struct file *file)
>  
>  	atomisp_destroy_pipes_stream_force(asd);
>  
> -	if (defer_fw_load) {
> -		ia_css_unload_firmware();
> -		isp->css_env.isp_css_fw.data = NULL;
> -		isp->css_env.isp_css_fw.bytes = 0;
> -	}
> -
>  	ret = v4l2_subdev_call(isp->flash, core, s_power, 0);
>  	if (ret < 0 && ret != -ENODEV && ret != -ENOIOCTLCMD)
>  		dev_warn(isp->dev, "Failed to power-off flash\n");
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.h b/drivers/staging/media/atomisp/pci/atomisp_fops.h
> index 10e43126b693..2efc5245e571 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_fops.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_fops.h
> @@ -33,6 +33,4 @@ int atomisp_qbuffers_to_css(struct atomisp_sub_device *asd);
>  
>  extern const struct v4l2_file_operations atomisp_fops;
>  
> -extern bool defer_fw_load;
> -
>  #endif /* __ATOMISP_FOPS_H__ */
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> index aa05c69a5c6b..2a949d3dc5d1 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> @@ -58,12 +58,6 @@ static uint skip_fwload;
>  module_param(skip_fwload, uint, 0644);
>  MODULE_PARM_DESC(skip_fwload, "Skip atomisp firmware load");
>  
> -/* memory optimization: deferred firmware loading */
> -bool defer_fw_load;
> -module_param(defer_fw_load, bool, 0644);
> -MODULE_PARM_DESC(defer_fw_load,
> -		 "Defer FW loading until device is opened (default:disable)");
> -
>  /* cross componnet debug message flag */
>  int dbg_level;
>  module_param(dbg_level, int, 0644);
> @@ -1514,21 +1508,17 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
>  	isp->max_isr_latency = ATOMISP_MAX_ISR_LATENCY;
>  
>  	/* Load isp firmware from user space */
> -	if (!defer_fw_load) {
> -		isp->firmware = atomisp_load_firmware(isp);
> -		if (!isp->firmware) {
> -			err = -ENOENT;
> -			dev_dbg(&pdev->dev, "Firmware load failed\n");
> -			goto load_fw_fail;
> -		}
> +	isp->firmware = atomisp_load_firmware(isp);
> +	if (!isp->firmware) {
> +		err = -ENOENT;
> +		dev_dbg(&pdev->dev, "Firmware load failed\n");
> +		goto load_fw_fail;
> +	}
>  
> -		err = sh_css_check_firmware_version(isp->dev, isp->firmware->data);
> -		if (err) {
> -			dev_dbg(&pdev->dev, "Firmware version check failed\n");
> -			goto fw_validation_fail;
> -		}
> -	} else {
> -		dev_info(&pdev->dev, "Firmware load will be deferred\n");
> +	err = sh_css_check_firmware_version(isp->dev, isp->firmware->data);
> +	if (err) {
> +		dev_dbg(&pdev->dev, "Firmware version check failed\n");
> +		goto fw_validation_fail;
>  	}
>  
>  	pci_set_master(pdev);
> @@ -1628,14 +1618,10 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
>  	}
>  
>  	/* Load firmware into ISP memory */
> -	if (!defer_fw_load) {
> -		err = atomisp_css_load_firmware(isp);
> -		if (err) {
> -			dev_err(&pdev->dev, "Failed to init css.\n");
> -			goto css_init_fail;
> -		}
> -	} else {
> -		dev_dbg(&pdev->dev, "Skip css init.\n");
> +	err = atomisp_css_load_firmware(isp);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to init css.\n");
> +		goto css_init_fail;
>  	}
>  	/* Clear FW image from memory */
>  	release_firmware(isp->firmware);
> -- 
> 2.39.0
> 

-- 
With Best Regards,
Andy Shevchenko