linux-kernel - RE: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <063D6719AE5E284EB5DD2968C1650D6D548EB59F@AcuExch.aculab.com>
Date:	Thu, 24 Mar 2016 14:01:10 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'David Lechner' <david@...hnology.com>,
	Sekhar Nori <nsekhar@...com>
CC:	Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
	"Mark Rutland" <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Russell King <linux@....linux.org.uk>,
	Kevin Hilman <khilman@...nel.org>,
	Kishon Vijay Abraham I <kishon@...com>,
	Alan Stern <stern@...land.harvard.edu>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Bin Liu <b-liu@...com>,
	Andreas Färber <afaerber@...e.de>,
	Tony Lindgren <tony@...mide.com>,
	Robert Jarzmik <robert.jarzmik@...e.fr>,
	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: RE: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB
 PHY

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