Re: [PATCH 2/3 v6] arm: omap: am335x: enable phy controls

[Felipe Balbi <balbi@xxxxxx>]

On Tue, May 01, 2012 at 12:38:00AM +0000, Gupta, Ajay Kumar wrote:
> Hi,
> 
> > -----Original Message-----
> > From: Balbi, Felipe
> > Sent: Monday, April 30, 2012 2:03 PM
> > To: Gupta, Ajay Kumar
> > Cc: linux-usb@xxxxxxxxxxxxxxx; Balbi, Felipe; tony@xxxxxxxxxxx
> > Subject: Re: [PATCH 2/3 v6] arm: omap: am335x: enable phy controls
> > 
> > Hi,
> > 
> > On Mon, Mar 12, 2012 at 07:30:23PM +0530, Ajay Kumar Gupta wrote:
> > > Switch on the phy for am335x.
> > >
> > > Signed-off-by: Ajay Kumar Gupta <ajay.gupta@xxxxxx>
> > > ---
> > > Updated the uses of cpu_is_xxx based on Tony's comment.
> > >
> > >  arch/arm/mach-omap2/omap_phy_internal.c |   34
> > ++++++++++++++++++++++--------
> > >  1 files changed, 25 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-omap2/omap_phy_internal.c
> > > b/arch/arm/mach-omap2/omap_phy_internal.c
> > > index 4c90477..7d7b3a2 100644
> > > --- a/arch/arm/mach-omap2/omap_phy_internal.c
> > > +++ b/arch/arm/mach-omap2/omap_phy_internal.c
> > > @@ -265,8 +265,21 @@ void ti81xx_musb_phy_power(u8 on)  {
> > >  	void __iomem *scm_base = NULL;
> > >  	u32 usbphycfg;
> > > +	static u8 ti816x, ti814x, ti81xx, am33xx, once;
> > > +
> > > +	if (!once) {
> > > +		ti816x = cpu_is_ti816x();
> > > +		ti814x = cpu_is_ti814x();
> > > +		ti81xx = cpu_is_ti81xx();
> > > +		am33xx = cpu_is_am33xx();
> > > +		once = 1;
> > > +	}
> > 
> > wow!!! This is so wrong... This whole omap_phy_internal needs to become
> > a driver. I'm not taking this patch, sorry. Only the next one, if it
> > compiles properly.
> 
> I understand you have been asking for a driver for omap_phy_internal but
> the same time this patch and above change was based on feedback from you
> and Tony.
> 
> If you remember, Tony wanted cpu_is_xxx() check to be done once only in
> the function and so the change.

and your answer is this ? Sorry, but I cannot accept it. Most likely you
have a revision register around to test against it and, even if you
decide not to use the revision resgister (why would you even decided
that ?) you don't need 5 u8 variables for this. You could have something
like:

u8 features = 0;

if (cpu_is_ti816x())
	features |= OMAP_PHY_HAS_PWRDN_BITS;

then, when you need to check for your usbphycfg register, you could:

if (features & OMAP_PHY_HAS_PWRDN_BITS) {
	usbphycfg &= ~(USBPHY_CM_PWRDN | USBPHY_OTG_PWRDN
			| USBPHY_DPINPUT | USBPHY_DMINPUT);
	usbphycfg |= (USBPHY_OTGVDET_EN | USBPHY_OTGSESSEND_EN
			| USBPHY_DPOPBUFCTL | USBPHY_DMOPBUFCTL);
} else {
	usbphycfg |= TI816X_USBPHY0_NORMAL_MODE;
	usbphycfg &= ~TI816X_USBPHY_REFCLK_OSC;
}

Ideally, though, this would be done by matching against a revision
register and completely drop the CPU tests.

-- 
balbi

Attachment: signature.asc
Description: Digital signature