Message-ID: <20080324020832.GA13935@linux-sh.org>
Date:	Mon, 24 Mar 2008 11:08:32 +0900
From:	Paul Mundt <lethal@...ux-sh.org>
To:	Adrian McMenamin <adrian@...golddream.dyndns.info>,
	J??rn Engel <joern@...fs.org>
Cc:	dwmw2 <dwmw2@...radead.org>, Andrew Morton <akpm@...l.org>,
	linux-sh <linux-sh@...r.kernel.org>, Greg KH <greg@...ah.com>,
	LKML <linux-kernel@...r.kernel.org>,
	MTD <linux-mtd@...ts.infradead.org>
Subject: Re: [PATCH] 2/3 mtd: add support for flash on the SEGA Dreamcast Visual Memory Unit

On Sat, Mar 22, 2008 at 06:39:07PM +0000, Adrian McMenamin wrote:
> On Sat, 2008-03-22 at 19:32 +0100, J??rn Engel wrote:
> > Without a doubt, buffering is useful.  However I question how useful it
> > is to implement this in individual device drivers instead of once in
> > mtd_core.c.
> 
> Well, it's obviously useful to this device. Are you making best the
> enemy of better?
> 
Adrian, relax. These are helpful comments, and you should be taking them
under consideration even if you have no immediate plans to implement them
in the current driver.

Buffering is useful for this device, but the suggestion is that it's a
useful capability to have for other parts also, and so it should be done
in the generic layer. Doing this requires a bit more work on your part,
but it's in no way removing the ability to do buffering in the VMU MTD
driver (the logic is just moved up a level). You seem to be implying
that the options are having buffering in the driver as it is today, or
not at all, which is a position that doesn't even factor in the initial
comment. This is not helpful.

While it's certainly possible to do all of your work off in your own
corner, it tends to be detrimental to the subsystem if you have stuff
that can and should be done generically. Likewise, it's a give and take,
if you can't be bothered trying to do this incrementally in the subsystem
itself now, there's not much motivation for you to do so once the driver
has been merged either.

> > Given that you have ignored most of my previous comments, NAK. 
> 
> Not good enough, frankly. What comments have I ignored? I didn't
> implement your suggestion that a void* memory type become be32* because
> it was totally inappropriate for a type that is passed in both be32 and
> le32 and as u8.
> 
While I haven't seen the previous comments, the implication seems to be
that there are more than one outstanding.

> >  I don't mind merging code that isn't up to our standards yet.  But I
> >  have a bad feeling about a maintainer that does not understand
> >  review comments.  Since you had similar problems understanding
> >  Andrew, part of the blame may sit on your side.
> 
> I'm sorry that you feel that way, but as you took the hump when I said
> that this:
> 
> "Possibly the big-endian annotations need to trickly though the layers
> here as well."
> 
> Isn't good english (and it's not) and asked you - twice - to explain
> what you meant. I cannot accept your summary.
> 
Just as there's no motivation for others to accept merging your code. You
really need to work on handling feedback in a less oppositional way if
you ever want to have your changes merged.

As far as "good" english goes, this is l-k, hang your degree at the
door and get off your high horse. Even the native speakers perform
horrendous atrocities against the english language, that's life, deal
with it. In technical discussions this doesn't tend to matter at all, as
long as the meaning is obvious -- which in this case it certainly seems
to be. If you're speaking a different language, perhaps it's a
miscommunication or misinterpretation of technical terms rather than
anything else. Do not automatically assume the blame lies with people who
are trying to help you.

On Sat, Mar 22, 2008 at 07:56:01PM +0100, J??rn Engel wrote:
> Your argument would make sense if referring to a transport layer where
> some structure is just passed along as payload data.  But you are
> dereferencing a structure.  And instead of declaring a structure, you
> still do this:
> 	((unsigned long *)sendbuf)[0] = cpu_to_be32(MAPLE_FUNC_MEMCARD);
> 	((unsigned long *)sendbuf)[1] = cpu_to_be32(partition << 24 | num);
> 
> If that is a valid description of "appropriate" in your dictionary then
> we clearly don't speak the same language.  Even your own suggestion to
> change the cast to (u32*) would have been an improvement.
> 
Agreed, this stuff is very much a mess, and making the structure more
explicit would help quite a bit in terms of readability. This is legacy
stuff that's been hanging around for close to a decade now, it would be
nice to see this cleaned up and looking more like code.

> I tried to help you by a) reviewing the code and b) even converting the
> function myself after deciding that you don't understand what I meant.  
> Clearly this didn't work.  And maybe the problem is me, either because
> my English is insufficient or I am a d***head.  Quite possible.
> 
> But even if I am the problem, you should be able to understand
> somebody's review comments and act accordingly.  If not, I have a really
> bad feeling about this.  And I hope someone else has more patience than
> me, because this code still needs work.
> 
I've hit the same issues with Adrian's patches in the past, but I've
weighed this against a couple of factors:

	- Is it reasonably isolated.
	- Does it break anything else.
	- Is it more likely to get pointed at and fixed in-tree or out.

The merging of less-than-desirable code is certainly a factor here, but
it's incredibly unlikely that any of this code would get cleaned up if it
hadn't been merged. This has certainly been the case for maple, and the
related peripherals. It still has a long way to go, but it's in much
better shape these days. Adrian has done some good work in this area,
even if his bizarre oppositional development model flies in the face of
all conventional wisdom. It's slow progress, but I wouldn't have merged
the maple stuff at all if it didn't seem liks the pros outweighed the
cons. Most of the rest of us that were working in this area 7-8 years ago
have neither the time or inclination to do anything with it today, so
without these bits getting merged, it's unlikely they ever will. The fact
there's still someone hacking on this stuff today is a good indicator
that he'll be around in the future to keep it running also, hopefully
figuring out how to work with subsystem maintainers and folks doing
thankless code review in the process.

With that said, the current code obviously needs quite a bit of work
before it can be merged. I'm obviously coming in to this thread late, so
it would be helpful to have a pointer to the initial review comments. The
comments made by Andrew and myself on the maple bits I assume are being
worked on and will be respun at some point. I'll attempt to work with
Adrian on getting the rest of these issues addressed.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists