[RFC] drivers: phy: add generic PHY framework

Message ID	1347623899-14485-1-git-send-email-kishon@ti.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-omap-owner@vger.kernel.org> From: Kishon Vijay Abraham I <kishon@ti.com> To: <balbi@ti.com>, <arnd@arndb.de>, <gregkh@linuxfoundation.org>, <linux-omap@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-usb@vger.kernel.org> CC: <richard.zhao@freescale.com>, <B29397@freescale.com>, <alexander.shishkin@linux.intel.com>, <mkl@pengutronix.de>, <marex@denx.de>, Kishon Vijay Abraham I <kishon@ti.com> Subject: [RFC PATCH] drivers: phy: add generic PHY framework Date: Fri, 14 Sep 2012 17:28:19 +0530 Message-ID: <1347623899-14485-1-git-send-email-kishon@ti.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-omap-owner@vger.kernel.org Precedence: bulk

Kishon Vijay Abraham I Sept. 14, 2012, 11:58 a.m. UTC

The PHY framework provides a set of API's for the PHY drivers to
create/remove a PHY and the PHY users to obtain a reference to the PHY
using or without using phandle. If the PHY users has 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
information about the PHY and ops like init, exit, suspend, resume,
poweron, shutdown.

Nyet-signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
This framework is actually intended to be used by all the PHY drivers in the
kernel. Though it's going to take a while for that, I intend to migrate
existing USB/OTG phy drivers to use this framework as we align on the design
of this framework. Once I migrate these phy drivers, I'll be able to test this
framework (I haven't tested this framework so far). I sent this patch early
so as to get review comments and align on the design. Thanks :-)

 drivers/Kconfig         |    2 +
 drivers/Makefile        |    2 +
 drivers/phy/Kconfig     |   13 ++
 drivers/phy/Makefile    |    5 +
 drivers/phy/phy-core.c  |  437 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/phy/phy.h |  181 ++++++++++++++++++++
 6 files changed, 640 insertions(+)
 create mode 100644 drivers/phy/Kconfig
 create mode 100644 drivers/phy/Makefile
 create mode 100644 drivers/phy/phy-core.c
 create mode 100644 include/linux/phy/phy.h

Marc Kleine-Budde Sept. 14, 2012, 12:28 p.m. UTC | #1

On 09/14/2012 01:58 PM, Kishon Vijay Abraham I wrote:
> The PHY framework provides a set of API's for the PHY drivers to
> create/remove a PHY and the PHY users to obtain a reference to the PHY
> using or without using phandle. If the PHY users has 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
> information about the PHY and ops like init, exit, suspend, resume,
> poweron, shutdown.

Some comments inside.

While looking over the code, I was thinking why not abstract the phy
with a "bus" in the linux kernel. The ethernet phys are on the mdio_bus,
see /sys/bus/mdio_bus. This saves you hand crafting devices, drivers and
bindings,

Marc

> 
> Nyet-signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
> This framework is actually intended to be used by all the PHY drivers in the
> kernel. Though it's going to take a while for that, I intend to migrate
> existing USB/OTG phy drivers to use this framework as we align on the design
> of this framework. Once I migrate these phy drivers, I'll be able to test this
> framework (I haven't tested this framework so far). I sent this patch early
> so as to get review comments and align on the design. Thanks :-)
> 
>  drivers/Kconfig         |    2 +
>  drivers/Makefile        |    2 +
>  drivers/phy/Kconfig     |   13 ++
>  drivers/phy/Makefile    |    5 +
>  drivers/phy/phy-core.c  |  437 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/phy/phy.h |  181 ++++++++++++++++++++
>  6 files changed, 640 insertions(+)
>  create mode 100644 drivers/phy/Kconfig
>  create mode 100644 drivers/phy/Makefile
>  create mode 100644 drivers/phy/phy-core.c
>  create mode 100644 include/linux/phy/phy.h
> 
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index ece958d..8488818 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -152,4 +152,6 @@ source "drivers/vme/Kconfig"
>  
>  source "drivers/pwm/Kconfig"
>  
> +source "drivers/phy/Kconfig"
> +
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 5b42184..63d6bbe 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -38,6 +38,8 @@ obj-y				+= char/
>  # gpu/ comes after char for AGP vs DRM startup
>  obj-y				+= gpu/
>  
> +obj-y				+= phy/
> +
>  obj-$(CONFIG_CONNECTOR)		+= connector/
>  
>  # i810fb and intelfb depend on char/agp/
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> new file mode 100644
> index 0000000..34f7077
> --- /dev/null
> +++ b/drivers/phy/Kconfig
> @@ -0,0 +1,13 @@
> +#
> +# PHY
> +#
> +
> +menuconfig GENERIC_PHY
> +	tristate "Generic PHY Support"
> +	help
> +	  Generic PHY support.
> +
> +	  This framework is designed to provide a generic interface for PHY
> +	  devices present in the kernel. This layer will have the generic
> +	  API by which phy drivers can create PHY using the phy framework and
> +	  phy users can obtain reference to the PHY.
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> new file mode 100644
> index 0000000..9e9560f
> --- /dev/null
> +++ b/drivers/phy/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for the phy drivers.
> +#
> +
> +obj-$(CONFIG_GENERIC_PHY)	+= phy-core.o
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> new file mode 100644
> index 0000000..c55446a
> --- /dev/null
> +++ b/drivers/phy/phy-core.c
> @@ -0,0 +1,437 @@
> +/*
> + * phy-core.c  --  Generic Phy framework.
> + *
> + * Copyright (C) 2012 Texas Instruments
> + *
> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/export.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/phy/phy.h>
> +
> +static struct class *phy_class;
> +static LIST_HEAD(phy_list);
> +static DEFINE_MUTEX(phy_list_mutex);
> +static LIST_HEAD(phy_bind_list);
> +
> +static void devm_phy_release(struct device *dev, void *res)
> +{
> +	struct phy *phy = *(struct phy **)res;

What about adding a struct phy_res, doing so,m you don't need these
casts, and it's easier to add more pointers if needed.

> +
> +	phy_put(phy);
> +}
> +
> +static int devm_phy_match(struct device *dev, void *res, void *match_data)
> +{
> +	return res == match_data;
> +}
> +
> +static struct phy *phy_lookup(struct device *dev, u8 index)
> +{
> +	struct phy_bind *phy_bind = NULL;
> +
> +	list_for_each_entry(phy_bind, &phy_bind_list, list) {
> +		if (!(strcmp(phy_bind->dev_name, dev_name(dev))) &&
> +				phy_bind->index == index)
> +			return phy_bind->phy;
> +	}
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +static struct phy *of_phy_lookup(struct device *dev, struct device_node *node)
> +{
> +	int index = 0;
> +	struct phy  *phy;
                  ^^

Please remove that stray space.

> +	struct phy_bind *phy_map = NULL;
> +
> +	list_for_each_entry(phy_map, &phy_bind_list, list)
> +		if (!(strcmp(phy_map->dev_name, dev_name(dev))))
> +			index++;
> +
> +	list_for_each_entry(phy, &phy_list, head) {
> +		if (node != phy->desc->of_node)
> +			continue;
> +
> +		phy_map = phy_bind(dev_name(dev), index, dev_name(&phy->dev));
> +		if (!IS_ERR(phy_map)) {
> +			phy_map->phy = phy;
> +			phy_map->auto_bind = true;
> +		}
> +
> +		return phy;
> +	}
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +/**
> + * devm_phy_get - lookup and obtain a reference to a phy.
> + * @dev: device that requests this phy
> + * @index: the index of the phy
> + *
> + * Gets the phy using phy_get(), and associates a device with it using
> + * devres. On driver detach, release function is invoked on the devres data,
> + * then, devres data is freed.
> + */
> +struct phy *devm_phy_get(struct device *dev, u8 index)
> +{
> +	struct phy **ptr, *phy;
> +
> +	ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return NULL;
> +
> +	phy = phy_get(dev, index);
> +	if (!IS_ERR(phy)) {
> +		*ptr = phy;
> +		devres_add(dev, ptr);
> +	} else
> +		devres_free(ptr);

nitpick: when when if has { }, else should have, too.

> +
> +	return phy;
> +}
> +EXPORT_SYMBOL_GPL(devm_phy_get);
> +
> +/**
> + * devm_phy_put - release the PHY
> + * @dev: device that wants to release this phy
> + * @phy: the phy returned by devm_phy_get()
> + *
> + * destroys the devres associated with this phy and invokes phy_put
> + * to release the phy.
> + */
> +void devm_phy_put(struct device *dev, struct phy *phy)
> +{
> +	int r;
> +
> +	r = devres_destroy(dev, devm_phy_release, devm_phy_match, phy);
> +	dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n");
> +}
> +EXPORT_SYMBOL_GPL(devm_phy_put);
> +
> +/**
> + * devm_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
> + *
> + * Returns the phy driver associated with the given phandle value,
> + * after getting a refcount to it or -ENODEV if there is no such phy.
> + * While at that, it also associates the device with the phy using devres.
> + * On driver detach, release function is invoked on the devres data,
> + * then, devres data is freed.
> + */
> +struct phy *devm_of_phy_get(struct device *dev, const char *phandle)

