Re: [PATCH 11/16] OPP: Add dev_pm_opp_add_dynamic() to allow more flexibility

[Ulf Hansson]

On Thu, 8 Jun 2023 at 07:29, Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
>
> On 07-06-23, 14:46, Ulf Hansson wrote:
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 954c94865cf5..0e6ee2980f88 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1921,8 +1921,7 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> >   * _opp_add_v1() - Allocate a OPP based on v1 bindings.
> >   * @opp_table:       OPP table
> >   * @dev:     device for which we do this operation
> > - * @freq:    Frequency in Hz for this OPP
> > - * @u_volt:  Voltage in uVolts for this OPP
> > + * @opp:     The OPP to add
> >   * @dynamic: Dynamically added OPPs.
> >   *
> >   * This function adds an opp definition to the opp table and returns status.
> > @@ -1940,10 +1939,10 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
> >   * -ENOMEM   Memory allocation failure
> >   */
> >  int _opp_add_v1(struct opp_table *opp_table, struct device *dev,
> > -             unsigned long freq, long u_volt, bool dynamic)
> > +             struct dev_pm_opp_data *opp, bool dynamic)
>
> The name `opp` is mostly used for instances of `struct dev_pm_opp`. Can we use a
> different name here please for the data ?

Certainly, what do you suggest?

>
> > +/**
> > + * dev_pm_opp_add()  - Add an OPP table from a table definitions
> > + * @dev:     device for which we do this operation
> > + * @freq:    Frequency in Hz for this OPP
> > + * @u_volt:  Voltage in uVolts for this OPP
> > + *
> > + * This function adds an opp definition to the opp table and returns status.
> > + * The opp is made available by default and it can be controlled using
> > + * dev_pm_opp_enable/disable functions.
> > + *
> > + * Return:
> > + * 0         On success OR
> > + *           Duplicate OPPs (both freq and volt are same) and opp->available
> > + * -EEXIST   Freq are same and volt are different OR
> > + *           Duplicate OPPs (both freq and volt are same) and !opp->available
> > + * -ENOMEM   Memory allocation failure
> > + */
> > +int dev_pm_opp_add(struct device *dev, unsigned long freq, unsigned long u_volt)
>
> Maybe move this to include/linux/pm_opp.h and mark it static inline and get rid
> of documentation too.

Good idea!

>
> > +{
> > +     struct dev_pm_opp_data opp;
> > +
> > +     memset(&opp, 0, sizeof(opp));
>
> What about
>         struct dev_pm_opp_data data = {0};
>
> I think it is guaranteed that all the fields will be 0 now, not the padding of
> course, but we don't care about that here.
>
> > +     opp.freq = freq;
> > +     opp.u_volt = u_volt;
>
> Or maybe just
>
>         struct dev_pm_opp_data data = {
>                 .freq = freq,
>                 .u_volt = u_volt,
>         };
>
> Rest must be 0.

Good suggestions both, I will change to whatever is best suitable!

Thanks for reviewing!

Kind regards
Uffe