[PATCH v5 01/10] gna: add PCI driver module

[Maciej Kwapulinski]

On 10/21/2022 6:20 AM, Greg Kroah-Hartman wrote:
> 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 :(

actually I've sent patch v4 (once) and patch v5 (once).  according to change log in cover letter:

·v4->v5:$
·-·indentation·fixed·in·drivers/gpu/drm/gna/Kconfig$

>
> 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.
well, it provides metadata
>
>
>> 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.

I just wanted to have file's final render from very beginning.