Re: [PATCH 04/39] mtd: nand: denali: remove more unused struct members

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Wed, 30 Nov 2016 16:16:35 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 
> 2016-11-28 0:12 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Sun, 27 Nov 2016 03:05:50 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> > Please add a description here.
> >
> > Also, this commit tends to validate my fears: you should have wait for
> > the full rework/cleanup to be done before submitting the first round of
> > cleanups. Indeed, commit c4ae0977f57d ("mtd: nand: denali: remove unused
> > struct member denali_nand_info::idx") was removing one of these unused
> > fields, leaving 2 of them behind.  
> 
> Right.
> No difference except that
> denali->idx was initialized to zero(, but not referenced).
> 
> I could squash the two patches.

That's not a big deal.

> 
> 
> > While I like when things I clearly separated in different commits, when
> > you push the logic too far, you end up with big series which are not
> > necessarily easier to review, and several commits that are achieving
> > the same goal...  
> 
> 
> I must admit that I hurried up in posting the first round.
> But, please note I did not ask you to pick it up for v4.10-rc1.
> After all, it was your choice whether you picked it soon or
> waited until you saw the big picture.
> You could have postponed it until v4.11-rc1 if you had wanted.

That's true. But it was not clear from your cover letter that you
were posting this series just to get feedback and not necessarily to
get them applied.

> 
> My idea was, I'd like to get feedback earlier
> (especially from Intel engineers).
> 
> I fear that I do not reveal anything until I complete my work.
> If I am doing wrong in the early patches in my big series,
> I might end up with lots of effort to turn around.
> 
> I dropped various Intel-specific things,
> for example commit c9e025843242 ("mtd: nand: denali: remove
> detect_partition_feature()")
> removed the whole function I do not understand.
> There was possibility that it might be locally used by Intel platforms.
> 
> If I had gotten negative comments for removal, I'd have needed more efforts
> to not break any old functions.
> 
> As a result, nobody was opposed to delete such things.
> So, I can confidently continue my work on cleaner and more *stable* base.
> 
> 

Okay, got it. So, I should not apply any patches from this series until
you've completed your rework (round2+3 posted).
I think I'll still apply patch 1 early, because it's not directly
related to the denali rework, and the fix looks good.

Regards,

Boris