On Tue, 2015-09-15 at 09:07AM +0800, Shawn Lin wrote: > 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. :) Makes all sense to me. Thanks for all the details. Sören -- 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
