From: David Lechner > Sent: 23 March 2016 18:07 > On 03/23/2016 12:21 PM, Sekhar Nori wrote: > >> +/* DA8xx CFGCHIP2 (USB PHY Control) register bits */ > >> +#define PHYCLKGD (1 << 17) > >> +#define VBUSSENSE (1 << 16) > >> +#define RESET (1 << 15) > >> +#define OTGMODE_MASK (3 << 13) > >> +#define NO_OVERRIDE (0 << 13) > >> +#define FORCE_HOST (1 << 13) > >> +#define FORCE_DEVICE (2 << 13) > >> +#define FORCE_HOST_VBUS_LOW (3 << 13) I think I'd define the above as: #define OTG_MODE(x) ((x) << 13) #define OTGMODE_MASK OTG_MODE(3) #define NO_OVERRIDE OTG_MODE(0) #define FORCE_HOST OTG_MODE(1) #define FORCE_DEVICE OTG_MODE(2) #define FORCE_HOST_VBUS_LOW OTG_MODE(3) Then realise that all the names need changing (to include OTG). > >> +#define USB1PHYCLKMUX (1 << 12) > >> +#define USB2PHYCLKMUX (1 << 11) > >> +#define PHYPWRDN (1 << 10) > >> +#define OTGPWRDN (1 << 9) > >> +#define DATPOL (1 << 8) > >> +#define USB1SUSPENDM (1 << 7) > >> +#define PHY_PLLON (1 << 6) > >> +#define SESENDEN (1 << 5) > >> +#define VBDTCTEN (1 << 4) > >> +#define REFFREQ_MASK (0xf << 0) > >> +#define REFFREQ_12MHZ (1 << 0) > >> +#define REFFREQ_24MHZ (2 << 0) > >> +#define REFFREQ_48MHZ (3 << 0) > >> +#define REFFREQ_19_2MHZ (4 << 0) > >> +#define REFFREQ_38_4MHZ (5 << 0) > >> +#define REFFREQ_13MHZ (6 << 0) > >> +#define REFFREQ_26MHZ (7 << 0) > >> +#define REFFREQ_20MHZ (8 << 0) > >> +#define REFFREQ_40MHZ (9 << 0) I'd define these similarly to the OTGxxx values. > > Many of these register bits are unused. I guess opinion varies around > > this, but I get confused with unnecessary bit definitions and register > > offsets. I tend to search for it and its sort of disappointing to see > > that its basically unused. Of course, you should wait for PHY > > maintainers preference. It can be useful to see what isn't being set. Sometimes this can be very useful when making changes to a driver. For status values it is particularly important to know what all the bits mean. > My thinking was that since this driver *owns* the CFGCHIP2 register that > is would eventually register clocks using the common clock framework, in > which case, it would use many of these registers. > > But, based on feedback on another commit, if we go the syscon route, > then yes, I will remove the unused values. > > > >> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode); > > > > Hmm, since this driver exports this symbol, I think it should also > > provide an include file in include/linux/phy for users of the symbol. Or > > perhaps there should be a generic API around this since it looks like > > most USB phys will need something similar? > > > > The reason I didn't make a header file is that this function is only > meant to be use in one place and would likely cause a crash if used > anywhere else. And a compilation error if compiled with -Wmissing-prototypes. Sounds like you need a header file just for this driver's 'private' exports. IMHO .c files shouldn't contain 'extern' anywhere. I removed a load from some local code and found quite a few lurking bugs where the types didn't match. David -- 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
