From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7EF7140E8EC for ; Fri, 5 Jun 2026 13:30:35 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780666236; cv=none; b=qkeB+DtM2ctYE6StFniMMHkeD7jE33j2ZFNFJxtedwffx0+rhGkPEfoMIXPk9MLjUXRBtj65y0QBsfDB6tPs/A7Zu7uf3n6EQam6GcRwOvFa/hL1f204VZ4vVabW0l6Phj2ESsUTGXa/FNAyl/jXLtOEIOwtaelvZLh2q9ixjTw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780666236; c=relaxed/simple; bh=ujGokoz1aOpKIyYOCgRd4NH4I1kHRbo92XVd2t6X0Fw=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=aUytBbbJf+5yZ6buZBW1iXGINsArXF+ODMlVPo2JlelkRF2q6BpQcSatAsilTZ+tAvn4OBD8Er6KiwbVLBPeY0zTYMZjzZEw31QtANRDcudcpDENOBFl38v1lzpgu5xV2Qr5PEkcFUTYzAgZcw9C5dYFEmk2c9swu8+rm+iAYAo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=ZHKcXO3G; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="ZHKcXO3G" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 1273F1F00893; Fri, 5 Jun 2026 13:30:35 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1780666235; bh=Lue4TA2r/hBzzyGhrJWVUxEsy0tYtPVoVgjK8cBr0Cc=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=ZHKcXO3GrjvSsFuEjw4/LKTjaDd/hYBJhXG+eHJDL6YyQkSubPL9xDgKspFnxvsN7 Q+MNPzK3v39ErX+NKMoTbnf2tbwRq53NV6Z80E85tA6RShRD1elxQ7cX0F8ItsN1tM p55AMsKHfYKoO3zM+XntmMCzws47dTtU9D0JGqzZON2EV2NF/96fYvjtsZrmjnnPxf lzycM11xVCb5PYrEbZrkTHHGgQhkPRNEmvdBr/Wcplw87alfbSxPK91UFtI0dtwXIj ul5uLFqMh2jqDGUHTSrDXg1RyBdMeVXMRTwuWKTfQk+tDIzPRy3doCm/ciMDy/k2lN 268kBN1+rwjRw== From: sashiko-bot@kernel.org Subject: Re: [PATCH v7 2/8] media: qcom: camss: csiphy-3ph: Use odd bits for configuring C-PHY lanes Reply-To: sashiko-reviews@lists.linux.dev To: "David Heidelberg via B4 Relay" Cc: linux-media@vger.kernel.org In-Reply-To: <20260605-qcom-cphy-v7-2-426c37e9008f@ixit.cz> References: <20260605-qcom-cphy-v7-2-426c37e9008f@ixit.cz> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Fri, 05 Jun 2026 13:30:34 +0000 Message-Id: <20260605133035.1273F1F00893@smtp.kernel.org> Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Thank you for your contribution! Sashiko AI review found 1 potential issue(= s) to consider: - [Medium] Commit message claims to return failure but code does not implem= ent error propagation. -- commit da5484f83dd0e6203bd1f47a4299fe8d0bb8c433 Author: David Heidelberg media: qcom: camss: csiphy-3ph: Use odd bits for configuring C-PHY lanes This commit adds support for C-PHY lane configuration by utilizing odd bits instead of the even bits used for D-PHY mode. It updates the lane mask calculation and enablement routines for C-PHY hardware requirements. > Since there can be unrecognized configuration allow returning failure. [Severity: Medium] The commit message explicitly states "allow returning failure" for unrecognized configurations, but does the code actually implement this? Looking at csiphy_get_lane_mask() and csiphy_lanes_enable(), the code does not appear to propagate any errors. Could the commit message be updated to reflect the use of warnings, or should the functions be changed to return an error code? > diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/d= rivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c > index dac8d2ecf7995..fa24fc9706748 100644 > --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c > +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c [ ... ] > @@ -1113,13 +1114,22 @@ static void csiphy_gen2_config_lanes(struct csiph= y_device *csiphy, > =20 > static u8 csiphy_get_lane_mask(struct csiphy_lanes_cfg *lane_cfg) > { > - u8 lane_mask; > - int i; > + u8 lane_mask =3D 0; > + u8 offset =3D 0; > =20 > - lane_mask =3D CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE; > + switch (lane_cfg->phy_cfg) { > + case V4L2_MBUS_CSI2_CPHY: > + offset =3D 1; > + break; > + case V4L2_MBUS_CSI2_DPHY: > + lane_mask =3D CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE; > + break; > + default: > + break; > + } [Severity: Medium] In csiphy_get_lane_mask(), the default case silently falls through and returns 0. Is this intended to be treated as a failure condition? [ ... ] > @@ -1160,9 +1170,22 @@ static void csiphy_lanes_enable(struct csiphy_devi= ce *csiphy, [ ... ] > - val =3D CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE; > - for (i =3D 0; i < c->num_data; i++) > - val |=3D BIT(c->data[i].pos * 2); > + val =3D 0; > + > + switch (c->phy_cfg) { > + case V4L2_MBUS_CSI2_CPHY: > + for (i =3D 0; i < c->num_data; i++) > + val |=3D BIT((c->data[i].pos * 2) + 1); > + break; > + case V4L2_MBUS_CSI2_DPHY: > + val =3D CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE; > + > + for (i =3D 0; i < c->num_data; i++) > + val |=3D BIT(c->data[i].pos * 2); > + break; > + default: > + WARN_ONCE(1, "Unsupported bus type %d!\n", c->phy_cfg); > + } [Severity: Medium] Similarly, since csiphy_lanes_enable() has a void return type, the caller has no way to detect this failure. While misconfiguration might be prevented earlier during device tree parsing, does this function need to be updated to actually return the failure mentioned in the commit message? --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260605-qcom-cphy-= v7-0-426c37e9008f@ixit.cz?part=3D2