Re: [PATCH v6 1/9] drivers: phy: add generic PHY framework

[Sylwester Nawrocki <sylvester.nawrocki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi Kishon,

On 04/29/2013 12:03 PM, Kishon Vijay Abraham I wrote:
> The PHY framework provides a set of APIs for the PHY drivers to
> create/destroy a PHY and APIs for the PHY users to obtain a reference to the
> PHY with or without using phandle. For dt-boot, the PHY drivers should
> also register *PHY provider* with the framework.
>
> PHY drivers should create the PHY by passing id and ops like init, exit,
> power_on and power_off. This framework is also pm runtime enabled.
>
> The documentation for the generic PHY framework is added in
> Documentation/phy.txt and the documentation for dt binding can be found at
> Documentation/devicetree/bindings/phy/phy-bindings.txt
>
> Signed-off-by: Kishon Vijay Abraham I<kishon-l0cyMroinI0@public.gmane.org>

Thanks for working on this. For the record, I plan to give this a try
in the end of this week, with my simple MIPI CSI/DSI PHY driver. I might
have some more comments then. For now just couple of remarks after
reading the documentation.

> ---
>   .../devicetree/bindings/phy/phy-bindings.txt       |   66 +++
>   Documentation/phy.txt                              |  123 +++++
>   MAINTAINERS                                        |    7 +
>   drivers/Kconfig                                    |    2 +
>   drivers/Makefile                                   |    2 +
>   drivers/phy/Kconfig                                |   13 +
>   drivers/phy/Makefile                               |    5 +
>   drivers/phy/phy-core.c                             |  539 ++++++++++++++++++++
>   include/linux/phy/phy.h                            |  248 +++++++++
>   9 files changed, 1005 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/phy/phy-bindings.txt
>   create mode 100644 Documentation/phy.txt
>   create mode 100644 drivers/phy/Kconfig
>   create mode 100644 drivers/phy/Makefile
>   create mode 100644 drivers/phy/phy-core.c
>   create mode 100644 include/linux/phy/phy.h
>
> diff --git a/Documentation/devicetree/bindings/phy/phy-bindings.txt b/Documentation/devicetree/bindings/phy/phy-bindings.txt
> new file mode 100644
> index 0000000..8ae844f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-bindings.txt
> @@ -0,0 +1,66 @@
> +This document explains only the device tree data binding. For general
> +information about PHY subsystem refer to Documentation/phy.txt
> +
> +PHY device node
> +===============
> +
> +Required Properties:
> +#phy-cells:	Number of cells in a PHY specifier;  The meaning of all those
> +		cells is defined by the binding for the phy node. The PHY
> +		provider can use the values in cells to find the appropriate
> +		PHY.
> +
> +For example:
> +
> +phys: phy {
> +    compatible = "xxx";
> +    reg =<...>;
> +    .
> +    .
> +    #phy-cells =<1>;
> +    .
> +    .
> +};
> +
> +That node describes an IP block (PHY provider) that implements 2 different PHYs.
> +In order to differentiate between these 2 PHYs, an additonal specifier should be
> +given while trying to get a reference to it.
> +
> +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
> +
> +Example 1:
> +usb1: usb_otg_ss@xxx {
> +    compatible = "xxx";
> +    reg =<xxx>;
> +    .
> +    .
> +    phys =<&usb2_phy>,<&usb3_phy>;
> +    phy-names = "usb2phy", "usb3phy";
> +    .
> +    .
> +};
> +
> +This node represents a controller that uses two PHYs, one for usb2 and one for
> +usb3.
> +
> +Example 2:
> +usb2: usb_otg_ss@xxx {
> +    compatible = "xxx";
> +    reg =<xxx>;
> +    .
> +    .
> +    phys =<&phys 1>;
> +    phy-names = "usbphy";
> +    .
> +    .
> +};
> +
> +This node represents a controller that uses one of the PHYs of the PHY provider
> +device defined previously. Note that the phy handle has an additional specifier
> +"1" to differentiate between the two PHYs.
> diff --git a/Documentation/phy.txt b/Documentation/phy.txt
> new file mode 100644
> index 0000000..408d25f
> --- /dev/null
> +++ b/Documentation/phy.txt
> @@ -0,0 +1,123 @@
> +			    PHY SUBSYSTEM
> +		  Kishon Vijay Abraham I<kishon-l0cyMroinI0@public.gmane.org>
> +
> +This document explains the Generic PHY Framework along with the APIs provided,
> +and how-to-use.
> +
> +1. Introduction
> +
> +*PHY* is the abbreviation for physical layer. It is used to connect a device
> +to the physical medium e.g., the USB controller has a PHY to provide functions
> +such as serialization, de-serialization, encoding, decoding and is responsible
> +for obtaining the required data transmission rate. Note that some USB
> +controller has PHY functionality embedded into it and others use an external

"Note that some USB
controllers have PHY functionality embedded into them..." ?

> +PHY. Other peripherals that uses a PHY include Wireless LAN, Ethernet,

s/uses/use ?

> +SATA etc.
> +
> +The intention of creating this framework is to bring the phy drivers spread

s/phy/PHY ?

> +all over the Linux kernel to drivers/phy to increase code re-use and for
> +better code maintainability.
> +
> +This framework will be of use only to devices that uses external PHY (PHY

s/that uses/that use ?

> +functionality is not embedded within the controller).
> +
> +2. Registering/UnRegistering the PHY provider

s/UnRegistering/Unregistering ?

> +
> +PHY provider refers to an entity that implements one or more PHY instances.
> +For the simple case where the PHY provider implements only a single instance of
> +the PHY, the framework provides its own implementation of of_xlate in
> +of_phy_simple_xlate. If the PHY provider implements multiple instances, it
> +should provide it's own implementation of of_xlate. of_xlate is used only for

s/it's/its

> +dt boot case.
> +
> +struct phy_provider *of_phy_provider_register(struct device *dev,
> +	struct module *owner, struct phy * (*of_xlate)(struct device *dev,
> +	struct of_phandle_args *args));
> +struct phy_provider *devm_of_phy_provider_register(struct device *dev,
> +	struct module *owner, struct phy * (*of_xlate)(struct device *dev,
> +	struct of_phandle_args *args))
> +
> +of_phy_provider_register and devm_of_phy_provider_register can be used to
> +register the phy_provider and it takes device, owner and of_xlate as
> +arguments. For the dt boot case, all PHY providers should use one of the above
> +2 APIs to register the PHY provider.
> +
> +void devm_of_phy_provider_unregister(struct device *dev,
> +	struct phy_provider *phy_provider);
> +void of_phy_provider_unregister(struct phy_provider *phy_provider);
> +
> +devm_of_phy_provider_unregister and of_phy_provider_unregister can be used to
> +unregister the PHY.
> +
> +3. Creating the PHY
> +
> +The PHY driver should create the PHY in order for other peripheral controllers
> +to make use of it. The PHY framework provides 2 APIs to create the PHY.
> +
> +struct phy *phy_create(struct device *dev, int id, const struct phy_ops *ops,
> +	void *priv);
> +struct phy *devm_phy_create(struct device *dev, int id,
> +	const struct phy_ops *ops, void *priv);
> +
> +The PHY drivers can use one of the above 2 APIs to create the PHY by passing
> +the device pointer, id, phy ops and a driver data.
> +phy_ops is a set of function pointers for performing PHY operations such as
> +init, exit, power_on and power_off.
> +
> +4. Getting a reference to the PHY
> +
> +Before the controller can make use of the PHY, it has to get a reference to
> +it. This framework provides the following APIs to get a reference to the PHY.
> +
> +struct phy *phy_get(struct device *dev, const char *string);
> +struct phy *devm_phy_get(struct device *dev, const char *string);
> +
> +phy_get and devm_phy_get can be used to get the PHY. In the case of dt boot,
> +the string arguments should contain the phy name as given in the dt data and
> +in the case of non-dt boot, it should contain the device name of the PHY.

How about allowing null strings ? At least it seems not difficult for dt 
boot.
But there would have been some asymmetry, for non-dt an error would have to
be returned when the passed string is null.

> +The only difference between the two APIs is that devm_phy_get associates the
> +device with the PHY using devres on successful PHY get. On driver detach,
> +release function is invoked on the the devres data and devres data is freed.
> +
> +5. Releasing a reference to the PHY
> +
> +When the controller no longer needs the PHY, it has to release the reference
> +to the PHY it has obtained using the APIs mentioned in the above section. The
> +PHY framework provides 2 APIS to release a reference to the PHY.

APIs ?

> +void phy_put(struct phy *phy);
> +void devm_phy_put(struct device *dev, struct phy *phy);
> +
> +Both these APIs are used to release a reference to the PHY and devm_phy_put
> +destroys the devres associated with this PHY.
> +
> +6. Destroying the PHY
> +
> +When the driver that created the PHY is unloaded, it should destroy the PHY it
> +created using one of the following 2 APIs.
> +
> +void phy_destroy(struct phy *phy);
> +void devm_phy_destroy(struct device *dev, struct phy *phy);
> +
> +Both these APIs destroys the PHY and devm_phy_destroy destroys the devres

s/APIs destroys/APIs destroy ?

> +associated with this PHY.
> +
> +7. PM Runtime
> +
> +This subsystem is pm runtime enabled. So while creating the PHY,
> +pm_runtime_enable of the phy device created by this subsystem is called and
> +while destroying the PHY, pm_runtime_disable is called. Note that the phy
> +device created by this subsystem will be a child of the device that calls
> +phy_create (PHY provider device).
> +
> +During phy_init, the clocks are enabled by calling get_sync and the clocks are

I think, if you mean pm_runtime_get_sync(), it should be said precisely 
here.

> +disable by calling put_sync during phy_exit. get_sync of the phy_device

Ditto.

s/disable/disabled

> +created by this susbsystem will invoke get_sync of PHY provider device because

s/susbsystem/subsystem

> +of parent-child relationship. For all other pm operations, there are exported
> +APIs like phy_pm_runtime_get, phy_pm_runtime_get_sync, phy_pm_runtime_put,
> +phy_pm_runtime_put_sync, phy_pm_runtime_allow and phy_pm_runtime_forbid.
> +
> +8. DeviceTree Binding
> +
> +The documentation for PHY dt binding can be found @
> +Documentation/devicetree/bindings/phy/phy-bindings.txt

> +/**
> + * phy_release() - release the phy
> + * @dev: the dev member within phy
> + *
> + * when the last reference to the device is removed; it is called

s/when/When
s/removed;/removed ?

> + * from the embedded kobject as release method.
> + */

> +/**
> + * struct phy - represent the phy device

s/represent/represents ?

> + * @dev: phy device
> + * @id: id of the phy
> + * @ops: function pointers for performing phy operations
> + */
> +struct phy {
> +	struct device		dev;
> +	int			id;
> +	const struct phy_ops	*ops;
> +};
> +
> +/**
> + * struct phy_provider - represent the phy provider

s/represent/represents ?

> + * @dev: phy provider device
> + * @owner: the module owner having of_xlate
> + * @of_xlate: function pointer to obtain phy instance from phy pointer
> + * @list: to maintain a linked list of PHY provider

s/provider/providers ?

> + */
> +struct phy_provider {
> +	struct device		*dev;
> +	struct module		*owner;
> +	struct list_head	list;
> +	struct phy * (*of_xlate)(struct device *dev,
> +		struct of_phandle_args *args);
> +};

Regards,
Sylwester