From: David Laight <David.Laight@ACULAB.COM>
To: "'David Lechner'" <david@lechnology.com>, Sekhar Nori <nsekhar@ti.com>
CC: Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
        "Mark Rutland" <mark.rutland@arm.com>,
        Ian Campbell <ijc+devicetree@hellion.org.uk>,
        Kumar Gala <galak@codeaurora.org>,
        Russell King <linux@arm.linux.org.uk>,
        Kevin Hilman <khilman@kernel.org>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Alan Stern <stern@rowland.harvard.edu>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Bin Liu <b-liu@ti.com>,
        =?Windows-1252?Q?Andreas_F=E4rber?= <afaerber@suse.de>,
        Tony Lindgren <tony@atomide.com>,
        Robert Jarzmik <robert.jarzmik@free.fr>,
        Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
Subject: RE: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB
 PHY
Thread-Topic: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB
 PHY
Thread-Index: AQHRhS7H1FmGO22Cgk6DxxhQggRyeZ9onT5Q
Date: Thu, 24 Mar 2016 14:01:10 +0000
Message-ID: <063D6719AE5E284EB5DD2968C1650D6D548EB59F@AcuExch.aculab.com>
References: <1458181615-27782-1-git-send-email-david@lechnology.com>
 <1458181615-27782-7-git-send-email-david@lechnology.com>
 <56F2D0AD.4040005@ti.com> <56F2DB41.7010708@lechnology.com>
In-Reply-To: <56F2DB41.7010708@lechnology.com>
Accept-Language: en-GB, en-US
Content-Language: en-US
Content-Type: text/plain; charset="Windows-1252"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3132
Lines: 83

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