[v5,1/6] drivers: phy: add generic PHY framework

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@ti.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

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@ti.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@ti.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@ti.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@ti.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@ti.com>
> +L:	linux-kernel@vger.kernel.org
> +S:	Supported
> +F:	drivers/phy/
> +F:	include/linux/phy/
> +
>  GENERIC UIO DRIVER FOR PCI DEVICES
>  M:	"Michael S. Tsirkin" <mst@redhat.com>
>  L:	kvm@vger.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@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 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 ?

Hi,

On Wednesday 03 April 2013 07:12 PM, Felipe Balbi wrote:
> 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@ti.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@ti.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@ti.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@ti.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@ti.com>
>> +L:	linux-kernel@vger.kernel.org
>> +S:	Supported
>> +F:	drivers/phy/
>> +F:	include/linux/phy/
>> +
>>   GENERIC UIO DRIVER FOR PCI DEVICES
>>   M:	"Michael S. Tsirkin" <mst@redhat.com>
>>   L:	kvm@vger.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@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 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.

hmm.. ok.
>
>> +/**
>> + * 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.

yeah. Have that no-op in header file.
>
>> +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...)

hmm.. this is for the common case where the PHY provider implements only 
one PHY. And both phy provider and phy_instance is represented by struct 
phy *.

For the case where PHY provider implements multiple PHYs (here it will 
have a single dt node), the PHY provider will implement it's own version 
of of_xlate that takes *of_phandle_args* as argument and finds the 
appropriate PHY.
>
>> +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

Not really. of_xlate is mandatory (it's even checked in phy_create). 
Either the PHY provider can implement it's own version or use the 
implementation above (by filling the function pointer).

> implementing the above and have a check for of_xlate() being a valid
> pointer here ?

Having the way it is actually mandates the PHY providers to always 
provide of_xlate which IMO is better since some PHY providers wont 
accidentally be using the default implementation.
>
>> +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.

hmm.. ok.
>
>> +		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 :-)

This is for the case where the PHY driver gets loaded before the PHY 
framework. I could have returned EPROBE_DEFER here instead I thought 
will have it this way.
>
>> +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

Actually not using *type* anywhere. Just used as an additional info 
about the PHY. It's actually not even enum. Maybe I can remove it 
completely.
> are currently for USB3 and SATA (and could just as easily be used for
> PCIe)

Yeah. Me and Balaji were planning to work on it for having a single 
driver to be used for all the above.
>
>> +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 ?

er.. for the case where phy driver is loaded before this PHY framework, 
phy_create would have already called phy_core_init to create the 
phy_class. So module_init() is not needed at all since we have already 
created the phy_class. I think this looks a bit hacky.
Either we can have EPROBE_DEFER in phy_create or have this module as 
subsys_initcall() like I had it before. I would actually prefer it to be 
subsys_initcall().

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

hi,

On Wed, Apr 03, 2013 at 07:48:42PM +0530, Kishon Vijay Abraham I wrote:
> >>+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...)
> 
> hmm.. this is for the common case where the PHY provider implements
> only one PHY. And both phy provider and phy_instance is represented
> by struct phy *.
> 
> For the case where PHY provider implements multiple PHYs (here it
> will have a single dt node), the PHY provider will implement it's own
> version of of_xlate that takes *of_phandle_args* as argument and
> finds the appropriate PHY.

got it.

> >>+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
> 
> Not really. of_xlate is mandatory (it's even checked in phy_create).
> Either the PHY provider can implement it's own version or use the
> implementation above (by filling the function pointer).

alright.

> >implementing the above and have a check for of_xlate() being a valid
> >pointer here ?
> 
> Having the way it is actually mandates the PHY providers to always
> provide of_xlate which IMO is better since some PHY providers wont
> accidentally be using the default implementation.

ok cool, thanks for clarifying.

> >>+		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 :-)
> 
> This is for the case where the PHY driver gets loaded before the PHY
> framework. I could have returned EPROBE_DEFER here instead I thought
> will have it this way.

looks a bit weird IMO. Is it really possible for PHY to load before ?
Since PHY driver uses symbols from phy-core, modprobe will make sure to
load drivers based on symbol dependency, right ?

> >>+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
> 
> Actually not using *type* anywhere. Just used as an additional info
> about the PHY. It's actually not even enum. Maybe I can remove it
> completely.

makes sense.

> >are currently for USB3 and SATA (and could just as easily be used for
> >PCIe)
> 
> Yeah. Me and Balaji were planning to work on it for having a single
> driver to be used for all the above.

cool :-)

Hi,

On Wednesday 03 April 2013 07:57 PM, Felipe Balbi wrote:
> hi,
>
> On Wed, Apr 03, 2013 at 07:48:42PM +0530, Kishon Vijay Abraham I wrote:
>>>> +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...)
>>
>> hmm.. this is for the common case where the PHY provider implements
>> only one PHY. And both phy provider and phy_instance is represented
>> by struct phy *.
>>
>> For the case where PHY provider implements multiple PHYs (here it
>> will have a single dt node), the PHY provider will implement it's own
>> version of of_xlate that takes *of_phandle_args* as argument and
>> finds the appropriate PHY.
>
> got it.
>
>>>> +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
>>
>> Not really. of_xlate is mandatory (it's even checked in phy_create).
>> Either the PHY provider can implement it's own version or use the
>> implementation above (by filling the function pointer).
>
> alright.
>
>>> implementing the above and have a check for of_xlate() being a valid
>>> pointer here ?
>>
>> Having the way it is actually mandates the PHY providers to always
>> provide of_xlate which IMO is better since some PHY providers wont
>> accidentally be using the default implementation.
>
> ok cool, thanks for clarifying.
>
>>>> +		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 :-)
>>
>> This is for the case where the PHY driver gets loaded before the PHY
>> framework. I could have returned EPROBE_DEFER here instead I thought
>> will have it this way.
>
> looks a bit weird IMO. Is it really possible for PHY to load before ?

yeah. it actually happened when I tried with beagle and had all the 
modules as built-in. Because twl4030 has subsys_initcall(), it loads 
before PHY framework.

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

Hi,

On Wed, Apr 03, 2013 at 08:02:52PM +0530, Kishon Vijay Abraham I wrote:
> >>>>+		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 :-)
> >>
> >>This is for the case where the PHY driver gets loaded before the PHY
> >>framework. I could have returned EPROBE_DEFER here instead I thought
> >>will have it this way.
> >
> >looks a bit weird IMO. Is it really possible for PHY to load before ?
> 
> yeah. it actually happened when I tried with beagle and had all the
> modules as built-in. Because twl4030 has subsys_initcall(), it loads
> before PHY framework.

that's a bug in twl4030.

On 04/03/2013 02:53 PM, Kishon Vijay Abraham I wrote:
> --- /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

s/dt data/device tree ?

> +PHY subsystem refer Documentation/phy.txt

s/refer/refer to ?

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

s/PHYs/PHYs,  ?

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

I find it a bit difficult to parse. Perhaps

"This node represents a controller that uses one of the PHYs of the PHY 
provider
device defined previously. ..."

or something similar ?

> +differentiate between the two PHYs.

s/the two/the two available  ?

> 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@ti.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

Shouldn't it be "...medium, e.g. the USB..." ?

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

"controllers have PHY functionality embedded into them" ?

> +PHY. Other peripherals that uses a PHY include Wireless LAN, Ethernet,

s/uses/use ?

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

s/uses/use ?

> +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.

s/in the dt data of/in the device tree node of ?

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

Why do we need separate functions for dt and non-dt ? Couldn't it be handled
internally by the framework ? So the PHY users can use same calls for dt
and non-dt, like in case of e.g. the regulators API ?

Also signatures of some functions are different now:

extern struct phy *phy_get(struct device *dev, int index);
extern struct phy *devm_phy_get(struct device *dev, int index);
extern struct phy *of_phy_get(struct device *dev, int index);
extern struct phy *devm_of_phy_get(struct device *dev, int index);

BTW, I think "extern" could be dropped, does it have any significance in
function declarations in header files ?

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

s/the the/the

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

s/phandle/property ?

> +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.

s/APIS/APIs ?

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

s/APIs destroys/APIs destroy ?

> +associated with this PHY.
> +
> +7. DeviceTree Binding
> +
> +The documentation for PHY dt binding can be found @
> +Documentation/devicetree/bindings/phy/phy-bindings.txt

> --- /dev/null
> +++ b/drivers/phy/phy-core.c
> @@ -0,0 +1,616 @@

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

There is currently nothing preventing a call to this function before
phy_class is initialized. It happened during my tests, and the nice
stack dump showed that it was the PHY user attempting to get the PHY
before the framework got initialized. So either phy_core_init should
be a subsys_initcall or we need a better protection against phy_class
being NULL or ERR_PTR in more places.

> +	while ((dev = class_dev_iter_next(&iter))) {
> +		phy = container_of(dev, struct phy, dev);
> +		if (node != phy->of_node)
> +			continue;
> +
> +		class_dev_iter_exit(&iter);
> +		return phy;
> +	}
> +
> +	class_dev_iter_exit(&iter);
> +	return ERR_PTR(-EPROBE_DEFER);
> +}

> +/**
> + * of_phy_get() - lookup and obtain a reference to a phy by phandle
> + * @dev: device that requests this phy
> + * @index: the index of the phy
> + *
> + * Returns the phy associated with the given phandle value,
> + * after getting a refcount to it or -ENODEV if there is no such phy or
> + * -EPROBE_DEFER if there is a phandle to the phy, but the device is
> + * not yet loaded.
> + */
> +struct phy *of_phy_get(struct device *dev, 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);
> +	if (IS_ERR(phy))
> +		goto err1;
> +
> +	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;

