Re: [PATCH v4 1/6] drivers: phy: add generic PHY framework

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Just couple minor comments...

On 03/28/2013 06:43 AM, Kishon Vijay Abraham I wrote:
> The PHY framework provides a set of APIs for the PHY drivers to
> create/destroy a PHY and APIs for the PHY users to obtain a reference to the
> PHY with or without using phandle. To obtain a reference to the PHY without
> using phandle, the platform specfic intialization code (say from board file)
> should have already called phy_bind with the binding information. The binding
> information consists of phy's device name, phy user device name and an index.
> The index is used when the same phy user binds to mulitple phys.
>
> PHY drivers should create the PHY by passing phy_descriptor that has
> describes the PHY (label, type etc..) and ops like init, exit, suspend, resume,
> poweron, shutdown.
>
> The documentation for the generic PHY framework is added in
> Documentation/phy.txt and the documentation for the sysfs entry is added
> in Documentation/ABI/testing/sysfs-class-phy and the documentation for
> dt binding is can be found at
> Documentation/devicetree/bindings/phy/phy-bindings.txt
>
> Signed-off-by: Kishon Vijay Abraham I<kishon@ti.com>
> ---
[...]
> +/**
> + * phy_put - release the PHY

nit: According to kernel-doc documentation function names should have
parentheses appended to the name. So it would need to be

+ * phy_put() - release the PHY

in this case and it applies to multiple places elsewhere in this patch.

> + * @phy: the phy returned by phy_get()
> + *
> + * Releases a refcount the caller received from phy_get().
> + */
> +void phy_put(struct phy *phy)
> +{
> +	if (phy) {
> +		module_put(phy->ops->owner);
> +		put_device(&phy->dev);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(phy_put);
[...]
> +/**
> + * devm_of_phy_get_byname - lookup and obtain a reference to a phy by name
> + * @dev: device that requests this phy
> + * @string - the phy name as given in the dt data

s/ -/:

> + *
> + * Calls devm_of_phy_get (which associates the device with the phy using devres
> + * on successful phy get) and passes on the return value of devm_of_phy_get.
> + */
> +struct phy *devm_of_phy_get_byname(struct device *dev, const char *string)
> +{
> +	int index;
> +
> +	if (!dev->of_node) {
> +		dev_dbg(dev, "device does not have a device node entry\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	index = of_property_match_string(dev->of_node, "phy-names", string);
> +
> +	return devm_of_phy_get(dev, index);
> +}
> +EXPORT_SYMBOL_GPL(devm_of_phy_get_byname);
> +
> +/**
> + * phy_get - lookup and obtain a reference to a phy.
> + * @dev: device that requests this phy
> + * @index: the index of the phy
> + *
> + * Returns the phy driver, after getting a refcount to it; or
> + * -ENODEV if there is no such phy.  The caller is responsible for
> + * calling phy_put() to release that count.
> + */
> +struct phy *phy_get(struct device *dev, int index)
> +{
> +	struct phy *phy = NULL;

Unneeded initialization.

> +	phy = phy_lookup(dev, index);
> +	if (IS_ERR(phy)) {
> +		dev_err(dev, "unable to find phy\n");
> +		goto err0;

Wouldn't it be better to just do:

		return phy;
> +	}
> +
> +	if (!try_module_get(phy->ops->owner)) {
> +		phy = ERR_PTR(-EPROBE_DEFER);

and
		return ERR_PTR(-EPROBE_DEFER);

> +		goto err0;

and to drop this goto and the label below ?

> +	}
> +
> +	get_device(&phy->dev);
> +
> +err0:
> +	return phy;
> +}
> +EXPORT_SYMBOL_GPL(phy_get);

> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> new file mode 100644
> index 0000000..0cb2298
> --- /dev/null
> +++ b/include/linux/phy/phy.h
> @@ -0,0 +1,237 @@
> +/*
> + * phy.h -- generic phy header file
[...]
> +#ifndef __DRIVERS_PHY_H
> +#define __DRIVERS_PHY_H
> +
> +#include<linux/err.h>
> +#include<linux/of.h>
> +
> +struct phy

I think you also need to add either

#include <linux/device.h>

or

struct device;

struct device * is used further in this file.

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

Could these be named power_on/power_off ? Looks a bit more symmetric
to me that way.

> + * @owner: the module owner containing the ops
> + */
> +struct phy_ops {
> +	int	(*init)(struct phy *phy);
> +	int	(*exit)(struct phy *phy);
> +	int	(*suspend)(struct phy *phy);
> +	int	(*resume)(struct phy *phy);
> +	int	(*poweron)(struct phy *phy);
> +	int	(*shutdown)(struct phy *phy);
> +	struct module *owner;
> +};

Thanks,
Sylwester