Den 06.02.2017 16.53, skrev Daniel Vetter:
On Mon, Feb 06, 2017 at 12:08:47PM +0100, Thierry Reding wrote:
On Mon, Feb 06, 2017 at 11:07:42AM +0100, Daniel Vetter wrote:
On Mon, Feb 6, 2017 at 10:35 AM, Thierry Reding <thierry.reding@xxxxxxxxx>
wrote:
+EXPORT_SYMBOL(tinydrm_disable_backlight);
+#endif
These look like they really should be part of the backlight subsystem.
I
don't see anything DRM specific about them. Well, except for the error
messages.
So this is a bit an unpopular opinion with some folks, but I don't
require
anyone to submit new code to subsystems outside of drm for new drivers.
Simply because it takes months to get stuff landed, and in general it's
not worth the trouble.
"Not worth the trouble" is very subjective. If you look at the Linux
kernel in general, one of the reasons why it works so well is because
the changes we make apply to the kernel as a whole. Yes, sometimes that
makes things more difficult and time-consuming, but it also means that
the end result will be much more widely usable and therefore benefits
everyone else in return. In my opinion that's a large part of why the
kernel is so successful.
We have piles of stuff in drm and drm drivers that should be in core but
isn't.
Imo the only reasonable way is to merge as-is, then follow-up with a
patch
series to move the helper into the right subsystem. Most often
unfortunately that follow-up patch series will just die.
Of course follow-up series die. That's because nobody cares to follow-up
once their code has been merged.
Collecting our own helpers or variants of subsystems is a great way of
isolating ourselves from the rest of the community. I don't think that's
a good solution in the long run at all.
We have a bunch of patch series that we resubmit for months and they go
exactly nowhere. They don't die because we stop caring, they die because
they die. Some of them we even need to constantly rebase and carry around
in drm-tip since our CI would Oops or spew WARNIGs all over the place.
There's simply some areas of the kernel which seem overloaded under patches
and no one is willing or able to fix things, and I can't fix the entire
kernel. Nor expect contributors (who have much less political weight to
throw around than me) to do that and succeed. And we don't end up with
worse code in the drm subsystem, since we can still do the refactoring
within drm helpers and end up with clean drivers.
I fully agree that it's not great for the kernel's future, but when I'm
stuck with the option to get shit done or burning out playing the
upstreaming game, the choice is easy. And in the end I care about open
source gfx much more than the kernel, and I think for open source gfx's
success it's crucial that we're welcoming to new contributors and don't
throw up massive roadblocks. Open source gfx is tiny and still far away
from world domination, we need _lots_ more people. If that means routing
around other subsystems for them, I'm all for it.
I can't say I fully agree with that sentiment. I do see how routing
around subsystems can be useful occasionally. If nobody will merge the
code, or if nobody cares, then by all means, let's make them DRM-
specific helpers.
But I think we need to at least try to do the right thing. If only to
teach people what the right way is. If we start accepting such things
by default, how can we expect contributors to even try?
I also think that contributors will often end up contributing not only
to DRM but to the kernel as a whole. As such it should be part of our
mentoring to teach them about how the process works as a rule, even if
the occasional exception is necessary to get things done.
In this particular case, I know for a fact that both backlight and SPI
maintainers are very responsive, so that's not a good excuse.
I definitely don't want that we don't attempt this. But brought from years
of experience, I recommend to merge first (with pre-refactoring already
applied, but helpers only extracted, not yet at the right spot), and then
follow up with. Because on average, there's way too many trees with
overloaded maintainers who maybe look at your patch once per kernel
release cycle.
If you know that backlight and spi isn't one of these areas (anything that
goes through takashi/sound is a similar good experience for us on the i915
side), then I guess we can try. But then Noralf has already written a few
months worth of really great refactoring, and I'm seriously starting to
feel guilty for volunteering him for all of this. Even though he seems to
be really good at it, and seems to not mind, it's getting a bit silly.
Given that I'd say up to Noralf.
In short, there's always a balance.
Yes, it's a balance between the perfect and not's so perfect,
the professinal and the amateur, and the drm expert and the newbie.
If I knew how much time I would have to spend on tinydrm to get it
merged, then I would never have started it. I wondered, a couple of
months back, if I should just cut my losses and move to something else.
dri-devel is a friendly place and I do get help to keep moving, but I
get the feeling that drm is really a place for professionals that write
kernel code 50 hours a week. And there's nothing wrong in that, drm is
complex and maybe that kind of expertice and work-hours are needed to
do work here.
It's not that I plan on backing out now, I'm just putting down a few
words about the challenge it has been for me as a hobbyist to meet drm.
As for the specifics,
Backlight enable/disable helpers, I haven't thought about those as
backlight subsystem helpers. But I see some drm panel drivers that do
the same, so it makes sense.
But what I'm feeling is this:
If I'm expected to get everything right, then I will "never" get this
merged.
This thing by itself isn't much, but adding up every little thing
amounts to a lot. And it's not just "make this patch", it's probably
clean up the drivers as well :-)
tinydrm_spi_transfer() can print the content of the buffer. This is
important at least until I have seen some real use of the drivers.
I have emulation code for handling bit widths not supported by the SPI
controller, so I want to see what goes over the wire. SPI core uses
trace events to track what's going on, and I don't think I can get
buffer dumping into that code.
Don't get me wrong Thierry, I do appreciate that you take the time to
look at the code. I'm just frustrated that it takes so long to get this
right. I thought that all I needed now was a DT maintainer ack :-)
Noralf.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
