Nouveau - Mar 2022 - [PATCH] drm/nouveau/bios: Rename prom_init() and friends functions

This mostly looks good to me. Just one question (and one comment down below
that needs addressing). Is this with ppc32? (I ask because ppc64le doesn't
seem to hit this compilation error).

On Fri, 2022-03-04 at 18:20 +0100, Christophe Leroy
wrote:
> While working on powerpc headers, I ended up with the following error.
> 
> ????????drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c:48:1: error:
> conflicting types for 'prom_init'; have 'void *(struct
nvkm_bios *, const
> char *)'
> ????????make[5]: *** [scripts/Makefile.build:288:
> drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.o] Error 1
> 
> powerpc and a few other architectures have a prom_init() global function.
> One day or another it will conflict with the one in shadowrom.c
> 
> Those being static, they can easily be renamed. Do it.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy at csgroup.eu>
> ---
> ?drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c | 12 ++++++------
> ?1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c
> index ffa4b395220a..9c951e90e622 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c
> @@ -25,7 +25,7 @@
> ?#include <subdev/pci.h>
> ?
> ?static u32
> -prom_read(void *data, u32 offset, u32 length, struct nvkm_bios *bios)
> +nvbios_rom_read(void *data, u32 offset, u32 length, struct nvkm_bios
*bios)
> ?{
> ????????struct nvkm_device *device = data;
> ????????u32 i;
> @@ -38,14 +38,14 @@ prom_read(void *data, u32 offset, u32 length, struct
> nvkm_bios *bios)
> ?}
> ?
> ?static void
> -prom_fini(void *data)
> +nvbios_rom_fini(void *data)
> ?{
> ????????struct nvkm_device *device = data;
> ????????nvkm_pci_rom_shadow(device->pci, true);
> ?}
> ?
> ?static void *
> -prom_init(struct nvkm_bios *bios, const char *name)
> +nvbios_rom_init(struct nvkm_bios *bios, const char *name)
> ?{
> ????????struct nvkm_device *device = bios->subdev.device;
> ????????if (device->card_type == NV_40 && device->chipset
>= 0x4c)
> @@ -57,8 +57,8 @@ prom_init(struct nvkm_bios *bios, const char *name)
> ?const struct nvbios_source
> ?nvbios_rom = {
> ????????.name = "PROM",
> -???????.init = prom_init,
> -???????.fini = prom_fini,
> -???????.read = prom_read,
> +???????.init = nvbios_rom_init,
> +???????.fini = nvbios_rom_fini,
> +???????.read = nvbios_rom_read,
Seeing as the source name is prom, I think using the naming convention
nvbios_prom_* would be better then nvbios_rom_*.
> ????????.rw = false,
> ?};
-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Le 04/03/2022 ? 21:24, Lyude Paul a ?crit?:
> This mostly looks good to me. Just one question (and one comment down below
> that needs addressing). Is this with ppc32? (I ask because ppc64le
doesn't
> seem to hit this compilation error).
That's with PPC64, see 
http://kisskb.ellerman.id.au/kisskb/branch/chleroy/head/252ba609bea83234d2e35841c19ae84c67b43ec7/

But that's not (yet) with the mainline tree. That's work I'm doing
to
cleanup our asm/asm-protoypes.h header.

Since commit 4efca4ed05cb ("kbuild: modversions for EXPORT_SYMBOL() for 
asm") that file is dedicated to prototypes of functions defined in 
assembly. Therefore I'm trying to dispatch C functions prototypes in 
other headers. I wanted to move prom_init() prototype into asm/prom.h 
and then I hit the problem.

In the beginning I was thinking about just changing the name of the 
function in powerpc, but as I see that M68K, MIPS and SPARC also have a 
prom_init() function, I thought it would be better to change the name in 
shadowrom.c to avoid any future conflict like the one I got while 
reworking the headers.

>> @@ -57,8 +57,8 @@ prom_init(struct nvkm_bios *bios, const char *name)
>>  ?const struct nvbios_source
>>  ?nvbios_rom = {
>>  ????????.name = "PROM",
>> -???????.init = prom_init,
>> -???????.fini = prom_fini,
>> -???????.read = prom_read,
>> +???????.init = nvbios_rom_init,
>> +???????.fini = nvbios_rom_fini,
>> +???????.read = nvbios_rom_read,
> 
> Seeing as the source name is prom, I think using the naming convention
> nvbios_prom_* would be better then nvbios_rom_*.
> 
Yes I wasn't sure about the best naming as the file name is shadowrom.c 
and not shadowprom.c.

I will send v2 using nvbios_prom_* as a name.

Christophe