Shouldn't it be done with the phy_bind_mutex held ? I guess an unlocked
version of the phy_bind functions is needed, so it can be used internally.

> +	}
> +
> +	get_device(&phy->dev);
> +
> +err1:
> +	module_put(phy->ops->owner);
> +
> +err0:
> +	of_node_put(node);
> +
> +	return phy;
> +}
> +EXPORT_SYMBOL_GPL(of_phy_get);

> +/**
> + * phy_create() - create a new phy
> + * @dev: device that is creating the new phy
> + * @label: label given to phy
> + * @of_node: device node of the phy
> + * @type: specifies the phy type
> + * @ops: function pointers for performing phy operations
> + * @priv: private data from phy driver
> + *
> + * Called to create a phy using phy framework.
> + */
> +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");
> +		ret = -EINVAL;
> +		goto err0;
> +	}
> +
> +	if (!ops || !ops->of_xlate || !priv) {
> +		dev_err(dev, "no PHY ops/PHY data provided\n");
> +		ret = -EINVAL;
> +		goto err0;
> +	}
> +
> +	if (!phy_class)
> +		phy_core_init();
> +
> +	phy = kzalloc(sizeof(*phy), GFP_KERNEL);
> +	if (!phy) {
> +		ret = -ENOMEM;
> +		goto err0;
> +	}
> +
> +	devname = dev_name(dev);

