From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/msm/hdmi: switch to generic PHY subsystem Date: Mon, 16 Mar 2026 12:07:58 +1000 Message-ID: In-Reply-To: <20260314-fd-hdmi-phy-v5-1-58122ae96d3b@oss.qualcomm.com> References: <20260314-fd-hdmi-phy-v5-0-58122ae96d3b@oss.qualcomm.com> <20260314-fd-hdmi-phy-v5-1-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 This is the main patch (~5800 lines changed). It removes the ad-hoc HDMI PH= Y implementation from DRM/MSM and creates two new PHY drivers under `driver= s/phy/qualcomm/`. **DRM side changes (good):** The `hdmi.c` changes cleanly replace the custom `msm_hdmi_get_phy()`/`msm_h= dmi_put_phy()` with `devm_phy_get()`: ```c - ret =3D msm_hdmi_get_phy(hdmi); - if (ret) { + hdmi->phy =3D devm_phy_get(&pdev->dev, NULL); + if (IS_ERR(hdmi->phy)) { ``` The `hdmi.h` change from `struct hdmi_phy *phy` to `struct phy *phy` is cle= an. **Bridge changes =E2=80=94 potential issue with `pm_runtime_get_sync` in at= omic context:** In `hdmi_bridge.c`, the cover letter mentions replacing `pm_runtime_resume_= and_get()` with `pm_runtime_get_sync()` because `atomic_pre_enable()` canno= t fail. However, the code actually uses `pm_runtime_get_sync()` without che= cking its return value: ```c if (!hdmi->power_on) { phy_init(hdmi->phy); pm_runtime_get_sync(&hdmi->pdev->dev); ``` If `pm_runtime_get_sync()` fails, the reference count is still incremented = but the device may not be powered. The old code had the same issue with `pm= _runtime_resume_and_get()`. Since this is in `atomic_pre_enable`, failure h= andling is limited, but at minimum a `WARN_ON` on failure would be helpful.= Also `phy_init()` can fail but the return value is not checked here. **extp_clk handling =E2=80=94 good refactoring:** The extp_clk enable/disable is moved to a single place (after PHY power on,= before PHY power off) per Neil's feedback: ```c + ret =3D phy_power_on(hdmi->phy); + if (WARN_ON(ret)) + return; + + if (hdmi->extp_clk) { + ret =3D clk_set_rate(hdmi->extp_clk, hdmi->pixclock); ``` **Missing compatible in preqmp driver:** The old `hdmi_phy.c` had a compatible for `"qcom,hdmi-phy-8084"` mapping to= `msm_hdmi_phy_8x74_cfg`. The new `phy-qcom-hdmi-preqmp.c` only has: ```c +static const struct of_device_id qcom_hdmi_preqmp_of_match_table[] =3D { + { .compatible =3D "qcom,hdmi-phy-8660", .data =3D &msm8x60_hdmi_phy_cfg, = }, + { .compatible =3D "qcom,hdmi-phy-8960", .data =3D &msm8960_hdmi_phy_cfg, = }, + { .compatible =3D "qcom,hdmi-phy-8974", .data =3D &msm8974_hdmi_phy_cfg, = }, + { }, ``` The `"qcom,hdmi-phy-8084"` compatible is missing. If APQ8084 DT bindings us= e this compatible, it will break. This needs to be added (mapping to `msm89= 74_hdmi_phy_cfg`, same as before). **QMP base driver =E2=80=94 missing MSM8998 compatible:** Similarly, `phy-qcom-qmp-hdmi-base.c` only lists 8996: ```c +static const struct of_device_id qmp_hdmi_of_match_table[] =3D { + { + .compatible =3D "qcom,hdmi-phy-8996", .data =3D &qmp_hdmi_8996_cfg, + }, + { }, ``` But `qmp_hdmi_8998_cfg` is defined and exported. The `"qcom,hdmi-phy-8998"`= compatible from the old driver is missing here. This is a regression =E2= =80=94 MSM8998 HDMI PHY will not probe. **`pm_runtime_put_noidle` in phy_exit:** Both `qcom_hdmi_preqmp_phy_exit()` and `qmp_hdmi_phy_exit()` use `pm_runtim= e_put_noidle()`: ```c +static int qcom_hdmi_preqmp_phy_exit(struct phy *phy) +{ + struct qcom_hdmi_preqmp_phy *hdmi_phy =3D phy_get_drvdata(phy); + + pm_runtime_put_noidle(hdmi_phy->dev); ``` The `_noidle` variant doesn't trigger suspend. If `phy_init()` calls `pm_ru= ntime_resume_and_get()`, using `pm_runtime_put_noidle()` in exit means the = device won't actually be suspended via runtime PM. This seems intentional (= the DRM side manages its own `pm_runtime_put`), but it's worth verifying th= at the PM reference counting is balanced. **Probe-time pm_runtime:** Both probe functions do `pm_runtime_resume_and_get()` to register the clock= provider, then `pm_runtime_put_noidle()`: ```c + ret =3D pm_runtime_resume_and_get(&pdev->dev); + ... + pm_runtime_put_noidle(&pdev->dev); + return PTR_ERR_OR_ZERO(phy_provider); ``` Using `pm_runtime_put_noidle` instead of `pm_runtime_put` means the device = stays active after probe. This might be intentional for clock readback, but= `pm_runtime_put_sync` would be more conventional here. **`phy-qcom-hdmi-28lpm.c` =E2=80=94 uninitialized variable use:** ```c + unsigned int div; + ... + dev_dbg(hdmi_phy->dev, "rate=3D%u, div =3D %d, vco =3D %lu", pixclk, div,= vco_freq); ``` `div` is used in the `dev_dbg` before being assigned. It's only assigned la= ter via `div =3D qcom_hdmi_8960_divs[div_idx]` in the caller. Minor issue s= ince it's just debug output, but should be fixed. **`phy-qcom-hdmi-28lpm.c` =E2=80=94 potential bug in pll_disable:** ```c +static void qcom_hdmi_msm8960_phy_pll_disable(struct qcom_hdmi_preqmp_phy = *phy) +{ + ... + val =3D hdmi_pll_read(phy, REG_HDMI_8960_PHY_PLL_PWRDN_B); + val |=3D HDMI_8960_PHY_REG12_SW_RESET; + val &=3D ~HDMI_8960_PHY_REG12_PWRDN_B; ``` This is reading a PLL register but applying PHY register bitmasks (`HDMI_89= 60_PHY_REG12_SW_RESET` =3D 0x20, `HDMI_8960_PHY_REG12_PWRDN_B` =3D 0x80). T= hese constants were defined for a different register (PHY_REG12), but they = happen to have the right bit values for the PLL_PWRDN_B register based on t= he downstream code. Still, using PHY register masks on a PLL register is co= nfusing and fragile. Consider defining proper PLL_PWRDN_B field masks. **`phy-qcom-hdmi-45nm.c` =E2=80=94 deprecated header:** ```c +#include ``` `of_device.h` is deprecated in favor of `mod_devicetable.h` + `property.h`.= Minor style nit. **Kconfig =E2=80=94 typo:** ``` + Enable this to support the Qualcomm HDMI PHY presend on 32-bit platform= s: ``` "presend" should be "present". --- Generated by Claude Code Patch Reviewer