Re: [PATCH] drivers: phy: add generic PHY framework

[Joe Perches <joe@perches.com>]

On Wed, 2012-09-26 at 20:31 +0530, Kishon Vijay Abraham I wrote:
> The PHY framework provides a set of API's for the PHY drivers to
> create/destroy a PHY and API's 

Just some trivial notes.

> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
[]
> @@ -0,0 +1,445 @@
[]
> +static void devm_phy_release(struct device *dev, void *res)
> +{
> +	struct phy *phy = *(struct phy **)res;

That's a bit twisted isn't it?
Perhaps
	struct phy *phy = res;

[]

> +static int devm_phy_match(struct device *dev, void *res, void *match_data)
> +{
> +	return res == match_data;

static bool devm_phy_match(etc...)

[]

> +struct phy *devm_of_phy_get(struct device *dev, const char *phandle, u8 index)
> +{
> +	struct phy *phy = NULL, **ptr;
> +	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);
> +	}
> +
> +	ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);

Is this the right size?

Because ptr is **, perhaps sizeof(struct phy) is clearer.

> +	if (!ptr) {
> +		dev_dbg(dev, "failed to allocate memory for devres\n");

alloc failures generally don't need specific OOM messages
as the general alloc OOM dumps stack.

> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	mutex_lock(&phy_list_mutex);
> +
> +	phy = of_phy_lookup(dev, node);
> +	if (IS_ERR(phy) || !phy->dev.class ||
> +			!try_module_get(phy->dev.class->owner)) {

I think it's tasteful coding style to align like:

	if (IS_ERR(phy) || !phy->dev.class ||
	    !try_module_get(phy->dev.class->owner)) {
[]

> +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;
> +
> +	if (!dev || !desc) {
> +		dev_err(dev, "no descriptor/device provided for PHY\n");
> +		ret = -EINVAL;
> +		goto err0;
> +	}
> +
> +	if (!desc->ops) {
> +		dev_err(dev, "no PHY ops provided\n");
> +		ret = -EINVAL;
> +		goto err0;
> +	}
> +
> +	phy = kzalloc(sizeof(*phy), GFP_KERNEL);
> +	if (!phy) {
> +		dev_err(dev, "no memory for PHY\n");

No OOM message required...

[]

> +static int __init phy_core_init(void)
> +{
> +	phy_class = class_create(THIS_MODULE, "phy");
> +	if (IS_ERR(phy_class)) {
> +		pr_err("failed to create phy class --> %ld\n",
> +			PTR_ERR(phy_class));

You might add #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
before any include so that pr_<levels> are prefixed.

cheers, Joe