OK, a sort of serious issue here is that you can't call phy_create()
multiple times for same PHY provider device.

device_add() further will fail as sysfs won't let you create multiple
objects with same name. So I would assume we need to add a new argument
to phy_create() or rename @type to e.g. @phy_id, which could be
concatenated with dev_name(dev) to create a unique name of the phy
device.

And how is the PHY provider supposed to identify a PHY in its common PHY
ops, now when the struct phy port field is removed ? I have temporarily
(ab)used the type field in order to select proper registers within the
phy ops.

> +	device_initialize(&phy->dev);
> +
> +	phy->dev.class = phy_class;
> +	phy->dev.parent = dev;
> +	phy->label = label;

What about duplicating the string here ? That would make the API a bit
more convenient and the callers wouldn't be required to keep all the
labels around.

> +	phy->of_node = of_node;
> +	phy->type = type;
> +	phy->ops = ops;
> +
> +	dev_set_drvdata(&phy->dev, priv);
> +
> +	ret = dev_set_name(&phy->dev, "%s", devname);
> +	if (ret)
> +		goto err1;
> +
> +	mutex_lock(&phy_bind_mutex);
> +	list_for_each_entry(phy_bind,&phy_bind_list, list)
> +		if (!(strcmp(phy_bind->phy_dev_name, devname)))
> +			phy_bind->phy = phy;
> +	mutex_unlock(&phy_bind_mutex);
> +
> +	ret = device_add(&phy->dev);
> +	if (ret)
> +		goto err2;
> +
> +	return phy;
> +
> +err2:
> +	phy_bind->phy = NULL;
> +
> +err1:
> +	put_device(&phy->dev);
> +	kfree(phy);
> +
> +err0:
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(phy_create);

> +/**
> + * devm_phy_create() - create a new phy
> + * @dev: device that is creating the new phy
> + * @dev: device that is creating the new phy

Duplicated line.

> + * @label: label given to phy
> + * @of_node: device node of the phy
> + * @type: specifies the phy type
> + * @ops: function pointers for performing phy operations
> + * @priv: private data from phy driver
> + *
> + * Creates a new PHY device adding it to the PHY class.
> + * 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.
> + */

> +/**
> + * 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, int index,
> +				const char *phy_dev_name)
> +{
> +	struct phy_bind *phy_bind;
> +
> +	mutex_lock(&phy_bind_mutex);
> +	list_for_each_entry(phy_bind,&phy_bind_list, list) {
> +		if (!strcmp(phy_bind->dev_name, dev_name)&&  phy_bind->index ==
> +			index) {
> +			phy_bind->phy_dev_name = phy_dev_name;
> +			goto ret0;
> +		}
> +	}
> +
> +	phy_bind = kzalloc(sizeof(*phy_bind), GFP_KERNEL);
> +	if (!phy_bind) {
> +		phy_bind = ERR_PTR(-ENOMEM);
> +		goto ret0;
> +	}
> +
> +	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);
> +
> +ret0:
> +	mutex_unlock(&phy_bind_mutex);
> +	return phy_bind;
> +}
> +EXPORT_SYMBOL_GPL(phy_bind);

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

Shouldn't this iteration also be protected with the phy_bind_mutex ?

> +	list_for_each_entry(phy_bind,&phy_bind_list, list)
> +		if (phy_bind->phy == phy)
> +			p += sprintf(p, "%s %d %s\n", phy_bind->dev_name,
> +				phy_bind->index, phy_bind->phy_dev_name);
> +	len = (p - buf);
> +
> +	return len;
> +}

> +static int phy_core_init(void)
> +{
> +	if (phy_class)
> +		return 0;
> +
> +	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;
> +}
> +module_init(phy_core_init);

Having this framework initialized before the PHY provider and consumer
drivers could save some unnecessary probe deferrals. I was wondering,
what is really an advantage of having it as module_init(), rather than
subsys_initcall() ?

> +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..97a48bd
> --- /dev/null
> +++ b/include/linux/phy/phy.h
> @@ -0,0 +1,228 @@
> +/*
> + * phy.h -- generic phy header file
> + *

> +/**
> + * struct phy - represent the phy device
> + * @dev: phy device
> + * @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 {
> +	struct device		dev;
> +	const char		*label;
> +	int			type;
> +	struct device_node	*of_node;
> +	struct phy_ops		*ops;

Something in this structure that would identify a PHY when a PHY provider
supports multiple PHYs is probably needed.

> +};
> +
> +/**
> + * 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;
> +	int		index;
> +	int		auto_bind:1;
> +	struct phy	*phy;
> +	struct list_head list;
> +};
> +
> +static inline int phy_poweron(struct phy *phy)

phy_power_on ? Just a humble suggestion...

> +{
> +	if (phy->ops->power_on)
> +		return phy->ops->power_on(phy);
> +
> +	return -EINVAL;
> +}
> +
> +static inline int phy_poweroff(struct phy *phy)

and phy_power_off ? :-)

> +{
> +	if (phy->ops->power_off)
> +		return phy->ops->power_off(phy);
> +
> +	return -EINVAL;
> +}
> +#endif /* __DRIVERS_PHY_H */

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

On 04/03/2013 06:53 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,
> 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

> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h

> +extern struct phy *devm_phy_create(struct device *dev, const char *label,
> +	struct device_node *of_node, int type, struct phy_ops *ops,
> +	void *priv);

Can't the function get of_node from dev->of_node?

I wonder if we shouldn't split up the registration a bit though:

A function which registers a PHY object itself. That's the function above.

A function which registers a DT-based PHY provider.

Then, the of_xlate op would be part of the PHY provider, not part of
some random PHY that happens to exist on that node. So:

struct phy {
	struct device *dev;
	struct module *owner;
	int	(*init)(struct phy *phy);
	int	(*exit)(struct phy *phy);
	int	(*suspend)(struct phy *phy);
	int	(*resume)(struct phy *phy);
	int	(*power_on)(struct phy *phy);
	int	(*power_off)(struct phy *phy);
};

int phy_register(struct phy *phy);

All PHY providers would use that API, whether running in a DT-base
system or not.

struct of_phy_provider {
	struct device *dev;
	struct phy * (*of_xlate)(struct of_phy_provider *provider,
			struct of_phandle_args *args);
};

int phy_register_of_provider(struct of_phy_provider *provider);

Only DT-based PHY providers would use that API.

... or something like that?

phy_get() would do something like:

	if dev->of_node:
		# look up using registerd of_phy_providers
		phy = phy_get_of(...)
		if phy: return phy
	# now look up using whatever other mapping table exists
	phy = ...
	return phy

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

Hi,

On Wed, Apr 03, 2013 at 05:54:11PM -0600, Stephen Warren wrote:
> struct phy {
> 	struct device *dev;
> 	struct module *owner;
> 	int	(*init)(struct phy *phy);
> 	int	(*exit)(struct phy *phy);
> 	int	(*suspend)(struct phy *phy);
> 	int	(*resume)(struct phy *phy);

I wonder whether it makes sense to provide ->suspend/->resume callbacks
here. We already have dev_pm_ops providing those methods and we can just
wrap pm_runtime_* calls to be called by consumers.

Hi,

On Wednesday 03 April 2013 09:17 PM, Felipe Balbi wrote:
> Hi,
>
> On Wed, Apr 03, 2013 at 08:02:52PM +0530, Kishon Vijay Abraham I wrote:
>>>>>> +		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 :-)
>>>>
>>>> This is for the case where the PHY driver gets loaded before the PHY
>>>> framework. I could have returned EPROBE_DEFER here instead I thought
>>>> will have it this way.
>>>
>>> looks a bit weird IMO. Is it really possible for PHY to load before ?
>>
>> yeah. it actually happened when I tried with beagle and had all the
>> modules as built-in. Because twl4030 has subsys_initcall(), it loads
>> before PHY framework.
>
> that's a bug in twl4030.

right. Will fix it.

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

Hi,

On Thursday 04 April 2013 03:16 AM, Sylwester Nawrocki wrote:
> On 04/03/2013 02:53 PM, Kishon Vijay Abraham I wrote:
.
.
<snip>
.
.
>> 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@ti.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
>
> Shouldn't it be "...medium, e.g. the USB..." ?
>
>> +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
>
> "controllers have PHY functionality embedded into them" ?
>
>> +PHY. Other peripherals that uses a PHY include Wireless LAN, Ethernet,
>
> s/uses/use ?
>
>> +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
>
> s/uses/use ?
>
>> +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.
>
> s/in the dt data of/in the device tree node of ?
>
>> +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);
>
> Why do we need separate functions for dt and non-dt ? Couldn't it be
> handled
> internally by the framework ? So the PHY users can use same calls for dt
> and non-dt, like in case of e.g. the regulators API ?

yeah. Indeed it makes sense to do it that way.
>
> Also signatures of some functions are different now:
>
> extern struct phy *phy_get(struct device *dev, int index);
> extern struct phy *devm_phy_get(struct device *dev, int index);
> extern struct phy *of_phy_get(struct device *dev, int index);
> extern struct phy *devm_of_phy_get(struct device *dev, int index);

My bad :-(

>
> BTW, I think "extern" could be dropped, does it have any significance in
> function declarations in header files ?

it shouldn't have any effect I guess. It's harmless nevertheless.

>
>> +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
>
> s/the the/the
>
>> +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
>
> s/phandle/property ?
>
>> +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.
>
> s/APIS/APIs ?
>
>> +
>> +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
>
> s/APIs destroys/APIs destroy ?
>
>> +associated with this PHY.
>> +
>> +7. DeviceTree Binding
>> +
>> +The documentation for PHY dt binding can be found @
>> +Documentation/devicetree/bindings/phy/phy-bindings.txt
>
>> --- /dev/null
>> +++ b/drivers/phy/phy-core.c
>> @@ -0,0 +1,616 @@
>
>> +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);
>
> There is currently nothing preventing a call to this function before
> phy_class is initialized. It happened during my tests, and the nice
> stack dump showed that it was the PHY user attempting to get the PHY
> before the framework got initialized. So either phy_core_init should
> be a subsys_initcall or we need a better protection against phy_class
> being NULL or ERR_PTR in more places.

