Date:	Wed, 3 Apr 2013 16:42:18 +0300
From:	Felipe Balbi <balbi@...com>
To:	Kishon Vijay Abraham I <kishon@...com>
CC:	<balbi@...com>, <gregkh@...uxfoundation.org>, <arnd@...db.de>,
	<akpm@...ux-foundation.org>, <swarren@...dotorg.org>,
	<sylvester.nawrocki@...il.com>, <rob@...dley.net>,
	<netdev@...r.kernel.org>, <davem@...emloft.net>,
	<cesarb@...arb.net>, <linux-usb@...r.kernel.org>,
	<linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<tony@...mide.com>, <grant.likely@...retlab.ca>,
	<rob.herring@...xeda.com>, <b-cousson@...com>,
	<linux@....linux.org.uk>, <eballetbo@...il.com>,
	<javier@...hile0.org>, <mchehab@...hat.com>,
	<santosh.shilimkar@...com>, <broonie@...nsource.wolfsonmicro.com>,
	<swarren@...dia.com>, <linux-doc@...r.kernel.org>,
	<devicetree-discuss@...ts.ozlabs.org>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v5 1/6] drivers: phy: add generic PHY framework

On Wed, Apr 03, 2013 at 06:23:49PM +0530, 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,
> power_on, power_off.
> 
> 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@...com>
> ---
>  Documentation/ABI/testing/sysfs-class-phy          |   15 +
>  .../devicetree/bindings/phy/phy-bindings.txt       |   67 +++
>  Documentation/phy.txt                              |  113 ++++
>  MAINTAINERS                                        |    7 +
>  drivers/Kconfig                                    |    2 +
>  drivers/Makefile                                   |    2 +
>  drivers/phy/Kconfig                                |   13 +
>  drivers/phy/Makefile                               |    5 +
>  drivers/phy/phy-core.c                             |  616 ++++++++++++++++++++
>  include/linux/phy/phy.h                            |  228 ++++++++
>  10 files changed, 1068 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-phy
>  create mode 100644 Documentation/devicetree/bindings/phy/phy-bindings.txt
>  create mode 100644 Documentation/phy.txt
>  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/Documentation/ABI/testing/sysfs-class-phy b/Documentation/ABI/testing/sysfs-class-phy
> new file mode 100644
> index 0000000..b735467
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-phy
> @@ -0,0 +1,15 @@
> +What:		/sys/class/phy/<phy>/label
> +Date:		Apr 2013
> +KernelVersion:	3.10
> +Contact:	Kishon Vijay Abraham I <kishon@...com>
> +Description:
> +		This is a read-only file for getting the label of the phy.
> +
> +What:		/sys/class/phy/<phy>/phy_bind
> +Date:		Apr 2013
> +KernelVersion:	3.10
> +Contact:	Kishon Vijay Abraham I <kishon@...com>
> +Description:
> +		This is a read-only file for reading the phy binding
> +		information. It contains the device name of the controller,
> +		the index and the device name of the PHY in that order.
> diff --git a/Documentation/devicetree/bindings/phy/phy-bindings.txt b/Documentation/devicetree/bindings/phy/phy-bindings.txt
> new file mode 100644
> index 0000000..e7b246a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-bindings.txt
> @@ -0,0 +1,67 @@
> +This document explains only the dt data binding. For general information about
> +PHY subsystem refer Documentation/phy.txt
> +
> +PHY device node
> +===============
> +
> +Required Properties:
> +#phy-cells:	Number of cells in a PHY specifier;  The meaning of all those
> +		cells is defined by the binding for the phy node. The PHY
> +		provider can use the values in cells to find the appropriate
> +		PHY.
> +
> +For example:
> +
> +phys: phy {
> +    compatible = "xxx";
> +    reg = <...>;
> +    .
> +    .
> +    #phy-cells = <1>;
> +    .
> +    .
> +};
> +
> +That node describes an IP block that implements 2 different PHYs. In order to
> +differentiate between these 2 PHYs, an additonal specifier should be given
> +while trying to get a reference to it.
> +
> +PHY user node
> +=============
> +
> +Required Properties:
> +phys : the phandle for the PHY device (used by the PHY subsystem)
> +
> +Optional properties:
> +phy-names : the names of the PHY corresponding to the PHYs present in the
> +	    *phys* phandle
> +
> +Example 1:
> +usb1: usb_otg_ss@xxx {
> +    compatible = "xxx";
> +    reg = <xxx>;
> +    .
> +    .
> +    phys = <&usb2_phy>, <&usb3_phy>;
> +    phy-names = "usb2phy", "usb3phy";
> +    .
> +    .
> +};
> +
> +This node represents a controller that uses two PHYs one for usb2 and one for
> +usb3.
> +
> +Example 2:
> +usb2: usb_otg_ss@xxx {
> +    compatible = "xxx";
> +    reg = <xxx>;
> +    .
> +    .
> +    phys = <&phys 1>;
> +    .
> +    .
> +};
> +
> +This node represents a controller that uses one of the PHYs which is defined
> +previously. Note that the phy handle has an additional specifier "1" to
> +differentiate between the two PHYs.
> diff --git a/Documentation/phy.txt b/Documentation/phy.txt
> new file mode 100644
> index 0000000..7785ec0
> --- /dev/null
> +++ b/Documentation/phy.txt
> @@ -0,0 +1,113 @@
> +			    PHY SUBSYSTEM
> +		  Kishon Vijay Abraham I <kishon@...com>
> +
> +This document explains the Generic PHY Framework along with the APIs provided,
> +and how-to-use.
> +
> +1. Introduction
> +
> +*PHY* is the abbreviation for physical layer. It is used to connect a device
> +to the physical medium e.g., the USB controller has a PHY to provide functions
> +such as serialization, de-serialization, encoding, decoding and is responsible
> +for obtaining the required data transmission rate. Note that some USB
> +controller has PHY functionality embedded into it and others use an external
> +PHY. Other peripherals that uses a PHY include Wireless LAN, Ethernet,
> +SATA etc.
> +
> +The intention of creating this framework is to bring the phy drivers spread
> +all over the Linux kernel to drivers/phy to increase code re-use and to
> +increase code maintainability.
> +
> +This framework will be of use only to devices that uses external PHY (PHY
> +functionality is not embedded within the controller).
> +
> +2. Creating the PHY
> +
> +The PHY driver should create the PHY in order for other peripheral controllers
> +to make use of it. The PHY framework provides 2 APIs to create the PHY.
> +
> +struct phy *phy_create(struct device *dev, const char *label,
> +	struct device_node *of_node, int type, struct phy_ops *ops,
> +	void *priv);
> +struct phy *devm_phy_create(struct device *dev, const char *label,
> +	struct device_node *of_node, int type, struct phy_ops *ops,
> +	void *priv);
> +
> +The PHY drivers can use one of the above 2 APIs to create the PHY by passing
> +the device pointer, label, device node, type, phy ops and a driver data.
> +phy_ops is a set of function pointers for performing PHY operations such as
> +init, exit, suspend, resume, power_on and power_off.
> +
> +3. Binding the PHY to the controller
> +
> +The framework provides an API for binding the controller to the PHY in the
> +case of non dt boot.
> +
> +struct phy_bind *phy_bind(const char *dev_name, int index,
> +				const char *phy_dev_name);
> +
> +The API fills the phy_bind structure with the dev_name (device name of the
> +controller), index and phy_dev_name (device name of the PHY). This will
> +be used when the controller requests this phy. This API should be used by
> +platform specific initialization code (board file).
> +
> +In the case of dt boot, the binding information should be added in the dt
> +data of the controller.
> +
> +4. Getting a reference to the PHY
> +
> +Before the controller can make use of the PHY, it has to get a reference to
> +it. This framework provides 6 APIs to get a reference to the PHY.
> +
> +struct phy *phy_get(struct device *dev, int index);
> +struct phy *devm_phy_get(struct device *dev, int index);
> +struct phy *of_phy_get(struct device *dev, const char *phandle, int index);
> +struct phy *devm_of_phy_get(struct device *dev, const char *phandle, int index);
> +struct phy *of_phy_get_byname(struct device *dev, const char *string);
> +struct phy *devm_of_phy_get_byname(struct device *dev, const char *string);
> +
> +phy_get and devm_phy_get can be used to get the PHY in non-dt boot. This API
> +uses the binding information added using the phy_bind API to find and return
> +the appropriate PHY. The only difference between the two APIs is that
> +devm_phy_get associates the device with the PHY using devres on successful PHY
> +get. On driver detach, release function is invoked on the the devres data and
> +devres data is freed.
> +
> +of_phy_get and devm_of_phy_get can be used to get the PHY in dt boot. These
> +APIs take the phandle and index to get a reference to the PHY. The only
> +difference between the two APIs is that devm_of_phy_get associates the device
> +with the PHY using devres on successful phy get. On driver detach, release
> +function is invoked on the devres data and it is freed.
> +
> +of_phy_get_byname and devm_of_phy_get_byname can also be used to get the PHY
> +in dt boot. It is same as the above API except that the user has to pass the
> +phy name as filled in "phy-names" phandle in dt data and the framework will
> +find the index and get the PHY.
> +
> +5. Releasing a reference to the PHY
> +
> +When the controller no longer needs the PHY, it has to release the reference
> +to the PHY it has obtained using the APIs mentioned in the above section. The
> +PHY framework provides 2 APIS to release a reference to the PHY.
> +
> +void phy_put(struct phy *phy);
> +void devm_phy_put(struct device *dev, struct phy *phy);
> +
> +Both these APIs are used to release a reference to the PHY and devm_phy_put
> +destroys the devres associated with this PHY.
> +
> +6. Destroying the PHY
> +
> +When the driver that created the PHY is unloaded, it should destroy the PHY it
> +created using one of the following 2 APIs.
> +
> +void phy_destroy(struct phy *phy);
> +void devm_phy_destroy(struct device *dev, struct phy *phy);
> +
> +Both these APIs destroys the PHY and devm_phy_destroy destroys the devres
> +associated with this PHY.
> +
> +7. DeviceTree Binding
> +
> +The documentation for PHY dt binding can be found @
> +Documentation/devicetree/bindings/phy/phy-bindings.txt
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 72b0843..f2674e7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3474,6 +3474,13 @@ S:	Maintained
>  F:	include/asm-generic
>  F:	include/uapi/asm-generic
>  
> +GENERIC PHY FRAMEWORK
> +M:	Kishon Vijay Abraham I <kishon@...com>
> +L:	linux-kernel@...r.kernel.org
> +S:	Supported
> +F:	drivers/phy/
> +F:	include/linux/phy/
> +
>  GENERIC UIO DRIVER FOR PCI DEVICES
>  M:	"Michael S. Tsirkin" <mst@...hat.com>
>  L:	kvm@...r.kernel.org
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 202fa6d..ad2c374a 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -162,4 +162,6 @@ source "drivers/irqchip/Kconfig"
>  
>  source "drivers/ipack/Kconfig"
>  
> +source "drivers/phy/Kconfig"
> +
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index dce39a9..9da8321 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -45,6 +45,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..5f85909
> --- /dev/null
> +++ b/drivers/phy/Kconfig
> @@ -0,0 +1,13 @@
> +#
> +# PHY
> +#
> +
> +menuconfig GENERIC_PHY
> +	tristate "PHY Subsystem"
> +	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..1d753f2
> --- /dev/null
> +++ b/drivers/phy/phy-core.c
> @@ -0,0 +1,616 @@
> +/*
> + * phy-core.c  --  Generic Phy framework.
> + *
> + * Copyright (C) 2013 Texas Instruments
> + *
> + * Author: Kishon Vijay Abraham I <kishon@...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 DEFINE_MUTEX(phy_bind_mutex);
> +static LIST_HEAD(phy_bind_list);
> +static int phy_core_init(void);
> +
> +static void devm_phy_release(struct device *dev, void *res)
> +{
> +	struct phy *phy = *(struct phy **)res;
> +
> +	phy_put(phy);
> +}
> +
> +static void devm_phy_consume(struct device *dev, void *res)
> +{
> +	struct phy *phy = *(struct phy **)res;
> +
> +	phy_destroy(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, int 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)) {
> +			if (phy_bind->phy)
> +				return phy_bind->phy;
> +			else
> +				return ERR_PTR(-EPROBE_DEFER);
> +		}
> +	}
> +
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +static struct phy *of_phy_lookup(struct device_node *node)
> +{
> +	struct phy *phy;
> +	struct device *dev;
> +	struct class_dev_iter iter;
> +
> +	class_dev_iter_init(&iter, phy_class, NULL, NULL);
> +	while ((dev = class_dev_iter_next(&iter))) {
> +		phy = container_of(dev, struct phy, dev);

it would look a bit better if you provided a to_phy() macro. Specially
since this container_of() repeats multiple times in this file.

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

I would rather:

if (WARN(IS_ERR(phy), "invalid parameter\n"))
	return;

module_put(phy->ops->owner);
put_device(&phy->dev);

that way we can catch users passing bogus pointers here. When PHY layer
is disabled, you want to make this is no-op with a static inline in a
header anyway.

> +struct phy *of_phy_xlate(struct phy *phy, struct of_phandle_args *args)
> +{
> +	return phy;
> +}
> +EXPORT_SYMBOL_GPL(of_phy_xlate);

so you get a PHY and just return it ? What gives ?? (maybe I skipped
some of the discussion...)

> +struct phy *of_phy_get(struct device *dev, int index)
> +{
> +	int ret;
> +	struct phy *phy = NULL;
> +	struct phy_bind *phy_map = NULL;
> +	struct of_phandle_args args;
> +	struct device_node *node;
> +
> +	if (!dev->of_node) {
> +		dev_dbg(dev, "device does not have a device node entry\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	ret = of_parse_phandle_with_args(dev->of_node, "phys", "#phy-cells",
> +		index, &args);
> +	if (ret) {
> +		dev_dbg(dev, "failed to get phy in %s node\n",
> +			dev->of_node->full_name);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	phy = of_phy_lookup(args.np);
> +	if (IS_ERR(phy) || !try_module_get(phy->ops->owner)) {
> +		phy = ERR_PTR(-EPROBE_DEFER);
> +		goto err0;
> +	}
> +
> +	phy = phy->ops->of_xlate(phy, &args);

alright, so of_xlate() is optional, am I right ? How about not
implementing the above and have a check for of_xlate() being a valid
pointer here ?

> +struct phy *phy_create(struct device *dev, const char *label,
> +	struct device_node *of_node, int type, struct phy_ops *ops,
> +	void *priv)
> +{
> +	int ret;
> +	struct phy *phy;
> +	struct phy_bind *phy_bind;
> +	const char *devname = NULL;
> +
> +	if (!dev) {
> +		dev_err(dev, "no device provided for PHY\n");

I'd call this a WARN() or am I too pedantic? :-p

> +	if (!ops || !ops->of_xlate || !priv) {
> +		dev_err(dev, "no PHY ops/PHY data provided\n");

likewise here.

> +		ret = -EINVAL;
> +		goto err0;
> +	}
> +
> +	if (!phy_class)
> +		phy_core_init();

why don't you setup the class on module_init ? Then this would be a
terrible error condition here :-)

> +static struct device_attribute phy_dev_attrs[] = {
> +	__ATTR(label, 0444, phy_name_show, NULL),
> +	__ATTR(phy_bind, 0444, phy_bind_show, NULL),

you could expose a human-readable 'type' string. BTW, how are you using
type ? USB2/USB3/etc ? Have you considered our OMAP5 SerDes pair which
are currently for USB3 and SATA (and could just as easily be used for
PCIe)

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

how about dev_vdbg() ? I doubt anyone will be waiting for this
message... Just a thought

> +static int phy_core_init(void)
> +{
> +	if (phy_class)
> +		return 0;

Weird.. if you initialize the class here, why do you need to initialize
it during phy_create() ?

What's going on ? Also, module_init() will only be called once, why this
if (phy_class) check ?

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)

Powered by blists - more mailing lists