Hi, On 07/17/2014 01:42 PM, Hans de Goede wrote: > Hi, > > On 07/17/2014 12:52 PM, Thierry Reding wrote: >> On Thu, Jul 17, 2014 at 12:23:47PM +0200, Hans de Goede wrote: > > <snip> > >>> The libahci_platform.c code / ahci_platform.c code is also used for >>> devices going way back who may not yet be using the new clk framework, >>> so where we need to use clk_get(dev, NULL); quoting from libahci_platform.c : >>> >>> for (i = 0; i < AHCI_MAX_CLKS; i++) { >>> /* >>> * For now we must use clk_get(dev, NULL) for the first clock, >>> * because some platforms (da850, spear13xx) are not yet >>> * converted to use devicetree for clocks. For new platforms >>> * this is equivalent to of_clk_get(dev->of_node, 0). >>> */ >>> if (i == 0) >>> clk = clk_get(dev, NULL); >>> else >>> clk = of_clk_get(dev->of_node, i); >>> >>> if (IS_ERR(clk)) { >>> rc = PTR_ERR(clk); >>> if (rc == -EPROBE_DEFER) >>> goto err_out; >>> break; >>> } >>> hpriv->clks[i] = clk; >>> } >>> >>> And there is no devm variant of that, nor is there one to get clocks by index. >>> Note that we also need ahci_platform_put_resources for runtime pm support, so >>> that one is going to stay around anyways and thus there is not that much value >>> in fixing this. >>> >>> So although I like Thierry's idea, if we go this way (which sounds good), we >>> should add support for taking a NULL ahci_platform_resources argument and in >>> that case behave as before, esp. because of the platforms needing the old >>> style clock handling. An advantage of doing this, is that we can simply patch >>> all existing users to pass NULL. >> >> Isn't the "legacy" case really just this: >> >> static const char *const legacy_ahci_clocks[] = { >> NULL >> }; >> >> static const struct ahci_platform_resources legacy_ahci_resources = { >> .num_clocks = ARRAY_SIZE(legacy_ahci_clocks), >> .clocks = legacy_ahci_clocks, >> }; >> >> ? > > Ah yes that would work for the really legacy ones, as well as less legacy > (full dts) ones with only one clk, we need to check if there are current > users which use more then one clk (yes there are which is why MAX_CLKS > was 3) and fixup those to pass in a correct ahci_platform_resources struct > then. > > The checking + fixing up will be a bit of extra work, but I think the end result > will be quite nice. so I'm all in favor of this. Correction, this is not going to work I'm afraid, as not all current dts files set clock-names. So we need a fallback to get clks by index for compatibility with old dts files. At least: arch/arm/boot/dts/sun4i-a10.dtsi and arch/arm/boot/dts/sun7i-a20.dtsi are affected. Note in this case the dts file is typically not burned into a ROM or some such, so IMHO we could get away with requiring a new dts file. So it might be worthwhile to still do a full check if all affected SoCs and see if we can move over to using clock-names for all platforms with more then 1 ahci/sata clk. FWIW I've also just checked imx6q.dts which is the one which has 3 clocks, and that one does define clk names. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html