Just couple minor comments...
On 03/28/2013 06:43 AM, 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. To obtain a reference to the PHY without
using phandle, the platform specfic intialization code (say from board file)
should have already called phy_bind with the binding information. The binding
information consists of phy's device name, phy user device name and an index.
The index is used when the same phy user binds to mulitple phys.
PHY drivers should create the PHY by passing phy_descriptor that has
describes the PHY (label, type etc..) and ops like init, exit, suspend, resume,
poweron, shutdown.
The documentation for the generic PHY framework is added in
Documentation/phy.txt and the documentation for the sysfs entry is added
in Documentation/ABI/testing/sysfs-class-phy and the documentation for
dt binding is can be found at
Documentation/devicetree/bindings/phy/phy-bindings.txt
Signed-off-by: Kishon Vijay Abraham I<kishon@xxxxxx>
---
[...]
+/**
+ * phy_put - release the PHY
nit: According to kernel-doc documentation function names should have
parentheses appended to the name. So it would need to be
+ * phy_put() - release the PHY
in this case and it applies to multiple places elsewhere in this patch.
+ * @phy: the phy returned by phy_get()
+ *
+ * Releases a refcount the caller received from phy_get().
+ */
+void phy_put(struct phy *phy)
+{
+ if (phy) {
+ module_put(phy->ops->owner);
+ put_device(&phy->dev);
+ }
+}
+EXPORT_SYMBOL_GPL(phy_put);
[...]
+/**
+ * devm_of_phy_get_byname - lookup and obtain a reference to a phy by name
+ * @dev: device that requests this phy
+ * @string - the phy name as given in the dt data
s/ -/:
+ *
+ * Calls devm_of_phy_get (which associates the device with the phy using devres
+ * on successful phy get) and passes on the return value of devm_of_phy_get.
+ */
+struct phy *devm_of_phy_get_byname(struct device *dev, const char *string)
+{
+ int index;
+
+ if (!dev->of_node) {
+ dev_dbg(dev, "device does not have a device node entry\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ index = of_property_match_string(dev->of_node, "phy-names", string);
+
+ return devm_of_phy_get(dev, index);
+}
+EXPORT_SYMBOL_GPL(devm_of_phy_get_byname);
+
+/**
+ * phy_get - lookup and obtain a reference to a phy.
+ * @dev: device that requests this phy
+ * @index: the index of the phy
+ *
+ * Returns the phy driver, after getting a refcount to it; or
+ * -ENODEV if there is no such phy. The caller is responsible for
+ * calling phy_put() to release that count.
+ */
+struct phy *phy_get(struct device *dev, int index)
+{
+ struct phy *phy = NULL;
Unneeded initialization.
+ phy = phy_lookup(dev, index);
+ if (IS_ERR(phy)) {
+ dev_err(dev, "unable to find phy\n");
+ goto err0;
Wouldn't it be better to just do:
return phy;
+ }
+
+ if (!try_module_get(phy->ops->owner)) {
+ phy = ERR_PTR(-EPROBE_DEFER);
and
return ERR_PTR(-EPROBE_DEFER);
+ goto err0;
and to drop this goto and the label below ?
+ }
+
+ get_device(&phy->dev);
+
+err0:
+ return phy;
+}
+EXPORT_SYMBOL_GPL(phy_get);
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
new file mode 100644
index 0000000..0cb2298
--- /dev/null
+++ b/include/linux/phy/phy.h
@@ -0,0 +1,237 @@
+/*
+ * phy.h -- generic phy header file
[...]
+#ifndef __DRIVERS_PHY_H
+#define __DRIVERS_PHY_H
+
+#include<linux/err.h>
+#include<linux/of.h>
+
+struct phy
I think you also need to add either
#include <linux/device.h>
or
struct device;
struct device * is used further in this file.
+/**
+ * struct phy_ops - set of function pointers for performing phy operations
+ * @init: operation to be performed for initializing phy
+ * @exit: operation to be performed while exiting
+ * @suspend: suspending the phy
+ * @resume: resuming the phy
+ * @poweron: powering on the phy
+ * @shutdown: shutting down the phy
Could these be named power_on/power_off ? Looks a bit more symmetric
to me that way.
+ * @owner: the module owner containing the ops
+ */
+struct phy_ops {
+ int (*init)(struct phy *phy);
+ int (*exit)(struct phy *phy);
+ int (*suspend)(struct phy *phy);
+ int (*resume)(struct phy *phy);
+ int (*poweron)(struct phy *phy);
+ int (*shutdown)(struct phy *phy);
+ struct module *owner;
+};
Thanks,
Sylwester
--
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
