| Message ID | 55CBC14B.8000509@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 12 Aug 14:57 PDT 2015, Stephen Boyd wrote: > On 08/12/2015 01:18 PM, Bjorn Andersson wrote: > > On Tue 11 Aug 15:49 PDT 2015, Stephen Boyd wrote: > > > >> On 07/08, Rajendra Nayak wrote: > >>> diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c > >>> index eb6a4f9..2c80d03 100644 > >>> --- a/drivers/clk/qcom/gcc-msm8960.c > >>> +++ b/drivers/clk/qcom/gcc-msm8960.c > >>> @@ -15,6 +15,7 @@ > >>> #include <linux/bitops.h> > >>> #include <linux/err.h> > >>> #include <linux/platform_device.h> > >>> +#include <linux/of_platform.h> > >>> #include <linux/module.h> > >>> #include <linux/of.h> > >>> #include <linux/of_device.h> > >>> @@ -3520,7 +3521,8 @@ static int gcc_msm8960_probe(struct platform_device *pdev) > >>> if (IS_ERR(clk)) > >>> return PTR_ERR(clk); > >>> > >>> - return qcom_cc_probe(pdev, match->data); > >>> + qcom_cc_probe(pdev, match->data); > >>> + return of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev); > >> We just lost the error code from qcom_cc_probe()... > >> > >> Also, I don't like having a subnode in DT. Why can't we use the > >> same node as the GCC node and create a virtual child device here > >> for tsens? We can assign the same of_node that this platform > >> device has so that DT keeps working correctly. > >> > > Can't we make the gcc driver support being a child of a simple-mfd by > > having it attempting to acquire the regmap of the parent and falling > > back to creating its own mmio regmap? > > So we would need to make a new device and driver for the simple-mfd > parent? No that part is already in mainline, so my idea was that we add an early return in qcom_cc_map() like: struct device *dev = &pdev->dev; + struct regmap *map; + + map = syscon_node_to_regmap(dev->parent->of_node); + if (!IS_ERR(map)) + return map; And then just update the dts to: gcc { compatible = "syscon", "simple-mfd"; reg = <0x00900000 0x4000>; gcc: clock-controller { compatible = "qcom,gcc-apq8064"; #clock-cells = <1>; #reset-cells = <1>; }; }; But as I implemented this I realized that the syscon_node_to_regmap() does not register the regmap as a devres of the parent, and as such it's not caught by the regmap lookup in devm_clk_register_regmap(). So either one would need to make the syscon throw the regmap into devres or make it possible to pass a regmap to devm_clk_register_regmap() for this to work. > I'm confused about what you're suggesting and what benefit it > has versus creating a child of the clock controller device. The benefit would simply be that we're using the already existing mechanism for handling multiple platform_drivers sharing a hw block. > > Here's the patch I'm suggesting. The device name is probably wrong, but > you get the idea. Looks very much like my take on it as well, I do however have concerns that suddenly the node called "clock-controller" will have to come with tsens related properties. Are all the tsens-in-gcc blocks the same? Or do you intend to of_match on the gcc compatible in the tsens driver? Regards, Bjorn
On 08/12, Bjorn Andersson wrote: > On Wed 12 Aug 14:57 PDT 2015, Stephen Boyd wrote: > > > Here's the patch I'm suggesting. The device name is probably wrong, but > > you get the idea. > > Looks very much like my take on it as well, I do however have concerns > that suddenly the node called "clock-controller" will have to come with > tsens related properties. > > Are all the tsens-in-gcc blocks the same? Yes they're all the same, except some of them have more sensors than others. It depends on what SoC the hardware is instantiated in. They also messed up the software interface and had to add discontinuous registers for the sensors because it was never designed to have more than 4 sensors or something like that. But even that sort of change can be handled by figuring out the format of the sensor data in the qfprom. > Or do you intend to of_match > on the gcc compatible in the tsens driver? > I hope we don't have to of_match in the tsens driver on gcc compatible strings.
diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c index aa294b1bad34..802e9794f76e 100644 --- a/drivers/clk/qcom/gcc-msm8960.c +++ b/drivers/clk/qcom/gcc-msm8960.c @@ -3505,7 +3505,9 @@ static int gcc_msm8960_probe(struct platform_device *pdev) { struct clk *clk; struct device *dev = &pdev->dev; + struct platform_device *tsens; const struct of_device_id *match; + int ret; match = of_match_device(gcc_msm8960_match_table, &pdev->dev); if (!match) @@ -3520,7 +3522,30 @@ static int gcc_msm8960_probe(struct platform_device *pdev) if (IS_ERR(clk)) return PTR_ERR(clk); - return qcom_cc_probe(pdev, match->data); + ret = qcom_cc_probe(pdev, match->data); + if (ret) + return ret; + + tsens = platform_device_alloc("tsens", -1); + if (!tsens) { + ret = -ENOMEM; + goto err_alloc; + } + + tsens->dev.parent = &pdev->dev; + tsens->dev.of_node = pdev->dev.of_node; + ret = platform_device_add(tsens); + if (ret) + goto err_add; + + return 0; + +err_add: + platform_device_put(tsens); +err_alloc: + qcom_cc_remove(pdev); + + return ret; } static int gcc_msm8960_remove(struct platform_device *pdev)