Re: [PATCH RFC v1 00/52] drm/crtc: Rename struct drm_crtc::dev to drm_dev

[Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>]

On Wed, Jul 12, 2023 at 12:46:33PM +0200, Christian König wrote:
> Am 12.07.23 um 11:46 schrieb Uwe Kleine-König:
> > 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.
> > 
> > To make this series a bit easier handleable, I first added an alias for
> > drm_crtc::dev, then converted the drivers one after another and the last
> > patch drops the "dev" name. This has the advantage of being easier to
> > review, and if I should have missed an instance only the last patch must
> > be dropped/reverted. Also this series might conflict with other patches,
> > in this case the remaining patches can still go in (apart from the last
> > one of course). Maybe it also makes sense to delay applying the last
> > patch by one development cycle?
> 
> When you automatically generate the patch (with cocci for example) I usually
> prefer a single patch instead.

Maybe I'm too stupid, but only parts of this patch were created by
coccinelle. I failed to convert code like

-       spin_lock_irq(&crtc->dev->event_lock);
+       spin_lock_irq(&crtc->drm_dev->event_lock);

Added Julia to Cc, maybe she has a hint?!

(Up to now it's only 

@@
struct drm_crtc *crtc;
@@
-crtc->dev
+crtc->drm_dev

)

> Background is that this makes merge conflicts easier to handle and detect.

Really? Each file (apart from include/drm/drm_crtc.h) is only touched
once. So unless I'm missing something you don't get less or easier
conflicts by doing it all in a single patch. But you gain the freedom to
drop a patch for one driver without having to drop the rest with it. So
I still like the split version better, but I'm open to a more verbose
reasoning from your side.

> When you have multiple patches and a merge conflict because of some added
> lines using the old field the build breaks only on the last patch which
> removes the old field.

Then you can revert/drop the last patch without having to respin the
whole single patch and thus caring for still more conflicts that arise
until the new version is sent.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature