Re: [PATCH v2 00/25] mtd: nand: refactor the NAND subsystem (part 1)

[Boris Brezillon]

Hi Brian,

On Tue, 8 Dec 2015 16:36:24 -0800
Brian Norris  wrote:

> Hi,
> 
> On Tue, Dec 01, 2015 at 12:02:57PM +0100, Boris Brezillon wrote:
> > Hello,
> > 
> > This huge series aims at clarifying the relationship between the mtd and
> > nand_chip structures and hiding NAND framework internals to NAND
> > controller drivers.
> > 
> > The first part of the series provide an mtd_to_nand() helper to hide the
> > way mtd and nand_chip are linked together.
> > 
> > The second part of the series embeds the mtd structure into the nand_chip
> > one so that NAND controller drivers don't have to bother allocating the
> > MTD device and linking it with the NAND chip.
> > 
> > The last part of the series hides accesses to the chip->priv field behind
> > two helper functions.
> > 
> > This allows removal of some of the boilerplate code done in all NAND
> > controller drivers, but most importantly, it unifies a bit the way NAND
> > chip structures are instantiated (even though we still have two different
> > kinds of drivers: those embedding the nand_chip struct into their private
> > nand chip representation, and those allocating two different structures
> > and linking them together with the chip->priv field).
> > 
> > As said in the title, this refactoring is only the first step. I plan to
> > rework the NAND controller / NAND chip separation for pretty much the same
> > reasons: clarifying the separation between the two concepts, and getting
> > rid of more boilerplate code in NAND controller drivers.
> > 
> > Stay tuned ;-).
> > 
> > Best Regards,
> > 
> > Boris
> > 
> > Changes since v1:
> > - dropped already applied patches
> > - fixed some typos
> > - manually fixed some modifications omitted by the coccinelle scripts
> > - manually reworked modifactions done by coccinelle scripts to improve
> >   readability and fix coding style issues
> > 
> > Boris Brezillon (25):
> >   ARM: nand: make use of mtd_to_nand() where appropriate
> 
> I've sent this (+ the introduction of mtd_to_nand()) in a pull request
> to arm-soc, as well as to l2-mtd.git.
> 
> >   blackfin: nand: make use of mtd_to_nand() where appropriate
> >   cris: nand: make use of mtd_to_nand() where appropriate
> >   mips: nand: make use of mtd_to_nand() where appropriate
> 
> I've based these on v4.4-rc1 and brought them into l2-mtd.git, in case
> any arch/{blackfin,cris,mips}/ people want to pull them in too.
> 
> >   sh: nand: make use of mtd_to_nand() where appropriate
> >   mtd: nand: make use of mtd_to_nand() in NAND core code
> >   mtd: nand: make use of mtd_to_nand() in NAND drivers
> >   staging: mt29f_spinand: make use of mtd_to_nand()
> >   mtd: nand: embed an mtd_info structure into nand_chip
> >   mtd: nand: add nand_to_mtd() helper
> 
> Pulled the rest up to here into l2-mtd.git.

Okay, thanks.

> 
> >   coccinelle: nand: detect and correct drivers embedding an mtd_info
> > object
> 
> I believe Julia had comments on this. That probably would go through the
> kbuild tree in the end anyway (?).

Julia proposed to generate this script using sgen, so I guess this will
come later through a different tree.

> 
> >   mtd: nand: use the mtd instance embedded in struct nand_chip
> 
> There's still at least one problem in this patch (commented on the
> patch). It touches a lot of drivers, so any extra review would be great.

Yep, I think I'll resend the series and split the modification so that
you can pick changes independently (as you suggested a few days ago).

Anyway, reviews from other people would be much appreciated. As I said,
most of changes have been automated with coccinelle, but some of them
have been made manually done, and this is the case for all drivers using
the following pattern:

var = kzalloc(sizeof(struct mtd_info) +
  sizeof(struct nand_chip) + ... ,...);

because I failed to find a pattern common enough to match the cases I
had, and manually fixing them was easier for me...

> 
> >   mtd: nand: update the documentation to reflect framework changes
> >   staging: mt29f_spinand: use the mtd instance embedded in struct
> > nand_chip
> >   cris: nand: use the mtd instance embedded in struct nand_chip
> >   mtd: nand: update mtd_to_nand()
> >   mtd: nand: remove useless mtd->priv = chip assignments
> >   cris: nand: remove useless mtd->priv = chip assignments
> >   staging: mt29f_spinand: remove useless mtd->priv = chip assignment
> >   mtd: nand: simplify nand_dt_init() usage
> >   mtd: nand: kill the chip->flash_node field
> >   mtd: nand: add helpers to access ->priv
> >   ARM: make use of nand_set/get_controller_data() helpers
> >   mtd: nand: make use of nand_set/get_controller_data() helpers
> >   staging: mt29f_spinand: make use of nand_set/get_controller_data()
> > helpers
> 
> All the rest look good to me. I'll wait until I get a good copy of patch
> 12 before taking them.

Sure.

Thanks,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel 

Re: [PATCH v2 00/25] mtd: nand: refactor the NAND subsystem (part 1)

[Brian Norris]

Hi,

