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

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |