Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/48196 )
Change subject: Simplifying the initialisation flow for it85spi ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c File it85spi.c:
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@303 PS1, Line 303: data = calloc(1, sizeof(struct it85spi_data)); : if (!data) { : msg_perr("Unable to allocate space for extra SPI master data.\n"); : return SPI_GENERIC_ERROR; : } : : spi_master_it85xx.data = data; : : if (register_shutdown(it85xx_shutdown, data)) { : goto it85spi_init_error;
in fact don't even bother checking the return value of register_shutdown() as it's useless to do so […]
I am keeping this check for return value of register_shutdown because as it is currently implemented seems like it can return 1 or 0. If this aspect needs to be changed/improved, I can do it later as a next step. Moving everything to the bottom of the function as you suggested.
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@339 PS1, Line 339: chipaddr
these casts are not needed
Done
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@341 PS1, Line 341: goto it85spi_init_error;
this should be a basic early return the code was correct before.
Done
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@346 PS1, Line 346: data->ce_high = (unsigned char *)(base + 0xE00); /* 0xFFFFFE00 */ : data->ce_low = (unsigned char *)(base + 0xD00); /* 0xFFFFFD00 */
this is common code on both sides of the pre-processor directive.
Yes I agree! Can I take care of this code behind the flags in the next patch?
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@349 PS1, Line 349: : msg_pdbg("%s: internal_buses_supported=0x%x\n", __func__, : internal_buses_supported);
this isn't needed.
There are lots of similar debug messages in this file. I am not sure what exactly they are for so I don't want to remove them. In any case, if we decide to remove them can I do it in the next patch?
https://review.coreboot.org/c/flashrom/+/48196/1/it85spi.c@362 PS1, Line 362: */
this comment block is all related the to the above code and should be immediately close to the regis […]
Not sure what do you mean here: do you mean this FIXME comment should be below /* Set this as SPI controller */ one? So just swap comments?