On Tuesday 19 February 2013, kishon wrote:
> >> +
> >> + 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.
Hmm, that relies on the fact that mdio uses a 'bus_type' while the new phy
support uses a 'class', and it will break if we ever get to the point
where those two concepts are merged. I would rather not plan ahead here.
> > 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,
> };
In other subsystems, that is what the device->private_data pointer is used
for, which you could also pass to phy_create, or set after calling that
function.
> > 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?
Nevermind, when I wrote that sentence, I had not yet noticed that the
phy_descriptor is kept around. I was thinking that the structure was
only used to pass more arguments into phy_create.
Arnd
--
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
