Re: [PATCH 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan

[Ulf Hansson]

[...]

>> I understand the phy is optional, but you still need to handle the
>> EPROBE_DEFER case.
>>
>> Perhaps you should also use devm_phy_optional_get() instead!?
>
>
> I already changed it in version-2 [1]. :)
> phy is mandatory for sdhci-arasan,5.1.
>
> [1]: https://patchwork.kernel.org/patch/7173251/

Oh, apologize for reviewing the old version!

>
>>
>>> +               ret = phy_power_on(sdhci_arasan->phy);
>>
>>
>> This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()?
>>
>> Similar comment applies to phy_exit() and phy_power_off().
>
>
> Both are okay. It depends how the phy-driver implement the four interfaces.
> From my case, power_on for arasan's phy driver firstly enable phy's clk and
> open power-domain, then programme phy internal registers to activate phy.
> Without enabling phy's clk and power-domain, we cannot do phy_init since phy
> can't be accessed by CPU.
>
> But here, I think you are right. It does look a bit weird.
> I think the better way is to remove phy_power_on here, and let phy-driver do
> power_on in phy_init internally. Similarly in the remove call.

That makes sense to me! I think it would also follow other phy clients
use of the phy API.

What makes me wonder though is from a power management point of view.
*When* do you need to have phy initialized and powered?

1) For a removable card can leave the phy uninitialised or powered
off, but still detect card insertion/removal via GPIO? Is that a valid
scenario for you?

2) Considering the runtime PM case for the sdhci device. Typically you
can gate clocks etc at runtime suspend to save power, but what about
the phy? Can you power off it in runtime suspend?

[...]

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/