Whats the initcall used in your PHY user? Looks like more people prefer 
having module_init and any issue because of it should be fixed in PHY 
providers and PHY users.
>
>> +    while ((dev = class_dev_iter_next(&iter))) {
>> +        phy = container_of(dev, struct phy, dev);
>> +        if (node != phy->of_node)
>> +            continue;
>> +
>> +        class_dev_iter_exit(&iter);
>> +        return phy;
>> +    }
>> +
>> +    class_dev_iter_exit(&iter);
>> +    return ERR_PTR(-EPROBE_DEFER);
>> +}
>
>> +/**
>> + * of_phy_get() - lookup and obtain a reference to a phy by phandle
>> + * @dev: device that requests this phy
>> + * @index: the index of the phy
>> + *
>> + * Returns the phy associated with the given phandle value,
>> + * after getting a refcount to it or -ENODEV if there is no such phy or
>> + * -EPROBE_DEFER if there is a phandle to the phy, but the device is
>> + * not yet loaded.
>> + */
>> +struct phy *of_phy_get(struct device *dev, 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);
>> +    if (IS_ERR(phy))
>> +        goto err1;
>> +
>> +    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;
>
> Shouldn't it be done with the phy_bind_mutex held ? I guess an unlocked
> version of the phy_bind functions is needed, so it can be used internally.

The locking is done inside phy_bind function. I'm not sure but I vaguely 
remember getting a dump stack (incorrect mutex ordering or something) 
when trying to have phy_bind_mutex here. I can check it again.
>
>> +    }
>> +
>> +    get_device(&phy->dev);
>> +
>> +err1:
>> +    module_put(phy->ops->owner);
>> +
>> +err0:
>> +    of_node_put(node);
>> +
>> +    return phy;
>> +}
>> +EXPORT_SYMBOL_GPL(of_phy_get);
>
>> +/**
>> + * phy_create() - create a new phy
>> + * @dev: device that is creating the new phy
>> + * @label: label given to phy
>> + * @of_node: device node of the phy
>> + * @type: specifies the phy type
>> + * @ops: function pointers for performing phy operations
>> + * @priv: private data from phy driver
>> + *
>> + * Called to create a phy using phy framework.
>> + */
>> +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");
>> +        ret = -EINVAL;
>> +        goto err0;
>> +    }
>> +
>> +    if (!ops || !ops->of_xlate || !priv) {
>> +        dev_err(dev, "no PHY ops/PHY data provided\n");
>> +        ret = -EINVAL;
>> +        goto err0;
>> +    }
>> +
>> +    if (!phy_class)
>> +        phy_core_init();
>> +
>> +    phy = kzalloc(sizeof(*phy), GFP_KERNEL);
>> +    if (!phy) {
>> +        ret = -ENOMEM;
>> +        goto err0;
>> +    }
>> +
>> +    devname = dev_name(dev);
>
> OK, a sort of serious issue here is that you can't call phy_create()
> multiple times for same PHY provider device.

Ah.. right.
>
> device_add() further will fail as sysfs won't let you create multiple
> objects with same name. So I would assume we need to add a new argument
> to phy_create() or rename @type to e.g. @phy_id, which could be
> concatenated with dev_name(dev) to create a unique name of the phy
> device.

hmm.. ok
>
> And how is the PHY provider supposed to identify a PHY in its common PHY
> ops, now when the struct phy port field is removed ? I have temporarily
> (ab)used the type field in order to select proper registers within the
> phy ops.

Can't it be handled in the PHY provider driver without using struct phy 
*? Moreover the PHY ops passes on the *phy to phy provider right? Not 
sure I understand you here.
>
>> +    device_initialize(&phy->dev);
>> +
>> +    phy->dev.class = phy_class;
>> +    phy->dev.parent = dev;
>> +    phy->label = label;
>
> What about duplicating the string here ? That would make the API a bit
> more convenient and the callers wouldn't be required to keep all the
> labels around.

