From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: phy: qualcomm: add MSM8974 HDMI PHY support
Date: Sat, 16 May 2026 11:32:10 +1000
Message-ID: <review-patch5-20260513-fd-hdmi-phy-v9-5-ca98c72f1f9f@oss.qualcomm.com>
In-Reply-To: <20260513-fd-hdmi-phy-v9-5-ca98c72f1f9f@oss.qualcomm.com>
References: <20260513-fd-hdmi-phy-v9-0-ca98c72f1f9f@oss.qualcomm.com> <20260513-fd-hdmi-phy-v9-5-ca98c72f1f9f@oss.qualcomm.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

Adds PLL programming and power sequencing for the MSM8974 HDMI PHY, building on the UNI PLL framework from patches 3-4. This is a new platform enablement that was previously not supported upstream.

**`qcom_uniphy_setup()` vs `qcom_28lpm_set_rate()`**: These two functions compute the same PLL parameters using slightly different approaches. The 28lpm version inlines the math while the uniphy version takes pre-computed flags. This is noted in patch 2's comment: "This function is close to UNIPHY, but it has slightly different fields". The difference is acceptable given the register-level differences between the two PHY variants.

**Calibration register difference**: In the 28lpm driver (patch 2):
```c
write16(vco_freq / 1000, hdmi_phy->pll_reg + REG_HDMI_8960_PHY_PLL_VCOCAL_CFG0);
```
vs in the 8974 uniphy setup (patch 5):
```c
write16(vco_freq / 16, base + UNIPHY_PLL_CAL_CFG10);
```
These write to different calibration registers with different scaling factors, which is expected since they're different PHY generations, but the `/16` for the uniphy setup seems low-level and worth verifying against the hardware docs -- for a VCO frequency of ~1.5 GHz, `vco_freq / 16` would be ~93.75 MHz, which fits in a 16-bit register. For the 28lpm, `vco_freq / 1000` for 1.5 GHz would be 1,500,000, which also fits in a 16-bit write (u16 max is 65535... actually it doesn't: 1,500,000 > 65535). **This looks like a potential bug**: `write16()` takes a `u16`, but `vco_freq / 1000` for typical VCO frequencies will exceed 16 bits. For example, with a VCO of 750 MHz (the minimum), `vco_freq / 1000 = 750000`, which truncates to `u16`. The old code's VCOCAL_CFG0 values were always <= 0xff (single byte), suggesting the register expects a much smaller value. This should be verified.

**Pixel clock magic numbers**: The power-on sequence has hardcoded comparisons:
```c
if (pixclk == 297000000) {
    ...
} else if (pixclk == 268500000) {
    ...
}
```
These are standard HDMI pixel clocks (4K@30Hz and 2560x1440@60Hz respectively), so the magic numbers are acceptable in this context, though named constants would be slightly cleaner.

**Duplicated helpers**: `write16()`, `write24()`, `read24()` are duplicated again here (also in the base patch's QMP files). This is now the 4th copy. A shared utility header would reduce maintenance burden.

**Acked-by**: Has `Acked-by: Konrad Dybcio`.

---
Generated by Claude Code Patch Reviewer