We should discuss first how the DT binding for a phy should look like. I
don't like that you hardcode the index (in of_parse_phandle()) to "0".
Then we have the problem with USB2 and USB3 phys for the same usb device.

> +{
> +	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, 0);
> +	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);
> +	if (!ptr) {
> +		dev_dbg(dev, "failed to allocate memory for devres\n");
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	mutex_lock(&phy_list_mutex);
> +
> +	phy = of_phy_lookup(dev, node);
> +	if (IS_ERR(phy) || !try_module_get(phy->dev.driver->owner)) {
> +		devres_free(ptr);
> +		goto err0;
> +	}

Please return -EPROBE_DEFER is the phy is not yet registered.

> +
> +	*ptr = phy;
> +	devres_add(dev, ptr);
> +
> +	get_device(&phy->dev);
> +
> +err0:
> +	mutex_unlock(&phy_list_mutex);
> +
> +	return phy;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_phy_get);
> +
> +/**
> + * 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, u8 index)
> +{
> +	struct phy	*phy = NULL;
> +
> +	mutex_lock(&phy_list_mutex);
> +
> +	phy = phy_lookup(dev, index);
> +	if (IS_ERR(phy)) {
> +		pr_err("unable to find phy\n");

dev_err()

> +		goto err0;
> +	}
> +
> +	get_device(&phy->dev);
> +
> +err0:
> +	mutex_unlock(&phy_list_mutex);
> +
> +	return phy;
> +}
> +EXPORT_SYMBOL_GPL(phy_get);
> +
> +/**
> + * phy_put - release the PHY
> + * @phy: the phy returned by phy_get()
> + *
> + * Releases a refcount the caller received from phy_get().
> + */
> +void phy_put(struct phy *phy)
> +{
> +	if (phy)
> +		put_device(&phy->dev);
> +}
> +EXPORT_SYMBOL_GPL(phy_put);
> +
> +/**
> + * create_phy - 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 *create_phy(struct device *dev, struct phy_descriptor *desc)

All other functions are named phy_*, please rename this, too.

> +{
> +	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");
> +		ret = -ENOMEM;
> +		goto err0;
> +	}
> +
> +	devname = dev_name(dev);
> +	device_initialize(&phy->dev);
> +	phy->desc = desc;
> +	phy->dev.class = phy_class;
> +	phy->dev.parent = dev;
> +	ret = dev_set_name(&phy->dev, "%s", devname);
> +	if (ret)
> +		goto err1;
> +
> +	mutex_lock(&phy_list_mutex);
> +	list_for_each_entry(phy_bind, &phy_bind_list, list)
> +		if (!(strcmp(phy_bind->phy_dev_name, devname)))
> +			phy_bind->phy = phy;
> +
> +	list_add_tail(&phy->head, &phy_list);
> +
> +	ret = device_add(&phy->dev);
> +	if (ret)
> +		goto err2;
> +
> +	mutex_unlock(&phy_list_mutex);
> +
> +	return phy;
> +
> +err2:
> +	phy_bind->phy = NULL;
> +	list_del(&phy->head);
> +	mutex_unlock(&phy_list_mutex);
> +
> +err1:
> +	put_device(&phy->dev);
> +	kfree(phy);
> +
> +err0:
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(create_phy);
> +
> +/**
> + * destroy_phy - destroy the phy
> + * @phy: the phy to be destroyed
> + *
> + * Called to destroy the phy.
> + */
> +void destroy_phy(struct phy *phy)
> +{
> +	struct phy_bind *phy_bind;
> +
> +	mutex_lock(&phy_list_mutex);
> +	list_for_each_entry(phy_bind, &phy_bind_list, list) {
> +		if (phy_bind->phy == phy)
> +			phy_bind->phy = NULL;
> +
> +		if (phy_bind->auto_bind) {
> +			list_del(&phy_bind->list);
> +			kfree(phy_bind);
> +		}
> +	}
> +
> +	list_del(&phy->head);
> +	mutex_unlock(&phy_list_mutex);
> +
> +	device_unregister(&phy->dev);
> +}
> +EXPORT_SYMBOL_GPL(destroy_phy);
> +
> +/**
> + * phy_bind - bind the phy and the controller that uses the phy
> + * @dev_name: the device name of the device that will bind to the phy
> + * @index: index to specify the port number
> + * @phy_dev_name: the device name of the phy
> + *
> + * Fills the phy_bind structure with the dev_name and phy_dev_name. This will
> + * be used when the phy driver registers the phy and when the controller
> + * requests this phy.
> + *
> + * To be used by platform specific initialization code.
> + */
> +struct phy_bind *phy_bind(const char *dev_name, u8 index,
> +				const char *phy_dev_name)
> +{
> +	struct phy_bind *phy_bind;
> +
> +	phy_bind = kzalloc(sizeof(*phy_bind), GFP_KERNEL);
> +	if (!phy_bind) {
> +		pr_err("phy_bind(): No memory for phy_bind");
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	phy_bind->dev_name = dev_name;
> +	phy_bind->phy_dev_name = phy_dev_name;
> +	phy_bind->index = index;
> +	phy_bind->auto_bind = false;
> +
> +	list_add_tail(&phy_bind->list, &phy_bind_list);
> +
> +	return phy_bind;
> +}
> +EXPORT_SYMBOL_GPL(phy_bind);
> +
> +static ssize_t phy_name_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct phy *phy;
> +
> +	phy = container_of(dev, struct phy, dev);
> +
> +	return sprintf(buf, "%s\n", phy->desc->label);
> +}
> +
> +static ssize_t phy_bind_show(struct device *dev,
> +			     struct device_attribute *attr, char *buf)
> +{
> +	struct phy_bind *phy_bind;
> +	struct phy *phy;
> +	char *p = buf;
> +	int len;
> +
> +	phy = container_of(dev, struct phy, dev);
> +
> +	list_for_each_entry(phy_bind, &phy_bind_list, list)
> +		if (phy_bind->phy == phy)
> +			p += sprintf(p, "%s %d\n", phy_bind->dev_name,
> +							phy_bind->index);
> +	len = (p - buf);
> +
> +	return len;
> +}
> +
> +static struct device_attribute phy_dev_attrs[] = {
> +	__ATTR(label, 0444, phy_name_show, NULL),
> +	__ATTR(bind, 0444, phy_bind_show, NULL),
> +	__ATTR_NULL,
> +};
> +
> +/**
> + * phy_release - release the phy
> + * @dev: the dev member within phy
> + *
> + * when the last reference to the device is removed; it is called
> + * from the embedded kobject as release method.
> + */
> +static void phy_release(struct device *dev)
> +{
> +	struct phy *phy;
> +
> +	phy = container_of(dev, struct phy, dev);
> +	dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
> +	kfree(phy);
> +}
> +
> +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));
> +		return PTR_ERR(phy_class);
> +	}
> +
> +	phy_class->dev_release = phy_release;
> +	phy_class->dev_attrs = phy_dev_attrs;
> +
> +	return 0;
> +}
> +subsys_initcall(phy_core_init);
> +
> +static void __exit phy_core_exit(void)
> +{
> +	class_destroy(phy_class);
> +}
> +module_exit(phy_core_exit);
> +
> +MODULE_DESCRIPTION("Generic Phy Framework");
> +MODULE_AUTHOR("Kishon Vijay Abraham I <kishon@ti.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> new file mode 100644
> index 0000000..831c1c0
> --- /dev/null
> +++ b/include/linux/phy/phy.h
> @@ -0,0 +1,181 @@
> +/*
> + * phy.h -- generic phy header file
> + *
> + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __DRIVERS_PHY_H
> +#define __DRIVERS_PHY_H
> +
> +struct phy;
> +
> +/**
> + * struct phy_descriptor - structure that describes the PHY
> + * @label: label given to phy
> + * @type: specifies the phy type
> + * @of_node: device node of the phy
> + * @ops: function pointers for performing phy operations
> + */
> +struct phy_descriptor {
> +	const char		*label;
> +	int			type;
> +	struct device_node	*of_node;
> +	struct phy_ops		*ops;
> +};
> +
> +/**
> + * 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
> + */
> +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 phy - represent the phy device
> + * @desc: descriptor for the phy
> + * @head: to support multiple transceivers
> + */
> +struct phy {
> +	struct device		dev;
> +	struct phy_descriptor	*desc;
> +	struct list_head	head;
> +};
> +
> +/**
> + * struct phy_bind - represent the binding for the phy
> + * @dev_name: the device name of the device that will bind to the phy
> + * @phy_dev_name: the device name of the phy
> + * @index: used if a single controller uses multiple phys
> + * @auto_bind: tells if the binding is done explicitly from board file or not
> + * @phy: reference to the phy
> + * @list: to maintain a linked list of the binding information
> + */
> +struct phy_bind {
> +	const char	*dev_name;
> +	const char	*phy_dev_name;
> +	u8		index;
> +	int		auto_bind:1;
> +	struct phy	*phy;
> +	struct list_head list;
> +};
> +
> +#if defined(CONFIG_GENERIC_PHY) || defined(CONFIG_GENERIC_PHY_MODULE)
> +extern struct phy *devm_phy_get(struct device *dev, u8 index);
> +extern void devm_phy_put(struct device *dev, struct phy *phy);
> +extern struct phy *devm_of_phy_get(struct device *dev, const char *phandle);
> +extern struct phy *phy_get(struct device *dev, u8 index);
> +extern void phy_put(struct phy *phy);
> +extern struct phy *create_phy(struct device *dev, struct phy_descriptor *desc);
> +extern void destroy_phy(struct phy *phy);
> +extern struct phy_bind *phy_bind(const char *dev_name, u8 index,
> +				const char *phy_dev_name);
> +#else
> +static inline struct phy *devm_phy_get(struct device *dev, u8 index)
> +{
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static inline void devm_phy_put(struct device *dev, struct phy *phy)
> +{
> +}
> +
> +static inline struct phy *devm_of_phy_get(struct device *dev,
> +		const char *phandle)
> +{
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static inline struct phy *phy_get(struct device *dev, u8 index)
> +{
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static inline void phy_put(struct phy *phy)
> +{
> +}
> +
> +static inline struct phy *create_phy(struct device *dev,
> +	struct phy_descriptor *desc)
> +{
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static inline void destroy_phy(struct phy *phy)
> +{
> +}
> +
> +static inline struct phy_bind *phy_bind(const char *dev_name, u8 index,
> +				const char *phy_dev_name)
> +{
> +}
> +#endif
> +
> +static inline int phy_init(struct phy *phy)
> +{
> +	if (phy->desc->ops->init)
> +		return phy->desc->ops->init(phy->desc);
> +
> +	return -EINVAL;
> +}
> +
> +static inline int phy_exit(struct phy *phy)
> +{
> +	if (phy->desc->ops->exit)
> +		return phy->desc->ops->exit(phy->desc);
> +
> +	return -EINVAL;
> +}
> +
> +static inline int phy_suspend(struct phy *phy)
> +{
> +	if (phy->desc->ops->suspend)
> +		return phy->desc->ops->suspend(phy->desc);
> +
> +	return -EINVAL;
> +}
> +
> +static inline int phy_resume(struct phy *phy)
> +{
> +	if (phy->desc->ops->resume)
> +		return phy->desc->ops->resume(phy->desc);
> +
> +	return -EINVAL;
> +}
> +
> +static inline int phy_poweron(struct phy *phy)
> +{
> +	if (phy->desc->ops->poweron)
> +		return phy->desc->ops->poweron(phy->desc);
> +
> +	return -EINVAL;
> +}
> +
> +static inline void phy_shutdown(struct phy *phy)
> +{
> +	if (phy->desc->ops->shutdown)
> +		phy->desc->ops->shutdown(phy->desc);
> +}
> +#endif /* __DRIVERS_PHY_H */
>

Kishon Vijay Abraham I Sept. 14, 2012, 1:06 p.m. UTC | #2

Hi,

On Fri, Sep 14, 2012 at 5:58 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 09/14/2012 01:58 PM, Kishon Vijay Abraham I wrote:
>> The PHY framework provides a set of API's for the PHY drivers to
>> create/remove a PHY and the PHY users to obtain a reference to the PHY
>> using or without using phandle. If the PHY users has 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
>> information about the PHY and ops like init, exit, suspend, resume,
>> poweron, shutdown.
>
> Some comments inside.
>
> While looking over the code, I was thinking why not abstract the phy
> with a "bus" in the linux kernel. The ethernet phys are on the mdio_bus,
> see /sys/bus/mdio_bus. This saves you hand crafting devices, drivers and
> bindings,

well... have to think about it.
>
> Marc
>
>>
>> Nyet-signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>> This framework is actually intended to be used by all the PHY drivers in the
>> kernel. Though it's going to take a while for that, I intend to migrate
>> existing USB/OTG phy drivers to use this framework as we align on the design
>> of this framework. Once I migrate these phy drivers, I'll be able to test this
>> framework (I haven't tested this framework so far). I sent this patch early
>> so as to get review comments and align on the design. Thanks :-)
>>
>>  drivers/Kconfig         |    2 +
>>  drivers/Makefile        |    2 +
>>  drivers/phy/Kconfig     |   13 ++
>>  drivers/phy/Makefile    |    5 +
>>  drivers/phy/phy-core.c  |  437 +++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/phy/phy.h |  181 ++++++++++++++++++++
>>  6 files changed, 640 insertions(+)
>>  create mode 100644 drivers/phy/Kconfig
>>  create mode 100644 drivers/phy/Makefile
>>  create mode 100644 drivers/phy/phy-core.c
>>  create mode 100644 include/linux/phy/phy.h
>>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index ece958d..8488818 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -152,4 +152,6 @@ source "drivers/vme/Kconfig"
>>
>>  source "drivers/pwm/Kconfig"
>>
>> +source "drivers/phy/Kconfig"
>> +
>>  endmenu
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 5b42184..63d6bbe 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -38,6 +38,8 @@ obj-y                               += char/
>>  # gpu/ comes after char for AGP vs DRM startup
>>  obj-y                                += gpu/
>>
>> +obj-y                                += phy/
>> +
>>  obj-$(CONFIG_CONNECTOR)              += connector/
>>
>>  # i810fb and intelfb depend on char/agp/
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> new file mode 100644
>> index 0000000..34f7077
>> --- /dev/null
>> +++ b/drivers/phy/Kconfig
>> @@ -0,0 +1,13 @@
>> +#
>> +# PHY
>> +#
>> +
>> +menuconfig GENERIC_PHY
>> +     tristate "Generic PHY Support"
>> +     help
>> +       Generic PHY support.
>> +
>> +       This framework is designed to provide a generic interface for PHY
>> +       devices present in the kernel. This layer will have the generic
>> +       API by which phy drivers can create PHY using the phy framework and
>> +       phy users can obtain reference to the PHY.
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> new file mode 100644
>> index 0000000..9e9560f
>> --- /dev/null
>> +++ b/drivers/phy/Makefile
>> @@ -0,0 +1,5 @@
>> +#
>> +# Makefile for the phy drivers.
>> +#
>> +
>> +obj-$(CONFIG_GENERIC_PHY)    += phy-core.o
>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>> new file mode 100644
>> index 0000000..c55446a
>> --- /dev/null
>> +++ b/drivers/phy/phy-core.c
>> @@ -0,0 +1,437 @@
>> +/*
>> + * phy-core.c  --  Generic Phy framework.
>> + *
>> + * Copyright (C) 2012 Texas Instruments
>> + *
>> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
>> + *
>> + * This program is free software; you can redistribute  it and/or modify it
>> + * under  the terms of  the GNU General  Public License as published by the
>> + * Free Software Foundation;  either version 2 of the  License, or (at your
>> + * option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/export.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/device.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/phy/phy.h>
>> +
>> +static struct class *phy_class;
>> +static LIST_HEAD(phy_list);
>> +static DEFINE_MUTEX(phy_list_mutex);
>> +static LIST_HEAD(phy_bind_list);
>> +
>> +static void devm_phy_release(struct device *dev, void *res)
>> +{
>> +     struct phy *phy = *(struct phy **)res;
>
> What about adding a struct phy_res, doing so,m you don't need these
> casts, and it's easier to add more pointers if needed.

Wont we still need to do the cast since you get only a void pointer.
Maybe I'm not getting you.
>
>> +
>> +     phy_put(phy);
>> +}
>> +
>> +static int devm_phy_match(struct device *dev, void *res, void *match_data)
>> +{
>> +     return res == match_data;
>> +}
>> +
>> +static struct phy *phy_lookup(struct device *dev, u8 index)
>> +{
>> +     struct phy_bind *phy_bind = NULL;
>> +
>> +     list_for_each_entry(phy_bind, &phy_bind_list, list) {
>> +             if (!(strcmp(phy_bind->dev_name, dev_name(dev))) &&
>> +                             phy_bind->index == index)
>> +                     return phy_bind->phy;
>> +     }
>> +
>> +     return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static struct phy *of_phy_lookup(struct device *dev, struct device_node *node)
>> +{
>> +     int index = 0;
>> +     struct phy  *phy;
>                   ^^
>
> Please remove that stray space.

Sure.
>
>> +     struct phy_bind *phy_map = NULL;
>> +
>> +     list_for_each_entry(phy_map, &phy_bind_list, list)
>> +             if (!(strcmp(phy_map->dev_name, dev_name(dev))))
>> +                     index++;
>> +
>> +     list_for_each_entry(phy, &phy_list, head) {
>> +             if (node != phy->desc->of_node)
>> +                     continue;
>> +
>> +             phy_map = phy_bind(dev_name(dev), index, dev_name(&phy->dev));
>> +             if (!IS_ERR(phy_map)) {
>> +                     phy_map->phy = phy;
>> +                     phy_map->auto_bind = true;
>> +             }
>> +
>> +             return phy;
>> +     }
>> +
>> +     return ERR_PTR(-ENODEV);
>> +}
>> +
>> +/**
>> + * devm_phy_get - lookup and obtain a reference to a phy.
>> + * @dev: device that requests this phy
>> + * @index: the index of the phy
>> + *
>> + * Gets the phy using phy_get(), and associates a device with it using
>> + * devres. On driver detach, release function is invoked on the devres data,
>> + * then, devres data is freed.
>> + */
>> +struct phy *devm_phy_get(struct device *dev, u8 index)
>> +{
>> +     struct phy **ptr, *phy;
>> +
>> +     ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
>> +     if (!ptr)
>> +             return NULL;
>> +
>> +     phy = phy_get(dev, index);
>> +     if (!IS_ERR(phy)) {
>> +             *ptr = phy;
>> +             devres_add(dev, ptr);
>> +     } else
>> +             devres_free(ptr);
>
> nitpick: when when if has { }, else should have, too.

Sure.
>
>> +
>> +     return phy;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_phy_get);
>> +
>> +/**
>> + * devm_phy_put - release the PHY
>> + * @dev: device that wants to release this phy
>> + * @phy: the phy returned by devm_phy_get()
>> + *
>> + * destroys the devres associated with this phy and invokes phy_put
>> + * to release the phy.
>> + */
>> +void devm_phy_put(struct device *dev, struct phy *phy)
>> +{
>> +     int r;
>> +
>> +     r = devres_destroy(dev, devm_phy_release, devm_phy_match, phy);
>> +     dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n");
>> +}
>> +EXPORT_SYMBOL_GPL(devm_phy_put);
>> +
>> +/**
>> + * devm_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
>> + *
>> + * Returns the phy driver associated with the given phandle value,
>> + * after getting a refcount to it or -ENODEV if there is no such phy.
>> + * While at that, it also associates the device with the phy using devres.
>> + * On driver detach, release function is invoked on the devres data,
>> + * then, devres data is freed.
>> + */
>> +struct phy *devm_of_phy_get(struct device *dev, const char *phandle)
>
> We should discuss first how the DT binding for a phy should look like. I
> don't like that you hardcode the index (in of_parse_phandle()) to "0".
> Then we have the problem with USB2 and USB3 phys for the same usb device.

I think then we should modify this API to take *index* which should be
used when we have a single controller using multiple phys. By that
we'll make both dt and non-dt similar in that, both of them will take
this 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, 0);
>> +     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);
>> +     if (!ptr) {
>> +             dev_dbg(dev, "failed to allocate memory for devres\n");
>> +             return ERR_PTR(-ENOMEM);
>> +     }
>> +
>> +     mutex_lock(&phy_list_mutex);
>> +
>> +     phy = of_phy_lookup(dev, node);
>> +     if (IS_ERR(phy) || !try_module_get(phy->dev.driver->owner)) {
>> +             devres_free(ptr);
>> +             goto err0;
>> +     }
>
> Please return -EPROBE_DEFER is the phy is not yet registered.

I really dont prefer to return -EPROBE_DEFER from the *framework*.
IMHO, it's up-to the caller of this API to return *probe* errors.
>
>> +
>> +     *ptr = phy;
>> +     devres_add(dev, ptr);
>> +
>> +     get_device(&phy->dev);
>> +
>> +err0:
>> +     mutex_unlock(&phy_list_mutex);
>> +
>> +     return phy;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_of_phy_get);
>> +
>> +/**
>> + * 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, u8 index)
>> +{
>> +     struct phy      *phy = NULL;
>> +
>> +     mutex_lock(&phy_list_mutex);
>> +
>> +     phy = phy_lookup(dev, index);
>> +     if (IS_ERR(phy)) {
>> +             pr_err("unable to find phy\n");
>
> dev_err()

Sure.
>
>> +             goto err0;
>> +     }
>> +
>> +     get_device(&phy->dev);
>> +
>> +err0:
>> +     mutex_unlock(&phy_list_mutex);
>> +
>> +     return phy;
>> +}
>> +EXPORT_SYMBOL_GPL(phy_get);
>> +
>> +/**
>> + * phy_put - release the PHY
>> + * @phy: the phy returned by phy_get()
>> + *
>> + * Releases a refcount the caller received from phy_get().
>> + */
>> +void phy_put(struct phy *phy)
>> +{
>> +     if (phy)
>> +             put_device(&phy->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(phy_put);
>> +
>> +/**
>> + * create_phy - 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 *create_phy(struct device *dev, struct phy_descriptor *desc)
>
> All other functions are named phy_*, please rename this, too.

Sure.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Felipe Balbi Sept. 14, 2012, 1:12 p.m. UTC | #3

On Fri, Sep 14, 2012 at 02:28:19PM +0200, Marc Kleine-Budde wrote:
> On 09/14/2012 01:58 PM, Kishon Vijay Abraham I wrote:
> > The PHY framework provides a set of API's for the PHY drivers to
> > create/remove a PHY and the PHY users to obtain a reference to the PHY
> > using or without using phandle. If the PHY users has 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
> > information about the PHY and ops like init, exit, suspend, resume,
> > poweron, shutdown.
> 
> Some comments inside.
> 
> While looking over the code, I was thinking why not abstract the phy
> with a "bus" in the linux kernel. The ethernet phys are on the mdio_bus,
> see /sys/bus/mdio_bus. This saves you hand crafting devices, drivers and
> bindings,

I don't think that's a good idea, actually. You can have USB PHYs which
are memory mapped, or connected through i2c, or connected through any
other bus. If the PHYs layer itself is a bus, it means we will need to
register a device on two different buses, which doesn't sound very
nice IMHO.

Chen Peter-B29397 Sept. 17, 2012, 1:20 a.m. UTC | #4

> 
> The PHY framework provides a set of API's for the PHY drivers to
> create/remove a PHY and the PHY users to obtain a reference to the PHY
> using or without using phandle. If the PHY users has 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.
> 

What's an example of "the same phy user binds to multiple phys"?
I only remembered that Felipe said there are two phy users for one single phy at 
omap5 that is both usb3 and sata uses the same phy.


 


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kishon Vijay Abraham I Sept. 17, 2012, 5:49 a.m. UTC | #5

Hi,

On Mon, Sep 17, 2012 at 6:50 AM, Chen Peter-B29397 <B29397@freescale.com> wrote:
>
>>
>> The PHY framework provides a set of API's for the PHY drivers to
>> create/remove a PHY and the PHY users to obtain a reference to the PHY
>> using or without using phandle. If the PHY users has 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.
>>
>
> What's an example of "the same phy user binds to multiple phys"?

Single controller using multiple phys..
> I only remembered that Felipe said there are two phy users for one single phy at
> omap5 that is both usb3 and sata uses the same phy.

*index* is used when a single controller uses multiple phys. For
example it could be used for dwc3 (usb3 controller) where it uses usb2
phy and usb3 phy.

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marc Kleine-Budde Sept. 17, 2012, 9:33 a.m. UTC | #6

On 09/14/2012 03:06 PM, ABRAHAM, KISHON VIJAY wrote:

[...]

>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>> new file mode 100644
>>> index 0000000..c55446a
>>> --- /dev/null
>>> +++ b/drivers/phy/phy-core.c
>>> @@ -0,0 +1,437 @@
>>> +/*
>>> + * phy-core.c  --  Generic Phy framework.
>>> + *
>>> + * Copyright (C) 2012 Texas Instruments
>>> + *
>>> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
>>> + *
>>> + * This program is free software; you can redistribute  it and/or modify it
>>> + * under  the terms of  the GNU General  Public License as published by the
>>> + * Free Software Foundation;  either version 2 of the  License, or (at your
>>> + * option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/export.h>
>>> +#include <linux/module.h>
>>> +#include <linux/err.h>
>>> +#include <linux/device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/of.h>
>>> +#include <linux/phy/phy.h>
>>> +
>>> +static struct class *phy_class;
>>> +static LIST_HEAD(phy_list);
>>> +static DEFINE_MUTEX(phy_list_mutex);
>>> +static LIST_HEAD(phy_bind_list);
>>> +
>>> +static void devm_phy_release(struct device *dev, void *res)
>>> +{
>>> +     struct phy *phy = *(struct phy **)res;
>>
>> What about adding a struct phy_res, doing so,m you don't need these
>> casts, and it's easier to add more pointers if needed.
> 
> Wont we still need to do the cast since you get only a void pointer.
> Maybe I'm not getting you.

As "res" is a void pointer, no need to hast to to a "struct phy_res"
pointer, if you think that's unclean code, you can still cast it. But
IMHO the code is far more readable.

>>> +
>>> +     phy_put(phy);
>>> +}
>>> +
>>> +static int devm_phy_match(struct device *dev, void *res, void *match_data)
>>> +{
>>> +     return res == match_data;
>>> +}
>>> +
>>> +static struct phy *phy_lookup(struct device *dev, u8 index)
>>> +{
>>> +     struct phy_bind *phy_bind = NULL;
>>> +
>>> +     list_for_each_entry(phy_bind, &phy_bind_list, list) {
>>> +             if (!(strcmp(phy_bind->dev_name, dev_name(dev))) &&
>>> +                             phy_bind->index == index)
>>> +                     return phy_bind->phy;
>>> +     }
>>> +
>>> +     return ERR_PTR(-ENODEV);
>>> +}
>>> +
>>> +static struct phy *of_phy_lookup(struct device *dev, struct device_node *node)
>>> +{
>>> +     int index = 0;
>>> +     struct phy  *phy;
>>                   ^^
>>
>> Please remove that stray space.
> 
> Sure.
>>
>>> +     struct phy_bind *phy_map = NULL;
>>> +
>>> +     list_for_each_entry(phy_map, &phy_bind_list, list)
>>> +             if (!(strcmp(phy_map->dev_name, dev_name(dev))))
>>> +                     index++;
>>> +
>>> +     list_for_each_entry(phy, &phy_list, head) {
>>> +             if (node != phy->desc->of_node)
>>> +                     continue;
>>> +
>>> +             phy_map = phy_bind(dev_name(dev), index, dev_name(&phy->dev));
>>> +             if (!IS_ERR(phy_map)) {
>>> +                     phy_map->phy = phy;
>>> +                     phy_map->auto_bind = true;
>>> +             }
>>> +
>>> +             return phy;
>>> +     }
>>> +
>>> +     return ERR_PTR(-ENODEV);
>>> +}
>>> +
>>> +/**
>>> + * devm_phy_get - lookup and obtain a reference to a phy.
>>> + * @dev: device that requests this phy
>>> + * @index: the index of the phy
>>> + *
>>> + * Gets the phy using phy_get(), and associates a device with it using
>>> + * devres. On driver detach, release function is invoked on the devres data,
>>> + * then, devres data is freed.
>>> + */
>>> +struct phy *devm_phy_get(struct device *dev, u8 index)
>>> +{
>>> +     struct phy **ptr, *phy;
>>> +
>>> +     ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
>>> +     if (!ptr)
>>> +             return NULL;
>>> +
>>> +     phy = phy_get(dev, index);
>>> +     if (!IS_ERR(phy)) {
>>> +             *ptr = phy;
>>> +             devres_add(dev, ptr);
>>> +     } else
>>> +             devres_free(ptr);
>>
>> nitpick: when when if has { }, else should have, too.
> 
> Sure.
>>
>>> +
>>> +     return phy;
>>> +}
>>> +EXPORT_SYMBOL_GPL(devm_phy_get);
>>> +
>>> +/**
>>> + * devm_phy_put - release the PHY
>>> + * @dev: device that wants to release this phy
>>> + * @phy: the phy returned by devm_phy_get()
>>> + *
>>> + * destroys the devres associated with this phy and invokes phy_put
>>> + * to release the phy.
>>> + */
>>> +void devm_phy_put(struct device *dev, struct phy *phy)
>>> +{
>>> +     int r;
>>> +
>>> +     r = devres_destroy(dev, devm_phy_release, devm_phy_match, phy);
>>> +     dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n");
>>> +}
>>> +EXPORT_SYMBOL_GPL(devm_phy_put);
>>> +
>>> +/**
>>> + * devm_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
>>> + *
>>> + * Returns the phy driver associated with the given phandle value,
>>> + * after getting a refcount to it or -ENODEV if there is no such phy.
>>> + * While at that, it also associates the device with the phy using devres.
>>> + * On driver detach, release function is invoked on the devres data,
>>> + * then, devres data is freed.
>>> + */
>>> +struct phy *devm_of_phy_get(struct device *dev, const char *phandle)
>>
>> We should discuss first how the DT binding for a phy should look like. I
>> don't like that you hardcode the index (in of_parse_phandle()) to "0".
>> Then we have the problem with USB2 and USB3 phys for the same usb device.
> 
> I think then we should modify this API to take *index* which should be
> used when we have a single controller using multiple phys. By that
> we'll make both dt and non-dt similar in that, both of them will take
> this index.

That would be a plus.

>>
>>> +{
>>> +     struct phy      *phy = NULL, **ptr;

nitpick: stray spaces

>>> +     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, 0);

BTW: Is the node freed somewhere?

>>> +     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);
>>> +     if (!ptr) {
>>> +             dev_dbg(dev, "failed to allocate memory for devres\n");
>>> +             return ERR_PTR(-ENOMEM);
>>> +     }
>>> +
>>> +     mutex_lock(&phy_list_mutex);
>>> +
>>> +     phy = of_phy_lookup(dev, node);
>>> +     if (IS_ERR(phy) || !try_module_get(phy->dev.driver->owner)) {

Where's the corresponding module_put? You should add module_get to the
non dt function, too.

>>> +             devres_free(ptr);
>>> +             goto err0;
>>> +     }
>>
>> Please return -EPROBE_DEFER is the phy is not yet registered.
> 
> I really dont prefer to return -EPROBE_DEFER from the *framework*.
> IMHO, it's up-to the caller of this API to return *probe* errors.

If we leave it to the caller, you have to proper design your return
values. There is a difference betweeen:
a) the phandle + index point to a non existing device node
b) the device node cannot be found in the list of phys (yet)

Error a) is permanent (in case of a static DT). Case b) indeed can be
solved by deferring the probe. Why should the framework not return
-EPROBE_DEFER in this case?

