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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
