[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7a727add-6aa6-fe3d-b2bd-7e0bd2f93579@linaro.org>
Date: Fri, 28 Jul 2023 19:41:14 +0200
From: Konrad Dybcio <konrad.dybcio@...aro.org>
To: Vikash Garodia <quic_vgarodia@...cinc.com>,
stanimir.k.varbanov@...il.com, agross@...nel.org,
andersson@...nel.org, mchehab@...nel.org, hans.verkuil@...co.com,
linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
linux-arm-msm@...r.kernel.org
Cc: quic_dikshita@...cinc.com
Subject: Re: [PATCH 10/33] iris: vidc: add helper functions
On 28.07.2023 15:23, Vikash Garodia wrote:
> This implements common helper functions for v4l2 to vidc and
> vice versa conversion for different enums.
> Add helpers for state checks, buffer management, locks etc.
>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@...cinc.com>
> Signed-off-by: Vikash Garodia <quic_vgarodia@...cinc.com>
> ---
[...]
> +
> +#define is_odd(val) ((val) % 2 == 1)
> +#define in_range(val, min, max) (((min) <= (val)) && ((val) <= (max)))
> +#define COUNT_BITS(a, out) { \
hweight.* functions?
[...]
> +
> +const char *cap_name(enum msm_vidc_inst_capability_type cap_id)
> +{
> + const char *name = "UNKNOWN CAP";
Perhaps it'd be worth to include the unknown cap id here
> +
> + if (cap_id >= ARRAY_SIZE(cap_name_arr))
> + goto exit;
> +
> + name = cap_name_arr[cap_id];
> +
> +exit:
> + return name;
> +}
[...]
> +
> +const char *buf_name(enum msm_vidc_buffer_type type)
> +{
> + const char *name = "UNKNOWN BUF";
Similarly here
> +
> + if (type >= ARRAY_SIZE(buf_type_name_arr))
> + goto exit;
> +
> + name = buf_type_name_arr[type];
> +
> +exit:
> + return name;
> +}
[...]
> +const char *v4l2_type_name(u32 port)
> +{
> + switch (port) {
switch-case seems a bit excessive here.
> + case INPUT_MPLANE: return "INPUT";
> + case OUTPUT_MPLANE: return "OUTPUT";
> + }
> +
> + return "UNKNOWN";
> +}
[...]
There's some more stuff I'd comment on, but 4500 lines in a single patch
is way too much to logically follow.
Couple more style suggestions:
- use Reverse-Christmas-tree sorting for variable declarations
- some oneliner functions could possibly become preprocessor macros
- when printing giant debug messages, you may want to use loops
- make sure your indentation is in order, 100 chars per line is
totally fine
- generally inline magic hex values are discouraged, but if they're
necessary, the hex should be lowercase
Konrad
Powered by blists - more mailing lists