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

login
register
mail settings
Submitter Lucas Stach
Date Oct. 30, 2012, 9:22 a.m.
Message ID <1351588973-20699-3-git-send-email-dev@lynxeye.de>
Download mbox | patch
Permalink /patch/195339/
State Superseded
Delegated to: Marek Vasut
Headers show

Comments

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(-)
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

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",