On Fri, 15 Nov 2019 14:32:47 +0000 gengdongjiu <gengdongjiu@xxxxxxxxxx> wrote: > > > + */ > > > +static void acpi_ghes_build_notify(GArray *table, const uint8_t type) > > > > typically format should be build_WHAT(), so > > build_ghes_hw_error_notification() > > > > And I'd move this out into its own patch. > > this applies to other trivial in-depended sub-tables, that take all data needed to construct them from supplied arguments. > > I very used your suggested method in previous series[1], but other maintainer suggested to move this function to this file, because he think only GHES used it Using this file is ok, what I've meant for you to split function from this patch into a separate smaller patch. With the way code split now I have to review 2 big complex patches at the same which makes reviewing hard and I have a hard time convincing myself that code it correct. Moving small self-contained chunks of code in to separate smaller patches makes review easier. > > [1]: > https://patchwork.ozlabs.org/cover/1099428/ > > > > > > +{ > > > + /* Type */ > > > + build_append_int_noprefix(table, type, 1); > > > + /* > > > + * Length: > > > + * Total length of the structure in bytes > > > + */ > > > + build_append_int_noprefix(table, 28, 1); > > > + /* Configuration Write Enable */ > > > + build_append_int_noprefix(table, 0, 2); > > > + /* Poll Interval */ > > > + build_append_int_noprefix(table, 0, 4); > > > + /* Vector */ > > > + build_append_int_noprefix(table, 0, 4); > > > + /* Switch To Polling Threshold Value */ > > > + build_append_int_noprefix(table, 0, 4); > > > + /* Switch To Polling Threshold Window */ > > > + build_append_int_noprefix(table, 0, 4); > > > + /* Error Threshold Value */ > > > + build_append_int_noprefix(table, 0, 4); > > > + /* Error Threshold Window */ > > > + build_append_int_noprefix(table, 0, 4); } > > > + > > > > /* > > Initialize "etc/hardware_errors" and "etc/hardware_errors_addr" fwcfg blobs. > > See docs/specs/acpi_hest_ghes.rst for blobs format */ > > > +void acpi_ghes_build_error_table(GArray *hardware_errors, BIOSLinker > > > +*linker) > > build_ghes_error_table() > > > > also I'd move this function into its own patch along with other related code that initializes and wires it into virt board. > > I ever use your suggested method[1], but other maintainer, it seems Michael, suggested to move these functions to this file that used it, because he think only GHES used it. > > [1]: > https://patchwork.ozlabs.org/patch/1099424/ > https://patchwork.ozlabs.org/patch/1099425/ > https://patchwork.ozlabs.org/patch/1099430/ > >
