RE: [PATCH 01/19] bus/cdx: add the cdx bus driver

[Gupta, Nipun]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> Sent: Tuesday, January 17, 2023 10:52 PM
> To: Gupta, Nipun <Nipun.Gupta@xxxxxxx>; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> rafael@xxxxxxxxxx; eric.auger@xxxxxxxxxx; alex.williamson@xxxxxxxxxx;
> cohuck@xxxxxxxxxx; song.bao.hua@xxxxxxxxxxxxx;
> mchehab+huawei@xxxxxxxxxx; maz@xxxxxxxxxx; f.fainelli@xxxxxxxxx;
> jeffrey.l.hugo@xxxxxxxxx; saravanak@xxxxxxxxxx; Michael.Srba@xxxxxxxxx;
> mani@xxxxxxxxxx; yishaih@xxxxxxxxxx; jgg@xxxxxxxx; jgg@xxxxxxxxxx;
> robin.murphy@xxxxxxx; will@xxxxxxxxxx; joro@xxxxxxxxxx;
> masahiroy@xxxxxxxxxx; ndesaulniers@xxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kbuild@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Cc: okaya@xxxxxxxxxx; Anand, Harpreet <harpreet.anand@xxxxxxx>; Agarwal,
> Nikhil <nikhil.agarwal@xxxxxxx>; Simek, Michal <michal.simek@xxxxxxx>;
> git (AMD-Xilinx) <git@xxxxxxx>
> Subject: Re: [PATCH 01/19] bus/cdx: add the cdx bus driver
> 
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
> 
> 
> Hi--
> 
> On 1/17/23 05:41, Nipun Gupta wrote:
> > Introduce AMD CDX bus, which provides a mechanism for scanning
> > and probing CDX devices. These devices are memory mapped on
> > system bus for Application Processors(APUs).
> >
> > CDX devices can be changed dynamically in the Fabric and CDX
> > bus interacts with CDX controller to rescan the bus and
> > rediscover the devices.
> >
> > Signed-off-by: Nipun Gupta <nipun.gupta@xxxxxxx>
> > Signed-off-by: Tarak Reddy <tarak.reddy@xxxxxxx>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-cdx |  12 +
> >  MAINTAINERS                             |   7 +
> >  drivers/bus/Kconfig                     |   1 +
> >  drivers/bus/Makefile                    |   2 +
> >  drivers/bus/cdx/Kconfig                 |  14 +
> >  drivers/bus/cdx/Makefile                |   8 +
> >  drivers/bus/cdx/cdx.c                   | 433 ++++++++++++++++++++++++
> >  drivers/bus/cdx/cdx.h                   |  62 ++++
> >  include/linux/cdx/cdx_bus.h             | 153 +++++++++
> >  include/linux/mod_devicetable.h         |  15 +
> >  scripts/mod/devicetable-offsets.c       |   4 +
> >  scripts/mod/file2alias.c                |  12 +
> >  12 files changed, 723 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-cdx
> >  create mode 100644 drivers/bus/cdx/Kconfig
> >  create mode 100644 drivers/bus/cdx/Makefile
> >  create mode 100644 drivers/bus/cdx/cdx.c
> >  create mode 100644 drivers/bus/cdx/cdx.h
> >  create mode 100644 include/linux/cdx/cdx_bus.h
> >
> 
> > diff --git a/drivers/bus/cdx/Kconfig b/drivers/bus/cdx/Kconfig
> > new file mode 100644
> > index 000000000000..54e0623ebcff
> > --- /dev/null
> > +++ b/drivers/bus/cdx/Kconfig
> > @@ -0,0 +1,14 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# CDX bus configuration
> > +#
> > +# Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> > +#
> > +
> > +config CDX_BUS
> > +     bool "CDX Bus driver"
> > +     help
> > +       Driver to enable CDX Bus. CDX bus provides a mechanism for
> > +       scanning and probing of CDX devices. CDX devices are memory
> > +       mapped on system bus for embedded CPUs. CDX bus uses CDX
> > +       controller and firmware to scan the CDX devices.
> 
> Would you mind telling people who use 'make *config' what CDX means,
> either in the bool prompt string or in the help text?

Sure. Will update the help section for this.

> 
> 
> > +/**
> > + * cdx_unregister_device - Unregister a CDX device
> > + * @dev: CDX device
> > + * @data: This is always passed as NULL, and is not used in this API,
> > + *     but is required here as the bus_for_each_dev() API expects
> > + *     the passed function (cdx_unregister_device) to have this
> > + *     as an argument.
> > + *
> > + * @return: -errno on failure, 0 on success.
> 
> The syntax (or spelling) for a function's return value in kernel-doc is just:
>  * Return: -errno on failure, 0 on success.
> 
> That should be changed in many places throughout.

Agree. Will update at all places.

Thanks,
Nipun

> 
> Thanks.
> 
> --
> ~Randy