On Wed, Jun 11, 2025 at 10:32:28AM -0500, Bjorn Andersson wrote:
> On Mon, Jun 02, 2025 at 10:19:03AM -0300, Hiago De Franco wrote:
> > From: Hiago De Franco <hiago.fra...@toradex.com>
> >
> > This helper function returns the current power status of a given generic
> > power domain.
> >
>
> Please correct me if I'm wrong, but this returns the momentary status of
> the device's associated genpd, and as genpds can be shared among devices
> wouldn't there be a risk that you think the genpd is on but then that
> other device powers it off?
I am not fully familiar with the genpd's, so my knowledge might be
limited, but I think this is correct, if the genpd is shared.
>
> > As example, remoteproc/imx_rproc.c can now use this function to check
> > the power status of the remote core to properly set "attached" or
> > "offline" modes.
>
> I presume this example works because there is a dedicated, single usage,
> genpd for the remoteproc instance?
Peng might correct if I am wrong, but yes, I believe this is correct.
>
> >
> > Suggested-by: Ulf Hansson <ulf.hans...@linaro.org>
> > Signed-off-by: Hiago De Franco <hiago.fra...@toradex.com>
> > ---
> > v4: New patch.
> > ---
> > drivers/pmdomain/core.c | 27 +++++++++++++++++++++++++++
> > include/linux/pm_domain.h | 6 ++++++
> > 2 files changed, 33 insertions(+)
> >
> > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > index ff5c7f2b69ce..bcb74d10960c 100644
> > --- a/drivers/pmdomain/core.c
> > +++ b/drivers/pmdomain/core.c
> > @@ -758,6 +758,33 @@ int dev_pm_genpd_rpm_always_on(struct device *dev,
> > bool on)
> > }
> > EXPORT_SYMBOL_GPL(dev_pm_genpd_rpm_always_on);
> >
> > +/**
> > + * dev_pm_genpd_is_on - Get device's power status
>
> Functions in kernel-doc should have () prefix
Thanks, I will correct this is next patch version.
>
> > + *
> > + * @dev: Device to get the current power status
> > + *
> > + * This function checks whether the generic power domain is on or not by
> > + * verifying if genpd_status_on equals GENPD_STATE_ON.
> > + *
>
> If my understanding is correct, I'd like a warning here saying that this
> is dangerous if the underlying genpd is shared.
I believe this is correct, maybe Peng or Ulf can also comment here, but
if that is the case then I can update the comment.
>
> Regards,
> Bjorn
>
> > + * Return: 'true' if the device's power domain is on, 'false' otherwise.
> > + */
> > +bool dev_pm_genpd_is_on(struct device *dev)
> > +{
> > + struct generic_pm_domain *genpd;
> > + bool is_on;
> > +
> > + genpd = dev_to_genpd_safe(dev);
> > + if (!genpd)
> > + return false;
> > +
> > + genpd_lock(genpd);
> > + is_on = genpd_status_on(genpd);
> > + genpd_unlock(genpd);
> > +
> > + return is_on;
> > +}
> > +EXPORT_SYMBOL_GPL(dev_pm_genpd_is_on);
> > +
> > /**
> > * pm_genpd_inc_rejected() - Adjust the rejected/usage counts for an
> > idle-state.
> > *
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index 0b18160901a2..c12580b6579b 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -301,6 +301,7 @@ void dev_pm_genpd_synced_poweroff(struct device *dev);
> > int dev_pm_genpd_set_hwmode(struct device *dev, bool enable);
> > bool dev_pm_genpd_get_hwmode(struct device *dev);
> > int dev_pm_genpd_rpm_always_on(struct device *dev, bool on);
> > +bool dev_pm_genpd_is_on(struct device *dev);
> >
> > extern struct dev_power_governor simple_qos_governor;
> > extern struct dev_power_governor pm_domain_always_on_gov;
> > @@ -393,6 +394,11 @@ static inline int dev_pm_genpd_rpm_always_on(struct
> > device *dev, bool on)
> > return -EOPNOTSUPP;
> > }
> >
> > +static inline bool dev_pm_genpd_is_on(struct device *dev)
> > +{
> > + return false;
> > +}
> > +
> > #define simple_qos_governor (*(struct dev_power_governor
> > *)(NULL))
> > #define pm_domain_always_on_gov (*(struct dev_power_governor
> > *)(NULL))
> > #endif
> > --
> > 2.39.5
> >
Best Regards,
Hiago.
