Re: [PATCH v4 1/6] drivers: phy: add generic PHY framework

From: Kishon Vijay Abraham I
Date: Tue Apr 02 2013 - 04:40:51 EST

Hi,

On Tuesday 02 April 2013 01:04 AM, Sylwester Nawrocki wrote:
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.

Will fix it.

+ * @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/ -/:

Ok.

+ *
+ * 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:

Indeed.

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.

Ok.

+/**
+ * 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.

Ok. Will have it that way.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/