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

[Yixun Lan <dlan@kernel.org>]

Hi Troy,

On 07:12 Mon 09 Nov     , Troy Mitchell wrote:
> 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".
> 
Ok, will update

> [...]
> > +#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.
> 
Ok, will drop them

> [...]
> > +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().
> 
Yes, I think it's ok to propogate the err

> [...]
> > +static bool spacemit_pm_is_off_state(struct spacemit_pmu *pmu,
> > +				     struct spacemit_pm_domain *pd)
> [...]
> > +	return (reg & bit) ? false : true;
> return !(reg & bit);
> 
Actually, there isn't much difference, your version is slightly short

> [...]
> > +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?
> 

No, I won't add suppress_bind_attrs

As I'm also aware of this problem, but intend to address it in a different
way, and should send a patch in another separate thread..

In my opinion, Adding "suppress_bind_attrs = true" will only mitigate
the problem, in the case of this driver, it will still trigger a problem
while unloading the driver with rmmod command if built as kernel module,
consumer side won't get notification with the event. Ideally the device link
machanism should solve this, currently I'm working on a patch for this..

-- 
Yixun Lan (dlan)