Re: [PATCH v3 5/8] media: qcom: camss: Move format related functions

["Gjorgji Rosikopulos (Consultant)" <quic_grosikop@quicinc.com>]

Hi Vladimir,

Thanks for the review,


On 5/13/2024 6:39 PM, Vladimir Zapolskiy wrote:
> On 4/11/24 15:45, Gjorgji Rosikopulos wrote:
>> From: Radoslav Tsvetkov <quic_rtsvetko@quicinc.com>
>>
>> Move out the format related helper functions from vfe and video in a
>> separate file. The goal here is to create a format API.
>>
>> Signed-off-by: Radoslav Tsvetkov <quic_rtsvetko@quicinc.com>
>> Signed-off-by: Gjorgji Rosikopulos <quic_grosikop@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/camss/Makefile    |  1 +
>>   .../media/platform/qcom/camss/camss-format.c  | 98 +++++++++++++++++++
>>   .../media/platform/qcom/camss/camss-format.h  |  5 +
>>   drivers/media/platform/qcom/camss/camss-vfe.c | 86 +++++-----------
>>   .../media/platform/qcom/camss/camss-video.c   | 26 +----
>>   5 files changed, 128 insertions(+), 88 deletions(-)
>>   create mode 100644 drivers/media/platform/qcom/camss/camss-format.c
>>
>> diff --git a/drivers/media/platform/qcom/camss/Makefile
>> b/drivers/media/platform/qcom/camss/Makefile
>> index 0d4389ab312d..e636968a1126 100644
>> --- a/drivers/media/platform/qcom/camss/Makefile
>> +++ b/drivers/media/platform/qcom/camss/Makefile
>> @@ -19,5 +19,6 @@ qcom-camss-objs += \
>>           camss-vfe-gen1.o \
>>           camss-vfe.o \
>>           camss-video.o \
>> +        camss-format.o \
>>     obj-$(CONFIG_VIDEO_QCOM_CAMSS) += qcom-camss.o
>> diff --git a/drivers/media/platform/qcom/camss/camss-format.c
>> b/drivers/media/platform/qcom/camss/camss-format.c
>> new file mode 100644
>> index 000000000000..6279cb099625
>> --- /dev/null
>> +++ b/drivers/media/platform/qcom/camss/camss-format.c
>> @@ -0,0 +1,98 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Copyright (c) 2023, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2023 Qualcomm Technologies, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
> 
> SPDX-License-Identifier is fully sufficient, the licence description
> shall be removed.

I need to check, but as i can see with other files the license
description can be removed.

> 
>> +
>> +#include <linux/bug.h>
>> +#include <linux/errno.h>
>> +
>> +#include "camss-format.h"
>> +
>> +/*
>> + * camss_format_get_bpp - Map media bus format to bits per pixel
>> + * @formats: supported media bus formats array
>> + * @nformats: size of @formats array
>> + * @code: media bus format code
>> + *
>> + * Return number of bits per pixel
>> + */
>> +u8 camss_format_get_bpp(const struct camss_format_info *formats,
>> unsigned int nformats, u32 code)
>> +{
>> +    unsigned int i;
>> +
>> +    for (i = 0; i < nformats; i++)
>> +        if (code == formats[i].code)
>> +            return formats[i].mbus_bpp;
>> +
>> +    WARN(1, "Unknown format\n");
>> +
>> +    return formats[0].mbus_bpp;
>> +}
>> +
>> +/*
>> + * camss_format_find_code - Find a format code in an array
>> + * @code: a pointer to media bus format codes array
>> + * @n_code: size of @code array
>> + * @index: index of code in the array
>> + * @req_code: required code
>> + *
>> + * Return media bus format code
>> + */
>> +u32 camss_format_find_code(u32 *code, unsigned int n_code, unsigned
>> int index, u32 req_code)
>> +{
>> +    int i;
>> +
>> +    if (!req_code && index >= n_code)
>> +        return 0;
>> +
> 
> 0 as an error condition indicator is not very common, at least it shall be
> documented in the comment.

The original function was vfe_find_code. This change moves all format
related functions across the sub-device files to camss-format
I believe that 0 is default format.

> 
>> +    for (i = 0; i < n_code; i++) {
>> +        if (req_code) {
>> +            if (req_code == code[i])
>> +                return req_code;
>> +        } else {
>> +            if (i == index)
>> +                return code[i];
>> +        }
>> +    }
>> +
>> +    return code[0];
>> +}
>> +
>> +/*
>> + * camss_format_find_format - Find a format in an array
>> + * @code: media bus format code
>> + * @pixelformat: V4L2 pixel format FCC identifier
>> + * @formats: a pointer to formats array
>> + * @nformats: size of @formats array
>> + *
>> + * Return index of a format or a negative error code otherwise
>> + */
>> +int camss_format_find_format(u32 code, u32 pixelformat, const struct
>> camss_format_info *formats,
>> +                 unsigned int nformats)
>> +{
>> +    int i;
> 
> unsigned int i

Maybe it makes sense to go to all functions already existing in camss
and change int with unsigned int for for loops...

> 
>> +
>> +    for (i = 0; i < nformats; i++) {
>> +        if (formats[i].code == code &&
>> +            formats[i].pixelformat == pixelformat)
>> +            return i;
>> +    }
>> +
>> +    for (i = 0; i < nformats; i++) {
>> +        if (formats[i].code == code)
>> +            return i;
>> +    }
>> +
>> +    WARN_ON(1);
>> +
> 
> WARN_ON() is not needed here, it has to be removed.

Again this is migrated code from camss-video :/. I guess we need bigger
consensus to remove this WARN_ON. For me it makes sense to be removed.

~Gjorgji