Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

+ akpm

On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote:
> 
> > 
> > This patch
> > - updates DT binding for selection of ecc-scheme
> > - updates DT binding for detection of ELM h/w engine
> > - removes following obselete ECC schemes
> > 	OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming
> > ECC)
> > 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit
> > Hamming ECC scheme)
> > - updates DT binding documentation for mtd/gpmc-nand
> > 
> > Signed-off-by: Pekon Gupta <pekon@ti.com>
> > ---
> 
> Dear Olof and other DT Maintainers,
> 
> This patch series has missed multiple merge windows, and
> much of the other development work on mtd/nand/omap
> driver is gated due to this.
> So, request you to please review this patch set and Ack it
> if all your mentioned concerns are resolved.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Brian Norris]

On Wed, Sep 25, 2013 at 08:46:19AM -0500, Felipe Balbi wrote:
> + akpm
> 
> On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote:
> > 
> > > 
> > > This patch
> > > - updates DT binding for selection of ecc-scheme
> > > - updates DT binding for detection of ELM h/w engine
> > > - removes following obselete ECC schemes
> > > 	OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming
> > > ECC)
> > > 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit
> > > Hamming ECC scheme)
> > > - updates DT binding documentation for mtd/gpmc-nand
> > > 
> > > Signed-off-by: Pekon Gupta <pekon@ti.com>
> > > ---
> > 
> > Dear Olof and other DT Maintainers,
> > 
> > This patch series has missed multiple merge windows, and
> > much of the other development work on mtd/nand/omap
> > driver is gated due to this.
> > So, request you to please review this patch set and Ack it
> > if all your mentioned concerns are resolved.

I've been waiting on Olof's response for this one. It seems like you
addressed his comments well enough for me, although (in line with his
comments) you are admittedly still breaking the DT ABI for these
devices. IMO, that's OK given the low quality of the original bindings.

I will make my own last pass at this series and push it to l2-mtd.git if
it looks OK.

Brian

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Brian Norris]

Hi Pekon,

On Wed, Sep 25, 2013 at 08:46:19AM -0500, Felipe Balbi wrote:
> + akpm
> 
> On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote:
> > 
> > > 
> > > This patch
> > > - updates DT binding for selection of ecc-scheme
> > > - updates DT binding for detection of ELM h/w engine
> > > - removes following obselete ECC schemes
> > > 	OMAP_ECC_HAMMING_CODE_DEFAULT (S/W based 1-bit Hamming
> > > ECC)
> > > 	OMAP_ECC_HAMMING_CODE_HW_ROMCODE (H/W based 1-bit
> > > Hamming ECC scheme)
> > > - updates DT binding documentation for mtd/gpmc-nand
> > > 
> > > Signed-off-by: Pekon Gupta <pekon@ti.com>
> > > ---
> > 
> > Dear Olof and other DT Maintainers,
> > 
> > This patch series has missed multiple merge windows, and
> > much of the other development work on mtd/nand/omap
> > driver is gated due to this.

Also, to be fair here: you only started CC'ing the appropriate DT
people/list around v4, after which you got a (somewhat) prompt and
thorough review of the DT bindings. Then several months passed before
you addressed the reviews. So the "multiple merge windows" is not
entirely to be blamed on others ;)

Brian

RE: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Gupta, Pekon]

Hi Brian,

> 
> Hi Pekon,
> 
> On Wed, Sep 25, 2013 at 08:46:19AM -0500, Felipe Balbi wrote:
> > + akpm
> >
> > On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote:
[snip]
> > >
> > > Dear Olof and other DT Maintainers,
> > >
> > > This patch series has missed multiple merge windows, and
> > > much of the other development work on mtd/nand/omap
> > > driver is gated due to this.
> 
> Also, to be fair here: you only started CC'ing the appropriate DT
> people/list around v4, after which you got a (somewhat) prompt and
> thorough review of the DT bindings. Then several months passed before
> you addressed the reviews. So the "multiple merge windows" is not
> entirely to be blamed on others ;)
> 
> Brian

