Claude review: drm/bridge: cdns-mhdp8546: Add suspend resume support to the bridge driver

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

Patch Review

**Critical: Missing notifier removal in remove path**

The notifier is registered in probe via `dev_pm_genpd_add_notifier()`, but `cdns_mhdp_remove()` never calls `dev_pm_genpd_remove_notifier()`. This will leave a dangling notifier pointing at freed memory after the driver is removed.

**Critical: Clock management conflicts with `devm_clk_get_enabled()`**

The probe function at line 2275 of the current tree uses `devm_clk_get_enabled()`, which means the clock is managed by devres and will be automatically disabled/unprepared at device removal. But `cdns_mhdp_suspend()` calls `clk_disable_unprepare(mhdp->clk)` and `cdns_mhdp_resume()` calls `clk_prepare_enable(mhdp->clk)`. This creates a conflict: on driver removal after a resume, devres will try to disable an already-managed clock, and the enable/disable count will be unbalanced. The driver should either switch to `devm_clk_get()` (not the `_enabled` variant) and manage clock enable/disable explicitly, or find another approach for suspend/resume clock handling.

**Bug: `hw_state` not set to `MHDP_HW_STOPPED` on resume error paths**

In `cdns_mhdp_resume()`, if `cdns_mhdp_load_firmware()` or `wait_event_timeout` or `cdns_mhdp_set_firmware_active()` fails, the function returns an error but `mhdp->hw_state` is never updated. The suspend function sets `hw_state = MHDP_HW_STOPPED`, and it stays that way on resume failure, but the comment block in the header file (line 317) says:
```
MHDP_HW_READY <-> MHDP_HW_STOPPED
```
If resume partially fails (e.g. firmware loaded but timeout waiting for ready), the state machine is left in an inconsistent position. At minimum, on resume failure, `hw_state` should be explicitly set to `MHDP_HW_STOPPED` so a subsequent suspend/resume cycle doesn't malfunction.

**Bug: `wait_event_timeout` return value mishandled in suspend**

```c
ret = wait_event_timeout(mhdp->fw_load_wq,
			 mhdp->hw_state == MHDP_HW_READY,
			 timeout);

spin_lock(&mhdp->start_lock);
if (mhdp->hw_state != MHDP_HW_READY) {
```

`wait_event_timeout` returns 0 on timeout and >0 on success. Here `ret` is set to the timeout return value (positive on success), but then later `ret` is overwritten by `cdns_mhdp_set_firmware_active()`. However, if the wait succeeds but `hw_state == MHDP_HW_READY`, then the code proceeds and `ret` is still the positive jiffies remaining value. Although it gets overwritten by `cdns_mhdp_set_firmware_active()` which returns 0 on success, it would be cleaner to reset `ret = 0` after the wait succeeds, as the existing `int ret = 0` initialization is dead. Same issue exists in `cdns_mhdp_resume()`:

```c
ret = wait_event_timeout(mhdp->fw_load_wq,
			mhdp->hw_state == MHDP_HW_READY,
			msecs_to_jiffies(1000));
if (ret == 0) {
	...
	ret = -ETIMEDOUT;
	goto phy_off;
}
```

If `wait_event_timeout` returns a positive value (success), `ret` is now some positive jiffies count but the success path does `mhdp->powered_off = false; return 0;` — so this particular case is fine. But it is fragile and inconsistent. In the suspend case, if the success path after `cdns_mhdp_set_firmware_active` had any error checks using `ret`, the stale positive value could leak through.

**Issue: `cancel_work_sync(&mhdp->hpd_work)` in suspend but not `modeset_retry_work`**

The suspend function cancels `hpd_work` but not `modeset_retry_work`. The remove function cancels both (`cancel_work_sync(&mhdp->modeset_retry_work)` and `flush_work(&mhdp->hpd_work)`). If a modeset retry is pending during suspend, it could fire after the firmware is deactivated and the PHY is powered off, causing failures.

**Issue: No bridge state management during suspend/resume**

The suspend/resume doesn't interact with the DRM bridge state at all. If the display is active when suspend happens, resume will reload/activate firmware but won't restore the display mode, link training, or any active stream state. While this might be handled at a higher level by the DRM framework's atomic resume, it's worth considering whether `cdns_mhdp_bridge_hpd_enable()` should be called in resume (as `cdns_mhdp_fw_activate()` does at line 701) to re-enable HPD interrupt handling.

**Minor: `DEFINE_SIMPLE_DEV_PM_OPS` placement**

The `DEFINE_SIMPLE_DEV_PM_OPS` macro is placed between the suspend/resume functions and `cdns_mhdp_probe()`. This is fine functionally, but conventionally PM ops definitions are placed closer to the driver struct.

**Minor: Missing `NOTIFY_DONE` return in notifier**

```c
static int mhdp_pd_notifier_cb(struct notifier_block *nb,
			unsigned long action, void *data)
{
	struct cdns_mhdp_device *mhdp = container_of(nb, struct cdns_mhdp_device, pd_nb);

	if (action == GENPD_NOTIFY_OFF)
		mhdp->powered_off = true;

	return 0;
}
```

Returning `0` is equivalent to `NOTIFY_DONE`, so it works, but using the symbolic constant `NOTIFY_DONE` is conventional and clearer.

**Minor: Race on `powered_off` flag**

`mhdp->powered_off` is a plain `bool` accessed from the genpd notifier callback (which may run in arbitrary context) and from the suspend/resume paths. There is no locking around it. While it's likely benign since the writes are not concurrent in practice (the notifier fires during the power-off sequence, and resume reads it after), it would be more robust to use `start_lock` or at minimum make it atomic.

---
Generated by Claude Code Patch Reviewer