Claude review: drm/msm/hdmi: switch to generic PHY subsystem

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

This is the main patch (~5800 lines changed). It removes the ad-hoc HDMI PHY implementation from DRM/MSM and creates two new PHY drivers under `drivers/phy/qualcomm/`.

**DRM side changes (good):**

The `hdmi.c` changes cleanly replace the custom `msm_hdmi_get_phy()`/`msm_hdmi_put_phy()` with `devm_phy_get()`:

```c
-	ret = msm_hdmi_get_phy(hdmi);
-	if (ret) {
+	hdmi->phy = 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 clean.

**Bridge changes — potential issue with `pm_runtime_get_sync` in atomic context:**

In `hdmi_bridge.c`, the cover letter mentions replacing `pm_runtime_resume_and_get()` with `pm_runtime_get_sync()` because `atomic_pre_enable()` cannot fail. However, the code actually uses `pm_runtime_get_sync()` without checking 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 handling 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 — 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 = phy_power_on(hdmi->phy);
+	if (WARN_ON(ret))
+		return;
+
+	if (hdmi->extp_clk) {
+		ret = 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[] = {
+	{ .compatible = "qcom,hdmi-phy-8660", .data = &msm8x60_hdmi_phy_cfg, },
+	{ .compatible = "qcom,hdmi-phy-8960", .data = &msm8960_hdmi_phy_cfg, },
+	{ .compatible = "qcom,hdmi-phy-8974", .data = &msm8974_hdmi_phy_cfg, },
+	{ },
```

The `"qcom,hdmi-phy-8084"` compatible is missing. If APQ8084 DT bindings use this compatible, it will break. This needs to be added (mapping to `msm8974_hdmi_phy_cfg`, same as before).

**QMP base driver — 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[] = {
+	{
+		.compatible = "qcom,hdmi-phy-8996", .data = &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 — 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_runtime_put_noidle()`:

```c
+static int qcom_hdmi_preqmp_phy_exit(struct phy *phy)
+{
+	struct qcom_hdmi_preqmp_phy *hdmi_phy = phy_get_drvdata(phy);
+
+	pm_runtime_put_noidle(hdmi_phy->dev);
```

The `_noidle` variant doesn't trigger suspend. If `phy_init()` calls `pm_runtime_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 that 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 = 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` — uninitialized variable use:**

```c
+	unsigned int div;
+	...
+	dev_dbg(hdmi_phy->dev, "rate=%u, div = %d, vco = %lu", pixclk, div, vco_freq);
```

`div` is used in the `dev_dbg` before being assigned. It's only assigned later via `div = qcom_hdmi_8960_divs[div_idx]` in the caller. Minor issue since it's just debug output, but should be fixed.

**`phy-qcom-hdmi-28lpm.c` — potential bug in pll_disable:**

```c
+static void qcom_hdmi_msm8960_phy_pll_disable(struct qcom_hdmi_preqmp_phy *phy)
+{
+	...
+	val = hdmi_pll_read(phy, REG_HDMI_8960_PHY_PLL_PWRDN_B);
+	val |= HDMI_8960_PHY_REG12_SW_RESET;
+	val &= ~HDMI_8960_PHY_REG12_PWRDN_B;
```

This is reading a PLL register but applying PHY register bitmasks (`HDMI_8960_PHY_REG12_SW_RESET` = 0x20, `HDMI_8960_PHY_REG12_PWRDN_B` = 0x80). These 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 the downstream code. Still, using PHY register masks on a PLL register is confusing and fragile. Consider defining proper PLL_PWRDN_B field masks.

**`phy-qcom-hdmi-45nm.c` — deprecated header:**

```c
+#include <linux/of_device.h>
```

`of_device.h` is deprecated in favor of `mod_devicetable.h` + `property.h`. Minor style nit.

**Kconfig — typo:**

```
+	  Enable this to support the Qualcomm HDMI PHY presend on 32-bit platforms:
```

"presend" should be "present".

---
Generated by Claude Code Patch Reviewer