Uwe Kleine-König
2023-Jul-12 16:10 UTC
[Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hello Jani, On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote:> On Wed, 12 Jul 2023, Uwe Kleine-K?nig <u.kleine-koenig at pengutronix.de> wrote: > > Hello, > > > > while I debugged an issue in the imx-lcdc driver I was constantly > > irritated about struct drm_device pointer variables being named "dev" > > because with that name I usually expect a struct device pointer. > > > > I think there is a big benefit when these are all renamed to "drm_dev". > > I have no strong preference here though, so "drmdev" or "drm" are fine > > for me, too. Let the bikesheding begin! > > > > Some statistics: > > > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | sort -n > > 1 struct drm_device *adev_to_drm > > 1 struct drm_device *drm_ > > 1 struct drm_device *drm_dev > > 1 struct drm_device *drm_dev > > 1 struct drm_device *pdev > > 1 struct drm_device *rdev > > 1 struct drm_device *vdev > > 2 struct drm_device *dcss_drv_dev_to_drm > > 2 struct drm_device **ddev > > 2 struct drm_device *drm_dev_alloc > > 2 struct drm_device *mock > > 2 struct drm_device *p_ddev > > 5 struct drm_device *device > > 9 struct drm_device * dev > > 25 struct drm_device *d > > 95 struct drm_device * > > 216 struct drm_device *ddev > > 234 struct drm_device *drm_dev > > 611 struct drm_device *drm > > 4190 struct drm_device *dev > > > > This series starts with renaming struct drm_crtc::dev to drm_dev. If > > it's not only me and others like the result of this effort it should be > > followed up by adapting the other structs and the individual usages in > > the different drivers. > > I think this is an unnecessary change. In drm, a dev is usually a drm > device, i.e. struct drm_device *.Well, unless it's not. Prominently there is struct drm_device { ... struct device *dev; ... }; which yields quite a few code locations using dev->dev which is IMHO unnecessary irritating: $ git grep '\<dev->dev' v6.5-rc1 drivers/gpu/drm | wc -l 1633 Also the functions that deal with both a struct device and a struct drm_device often use "dev" for the struct device and then "ddev" for the drm_device (see for example amdgpu_device_get_pcie_replay_count()).> If folks insist on following through with this anyway, I'm firmly in the > camp the name should be "drm" and nothing else.Up to now positive feedback is in the majority. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | https://www.pengutronix.de/ | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: not available URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20230712/317276cb/attachment-0001.sig>
Geert Uytterhoeven
2023-Jul-13 06:52 UTC
[PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Uwe, Let's add some fuel to keep the thread alive ;-) On Wed, Jul 12, 2023 at 6:13?PM Uwe Kleine-K?nig <u.kleine-koenig at pengutronix.de> wrote:> On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote: > > On Wed, 12 Jul 2023, Uwe Kleine-K?nig <u.kleine-koenig at pengutronix.de> wrote: > > > Hello, > > > > > > while I debugged an issue in the imx-lcdc driver I was constantly > > > irritated about struct drm_device pointer variables being named "dev" > > > because with that name I usually expect a struct device pointer. > > > > > > I think there is a big benefit when these are all renamed to "drm_dev". > > > I have no strong preference here though, so "drmdev" or "drm" are fine > > > for me, too. Let the bikesheding begin! > > > > > > Some statistics: > > > > > > $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | sort -n > > > 1 struct drm_device *adev_to_drm > > > 1 struct drm_device *drm_ > > > 1 struct drm_device *drm_dev > > > 1 struct drm_device *drm_dev > > > 1 struct drm_device *pdev > > > 1 struct drm_device *rdev > > > 1 struct drm_device *vdev > > > 2 struct drm_device *dcss_drv_dev_to_drm > > > 2 struct drm_device **ddev > > > 2 struct drm_device *drm_dev_alloc > > > 2 struct drm_device *mock > > > 2 struct drm_device *p_ddev > > > 5 struct drm_device *device > > > 9 struct drm_device * dev > > > 25 struct drm_device *d > > > 95 struct drm_device * > > > 216 struct drm_device *ddev > > > 234 struct drm_device *drm_dev > > > 611 struct drm_device *drm > > > 4190 struct drm_device *dev > > > > > > This series starts with renaming struct drm_crtc::dev to drm_dev. If > > > it's not only me and others like the result of this effort it should be > > > followed up by adapting the other structs and the individual usages in > > > the different drivers. > > > > I think this is an unnecessary change. In drm, a dev is usually a drm > > device, i.e. struct drm_device *. > > Well, unless it's not. Prominently there is > > struct drm_device { > ... > struct device *dev; > ... > }; > > which yields quite a few code locations using dev->dev which is > IMHO unnecessary irritating: > > $ git grep '\<dev->dev' v6.5-rc1 drivers/gpu/drm | wc -l > 1633I find that irritating as well... Same for e.g. crtc->crtc. Hence that's why I had sent patches to rename the base members in the shmob_drm-specific subclasses of drm_{crtc,connector,plane} to "base". https://lore.kernel.org/dri-devel/b3daca80f82625ba14e3aeaf2fca6dcefa056e47.1687423204.git.geert+renesas at glider.be> Also the functions that deal with both a struct device and a struct > drm_device often use "dev" for the struct device and then "ddev" for > the drm_device (see for example amdgpu_device_get_pcie_replay_count()).I guess you considered "drm_dev", because it is still a short name? Code dealing with platform devices usually uses "pdev" and "dev". Same for PCI drivers (despite "pci_dev" being a short name). So my personal preference goes to "ddev". EOF (End-of-Fuel ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Thomas Zimmermann
2023-Jul-13 07:47 UTC
[PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
Hi Am 12.07.23 um 18:10 schrieb Uwe Kleine-K?nig:> Hello Jani, > > On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote: >> On Wed, 12 Jul 2023, Uwe Kleine-K?nig <u.kleine-koenig at pengutronix.de> wrote: >>> Hello, >>> >>> while I debugged an issue in the imx-lcdc driver I was constantly >>> irritated about struct drm_device pointer variables being named "dev" >>> because with that name I usually expect a struct device pointer. >>> >>> I think there is a big benefit when these are all renamed to "drm_dev". >>> I have no strong preference here though, so "drmdev" or "drm" are fine >>> for me, too. Let the bikesheding begin! >>> >>> Some statistics: >>> >>> $ git grep -ohE 'struct drm_device *\* *[^ (),;]*' v6.5-rc1 | sort | uniq -c | sort -n >>> 1 struct drm_device *adev_to_drm >>> 1 struct drm_device *drm_ >>> 1 struct drm_device *drm_dev >>> 1 struct drm_device *drm_dev >>> 1 struct drm_device *pdev >>> 1 struct drm_device *rdev >>> 1 struct drm_device *vdev >>> 2 struct drm_device *dcss_drv_dev_to_drm >>> 2 struct drm_device **ddev >>> 2 struct drm_device *drm_dev_alloc >>> 2 struct drm_device *mock >>> 2 struct drm_device *p_ddev >>> 5 struct drm_device *device >>> 9 struct drm_device * dev >>> 25 struct drm_device *d >>> 95 struct drm_device * >>> 216 struct drm_device *ddev >>> 234 struct drm_device *drm_dev >>> 611 struct drm_device *drm >>> 4190 struct drm_device *dev >>> >>> This series starts with renaming struct drm_crtc::dev to drm_dev. If >>> it's not only me and others like the result of this effort it should be >>> followed up by adapting the other structs and the individual usages in >>> the different drivers. >> >> I think this is an unnecessary change. In drm, a dev is usually a drm >> device, i.e. struct drm_device *. > > Well, unless it's not. Prominently there is > > struct drm_device { > ... > struct device *dev; > ... > };Jani's point is that it's only inconvenient at the first time. Everyone gets use to it. Best regards Thomas> > which yields quite a few code locations using dev->dev which is > IMHO unnecessary irritating: > > $ git grep '\<dev->dev' v6.5-rc1 drivers/gpu/drm | wc -l > 1633 > > Also the functions that deal with both a struct device and a struct > drm_device often use "dev" for the struct device and then "ddev" for > the drm_device (see for example amdgpu_device_get_pcie_replay_count()). > >> If folks insist on following through with this anyway, I'm firmly in the >> camp the name should be "drm" and nothing else. > > Up to now positive feedback is in the majority. > > Best regards > Uwe >-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg) -------------- next part -------------- A non-text attachment was scrubbed... Name: OpenPGP_signature Type: application/pgp-signature Size: 840 bytes Desc: OpenPGP digital signature URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20230713/b2543b35/attachment.sig>
Uwe Kleine-König
2023-Jul-13 10:03 UTC
[Nouveau] [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev
On Thu, Jul 13, 2023 at 08:52:12AM +0200, Geert Uytterhoeven wrote:> Hi Uwe, > > Let's add some fuel to keep the thread alive ;-) > > On Wed, Jul 12, 2023 at 6:13?PM Uwe Kleine-K?nig > <u.kleine-koenig at pengutronix.de> wrote: > > On Wed, Jul 12, 2023 at 05:34:28PM +0300, Jani Nikula wrote: > > > I think this is an unnecessary change. In drm, a dev is usually a drm > > > device, i.e. struct drm_device *. > > > > Well, unless it's not. Prominently there is > > > > struct drm_device { > > ... > > struct device *dev; > > ... > > }; > > > > which yields quite a few code locations using dev->dev which is > > IMHO unnecessary irritating: > > > > $ git grep '\<dev->dev' v6.5-rc1 drivers/gpu/drm | wc -l > > 1633 > > I find that irritating as well... > > Same for e.g. crtc->crtc. > > Hence that's why I had sent patches to rename the base members in the > shmob_drm-specific subclasses of drm_{crtc,connector,plane} to "base". > https://lore.kernel.org/dri-devel/b3daca80f82625ba14e3aeaf2fca6dcefa056e47.1687423204.git.geert+renesas at glider.be > > > Also the functions that deal with both a struct device and a struct > > drm_device often use "dev" for the struct device and then "ddev" for > > the drm_device (see for example amdgpu_device_get_pcie_replay_count()). > > I guess you considered "drm_dev", because it is still a short name?I considered drm_dev because it is still moderately short and a good approximation of "drm_device". Other than that the main driving force to pick "drm_dev" was that it's unique enough that I could have done s/\<drm_dev\>/$nameofchoice/ on the initial patch and get it mostly right.> Code dealing with platform devices usually uses "pdev" and "dev". > Same for PCI drivers (despite "pci_dev" being a short name).pci_dev and platform_device both typlically using pdev already annoyed me in the past. However less than drm_device *dev because for pci_dev + platform_device there is little overlap.> So my personal preference goes to "ddev".I sticked to "drm" for the new series. I think this provides less fuel. Best regards and thanks for your thoughts, Uwe -- Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | https://www.pengutronix.de/ | -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: not available URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20230713/ecf3b3ad/attachment-0001.sig>