[U-Boot,3/8] tegra: usb: fold initial pll setup into board_usb_init

The setup is trivial, no need to split this out into a separate function.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 arch/arm/cpu/armv7/tegra20/usb.c | 15 +++++----------
 1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)

Hi Lucas,

On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> The setup is trivial, no need to split this out into a separate function.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
>  arch/arm/cpu/armv7/tegra20/usb.c | 15 +++++----------
>  1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)
>
> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
> index 1725cd1..e61bd69 100644
> --- a/arch/arm/cpu/armv7/tegra20/usb.c
> +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> @@ -417,13 +417,6 @@ static int init_ulpi_usb_controller(struct fdt_usb *config)
>  }
>  #endif
>
> -static void config_clock(const u32 timing[])
> -{
> -       clock_start_pll(CLOCK_ID_USB,
> -               timing[PARAM_DIVM], timing[PARAM_DIVN], timing[PARAM_DIVP],
> -               timing[PARAM_CPCON], timing[PARAM_LFCON]);
> -}
> -
>  /**
>   * Add a new USB port to the list of available ports.
>   *
> @@ -534,13 +527,15 @@ int board_usb_init(const void *blob)
>  {
>         struct fdt_usb config;
>         unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
> -       enum clock_osc_freq freq;
>         int node_list[USB_PORTS_MAX];
>         int node, count, i;
> +       u32 *timing;
>
>         /* Set up the USB clocks correctly based on our oscillator frequency */
> -       freq = clock_get_osc_freq();
> -       config_clock(usb_pll[freq]);
> +       timing = usb_pll[clock_get_osc_freq()];
> +       clock_start_pll(CLOCK_ID_USB,
> +               timing[PARAM_DIVM], timing[PARAM_DIVN], timing[PARAM_DIVP],
> +               timing[PARAM_CPCON], timing[PARAM_LFCON]);

Sorry I don't see the benefit of this change. The function is there to
handle a clearly-defined task, hiding the detail of clock config
elsewhere. It has no effect on code generated.

>
>         /* count may return <0 on error */
>         count = fdtdec_find_aliases_for_id(blob, "usb",
> --
> 1.7.11.7
>

Regards,
Simon

Am Dienstag, den 30.10.2012, 06:23 -0700 schrieb Simon Glass:
> Hi Lucas,
> 
> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev@lynxeye.de> wrote:
> > The setup is trivial, no need to split this out into a separate function.
> >
> > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> > ---
> >  arch/arm/cpu/armv7/tegra20/usb.c | 15 +++++----------
> >  1 Datei geändert, 5 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)
> >
> > diff --git a/arch/arm/cpu/armv7/tegra20/usb.c b/arch/arm/cpu/armv7/tegra20/usb.c
> > index 1725cd1..e61bd69 100644
> > --- a/arch/arm/cpu/armv7/tegra20/usb.c
> > +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> > @@ -417,13 +417,6 @@ static int init_ulpi_usb_controller(struct fdt_usb *config)
> >  }
> >  #endif
> >
> > -static void config_clock(const u32 timing[])
> > -{
> > -       clock_start_pll(CLOCK_ID_USB,
> > -               timing[PARAM_DIVM], timing[PARAM_DIVN], timing[PARAM_DIVP],
> > -               timing[PARAM_CPCON], timing[PARAM_LFCON]);
> > -}
> > -
> >  /**
> >   * Add a new USB port to the list of available ports.
> >   *
> > @@ -534,13 +527,15 @@ int board_usb_init(const void *blob)
> >  {
> >         struct fdt_usb config;
> >         unsigned osc_freq = clock_get_rate(CLOCK_ID_OSC);
> > -       enum clock_osc_freq freq;
> >         int node_list[USB_PORTS_MAX];
> >         int node, count, i;
> > +       u32 *timing;
> >
> >         /* Set up the USB clocks correctly based on our oscillator frequency */
> > -       freq = clock_get_osc_freq();
> > -       config_clock(usb_pll[freq]);
> > +       timing = usb_pll[clock_get_osc_freq()];
> > +       clock_start_pll(CLOCK_ID_USB,
> > +               timing[PARAM_DIVM], timing[PARAM_DIVN], timing[PARAM_DIVP],
> > +               timing[PARAM_CPCON], timing[PARAM_LFCON]);
> 
> Sorry I don't see the benefit of this change. The function is there to
> handle a clearly-defined task, hiding the detail of clock config
> elsewhere. It has no effect on code generated.
> 
It's more of a personal thing, that every time there is a function call
it breaks the flow when reading the code. And IMHO it's not worth the
break if the called function does nothing other than just calling
another function.

If other people also dislike the change I may just drop it, but I would
like to hear some more opinions about this first.

Regards,
Lucas