Attention is currently required from: Jakub Czapiga, Michał Żygowski, Subrata Banik.
Marek Maślanka has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79909?usp=email )
Change subject: soc/intel/common/block: Add support for watchdog ......................................................................
Patch Set 13:
(2 comments)
File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/79909/comment/60029fb0_71be61e5 : PS12, Line 468: entry->action = ACPI_WDAT_SET_RUNNING_STATE; : entry->instruction = ACPI_WDAT_WRITE_VALUE | ACPI_WDAT_PRESERVE_REGISTER; : entry->mask = 0x01; : entry->register_region.space_id = ACPI_ADDRESS_SPACE_IO; : entry->register_region.addrl = tcobase + TCO1_CNT; : entry->register_region.access_size = ACPI_WDAT_ACCESS_SIZE_WORD; : entry->register_region.bit_offset = 11;
I would prefer to keep it consistent with filling in other entries instead of creating separate fu […]
By "consistent with filling in other entries" I meant that you want to have a fill_wdat_entry function that will only fill those two entries ACPI_WDAT_SET_RUNNING_STATE and ACPI_WDAT_SET_STOPPED_STATE, and leave the rest unchanged (fills it directly in the acpi_soc_fill_wdat function).
Anyway, notice that the entries differ not only in `action` type, but also in other fields, which will require function with many parameter. Maybe it will be better to introduce separate functions for related entries/actions like, "fill_wdat_run_status_entry", "fill_wdat_boot_status_entry", "fill_wdat_ping_entry" and "fill_wdat_timeout_entry". In that case functions will have a small amount of parameters.
File src/soc/intel/common/block/acpi/acpi.c:
https://review.coreboot.org/c/coreboot/+/79909/comment/20f72884_55c699e4 : PS13, Line 396: rightmost_bit_offset
i'm unable to follow which right most bit u r considering ? may be a little meaningful name with the […]
The only thing that comes to mind to make the name more descriptive is to change it to `find_rightmost_set_bit_offset`