[5/9] usb: add phy connection by phy-mode

Commit Message

Michael Grzeschik Nov. 14, 2012, 4:19 p.m. UTC

This patch makes it possible to set the connection of the usbphy to the
soc. It is derived from the oftree bindings for the ethernetphy and adds
similar helperfunctions.

Signed-off-by: Michael Grzeschik <[e-mail]>
Signed-off-by: Marc Kleine-Budde <[e-mail]>
---
 drivers/of/Kconfig        |    4 ++++
 drivers/of/Makefile       |    1 +
 drivers/of/of_usbphy.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_usbphy.h |   15 ++++++++++++++
 include/linux/usb/phy.h   |    8 ++++++++
 5 files changed, 77 insertions(+)
 create mode 100644 drivers/of/of_usbphy.c
 create mode 100644 include/linux/of_usbphy.h

Comments

Alexander Shishkin Nov. 16, 2012, 9:25 a.m. UTC | #1

Michael Grzeschik <[e-mail]> writes:

> This patch makes it possible to set the connection of the usbphy to the
> soc. It is derived from the oftree bindings for the ethernetphy and adds
> similar helperfunctions.
>
> Signed-off-by: Michael Grzeschik <[e-mail]>
> Signed-off-by: Marc Kleine-Budde <[e-mail]>
> ---
>  drivers/of/Kconfig        |    4 ++++
>  drivers/of/Makefile       |    1 +
>  drivers/of/of_usbphy.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_usbphy.h |   15 ++++++++++++++
>  include/linux/usb/phy.h   |    8 ++++++++
>  5 files changed, 77 insertions(+)
>  create mode 100644 drivers/of/of_usbphy.c
>  create mode 100644 include/linux/of_usbphy.h

This looks a bit like Felipe's call, cc'ing him.

