From: David Lechner

Sent: 23 March 2016 18:07
On 03/23/2016 12:21 PM, Sekhar Nori wrote:

quoted

quoted

+/* 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).

quoted

quoted

+#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.

quoted

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.

quoted

quoted

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