Re: [rft/rfc/patch-v2.6.29-rc5+ 16/23] usb: host: ehci: add platform_data

[Felipe Balbi <me@felipebalbi.com>]

On Wed, Feb 25, 2009 at 12:35:05AM +0200, Grazvydas Ignotas wrote:
> On Mon, Feb 23, 2009 at 8:55 PM, Felipe Balbi <me@felipebalbi.com> wrote:
> > Adding a platform_data to the driver allow us
> > to remove some of the ifdeferry in the code.
> >
> 
> <snip>
> 
> > diff --git a/arch/arm/mach-omap2/board-omap3pandora.c b/arch/arm/mach-omap2/board-omap3pandora.c
> > index b8a78c0..08215c0 100644
> > --- a/arch/arm/mach-omap2/board-omap3pandora.c
> > +++ b/arch/arm/mach-omap2/board-omap3pandora.c
> > @@ -297,7 +297,7 @@ static void __init omap3pandora_init(void)
> >        spi_register_board_info(omap3pandora_spi_board_info,
> >                        ARRAY_SIZE(omap3pandora_spi_board_info));
> >        usb_musb_init();
> > -       usb_ehci_init();
> > +       usb_ehci_init(EHCI_HCD_OMAP_MODE_PHY, true, true, 57, 61);
> 
> Perhaps it would be better to have .chargepump and .phy_reset set to
> false by default, as pandora doesn't need that chargepump thing (I
> guess some other boards too), and phy reset GPIO is wrong not only for
> pandora I think. Board maintainers can then send patches as needed.
> There also should be a way to specify only one reset GPIO, most boards
> have only one EHCI port I think. Note that .phy_reset is required for
> EHCI to work on pandora (in case you have thoughts of removing that
> code later).

I made it true by default because those where always true in PHY mode as
of current driver. The correct way to do it, would be to move both to
board-file since those workarounds are board-specific.

We need a list of the boards that need the workarounds and a list of
those which don't need so we can move the workarounds to the
board-files.

> 
> > diff --git a/arch/arm/mach-omap2/usb-ehci.c b/arch/arm/mach-omap2/usb-ehci.c
> > index 23fe857..30e1ad6 100644
> > --- a/arch/arm/mach-omap2/usb-ehci.c
> > +++ b/arch/arm/mach-omap2/usb-ehci.c
> > @@ -145,8 +145,20 @@ static void setup_ehci_io_mux(void)
> >        return;
> >  }
> >
> > -void __init usb_ehci_init(void)
> > +void __init usb_ehci_init(enum ehci_hcd_omap_mode phy_mode,
> > +               int chargepump, int phy_reset, int reset_gpio_port1,
> > +               int reset_gpio_port2)
> >  {
> > +       struct ehci_hcd_omap_platform_data pdata = {
> > +               .phy_mode               = phy_mode,
> > +               .chargepump             = chargepump,
> > +               .phy_reset              = phy_reset,
> > +               .reset_gpio_port1       = reset_gpio_port1,
> > +               .reset_gpio_port2       = reset_gpio_port2,
> > +       };
> > +
> > +       ehci_device.dev.platform_data = &pdata;
> > +
> 
> hmm, doesn't pdata end up on stack here? platform_device_register()
> doesn't seem to make a copy of that structure.

hmm... should make that __initconst and use
platform_device_add_data();

Will fix, thanks a lot

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html