Re: [PATCH v7 1/5] cdx: add the headers to include/linux

[Yazen Ghannam <yazen.ghannam@amd.com>]

On Thu, May 29, 2025 at 12:30:13PM +0530, Shubhrajyoti Datta wrote:
> Add a the bitfield.h and mcdi.h headers.

Extra "a"?

> This is in preparation for VersalNET EDAC
> driver that relies on it.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
> 
> Changes in v7:
> - add a minimal header instead moving them
> 
> Changes in v6:
>  - Patch added
> 
>  include/linux/cdx/bitfield.h |  78 ++++++++++++++
>  include/linux/cdx/mcdi.h     | 192 +++++++++++++++++++++++++++++++++++

These only get used by VersalNET EDAC driver at the end.

So the headers can go in drivers/edac.

Also, maybe these can all be a single header file? It seems like each
one is just included in the next one.

	$ git grep "cdx/bitfield"
	include/linux/cdx/mcdi.h:#include "linux/cdx/bitfield.h"
	$ git grep "cdx/mcdi"
	include/linux/cdx/edac_cdx_pcol.h:#include  <linux/cdx/mcdi.h>
	$ git grep "cdx/edac_cdx_pcol"
	drivers/edac/versalnet_edac.c:#include <linux/cdx/edac_cdx_pcol.h>

If these are truly independent header files, then the "c" file should
include them all. You should not depend on a header file including other
header files, if possible.

From "Documentation/process/submit-checklist.rst":
   1) If you use a facility then #include the file that defines/declares
      that facility.  Don't depend on other header files pulling in ones
      that you use.

Thanks,
Yazen