Re: [PATCH 2/5] staging: media: atomisp: runtime: frame: remove #ifdef ISP2401

[Hans de Goede <hdegoede@redhat.com>]

Hi Kate,

On 4/25/23 09:48, Kate Hsuan wrote:
> The actions of ISP2401 and 2400 are determined at the runtime.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  .../media/atomisp/pci/runtime/frame/src/frame.c   | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c b/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c
> index 83bb42e05421..425e75f7dda7 100644
> --- a/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c
> +++ b/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c
> @@ -601,9 +601,9 @@ static void frame_init_qplane6_planes(struct ia_css_frame *frame)
>  
>  static int frame_allocate_buffer_data(struct ia_css_frame *frame)
>  {
> -#ifdef ISP2401
> -	IA_CSS_ENTER_LEAVE_PRIVATE("frame->data_bytes=%d\n", frame->data_bytes);
> -#endif
> +	if (IS_ISP2401)
> +		IA_CSS_ENTER_LEAVE_PRIVATE("frame->data_bytes=%d\n", frame->data_bytes);
> +
>  	frame->data = hmm_alloc(frame->data_bytes);
>  	if (frame->data == mmgr_NULL)
>  		return -ENOMEM;

This is just a debug log message, IMHO it is best to just completely remove
the message for both ISP models.


> @@ -635,11 +635,10 @@ static int frame_allocate_with_data(struct ia_css_frame **frame,
>  
>  	if (err) {
>  		kvfree(me);
> -#ifndef ISP2401
> -		return err;
> -#else
> -		me = NULL;
> -#endif
> +		if (IS_ISP2401)
> +			me = NULL;
> +		else
> +			return err;
>  	}
>  
>  	*frame = me;

This one is also weird. I have checked the only caller of this and it
does not matter what *frame is set to since as long as this returns
non 0 the *frame is ignored and the functions always returns err
(just outside of the context of the patch).

So this can be simplified to just:

	if (err) {
		kvfree(me);
		me = NULL;
	}

And then fall through to the original code of:

  	*frame = me;

	return err;
}


The me = NULL is not strictly necessary but setting *frame = NULL
on errors is a bit cleaner and may help catch future bugs.

Regards,

Hans