On Tue, Jan 29, 2013 at 8:33 AM, Felipe Balbi <balbi@xxxxxx> wrote: > Hi, > > On Tue, Jan 29, 2013 at 07:40:23PM +0530, kishon wrote: >> On Tuesday 29 January 2013 07:23 PM, Wolfram Sang wrote: >> >>>+ err = of_property_read_string(np, "phy_type", &phy_type); >> >>>+ if (err < 0) >> >>>+ return USBPHY_INTERFACE_MODE_NA; >> >> >> >>Why don't we use a u32 property type for the *phy-type*? IMHO we >> >>should use string property only when the property should be >> >>absolutely unambiguous (e.g., compatible property should be string). >> > >> >If we would use u32-numbers in the compatible entry, this would also be >> >unambiguous, no? 0xd00dfeed would be the at24-driver. Pretty specific. >> >> hehe... But we don't have a corresponding *enum* representing the >> drivers :-) >> > >> >I don't mind having readable devicetrees. And we have it for ethernet >> >phys already with strings, so it would be consistent. >> >> Ok. Fine with it then :-) > > I prefer u32 here, because we have the matching enum. Otherwise we end > up with: > > of_property_read_string(...,&type); > > if (!strcmp(type, "ulpi")) > foo(); > else if (!strcmp(type, "utmi")) > bar(); > else if (!strcmp(type, "pipe3")) > baz(); > else > BUG(); > > and I don't like that, it's ugly and error prone. Also looks awful when > someone needs to change that, the diff looks messy. A u32 followed by a > switch statement looks nicer. I wholeheartedly and vehemently disagree. Device trees don't exist to make Linux look prettier, *OR* clean up your source code of string comparisons when you think comparing an integer looks or feels cleaner. They exist to provide a hardware description. phy-type 0x3 does not describe anything except that you have to go look up what 0x3 means, and means device trees cannot be internally consistent within themselves or publically existing documentation (it is certain that there is no USB PHY specification that defines 0x3 as anything, which means the value is entirely Linux specific). Matching an enum or magic number encoded into a u32 in some external source code to define a type that wasn't just an index is not something that anyone has ever reasonably done in any traditional IEEE1275 device tree or OpenFirmware-committee binding, and we shouldn't just be subverting the existing standard to make Linux code "look prettier". Please consider other operating systems, which would need to copy the definition of the enum which may not be possible when thinking of Linux vs. FreeBSD or so, and in general the readability of the device tree - phy-type 0x3 is going to require a comment in the device tree source to remind people what that means, or a cross-reference to a binding which is more work than most developers want to do to make sure they specified the correct PHY type. A string is completely and unavoidably correct - a typo in the phy-type means a match against valid bindings is impossible and an instant bug. A mistaken u32 value means the wrong phy-type is defined which has increased potential to provide misconfigured phy with the wrong type and less warning than a string literal not being absolutely identical. I think that means more buggy device trees will get past where it doesn't actually work properly, and more time will be spent working out WHY it doesn't actually work properly. It is much better to be totally unambiguous in the device tree as per the type and a string is the best way. If you really want effective, less error-prone code, define all the existing or useful types as preprocessor defines (#define PHY_TYPE_UTMI_WIDE "utmiw") and use those to match the binding. I wouldn't hand-code a property string inline even if you offered me a million dollars to do so. Matching the dr_mode property is already done in a drivers/of or of_phy.h include so just move the potential match code to there and return the correct enum (which is arguably Linux-specific) from the string and give a big fat error from the match function if none of the valid bindings matches up. BTW I disagree with the use of underscores in device trees as well, I wouldn't use an underscore for a new property. But in the case of dr_mode it is well used already especially for Freescale controllers on PPC where there are a significant handful of DTS (or real OF) that implement it. It might not be a bad idea, though, to update the binding deprecating dr_mode in favor of something that is much less ambiguous and descriptive itself - "dr-mode" is an acceptable fix but a "role" property or "dual-role-mode" is much more descriptive. Moving the matching to some common code since more than one controller uses it and then adding a new, better property name with dr_mode as an acceptable but unrecommended fallback means this can be left to history. -- Matt Sealey <matt@xxxxxxxxxxxxxx> Product Development Analyst, Genesi USA, Inc. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
