Re: Re: [PATCH v2 2/2] ehci-platform: Add support for clks and phy passed through devicetree

Hi,

On 01/09/2014 04:47 PM, Alan Stern wrote:

On Thu, 9 Jan 2014, Hans de Goede wrote:

quoted

quoted

quoted

+Optional properties:
+- clocks : a list of phandle + clock specifier pairs, one for each entry
+   in clock-names.
+- clock-names : "clk0", "clk1", ...
+- phys : phy
+- phy-names : "phy0"
+
+Example:
+
+	ehci@d8007900 {
+		compatible = "via,vt8500-ehci";
+		reg = <0xd8007900 0x200>;
+		interrupts = <43>;
+		clocks = <&usb_clk 6>, <&ahb_gates 2>;
+		clock-names = "clk0", "clk1";

I'm really not convinced by this either. It prevents you from doing
anything useful out of these clocks, and the only thing you can
actually do with it is calling clk_get, and that's pretty much it.

What if you some platform needs to adjust the rate of one of the two?

Then it needs its own driver. This is intended as a binding for a
*generic* driver, which is meant to cover simple straight forward
non-pci ohci cases. For more complex cases a separate driver will
need to be written.

I must say I'm becoming a bit unhappy with how the reviews of devicetree
bindings are being done. In one case it is not generic enough (ahci-sunxi).

If I then try to make it more generic in a case where that can actually
be done as the hardware is pretty straight forward, it is not specific enough.
You can simply not have both!

quoted

Or wants to cut one but not the other for any reason?

This is another example of non generic behavior, requiring a separate
(small using the existing ohci core) platform glue driver, like the *19* we
already have.

Would DT allow ehci-platform.c to access the clocks by their index in
the array, rather than by name?  After all, you don't really care about
the names at all, since the driver knows nothing about the clocks'
functions.

Yes, actually I've been discussing this with Maxime on irc and I'm
just done preparing a v3 doing exactly that. I was just checking mail
for any more review comments before pushing out v3 :)

The same is true of the phy entry.

The phy entry must have a name, this is specified in the phy bindings
in Documentation/devicetree/bindings/phy/phy-bindings.txt:

"""
PHY user node
=============

Required Properties:
phys : the phandle for the PHY device (used by the PHY subsystem)
phy-names : the names of the PHY corresponding to the PHYs present in the
             *phys* phandle
"""

Now that I've dropped the clock-names, I've changed the ugly "phy0" name
to "usb" as the phy pointed to is supposed to be an usb phy.

Regards,

Hans