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

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Jiri Slaby <jirislaby@kernel.org>,
	linux-pm@vger.kernel.org,
	Vincent Guittot <vincent.guittot@linaro.org>,
	"Rafael J. Wysocki" <rafael@kernel.org>,
	Stephen Boyd <sboyd@kernel.org>, Nishanth Menon <nm@ti.com>,
	linux-arm-msm@vger.kernel.org, linux-serial@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2 24/30] serial: qcom: Migrate to dev_pm_opp_set_config()
Date: Fri, 1 Jul 2022 12:18:17 +0200	[thread overview]
Message-ID: <Yr7J6f6+EQfXFjYN@kroah.com> (raw)
In-Reply-To: <20220701100100.bxv4t4t7iqphalpv@vireshk-i7>

On Fri, Jul 01, 2022 at 03:31:00PM +0530, Viresh Kumar wrote:
> On 01-07-22, 11:39, Greg Kroah-Hartman wrote:
> > It's now more complex for simple drivers like this, right?
> 
> They need to add a structure, yes.
> 
> > 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,
> 
> That can be done, yes.
> 
> > allowing you
> > to continue to do simple things without the overhead of a driver having
> > to create a structure on the stack
> 
> I didn't think of it as complexity, and I still feel it is okay-ish.
> 
> > and remember how the "const char *[]"
> > syntax looks like (seriously, that's crazy).
> 
> The syntax can be fixed, if we want, by avoiding the cast with
> something like this:
> 
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index a018b45c5a9a..1a5480214a43 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -2559,8 +2559,9 @@ static int sdhci_msm_probe(struct platform_device *pdev)
>         const struct sdhci_msm_offset *msm_offset;
>         const struct sdhci_msm_variant_info *var_info;
>         struct device_node *node = pdev->dev.of_node;
> +       const char *clks[] = { "core" };
>         struct dev_pm_opp_config opp_config = {
> -               .clk_names = (const char *[]){ "core" },
> +               .clk_names = clks,
>                 .clk_count = 1,
>         };

Still crazy, but a bit better.

Why do you need the clk_count?  A null terminated list is better, 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.

> > Make it simple for simple things, and provide the ability to do complex
> > things only if that is required.
> 
> I still feel it isn't too bad for simple cases right now too, it is
> just a structure to fill out but I don't have hard feelings for
> keeping the old API around. I just feel it isn't too helpful to keep
> the old interfaces around, it will just confuse people at the best.

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.

> Anyway, I will keep them around.

Thanks, and drop the count field please.

thanks,

greg k-h