you mean use dev_name? The device names are sometime more cryptic so 
preferred to have it like this.
>
>> +    phy->of_node = of_node;
>> +    phy->type = type;
>> +    phy->ops = ops;
>> +
>> +    dev_set_drvdata(&phy->dev, priv);
>> +
>> +    ret = dev_set_name(&phy->dev, "%s", devname);
>> +    if (ret)
>> +        goto err1;
>> +
>> +    mutex_lock(&phy_bind_mutex);
>> +    list_for_each_entry(phy_bind,&phy_bind_list, list)
>> +        if (!(strcmp(phy_bind->phy_dev_name, devname)))
>> +            phy_bind->phy = phy;
>> +    mutex_unlock(&phy_bind_mutex);
>> +
>> +    ret = device_add(&phy->dev);
>> +    if (ret)
>> +        goto err2;
>> +
>> +    return phy;
>> +
>> +err2:
>> +    phy_bind->phy = NULL;
>> +
>> +err1:
>> +    put_device(&phy->dev);
>> +    kfree(phy);
>> +
>> +err0:
>> +    return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(phy_create);
>
>> +/**
>> + * devm_phy_create() - create a new phy
>> + * @dev: device that is creating the new phy
>> + * @dev: device that is creating the new phy
>
> Duplicated line.
>
>> + * @label: label given to phy
>> + * @of_node: device node of the phy
>> + * @type: specifies the phy type
>> + * @ops: function pointers for performing phy operations
>> + * @priv: private data from phy driver
>> + *
>> + * Creates a new PHY device adding it to the PHY class.
>> + * 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.
>> + */
>
>> +/**
>> + * 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, int index,
>> +                const char *phy_dev_name)
>> +{
>> +    struct phy_bind *phy_bind;
>> +
>> +    mutex_lock(&phy_bind_mutex);
>> +    list_for_each_entry(phy_bind,&phy_bind_list, list) {
>> +        if (!strcmp(phy_bind->dev_name, dev_name)&&  phy_bind->index ==
>> +            index) {
>> +            phy_bind->phy_dev_name = phy_dev_name;
>> +            goto ret0;
>> +        }
>> +    }
>> +
>> +    phy_bind = kzalloc(sizeof(*phy_bind), GFP_KERNEL);
>> +    if (!phy_bind) {
>> +        phy_bind = ERR_PTR(-ENOMEM);
>> +        goto ret0;
>> +    }
>> +
>> +    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);
>> +
>> +ret0:
>> +    mutex_unlock(&phy_bind_mutex);
>> +    return phy_bind;
>> +}
>> +EXPORT_SYMBOL_GPL(phy_bind);
>
>> +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);
>> +
>
> Shouldn't this iteration also be protected with the phy_bind_mutex ?

Indeed.
>
>> +    list_for_each_entry(phy_bind,&phy_bind_list, list)
>> +        if (phy_bind->phy == phy)
>> +            p += sprintf(p, "%s %d %s\n", phy_bind->dev_name,
>> +                phy_bind->index, phy_bind->phy_dev_name);
>> +    len = (p - buf);
>> +
>> +    return len;
>> +}
>
>> +static int phy_core_init(void)
>> +{
>> +    if (phy_class)
>> +        return 0;
>> +
>> +    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;
>> +}
>> +module_init(phy_core_init);
>
> Having this framework initialized before the PHY provider and consumer
> drivers could save some unnecessary probe deferrals. I was wondering,
> what is really an advantage of having it as module_init(), rather than
> subsys_initcall() ?

I'm not sure of the exact reason but after the advent of EPROBE_DEFER, 
everyone recommends to use module_init only.

Thanks for the detailed review.

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

Hi,

On 04/04/2013 11:21 AM, Kishon Vijay Abraham I wrote:
> On Thursday 04 April 2013 03:16 AM, Sylwester Nawrocki wrote:
>> On 04/03/2013 02:53 PM, Kishon Vijay Abraham I wrote:

>>> +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);
>>
>> Why do we need separate functions for dt and non-dt ? Couldn't it be
>> handled
>> internally by the framework ? So the PHY users can use same calls for dt
>> and non-dt, like in case of e.g. the regulators API ?
> 
> yeah. Indeed it makes sense to do it that way.
>
>> Also signatures of some functions are different now:
>>
>> extern struct phy *phy_get(struct device *dev, int index);
>> extern struct phy *devm_phy_get(struct device *dev, int index);
>> extern struct phy *of_phy_get(struct device *dev, int index);
>> extern struct phy *devm_of_phy_get(struct device *dev, int index);
> 
> My bad :-(
> 
>> BTW, I think "extern" could be dropped, does it have any significance in
>> function declarations in header files ?
> 
> it shouldn't have any effect I guess. It's harmless nevertheless.

Yup.

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

>>> --- /dev/null
>>> +++ b/drivers/phy/phy-core.c
>>> @@ -0,0 +1,616 @@
>>
>>> +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);
>>
>> There is currently nothing preventing a call to this function before
>> phy_class is initialized. It happened during my tests, and the nice
>> stack dump showed that it was the PHY user attempting to get the PHY
>> before the framework got initialized. So either phy_core_init should
>> be a subsys_initcall or we need a better protection against phy_class
>> being NULL or ERR_PTR in more places.
> 
> Whats the initcall used in your PHY user? Looks like more people prefer having

It happened in a regular platform driver probe() callback.

> module_init and any issue because of it should be fixed in PHY providers and
> PHY users.

OK. In fact this uncovered some issues in the MIPI DSI interface driver
(some unexpected interrupts). But this should just be fixed in those drivers
anyway. Now the DSI interface driver needs to wait for the PHY to be
registered and ready, so the initialization will likely be changed regardless
the framework initializes in module_init or earlier.

>>> +    while ((dev = class_dev_iter_next(&iter))) {
>>> +        phy = container_of(dev, struct phy, dev);
>>> +        if (node != phy->of_node)
>>> +            continue;
>>> +
>>> +        class_dev_iter_exit(&iter);
>>> +        return phy;
>>> +    }
>>> +
>>> +    class_dev_iter_exit(&iter);
>>> +    return ERR_PTR(-EPROBE_DEFER);
>>> +}
>>
>>> +/**
>>> + * of_phy_get() - lookup and obtain a reference to a phy by phandle
>>> + * @dev: device that requests this phy
>>> + * @index: the index of the phy
>>> + *
>>> + * Returns the phy associated with the given phandle value,
>>> + * after getting a refcount to it or -ENODEV if there is no such phy or
>>> + * -EPROBE_DEFER if there is a phandle to the phy, but the device is
>>> + * not yet loaded.
>>> + */
>>> +struct phy *of_phy_get(struct device *dev, 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);
>>> +    if (IS_ERR(phy))
>>> +        goto err1;
>>> +
>>> +    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;
>>
>> Shouldn't it be done with the phy_bind_mutex held ? I guess an unlocked
>> version of the phy_bind functions is needed, so it can be used internally.
> 
> The locking is done inside phy_bind function. I'm not sure but I vaguely
> remember getting a dump stack (incorrect mutex ordering or something) when
> trying to have phy_bind_mutex here. I can check it again.

Yes, IIUC the locking needs to be reworked a bit so "phy_map" is not modified
without the mutex held.

