Re: [PATCH v1 1/3] firmware: coreboot: support for parsing SMM related informations from coreboot tables

[Michal Gorlas <michal.gorlas@9elements.com>]

On Mon, Jun 16, 2025 at 11:16:18AM -0700, Brian Norris wrote:
> On Sat, Jun 14, 2025 at 02:53:18PM +0200, Michal Gorlas wrote:
> > On Thu, Jun 12, 2025 at 03:37:40PM -0700, Brian Norris wrote:
> > > > @@ -112,8 +122,8 @@ void coreboot_driver_unregister(struct coreboot_driver *driver);
> > > >   * boilerplate.  Each module may only use this macro once, and
> > > >   * calling it replaces module_init() and module_exit()
> > > >   */
> > > > -#define module_coreboot_driver(__coreboot_driver) \
> > > > +#define module_coreboot_driver(__coreboot_driver)                  \
> > > >  	module_driver(__coreboot_driver, coreboot_driver_register, \
> > > > -			coreboot_driver_unregister)
> > > > +		      coreboot_driver_unregister)
> > > 
> > > You're making arbitrary whitespace changes in this hunk. Try to avoid
> > > that, please.
> > > 
> > 
> > Sure, will do. It came from a style warning when running
> > scripts/checkpatch.pl. I thought it could be useful to fix it on the
> > same go.
> 
> That's odd, I don't see any such warning. Anyway, typically I'd expect
> such things not to be lumped together under the "separate your changes"
> guidance of Documentation/process/submitting-patches.rst (if they're
> worth changing at all), although that may not be a hard and fast rule.

My bad, the whitespace were not a consequence of checkpatch.pl
complaints, but rather running clang-format on coreboot_table.h
on a clean tree:

 * $ clang-format -i drivers/firmware/google/coreboot_table.h
 * $ git diff
 * diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
 * index bb6f0f7299b4..3d933c657535 100644
 * --- a/drivers/firmware/google/coreboot_table.h
 * +++ b/drivers/firmware/google/coreboot_table.h
 * @@ -61,15 +61,15 @@ struct lb_framebuffer {
 *         u32 x_resolution;
 *         u32 y_resolution;
 *         u32 bytes_per_line;
 * -       u8  bits_per_pixel;
 * -       u8  red_mask_pos;
 * -       u8  red_mask_size;
 * -       u8  green_mask_pos;
 * -       u8  green_mask_size;
 * -       u8  blue_mask_pos;
 * -       u8  blue_mask_size;
 * -       u8  reserved_mask_pos;
 * -       u8  reserved_mask_size;
 * +       u8 bits_per_pixel;
 * +       u8 red_mask_pos;
 * +       u8 red_mask_size;
 * +       u8 green_mask_pos;
 * +       u8 green_mask_size;
 * +       u8 blue_mask_pos;
 * +       u8 blue_mask_size;
 * +       u8 reserved_mask_pos;
 * +       u8 reserved_mask_size;
 *  };
 * 
 *  /* A device, additionally with information from coreboot. */
 * @@ -112,8 +112,8 @@ void coreboot_driver_unregister(struct coreboot_driver *driver);
 *   * boilerplate.  Each module may only use this macro once, and
 *   * calling it replaces module_init() and module_exit()
 *   */
 * -#define module_coreboot_driver(__coreboot_driver) \
 * +#define module_coreboot_driver(__coreboot_driver)                  \
 *         module_driver(__coreboot_driver, coreboot_driver_register, \
 * -                       coreboot_driver_unregister)
 * +                     coreboot_driver_unregister)
 * 
 *  #endif /* __COREBOOT_TABLE_H */

Anyway, I am not sure if it is even worth changing, its rather cosmetic. I
removed these changes in v2. 

Best,
Michal