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@xxxxxxxxxx>
> ---
> .../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