>>> +    }
>>> +
>>> +    get_device(&phy->dev);
>>> +
>>> +err1:
>>> +    module_put(phy->ops->owner);
>>> +
>>> +err0:
>>> +    of_node_put(node);
>>> +
>>> +    return phy;
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_phy_get);
>>
>>> +/**
>>> + * phy_create() - create a new phy
>>> + * @dev: device that is creating the new phy
>>> + * @label: label given to phy
>>> + * @of_node: device node of the phy
>>> + * @type: specifies the phy type
>>> + * @ops: function pointers for performing phy operations
>>> + * @priv: private data from phy driver
>>> + *
>>> + * Called to create a phy using phy framework.
>>> + */
>>> +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");
>>> +        ret = -EINVAL;
>>> +        goto err0;
>>> +    }
>>> +
>>> +    if (!ops || !ops->of_xlate || !priv) {
>>> +        dev_err(dev, "no PHY ops/PHY data provided\n");
>>> +        ret = -EINVAL;
>>> +        goto err0;
>>> +    }
>>> +
>>> +    if (!phy_class)
>>> +        phy_core_init();
>>> +
>>> +    phy = kzalloc(sizeof(*phy), GFP_KERNEL);
>>> +    if (!phy) {
>>> +        ret = -ENOMEM;
>>> +        goto err0;
>>> +    }
>>> +
>>> +    devname = dev_name(dev);
>>
>> OK, a sort of serious issue here is that you can't call phy_create()
>> multiple times for same PHY provider device.
> 
> Ah.. right.

The other question I had was why we needed struct device object for each
PHY. And if it wouldn't be enough to have only struct device * in
struct phy, and the real device would be the PHY provider only. I might
be missing some reasons behind this decision though, as I have not been
following your work since the beginning.

>> device_add() further will fail as sysfs won't let you create multiple
>> objects with same name. So I would assume we need to add a new argument
>> to phy_create() or rename @type to e.g. @phy_id, which could be
>> concatenated with dev_name(dev) to create a unique name of the phy
>> device.
> 
> hmm.. ok
>>
>> And how is the PHY provider supposed to identify a PHY in its common PHY
>> ops, now when the struct phy port field is removed ? I have temporarily
>> (ab)used the type field in order to select proper registers within the
>> phy ops.
> 
> Can't it be handled in the PHY provider driver without using struct phy *?

You need to select registers/bit fields corresponding to a specific PHY,
since the phy ops are common. So we need to know exactly with which phy
we deal at the moment and I can't see any other option to figure out that
than by dereferencing struct phy *... ;)

> Moreover the PHY ops passes on the *phy to phy provider right? Not sure I

Yes, all you get in the ops is phy *. I wanted to avoid global variables
in the provider driver and be able to get to any provider private data
from phy * only.

If I use something like below for the PHY provider private data then from
struct phy * only I would need to get an index. This could be looked up by
iterating over phys[] array, but that seems an unnecessary complication.
Maybe there are better/easier ways to resolve it, without adding a new
field to struct phy.

struct phy_povider_priv {
	struct phy *phys[4];
	...
};

static in phy_power_on(struct phy *phy)
{
	struct phy_provider_priv *priv = dev_get_drvdata(&phy->dev);
	int phy_id = ...
	....
}
...
struct phy_ops ops = {
	.power_on = phy_power_on,
	...
};

> understand you here.
>
>>> +    device_initialize(&phy->dev);
>>> +
>>> +    phy->dev.class = phy_class;
>>> +    phy->dev.parent = dev;
>>> +    phy->label = label;
>>
>> What about duplicating the string here ? That would make the API a bit
>> more convenient and the callers wouldn't be required to keep all the
>> labels around.
> 
> you mean use dev_name? The device names are sometime more cryptic so preferred
> to have it like this.

No, I meant something like:

phy->label = kstrdup(label, GFP_KERNEL);

>>> +static int phy_core_init(void)
>>> +{
>>> +    if (phy_class)
>>> +        return 0;
>>> +
>>> +    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;
>>> +}
>>> +module_init(phy_core_init);
>>
>> Having this framework initialized before the PHY provider and consumer
>> drivers could save some unnecessary probe deferrals. I was wondering,
>> what is really an advantage of having it as module_init(), rather than
>> subsys_initcall() ?
> 
> I'm not sure of the exact reason but after the advent of EPROBE_DEFER, everyone
> recommends to use module_init only.

Ah...OK. We just need to ensure the resources are properly checked before
they are used then.

> Thanks for the detailed review.

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

Hi,

