Claude review: drm/imagination: Improve firmware power off for layout_mars config

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

Patch Review

**Logic and correctness:**

The core logic change is correct — when `layout_mars` is true, Sidekick and SLC have been powered down by the firmware, so the kernel should not poll their idle registers. Instead, at the end, the MARS_IDLE register is polled for CPU and system arbiter (excluding SOCIF).

**Variable declaration ordering (minor style):**

```c
	u64 layout_mars_value = 0;
	bool layout_mars = false;
	bool meta_fw = pvr_dev->fw_dev.processor_type == PVR_FW_PROCESSOR_TYPE_META;
	u32 reg_value;
```

The kernel coding style prefers reverse-Christmas-tree ordering for variable declarations (longest line first). `meta_fw` is the longest declaration but appears after the shorter ones. This is a minor nit.

**Early SLC idle check addition:**

The original code had the SLC idle check only after the "Extra Idle checks" block (BIF_STATUS_MMU etc.). This patch adds an *additional* SLC idle check before the MTS DM association unset:

```c
	if (!layout_mars) {
		/* Wait for Sidekick/Jones to signal IDLE except for the Garten Wrapper. */
		err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE, sidekick_idle_mask,
					sidekick_idle_mask, POLL_TIMEOUT_USEC);
		if (err)
			return err;

		/* Wait for SLC to signal IDLE. */
		err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE, ROGUE_CR_SLC_IDLE_MASKFULL,
					 ROGUE_CR_SLC_IDLE_MASKFULL, POLL_TIMEOUT_USEC);
		if (err)
			return err;
	}
```

The original code only checked Sidekick idle before MTS teardown, and checked SLC idle *after* the extra idle checks. Now both Sidekick and SLC are checked in *both* locations for non-MARS configurations. The commit message doesn't explain why an additional early SLC idle check is needed. This should be clarified — is it a pre-existing bug fix or a new requirement? It's a semantic change that goes beyond the stated goal of skipping checks for LAYOUT_MARS.

**Alignment issue:**

```c
		err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE, ROGUE_CR_SLC_IDLE_MASKFULL,
					 ROGUE_CR_SLC_IDLE_MASKFULL, POLL_TIMEOUT_USEC);
```

There's a stray extra space before `ROGUE_CR_SLC_IDLE_MASKFULL` on the continuation line (one space too many for alignment). Compare with the second occurrence which aligns correctly with tabs only.

**Comment style (minor):**

```c
		/*
		 * As FW core has been moved from SIDEKICK to the new MARS domain, checking
		 * idle bits for CPU & System Arbiter excluding SOCIF which will never be
		 * idle if Host polling on this register
		 */
```

Missing period at end of comment. Kernel style expects properly punctuated multi-line comments.

**Garten idle condition:**

```c
	if (meta_fw || !layout_mars) {
		if (!skip_garten_idle) {
```

This is correct: for META firmware, always check the Garten wrapper idle via SIDEKICK_IDLE. For non-META, non-MARS, also check it. For non-META MARS, use the MARS_IDLE register instead. The logic is sound, but the `else` branch (MARS idle) is only reachable when `!meta_fw && layout_mars`, which is the RISC-V MARS case. A brief comment clarifying this would help readability.

**Blank line before error check:**

```c
					POLL_TIMEOUT_USEC);

		if (err)
			return err;
```

There's a blank line between the function call and the `if (err)` check in the MARS_IDLE polling block. This is inconsistent with every other poll+check pattern in the function which has no blank line.

---
Generated by Claude Code Patch Reviewer