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
