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 [thread overview]
Message-ID: <review-patch5-20260513-fd-hdmi-phy-v9-5-ca98c72f1f9f@oss.qualcomm.com> (raw)
In-Reply-To: <20260513-fd-hdmi-phy-v9-5-ca98c72f1f9f@oss.qualcomm.com>
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
next prev parent reply other threads:[~2026-05-16 1:32 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-13 18:14 [PATCH v9 0/5] drm/msm/hdmi & phy: use generic PHY framework Dmitry Baryshkov
2026-05-13 18:14 ` [PATCH v9 1/5] drm/msm/hdmi: switch to generic PHY subsystem Dmitry Baryshkov
2026-05-16 1:32 ` Claude review: " Claude Code Review Bot
2026-05-13 18:14 ` [PATCH v9 2/5] phy: qualcomm: hdmi-28lpm: provide dynamic configuration Dmitry Baryshkov
2026-05-16 1:32 ` Claude review: " Claude Code Review Bot
2026-05-13 18:14 ` [PATCH v9 3/5] phy: qcom: apq8064-sata: extract UNI PLL register defines Dmitry Baryshkov
2026-05-16 1:32 ` Claude review: " Claude Code Review Bot
2026-05-13 18:14 ` [PATCH v9 4/5] phy: qcom-uniphy: add more registers from display PHYs Dmitry Baryshkov
2026-05-16 1:32 ` Claude review: " Claude Code Review Bot
2026-05-13 18:14 ` [PATCH v9 5/5] phy: qualcomm: add MSM8974 HDMI PHY support Dmitry Baryshkov
2026-05-16 1:32 ` Claude Code Review Bot [this message]
2026-05-16 1:32 ` Claude review: drm/msm/hdmi & phy: use generic PHY framework Claude Code Review Bot
-- strict thread matches above, loose matches on Subject: below --
2026-03-23 22:56 [PATCH v7 0/4] " Dmitry Baryshkov
2026-03-23 22:56 ` [PATCH v7 4/4] phy: qualcomm: add MSM8974 HDMI PHY support Dmitry Baryshkov
2026-03-24 21:14 ` Claude review: " Claude Code Review Bot
2026-03-19 3:48 [PATCH v6 0/4] drm/msm/hdmi & phy: use generic PHY framework Dmitry Baryshkov
2026-03-19 3:48 ` [PATCH v6 4/4] phy: qualcomm: add MSM8974 HDMI PHY support Dmitry Baryshkov
2026-03-21 18:56 ` Claude review: " Claude Code Review Bot
2026-03-14 5:06 [PATCH v5 0/4] drm/msm/hdmi & phy: use generic PHY framework Dmitry Baryshkov
2026-03-14 5:06 ` [PATCH v5 4/4] phy: qualcomm: add MSM8974 HDMI PHY support Dmitry Baryshkov
2026-03-16 2:08 ` Claude review: " Claude Code Review Bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=review-patch5-20260513-fd-hdmi-phy-v9-5-ca98c72f1f9f@oss.qualcomm.com \
--to=claude-review@example.com \
--cc=dri-devel-reviews@example.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox