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

[Andrzej Hajda]

On 12.07.2023 13:07, Julia Lawall wrote:
>
> On Wed, 12 Jul 2023, Uwe Kleine-König wrote:
>
>> 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?!
> A priori, I see no reason why the rule below should not apply to the above
> code.  Is there a parsing problem in the containing function?  You can run
>
> spatch --parse-c file.c
>
> If there is a paring problem, please let me know and i will try to fix it
> so the while thing can be done automatically.

I guess some clever macros can fool spatch, at least I observe such 
things in i915 which often uses custom iterators.

Regards
Andrzej

>
> julia
>
>> (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/ |
> >