Re: [PATCH] Subject: [PATCH] firmware: google: Add comments to coreboot_table.c

[Greg KH <gregkh@linuxfoundation.org>]

On Sun, Sep 21, 2025 at 06:54:46PM +0530, Joshua Jayasingh wrote:
> The coreboot_table.c file contains no comments, making it difficult
> for new developers to understand the logic for parsing the coreboot
> tables and locating firmware entries.

Nit, your Subject: line has "Subject" in it again :)

> This patch adds comprehensive comments throughout the file, explaining
> the purpose of key functions, structs, and complex operations to improve
> overall readability and maintainability.
> 
> Signed-off-by: Joshua Jayasingh <joshuajayasingh08@gmail.com>
> ---
>  drivers/firmware/google/coreboot_table.c | 187 +++++++++++++++++++++--
>  1 file changed, 177 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> index 882db32e51be..d78038bd069f 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -2,7 +2,11 @@
>  /*
>   * coreboot_table.c
>   *
> - * Module providing coreboot table access.
> + * Module providing coreboot table access. This module creates a new bus type
> + * ("coreboot") and a platform driver. The driver finds the coreboot table in
> + * memory (via ACPI or Device Tree), parses it, and creates a new device on the
> + * coreboot bus for each entry in the table. Other drivers can then register
> + * with the coreboot bus to interact with these specific entries.
>   *
>   * Copyright 2017 Google Inc.
>   * Copyright 2017 Samuel Holland <samuel@sholland.org>
> @@ -21,26 +25,52 @@
>  
>  #include "coreboot_table.h"
>  
> +/* Helper macro to get the coreboot_device from a struct device. */
>  #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
> +/* Helper macro to get the coreboot_driver from a const struct device_driver. */
>  #define CB_DRV(d) container_of_const(d, struct coreboot_driver, drv)

Close, but not quiet correct documentation for this last macro.  Why do
you think that it has to be "const"?  It should work without it, right?

> +/**
> + * coreboot_bus_match - Match a coreboot device with a coreboot driver.
> + * @dev: The coreboot device.
> + * @drv: The coreboot device driver.
> + *
> + * This function is called by the driver core to determine if a driver can
> + * handle a specific device. It iterates through the driver's ID table and
> + * compares the coreboot entry tag of the device with the tags supported by
> + * the driver.
> + *
> + * Return: 1 if the device's tag matches an ID in the driver's table,
> + * 0 otherwise.
> + */
>  static int coreboot_bus_match(struct device *dev, const struct device_driver *drv)

Why are you documenting static functions that do "obvious" things?

I'm all for documenting stuff, but why do all of this to this old
driver?  Where did this documentation come from?  Was it generated by a
tool?

thanks,

greg k-h