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

[Yixun Lan <dlan@kernel.org>]

Hi Iker Pedrosa,

On 20:54 Fri 02 Oct     , Iker Pedrosa wrote:
> El vie, 18 sept 2026 a las 7:49, Yixun Lan (<dlan@kernel.org>) escribió:
> > [...]
> > diff --git a/drivers/pmdomain/spacemit/pm_domains.c b/drivers/pmdomain/spacemit/pm_domains.c
> > new file mode 100644
> > index 000000000000..d563e4e4e232
> > --- /dev/null
> > +++ b/drivers/pmdomain/spacemit/pm_domains.c
> > [...]
> > +static struct spacemit_pmu *gpmu;
> 
> Would it make sense to embed the 'struct spacemit_pmu' pointer directly into
> 'struct spacemit_pm_domain'? Other generic Power Domain drivers (Rockchip,
> QCOM, Renesas) use this pattern to keep domain callbacks self-contained and
> ready for multi-instance SoCs
> 
> > [...]
> > +static int spacemit_pd_power_on(struct generic_pm_domain *domain)
> > +{
> > [...]
> > + regmap_read(gpmu->regmap, APMU_POWER_STATUS_REG, &val);
> 
> Please check the return value of regmap_read()
> 
It's not necessary, or it's not really useful to check return value here,
regmap_read() shouldn't fail here, it generally access the io memory

> > [...]
> > + if (ret < 0) {
> > + dev_err(&domain->dev, "power-off domain: %d, error\n", spd->pm_index);
> 
> Typo: should this say "power-on domain" since this is inside
> spacemit_pd_power_on()?
> 
will drop this logic, I just followed vendor driver which try to force power
off donmain before doing the power on operation if it's already in 'on' state..

while found it's not really necessary to do this during actual testing

> > [...]
> > +static bool spacemit_pm_get_state(struct spacemit_pmu *pmu,
> > +  struct spacemit_pm_domain *pd)
> > +{
> > + const struct spacemit_pm_domain_param *p = pd->param;
> > + u32 reg, bit;
> > +
> > + regmap_read(pmu->regmap, APMU_POWER_STATUS_REG, &reg);
> 
> Please check the return value here as well
> 
I don't think the check is useful, in rare case it should fail

> > + bit = p->use_hw ? BIT(pd->param->bit_hw_pwr_stat) :
> > +  BIT(pd->param->bit_pwr_stat);
> 
> Since 'p' is initialized to 'pd->param' above, you can use 'p->bit_hw_pwr_stat'
> and 'p->bit_pwr_stat' directly here
Yes, will do

-- 
Yixun Lan (dlan)