On Thu, 11 Jun 2015 23:26:19 +0200 Łukasz Dmitrowski lukasz.dmitrowski@gmail.com wrote:
Hello,
My name is Łukasz Dmitrowski and I am one of coreboot/flashrom GSoC students this year. You can read about my project here: http://blogs.coreboot.org/blog/author/dmitro/.
Hi Łukasz,
regarding the window showing the supported hardware: Some features that would make it better than the existing interfaces to this database (namely the -L output of flashrom as well as the tables in the wiki): - searching - sorting - filtering (e.g. showing flash chips with 4 and 8 MB density only)
The extract components icon is not ideal. The arrow you are using symbolizes "undo" usually... I would suggest that you use something like http://fotofile.net/bt/root/usr/share/icons/Humanity/actions/22/extract-arch... (a typical symbol for unwrapping a parcel).
One of my first steps is updating and extending Nico Hubers libflashrom patch. Work is still in progress, but some changes are already made, so I thought that it will be good idea to send a patch just for review to know if I am going in a good direction. I want to improve my skills and provide best code quality as I can, so I am open for criticism - I am here to learn :)
Patch summary:
Makefile: I added cli_common.o and print.o to LIB_OBJS - it solves undefined references in current libflashrom version, but spoils standard flashrom build with 'make', I did not investigated it yet as my focus is to extend library with new functions and update already existing, need to discuss it with my mentor
cli_classic.c, cli_output.c, flash.h: Only Nico Hubers changes here, didn't touch it
libflashrom.c, libflashrom.h: I added 8 functions and made a few changes to existing ones, as some of used functions are static, or does not exist.
New functions:
int fl_supported_programmers(const char **supported_programmers); int fl_supported_flash_chips(fl_flashchip_info_t *fchips); int fl_supported_boards(fl_board_info_t *boards); int fl_supported_chipsets(fl_chipset_info_t *chipsets); int fl_supported_programmers_number(); int fl_supported_flash_chips_number(); int fl_supported_boards_number(); int fl_supported_chipsets_number();
New types: fl_flashchip_info_t - smaller version of struct flashchip fl_board_info_t - smaller version of struct board_info fl_chipset_info_t - smaller version of struct penable fl_test_state - copy of enum test_state
So we have 4 functions that return a list of supported programmers, chips, boards and chipsets. Next 4 functions return number of supported hardware of certain type. For example, to obtain a list of supported boards, you can create an array of structures fl_board_info of suitable size (the size is known - fl_supported_boards_number()), then pass it to fl_supported_boards(fl_board_info_t *boards) and you will have it filled with data.
Regarding changes in Nico Hubers functions: I made temporary comments for every change and marked it with LD_CHANGE_NOTE. Every comment contains previous code and reason why it has been commented out.
Thanks in advance for your insights!
As Urja mentioned already, your patch got mangled by your MUA, which introduced some line breaks where there shouldnt be any. Also, you annotated your changes to Nico's patch within the code. It would make much more sense if you would post those changes as independent patches with a proper change log, like you did in your github repository (well maybe not split in that many patches, but the idea is right). BTW github... you should have cloned my repository there instead of starting a complete new one without history.
Also, I agree with Urja that the functions returning only the number of X are not the way to go. I am not sure yet how to proceed with them and all the external types yet. Any new types should not have a _t suffix. These names are reserved by POSIX. However, I do not feel certain if we need new types at all..
Please repost your changes in a more reviewable manner, thanks.