>>
>>> +
>>> +     *ptr = phy;
>>> +     devres_add(dev, ptr);
>>> +
>>> +     get_device(&phy->dev);
>>> +
>>> +err0:
>>> +     mutex_unlock(&phy_list_mutex);
>>> +
>>> +     return phy;
>>> +}
>>> +EXPORT_SYMBOL_GPL(devm_of_phy_get);
>>> +

[...]

> Marc

Felipe Balbi Sept. 17, 2012, 1:19 p.m. UTC | #7

On Mon, Sep 17, 2012 at 11:19:53AM +0530, ABRAHAM, KISHON VIJAY wrote:
> Hi,
> 
> On Mon, Sep 17, 2012 at 6:50 AM, Chen Peter-B29397 <B29397@freescale.com> wrote:
> >
> >>
> >> The PHY framework provides a set of API's for the PHY drivers to
> >> create/remove a PHY and the PHY users to obtain a reference to the PHY
> >> using or without using phandle. If the PHY users has 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.
> >>
> >
> > What's an example of "the same phy user binds to multiple phys"?
> 
> Single controller using multiple phys..

to be more specific here: any usb3 controller needs a USB2 PHY and USB3
PHY.

Kishon Vijay Abraham I Sept. 26, 2012, 9:20 a.m. UTC | #8