Few points I would like to clarify here, *without* pointing anyone..

(1) It was Olof's comments which directed me to cc: devicetree-discuss list.
So wrong list was always cc-ed till v4.
Refer http://permalink.gmane.org/gmane.linux.ports.arm.kernel/249662

(2) The devicetree list has been updated in MAINTAINER file towards
end of July (22/July/2013) in following commit. Whereas the patch v4
was submitted on 2/July/2013. So I wasn't aware of this new DT maillist.
commit d0fb18c5c0caf2ed0eecf3d0145450ae708ed75a
Commit:     Grant Likely <grant.likely@linaro.org>
CommitDate: 2013-07-22

(3) When a maintainer gives a NAK, I expect him to at-least give
directions on what to change in the patch. There were no comments
given, neither new patch reviewed by the DT maintainer even after
sending multiple request directly.
http://lists.infradead.org/pipermail/linux-mtd/2013-July/047441.html
http://lists.infradead.org/pipermail/linux-mtd/2013-July/047629.html

Its only when Artem and yourself pitched in by sending a mail to
new DT mail-list that the DT maintainer reviewed and provided the
comments for fixes.
But by that time 3.11-rc6 had already gone past, and so I knew this
Series cannot go in, so I wanted to wait some more, to see if there
were any more comments on this. And frankly I was too much
frustrated by then.

Just to speak my opinion.
We all understand that maintainers are heavily loaded by tons of
emails, And reviewing each patch instantly is not possible.

But this load is now passing to developers like me as frustration,
because most of energy is being spent in re-submitting patches
again and again, without any proper direction or conclusion.
And then there is no time and energy left for good work, where
we can contribute to optimizing frame-works for performance or
adding new features.
So I see many good developers distracting away from mainline.

Thus, there should be a mechanism, where such load can be
distributed, and there are less emails. And developers don't
have to re-submit patch multiple times, by collecting most reviews
at once.  This way developer's like me can spend more time in doing
other constructive things.

Please consider this as constructive feedback, without targeting
any individual or team.


with regards, pekon

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Brian Norris]

On Wed, Sep 25, 2013 at 07:24:26PM +0000, Gupta, Pekon wrote:
> > On Wed, Sep 25, 2013 at 08:46:19AM -0500, Felipe Balbi wrote:
> > > + akpm
> > >
> > > On Tue, Sep 24, 2013 at 01:04:05PM -0500, Gupta, Pekon wrote:
> [snip]
> > > >
> > > > Dear Olof and other DT Maintainers,
> > > >
> > > > This patch series has missed multiple merge windows, and
> > > > much of the other development work on mtd/nand/omap
> > > > driver is gated due to this.
> > 
> > Also, to be fair here: you only started CC'ing the appropriate DT
> > people/list around v4, after which you got a (somewhat) prompt and
> > thorough review of the DT bindings. Then several months passed before
> > you addressed the reviews. So the "multiple merge windows" is not
> > entirely to be blamed on others ;)
> > 
> > Brian
> 
> Few points I would like to clarify here, *without* pointing anyone..
> 
> (1) It was Olof's comments which directed me to cc: devicetree-discuss list.
> So wrong list was always cc-ed till v4.
> Refer http://permalink.gmane.org/gmane.linux.ports.arm.kernel/249662
> 
> (2) The devicetree list has been updated in MAINTAINER file towards
> end of July (22/July/2013) in following commit. Whereas the patch v4
> was submitted on 2/July/2013. So I wasn't aware of this new DT maillist.
> commit d0fb18c5c0caf2ed0eecf3d0145450ae708ed75a
> Commit:     Grant Likely <grant.likely@linaro.org>
> CommitDate: 2013-07-22

Did you not notice the 'bounce' emails once Grant made the (IMO unwise)
decision to shut down the DT mailing list instead of just redirecting?

