View Change

3 comments:

• File it85spi.c:

  • Patch Set #5, Line 76: it85spi_data

    This is consistent with the rest of the tree, please keep things consistent and we can argue about t […]

    sgtm. Consistency is best.

  • Patch Set #5, Line 227:

    static int get_data_from_context(const struct flashctx *flash, struct it85spi_data **data)
    {
            if (!flash || !flash->mst || !flash->mst->spi.data) {
                    msg_perr("Unable to extract data from flash context.\n");
                    return SPI_GENERIC_ERROR;
            }
            *data = (struct it85spi_data *) flash->mst->spi.data;
    
            return 0;
    }
    
    

    While true that this could be inlined it would be inconsistent with other drivers. […]

    Consistency is good. Better would be to implement the unit tests in this CL that show the value in adding this extra complexity. Otherwise I still recommend the simpler and direct implementation for now, and then adding this complexity when there is value in doing so.

  • Patch Set #5, Line 321: spi_master_it85xx.data = data;

    We explicitly do not want static globals of any kind though, that would be counter to the entire pat […]

    Removing "static globals of any kind" is not a goal in and of itself. They are quite useful and efficient. There has to be some motivation to do so, such as when runtime allocation provides some value.

    The value of this patch is that it collects scattered device specific file-global static state variables into a single context structure, and then makes the functions pure by having them operate on a passed-in context struct rather than directly accessing global state.

    I don't see any additional value, at this point, of allocating the context struct on the heap rather than as a static global. AFAICT, it just introduces extra runtime complexity.

    Perhaps the value is realized by some other patches that perform additional refactoring and/or add unit tests?

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