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@xxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxx>
>> Signed-off-by: Gjorgji Rosikopulos <quic_grosikop@xxxxxxxxxxx>
>> ---
>> 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
