Re: [PATCH v3 2/2] pmdomain: spacemit: Add power domain driver

From: "Troy Mitchell" <troy.mitchell@linux.spacemit.com>
To: "Yixun Lan" <dlan@kernel.org>
Cc: "Rob Herring" <robh@kernel.org>,
	"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Ulf Hansson" <ulfh@kernel.org>, "Paul Walmsley" <pjw@kernel.org>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Alexandre Ghiti" <alex@ghiti.fr>,
	"Icenowy Zheng" <uwu@icenowy.me>,
	"Iker Pedrosa" <ikerpedrosam@gmail.com>,
	<devicetree@vger.kernel.org>, <linux-riscv@lists.infradead.org>,
	<spacemit@lists.linux.dev>, <linux-pm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>,
	"Troy Mitchell" <troy.mitchell@linux.spacemit.com>
Subject: Re: [PATCH v3 2/2] pmdomain: spacemit: Add power domain driver
Date: Mon, 09 Nov 2026 07:12:57 +0000	[thread overview]
Message-ID: <DMQHSDTR177I.1HWHAF4FWTFQ5@linux.spacemit.com> (raw)
In-Reply-To: <20261108-04-k3-pm-support-v3-2-b9dc0857039b@kernel.org>

[-- Attachment #1: Type: text/plain, Size: 2023 bytes --]

Hi Yixun,

On Sun, 08 Nov 2026 10:36:02 +0000, Yixun Lan wrote:
> +config SPACEMIT_PM_DOMAINS
> +	tristate "PM Domain driver for SpacemiT SoC"
[...]
> +	  Say y to enable device power management through PM domains.
This is tristate now, so "Say Y or M".

[...]
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/regmap.h>
> +#include <linux/auxiliary_bus.h>
io.h, err.h and platform_device.h are not used right now.

[...]
> +static int spacemit_pd_power_off(struct generic_pm_domain *domain)
[...]
> +	if (ret) {
> +		dev_err(&domain->dev, "failed to power off domain %s: %d\n",
> +			p->name, pd->pm_index);
> +		return -EBUSY;
> +	}

-EBUSY drops the -ETIMEDOUT returned by regmap_read_poll_timeout();
why not print ret and return it? Same in spacemit_pd_power_on().

[...]
> +static bool spacemit_pm_is_off_state(struct spacemit_pmu *pmu,
> +				     struct spacemit_pm_domain *pd)
[...]
> +	return (reg & bit) ? false : true;
return !(reg & bit);

[...]
> +static void spacemit_pm_domain_cleanup(struct spacemit_pmu *pmu)
> +{
> +	struct spacemit_pm_domain *pd;
> +	int i, ret;
> +
> +	for (i = 0; i < pmu->genpd_data.num_domains; i++) {
> +		if (!pmu->domains[i])
> +			continue;
> +
> +		pd = to_spacemit_pd(pmu->domains[i]);
> +		ret = pm_genpd_remove(pmu->domains[i]);
> +		if (ret)
> +			dev_err(pmu->dev, "failed to remove domain %s: %d - state may be inconsistent\n",
> +				pd->param->name, ret);
> +	}
> +}
[...]
> +static void spacemit_pm_domain_remove(struct auxiliary_device *adev)
> +{
> +	struct spacemit_pmu *pmu = auxiliary_get_drvdata(adev);
> +	struct device *dev = &adev->dev;
> +
> +	of_genpd_del_provider(dev->of_node);
> +	spacemit_pm_domain_cleanup(pmu);
> +}
pm_genpd_remove() fails with -EBUSY while a consumer is still attached,
but pd is freed by devres anyway. Set suppress_bind_attrs like most
pmdomain drivers?

-- 
Troy Mitchell

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]