Re: [PATCH v4 1/8] of: Add NVIDIA Tegra SATA controller binding

[Hans de Goede <hdegoede@redhat.com>]

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.

>>> 	static const char *const tegra_ahci_clocks[] = {
>>> 		"sata-oob", "cml1", pll_e",
>>> 	};
>>
>> Note you could also put the "sata" clock here, since then you will
>> know its index, and can use hpriv->clks to access it. Not putting
>> it here has the advantage of not doing the double enable / disable
>> (and the disadvantage of needing to the clk_get yourself).
> 
> Right, the intention here was that we wouldn't have it managed by the
> ahci_platform library because it needs special handling anyway. Having
> it in this table would therefore be redundant.

Ack.

Regards,

Hans