Re: [PATCH v7 1/9] drivers: phy: add generic PHY framework
On Wednesday 19 June 2013 02:52 AM, Sylwester Nawrocki wrote: Hi Kishon, I've noticed there is a little inconsistency between the code and documentation. On 06/13/2013 10:43 AM, Kishon Vijay Abraham I wrote: +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 'label' argument is missing in those function prototypes. It seems the labels must be unique, I guess this needs to be documented. And do we allow NULL labels ? If so, then there is probably a check for NULL label needed in phy_lookup() and if not, then phy_create() should fail when NULL label is passed ? Yeah. Label is used only for non-dt case so NULL should be allowed while phy_create(). Thanks Kishon ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 1/9] drivers: phy: add generic PHY framework
Hi,
On 06/13/2013 10:43 AM, Kishon Vijay Abraham I wrote:
+/**
+ * phy_create() - create a new phy
+ * @dev: device that is creating the new phy
+ * @id: id of the phy
+ * @ops: function pointers for performing phy operations
+ * @label: label given to the phy
+ * @priv: private data from phy driver
+ *
+ * Called to create a phy using phy framework.
+ */
+struct phy *phy_create(struct device *dev, u8 id, const struct phy_ops *ops,
+ const char *label, void *priv)
+{
+ int ret;
+ struct phy *phy;
+
+ if (!dev) {
+ dev_WARN(dev, no device provided for PHY\n);
+ ret = -EINVAL;
+ goto err0;
+ }
+
+ phy = kzalloc(sizeof(*phy), GFP_KERNEL);
+ if (!phy) {
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ device_initialize(phy-dev);
+
+ phy-dev.class = phy_class;
+ phy-dev.parent = dev;
+ phy-dev.of_node = dev-of_node;
+ phy-id = id;
+ phy-ops = ops;
+ phy-label = label;
Perhaps we could make it:
phy-label = kstrdup(label, GFP_KERNEL);
and free the label in phy_destroy() ?
That way the users would't need to keep the label around. It might
be useful when PHY provider generates the labels dynamically. I guess
it is OK for PHY providers to hard code the labels and having the PHY
user drivers passed labels in platform data ?
+ dev_set_drvdata(phy-dev, priv);
+
+ ret = dev_set_name(phy-dev, %s.%d, dev_name(dev), id);
+ if (ret)
+ goto err1;
+
+ ret = device_add(phy-dev);
+ if (ret)
+ goto err1;
+
+ if (pm_runtime_enabled(dev))
+ pm_runtime_enable(phy-dev);
+
+ return phy;
+
+err1:
+ put_device(phy-dev);
+ kfree(phy);
+
+err0:
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(phy_create);
+/**
+ * phy_destroy() - destroy the phy
+ * @phy: the phy to be destroyed
+ *
+ * Called to destroy the phy.
+ */
+void phy_destroy(struct phy *phy)
+{
+ pm_runtime_disable(phy-dev);
+ device_unregister(phy-dev);
Isn't kfree(phy); missing here ?
+}
+EXPORT_SYMBOL_GPL(phy_destroy);
Thanks,
Sylwester
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 1/9] drivers: phy: add generic PHY framework
Hi,
On Wednesday 19 June 2013 09:19 PM, Sylwester Nawrocki wrote:
Hi,
On 06/13/2013 10:43 AM, Kishon Vijay Abraham I wrote:
+/**
+ * phy_create() - create a new phy
+ * @dev: device that is creating the new phy
+ * @id: id of the phy
+ * @ops: function pointers for performing phy operations
+ * @label: label given to the phy
+ * @priv: private data from phy driver
+ *
+ * Called to create a phy using phy framework.
+ */
+struct phy *phy_create(struct device *dev, u8 id, const struct phy_ops *ops,
+ const char *label, void *priv)
+{
+ int ret;
+ struct phy *phy;
+
+ if (!dev) {
+ dev_WARN(dev, no device provided for PHY\n);
+ ret = -EINVAL;
+ goto err0;
+ }
+
+ phy = kzalloc(sizeof(*phy), GFP_KERNEL);
+ if (!phy) {
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ device_initialize(phy-dev);
+
+ phy-dev.class = phy_class;
+ phy-dev.parent = dev;
+ phy-dev.of_node = dev-of_node;
+ phy-id = id;
+ phy-ops = ops;
+ phy-label = label;
Perhaps we could make it:
phy-label = kstrdup(label, GFP_KERNEL);
and free the label in phy_destroy() ?
yeah.. Fixed.
That way the users would't need to keep the label around. It might
be useful when PHY provider generates the labels dynamically. I guess
it is OK for PHY providers to hard code the labels and having the PHY
user drivers passed labels in platform data ?
yeah. But the only concern here is there is no way to enforce the
labels are passed in platform data. The PHY user driver can also
hard code the labels while getting the reference to the PHY and we can
catch such cases only by review.
+ dev_set_drvdata(phy-dev, priv);
+
+ ret = dev_set_name(phy-dev, %s.%d, dev_name(dev), id);
+ if (ret)
+ goto err1;
+
+ ret = device_add(phy-dev);
+ if (ret)
+ goto err1;
+
+ if (pm_runtime_enabled(dev))
+ pm_runtime_enable(phy-dev);
+
+ return phy;
+
+err1:
+ put_device(phy-dev);
+ kfree(phy);
+
+err0:
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(phy_create);
+/**
+ * phy_destroy() - destroy the phy
+ * @phy: the phy to be destroyed
+ *
+ * Called to destroy the phy.
+ */
+void phy_destroy(struct phy *phy)
+{
+ pm_runtime_disable(phy-dev);
+ device_unregister(phy-dev);
Isn't kfree(phy); missing here ?
Not actually. Its done in phy_release (class's dev_release method)
Thanks
Kishon
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 1/9] drivers: phy: add generic PHY framework
Hi,
On Thu, Jun 13, 2013 at 02:13:51PM +0530, Kishon Vijay Abraham I wrote:
+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))
I would rename this to __of_phy_provider_register() and in a header
have:
#define of_phy_provider_register(dev, xlate)\
__of_phy_provider_register((dev), THIS_MODULE, (xlate))
then your users don't need to remember always passing THIS_MODULE
argument.
+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))
likewise.
+/**
+ * phy_release() - release the phy
+ * @dev: the dev member within phy
+ *
+ * When the last reference to the device is removed, it is called
+ * from the embedded kobject as release method.
+ */
+static void phy_release(struct device *dev)
+{
+ struct phy *phy;
+
+ phy = container_of(dev, struct phy, dev);
I would have a:
#define to_phy(dev) (container_of((dev), struct phy dev))
somewhere in a header, just for the sake of brevity :-p
+ dev_dbg(dev, releasing '%s'\n, dev_name(dev));
make it dev_vdbg() instead.
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
new file mode 100644
index 000..df0c98a
--- /dev/null
+++ b/include/linux/phy/phy.h
[ ... ]
+static inline int phy_init(struct phy *phy)
+{
+ int ret = -ENOTSUPP;
+
+ if (!pm_runtime_enabled(phy-dev))
+ goto no_pm_runtime;
+
+ ret = pm_runtime_get_sync(phy-dev);
+ if (ret 0) {
+ dev_err(phy-dev, get sync failed: %d\n, ret);
+ return ret;
+ }
+
+no_pm_runtime:
you can avoid this goto if you code as below, note that I'm also solving
a possible unbalanced PM runtime call which would prevent actual
*runtime* suspend of the PHY:
ret = phy_pm_runtime_get_sync(phy);
if (ret 0 ret != -ENOTSUPP)
return ret;
if (phy-ops-init) {
ret = phy-ops-init(phy);
if (ret 0) {
dev_err(phy-dev, failed to initialize -- %d\n,
ret);
goto out;
}
}
out:
/*
* In order to avoid unbalanced PM runtime calls, let's make
* sure to disable what we might have enabled when entering this
* function.
*/
phy_pm_runtime_put(phy);
return ret;
+}
+
+static inline int phy_exit(struct phy *phy)
+{
+ int ret = -ENOTSUPP;
+
+ if (phy-ops-exit)
+ ret = phy-ops-exit(phy);
+
+ if (!pm_runtime_enabled(phy-dev))
+ goto no_pm_runtime;
+
+ ret = pm_runtime_put_sync(phy-dev);
move your pm runtime wrappers before these calls and you can use them.
--
balbi
signature.asc
Description: Digital signature
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v7 1/9] drivers: phy: add generic PHY framework
Hi Kishon,
I've noticed there is a little inconsistency between the code and
documentation.
On 06/13/2013 10:43 AM, Kishon Vijay Abraham I wrote:
+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 'label' argument is missing in those function prototypes. It seems the
labels must be unique, I guess this needs to be documented. And do we allow
NULL labels ? If so, then there is probably a check for NULL label needed
in phy_lookup() and if not, then phy_create() should fail when NULL label
is passed ?
+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.
+/**
+ * phy_create() - create a new phy
+ * @dev: device that is creating the new phy
+ * @id: id of the phy
+ * @ops: function pointers for performing phy operations
+ * @label: label given to the phy
+ * @priv: private data from phy driver
+ *
+ * Called to create a phy using phy framework.
+ */
+struct phy *phy_create(struct device *dev, u8 id, const struct phy_ops *ops,
+ const char *label, void *priv)
+{
+ int ret;
+ struct phy *phy;
+
+ if (!dev) {
+ dev_WARN(dev, no device provided for PHY\n);
+ ret = -EINVAL;
+ goto err0;
+ }
+
+ phy = kzalloc(sizeof(*phy), GFP_KERNEL);
+ if (!phy) {
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ device_initialize(phy-dev);
+
+ phy-dev.class = phy_class;
+ phy-dev.parent = dev;
+ phy-dev.of_node = dev-of_node;
+ phy-id = id;
+ phy-ops = ops;
+ phy-label = label;
+
+ dev_set_drvdata(phy-dev, priv);
+
+ ret = dev_set_name(phy-dev, %s.%d, dev_name(dev), id);
+ if (ret)
+ goto err1;
+
+ ret = device_add(phy-dev);
+ if (ret)
+ goto err1;
+
+ if (pm_runtime_enabled(dev))
+ pm_runtime_enable(phy-dev);
+
+ return phy;
+
+err1:
+ put_device(phy-dev);
+ kfree(phy);
+
+err0:
+ return ERR_PTR(ret);
+}
Thanks,
Sylwester
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH v7 1/9] drivers: phy: add generic PHY framework
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 kis...@ti.com
---
.../devicetree/bindings/phy/phy-bindings.txt | 66 +++
Documentation/phy.txt | 124 +
MAINTAINERS|7 +
drivers/Kconfig|2 +
drivers/Makefile |2 +
drivers/phy/Kconfig| 13 +
drivers/phy/Makefile |5 +
drivers/phy/phy-core.c | 543
include/linux/phy/phy.h| 284 ++
9 files changed, 1046 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 000..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 000..059ef0e
--- /dev/null
+++ b/Documentation/phy.txt
@@ -0,0 +1,124 @@
+ PHY SUBSYSTEM
+ Kishon Vijay Abraham I kis...@ti.com
+
+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
+controllers have PHY functionality embedded into it and others use an external
+PHY. Other peripherals that use PHY include Wireless LAN, Ethernet,
+SATA etc.
+
+The intention of creating this framework is to bring the PHY drivers spread
+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 use external PHY (PHY
+functionality is not embedded within the controller).
+
+2. Registering/Unregistering the PHY provider
+
+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 its own
