| Message ID | 1f3328dafaf9e2944fba8ec9e55e3072a63a4192.1656660185.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | OPP: Add new configuration interface: dev_pm_opp_set_config() | expand |
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 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 :)
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, 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, }; > 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. Anyway, I will keep them around.
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
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 Fri, Jul 01, 2022 at 03:59:26PM +0530, Viresh Kumar wrote: > 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. But that's the "normal" way the kernel does things. Trying to keep a count in sync with a list is a pain, and just gets harder and harder over time. Make it a null-terminated list so that the cpu makes this always work and prevents errors. > > 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 "in sync" is the count issue. Don't force humans to count up the number of items in a list please. > > 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). 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 "". thanks, greg k-h
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 :)
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); if (ret) return ret; /* OPP table is optional */
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(-)