[U-Boot] [PATCH 2/8] tegra: usb: make controller init functions more self contained

[Simon Glass]

Hi Lucas,

On Tue, Oct 30, 2012 at 6:16 AM, Lucas Stach <dev at lynxeye.de> wrote:
> Hello Simon,
>
> Am Dienstag, den 30.10.2012, 06:03 -0700 schrieb Simon Glass:
>> Hi Lucas,
>>
>> On Tue, Oct 30, 2012 at 2:22 AM, Lucas Stach <dev at lynxeye.de> wrote:
>> > There is no need to pass around all those parameters. The init functions
>> > are able to easily extract all the needed setup info on their own.
>> >
>> > Signed-off-by: Lucas Stach <dev at lynxeye.de>
>> > ---
>> >  arch/arm/cpu/armv7/tegra20/usb.c | 24 ++++++++++++------------
>> >  1 Datei geändert, 12 Zeilen hinzugefügt(+), 12 Zeilen entfernt(-)
>>
>> I'm not sure I agree with the premise of this patch.
>>
>> At the top level it calls clock_get_osc_freq() to get the frequency.
>> That is then passed to the two places that need it.
>>
>> It doesn't seem right to me to call clock_get_osc_freq() again in the
>> lower level function just to avoid a parameter. On ARM at least a few
>> parameters are a cheap way of passing data around.
>>
> The intent of this patch is not really to save up on parameters passed,
> but to make it possible to later move out the controller initialization
> into the ehci_hcd_init function without having to save away this global
> state for later use.
>
> We have to init at most 2 controllers where timing matters, so I think
> it's the right thing to get the SoC global clock state at those two
> occasions to avoid inflating the file global state.

OK fair enough, I see that you want to do these two types of init at
different times.

>
>> It also allows the lower-level functions to deal with what they need
>> to, rather than all functions having to reference the global state
>> independently, each one digging down to what it actually needs.
>>
> The controller init functions get passed the state only of the one port
> they have to initialize. There is no point in extracting things at an
> upper level and passing it into the functions, if it's exactly the same
> thing that is stored in the port state.

Well if the upper level is extracting it anyway, it often saves code
space to pass the extracted value. I would like to avoid this sort of
thing:

void do_something(struct level1 *level1)
{
   struct level2 *level2 = level1->level2;
   struct level3 *level3 = level2->level3;

   level3->...
   level3->...
}

void do_something_else(struct level1 *level1)
{
   struct level2 *level2 = level1->level2;
   struct level3 *level3 = level2->level3;

   level3->...
   level3->...
}

main()
{
   do_something(level1)
   do_something_else(level1)
}


I generally prefer:

void do_something(struct level3 *level3)
{
   level3->...
   level3->...
}

void do_something_else(struct level3 *level3)
{
   level3->...
   level3->...
}

main()
{
   struct level2 *level2 = level1->level2;
   struct level3 *level3 = level2->level3;

   do_something(level3)
   do_something_else(level3)
}


I hope that example makes sense.

>
> Regards,
> Lucas
>
>

Regards,
Simon