Hi,
On Tue, May 22, 2012 at 12:36 AM, Felipe Balbi <balbi@xxxxxx> wrote:
> 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 ?
Thats right. All these PHY specific settings should be done in phy
driver and for SCM specific settings, Eduardo has a version of it.
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