> (3) When a maintainer gives a NAK, I expect him to at-least give
> directions on what to change in the patch. There were no comments
> given, neither new patch reviewed by the DT maintainer even after
> sending multiple request directly.
> http://lists.infradead.org/pipermail/linux-mtd/2013-July/047441.html
> http://lists.infradead.org/pipermail/linux-mtd/2013-July/047629.html

Those are links to your own emails, not to any NAKs. Any NAKs I saw
listed reasons. I NAK'd based on the ABI breakage and lack of review by
the DT maintainership; Arnd NAK'd based on similar reasons; Olof later
(once you got the right list) gave constructive criticism on how to
remove the Linux-isms and other software implementation details from the
DT binding.

> Its only when Artem and yourself pitched in by sending a mail to
> new DT mail-list that the DT maintainer reviewed and provided the
> comments for fixes.
> But by that time 3.11-rc6 had already gone past, and so I knew this
> Series cannot go in, so I wanted to wait some more, to see if there
> were any more comments on this. And frankly I was too much
> frustrated by then.
> 
> Just to speak my opinion.
> We all understand that maintainers are heavily loaded by tons of
> emails, And reviewing each patch instantly is not possible.
> 
> But this load is now passing to developers like me as frustration,
> because most of energy is being spent in re-submitting patches
> again and again, without any proper direction or conclusion.
> And then there is no time and energy left for good work, where
> we can contribute to optimizing frame-works for performance or
> adding new features.
> So I see many good developers distracting away from mainline.

I sincerely hope that this isn't (or at least doesn't continue to be)
the norm.

> Thus, there should be a mechanism, where such load can be
> distributed, and there are less emails. And developers don't
> have to re-submit patch multiple times, by collecting most reviews
> at once.  This way developer's like me can spend more time in doing
> other constructive things.

I understand your concerns. I have been frustrated with slow responses
on MTD stuff as well, which is why I'm stepping in to review and commit
more. But I see that your patch hit a unique combination of events that
helped slow things down even more than usual.

MTD maintainership has been slowing down for a while, and in the
midst of your patch series, I began stepping in to take care of some of
that load. I didn't quite have a good picture of what patches were
pending without comments at that point, so patch series like yours were
in limbo with no one to review.

Additionally, Grant stepped down from DT maintainership and
correspondingly shut down and transitioned the DT mailing list. This
directly affected your patch series.

Lastly, it is extra difficult to deal with patch sets like this that
cross subsystems and require reviews from multiple constituencies. And
that is even more difficult when you aren't even emailing the correct
people (for whatever reason).

I believe that many of the factors that slowed down your particular
series have been ameliorated.

Time has allowed developers to become more aware of the change in DT
maintainership and mailing list, and I can more promptly redirect
developers who are still CC'ing the wrong people/lists.

I am able to spend a little more time on MTD stuff, to help push
development along a little faster.

Olof has given good advice on your DT binding and has (slowly) been
responding to other requests for DT review that make it to his list. I
see that he hasn't followed up on your changes (this v6), so pinging him
(as you did) is probably the correct approach. But please do recognize
that the DT list is very high volume, so it's hard to get good timely
responses there.

Anyway, at this point I think your patch series should be nearly
complete. I made a few comments on your patches, and I'd imagine you
only should need one more revision (v7) before I can accept it to the
l2-mtd.git tree.

> Please consider this as constructive feedback, without targeting
> any individual or team.

Thank you for your thoughtful response.

Brian

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Olof Johansson]

On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris
<computersforpeace@gmail.com> wrote:

> Olof has given good advice on your DT binding and has (slowly) been
> responding to other requests for DT review that make it to his list. I
> see that he hasn't followed up on your changes (this v6), so pinging him
> (as you did) is probably the correct approach. But please do recognize
> that the DT list is very high volume, so it's hard to get good timely
> responses there.

I am not a DT mainainer, but sometimes when I see a binding that
appears to be wrong I speak up. In this case, the binding was one of
those.

That doesn't mean you should necessarily rely on me for the rest of
the review, there are several dedicated maintainers right now that
should be able to spread the load across them, and it is their ack you
should seek on the binding, not mine.

