| Message ID | 1480183585-592-5-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Superseded |
| Headers | show |
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.
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...
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> drivers/mtd/nand/denali.h | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
> index fd1ae08..4fad43b 100644
> --- a/drivers/mtd/nand/denali.h
> +++ b/drivers/mtd/nand/denali.h
> @@ -407,7 +407,6 @@ struct denali_nand_info {
> struct nand_buf buf;
> struct device *dev;
> int total_used_banks;
> - uint32_t block; /* stored for future use */
> uint16_t page;
> void __iomem *flash_reg; /* Mapped io reg base address */
> void __iomem *flash_mem; /* Mapped io reg base address */
> @@ -416,7 +415,6 @@ struct denali_nand_info {
> struct completion complete;
> spinlock_t irq_lock;
> uint32_t irq_status;
> - int irq_debug_array[32];
> int irq;
>
> uint32_t devnum; /* represent how many nands connected */
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. > 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. 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. -- Best Regards Masahiro Yamada
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
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h index fd1ae08..4fad43b 100644 --- a/drivers/mtd/nand/denali.h +++ b/drivers/mtd/nand/denali.h @@ -407,7 +407,6 @@ struct denali_nand_info { struct nand_buf buf; struct device *dev; int total_used_banks; - uint32_t block; /* stored for future use */ uint16_t page; void __iomem *flash_reg; /* Mapped io reg base address */ void __iomem *flash_mem; /* Mapped io reg base address */ @@ -416,7 +415,6 @@ struct denali_nand_info { struct completion complete; spinlock_t irq_lock; uint32_t irq_status; - int irq_debug_array[32]; int irq; uint32_t devnum; /* represent how many nands connected */
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- drivers/mtd/nand/denali.h | 2 -- 1 file changed, 2 deletions(-) -- 2.7.4