Hi,
Thanks for you patch, I'm here join to the discussion.
I will express my opinion, educate me if I'm wrong.
My understanding is that drm_device is not really a device like
pci_device and platform_device.
the name 'dev' is generally refer to 'struct device *'.
So I think, the name 'drm' or 'ddev' is more better than 'drm_dev', and
easy to type.
'drm_dev' is looks ugly.
Try to find a best name you love and replace all.
No union please, that is just a grammar sugar. Personally I don't want it.
On 2023/7/12 21:38, Uwe Kleine-König wrote:
Hello Maxime,
On Wed, Jul 12, 2023 at 02:52:38PM +0200, Maxime Ripard wrote:
On Wed, Jul 12, 2023 at 01:02:53PM +0200, Uwe Kleine-König wrote:
Background is that this makes merge conflicts easier to handle and detect.
Really?
FWIW, I agree with Christian here.
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.
Not really, because the last patch removed the union anyway. So you have
to revert both the last patch, plus that driver one. And then you need
to add a TODO to remove that union eventually.
Yes, with a single patch you have only one revert (but 194 files changed,
1264 insertions(+), 1296 deletions(-)) instead of two (one of them: 1
file changed, 9 insertions(+), 1 deletion(-); the other maybe a bit
bigger). (And maybe you get away with just reverting the last patch.)
With a single patch the TODO after a revert is "redo it all again (and
prepare for a different set of conflicts)" while with the split series
it's only "fix that one driver that was forgotten/borked" + reapply that
10 line patch. As the one who gets that TODO, I prefer the latter.
So in sum: If your metric is "small count of reverted commits", you're
right. If however your metric is: Better get 95% of this series' change
in than maybe 0%, the split series is the way to do it.
With me having spend ~3h on this series' changes, it's maybe
understandable that I did it the way I did.
FTR: This series was created on top of v6.5-rc1.
I think, this series should be able applied to drm-tip or drm-misc.
And should pass the compile test.
If you apply it to
drm-misc-next you get a (trivial) conflict in patch #2. If I consider to
be the responsible maintainer who applies this series, I like being able
to just do git am --skip then.
FTR#2: In drm-misc-next is a new driver
(drivers/gpu/drm/loongson/lsdc_crtc.c) so skipping the last patch for
now might indeed be a good idea.
No, that driver should be nuked together if this series is going to be
applied(after get acked-by and/or reviewed-by).
Please don't leave that driver alone there.
So I still like the split version better, but I'm open to a more
verbose reasoning from your side.
You're doing only one thing here, really: you change the name of a
structure field. If it was shared between multiple maintainers, then
sure, splitting that up is easier for everyone, but this will go through
drm-misc, so I can't see the benefit it brings.
I see your argument, but I think mine weights more.
Best regards
Uwe
