Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1530707 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 04/39] mtd: nand: denali: remove more unused struct members Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-11-26 19:10 +0100
    Re: [PATCH 04/39] mtd: nand: denali: remove more unused struct  members Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-11-27 16:20 +0100
      Re: [PATCH 04/39] mtd: nand: denali: remove more unused struct members Masahiro Yamada <yamada.masahiro@socionext.com> - 2016-11-30 08:20 +0100
        Re: [PATCH 04/39] mtd: nand: denali: remove more unused struct  members Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-11-30 08:50 +0100

#1530707 — [PATCH 04/39] mtd: nand: denali: remove more unused struct members

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 */
-- 
2.7.4

[toc] | [next] | [standalone]

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

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 */

[toc] | [prev] | [next] | [standalone]

#1533012

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web