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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux