Re: [PATCH 06/12] drm/xe/vrsr: Initialize VRSR feature

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Apr 01, 2025 at 09:02:19PM +0530, Anshuman Gupta wrote:
> From: Badal Nilawar <badal.nilawar@intel.com>
> 
> Initialize VRSR feature by requesting Auxilary Power and PERST# assertion
> delay. Include an API to enable VRSR feature.

s/Auxilary/Auxiliary/

I would include the name of the API directly.

> +#define     PCODE_D3_VRSR_PERST_SHIFT	16

PCODE_D3_VRSR_PERST_SHIFT is not used by this series; maybe omit it?

> +#define	    POWER_D3_VRSR_PSERST_MASK	REG_GENMASK(31, 16)

s/PSERST/PERST/ (looks like a typo?)

> +static void xe_pm_vrsr_init(struct xe_device *xe)
> +{
> +	int ret;
> +
> +	/* Check if platform support d3cold vrsr */

Nicer to consistently capitalize as "VRSR" in comments, commit
logs, and messages.

Similar with "D3cold" ("D3cold" is used ~100 times in the tree,
"D3Cold" ~20, mostly in xe).

> +	if (!xe->info.has_vrsr)
> +		return;
> +
> +	if (!xe_pm_vrsr_capable(xe))
> +		return;
> +
> +	/*
> +	 * If the VRSR initialization fails, the device will proceed with the regular
> +	 * D3 Cold flow
> +	 */
> +	ret = pci_acpi_aux_power_setup(xe);
> +	if (ret) {
> +		drm_info(&xe->drm, "VRSR capable %s\n", "No");

Kinda weird to use %s when the text is a known constant.

> +		return;
> +	}
> +
> +	xe->d3cold.vrsr_capable = true;
> +	drm_info(&xe->drm, "VRSR capable %s\n", "Yes");

Same.

> +}