Attention is currently required from: Michał Żygowski, Angel Pons, Michael Niewöhner.

Patch set 6:Code-Review +1

View Change

4 comments:

• File acpi_ec.h:

  • Patch Set #6, Line 20: #if defined(__i386__) || defined(__x86_64__)

    I know it's still a common pattern, but this shouldn't be here. Such
    things are up to the build system and shouldn't clutter the code.
    Same for the `.c` file, it should simply not be compiled if incompatible.

  • Patch Set #6, Line 39: uint8_t address, uint8_t *data

    I guess it's ok because even a C compiler notices the different types
    (by now), but we usually keep the destination as the first argument (like
    in standard C functions, memcpy() etc.). That keeps the order like it
    would be in an assignment statement.

• File acpi_ec.c:

  • Patch Set #6, Line 43: EC_MAX_STATUS_CHECKS

    If this is the default, why is it also part of the API?

  • Patch Set #6, Line 48: }

    Would you mind exchanging the conditions? It seems odd not to check the
    index in the for condition. e.g.

        for (i = 0; i < max_checks; ++i) {
                if ((INB(EC_STS_CMD) & EC_STS_IBF) == 0)
                        return true;
        }
        
        return false;

To view, visit change 55714. To unsubscribe, or for help writing mail filters, visit settings.