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.
