Attention is currently required from: Nico Huber, Michał Żygowski.
41 comments:
File Makefile:
ifeq ($(CONFIG_TUXEDO_EC), yes)
UNSUPPORTED_FEATURES += CONFIG_TUXEDO_EC=yes
else
override CONFIG_TUXEDO_EC = no
endif
Any reason why this programmer wouldn't work with libpayload? I'm not asking you to test it, just remove this if there's no particular reason for it.
Patch Set #1, Line 839: override CONFIG_TUXEDO_EC = no
hmmm, but this programmer does not use libpci?
Patch Set #1, Line 903: # Disable internal DMI decoder to be able to locate DMI tables on EFI systems and match TUXEDO laptops
Why?
File flashrom.8.tmpl:
Was this removed on purpose?
Patch Set #1, Line 1399: autoloading
What's autoloading?
Patch Set #1, Line 1411: ite5570
How would the reader of the man page know when to enable IT5570 support?
File flashrom.c:
Patch Set #1, Line 42: #if CONFIG_TUXEDO_EC == 1
I'd appreciate if we could avoid using preprocessor.
Patch Set #1, Line 2109: #if CONFIG_TUXEDO_EC == 1
I can see why you'd want to do this, but tacking it right here feels odd.
File tuxedo_ec.c:
Patch Set #1, Line 68: typedef struct
Typedefs are frowned upon: https://www.kernel.org/doc/html/latest/process/coding-style.html#typedefs
Patch Set #1, Line 184: flashrom@flashrom.org
Personally, I wouldn't know what to do with such a report on the mailing list.
msg_pwarn("Querying EC ROM size returned unexpected result.\n"
"Probably the EC has just been flashed and the EC "
"RAM has been reset.\nYou may need to pass the flash"
" size via the programmer parameters or try again in"
" a while.\nIf this warning persists please email a "
"report to flashrom@flashrom.org\n");
Breaking user-visible strings makes it harder to grep for them. How about:
"Querying EC ROM size returned unexpected result.\n"
"Probably the EC has just been flashed and the EC RAM has been reset.\n"
"You may need to pass the flash size via the programmer parameters or try again in a while.\n"
"If this warning persists please email a report to flashrom@flashrom.org\n"
Patch Set #1, Line 205: ec_read_reg
Why `ec_read_reg` and not `tuxedo_ec_read_byte`?
if (!ec_read_reg(0x10, ®_value)) {
msg_perr("Failed to query first byte of state register.\n");
return false;
}
if (!ec_read_reg(0x10, ®_value)) {
msg_perr("Failed to query AC adapter state.\n");
return false;
}
Do I understand correctly that reading this register results in a side-effect? How does one know which is the first byte?
Patch Set #1, Line 235: len % BYTES_PER_BLOCK != 0 && len < BYTES_PER_BLOCK
Are you sure this check is supposed to use an AND? Shouldn't it be checking that the length is a multiple of `BYTES_PER_BLOCK`?
if (!len || len % BYTES_PER_BLOCK != 0) {Patch Set #1, Line 286: return tuxedo_ec_write_byte(ctx_data, data);
`break;` would be equivalent and easier to understand.
Patch Set #1, Line 316: unsigned int
uint8_t ?
Patch Set #1, Line 318: unsigned int
uint8_t ?
Patch Set #1, Line 324: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK
sizeof(ctx_data->first_kbyte)
Patch Set #1, Line 361: CHUNKS_PER_KBYTE * BYTES_PER_CHUNK
sizeof(ctx_data->first_kbyte)
Patch Set #1, Line 384: offset
`start + i`?
Patch Set #1, Line 387: offset - ctx_data->autoload_offset != 1
Can these checks ever be true? The state is reset to `AUTOLOAD_NONE` already.
Patch Set #1, Line 433: len % BYTES_PER_BLOCK != 0 && len < BYTES_PER_BLOCK
Are you sure this check is supposed to use an AND? Shouldn't it be checking that the length is a multiple of `BYTES_PER_BLOCK`?
if (!len || len % BYTES_PER_BLOCK != 0) {Patch Set #1, Line 477: internal_sleep(15000 * 64);
No way to poll for completion?
if (!ec_wait_for_obuf(ctx_data->control_port,
EC_MAX_STATUS_CHECKS * 3))
return 1;
if (INB(ctx_data->data_port) == 0xf8)
return 0;
Looks like this is open-coding `tuxedo_ec_read_byte` because the `max_checks` parameter isn't exposed.
Patch Set #1, Line 504: ++to_chunk
This would make flashrom erase too much?
Patch Set #1, Line 534: legnth
typo: length
Patch Set #1, Line 551: ctx_data->rom_size_in_kbytes
This is the only place where `rom_size_in_kbytes` is used. Given that it provides redundant information, why not drop the member and use `ctx_data->rom_size_in_blocks * KBYTES_PER_BLOCK` here?
Oh wait, this is what the block eraser size already does. If you don't want to repeat the calculation twice (it currently appears three times), make `rom_size_in_kbytes` a local constant inside this function.
Patch Set #1, Line 555: * Erase operation must be done in one sway.
Doesn't seem to be the case for IT5570? The existing code doesn't allow making use of this, though.
p = extract_programmer_param("type");
if (p && strcmp(p, "ec")) {
msg_pdbg("%s(): tuxedo_ec only supports \"ec\" type devices\n",
__func__);
ret = false;
}
free(p);
Remove this, please. I don't expect anything else to be supported in the foreseeable future, and this could be added later anyway.
Patch Set #1, Line 666: static void get_flash_part_from_id(tuxedo_ec_data_t *ctx_data, uint32_t manuf_id,
Please ignore. I left a comment here before, but Gerrit doesn't let me delete the draft...
Patch Set #1, Line 713: if (id_length == 4)
If you zero-initialise the array, this "if" can be removed.
Patch Set #1, Line 722: = NULL
Dead assignment
if (!dmi_match("^InfinityBook S 14 Gen6$") &&
!dmi_match("^InfinityBook S 15 Gen6$")) {
To ease adding support for more laptops in the future, put the strings in an array and loop over it?
Patch Set #1, Line 750: extra
extra in which sense?
if (register_opaque_master(&programmer_opaque_tuxedo_ec, ctx_data))
return 1;
msg_pdbg("%s(): successfully initialized tuxedo_ec\n", __func__);
return 0;
Drop the print and return `register_opaque_master` directly?
tuxedo_ec_init_exit:
tuxedo_ec_shutdown(ctx_data);
return 1;
tuxedo_ec_init_exit_shutdown:
tuxedo_ec_shutdown(ctx_data);
return 1;
Am I missing something, or are these two paths identical?
typo: not
Patch Set #1, Line 826: Ifthe
missing a space
If the current and the file's EC version is identical,
* return two (skip flashing)
Is there any reason to skip flashing if the EC version matches? flashrom already checks if the current contents already match, and skipping flashing when only the version matches could prevent recovering from a botched flash.
Patch Set #1, Line 832: Contects
typo: Contents
.name = "tuxedo_ec",
.type = OTHER,
.devs.note = "Embedded Controller of TUXEDO laptops.\n",
.init = tuxedo_ec_init,
.map_flash_region = fallback_map,
.unmap_flash_region = fallback_unmap,
.delay = internal_delay,
One tab too many in the indentation?
To view, visit change 55715. To unsubscribe, or for help writing mail filters, visit settings.