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

[Viresh Kumar]

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 ?

> +/**
> + * 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.

> +{
> +	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.

-- 
viresh