On 2015/9/14 23:07, Sören Brinkmann wrote:
Hi Shawn,
overall, it looks good to me. I have some questions though.
On Mon, 2015-09-14 at 02:29PM +0800, Shawn Lin wrote:
[...]
+err_phy_exit:
+ phy_init(phy);
Just to confirm, are these actions in the error path correct? E.g.
if the power_off() call fails, is it safe to call power_on()? Isn't
the phy still powered on? (this would apply to other error paths too)
Cool question!
While writing this, I had read generic phy stuffs deliberately to find a
solution for a case: how to deal with ping-pong fails? In another word,
if power_off call fails, then we should call power_on, but unfortunately
it fails again then we call power_off... so endless nested err
handling... No answer yet.
So, I assumed two cases happened when power_off call fails:
(1) *real power_off* is done, but some other stuffs in the calling path
fail, so phy is really power_off in theory. We need to power_on it
again, but if it fail, we don't know PHY is on or off since we don't
know power_on fails for what? *real power on* ? some other stuffs?
(2) *real power_off* isn't completed, so indeed it's *still* in power_on
state. The reason we never need to check the return value of power_on
cross the err handling is that whether power_on call successfully or
not, it's always make phy in power_on state.
Now, let's think about case(1).
After reading dozens of sample codes(such as USB, UFS, MBUS) that adopt
generic phy framework for PHY management, real thing should be like
that: they NEVER deal with case(1).
It's a trick of sub-phy drivers themself. power_on/off calling path
return err for two case:
<1> phy_runtime callback fails. It's after *real power on/off*, so
definitely *real power on/off* is conpleted. That is the case(2) I
mentioned above.
<2> sub-phy drivers return err for phy->ops->power_off(phy); Look
into all the sub-phy drivers twice, we find that they always return
success for phy->ops->power_off hook. Why? Because all of them
write registers to enable/disable something, always consider things
going well. Actually if we write value into a register, we have to think
that it's functional.
Anyway, back to this patch.
Indeed we also write value into arasan phy's register to do
phy_power_on/off/init/exit to make things work. Right, we return success
state for all of these them just as all the other sub-phy drivers do.
Feel free to let me know if I make mistakes or misunderstanding above.
+ return ret;
+}
[...]
+ }
+ }
I assume you looked at options for having the error paths in a
consolidated location? I guess this may be the nicest solution since all
of this is in this conditional block?
yep, otherwise we should add some *if* statements to check
sdhci_arasan->phy cross the err handles. And I intent to strictly limit
the phy stuffs under the scope of arasan,sdhci-5.1 currently.
Feel free to add my
Acked-by: Sören Brinkmann <soren.brinkmann@xxxxxxxxxx>
Thanks, Sören. :)
Sören
--
Best Regards
Shawn Lin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
