Re: [PATCH] opp: Add API for getting voltage from supplies

Re: [PATCH] opp: Add API for getting voltage from supplies

[Viresh Kumar]

On 04-12-18, 14:59, Nick Fan wrote:
> Add API to get voltage for multiple supplies from opp table

And who needs to use this new API ? It would be better to add the user in the
same series to make sure this really gets used.

> Signed-off-by: Nick Fan <Nick.Fan@mediatek.com>
> ---
>  drivers/opp/core.c     | 28 ++++++++++++++++++++++++++++
>  include/linux/pm_opp.h |  3 +++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 2c2df4e..ee73546 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -113,6 +113,34 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
>  EXPORT_SYMBOL_GPL(dev_pm_opp_get_voltage);
>  
>  /**
> + * dev_pm_opp_get_voltage_supply() - Gets the voltage corresponding to an opp
> + * with index
> + * @opp:        opp for which voltage has to be returned for
> + * @index:      index to specify the returned supplies
> + *
> + * Return: voltage in micro volt corresponding to the opp with index, else
> + * return 0
> + *
> + * This is useful for devices with multiple power supplies.
> + */
> +unsigned long dev_pm_opp_get_voltage_supply(struct dev_pm_opp *opp,
> +					    unsigned int index)

How will the users of this API get the index ?