On Tue, Dec 01, 2015 at 12:02:57PM +0100, Boris Brezillon wrote:
> Hello,
> 
> This huge series aims at clarifying the relationship between the mtd and
> nand_chip structures and hiding NAND framework internals to NAND
> controller drivers.
> 
> The first part of the series provide an mtd_to_nand() helper to hide the
> way mtd and nand_chip are linked together.
> 
> The second part of the series embeds the mtd structure into the nand_chip
> one so that NAND controller drivers don't have to bother allocating the
> MTD device and linking it with the NAND chip.
> 
> The last part of the series hides accesses to the chip->priv field behind
> two helper functions.
> 
> This allows removal of some of the boilerplate code done in all NAND
> controller drivers, but most importantly, it unifies a bit the way NAND
> chip structures are instantiated (even though we still have two different
> kinds of drivers: those embedding the nand_chip struct into their private
> nand chip representation, and those allocating two different structures
> and linking them together with the chip->priv field).
> 
> As said in the title, this refactoring is only the first step. I plan to
> rework the NAND controller / NAND chip separation for pretty much the same
> reasons: clarifying the separation between the two concepts, and getting
> rid of more boilerplate code in NAND controller drivers.
> 
> Stay tuned ;-).
> 
> Best Regards,
> 
> Boris
> 
> Changes since v1:
> - dropped already applied patches
> - fixed some typos
> - manually fixed some modifications omitted by the coccinelle scripts
> - manually reworked modifactions done by coccinelle scripts to improve
>   readability and fix coding style issues
> 
> Boris Brezillon (25):
>   ARM: nand: make use of mtd_to_nand() where appropriate

I've sent this (+ the introduction of mtd_to_nand()) in a pull request
to arm-soc, as well as to l2-mtd.git.

>   blackfin: nand: make use of mtd_to_nand() where appropriate
>   cris: nand: make use of mtd_to_nand() where appropriate
>   mips: nand: make use of mtd_to_nand() where appropriate

I've based these on v4.4-rc1 and brought them into l2-mtd.git, in case
any arch/{blackfin,cris,mips}/ people want to pull them in too.

>   sh: nand: make use of mtd_to_nand() where appropriate
>   mtd: nand: make use of mtd_to_nand() in NAND core code
>   mtd: nand: make use of mtd_to_nand() in NAND drivers
>   staging: mt29f_spinand: make use of mtd_to_nand()
>   mtd: nand: embed an mtd_info structure into nand_chip
>   mtd: nand: add nand_to_mtd() helper

Pulled the rest up to here into l2-mtd.git.

>   coccinelle: nand: detect and correct drivers embedding an mtd_info
> object

I believe Julia had comments on this. That probably would go through the
kbuild tree in the end anyway (?).

>   mtd: nand: use the mtd instance embedded in struct nand_chip

There's still at least one problem in this patch (commented on the
patch). It touches a lot of drivers, so any extra review would be great.

>   mtd: nand: update the documentation to reflect framework changes
>   staging: mt29f_spinand: use the mtd instance embedded in struct
> nand_chip
>   cris: nand: use the mtd instance embedded in struct nand_chip
>   mtd: nand: update mtd_to_nand()
>   mtd: nand: remove useless mtd->priv = chip assignments
>   cris: nand: remove useless mtd->priv = chip assignments
>   staging: mt29f_spinand: remove useless mtd->priv = chip assignment
>   mtd: nand: simplify nand_dt_init() usage
>   mtd: nand: kill the chip->flash_node field
>   mtd: nand: add helpers to access ->priv
>   ARM: make use of nand_set/get_controller_data() helpers
>   mtd: nand: make use of nand_set/get_controller_data() helpers
>   staging: mt29f_spinand: make use of nand_set/get_controller_data()
> helpers

All the rest look good to me. I'll wait until I get a good copy of patch
12 before taking them.

Brian

>  Documentation/DocBook/mtdnand.tmpl |  31 +++---
>  arch/arm/mach-ep93xx/snappercl15.c |   4 +-
>  arch/arm/mach-ep93xx/ts72xx.c  |   4 +-
>  arch/arm/mach-imx/mach-qong.c  |   2 +-
>  arch/arm/mach-ixp4xx/ixdp425-setup.c   |   6 +-
>  arch/arm/mach-omap1/board-nand.c   |   2 +-
>  arch/arm/mach-orion5x/ts78xx-setup.c   |   6 +-
>  arch/arm/mach-pxa/balloon3.c   |   2 +-
>  arch/arm/mach-pxa/em-x270.c|   2 +-
>  arch/arm/mach-pxa/palmtx.c |   2 +-
>  arch/blackfin/mach-bf537/boards/stamp.c|   2 +-
>  arch/blackfin/mach-bf561/boards/acvilon.c  |   2 +-
>  arch/cris/arch-v32/drivers/mach-a3/nandflash.c |   8 +-
>  arch/cris/arch-v32/drivers/mach-fs/nandflash.c |   8 +-
>  arch/mips/alchemy/devboards/db1200.c   |   2 +-
>  arch/mips/alchemy/devboards/db1300.c   |   2 +-
>  arch/mips/alchemy/devboards/db1550.c   |   2 +-
>  arch/mips/pnx833x/common/platform.c|   2 +-
>  arch/mips/rb532/devices.c