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

[Felipe Balbi <balbi@xxxxxx>]

Hi,

On Mon, May 07, 2012 at 06:31:28AM +0000, Gupta, Ajay Kumar wrote:
> Hi
> > On Wed, May 02, 2012 at 10:03:10AM +0000, Gupta, Ajay Kumar wrote:
> > > Hi,
> > >
> > > [..]
> > > > > > > +	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
> > >
> > > [...]
> > >
> > > > Most likely you have a revision register around to test against it
> > and,
> > >
> > > Yes, correct, revision register can be used and so cpu_is_xxx check
> > can be dropped.
> > >
> > > > 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.
> > >
> > > Agree. But we need to first kill omap_phy_internal and add a separate
> > > driver for the same and then add phy control support for ti81xx/am33xx
> > so
> > > I think it will have to wait longer.
> > 
> > why ? You don't need a "driver" to be able to read a revision register.
> > You already read/write to many registers without this file being using
> > any of the driver model infrastructure. Still, it can wait a little
> > longer, no problems.
> 
> If this file is not following any driver model then why to allow to even
> grow it further. We should not have allowed the omap4 phy control at first
> place itself.

patches fixing legacy stuff are highly welcome, you have no idea ;-)
Still, I believe Kishon has a version of this converted to a proper PHY
driver. Kishon, do I understand you correctly ?

-- 
balbi

Attachment: signature.asc
Description: Digital signature