That being said: this latest version looks good to me. See how much
simpler the binding got when you stopped trying to describe driver
behavior and focused on describing hardware? That's the way we want it
to look like.


So, I have no more objections to it, and I hope you can get a quick
review from a DT maintainer on the rest of the binding.


-Olof

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Brian Norris]

On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote:
> On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> 
> > Olof has given good advice on your DT binding and has (slowly) been
> > responding to other requests for DT review that make it to his list. I
> > see that he hasn't followed up on your changes (this v6), so pinging him
> > (as you did) is probably the correct approach. But please do recognize
> > that the DT list is very high volume, so it's hard to get good timely
> > responses there.
> 
> I am not a DT mainainer, but sometimes when I see a binding that
> appears to be wrong I speak up. In this case, the binding was one of
> those.

Whoops, my bad. I was deceived by the responses I've seen from you on
other issues (thanks, BTW). In that case, I haven't seen any response
from a proper DT binding maintainer :(
 
> So, I have no more objections to it, and I hope you can get a quick
> review from a DT maintainer on the rest of the binding.

At this point, I'm comfortable going ahead without their ack, since they
obviously don't care/don't have the manpower to review.

Brian

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Olof Johansson]

On Wed, Sep 25, 2013 at 2:29 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote:
>> On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris
>> <computersforpeace@gmail.com> wrote:
>>
>> > Olof has given good advice on your DT binding and has (slowly) been
>> > responding to other requests for DT review that make it to his list. I
>> > see that he hasn't followed up on your changes (this v6), so pinging him
>> > (as you did) is probably the correct approach. But please do recognize
>> > that the DT list is very high volume, so it's hard to get good timely
>> > responses there.
>>
>> I am not a DT mainainer, but sometimes when I see a binding that
>> appears to be wrong I speak up. In this case, the binding was one of
>> those.
>
> Whoops, my bad. I was deceived by the responses I've seen from you on
> other issues (thanks, BTW). In that case, I haven't seen any response
> from a proper DT binding maintainer :(
>
>> So, I have no more objections to it, and I hope you can get a quick
>> review from a DT maintainer on the rest of the binding.
>
> At this point, I'm comfortable going ahead without their ack, since they
> obviously don't care/don't have the manpower to review.

No, that is not how we handle device tree bindings. They need to be
reviewed, since we are moving over to a model where they will be
considered ABI and can't be changed after the fact. We have a long
backlog of mostly-unreviewed old bindings that we're going to do a
pass through and then lock down, but it would be good to not add to
that backlog with newer bindings.

In other words, there's a strong desire for actual acks on bindings
from those maintainers these days.


-Olof

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Brian Norris]

On Wed, Sep 25, 2013 at 2:32 PM, Olof Johansson <olof@lixom.net> wrote:
> On Wed, Sep 25, 2013 at 2:29 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
>> On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote:
>>> On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris
>>> <computersforpeace@gmail.com> wrote:
>>>
>>> > Olof has given good advice on your DT binding and has (slowly) been
>>> > responding to other requests for DT review that make it to his list. I
>>> > see that he hasn't followed up on your changes (this v6), so pinging him
>>> > (as you did) is probably the correct approach. But please do recognize
>>> > that the DT list is very high volume, so it's hard to get good timely
>>> > responses there.
>>>
>>> I am not a DT mainainer, but sometimes when I see a binding that
>>> appears to be wrong I speak up. In this case, the binding was one of
>>> those.
>>
>> Whoops, my bad. I was deceived by the responses I've seen from you on
>> other issues (thanks, BTW). In that case, I haven't seen any response
>> from a proper DT binding maintainer :(
>>
>>> So, I have no more objections to it, and I hope you can get a quick
>>> review from a DT maintainer on the rest of the binding.
>>
>> At this point, I'm comfortable going ahead without their ack, since they
>> obviously don't care/don't have the manpower to review.
>
> No, that is not how we handle device tree bindings. They need to be
> reviewed, since we are moving over to a model where they will be
> considered ABI and can't be changed after the fact. We have a long
> backlog of mostly-unreviewed old bindings that we're going to do a
> pass through and then lock down, but it would be good to not add to
> that backlog with newer bindings.
>
> In other words, there's a strong desire for actual acks on bindings
> from those maintainers these days.

This only works if we get a response. I'll repeat this fact: I have
seen absolutely zero response from any DT maintainer regarding this
binding, and they've been CC'd in some capacity since July:

Old revision from July (cross-posted, including the old DT list):
http://thread.gmane.org/gmane.linux.ports.arm.omap/100484/

New list:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg95238.html

All official DT binding maintainers are CC'd here: you can't say you
want more review of bindings, yet fail to review them across 3
versions and almost 3 months. Ball's in your court.

Brian

Re: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Mark Rutland]

