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
