On Wed, Oct 14, 2020 at 10:29 AM Wenbin Mei [off-list ref] wrote:

On Tue, 2020-10-13 at 17:10 +0200, Matthias Brugger wrote:

quoted

On 12/10/2020 14:45, Wenbin Mei wrote:

quoted

MT8192 msdc is an independent sub system, we need control more bus
clocks for it.
Add support for the additional subsys clocks to allow it to be
configured appropriately.

Signed-off-by: Wenbin Mei <redacted>
---
  drivers/mmc/host/mtk-sd.c | 74 +++++++++++++++++++++++++++++----------
  1 file changed, 56 insertions(+), 18 deletions(-)

diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index a704745e5882..c7df7510f120 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c

[...]

quoted

+static int msdc_of_clock_parse(struct platform_device *pdev,
+                          struct msdc_host *host)
+{
+   int ret;
+
+   host->src_clk = devm_clk_get(&pdev->dev, "source");
+   if (IS_ERR(host->src_clk))
+           return PTR_ERR(host->src_clk);
+
+   host->h_clk = devm_clk_get(&pdev->dev, "hclk");
+   if (IS_ERR(host->h_clk))
+           return PTR_ERR(host->h_clk);
+
+   host->bus_clk = devm_clk_get_optional(&pdev->dev, "bus_clk");
+   if (IS_ERR(host->bus_clk))
+           host->bus_clk = NULL;
+
+   /*source clock control gate is optional clock*/
+   host->src_clk_cg = devm_clk_get_optional(&pdev->dev, "source_cg");
+   if (IS_ERR(host->src_clk_cg))
+           host->src_clk_cg = NULL;
+
+   host->sys_clk_cg = devm_clk_get_optional(&pdev->dev, "sys_cg");
+   if (IS_ERR(host->sys_clk_cg))
+           host->sys_clk_cg = NULL;
+
+   /* If present, always enable for this clock gate */
+   clk_prepare_enable(host->sys_clk_cg);
+
+   host->bulk_clks[0].id = "pclk_cg";
+   host->bulk_clks[1].id = "axi_cg";
+   host->bulk_clks[2].id = "ahb_cg";

That looks at least suspicious. The pointers of id point to some strings defined
in the function. Aren't they out of scope once msdc_of_clock_parse() has returned?

These constants are not in stack range, so they will not be lost.
And I have confirmed it after msdc_of_clock_parse() has returned, these
ids still exist.

Yes I guess the constants end up in .rodata (or similar section), but
I'm not sure if this is absolutely guaranteed.

In any case, this is a commonly used pattern, so I'd hope it's fine
(just a sample, there are more):
https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/dwc/pcie-qcom.c#L266
https://elixir.bootlin.com/linux/latest/source/sound/soc/codecs/wm8994.c#L4638
https://elixir.bootlin.com/linux/latest/source/drivers/mfd/madera-core.c#L467
https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpio-dwapb.c#L675

quoted

Regards,
Matthias

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel