On Thu, Oct 20, 2022 at 07:53:25PM +0200, Maciej Kwapulinski wrote:
> Add a new PCI driver for Intel(R) Gaussian & Neural Accelerator
Please drop all of the (R) stuff in here, and in the Kconfig file and in
the .c files. If your lawyers insist on it, please point them at me and
I will be glad to discuss it with them.
> Documentation/gpu/drivers.rst | 1 +
> Documentation/gpu/gna.rst | 64 ++++++++++++++++++++++++++++++++
Why not just put the tiny documentation file in the .c code itself?
That way it stays up to date and might actually be noticed?
> --- /dev/null
> +++ b/drivers/gpu/drm/gna/Kconfig
> @@ -0,0 +1,12 @@
> +#
> +# Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA)
Again, drop the (R) stuff.
And no SPDX line?
> +#
> +
> +config DRM_GNA
> + tristate "Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA)"
> + depends on X86 && PCI
Why is this x86 only? Please let it build on other arches.
> + help
> + This option enables the Intel(R) Gaussian & Neural Accelerator
> + (Intel(R) GNA) driver: gna
> + Information about functionality is in
> + Documentation/gpu/gna.rst
See, you changed this from the first v5 version you sent :(
Also, this needs a lot more information, including the module name that
will be built and you can drop the documentation line.
> diff --git a/drivers/gpu/drm/gna/gna_device.c
> b/drivers/gpu/drm/gna/gna_device.c
> new file mode 100644
> index 000000000000..960b4341c18e
> --- /dev/null
> +++ b/drivers/gpu/drm/gna/gna_device.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright(c) 2017-2022 Intel Corporation
> +
> +#include <linux/module.h>
> +
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_DESCRIPTION("Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA)
> Driver");
> +MODULE_LICENSE("GPL");
No, that's not ok. Don't add a file that does nothing. Only add it
when you need it.
> diff --git a/drivers/gpu/drm/gna/gna_device.h
> b/drivers/gpu/drm/gna/gna_device.h
> new file mode 100644
> index 000000000000..4cc92f27765a
> --- /dev/null
> +++ b/drivers/gpu/drm/gna/gna_device.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2017-2022 Intel Corporation */
> +
> +#ifndef __GNA_DEVICE_H__
> +#define __GNA_DEVICE_H__
> +
> +#define DRIVER_NAME "gna"
This can be KBUILD_MODNAME and then the file isn't needed at all.
> +
> +#endif /* __GNA_DEVICE_H__ */
> diff --git a/drivers/gpu/drm/gna/gna_pci.c b/drivers/gpu/drm/gna/gna_pci.c
> new file mode 100644
> index 000000000000..6bd00c66f3a7
> --- /dev/null
> +++ b/drivers/gpu/drm/gna/gna_pci.c
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright(c) 2017-2022 Intel Corporation
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +
> +#include "gna_device.h"
> +#include "gna_pci.h"
> +
> +int gna_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pci_id)
> +{
> + int err;
> +
> + err = pcim_enable_device(pcidev);
> + if (err)
> + return err;
> +
> + err = pcim_iomap_regions(pcidev, BIT(0), pci_name(pcidev));
> + if (err)
> + return err;
> +
> + pci_set_master(pcidev);
> +
> + return 0;
> +}
> +
> +static struct pci_driver gna_pci_driver = {
> + .name = DRIVER_NAME,
> + .probe = gna_pci_probe,
> +};
> +
> +module_pci_driver(gna_pci_driver);
> diff --git a/drivers/gpu/drm/gna/gna_pci.h b/drivers/gpu/drm/gna/gna_pci.h
> new file mode 100644
> index 000000000000..b651fa2e6ea1
> --- /dev/null
> +++ b/drivers/gpu/drm/gna/gna_pci.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright(c) 2017-2022 Intel Corporation */
> +
> +#ifndef __GNA_PCI_H__
> +#define __GNA_PCI_H__
> +
> +struct pci_device_id;
> +struct pci_dev;
> +
> +int gna_pci_probe(struct pci_dev *dev, const struct pci_device_id *id);
This is not exported, nor should it be, so you do not need it in the .h
file.
This whole patch can be one .c file, not this mess of tiny ones.
thanks,
greg k-h
