| Message ID | 2d97ae92b9c06214be0e088a72cf303eb591bf01.1646414295.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/nouveau/bios: Rename prom_init() and friends functions | expand |
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@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, > };
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
Le 05/03/2022 à 08:38, Christophe Leroy a écrit : > > > 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. While preparing v2 I remembered that in fact, I called the functions nvbios_rom_* because the name of the nvbios_source struct is nvbios_rom, so for me it made sense to use the name of the struct as a prefix for the functions. So I'm OK to change it to nvbios_prom_* but it looks less logical to me. Please confirm you still prefer nvbios_prom as prefix to the function names. Christophe
Hi Paul, Le 05/03/2022 à 10:51, Christophe Leroy a écrit : > > > Le 05/03/2022 à 08:38, Christophe Leroy a écrit : >> >> >> 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. > > While preparing v2 I remembered that in fact, I called the functions > nvbios_rom_* because the name of the nvbios_source struct is nvbios_rom, > so for me it made sense to use the name of the struct as a prefix for > the functions. > > So I'm OK to change it to nvbios_prom_* but it looks less logical to me. > > Please confirm you still prefer nvbios_prom as prefix to the function > names. > Are you still expecting a v2 for this patch ? As the name of the structure is nvbios_rom, do you really prefer the functions to be called nvbios_prom_* as you mentionned in your comment ? In that case, do you also expect the structure name to be changed to nvbios_prom ? Thanks Christophe
Whoops, sorry! I was unsure of the preference in name we should go with so I poked Ben on the side to ask them, but I can see they haven't yet responded. I'll poke thme again and see if I can get a response. On Fri, 2022-03-18 at 10:55 +0100, Christophe Leroy wrote: > Hi Paul, > > Le 05/03/2022 à 10:51, Christophe Leroy a écrit : > > > > > > Le 05/03/2022 à 08:38, Christophe Leroy a écrit : > > > > > > > > > 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. > > > > While preparing v2 I remembered that in fact, I called the functions > > nvbios_rom_* because the name of the nvbios_source struct is nvbios_rom, > > so for me it made sense to use the name of the struct as a prefix for > > the functions. > > > > So I'm OK to change it to nvbios_prom_* but it looks less logical to me. > > > > Please confirm you still prefer nvbios_prom as prefix to the function > > names. > > > > Are you still expecting a v2 for this patch ? > > As the name of the structure is nvbios_rom, do you really prefer the > functions to be called nvbios_prom_* as you mentionned in your comment ? > > In that case, do you also expect the structure name to be changed to > nvbios_prom ? > > Thanks > Christophe >
On Sat, 19 Mar 2022 at 04:11, Lyude Paul <lyude@redhat.com> wrote: > > Whoops, sorry! I was unsure of the preference in name we should go with so I > poked Ben on the side to ask them, but I can see they haven't yet responded. > I'll poke thme again and see if I can get a response. Yeah, please keep _prom as opposed to _rom. It's a reference to the NV_PROM device. Ben. > > On Fri, 2022-03-18 at 10:55 +0100, Christophe Leroy wrote: > > Hi Paul, > > > > Le 05/03/2022 à 10:51, Christophe Leroy a écrit : > > > > > > > > > Le 05/03/2022 à 08:38, Christophe Leroy a écrit : > > > > > > > > > > > > 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. > > > > > > While preparing v2 I remembered that in fact, I called the functions > > > nvbios_rom_* because the name of the nvbios_source struct is nvbios_rom, > > > so for me it made sense to use the name of the struct as a prefix for > > > the functions. > > > > > > So I'm OK to change it to nvbios_prom_* but it looks less logical to me. > > > > > > Please confirm you still prefer nvbios_prom as prefix to the function > > > names. > > > > > > > Are you still expecting a v2 for this patch ? > > > > As the name of the structure is nvbios_rom, do you really prefer the > > functions to be called nvbios_prom_* as you mentionned in your comment ? > > > > In that case, do you also expect the structure name to be changed to > > nvbios_prom ? > > > > Thanks > > Christophe > > > > -- > Cheers, > Lyude Paul (she/her) > Software Engineer at Red Hat >
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, .rw = false, };
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@csgroup.eu> --- drivers/gpu/drm/nouveau/nvkm/subdev/bios/shadowrom.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)