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

Submitted by Lucas Stach on Oct. 30, 2012, 9:22 a.m.

Details

Message ID 1351588973-20699-3-git-send-email-dev@lynxeye.de
State Superseded
Delegated to: Marek Vasut
Headers show

Commit Message

Lucas Stach Oct. 30, 2012, 9:22 a.m.
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(-)

Comments

Simon Glass Oct. 30, 2012, 1:23 p.m.
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
Lucas Stach Oct. 30, 2012, 1:32 p.m.
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

Patch hide | download patch | download mbox

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]);
 
 	/* count may return <0 on error */
 	count = fdtdec_find_aliases_for_id(blob, "usb",