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

The OPP core now provides a unified API for setting all configuration
types, i.e. dev_pm_opp_set_config().

Lets start using it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

On Fri, Jul 01, 2022 at 01:50:19PM +0530, Viresh Kumar wrote:
> The OPP core now provides a unified API for setting all configuration
> types, i.e. dev_pm_opp_set_config().
> 
> Lets start using it.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 4733a233bd0c..ab667838d082 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1347,6 +1347,10 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  	int irq;
>  	bool console = false;
>  	struct uart_driver *drv;
> +	struct dev_pm_opp_config config = {
> +		.clk_names = (const char *[]){ "se" },
> +		.clk_count = 1,
> +	};
>  
>  	if (of_device_is_compatible(pdev->dev.of_node, "qcom,geni-debug-uart"))
>  		console = true;
> @@ -1430,7 +1434,7 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  	if (of_property_read_bool(pdev->dev.of_node, "cts-rts-swap"))
>  		port->cts_rts_swap = true;
>  
> -	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?

thanks,

greg k-h

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

On 01-07-22, 12:18, Greg Kroah-Hartman wrote:
> On Fri, Jul 01, 2022 at 03:31:00PM +0530, Viresh Kumar wrote:
> Still crazy, but a bit better.

:)

> Why do you need the clk_count?  A null terminated list is better,

Because I am not a big fan of the null terminated lists :)

I had to chase a bug once where someone removed that NULL at the end
and it was a nightmare to understand what's going on.

> as the
> compiler can do it for you and you do not have to keep things in sync
> like you are expecting people to be forced to do now.

I am not sure I understand what the compiler can do for us here.

The users will be required to do this here, isn't it ?

        const char *clks[] = { "core", NULL };
        struct dev_pm_opp_config opp_config = {
               .clk_names = clks,
        };


> The above is much more complex than a simple function call to make.
> Remember to make it very simple for driver authors, and more
> importantly, reviewers.

Hmm.

> Thanks, and drop the count field please.

There is one case at least [1] where we actually have to pass NULL in
the clk name. This is basically to allow the same code to run on
different devices, one where an OPP table is present and one where it
isn't. We don't want to do clk_set_rate() for the second case but just
use dev_pm_opp_set_rate() (which does a lot of stuff apart from just
clk).

On 01-07-22, 12:41, Greg Kroah-Hartman wrote:
> That feels completely wrong, don't have NULL for a name, make a fake name
> or something.  Don't make all users in the kernel have a horrible
> interface just for one piece of broken hardware out there.
> 
> Worst case, name it "".

This name goes into the second argument of clk_get(dev, const char *con_id);

I will see how else I should hack it :)