[coreboot-gerrit] Patch merged into coreboot/master: fe64060 exynos5250: De-switch-ify the pinmux configuration code.

gerrit at coreboot.org gerrit at coreboot.org
Wed Jul 10 21:49:59 CEST 2013


the following patch was just integrated into master:
commit fe6406033fe327d4ae408b02efc060b4b421bc03
Author: Gabe Black <gabeblack at google.com>
Date:   Sat Jun 15 20:33:05 2013 -0700

    exynos5250: De-switch-ify the pinmux configuration code.
    
    The pinmux code for the exynos5250 was all bundled into a single, large
    function which contained a switch statement that would set up the pins for
    different peripherals within the SOC. There was also a "flags" parameter, the
    meaning of which, if any, depended on which peripheral was being set up.
    
    There are several problems with that approach. First, the code is inefficient
    in both time and space. The caller knows which peripheral it wants to set up,
    but that information is encoded in a constant which has to be unpacked within
    the function before any action can be taken. If there were a function per
    peripheral, that information would be implicit. Also, the compiler and linker
    are forced to include the entire function with all its cases even if most of
    them are never called. If each peripheral was a function, the unused ones
    could be garbage collected.
    
    Second, it would be possible to try to set up a peripheral which that function
    doesn't know about, so there has to be additional error checking/handling. If
    each peripheral had a function, the fact that there was a function to call at
    all would imply that the call would be understood.
    
    Third, the flags parameter is fairly opaque, usually doesn't do anything, and
    sometimes has to have multiple values embedded in it. By having separate
    functions, you can have only the parameters you actually want, give them
    names that make sense, and pass in values directly.
    
    Fourth, having one giant function pretends to be a generic, portable API, but
    in reality, the only way it's useful is to call it with constants which are
    specific to a particular implementation of that API. It's highly unlikely that
    a bit of code will need to set up a peripheral but have no idea what that
    peripheral actually is.
    
    Call sights for the prior pinmux API have been updated. Also, pinmux
    initialization within the i2c driver was moved to be in the board setup code
    where it really probably belongs. The function block that implements the I2C
    controller may be shared between multiple SOCs (and in fact is), and those
    SOCs may have different pinmuxes (which they do).
    
    Other places this same sort of change can be made are the pinmux code for the
    5420, and the clock configuration code for both the 5250 and the 5420.
    
    Change-Id: Ie9133a895e0dd861cb06a6d5f995b8770b6dc8cf
    Signed-off-by: Gabe Black <gabeblack at chromium.org>
    Reviewed-on: http://review.coreboot.org/3673
    Tested-by: build bot (Jenkins)
    Reviewed-by: Stefan Reinauer <stefan.reinauer at coreboot.org>


See http://review.coreboot.org/3673 for details.

-gerrit



More information about the coreboot-gerrit mailing list