Re: [PATCH v2 1/5] drivers: phy: add generic PHY framework

[Arnd Bergmann <arnd@arndb.de>]

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