Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, Jul 01, 2022 at 02:54:58PM +0530, Viresh Kumar wrote:
> On 01-07-22, 10:44, Greg Kroah-Hartman wrote:
> > On Fri, Jul 01, 2022 at 01:50:19PM +0530, Viresh Kumar wrote:
> > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > > +	struct dev_pm_opp_config config = {
> > > +		.clk_names = (const char *[]){ "se" },
> > > +		.clk_count = 1,
> > > +	};
> > >  
> > > -	ret = devm_pm_opp_set_clkname(&pdev->dev, "se");
> > > +	ret = devm_pm_opp_set_config(&pdev->dev, &config);
> > 
> > This feels like a step back.  This is much harder now, what's wrong with
> > devm_pm_opp_set_clkname() as is?
> 
> Hi Greg,
> 
> There are a number of configurations one can do for a device's OPP
> table currently:
> 
> - clk, single or multiple (new)
> - helper to configure multiple clocks (for multiple clocks)
> - supplies or regulators
> - helper to configure supplies (for multiple supplies)
> - OPP supported-hw property
> - OPP Property-name
> - Genpd specific one
> - etc
> 
> One problem was that it was a mess within the OPP core with a separate
> interface for each of these interfaces, almost duplicate code, etc.
> 
> But then it was a bigger mess for the user drivers that need to manage
> a few of these. They were required to call multiple APIs, with all the
> interfaces returning tokens, which the callers need to save and supply
> back to free the resources later. More code, hard to manage, easy to
> abuse and add bugs to.
> 
> The new interface makes it easier and clean for everyone and allows
> easy upgrades of interfaces in future. Adding a new interface, like
> support for multiple clocks for a device that I just did, is much
> easier now.
> 
> I really believe this is a step in the right direction :)

It's now more complex for simple drivers like this, right?  Why not
provide translations of the devm_pm_opp_set_clkname() to use internally
devm_pm_opp_set_config() if you want to do complex things, allowing you
to continue to do simple things without the overhead of a driver having
to create a structure on the stack and remember how the "const char *[]"
syntax looks like (seriously, that's crazy).

Make it simple for simple things, and provide the ability to do complex
things only if that is required.

thanks,

greg k-h