Hi,

On Mon, Sep 17, 2012 at 3:03 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 09/14/2012 03:06 PM, ABRAHAM, KISHON VIJAY wrote:
>
> [...]
>
>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>>> new file mode 100644
>>>> index 0000000..c55446a
>>>> --- /dev/null
>>>> +++ b/drivers/phy/phy-core.c
>>>> @@ -0,0 +1,437 @@
>>>> +/*
>>>> + * phy-core.c  --  Generic Phy framework.
>>>> + *
>>>> + * Copyright (C) 2012 Texas Instruments
>>>> + *
>>>> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
>>>> + *
>>>> + * This program is free software; you can redistribute  it and/or modify it
>>>> + * under  the terms of  the GNU General  Public License as published by the
>>>> + * Free Software Foundation;  either version 2 of the  License, or (at your
>>>> + * option) any later version.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License
>>>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>> + */
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/export.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/phy/phy.h>
>>>> +
>>>> +static struct class *phy_class;
>>>> +static LIST_HEAD(phy_list);
>>>> +static DEFINE_MUTEX(phy_list_mutex);
>>>> +static LIST_HEAD(phy_bind_list);
>>>> +
>>>> +static void devm_phy_release(struct device *dev, void *res)
>>>> +{
>>>> +     struct phy *phy = *(struct phy **)res;
>>>
>>> What about adding a struct phy_res, doing so,m you don't need these
>>> casts, and it's easier to add more pointers if needed.
>>
>> Wont we still need to do the cast since you get only a void pointer.
>> Maybe I'm not getting you.
>
> As "res" is a void pointer, no need to hast to to a "struct phy_res"
> pointer, if you think that's unclean code, you can still cast it. But
> IMHO the code is far more readable.
>
>>>> +
>>>> +     phy_put(phy);
>>>> +}
>>>> +
>>>> +static int devm_phy_match(struct device *dev, void *res, void *match_data)
>>>> +{
>>>> +     return res == match_data;
>>>> +}
>>>> +
>>>> +static struct phy *phy_lookup(struct device *dev, u8 index)
>>>> +{
>>>> +     struct phy_bind *phy_bind = NULL;
>>>> +
>>>> +     list_for_each_entry(phy_bind, &phy_bind_list, list) {
>>>> +             if (!(strcmp(phy_bind->dev_name, dev_name(dev))) &&
>>>> +                             phy_bind->index == index)
>>>> +                     return phy_bind->phy;
>>>> +     }
>>>> +
>>>> +     return ERR_PTR(-ENODEV);
>>>> +}
>>>> +
>>>> +static struct phy *of_phy_lookup(struct device *dev, struct device_node *node)
>>>> +{
>>>> +     int index = 0;
>>>> +     struct phy  *phy;
>>>                   ^^
>>>
>>> Please remove that stray space.
>>
>> Sure.
>>>
>>>> +     struct phy_bind *phy_map = NULL;
>>>> +
>>>> +     list_for_each_entry(phy_map, &phy_bind_list, list)
>>>> +             if (!(strcmp(phy_map->dev_name, dev_name(dev))))
>>>> +                     index++;
>>>> +
>>>> +     list_for_each_entry(phy, &phy_list, head) {
>>>> +             if (node != phy->desc->of_node)
>>>> +                     continue;
>>>> +
>>>> +             phy_map = phy_bind(dev_name(dev), index, dev_name(&phy->dev));
>>>> +             if (!IS_ERR(phy_map)) {
>>>> +                     phy_map->phy = phy;
>>>> +                     phy_map->auto_bind = true;
>>>> +             }
>>>> +
>>>> +             return phy;
>>>> +     }
>>>> +
>>>> +     return ERR_PTR(-ENODEV);
>>>> +}
>>>> +
>>>> +/**
>>>> + * devm_phy_get - lookup and obtain a reference to a phy.
>>>> + * @dev: device that requests this phy
>>>> + * @index: the index of the phy
>>>> + *
>>>> + * Gets the phy using phy_get(), and associates a device with it using
>>>> + * devres. On driver detach, release function is invoked on the devres data,
>>>> + * then, devres data is freed.
>>>> + */
>>>> +struct phy *devm_phy_get(struct device *dev, u8 index)
>>>> +{
>>>> +     struct phy **ptr, *phy;
>>>> +
>>>> +     ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
>>>> +     if (!ptr)
>>>> +             return NULL;
>>>> +
>>>> +     phy = phy_get(dev, index);
>>>> +     if (!IS_ERR(phy)) {
>>>> +             *ptr = phy;
>>>> +             devres_add(dev, ptr);
>>>> +     } else
>>>> +             devres_free(ptr);
>>>
>>> nitpick: when when if has { }, else should have, too.
>>
>> Sure.
>>>
>>>> +
>>>> +     return phy;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(devm_phy_get);
>>>> +
>>>> +/**
>>>> + * devm_phy_put - release the PHY
>>>> + * @dev: device that wants to release this phy
>>>> + * @phy: the phy returned by devm_phy_get()
>>>> + *
>>>> + * destroys the devres associated with this phy and invokes phy_put
>>>> + * to release the phy.
>>>> + */
>>>> +void devm_phy_put(struct device *dev, struct phy *phy)
>>>> +{
>>>> +     int r;
>>>> +
>>>> +     r = devres_destroy(dev, devm_phy_release, devm_phy_match, phy);
>>>> +     dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n");
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(devm_phy_put);
>>>> +
>>>> +/**
>>>> + * devm_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
>>>> + *
>>>> + * Returns the phy driver associated with the given phandle value,
>>>> + * after getting a refcount to it or -ENODEV if there is no such phy.
>>>> + * While at that, it also associates the device with the phy using devres.
>>>> + * On driver detach, release function is invoked on the devres data,
>>>> + * then, devres data is freed.
>>>> + */
>>>> +struct phy *devm_of_phy_get(struct device *dev, const char *phandle)
>>>
>>> We should discuss first how the DT binding for a phy should look like. I
>>> don't like that you hardcode the index (in of_parse_phandle()) to "0".
>>> Then we have the problem with USB2 and USB3 phys for the same usb device.
>>
>> I think then we should modify this API to take *index* which should be
>> used when we have a single controller using multiple phys. By that
>> we'll make both dt and non-dt similar in that, both of them will take
>> this index.
>
> That would be a plus.
>
>>>
>>>> +{
>>>> +     struct phy      *phy = NULL, **ptr;
>
> nitpick: stray spaces
>
>>>> +     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, 0);
>
> BTW: Is the node freed somewhere?
>
>>>> +     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);
>>>> +     if (!ptr) {
>>>> +             dev_dbg(dev, "failed to allocate memory for devres\n");
>>>> +             return ERR_PTR(-ENOMEM);
>>>> +     }
>>>> +
>>>> +     mutex_lock(&phy_list_mutex);
>>>> +
>>>> +     phy = of_phy_lookup(dev, node);
>>>> +     if (IS_ERR(phy) || !try_module_get(phy->dev.driver->owner)) {
>
> Where's the corresponding module_put? You should add module_get to the
> non dt function, too.

Do you think try_module_get() is necessary here? And just figured out
*this* phy device does not have a associated driver (so
phy->dev.driver will be NULL).

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marc Kleine-Budde Sept. 26, 2012, 9:37 a.m. UTC | #9

On 09/26/2012 11:20 AM, ABRAHAM, KISHON VIJAY wrote:
> Hi,
> 
> On Mon, Sep 17, 2012 at 3:03 PM, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>> On 09/14/2012 03:06 PM, ABRAHAM, KISHON VIJAY wrote:
>>
>> [...]
>>
>>>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>>>> new file mode 100644
>>>>> index 0000000..c55446a
>>>>> --- /dev/null
>>>>> +++ b/drivers/phy/phy-core.c
>>>>> @@ -0,0 +1,437 @@
>>>>> +/*
>>>>> + * phy-core.c  --  Generic Phy framework.
>>>>> + *
>>>>> + * Copyright (C) 2012 Texas Instruments
>>>>> + *
>>>>> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
>>>>> + *
>>>>> + * This program is free software; you can redistribute  it and/or modify it
>>>>> + * under  the terms of  the GNU General  Public License as published by the
>>>>> + * Free Software Foundation;  either version 2 of the  License, or (at your
>>>>> + * option) any later version.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> + * GNU General Public License for more details.
>>>>> + *
>>>>> + * You should have received a copy of the GNU General Public License
>>>>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>> + */
>>>>> +
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/export.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/err.h>
>>>>> +#include <linux/device.h>
>>>>> +#include <linux/slab.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/phy/phy.h>
>>>>> +
>>>>> +static struct class *phy_class;
>>>>> +static LIST_HEAD(phy_list);
>>>>> +static DEFINE_MUTEX(phy_list_mutex);
>>>>> +static LIST_HEAD(phy_bind_list);
>>>>> +
>>>>> +static void devm_phy_release(struct device *dev, void *res)
>>>>> +{
>>>>> +     struct phy *phy = *(struct phy **)res;
>>>>
>>>> What about adding a struct phy_res, doing so,m you don't need these
>>>> casts, and it's easier to add more pointers if needed.
>>>
>>> Wont we still need to do the cast since you get only a void pointer.
>>> Maybe I'm not getting you.
>>
>> As "res" is a void pointer, no need to hast to to a "struct phy_res"
>> pointer, if you think that's unclean code, you can still cast it. But
>> IMHO the code is far more readable.
>>
>>>>> +
>>>>> +     phy_put(phy);
>>>>> +}
>>>>> +
>>>>> +static int devm_phy_match(struct device *dev, void *res, void *match_data)
>>>>> +{
>>>>> +     return res == match_data;
>>>>> +}
>>>>> +
>>>>> +static struct phy *phy_lookup(struct device *dev, u8 index)
>>>>> +{
>>>>> +     struct phy_bind *phy_bind = NULL;
>>>>> +
>>>>> +     list_for_each_entry(phy_bind, &phy_bind_list, list) {
>>>>> +             if (!(strcmp(phy_bind->dev_name, dev_name(dev))) &&
>>>>> +                             phy_bind->index == index)
>>>>> +                     return phy_bind->phy;
>>>>> +     }
>>>>> +
>>>>> +     return ERR_PTR(-ENODEV);
>>>>> +}
>>>>> +
>>>>> +static struct phy *of_phy_lookup(struct device *dev, struct device_node *node)
>>>>> +{
>>>>> +     int index = 0;
>>>>> +     struct phy  *phy;
>>>>                   ^^
>>>>
>>>> Please remove that stray space.
>>>
>>> Sure.
>>>>
>>>>> +     struct phy_bind *phy_map = NULL;
>>>>> +
>>>>> +     list_for_each_entry(phy_map, &phy_bind_list, list)
>>>>> +             if (!(strcmp(phy_map->dev_name, dev_name(dev))))
>>>>> +                     index++;
>>>>> +
>>>>> +     list_for_each_entry(phy, &phy_list, head) {
>>>>> +             if (node != phy->desc->of_node)
>>>>> +                     continue;
>>>>> +
>>>>> +             phy_map = phy_bind(dev_name(dev), index, dev_name(&phy->dev));
>>>>> +             if (!IS_ERR(phy_map)) {
>>>>> +                     phy_map->phy = phy;
>>>>> +                     phy_map->auto_bind = true;
>>>>> +             }
>>>>> +
>>>>> +             return phy;
>>>>> +     }
>>>>> +
>>>>> +     return ERR_PTR(-ENODEV);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * devm_phy_get - lookup and obtain a reference to a phy.
>>>>> + * @dev: device that requests this phy
>>>>> + * @index: the index of the phy
>>>>> + *
>>>>> + * Gets the phy using phy_get(), and associates a device with it using
>>>>> + * devres. On driver detach, release function is invoked on the devres data,
>>>>> + * then, devres data is freed.
>>>>> + */
>>>>> +struct phy *devm_phy_get(struct device *dev, u8 index)
>>>>> +{
>>>>> +     struct phy **ptr, *phy;
>>>>> +
>>>>> +     ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
>>>>> +     if (!ptr)
>>>>> +             return NULL;
>>>>> +
>>>>> +     phy = phy_get(dev, index);
>>>>> +     if (!IS_ERR(phy)) {
>>>>> +             *ptr = phy;
>>>>> +             devres_add(dev, ptr);
>>>>> +     } else
>>>>> +             devres_free(ptr);
>>>>
>>>> nitpick: when when if has { }, else should have, too.
>>>
>>> Sure.
>>>>
>>>>> +
>>>>> +     return phy;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(devm_phy_get);
>>>>> +
>>>>> +/**
>>>>> + * devm_phy_put - release the PHY
>>>>> + * @dev: device that wants to release this phy
>>>>> + * @phy: the phy returned by devm_phy_get()
>>>>> + *
>>>>> + * destroys the devres associated with this phy and invokes phy_put
>>>>> + * to release the phy.
>>>>> + */
>>>>> +void devm_phy_put(struct device *dev, struct phy *phy)
>>>>> +{
>>>>> +     int r;
>>>>> +
>>>>> +     r = devres_destroy(dev, devm_phy_release, devm_phy_match, phy);
>>>>> +     dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n");
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(devm_phy_put);
>>>>> +
>>>>> +/**
>>>>> + * devm_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
>>>>> + *
>>>>> + * Returns the phy driver associated with the given phandle value,
>>>>> + * after getting a refcount to it or -ENODEV if there is no such phy.
>>>>> + * While at that, it also associates the device with the phy using devres.
>>>>> + * On driver detach, release function is invoked on the devres data,
>>>>> + * then, devres data is freed.
>>>>> + */
>>>>> +struct phy *devm_of_phy_get(struct device *dev, const char *phandle)
>>>>
>>>> We should discuss first how the DT binding for a phy should look like. I
>>>> don't like that you hardcode the index (in of_parse_phandle()) to "0".
>>>> Then we have the problem with USB2 and USB3 phys for the same usb device.
>>>
>>> I think then we should modify this API to take *index* which should be
>>> used when we have a single controller using multiple phys. By that
>>> we'll make both dt and non-dt similar in that, both of them will take
>>> this index.
>>
>> That would be a plus.
>>
>>>>
>>>>> +{
>>>>> +     struct phy      *phy = NULL, **ptr;
>>
>> nitpick: stray spaces
>>
>>>>> +     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, 0);
>>
>> BTW: Is the node freed somewhere?
>>
>>>>> +     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);
>>>>> +     if (!ptr) {
>>>>> +             dev_dbg(dev, "failed to allocate memory for devres\n");
>>>>> +             return ERR_PTR(-ENOMEM);
>>>>> +     }
>>>>> +
>>>>> +     mutex_lock(&phy_list_mutex);
>>>>> +
>>>>> +     phy = of_phy_lookup(dev, node);
>>>>> +     if (IS_ERR(phy) || !try_module_get(phy->dev.driver->owner)) {
>>
>> Where's the corresponding module_put? You should add module_get to the
>> non dt function, too.
> 
> Do you think try_module_get() is necessary here? And just figured out
> *this* phy device does not have a associated driver (so
> phy->dev.driver will be NULL).

If your phy driver is a loadable module, it is needed. Otherweise the
module use counter is 0, you can unload the module and the kernel will
oops when someone will access the driver.

Marc

[RFC] drivers: phy: add generic PHY framework

Commit Message

Comments

Patch