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

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

 



On Sun, Jan 05, 2014 at 11:04:40PM +0000, Hans de Goede wrote:
> Currently ehci-platform is only used in combination with devicetree when used
> with some via socs. By extending it to (optionally) get clks and a phy from
> devicetree, and enabling / disabling those on power_on / off, it can be used
> more generically. Specifically after this commit it can be used for the
> ehci controller on Allwinner sunxi SoCs.
> 
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
>  .../devicetree/bindings/usb/platform-ehci.txt      |  25 +++++
>  .../devicetree/bindings/usb/vt8500-ehci.txt        |  12 --
>  drivers/usb/host/ehci-platform.c                   | 125 +++++++++++++++++----
>  3 files changed, 131 insertions(+), 31 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/usb/platform-ehci.txt
>  delete mode 100644 Documentation/devicetree/bindings/usb/vt8500-ehci.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/platform-ehci.txt b/Documentation/devicetree/bindings/usb/platform-ehci.txt
> new file mode 100644
> index 0000000..aba6da1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/platform-ehci.txt
> @@ -0,0 +1,25 @@
> +Generic Platform EHCI controller
> +
> +Required properties:
> + - compatible: Should be "platform-ehci"
> +   (legacy: via,vt8500-ehci" or "wm,prizm-ehci")

Is there any reason for these to be considered legacy? New devices could
just declare their compatibility with one of the existing
implementations, e.g:

ehci {
	compatible  = "some-vendor,ehci", "via,vt8500-ehci";
	...
}

No new compatible string needed in the driver, and we can easily add
quirks support later.

I'm also not a fan of "platform-*" naming, it's leaking a Linux-internal
implementation detail.

> + - reg: Address range of the ehci registers.
> + - interrupts: Should contain the ehci interrupt.
> +
> +Optional properties:
> + - clocks: array of clocks
> + - clock-names: clock names "ahb" and/or "ehci"

It would be nice to define clocks in terms of clock-names, as with my
OCHI comments.

[...]

> @@ -85,7 +159,7 @@ static int ehci_platform_probe(struct platform_device *dev)
>  
>  	/*
>  	 * use reasonable defaults so platforms don't have to provide these.
> -	 * with DT probing on ARM, none of these are set.
> +	 * with DT probing on ARM.

Did you mean to also get rid of the full-stop on the line before?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux