Hi,

On 07/17/2014 01:42 PM, Hans de Goede wrote:

Hi,

On 07/17/2014 12:52 PM, Thierry Reding wrote:

quoted

On Thu, Jul 17, 2014 at 12:23:47PM +0200, Hans de Goede wrote:

<snip>

quoted

quoted

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