Patch set 6:Code-Review +1

View Change

5 comments:

• File src/mainboard/intel/elkhartlake_crb/board_info.txt:

  • Patch Set #4, Line 6: Flashrom support: y

    We have not tested it, but it should be supported as the SPI chip we used in CRB is in the supported […]

    Well, support for the SPI chip itself does not mean that flashrom is aware of the new PCI device ID of the SPI controller in EHL. I would guess that at least thi ID is missing?

• File src/mainboard/intel/elkhartlake_crb/mainboard.c:

  • Patch Set #4, Line 33: sku2147483647

    Yeah this is more JSL & chrome specific, plan to remove it later in EHL updates.

    OK, let's not forget about it in the follow-up patches.

• File src/mainboard/intel/elkhartlake_crb/variants/baseboard/include/baseboard/variants.h:

  • Patch Set #6, Line 11: 3 sets of functions

    I guess you can write here "the next 3 functions" as the "set" was intending a couple of functions and you now define the "couple" to be 3.

• File src/mainboard/intel/elkhartlake_crb/variants/ehlcrb/gpio.c:

  • Patch Set #6, Line 17: const struct pad_config *variant_gpio_table(size_t *num)

    Hi Singer, thanks for reviewing. Since this is a copy code of mainboard, I don't wanna do it here. […]

    Why should this be 'configure_gpios()'? This function just returns a pointer to a table where the GPIO settings are stored. The "configure" part will be done in the common code. This is the API we are using for this purpose.

  • Patch Set #6, Line 20: return gpio_table;

    Why not just using `gpio_configure_pads(pads, num)` here?
    Because this is the API we have for this purpose which is used across the tree. Why should we change it? In addition, this is a function which is variant specific which will then be called from the mainboard code common for all variants. So you have to distinguish between the variants somewhere.
    The same applies for the other places here.

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