From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: firmware: qcom_scm: Migrate to generic PAS service
Date: Mon, 25 May 2026 18:55:29 +1000
Message-ID: <review-patch3-20260522115936.201208-4-sumit.garg@kernel.org>
In-Reply-To: <20260522115936.201208-4-sumit.garg@kernel.org>
References: <20260522115936.201208-1-sumit.garg@kernel.org> <20260522115936.201208-4-sumit.garg@kernel.org>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

This patch refactors SCM PAS functions into internal `__qcom_scm_pas_*` / `qcom_scm_pas_*` versions plus exported wrappers, then registers them as a `qcom_pas_ops` backend.

**Transitional type casting**: Throughout this patch, there are casts like `(struct qcom_pas_context *)ctx` and `(struct qcom_scm_pas_context *)ctx`. This relies on `qcom_scm_pas_context` and `qcom_pas_context` being layout-compatible. Looking at the header definitions, they are in fact identical structs. However, this is fragile -- if either struct changes independently, these casts would silently break. This is mitigated by patch 14 which removes the old type entirely. Since this is transitional code that exists only between patches 3 and 14, this is acceptable.

**`qcom_scm_is_pas_available()` check**:
```c
static bool qcom_scm_is_pas_available(void)
{
	if (!__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_PIL,
					  QCOM_SCM_PIL_PAS_AUTH_AND_RESET))
		return false;
	return true;
}
```
Could be simplified to `return __qcom_scm_is_call_available(...)` but this is a style nit.

**Registration in probe, unregistration in shutdown (not remove)**: `qcom_pas_ops_register` is called in `qcom_scm_probe()` but `qcom_pas_ops_unregister()` is called in `qcom_scm_shutdown()`, not in a `remove` callback. This means if the SCM platform device is unbound (e.g., via sysfs), the ops won't be unregistered. The SCM driver has no `remove` callback at all, so this is consistent with existing behavior, but worth noting.

---
Generated by Claude Code Patch Reviewer