2011/11/27 Simon Glass <s...@chromium.org>
> > +static int ulpi_wait(u32 ulpi_viewport, u32 ulpi_value, u32 ulpi_mask)
>
>
> How about a comment as to what this function does, params and return
> values?
>
> Also perhaps if you pass the operation to this function it can print
> the error message (perhaps with debug()) rather than ever calling
> having to do it? Or at least decide whether messages should be in
> bottom-level functions or in top-level - and perhaps considering using
> debug() instead of printf() at lower levels.
>
> > +{
> > + int timeout = CONFIG_USB_ULPI_TIMEOUT;
> > + u32 tmp;
> > +
> > + writel(ulpi_value, ulpi_viewport);
>
> Why do the write here? Should it be done in the write call below? At
> the very least you should rename this function.
>
>
This function issues ULPI wakeup or read-write request, writes the request
into ULPI viewport register and waits until ULPI interface signalizes
completion
of this request.
With error messages is the situation a bit difficult, because this function
only knows
whether the request was for wakeup or for read-write.
I think more specific error messages are more helpful, so I decided to
put them into
functions which knows what we've attempted to
do(ulpi_wakeup()/ulpi_read()/ulpi-write()).
If you really think that debug() is more appropriate in these functions, I
can change this,
but Igor doesn't consider this a reason for this patch not to be accepted,
and I agree with him.
And I'll rename this function into ulpi_request(), ulpi_wait() is really a
bit deceiveing.
Thanks for your comments.
Regards,
Jana Rapava
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
