Hi,
On Tuesday 19 February 2013 06:26 PM, Arnd Bergmann wrote:
On Tuesday 19 February 2013, Kishon Vijay Abraham I wrote:
+static struct class *phy_class;
+static LIST_HEAD(phy_list);
+static DEFINE_MUTEX(phy_list_mutex);
+static LIST_HEAD(phy_bind_list);
Hmm, so you actually do have a 'class'. There is a GregKH mandated ban on
new classes, meaning that one should be converted to a bus_type instead.
Also, you really should not need the global phy_list, phy_list_mutex
and phy_bind_list variables, since the driver core already provides
you with ways to iterate through devices on a class or bus.
ok.
+/**
+ * of_phy_get - lookup and obtain a reference to a phy by phandle
+ * @dev: device that requests this phy
+ * @phandle: name of the property holding the phy phandle value
+ * @index - the index of the phy
+ *
+ * Returns the phy associated with the given phandle value,
+ * after getting a refcount to it or -ENODEV if there is no such phy or
+ * -EPROBE_DEFER if there is a phandle to the phy, but the device is
+ * not yet loaded.
+ */
+struct phy *of_phy_get(struct device *dev, const char *phandle, u8 index)
+{
+ struct phy *phy = NULL;
+ struct phy_bind *phy_map = NULL;
+ struct device_node *node;
+
+ if (!dev->of_node) {
+ dev_dbg(dev, "device does not have a device node entry\n");
+ return ERR_PTR(-EINVAL);
+ }
+
+ node = of_parse_phandle(dev->of_node, phandle, index);
+ if (!node) {
+ dev_dbg(dev, "failed to get %s phandle in %s node\n", phandle,
+ dev->of_node->full_name);
+ return ERR_PTR(-ENODEV);
+ }
I wonder whether this one should be of_parse_phandle_with_args() instead,
so you can have client-specific configuration in the property. Basically
instead of
phy = <&usbphy0 &usbphy1>;
you can pass an arbitrary number of arguments along with this, as
determined by some property in the phy node:
usbphy0: phy@10000 {
#phy-cells = <1>;
};
ehci@20000 {
phy = <&usbphy0 23>;
};
Which in turn leads to the argument (23) being passed into a phy_bind().
I also wonder if you should allow arbitrary names for the property.
Can't this always be called 'phy'? If you allow named phys, it would
more more consistent with other bindings to have a list of phy handles
in a property called "phy", and a second property called "phy-names"
that contains the named strings.
Ok. Makes sense. We should make both *phy* and *phy-cells* standard here.
+/**
+ * phy_create - create a new phy
+ * @dev: device that is creating the new phy
+ * @desc: descriptor of the phy
+ *
+ * Called to create a phy using phy framework.
+ */
+struct phy *phy_create(struct device *dev, struct phy_descriptor *desc)
+{
+ int ret;
+ struct phy *phy;
+ struct phy_bind *phy_bind;
+ const char *devname = NULL;
...
+
+ devname = dev_name(dev);
+ device_initialize(&phy->dev);
+ phy->desc = desc;
+ phy->dev.class = phy_class;
+ phy->dev.parent = dev;
+ phy->dev.bus = desc->bus;
+ ret = dev_set_name(&phy->dev, "%s", devname);
Passing a bus_type through the descriptor seems misplaced. What is this for?
I thought if we are adding ethernet phys here (say drivers/phy/net), we
can make phy_device_create() (currently in drivers/net/phy/phy_device.c)
call phy_create with bus_type set to mdio_bus_type. Then we can have all
the PHYs showing up in /sys/class/phy/ and ethernet can continue to use
its own phy abstraction layer.
Why is this function not just:
struct phy *phy_create(struct device *dev, const char *label, int type,
struct phy_ops *ops);
since while calling the callback functions using ops, there wont be
anyway to get back the device specific structure pointer.
struct phy_dev {
.
.
struct phy_descriptor desc;
void __iomem *base;
.
.
};
static int phy_resume(struct phy_descriptor *desc)
{
//if we dont pass a member of phy_dev while *phy_create* we can't get
back phy_dev from callback functions as used below.
struct phy_dev *phy = desc_to_omapusb(desc);
return 0;
}
static struct phy_ops ops = {
.resume = phy_resume,
.owner = THIS_MODULE,
};
Passing a structure like you do here seems dangerous because when someone
decides to add members to the structure, existing code will not give a
build error but silently break.
Not sure I understood this point. Care to explain?
+/**
+ * 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
+ * @owner: the module owner containing the ops
+ */
+struct phy_ops {
+ int (*init)(struct phy_descriptor *desc);
+ int (*exit)(struct phy_descriptor *desc);
+ int (*suspend)(struct phy_descriptor *desc);
+ int (*resume)(struct phy_descriptor *desc);
+ int (*poweron)(struct phy_descriptor *desc);
+ int (*shutdown)(struct phy_descriptor *desc);
+ struct module *owner;
+};
Shouldn't these take the 'struct phy' as an argument? struct phy_descriptor is
not even based on a 'struct device'.
I actually used struct phy_descriptor for the reason mentioned above.
Thanks a lot for reviewing.
Regards
Kishon
--
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