On Thursday 04 April 2013 04:11 PM, Sylwester Nawrocki wrote:
> Hi,
>
> On 04/04/2013 11:21 AM, Kishon Vijay Abraham I wrote:
>> On Thursday 04 April 2013 03:16 AM, Sylwester Nawrocki wrote:
>>> On 04/03/2013 02:53 PM, Kishon Vijay Abraham I wrote:
>
>>>> +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);
>>>
>>> Why do we need separate functions for dt and non-dt ? Couldn't it be
>>> handled
>>> internally by the framework ? So the PHY users can use same calls for dt
>>> and non-dt, like in case of e.g. the regulators API ?
>>
>> yeah. Indeed it makes sense to do it that way.
>>
>>> Also signatures of some functions are different now:
>>>
>>> extern struct phy *phy_get(struct device *dev, int index);
>>> extern struct phy *devm_phy_get(struct device *dev, int index);
>>> extern struct phy *of_phy_get(struct device *dev, int index);
>>> extern struct phy *devm_of_phy_get(struct device *dev, int index);
>>
>> My bad :-(
>>
>>> BTW, I think "extern" could be dropped, does it have any significance in
>>> function declarations in header files ?
>>
>> it shouldn't have any effect I guess. It's harmless nevertheless.
>
> Yup.
>
>>>> +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);
>
>>>> --- /dev/null
>>>> +++ b/drivers/phy/phy-core.c
>>>> @@ -0,0 +1,616 @@
>>>
>>>> +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);
>>>
>>> There is currently nothing preventing a call to this function before
>>> phy_class is initialized. It happened during my tests, and the nice
>>> stack dump showed that it was the PHY user attempting to get the PHY
>>> before the framework got initialized. So either phy_core_init should
>>> be a subsys_initcall or we need a better protection against phy_class
>>> being NULL or ERR_PTR in more places.
>>
>> Whats the initcall used in your PHY user? Looks like more people prefer having
>
> It happened in a regular platform driver probe() callback.
>
>> module_init and any issue because of it should be fixed in PHY providers and
>> PHY users.
>
> OK. In fact this uncovered some issues in the MIPI DSI interface driver
> (some unexpected interrupts). But this should just be fixed in those drivers
> anyway. Now the DSI interface driver needs to wait for the PHY to be
> registered and ready, so the initialization will likely be changed regardless
> the framework initializes in module_init or earlier.
>
>>>> +    while ((dev = class_dev_iter_next(&iter))) {
>>>> +        phy = container_of(dev, struct phy, dev);
>>>> +        if (node != phy->of_node)
>>>> +            continue;
>>>> +
>>>> +        class_dev_iter_exit(&iter);
>>>> +        return phy;
>>>> +    }
>>>> +
>>>> +    class_dev_iter_exit(&iter);
>>>> +    return ERR_PTR(-EPROBE_DEFER);
>>>> +}
>>>
>>>> +/**
>>>> + * of_phy_get() - lookup and obtain a reference to a phy by phandle
>>>> + * @dev: device that requests this phy
>>>> + * @index: the index of the phy
>>>> + *
>>>> + * Returns the phy associated with the given phandle value,
>>>> + * after getting a refcount to it or -ENODEV if there is no such phy or
>>>> + * -EPROBE_DEFER if there is a phandle to the phy, but the device is
>>>> + * not yet loaded.
>>>> + */
>>>> +struct phy *of_phy_get(struct device *dev, 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);
>>>> +    if (IS_ERR(phy))
>>>> +        goto err1;
>>>> +
>>>> +    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;
>>>
>>> Shouldn't it be done with the phy_bind_mutex held ? I guess an unlocked
>>> version of the phy_bind functions is needed, so it can be used internally.
>>
>> The locking is done inside phy_bind function. I'm not sure but I vaguely
>> remember getting a dump stack (incorrect mutex ordering or something) when
>> trying to have phy_bind_mutex here. I can check it again.
>
> Yes, IIUC the locking needs to be reworked a bit so "phy_map" is not modified
> without the mutex held.
>
>>>> +    }
>>>> +
>>>> +    get_device(&phy->dev);
>>>> +
>>>> +err1:
>>>> +    module_put(phy->ops->owner);
>>>> +
>>>> +err0:
>>>> +    of_node_put(node);
>>>> +
>>>> +    return phy;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(of_phy_get);
>>>
>>>> +/**
>>>> + * phy_create() - create a new phy
>>>> + * @dev: device that is creating the new phy
>>>> + * @label: label given to phy
>>>> + * @of_node: device node of the phy
>>>> + * @type: specifies the phy type
>>>> + * @ops: function pointers for performing phy operations
>>>> + * @priv: private data from phy driver
>>>> + *
>>>> + * Called to create a phy using phy framework.
>>>> + */
>>>> +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");
>>>> +        ret = -EINVAL;
>>>> +        goto err0;
>>>> +    }
>>>> +
>>>> +    if (!ops || !ops->of_xlate || !priv) {
>>>> +        dev_err(dev, "no PHY ops/PHY data provided\n");
>>>> +        ret = -EINVAL;
>>>> +        goto err0;
>>>> +    }
>>>> +
>>>> +    if (!phy_class)
>>>> +        phy_core_init();
>>>> +
>>>> +    phy = kzalloc(sizeof(*phy), GFP_KERNEL);
>>>> +    if (!phy) {
>>>> +        ret = -ENOMEM;
>>>> +        goto err0;
>>>> +    }
>>>> +
>>>> +    devname = dev_name(dev);
>>>
>>> OK, a sort of serious issue here is that you can't call phy_create()
>>> multiple times for same PHY provider device.
>>
>> Ah.. right.
>
> The other question I had was why we needed struct device object for each
> PHY. And if it wouldn't be enough to have only struct device * in
> struct phy, and the real device would be the PHY provider only. I might

IMO the PHY framework shouldn't be modifying the properties of device of 
some other driver. Consider the case where the PHY provider implements 
other functions also (in addition to acting as a PHY) in which case the 
PHY provider entirely doesn't come under *PHY class* but only a part of 
it. There might be other reasons as well.

Thanks
Kishon


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

Hi,

On Thursday 04 April 2013 05:24 AM, Stephen Warren wrote:
> On 04/03/2013 06:53 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,
>> 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
>
>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
>
>> +extern struct phy *devm_phy_create(struct device *dev, const char *label,
>> +	struct device_node *of_node, int type, struct phy_ops *ops,
>> +	void *priv);
>
> Can't the function get of_node from dev->of_node?
>
> I wonder if we shouldn't split up the registration a bit though:
>
> A function which registers a PHY object itself. That's the function above.
>
> A function which registers a DT-based PHY provider.
>
> Then, the of_xlate op would be part of the PHY provider, not part of
> some random PHY that happens to exist on that node. So:

Yeah. Makes sense to me. By this we can avoid struct phy * to represent 
two different things (phy provider and phy instance).

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