View Change

4 comments:

• File it85spi.c:

  • Patch Set #5, Line 76: it85spi_data

    nit: _data is a bit redundant and verbose; It might be cleaner and more direct to just use: `struct […]

    This is consistent with the rest of the tree, please keep things consistent and we can argue about the merits of better identifier conventions after everything has converged to some consistency first.

  • Patch Set #5, Line 77:

            unsigned int shm_io_base;
            unsigned char *ce_high, *ce_low;
            int it85xx_scratch_rom_reenter;
    
    

    nit: probably want tab indents here in struct definition and for all the code below. […]

    thanks for picking this up.

  • 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;
    }
    
    

    Perhaps just return NULL on error: […]

    While true that this could be inlined it would be inconsistent with other drivers. Further, the helper is useful for writing unit-tests as the setup of flashromctx can be separated pre-spi-driver runs. This allows for driver state injection during unit-testing. The internal states can also be inspected using these helpers to constrain the test by way of type-signature and hence then the common return type of int was preserved. The consistent pattern here is one state tracker type and one getter from the `void *data` field.

    The over arching vision is to first bring the tree into a consistent state where all drivers are fully re-enterent and have common type-siguratures that bond logic problems into smaller state spaces. Finally, to be consistent across the entire tree so that more smaller optimisations can be made atomically at once rather than a fragmented mess of different ways that troubles us now.

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

    If spi_master_it85xx:it85spi_data is 1:1, and spi_master_it85xx is a static global, then there doesn […]

    We explicitly do not want static globals of any kind though, that would be counter to the entire patch. Further to that, this is inconsistent with the rest of the trees direction.

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