Re: [PATCH v3 1/9] opp: add new helper API dev_pm_opp_set_level()

[Praveen Talari]

Hi Viresh

Thank you for review.

On 5/2/2025 11:07 AM, Viresh Kumar wrote:
> On 02-05-25, 08:40, Praveen Talari wrote:
> > To configure a device to a specific performance level, consumer drivers
> > currently need to determine the OPP based on the exact level and then
> > set it, resulting in code duplication across drivers.
> >
> > The new helper API, dev_pm_opp_set_level(), addresses this issue by
> > providing a streamlined method for consumer drivers to find and set the
> > OPP based on the desired performance level, thereby eliminating
> > redundancy.
> >
> > Signed-off-by: Praveen Talari <quic_ptalari@xxxxxxxxxxx>
> >
> > v2 -> v3
> > - moved function defination to pm_opp.h from core.c with inline
> > - updated return value with IS_ERR(opp)
> >
> > v1 -> v2
> > - reorder sequence of tags in commit text
> As Trilok mentioned, this is not the right place for this.
>
> > ---
> >   include/linux/pm_opp.h | 27 +++++++++++++++++++++++++++
> >   1 file changed, 27 insertions(+)
> >
> > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
> > index e7b5c602c92f..31ed8a7b554e 100644
> > --- a/include/linux/pm_opp.h
> > +++ b/include/linux/pm_opp.h
> > @@ -197,6 +197,28 @@ int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
> >   void dev_pm_opp_remove_table(struct device *dev);
> >   void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
> >   int dev_pm_opp_sync_regulators(struct device *dev);
> > +
> > +/*
> > + * dev_pm_opp_set_level() - Configure device for a level
> > + * @dev: device for which we do this operation
> > + * @level: level to set to
> > + *
> > + * Return: 0 on success, a non-zero value if there is an error otherwise.
> > + */
> No need of these for simple wrappers.
>
> > +static inline int dev_pm_opp_set_level(struct device *dev, unsigned int level)
> > +{
> > +	struct dev_pm_opp *opp = dev_pm_opp_find_level_exact(dev, level);
> > +	int ret;
> > +
> > +	if (IS_ERR(opp))
> > +		return IS_ERR(opp);
> IS_ERR is wrong here, should be PTR_ERR.
>
> > +
> > +	ret = dev_pm_opp_set_opp(dev, opp);
> > +	dev_pm_opp_put(opp);
> > +
> > +	return ret;
> > +}
> > +
> >   #else
> >   static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
> >   {
> > @@ -461,6 +483,11 @@ static inline int dev_pm_opp_sync_regulators(struct device *dev)
> >   	return -EOPNOTSUPP;
> >   }
> >   
> > +static inline int dev_pm_opp_set_level(struct device *dev, unsigned int level)
> > +{
> > +	return -EOPNOTSUPP;
> > +}
> > +
> No need of these too for such wrappers. And then this isn't rebased
How come? i have synced  linux-next today itself and pushed from it, 
even i didn't face any issue.
Let me know how/where rebased i.e linux-next or linux?
> over latest changes in the OPP core.
>
> I modified it and applied the below version to my tree now.