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

[Iker Pedrosa]

Thank you for addressing the feedback. The series looks good now.

Reviewed-by: Iker Pedrosa <ikerpedrosam@xxxxxxxxx>

El mar, 10 nov 2026 a las 11:57, Yixun Lan (<dlan@xxxxxxxxxx>) escribió:
>
> 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)