From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CC38BCD484A for ; Wed, 4 Sep 2024 14:51:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=dcjFuQ1QEBKXphtaS5nfelxe1o3VzP7a8HZPAOwOmrc=; b=rj+S21OvXRS/ewdXkrxl/6Xq+p Z2n1gj69LABlnO5GwWbNlH12LF6l70XSwshM5FrogLtA4WmRbS0wwbbGeXIYim3WLEOHuxOWwoDxl tRZ58Jr1NM674dM9iKeBbwoXh6wlGSmisYnJsug124/hq3yPMfEWV20XQt2qyYUsYWMMy6J0MqAwC kfIk27bL0wdZvjs9PDbYp7ixmXTrJsMBN8cd8z7JUfEDEEFsVdU3Uq8Q5Kje6hNVXOihE/oH1yPfj aRnLjs9ex7AfiWxr8804+wzGJ6RfNfsiDUZeewtxtDQbxxLRnDvQEesCvRJHI3pL2Q0QYHeT2/DV1 K6GEHH7w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1slrLh-00000004rlr-1xYp; Wed, 04 Sep 2024 14:51:09 +0000 Received: from mail-ej1-x634.google.com ([2a00:1450:4864:20::634]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1slrKj-00000004rVZ-0ABe for linux-arm-kernel@lists.infradead.org; Wed, 04 Sep 2024 14:50:10 +0000 Received: by mail-ej1-x634.google.com with SMTP id a640c23a62f3a-a86e5e9ff05so780711966b.1 for ; Wed, 04 Sep 2024 07:50:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1725461407; x=1726066207; darn=lists.infradead.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=dcjFuQ1QEBKXphtaS5nfelxe1o3VzP7a8HZPAOwOmrc=; b=UwaIH+UgOIkDZpZmoZh+UzPJZ6GB9JLs4KwdghW/H+Bvs7CTD6/QtJWUXcD8dHlQdQ VUP9OruVQdxlTN2BgoaMe5+hRR42Fn+oHuyjXd2F+M4q5vqaZEqILxKtTYmfgwVVAEJ+ qO1iaSWYQh21gPIOsl0qO1d1AyIyVZgrrnQLHeZgWRS6r3v4EcRFtsm93gR38f1feABu Yk6h4YA+hZFHPQoAOOnlrcvupy3e1sn0OjwQZzeE7nHh0p6okhDYZt1LbSgF6SUHCQJh Tj0URG03WcjNku1iyURrfgPjMNkX9Pk5PRFEeKxjYJJC6o/RW1P/L89fY5dpXuNPJRh6 /rpw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1725461407; x=1726066207; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=dcjFuQ1QEBKXphtaS5nfelxe1o3VzP7a8HZPAOwOmrc=; b=gcjoar80vGBGo9h8QhWeFXc1zf41iyuFZhElcxN2HWCx0iXxQ7LWAUCguKM1vDisdu g6VGHawLGyBgi+DMoJKYvta6Sg464/2fNn4CUdIia3z9HTSOxLBuoBfFVH/5daQCtvlG 5t02hdO1EzU7pNYyzsYIBNEBtQYXMZ5YRY1PVPO2sNRFfh8FYSJbW6e7wl8Qt61VqQdr byyuPoh3d0tuPhEBcYRIugtJGpZO+t9zRoPwleBBWmW1/fZEJ24LiwbmwLRFKNuTH8Xu YwUwL3tPcN+bOh4MBhcw0XQ2OsL4ua4od5/lTmJJSldynC3EUwtz+d2IPO6cNgB5kKr2 kgkw== X-Forwarded-Encrypted: i=1; AJvYcCWdMiqB8Tqs3Kc1TBU9kQqhCTNJ2SBmDhcUChAjYuNMgfxfD1CGP6H4Wmn0odDYl28L8EhRuqhsvoyK3a4sV/ce@lists.infradead.org X-Gm-Message-State: AOJu0Yz4Ne3Ic/YwT2RUEmLpDzN94H9WLyg16RWU30OPSOKFdHrZutZc ZkELwRfi+VWUqmhWqImjqS0s1SEzkpE1amPLbHEtiCtrCrv6cQ+qp0p0QoKfhUQ= X-Google-Smtp-Source: AGHT+IFD6e2sBTQ7YLZkoGg9Y75jHEQCKVUG2WlUqQ66ORREB05uClSZs2uAJIbigWpd0npPHOHAxw== X-Received: by 2002:a17:907:3ea2:b0:a86:672d:8436 with SMTP id a640c23a62f3a-a8a32fe15e6mr335714766b.59.1725461407337; Wed, 04 Sep 2024 07:50:07 -0700 (PDT) Received: from [192.168.0.25] ([176.61.106.227]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a8a62048f5fsm1853566b.88.2024.09.04.07.50.06 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 04 Sep 2024 07:50:06 -0700 (PDT) Message-ID: Date: Wed, 4 Sep 2024 15:50:05 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 06/10] media: qcom: camss: Add camss_link_entities_v2 To: Vikram Sharma , Robert Foss , Todor Tomov , Bryan O'Donoghue , Mauro Carvalho Chehab , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Kapatrala Syed , Hariram Purushothaman , Bjorn Andersson , Konrad Dybcio , Hans Verkuil , cros-qcom-dts-watchers@chromium.org, Catalin Marinas , Will Deacon Cc: linux-arm-msm@vger.kernel.org, linux-media@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Suresh Vankadara , Trishansh Bhardwaj References: <20240904-camss_on_sc7280_rb3gen2_vision_v2_patches-v1-0-b18ddcd7d9df@quicinc.com> <20240904-camss_on_sc7280_rb3gen2_vision_v2_patches-v1-6-b18ddcd7d9df@quicinc.com> Content-Language: en-US From: Bryan O'Donoghue In-Reply-To: <20240904-camss_on_sc7280_rb3gen2_vision_v2_patches-v1-6-b18ddcd7d9df@quicinc.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240904_075009_126504_0C379FFE X-CRM114-Status: GOOD ( 28.36 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > Signed-off-by: Trishansh Bhardwaj > Signed-off-by: Vikram Sharma > --- > 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