> +{
> +	if (IS_ERR_OR_NULL(opp)) {
> +		pr_err("%s: Invalid parameters\n", __func__);
> +		return 0;
> +	}
> +
> +	if (index >= opp->opp_table->regulator_count) {
> +		pr_err("%s: Invalid supply index: %u\n", __func__, index);
> +		return 0;
> +	}
> +
> +	return opp->supplies[index].u_volt;

-- 
viresh

Re: [PATCH] opp: Add API for getting voltage from supplies

[Nick Fan]

On Tue, 2018-12-04 at 13:51 +0530, Viresh Kumar wrote:
> On 04-12-18, 14:59, Nick Fan wrote:
> > Add API to get voltage for multiple supplies from opp table
> 
> And who needs to use this new API ? It would be better to add the user in the
> same series to make sure this really gets used.

This new API would be required when handling multiple regulators.
You can check the example 4 in
Documentation/devicetree/bindings/opp/opp.txt for multiple regulators.

When we specify multiple regulator voltages in device tree, we are not
able to access the secondary supply voltages.
Because the dev_pm_opp_get_voltage only returns the first supply
voltages, this new API is required to get the specific supply.

> 
> > Signed-off-by: Nick Fan <Nick.Fan@mediatek.com>
> > ---
> >  drivers/opp/core.c     | 28 ++++++++++++++++++++++++++++
> >  include/linux/pm_opp.h |  3 +++
> >  2 files changed, 31 insertions(+)
> > 
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 2c2df4e..ee73546 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -113,6 +113,34 @@ unsigned long dev_pm_opp_get_voltage(struct dev_pm_opp *opp)
> >  EXPORT_SYMBOL_GPL(dev_pm_opp_get_voltage);
> >  
> >  /**
> > + * dev_pm_opp_get_voltage_supply() - Gets the voltage corresponding to an opp
> > + * with index
> > + * @opp:        opp for which voltage has to be returned for
> > + * @index:      index to specify the returned supplies
> > + *
> > + * Return: voltage in micro volt corresponding to the opp with index, else
> > + * return 0
> > + *
> > + * This is useful for devices with multiple power supplies.
> > + */
> > +unsigned long dev_pm_opp_get_voltage_supply(struct dev_pm_opp *opp,
> > +					    unsigned int index)
> 
> How will the users of this API get the index ?

For the users who only use one supply, they can use
dev_pm_opp_get_voltage to get the voltage data from an opp.
But if the users who use more than one supply, they will need this API
to get their voltage data from OPP.
The users should know about the supply count while creating opp table by
using dev_pm_opp_set_regulators function.
By using this API, the users can get the voltages by using index to
specify which supplies they want.

The following is a simple example to get multiple regulators by this
API.
for (i = 0; i < regulator_num; i++)
	target_volt[i] = dev_pm_opp_get_voltage_supply(opp, i);
> 
> > +{
> > +	if (IS_ERR_OR_NULL(opp)) {
> > +		pr_err("%s: Invalid parameters\n", __func__);
> > +		return 0;
> > +	}
> > +
> > +	if (index >= opp->opp_table->regulator_count) {
> > +		pr_err("%s: Invalid supply index: %u\n", __func__, index);
> > +		return 0;
> > +	}
> > +
> > +	return opp->supplies[index].u_volt;
> 
Nick Fan

Re: [PATCH] opp: Add API for getting voltage from supplies

[Viresh Kumar]

On 10-12-18, 20:36, Nick Fan wrote:
> For the users who only use one supply, they can use
> dev_pm_opp_get_voltage to get the voltage data from an opp.
> But if the users who use more than one supply, they will need this API
> to get their voltage data from OPP.
> The users should know about the supply count while creating opp table by
> using dev_pm_opp_set_regulators function.
> By using this API, the users can get the voltages by using index to
> specify which supplies they want.
> 
> The following is a simple example to get multiple regulators by this
> API.
> for (i = 0; i < regulator_num; i++)
> 	target_volt[i] = dev_pm_opp_get_voltage_supply(opp, i);

Fair enough. I couldn't find anything wrong with the patch. Will it be
possible to send this patch as part of a series which uses the new API
? So that we are sure of somebody using it eventually.

-- 
viresh

Re: [PATCH] opp: Add API for getting voltage from supplies

[Nick Fan]

On Thu, 2018-12-13 at 12:08 +0530, Viresh Kumar wrote:
> On 10-12-18, 20:36, Nick Fan wrote:
> > For the users who only use one supply, they can use
> > dev_pm_opp_get_voltage to get the voltage data from an opp.
> > But if the users who use more than one supply, they will need this API
> > to get their voltage data from OPP.
> > The users should know about the supply count while creating opp table by
> > using dev_pm_opp_set_regulators function.
> > By using this API, the users can get the voltages by using index to
> > specify which supplies they want.
> > 
> > The following is a simple example to get multiple regulators by this
> > API.
> > for (i = 0; i < regulator_num; i++)
> > 	target_volt[i] = dev_pm_opp_get_voltage_supply(opp, i);
> 
> Fair enough. I couldn't find anything wrong with the patch. Will it be
> possible to send this patch as part of a series which uses the new API
> ? So that we are sure of somebody using it eventually.
> 
This new API is suitable for the users that required to access for
multiple regulators. And I am one of users who uses this API, but I am
not able to upstream the GPU kernel driver which uses the new API.

--
Nick Fan

Re: [PATCH] opp: Add API for getting voltage from supplies

[Viresh Kumar]

On 13-12-18, 18:36, Nick Fan wrote:
> This new API is suitable for the users that required to access for
> multiple regulators. And I am one of users who uses this API, but I am
> not able to upstream the GPU kernel driver which uses the new API.

Look at it from mainline's perspective..

- We don't (can't) care about non-mainline stuff. Actually we do care,
  but we can't add code to mainline which is not going to be used by
  any mainline user.

- I am fine with introducing the new API, but that will happen only if
  there is going to be a mainline user as well.

- As you are going to maintain your GPU driver offline, you can
  maintain this patch too.

Sorry Nick.

-- 
viresh

Re: [PATCH] opp: Add API for getting voltage from supplies

[Nick Fan]

On Thu, 2018-12-13 at 16:12 +0530, Viresh Kumar wrote:
> On 13-12-18, 18:36, Nick Fan wrote:
> > This new API is suitable for the users that required to access for
> > multiple regulators. And I am one of users who uses this API, but I am
> > not able to upstream the GPU kernel driver which uses the new API.
> 
> Look at it from mainline's perspective..
> 
> - We don't (can't) care about non-mainline stuff. Actually we do care,
>   but we can't add code to mainline which is not going to be used by
>   any mainline user.
> 
> - I am fine with introducing the new API, but that will happen only if
>   there is going to be a mainline user as well.
> 
> - As you are going to maintain your GPU driver offline, you can
>   maintain this patch too.
> 
> Sorry Nick.
> 
It's OK.
I got your ideas.
Thanks a lot for the explanation and review on this patch.

--
Nick Fan