From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: phy: qualcomm: add MSM8974 HDMI PHY support Date: Mon, 16 Mar 2026 12:08:00 +1000 Message-ID: In-Reply-To: <20260314-fd-hdmi-phy-v5-4-58122ae96d3b@oss.qualcomm.com> References: <20260314-fd-hdmi-phy-v5-0-58122ae96d3b@oss.qualcomm.com> <20260314-fd-hdmi-phy-v5-4-58122ae96d3b@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review Adds PLL programming and rate calculation for the MSM8974 HDMI PHY, buildin= g on the infrastructure from patch 1 and the UNI PLL registers from patches= 2-3. **Comparison bug in `qcom_hdmi_msm8974_phy_pll_set_rate`:** ```c + if (pixclk =3D=3D 297000) { + hdmi_phy_write(hdmi_phy, REG_HDMI_8x74_ANA_CFG2, 0x06); + hdmi_phy_write(hdmi_phy, REG_HDMI_8x74_ANA_CFG3, 0x03); + } else if (pixclk =3D=3D 268500) { ``` `pixclk` is `hdmi_phy->hdmi_opts.tmds_char_rate` which is in Hz (e.g., 2970= 00000), but the comparisons use kHz values (297000, 268500). These comparis= ons will never match. This should be either `297000000` / `268500000`, or `= pixclk` should be divided by 1000 first. This is a functional bug. **`qcom_uniphy_recalc` =E2=80=94 refclk_div calculation:** ```c + rate *=3D (refclk_cfg >> 2) * 0x3 + 1; ``` The mask `0x3` applied after shifting right by 2 extracts bits [3:2] of ref= clk_cfg. But looking at the setup code: ```c + val =3D (ref_freq_mult_2 ? BIT(0) : 0) | + ((refclk_div - 1) << 2); ``` The divider is stored in bits [3:2] as `(refclk_div - 1)`. The recalc multi= plies by `(field & 0x3) + 1` which correctly recovers `refclk_div`. However= , `0x3` should probably be written as a proper mask. The expression `* 0x3`= specifically reads oddly =E2=80=94 it looks like it's multiplying by 3 rat= her than ANDing. It's actually `& 0x3` implicitly due to being the full 2-b= it field. Wait, re-reading: `(refclk_cfg >> 2) * 0x3 + 1` =E2=80=94 this is= `(field_value * 3) + 1`, not `(field_value & 3) + 1`. For `refclk_div=3D1`= , stored as 0, this gives `0*3+1 =3D 1` (correct). For other values this se= ems wrong. This looks like it should be `((refclk_cfg >> 2) & 0x3) + 1`. Th= is is another potential bug. **Missing `Reviewed-by` for patch 4:** Unlike patches 2 and 3 which have Ne= il Armstrong's Reviewed-by, this patch does not. Being the most complex new= patch, it would benefit from additional review. Overall this is a well-executed series with a few bugs that need fixing bef= ore merging, particularly the missing compatibles (8084, 8998), the pixclk = unit mismatch in patch 4, and the suspicious recalc formula. --- Generated by Claude Code Patch Reviewer