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

[kishon]

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-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/