On Wed, Sep 25, 2013 at 10:29:03PM +0100, Brian Norris wrote:
> On Wed, Sep 25, 2013 at 01:33:27PM -0700, Olof Johansson wrote:
> > On Wed, Sep 25, 2013 at 1:05 PM, Brian Norris
> > <computersforpeace@gmail.com> wrote:
> > 
> > > Olof has given good advice on your DT binding and has (slowly) been
> > > responding to other requests for DT review that make it to his list. I
> > > see that he hasn't followed up on your changes (this v6), so pinging him
> > > (as you did) is probably the correct approach. But please do recognize
> > > that the DT list is very high volume, so it's hard to get good timely
> > > responses there.
> > 
> > I am not a DT mainainer, but sometimes when I see a binding that
> > appears to be wrong I speak up. In this case, the binding was one of
> > those.
> 
> Whoops, my bad. I was deceived by the responses I've seen from you on
> other issues (thanks, BTW). In that case, I haven't seen any response
> from a proper DT binding maintainer :(

Hmm... this is the first email in this thread I've received, and I don't
have older postings either. It looks like I must have cocked up
subscribing to the devicetree list, but as I was CC'd directly on many
patches I hadn't noticed.

As far as I can see I wasn't CC'd directly on any version of this
series, which would have helped to highlight the series as needing
review.

Apologies for that. I've attempted to correct the problem. Hopefully
I've got this right and mails are not being silently dropped somewhere.

Pekon, could you please re-send this version of the patches?

Cheers,
Mark.

>  
> > So, I have no more objections to it, and I hope you can get a quick
> > review from a DT maintainer on the rest of the binding.
> 
> At this point, I'm comfortable going ahead without their ack, since they
> obviously don't care/don't have the manpower to review.
> 
> Brian
> 

RE: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Gupta, Pekon]

Hi Mark,

> 
> Pekon, could you please re-send this version of the patches?
> 

As already there are feedbacks on the patches, so re-sending the
Patch series might clutter someone else's mailbox.
Will it be possible for you to fetch the patches from MTD archives?
else I would send you the patches separately..

I'm attaching the URL from MTD archives for each patch separately,
and you can follow that thread to see existing comments.

[PATCH v6 0/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048613.html

[PATCH v6 1/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048611.html

[PATCH v6 2/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048612.html

[PATCH v6 3/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048615.html

[PATCH v6 3/4] http://lists.infradead.org/pipermail/linux-mtd/2013-September/048614.html


with regards, pekon

RE: [PATCH v6 1/4] ARM: OMAP2+: cleaned-up DT support of various ECC schemes

[Gupta, Pekon]

> 
> Anyway, at this point I think your patch series should be nearly
> complete. I made a few comments on your patches, and I'd imagine you
> only should need one more revision (v7) before I can accept it to the
> l2-mtd.git tree.
> 
Yes surely I will send next version (v7), but it might take few days.
As some more feedbacks on [PATCH 1] are pending for final conclusion
and this needs to be properly re-tested.

And thanks much to you and Olof for the feedbacks.
I agree some of Olof's feedbacks on DT bindings gave me new view to
look at things, And further simplified the NAND driver code.

with regards, pekon