| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a010743e-0018-4b6a-adeb-c6f3203f1e67@linaro.org>
Date: Wed, 4 Sep 2024 15:50:05 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Vikram Sharma <quic_vikramsa@...cinc.com>, Robert Foss
<rfoss@...nel.org>, Todor Tomov <todor.too@...il.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Kapatrala Syed <akapatra@...cinc.com>,
Hariram Purushothaman <hariramp@...cinc.com>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
cros-qcom-dts-watchers@...omium.org,
Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>
Cc: linux-arm-msm@...r.kernel.org, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
Suresh Vankadara <quic_svankada@...cinc.com>,
Trishansh Bhardwaj <quic_tbhardwa@...cinc.com>
Subject: Re: [PATCH 06/10] media: qcom: camss: Add camss_link_entities_v2
On 04/09/2024 12:10, Vikram Sharma wrote:
> Add camss_link_entities_v2, derived from the camss_link_entities
> function, to handle linking for targets without ISPIF.
>
> camss_link_entities -> Targets with ispif.
> camss_link_entities_v2 -> Targets without ispif.
>
> Signed-off-by: Suresh Vankadara <quic_svankada@...cinc.com>
> Signed-off-by: Trishansh Bhardwaj <quic_tbhardwa@...cinc.com>
> Signed-off-by: Vikram Sharma <quic_vikramsa@...cinc.com>
> ---
> drivers/media/platform/qcom/camss/camss.c | 53 ++++++++++++++++++++++++++++++-
> 1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 5e7235001239..516434686a27 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -2154,6 +2154,57 @@ static int camss_init_subdevices(struct camss *camss)
> return 0;
> }
>
> +/*
> + * camss_link_entities_v2 - Register subdev nodes and create links
> + * @camss: CAMSS device
> + *
> + * Return 0 on success or a negative error code on failure
> + */
> +static int camss_link_entities_v2(struct camss *camss)
> +{
> + int i, j;
> + int ret;
> +
> + for (i = 0; i < camss->res->csiphy_num; i++) {
> + for (j = 0; j < camss->res->csid_num; j++) {
> + ret = media_create_pad_link(&camss->csiphy[i].subdev.entity,
> + MSM_CSIPHY_PAD_SRC,
> + &camss->csid[j].subdev.entity,
> + MSM_CSID_PAD_SINK,
> + 0);
> + if (ret < 0) {
> + dev_err(camss->dev,
> + "Failed to link %s->%s entities: %d\n",
> + camss->csiphy[i].subdev.entity.name,
> + camss->csid[j].subdev.entity.name,
> + ret);
> + return ret;
> + }
> + }
> + }
Please reduce the above loop down into a common call that both
camss_link_entities_ispif()[1] and camss_link_entities() can call.
Because => functional decomposition and code reuse instead of code
replication.
> + for (i = 0; i < camss->res->csid_num; i++)
> + for (j = 0; j < camss->vfe[i].res->line_num; j++) {
> + struct v4l2_subdev *csid = &camss->csid[i].subdev;
> + struct v4l2_subdev *vfe = &camss->vfe[i].line[j].subdev;
> +
> + ret = media_create_pad_link(&csid->entity,
> + MSM_CSID_PAD_FIRST_SRC + j,
> + &vfe->entity,
> + MSM_VFE_PAD_SINK,
> + 0);
> + if (ret < 0) {
> + dev_err(camss->dev,
> + "Failed to link %s->%s entities: %d\n",
> + csid->entity.name,
> + vfe->entity.name,
> + ret);
> + return ret;
> + }
> + }
> + return 0;
> +}
Having a tidy function for every non ispif SoC seems like nice code,
approve.
However the corollary is the ispif version of the code should then drop the
if (camss->ispif) {
//do stuff
} else {
// do other stuff
}
i.e. your function pointer now determines if ispif is true so you can
drop the dead code on the else path in camss_link_entities_ispif()
> +
> /*
> * camss_link_entities - Register subdev nodes and create links
> * @camss: CAMSS device
> @@ -2769,7 +2820,7 @@ static const struct camss_resources sc8280xp_resources = {
> .csiphy_num = ARRAY_SIZE(csiphy_res_sc8280xp),
> .csid_num = ARRAY_SIZE(csid_res_sc8280xp),
> .vfe_num = ARRAY_SIZE(vfe_res_sc8280xp),
> - .link_entities = camss_link_entities
> + .link_entities = camss_link_entities_v2
> };
>
> static const struct camss_resources sc7280_resources = {
>
This change isn't correct.
There are six upstream camss entires
grep camss arch/arm64/boot/dts/qcom/*.dtsi | grep compat | wc -l
6
Only three of which have ispif
grep -m1 "ispif" arch/arm64/boot/dts/qcom/*.dtsi | wc -l
3
=> You've missed 2/3 of the SoCs the change should apply to - 845 and 8250.
[1] I also think the legacy function should be called
camss_link_entities_ispif() with the default being camss_link_entities();
1. Please change the name of the existing function to
camss_link_entities_ispif() to reflect its function and dependency.
2. Functionally decompose the top part of the loop into some common
method that both camss_link_entities_ispif() and
camss_link_entities() can call.
Because functionally decomposed and reused code is better code.
3. You need to make sure you hook the right set of SoCs with the
default and legacy versions of the function
-> msm8916/msm8996/sdm630 .link_entites = camss_link_entities_ispif()
-> sdm845/sc7280/sc8280xp .link_entities = camss_link_entities()
---
bod
Powered by blists - more mailing lists