>
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..28f99fb 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -67,6 +67,10 @@ config OF_MDIO
>  	help
>  	  OpenFirmware MDIO bus (Ethernet PHY) accessors
>  
> +config OF_USBPHY
> +	depends on USB
> +	def_bool y
> +
>  config OF_PCI
>  	def_tristate PCI
>  	depends on PCI
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..fdcaf51 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_OF_IRQ)    += irq.o
>  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>  obj-$(CONFIG_OF_I2C)	+= of_i2c.o
>  obj-$(CONFIG_OF_NET)	+= of_net.o
> +obj-$(CONFIG_OF_USBPHY)	+= of_usbphy.o
>  obj-$(CONFIG_OF_SELFTEST) += selftest.o
>  obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>  obj-$(CONFIG_OF_PCI)	+= of_pci.o
> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
> new file mode 100644
> index 0000000..2e71f7b
> --- /dev/null
> +++ b/drivers/of/of_usbphy.c
> @@ -0,0 +1,49 @@
> +/*
> + * OF helpers for network devices.
> + *
> + * This file is released under the GPLv2
> + *
> + * Initially copied out of drivers/of/of_net.c
> + */
> +#include <linux/etherdevice.h>
> +#include <linux/kernel.h>
> +#include <linux/of_usbphy.h>
> +#include <linux/usb/phy.h>
> +#include <linux/export.h>
> +
> +/**
> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
> + * into the device tree binding of 'phy-mode', so that USB
> + * device driver can get phy interface from device tree.
> + */
> +static const char *usbphy_modes[] = {
> +	[USBPHY_INTERFACE_MODE_NA]	= "",
> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
> +};
> +
> +/**
> + * of_get_phy_mode - Get phy mode for given device_node
> + * @np:	Pointer to the given device_node
> + *
> + * The function gets phy interface string from property 'phy-mode',
> + * and return its index in phy_modes table, or errno in error case.
> + */
> +const int of_get_usbphy_mode(struct device_node *np)
> +{
> +	const char *pm;
> +	int err, i;
> +
> +	err = of_property_read_string(np, "phy-mode", &pm);
> +	if (err < 0)
> +		return err;
> +
> +	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
> +		if (!strcasecmp(pm, usbphy_modes[i]))
> +			return i;
> +
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(of_get_usbphy_mode);
> diff --git a/include/linux/of_usbphy.h b/include/linux/of_usbphy.h
> new file mode 100644
> index 0000000..9a4132d
> --- /dev/null
> +++ b/include/linux/of_usbphy.h
> @@ -0,0 +1,15 @@
> +/*
> + * OF helpers for usb devices.
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_USBPHY_H
> +#define __LINUX_OF_USBPHY_H
> +
> +#ifdef CONFIG_OF_USBPHY
> +#include <linux/of.h>
> +extern const int of_get_usbphy_mode(struct device_node *np);
> +#endif
> +
> +#endif /* __LINUX_OF_USBPHY_H */
> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> index a29ae1e..150eb69 100644
> --- a/include/linux/usb/phy.h
> +++ b/include/linux/usb/phy.h
> @@ -12,6 +12,14 @@
>  #include <linux/notifier.h>
>  #include <linux/usb.h>
>  
> +enum usb_phy_interface {
> +	USBPHY_INTERFACE_MODE_NA,
> +	USBPHY_INTERFACE_MODE_UTMI,
> +	USBPHY_INTERFACE_MODE_UTMIW,
> +	USBPHY_INTERFACE_MODE_ULPI,
> +	USBPHY_INTERFACE_MODE_SERIAL,
> +};
> +
>  enum usb_phy_events {
>  	USB_EVENT_NONE,         /* no events or cable disconnected */
>  	USB_EVENT_VBUS,         /* vbus valid event */
> -- 
> 1.7.10.4

Felipe Balbi Nov. 16, 2012, 11:28 a.m. UTC | #2

Hi,

On Wed, Nov 14, 2012 at 05:19:06PM +0100, Michael Grzeschik wrote:
> This patch makes it possible to set the connection of the usbphy to the
> soc. It is derived from the oftree bindings for the ethernetphy and adds
> similar helperfunctions.
> 
> Signed-off-by: Michael Grzeschik <[e-mail]>
> Signed-off-by: Marc Kleine-Budde <[e-mail]>

I think Kishon had sent some patches related to this. Kishon ?

> ---
>  drivers/of/Kconfig        |    4 ++++
>  drivers/of/Makefile       |    1 +
>  drivers/of/of_usbphy.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_usbphy.h |   15 ++++++++++++++
>  include/linux/usb/phy.h   |    8 ++++++++
>  5 files changed, 77 insertions(+)
>  create mode 100644 drivers/of/of_usbphy.c
>  create mode 100644 include/linux/of_usbphy.h
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..28f99fb 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -67,6 +67,10 @@ config OF_MDIO
>  	help
>  	  OpenFirmware MDIO bus (Ethernet PHY) accessors
>  
> +config OF_USBPHY
> +	depends on USB
> +	def_bool y
> +
>  config OF_PCI
>  	def_tristate PCI
>  	depends on PCI
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..fdcaf51 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_OF_IRQ)    += irq.o
>  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>  obj-$(CONFIG_OF_I2C)	+= of_i2c.o
>  obj-$(CONFIG_OF_NET)	+= of_net.o
> +obj-$(CONFIG_OF_USBPHY)	+= of_usbphy.o
>  obj-$(CONFIG_OF_SELFTEST) += selftest.o
>  obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>  obj-$(CONFIG_OF_PCI)	+= of_pci.o
> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
> new file mode 100644
> index 0000000..2e71f7b
> --- /dev/null
> +++ b/drivers/of/of_usbphy.c
> @@ -0,0 +1,49 @@
> +/*
> + * OF helpers for network devices.
> + *
> + * This file is released under the GPLv2
> + *
> + * Initially copied out of drivers/of/of_net.c
> + */
> +#include <linux/etherdevice.h>
> +#include <linux/kernel.h>
> +#include <linux/of_usbphy.h>
> +#include <linux/usb/phy.h>
> +#include <linux/export.h>
> +
> +/**
> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
> + * into the device tree binding of 'phy-mode', so that USB
> + * device driver can get phy interface from device tree.
> + */
> +static const char *usbphy_modes[] = {
> +	[USBPHY_INTERFACE_MODE_NA]	= "",
> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
> +};
> +
> +/**
> + * of_get_phy_mode - Get phy mode for given device_node
> + * @np:	Pointer to the given device_node
> + *
> + * The function gets phy interface string from property 'phy-mode',
> + * and return its index in phy_modes table, or errno in error case.
> + */
> +const int of_get_usbphy_mode(struct device_node *np)
> +{
> +	const char *pm;
> +	int err, i;
> +
> +	err = of_property_read_string(np, "phy-mode", &pm);
> +	if (err < 0)
> +		return err;
> +
> +	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
> +		if (!strcasecmp(pm, usbphy_modes[i]))
> +			return i;
> +
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(of_get_usbphy_mode);
> diff --git a/include/linux/of_usbphy.h b/include/linux/of_usbphy.h
> new file mode 100644
> index 0000000..9a4132d
> --- /dev/null
> +++ b/include/linux/of_usbphy.h
> @@ -0,0 +1,15 @@
> +/*
> + * OF helpers for usb devices.
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_USBPHY_H
> +#define __LINUX_OF_USBPHY_H
> +
> +#ifdef CONFIG_OF_USBPHY
> +#include <linux/of.h>
> +extern const int of_get_usbphy_mode(struct device_node *np);
> +#endif
> +
> +#endif /* __LINUX_OF_USBPHY_H */
> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> index a29ae1e..150eb69 100644
> --- a/include/linux/usb/phy.h
> +++ b/include/linux/usb/phy.h
> @@ -12,6 +12,14 @@
>  #include <linux/notifier.h>
>  #include <linux/usb.h>
>  
> +enum usb_phy_interface {
> +	USBPHY_INTERFACE_MODE_NA,
> +	USBPHY_INTERFACE_MODE_UTMI,
> +	USBPHY_INTERFACE_MODE_UTMIW,
> +	USBPHY_INTERFACE_MODE_ULPI,
> +	USBPHY_INTERFACE_MODE_SERIAL,
> +};
> +
>  enum usb_phy_events {
>  	USB_EVENT_NONE,         /* no events or cable disconnected */
>  	USB_EVENT_VBUS,         /* vbus valid event */
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Felipe Balbi Nov. 16, 2012, 11:31 a.m. UTC | #3

Hi,

On Wed, Nov 14, 2012 at 05:19:06PM +0100, Michael Grzeschik wrote:
> This patch makes it possible to set the connection of the usbphy to the
> soc. It is derived from the oftree bindings for the ethernetphy and adds
> similar helperfunctions.
> 
> Signed-off-by: Michael Grzeschik <[e-mail]>
> Signed-off-by: Marc Kleine-Budde <[e-mail]>
> ---
>  drivers/of/Kconfig        |    4 ++++
>  drivers/of/Makefile       |    1 +
>  drivers/of/of_usbphy.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_usbphy.h |   15 ++++++++++++++
>  include/linux/usb/phy.h   |    8 ++++++++
>  5 files changed, 77 insertions(+)
>  create mode 100644 drivers/of/of_usbphy.c
>  create mode 100644 include/linux/of_usbphy.h
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..28f99fb 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -67,6 +67,10 @@ config OF_MDIO
>  	help
>  	  OpenFirmware MDIO bus (Ethernet PHY) accessors
>  
> +config OF_USBPHY
> +	depends on USB
> +	def_bool y
> +
>  config OF_PCI
>  	def_tristate PCI
>  	depends on PCI
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..fdcaf51 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_OF_IRQ)    += irq.o
>  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>  obj-$(CONFIG_OF_I2C)	+= of_i2c.o
>  obj-$(CONFIG_OF_NET)	+= of_net.o
> +obj-$(CONFIG_OF_USBPHY)	+= of_usbphy.o
>  obj-$(CONFIG_OF_SELFTEST) += selftest.o
>  obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>  obj-$(CONFIG_OF_PCI)	+= of_pci.o
> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
> new file mode 100644
> index 0000000..2e71f7b
> --- /dev/null
> +++ b/drivers/of/of_usbphy.c
> @@ -0,0 +1,49 @@
> +/*
> + * OF helpers for network devices.
> + *
> + * This file is released under the GPLv2
> + *
> + * Initially copied out of drivers/of/of_net.c
> + */
> +#include <linux/etherdevice.h>
> +#include <linux/kernel.h>
> +#include <linux/of_usbphy.h>
> +#include <linux/usb/phy.h>
> +#include <linux/export.h>
> +
> +/**
> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
> + * into the device tree binding of 'phy-mode', so that USB
> + * device driver can get phy interface from device tree.
> + */

provide proper kernel-doc

> +static const char *usbphy_modes[] = {
> +	[USBPHY_INTERFACE_MODE_NA]	= "",
> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
> +};

In fact, these would be better off as constants:

#define USBPHY_INTERFACE_MODE_UTMI	1
#define USBPHY_INTERFACE_MODE_UTMIW	2
...

> +/**
> + * of_get_phy_mode - Get phy mode for given device_node
> + * @np:	Pointer to the given device_node
> + *
> + * The function gets phy interface string from property 'phy-mode',
> + * and return its index in phy_modes table, or errno in error case.
> + */

why do you pass a string instead of passing an Integer ? This makes no
sense to me.

> +const int of_get_usbphy_mode(struct device_node *np)
> +{
> +	const char *pm;
> +	int err, i;
> +
> +	err = of_property_read_string(np, "phy-mode", &pm);
> +	if (err < 0)
> +		return err;
> +
> +	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
> +		if (!strcasecmp(pm, usbphy_modes[i]))
> +			return i;
> +
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(of_get_usbphy_mode);
> diff --git a/include/linux/of_usbphy.h b/include/linux/of_usbphy.h
> new file mode 100644
> index 0000000..9a4132d
> --- /dev/null
> +++ b/include/linux/of_usbphy.h
> @@ -0,0 +1,15 @@
> +/*
> + * OF helpers for usb devices.
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_USBPHY_H
> +#define __LINUX_OF_USBPHY_H
> +
> +#ifdef CONFIG_OF_USBPHY
> +#include <linux/of.h>
> +extern const int of_get_usbphy_mode(struct device_node *np);
> +#endif
> +
> +#endif /* __LINUX_OF_USBPHY_H */
> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> index a29ae1e..150eb69 100644
> --- a/include/linux/usb/phy.h
> +++ b/include/linux/usb/phy.h
> @@ -12,6 +12,14 @@
>  #include <linux/notifier.h>
>  #include <linux/usb.h>
>  
> +enum usb_phy_interface {
> +	USBPHY_INTERFACE_MODE_NA,
> +	USBPHY_INTERFACE_MODE_UTMI,
> +	USBPHY_INTERFACE_MODE_UTMIW,
> +	USBPHY_INTERFACE_MODE_ULPI,
> +	USBPHY_INTERFACE_MODE_SERIAL,
> +};

no documentation at all here... What does UTMIW mean ? Why do you mean
the NA mode ?

Marc Kleine-Budde Nov. 16, 2012, 11:44 a.m. UTC | #4

On 11/16/2012 12:31 PM, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Nov 14, 2012 at 05:19:06PM +0100, Michael Grzeschik wrote:
>> This patch makes it possible to set the connection of the usbphy to the
>> soc. It is derived from the oftree bindings for the ethernetphy and adds
>> similar helperfunctions.
>>
>> Signed-off-by: Michael Grzeschik <[e-mail]>
>> Signed-off-by: Marc Kleine-Budde <[e-mail]>
>> ---
>>  drivers/of/Kconfig        |    4 ++++
>>  drivers/of/Makefile       |    1 +
>>  drivers/of/of_usbphy.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/of_usbphy.h |   15 ++++++++++++++
>>  include/linux/usb/phy.h   |    8 ++++++++
>>  5 files changed, 77 insertions(+)
>>  create mode 100644 drivers/of/of_usbphy.c
>>  create mode 100644 include/linux/of_usbphy.h
>>
>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>> index dfba3e6..28f99fb 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -67,6 +67,10 @@ config OF_MDIO
>>  	help
>>  	  OpenFirmware MDIO bus (Ethernet PHY) accessors
>>  
>> +config OF_USBPHY
>> +	depends on USB
>> +	def_bool y
>> +
>>  config OF_PCI
>>  	def_tristate PCI
>>  	depends on PCI
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index e027f44..fdcaf51 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -6,6 +6,7 @@ obj-$(CONFIG_OF_IRQ)    += irq.o
>>  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>>  obj-$(CONFIG_OF_I2C)	+= of_i2c.o
>>  obj-$(CONFIG_OF_NET)	+= of_net.o
>> +obj-$(CONFIG_OF_USBPHY)	+= of_usbphy.o
>>  obj-$(CONFIG_OF_SELFTEST) += selftest.o
>>  obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>>  obj-$(CONFIG_OF_PCI)	+= of_pci.o
>> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
>> new file mode 100644
>> index 0000000..2e71f7b
>> --- /dev/null
>> +++ b/drivers/of/of_usbphy.c
>> @@ -0,0 +1,49 @@
>> +/*
>> + * OF helpers for network devices.
>> + *
>> + * This file is released under the GPLv2
>> + *
>> + * Initially copied out of drivers/of/of_net.c
>> + */
>> +#include <linux/etherdevice.h>
>> +#include <linux/kernel.h>
>> +#include <linux/of_usbphy.h>
>> +#include <linux/usb/phy.h>
>> +#include <linux/export.h>
>> +
>> +/**
>> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
>> + * into the device tree binding of 'phy-mode', so that USB
>> + * device driver can get phy interface from device tree.
>> + */
> 
> provide proper kernel-doc
> 
>> +static const char *usbphy_modes[] = {
>> +	[USBPHY_INTERFACE_MODE_NA]	= "",
>> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
>> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
>> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
>> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
>> +};
> 
> In fact, these would be better off as constants:
> 
> #define USBPHY_INTERFACE_MODE_UTMI	1
> #define USBPHY_INTERFACE_MODE_UTMIW	2
> ...

Why are defines better than an enum? BTW: this code is a copy of the
ethernet phy of-helper code.

> 
>> +/**
>> + * of_get_phy_mode - Get phy mode for given device_node
>> + * @np:	Pointer to the given device_node
>> + *
>> + * The function gets phy interface string from property 'phy-mode',
>> + * and return its index in phy_modes table, or errno in error case.
>> + */
> 
> why do you pass a string instead of passing an Integer ? This makes no
> sense to me.

This code returns an integer or rather an enum....see header file below.

>> +const int of_get_usbphy_mode(struct device_node *np)
>> +{
>> +	const char *pm;
>> +	int err, i;
>> +
>> +	err = of_property_read_string(np, "phy-mode", &pm);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
>> +		if (!strcasecmp(pm, usbphy_modes[i]))
>> +			return i;
>> +
>> +	return -ENODEV;
>> +}
>> +EXPORT_SYMBOL_GPL(of_get_usbphy_mode);
>> diff --git a/include/linux/of_usbphy.h b/include/linux/of_usbphy.h
>> new file mode 100644
>> index 0000000..9a4132d
>> --- /dev/null
>> +++ b/include/linux/of_usbphy.h
>> @@ -0,0 +1,15 @@
>> +/*
>> + * OF helpers for usb devices.
>> + *
>> + * This file is released under the GPLv2
>> + */
>> +
>> +#ifndef __LINUX_OF_USBPHY_H
>> +#define __LINUX_OF_USBPHY_H
>> +
>> +#ifdef CONFIG_OF_USBPHY
>> +#include <linux/of.h>
>> +extern const int of_get_usbphy_mode(struct device_node *np);
>> +#endif
>> +
>> +#endif /* __LINUX_OF_USBPHY_H */
>> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
>> index a29ae1e..150eb69 100644
>> --- a/include/linux/usb/phy.h
>> +++ b/include/linux/usb/phy.h
>> @@ -12,6 +12,14 @@
>>  #include <linux/notifier.h>
>>  #include <linux/usb.h>
>>  
>> +enum usb_phy_interface {
>> +	USBPHY_INTERFACE_MODE_NA,
>> +	USBPHY_INTERFACE_MODE_UTMI,
>> +	USBPHY_INTERFACE_MODE_UTMIW,
>> +	USBPHY_INTERFACE_MODE_ULPI,
>> +	USBPHY_INTERFACE_MODE_SERIAL,
>> +};
> 
> no documentation at all here... What does UTMIW mean ? Why do you mean
> the NA mode ?
> 

Good point, W means wide.

Marc

Felipe Balbi Nov. 16, 2012, 1:41 p.m. UTC | #5

Hi,

On Fri, Nov 16, 2012 at 12:44:19PM +0100, Marc Kleine-Budde wrote:
> >> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
> >> new file mode 100644
> >> index 0000000..2e71f7b
> >> --- /dev/null
> >> +++ b/drivers/of/of_usbphy.c
> >> @@ -0,0 +1,49 @@
> >> +/*
> >> + * OF helpers for network devices.
> >> + *
> >> + * This file is released under the GPLv2
> >> + *
> >> + * Initially copied out of drivers/of/of_net.c
> >> + */
> >> +#include <linux/etherdevice.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/of_usbphy.h>
> >> +#include <linux/usb/phy.h>
> >> +#include <linux/export.h>
> >> +
> >> +/**
> >> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
> >> + * into the device tree binding of 'phy-mode', so that USB
> >> + * device driver can get phy interface from device tree.
> >> + */
> > 
> > provide proper kernel-doc
> > 
> >> +static const char *usbphy_modes[] = {
> >> +	[USBPHY_INTERFACE_MODE_NA]	= "",
> >> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
> >> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
> >> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
> >> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
> >> +};
> > 
> > In fact, these would be better off as constants:
> > 
> > #define USBPHY_INTERFACE_MODE_UTMI	1
> > #define USBPHY_INTERFACE_MODE_UTMIW	2
> > ...
> 
> Why are defines better than an enum? BTW: this code is a copy of the

because with enums can change value if you add another one in the middle
and it's really easy to miss that sort of thing during review and cause
regressions to many DTS files.

> ethernet phy of-helper code.

so ?

> >> +/**
> >> + * of_get_phy_mode - Get phy mode for given device_node
> >> + * @np:	Pointer to the given device_node
> >> + *
> >> + * The function gets phy interface string from property 'phy-mode',
> >> + * and return its index in phy_modes table, or errno in error case.
> >> + */
> > 
> > why do you pass a string instead of passing an Integer ? This makes no
> > sense to me.
> 
> This code returns an integer or rather an enum....see header file below.

I mean through DT, should've been more explicit. The dts files should
pass an integer, not a string. Then you don't need this silly conversion
helper.

Marc Kleine-Budde Nov. 16, 2012, 2:32 p.m. UTC | #6

On 11/16/2012 02:41 PM, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Nov 16, 2012 at 12:44:19PM +0100, Marc Kleine-Budde wrote:
>>>> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
>>>> new file mode 100644
>>>> index 0000000..2e71f7b
>>>> --- /dev/null
>>>> +++ b/drivers/of/of_usbphy.c
>>>> @@ -0,0 +1,49 @@
>>>> +/*
>>>> + * OF helpers for network devices.
>>>> + *
>>>> + * This file is released under the GPLv2
>>>> + *
>>>> + * Initially copied out of drivers/of/of_net.c
>>>> + */
>>>> +#include <linux/etherdevice.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/of_usbphy.h>
>>>> +#include <linux/usb/phy.h>
>>>> +#include <linux/export.h>
>>>> +
>>>> +/**
>>>> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
>>>> + * into the device tree binding of 'phy-mode', so that USB
>>>> + * device driver can get phy interface from device tree.
>>>> + */
>>>
>>> provide proper kernel-doc
>>>
>>>> +static const char *usbphy_modes[] = {
>>>> +	[USBPHY_INTERFACE_MODE_NA]	= "",
>>>> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
>>>> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
>>>> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
>>>> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
>>>> +};
>>>
>>> In fact, these would be better off as constants:
>>>
>>> #define USBPHY_INTERFACE_MODE_UTMI	1
>>> #define USBPHY_INTERFACE_MODE_UTMIW	2
>>> ...
>>
>> Why are defines better than an enum? BTW: this code is a copy of the
> 
> because with enums can change value if you add another one in the middle
> and it's really easy to miss that sort of thing during review and cause
> regressions to many DTS files.

Now I get it, you want to use integers instead of strings in the DT. Why
do you want to have Integers in the DT? It seems to work with strings
pretty well for the Ethernet phys, why invent a new concept here?

>> ethernet phy of-helper code.
> 
> so ?

reviewed core, proven to work, if the concept works for Ethernet phys it
should work with USB phys, too.

> 
>>>> +/**
>>>> + * of_get_phy_mode - Get phy mode for given device_node
>>>> + * @np:	Pointer to the given device_node
>>>> + *
>>>> + * The function gets phy interface string from property 'phy-mode',
>>>> + * and return its index in phy_modes table, or errno in error case.
>>>> + */
>>>
>>> why do you pass a string instead of passing an Integer ? This makes no
>>> sense to me.
>>
>> This code returns an integer or rather an enum....see header file below.
> 
> I mean through DT, should've been more explicit. The dts files should
> pass an integer, not a string. Then you don't need this silly conversion
> helper.

Marc

Peter Chen Nov. 26, 2012, 9:56 a.m. UTC | #7

On Wed, Nov 14, 2012 at 05:19:06PM +0100, Michael Grzeschik wrote:
> This patch makes it possible to set the connection of the usbphy to the
> soc. It is derived from the oftree bindings for the ethernetphy and adds
> similar helperfunctions.
> 
> Signed-off-by: Michael Grzeschik <[e-mail]>
> Signed-off-by: Marc Kleine-Budde <[e-mail]>
> ---
>  drivers/of/Kconfig        |    4 ++++
>  drivers/of/Makefile       |    1 +
>  drivers/of/of_usbphy.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_usbphy.h |   15 ++++++++++++++
>  include/linux/usb/phy.h   |    8 ++++++++
>  5 files changed, 77 insertions(+)
>  create mode 100644 drivers/of/of_usbphy.c
>  create mode 100644 include/linux/of_usbphy.h
> 
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..28f99fb 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -67,6 +67,10 @@ config OF_MDIO
>  	help
>  	  OpenFirmware MDIO bus (Ethernet PHY) accessors
>  
> +config OF_USBPHY
> +	depends on USB
> +	def_bool y
> +
>  config OF_PCI
>  	def_tristate PCI
>  	depends on PCI
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..fdcaf51 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_OF_IRQ)    += irq.o
>  obj-$(CONFIG_OF_DEVICE) += device.o platform.o
>  obj-$(CONFIG_OF_I2C)	+= of_i2c.o
>  obj-$(CONFIG_OF_NET)	+= of_net.o
> +obj-$(CONFIG_OF_USBPHY)	+= of_usbphy.o
>  obj-$(CONFIG_OF_SELFTEST) += selftest.o
>  obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
>  obj-$(CONFIG_OF_PCI)	+= of_pci.o
> diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
> new file mode 100644
> index 0000000..2e71f7b
> --- /dev/null
> +++ b/drivers/of/of_usbphy.c
> @@ -0,0 +1,49 @@
> +/*
> + * OF helpers for network devices.
> + *
> + * This file is released under the GPLv2
> + *
> + * Initially copied out of drivers/of/of_net.c
> + */
> +#include <linux/etherdevice.h>
> +#include <linux/kernel.h>
> +#include <linux/of_usbphy.h>
> +#include <linux/usb/phy.h>
> +#include <linux/export.h>
> +
> +/**
> + * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
> + * into the device tree binding of 'phy-mode', so that USB
> + * device driver can get phy interface from device tree.
> + */
> +static const char *usbphy_modes[] = {
> +	[USBPHY_INTERFACE_MODE_NA]	= "",
> +	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
> +	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
> +	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
> +	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
"fsls"? or should be "serial"?
> +};
> +

Best Regards,
Peter Chen

Patch

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..28f99fb 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -67,6 +67,10 @@  config OF_MDIO
 	help
 	  OpenFirmware MDIO bus (Ethernet PHY) accessors
 
+config OF_USBPHY
+	depends on USB
+	def_bool y
+
 config OF_PCI
 	def_tristate PCI
 	depends on PCI
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..fdcaf51 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -6,6 +6,7 @@  obj-$(CONFIG_OF_IRQ)    += irq.o
 obj-$(CONFIG_OF_DEVICE) += device.o platform.o
 obj-$(CONFIG_OF_I2C)	+= of_i2c.o
 obj-$(CONFIG_OF_NET)	+= of_net.o
+obj-$(CONFIG_OF_USBPHY)	+= of_usbphy.o
 obj-$(CONFIG_OF_SELFTEST) += selftest.o
 obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
diff --git a/drivers/of/of_usbphy.c b/drivers/of/of_usbphy.c
new file mode 100644
index 0000000..2e71f7b
--- /dev/null
+++ b/drivers/of/of_usbphy.c
@@ -0,0 +1,49 @@ 
+/*
+ * OF helpers for network devices.
+ *
+ * This file is released under the GPLv2
+ *
+ * Initially copied out of drivers/of/of_net.c
+ */
+#include <linux/etherdevice.h>
+#include <linux/kernel.h>
+#include <linux/of_usbphy.h>
+#include <linux/usb/phy.h>
+#include <linux/export.h>
+
+/**
+ * It maps 'enum usb_phy_interface' found in include/linux/usb/phy.h
+ * into the device tree binding of 'phy-mode', so that USB
+ * device driver can get phy interface from device tree.
+ */
+static const char *usbphy_modes[] = {
+	[USBPHY_INTERFACE_MODE_NA]	= "",
+	[USBPHY_INTERFACE_MODE_UTMI]	= "utmi",
+	[USBPHY_INTERFACE_MODE_UTMIW]	= "utmiw",
+	[USBPHY_INTERFACE_MODE_ULPI]	= "ulpi",
+	[USBPHY_INTERFACE_MODE_SERIAL]	= "fsls",
+};
+
+/**
+ * of_get_phy_mode - Get phy mode for given device_node
+ * @np:	Pointer to the given device_node
+ *
+ * The function gets phy interface string from property 'phy-mode',
+ * and return its index in phy_modes table, or errno in error case.
+ */
+const int of_get_usbphy_mode(struct device_node *np)
+{
+	const char *pm;
+	int err, i;
+
+	err = of_property_read_string(np, "phy-mode", &pm);
+	if (err < 0)
+		return err;
+
+	for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
+		if (!strcasecmp(pm, usbphy_modes[i]))
+			return i;
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(of_get_usbphy_mode);
diff --git a/include/linux/of_usbphy.h b/include/linux/of_usbphy.h
new file mode 100644
index 0000000..9a4132d
--- /dev/null
+++ b/include/linux/of_usbphy.h
@@ -0,0 +1,15 @@ 
+/*
+ * OF helpers for usb devices.
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_USBPHY_H
+#define __LINUX_OF_USBPHY_H
+
+#ifdef CONFIG_OF_USBPHY
+#include <linux/of.h>
+extern const int of_get_usbphy_mode(struct device_node *np);
+#endif
+
+#endif /* __LINUX_OF_USBPHY_H */
diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
index a29ae1e..150eb69 100644
--- a/include/linux/usb/phy.h
+++ b/include/linux/usb/phy.h
@@ -12,6 +12,14 @@ 
 #include <linux/notifier.h>
 #include <linux/usb.h>
 
+enum usb_phy_interface {
+	USBPHY_INTERFACE_MODE_NA,
+	USBPHY_INTERFACE_MODE_UTMI,
+	USBPHY_INTERFACE_MODE_UTMIW,
+	USBPHY_INTERFACE_MODE_ULPI,
+	USBPHY_INTERFACE_MODE_SERIAL,
+};
+
 enum usb_phy_events {
 	USB_EVENT_NONE,         /* no events or cable disconnected */
 	USB_EVENT_VBUS,         /* vbus valid event */