[PATCH v4 1/4] bus/cdx: introduce cdx bus

[Gupta, Nipun]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Thomas Monjalon <thomas at monjalon.net>
> Sent: Wednesday, May 24, 2023 4:45 PM
> To: Gupta, Nipun <Nipun.Gupta at amd.com>
> Cc: dev at dpdk.org; david.marchand at redhat.com; Yigit, Ferruh
> <Ferruh.Yigit at amd.com>; Anand, Harpreet <harpreet.anand at amd.com>;
> Agarwal, Nikhil <nikhil.agarwal at amd.com>
> Subject: Re: [PATCH v4 1/4] bus/cdx: introduce cdx bus
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Hello,
>
> If I understand well, it is very specific to AMD devices.
> So I suggest adding "AMD" in title and descriptions.
>
> 08/05/2023 13:18, Nipun Gupta:
> > CDX bus supports multiple type of devices, which can be
> > exposed to user-space via vfio-cdx.
> >
> > vfio-cdx provides the MMIO IO_MEMORY regions as well as the
> > DMA interface for the device (IOMMU).
> >
> > This support aims to enable the DPDK to support the cdx
> > devices in user-space using VFIO interface.
> >
> > Signed-off-by: Nipun Gupta <nipun.gupta at amd.com>
> [...]
> > +CDX bus driver
>
> Can we name it "AMD CDX bus"?

Sure will update.

>
> > +M: Nipun Gupta <nipun.gupta at amd.com>
> > +M: Nikhil Agarwal <nikhil.agarwal at amd.com>
> > +F: drivers/bus/cdx/
>
> [...]
> > +* **Added CDX bus support.**
>
> Here as well and in other places, would be more precise to say "AMD CDX".
>
> > +
> > +  CDX bus driver has been added to support AMD CDX bus, which operates
> > +  on FPGA based CDX devices. The CDX devices are memory mapped on
> system
> > +  bus for embedded CPUs.
> [...]
> > +#ifndef _BUS_CDX_DRIVER_H_
> > +#define _BUS_CDX_DRIVER_H_
>
> In general I prefer not adding underscores,
> as it is not required, and not really private.

Okay.. I will update for all the header files.

>
> [...]
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <limits.h>
> > +#include <errno.h>
> > +#include <stdint.h>
> > +#include <inttypes.h>
>
> I would bet some includes are not needed for this header file.

Will cut them short.

Thanks,
Nipun

>
> > +
> > +#include <bus_driver.h>
> > +#include <dev_driver.h>
> > +#include <rte_debug.h>
> > +#include <rte_interrupts.h>
> > +#include <rte_dev.h>
> > +#include <rte_bus.h>
> [...]
>
>